Skip to content

Commit 018d45f

Browse files
nilmergnilmerg
committed
Transform custom variable filters as late as possible
fixes #865
1 parent 91827cb commit 018d45f

File tree

6 files changed

+214
-12
lines changed

6 files changed

+214
-12
lines changed

‎doc/02-Installation.md.d/From-Source.md

Lines changed: 1 addition & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -11,7 +11,7 @@ Make sure you use `icingadb` as the module name. The following requirements must
1111
* The following PHP modules must be installed: `cURL`, `dom`, `json`, `libxml`
1212
* [Icinga DB](https://github.com/Icinga/icingadb)
1313
* [Icinga Web 2](https://github.com/Icinga/icingaweb2) (≥2.9)
14-
* [Icinga PHP Library (ipl)](https://github.com/Icinga/icinga-php-library) (≥0.13)
14+
* [Icinga PHP Library (ipl)](https://github.com/Icinga/icinga-php-library) (≥0.13.2)
1515
* [Icinga PHP Thirdparty](https://github.com/Icinga/icinga-php-thirdparty) (≥0.12)
1616

1717
<!-- {% include "02-Installation.md" %} -->

‎library/Icingadb/Model/Behavior/FlattenedObjectVars.php

Lines changed: 34 additions & 5 deletions
Original file line numberDiff line numberDiff line change
@@ -11,6 +11,7 @@
1111
use ipl\Orm\Contract\QueryAwareBehavior;
1212
use ipl\Orm\Contract\RewriteColumnBehavior;
1313
use ipl\Orm\Query;
14+
use ipl\Stdlib\Data;
1415
use ipl\Stdlib\Filter;
1516

1617
class FlattenedObjectVars implements RewriteColumnBehavior, QueryAwareBehavior
@@ -32,11 +33,39 @@ public function rewriteCondition(Filter\Condition $condition, $relation = null)
3233
$column = $condition->metaData()->get('columnName');
3334
if ($column !== null) {
3435
$relation = substr($relation, 0, -5) . 'customvar_flat.';
35-
$nameFilter = Filter::like($relation . 'flatname', $column);
36-
$class = get_class($condition);
37-
$valueFilter = new $class($relation . 'flatvalue', $condition->getValue());
38-
39-
return Filter::all($nameFilter, $valueFilter);
36+
$condition->metaData()
37+
->set('requiresTransformation', true)
38+
->set('columnPath', $relation . $column)
39+
->set('relationPath', substr($relation, 0, -1));
40+
41+
// The ORM's FilterProcessor only optimizes filter conditions that are in the same level (chain).
42+
// Previously, this behavior transformed a single condition to an ALL chain and hence the semantics
43+
// of the level changed, since the FilterProcessor interpreted the conditions separately from there on.
44+
// To not change the semantics of the condition it is required to delay the transformation of the condition
45+
// until the subquery is created. Though, since the FilterProcessor only applies behaviors once, this
46+
// hack is required. (The entire filter, metadata and optimization is total garbage.)
47+
$oldMetaData = $condition->metaData();
48+
$metaDataProperty = (new \ReflectionClass($condition))->getProperty('metaData');
49+
$metaDataProperty->setAccessible(true);
50+
$metaDataProperty->setValue($condition, new class () extends Data {
51+
public function set($name, $value)
52+
{
53+
if ($name === 'behaviorsApplied') {
54+
return $this;
55+
}
56+
57+
return parent::set($name, $value);
58+
}
59+
});
60+
$condition->metaData()->merge($oldMetaData);
61+
62+
// But to make it even worse: If we do that, (not transforming the condition) the FilterProcessor sees
63+
// multiple conditions as targeting different columns, as it doesn't know that the *columns* are in fact
64+
// custom variables. It then attempts to combine the conditions with an AND, which is not possible, since
65+
// they refer to the same columns (flatname and flatvalue) after being transformed. So we have to make
66+
// the condition refer to a different column, which is totally irrelevant, but since it's always the same
67+
// column, the FilterProcessor won't attempt to combine the conditions. The literal icing on the cake.
68+
$condition->setColumn('always_the_same_but_totally_irrelevant');
4069
}
4170
}
4271

‎library/Icingadb/Model/CustomvarFlat.php

Lines changed: 17 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -6,9 +6,12 @@
66

77
use ipl\Orm\Behavior\Binary;
88
use ipl\Orm\Behaviors;
9+
use ipl\Orm\Contract\RewriteFilterBehavior;
910
use ipl\Orm\Model;
1011
use ipl\Orm\Query;
1112
use ipl\Orm\Relations;
13+
use ipl\Stdlib\Filter;
14+
use ipl\Stdlib\Filter\Condition;
1215
use Traversable;
1316

1417
/**
@@ -50,6 +53,20 @@ public function createBehaviors(Behaviors $behaviors)
5053
'customvar_id',
5154
'flatname_checksum'
5255
]));
56+
$behaviors->add(new class implements RewriteFilterBehavior {
57+
public function rewriteCondition(Condition $condition, $relation = null)
58+
{
59+
if ($condition->metaData()->has('requiresTransformation')) {
60+
/** @var string $columnName */
61+
$columnName = $condition->metaData()->get('columnName');
62+
$nameFilter = Filter::like($relation . 'flatname', $columnName);
63+
$class = get_class($condition);
64+
$valueFilter = new $class($relation . 'flatvalue', $condition->getValue());
65+
66+
return Filter::all($nameFilter, $valueFilter);
67+
}
68+
}
69+
});
5370
}
5471

5572
public function createRelations(Relations $relations)

‎module.info

Lines changed: 1 addition & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -1,6 +1,6 @@
11
Module: icingadb
22
Version: 1.1.1
33
Requires:
4-
Libraries: icinga-php-library (>=0.13.0), icinga-php-thirdparty (>=0.12.0)
4+
Libraries: icinga-php-library (>=0.13.2), icinga-php-thirdparty (>=0.12.0)
55
Description: Icinga DB Web
66
UI for Icinga DB – Provides a graphical interface to your Icinga monitoring

‎phpstan-baseline-standard.neon

Lines changed: 0 additions & 5 deletions
Original file line numberDiff line numberDiff line change
@@ -3055,11 +3055,6 @@ parameters:
30553055
count: 1
30563056
path: library/Icingadb/Model/Behavior/FlattenedObjectVars.php
30573057

3058-
-
3059-
message: "#^Parameter \\#2 \\$value of static method ipl\\\\Stdlib\\\\Filter\\:\\:like\\(\\) expects array\\<string\\>\\|string, mixed given\\.$#"
3060-
count: 1
3061-
path: library/Icingadb/Model/Behavior/FlattenedObjectVars.php
3062-
30633058
-
30643059
message: "#^Parameter \\#3 \\$subject of function str_replace expects array\\|string, string\\|null given\\.$#"
30653060
count: 1

‎test/php/library/Icingadb/Model/Behavior/FlattenedObjectVarsTest.php

Lines changed: 161 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,161 @@
1+
<?php
2+
3+
/* Icinga DB Web | (c) 2024 Icinga GmbH | GPLv2 */
4+
5+
namespace Tests\Icinga\Modules\Icingadb\Model\Behavior;
6+
7+
use Icinga\Module\Icingadb\Model\Host;
8+
use ipl\Sql\Connection;
9+
use ipl\Sql\Test\SqlAssertions;
10+
use ipl\Sql\Test\TestConnection;
11+
use ipl\Stdlib\Filter;
12+
use PHPUnit\Framework\TestCase;
13+
14+
class FlattenedObjectVarsTest extends TestCase
15+
{
16+
use SqlAssertions;
17+
18+
private const SINGLE_UNEQUAL_RESULT = <<<'SQL'
19+
SELECT host.id
20+
FROM host
21+
WHERE (host.id NOT IN ((SELECT sub_customvar_flat_host.id AS sub_customvar_flat_host_id
22+
FROM customvar_flat sub_customvar_flat
23+
INNER JOIN host_customvar sub_customvar_flat_host_customvar
24+
ON sub_customvar_flat_host_customvar.customvar_id =
25+
sub_customvar_flat.customvar_id
26+
INNER JOIN host sub_customvar_flat_host
27+
ON sub_customvar_flat_host.id = sub_customvar_flat_host_customvar.host_id
28+
WHERE ((sub_customvar_flat.flatname = ?) AND (sub_customvar_flat.flatvalue = ?))
29+
AND (sub_customvar_flat_host.id IS NOT NULL)
30+
GROUP BY sub_customvar_flat_host.id
31+
HAVING COUNT(DISTINCT sub_customvar_flat.id) >= ?)) OR host.id IS NULL)
32+
ORDER BY host.id
33+
SQL;
34+
35+
private const DOUBLE_UNEQUAL_RESULT = <<<'SQL'
36+
SELECT host.id
37+
FROM host
38+
WHERE (host.id NOT IN ((SELECT sub_customvar_flat_host.id AS sub_customvar_flat_host_id
39+
FROM customvar_flat sub_customvar_flat
40+
INNER JOIN host_customvar sub_customvar_flat_host_customvar
41+
ON sub_customvar_flat_host_customvar.customvar_id =
42+
sub_customvar_flat.customvar_id
43+
INNER JOIN host sub_customvar_flat_host
44+
ON sub_customvar_flat_host.id = sub_customvar_flat_host_customvar.host_id
45+
WHERE (((sub_customvar_flat.flatname = ?) AND (sub_customvar_flat.flatvalue = ?)) OR
46+
((sub_customvar_flat.flatname = ?) AND (sub_customvar_flat.flatvalue = ?)))
47+
AND (sub_customvar_flat_host.id IS NOT NULL)
48+
GROUP BY sub_customvar_flat_host.id
49+
HAVING COUNT(DISTINCT sub_customvar_flat.id) >= ?)) OR host.id IS NULL)
50+
ORDER BY host.id
51+
SQL;
52+
53+
private const EQUAL_UNEQUAL_RESULT = <<<'SQL'
54+
SELECT host.id
55+
FROM host
56+
WHERE ((host.id NOT IN ((SELECT sub_customvar_flat_host.id AS sub_customvar_flat_host_id
57+
FROM customvar_flat sub_customvar_flat
58+
INNER JOIN host_customvar sub_customvar_flat_host_customvar
59+
ON sub_customvar_flat_host_customvar.customvar_id =
60+
sub_customvar_flat.customvar_id
61+
INNER JOIN host sub_customvar_flat_host
62+
ON sub_customvar_flat_host.id = sub_customvar_flat_host_customvar.host_id
63+
WHERE ((sub_customvar_flat.flatname = ?) AND (sub_customvar_flat.flatvalue = ?))
64+
AND (sub_customvar_flat_host.id IS NOT NULL)
65+
GROUP BY sub_customvar_flat_host.id
66+
HAVING COUNT(DISTINCT sub_customvar_flat.id) >= ?)) OR host.id IS NULL))
67+
AND (host.id IN ((SELECT sub_customvar_flat_host.id AS sub_customvar_flat_host_id
68+
FROM customvar_flat sub_customvar_flat
69+
INNER JOIN host_customvar sub_customvar_flat_host_customvar
70+
ON sub_customvar_flat_host_customvar.customvar_id =
71+
sub_customvar_flat.customvar_id
72+
INNER JOIN host sub_customvar_flat_host
73+
ON sub_customvar_flat_host.id = sub_customvar_flat_host_customvar.host_id
74+
WHERE (sub_customvar_flat.flatname = ?)
75+
AND (sub_customvar_flat.flatvalue = ?)
76+
GROUP BY sub_customvar_flat_host.id
77+
HAVING COUNT(DISTINCT sub_customvar_flat.id) >= ?)))
78+
ORDER BY host.id
79+
SQL;
80+
81+
private const DOUBLE_EQUAL_RESULT = <<<'SQL'
82+
SELECT host.id
83+
FROM host
84+
WHERE host.id IN ((SELECT sub_customvar_flat_host.id AS sub_customvar_flat_host_id
85+
FROM customvar_flat sub_customvar_flat
86+
INNER JOIN host_customvar sub_customvar_flat_host_customvar
87+
ON sub_customvar_flat_host_customvar.customvar_id =
88+
sub_customvar_flat.customvar_id
89+
INNER JOIN host sub_customvar_flat_host
90+
ON sub_customvar_flat_host.id = sub_customvar_flat_host_customvar.host_id
91+
WHERE ((sub_customvar_flat.flatname = ?) AND (sub_customvar_flat.flatvalue = ?))
92+
OR ((sub_customvar_flat.flatname = ?) AND (sub_customvar_flat.flatvalue = ?))
93+
GROUP BY sub_customvar_flat_host.id
94+
HAVING COUNT(DISTINCT sub_customvar_flat.id) >= ?))
95+
ORDER BY host.id
96+
SQL;
97+
98+
/** @var Connection */
99+
private $connection;
100+
101+
public function setUp(): void
102+
{
103+
$this->connection = new TestConnection();
104+
$this->setUpSqlAssertions();
105+
}
106+
107+
public function testSingleUnequalCondition()
108+
{
109+
$query = Host::on($this->connection)
110+
->columns('host.id')
111+
->orderBy('host.id')
112+
->filter(Filter::unequal('host.vars.invalid', 'foo'));
113+
114+
$this->assertSql(self::SINGLE_UNEQUAL_RESULT, $query->assembleSelect(), ['invalid', 'foo', 1]);
115+
}
116+
117+
public function testDoubleUnequalCondition()
118+
{
119+
$query = Host::on($this->connection)
120+
->columns('host.id')
121+
->orderBy('host.id')
122+
->filter(Filter::unequal('host.vars.invalid', 'foo'))
123+
->filter(Filter::unequal('host.vars.missing', 'bar'));
124+
125+
$this->assertSql(
126+
self::DOUBLE_UNEQUAL_RESULT,
127+
$query->assembleSelect(),
128+
['invalid', 'foo', 'missing', 'bar', 1]
129+
);
130+
}
131+
132+
public function testEqualAndUnequalCondition()
133+
{
134+
$query = Host::on($this->connection)
135+
->columns('host.id')
136+
->orderBy('host.id')
137+
->filter(Filter::unequal('host.vars.invalid', 'bar'))
138+
->filter(Filter::equal('host.vars.env', 'foo'));
139+
140+
$this->assertSql(
141+
self::EQUAL_UNEQUAL_RESULT,
142+
$query->assembleSelect(),
143+
['invalid', 'bar', 1, 'env', 'foo', 1]
144+
);
145+
}
146+
147+
public function testDoubleEqualCondition()
148+
{
149+
$query = Host::on($this->connection)
150+
->columns('host.id')
151+
->orderBy('host.id')
152+
->filter(Filter::equal('host.vars.env', 'foo'))
153+
->filter(Filter::equal('host.vars.os', 'bar'));
154+
155+
$this->assertSql(
156+
self::DOUBLE_EQUAL_RESULT,
157+
$query->assembleSelect(),
158+
['env', 'foo', 'os', 'bar', 2]
159+
);
160+
}
161+
}

0 commit comments

Comments
 (0)