Skip to content

Commit

Permalink
Transform custom variable filters as late as possible
Browse files Browse the repository at this point in the history 
fixes #865
  • Loading branch information
@nilmerg
nilmerg committed Mar 12, 2024
1 parent 274b903 commit b8a5849
Show file tree
Hide file tree
Showing 4 changed files with 220 additions and 10 deletions.
39 changes: 34 additions & 5 deletions library/Icingadb/Model/Behavior/FlattenedObjectVars.php
View file
Original file line number Diff line number Diff line change
Expand Up @@ -11,6 +11,7 @@
use ipl\Orm\Contract\QueryAwareBehavior;
use ipl\Orm\Contract\RewriteColumnBehavior;
use ipl\Orm\Query;
use ipl\Stdlib\Data;
use ipl\Stdlib\Filter;

class FlattenedObjectVars implements RewriteColumnBehavior, QueryAwareBehavior
Expand All @@ -32,11 +33,39 @@ public function rewriteCondition(Filter\Condition $condition, $relation = null)
$column = $condition->metaData()->get('columnName');
if ($column !== null) {
$relation = substr($relation, 0, -5) . 'customvar_flat.';
$nameFilter = Filter::like($relation . 'flatname', $column);
$class = get_class($condition);
$valueFilter = new $class($relation . 'flatvalue', $condition->getValue());

return Filter::all($nameFilter, $valueFilter);
$condition->metaData()
->set('requiresTransformation', true)
->set('columnPath', $relation . $column)
->set('relationPath', substr($relation, 0, -1));

// The ORM's FilterProcessor only optimizes filter conditions that are in the same level (chain).
// Previously, this behavior transformed a single condition to an ALL chain and hence the semantics
// of the level changed, since the FilterProcessor interpreted the conditions separately from there on.
// To not change the semantics of the condition it is required to delay the transformation of the condition
// until the subquery is created. Though, since the FilterProcessor only applies behaviors once, this
// hack is required. (The entire filter, metadata and optimization is total garbage.)
$oldMetaData = $condition->metaData();
$reflection = new \ReflectionClass($condition);
$reflection->getProperty('metaData')->setAccessible(true);
$reflection->getProperty('metaData')->setValue($condition, new class () extends Data {
public function set($name, $value)
{
if ($name === 'behaviorsApplied') {
return $this;
}

return parent::set($name, $value);
}
});
$condition->metaData()->merge($oldMetaData);

// But to make it even worse: If we do that, (not transforming the condition) the FilterProcessor sees
// multiple conditions as targeting different columns, as it doesn't know that the *columns* are in fact
// custom variables. It then attempts to combine the conditions with an AND, which is not possible, since
// they refer to the same columns (flatname and flatvalue) after being transformed. So we have to make
// the condition refer to a different column, which is totally irrelevant, but since it's always the same
// column, the FilterProcessor won't attempt to combine the conditions. The literal icing on the cake.
$condition->setColumn('always_the_same_but_totally_irrelevant');
}
}

Expand Down
17 changes: 17 additions & 0 deletions library/Icingadb/Model/CustomvarFlat.php
View file
Original file line number Diff line number Diff line change
Expand Up @@ -6,9 +6,12 @@

use ipl\Orm\Behavior\Binary;
use ipl\Orm\Behaviors;
use ipl\Orm\Contract\RewriteFilterBehavior;
use ipl\Orm\Model;
use ipl\Orm\Query;
use ipl\Orm\Relations;
use ipl\Stdlib\Filter;
use ipl\Stdlib\Filter\Condition;
use Traversable;

class CustomvarFlat extends Model
Expand Down Expand Up @@ -42,6 +45,20 @@ public function createBehaviors(Behaviors $behaviors)
'customvar_id',
'flatname_checksum'
]));
$behaviors->add(new class implements RewriteFilterBehavior {
public function rewriteCondition(Condition $condition, $relation = null)
{
if ($condition->metaData()->has('requiresTransformation')) {
/** @var string $columnName */
$columnName = $condition->metaData()->get('columnName');
$nameFilter = Filter::like($relation . 'flatname', $columnName);
$class = get_class($condition);
$valueFilter = new $class($relation . 'flatvalue', $condition->getValue());

return Filter::all($nameFilter, $valueFilter);
}
}
});
}

public function createRelations(Relations $relations)
Expand Down
5 changes: 0 additions & 5 deletions phpstan-baseline.neon
View file
Original file line number Diff line number Diff line change
Expand Up @@ -3195,11 +3195,6 @@ parameters:
count: 1
path: library/Icingadb/Model/Behavior/FlattenedObjectVars.php

-
message: "#^Parameter \\#2 \\$value of static method ipl\\\\Stdlib\\\\Filter\\:\\:like\\(\\) expects array\\<string\\>\\|string, mixed given\\.$#"
count: 1
path: library/Icingadb/Model/Behavior/FlattenedObjectVars.php

-
message: "#^Parameter \\#3 \\$subject of function str_replace expects array\\|string, string\\|null given\\.$#"
count: 1
Expand Down
169 changes: 169 additions & 0 deletions test/php/library/Icingadb/Model/Behavior/FlattenedObjectVarsTest.php
View file
Original file line number Diff line number Diff line change
@@ -0,0 +1,169 @@
<?php

/* Icinga DB Web | (c) 2024 Icinga GmbH | GPLv2 */

namespace Tests\Icinga\Modules\Icingadb\Model\Behavior;

use Icinga\Module\Icingadb\Model\Host;
use ipl\Sql\Adapter\BaseAdapter;
use ipl\Sql\Connection;
use ipl\Stdlib\Filter;
use ipl\Tests\Sql\Lib\SqlAssertions;
use PHPUnit\Framework\TestCase;

class FlattenedObjectVarsTest extends TestCase
{
use SqlAssertions {
SqlAssertions::setUp as protected setUpSql;
}

private const SINGLE_UNEQUAL_RESULT = <<<'SQL'
SELECT host.id
FROM host
WHERE (host.id NOT IN ((SELECT sub_customvar_flat_host.id AS sub_customvar_flat_host_id
FROM customvar_flat sub_customvar_flat
INNER JOIN host_customvar sub_customvar_flat_host_customvar
ON sub_customvar_flat_host_customvar.customvar_id =
sub_customvar_flat.customvar_id
INNER JOIN host sub_customvar_flat_host
ON sub_customvar_flat_host.id = sub_customvar_flat_host_customvar.host_id
WHERE ((sub_customvar_flat.flatname = ?) AND (sub_customvar_flat.flatvalue = ?))
AND (sub_customvar_flat_host.id IS NOT NULL)
GROUP BY sub_customvar_flat_host.id
HAVING COUNT(DISTINCT sub_customvar_flat.id) >= ?)) OR host.id IS NULL)
ORDER BY host.id
SQL;

private const DOUBLE_UNEQUAL_RESULT = <<<'SQL'
SELECT host.id
FROM host
WHERE (host.id NOT IN ((SELECT sub_customvar_flat_host.id AS sub_customvar_flat_host_id
FROM customvar_flat sub_customvar_flat
INNER JOIN host_customvar sub_customvar_flat_host_customvar
ON sub_customvar_flat_host_customvar.customvar_id =
sub_customvar_flat.customvar_id
INNER JOIN host sub_customvar_flat_host
ON sub_customvar_flat_host.id = sub_customvar_flat_host_customvar.host_id
WHERE (((sub_customvar_flat.flatname = ?) AND (sub_customvar_flat.flatvalue = ?)) OR
((sub_customvar_flat.flatname = ?) AND (sub_customvar_flat.flatvalue = ?)))
AND (sub_customvar_flat_host.id IS NOT NULL)
GROUP BY sub_customvar_flat_host.id
HAVING COUNT(DISTINCT sub_customvar_flat.id) >= ?)) OR host.id IS NULL)
ORDER BY host.id
SQL;

private const EQUAL_UNEQUAL_RESULT = <<<'SQL'
SELECT host.id
FROM host
WHERE ((host.id NOT IN ((SELECT sub_customvar_flat_host.id AS sub_customvar_flat_host_id
FROM customvar_flat sub_customvar_flat
INNER JOIN host_customvar sub_customvar_flat_host_customvar
ON sub_customvar_flat_host_customvar.customvar_id =
sub_customvar_flat.customvar_id
INNER JOIN host sub_customvar_flat_host
ON sub_customvar_flat_host.id = sub_customvar_flat_host_customvar.host_id
WHERE ((sub_customvar_flat.flatname = ?) AND (sub_customvar_flat.flatvalue = ?))
AND (sub_customvar_flat_host.id IS NOT NULL)
GROUP BY sub_customvar_flat_host.id
HAVING COUNT(DISTINCT sub_customvar_flat.id) >= ?)) OR host.id IS NULL))
AND (host.id IN ((SELECT sub_customvar_flat_host.id AS sub_customvar_flat_host_id
FROM customvar_flat sub_customvar_flat
INNER JOIN host_customvar sub_customvar_flat_host_customvar
ON sub_customvar_flat_host_customvar.customvar_id =
sub_customvar_flat.customvar_id
INNER JOIN host sub_customvar_flat_host
ON sub_customvar_flat_host.id = sub_customvar_flat_host_customvar.host_id
WHERE (sub_customvar_flat.flatname = ?)
AND (sub_customvar_flat.flatvalue = ?)
GROUP BY sub_customvar_flat_host.id
HAVING COUNT(DISTINCT sub_customvar_flat.id) >= ?)))
ORDER BY host.id
SQL;

private const DOUBLE_EQUAL_RESULT = <<<'SQL'
SELECT host.id
FROM host
WHERE host.id IN ((SELECT sub_customvar_flat_host.id AS sub_customvar_flat_host_id
FROM customvar_flat sub_customvar_flat
INNER JOIN host_customvar sub_customvar_flat_host_customvar
ON sub_customvar_flat_host_customvar.customvar_id =
sub_customvar_flat.customvar_id
INNER JOIN host sub_customvar_flat_host
ON sub_customvar_flat_host.id = sub_customvar_flat_host_customvar.host_id
WHERE ((sub_customvar_flat.flatname = ?) AND (sub_customvar_flat.flatvalue = ?))
OR ((sub_customvar_flat.flatname = ?) AND (sub_customvar_flat.flatvalue = ?))
GROUP BY sub_customvar_flat_host.id
HAVING COUNT(DISTINCT sub_customvar_flat.id) >= ?))
ORDER BY host.id
SQL;

/** @var Connection */
private $connection;

public function setUp(): void
{
$this->connection = new class extends Connection {
public function __construct()
{
$this->adapter = new class extends BaseAdapter {};
}
};

$this->setUpSql();
}

public function testSingleUnequalCondition()
{
$query = Host::on($this->connection)
->columns('host.id')
->orderBy('host.id')
->filter(Filter::unequal('host.vars.invalid', 'foo'));

$this->assertSql(self::SINGLE_UNEQUAL_RESULT, $query->assembleSelect(), ['invalid', 'foo', 1]);
}

public function testDoubleUnequalCondition()
{
$query = Host::on($this->connection)
->columns('host.id')
->orderBy('host.id')
->filter(Filter::unequal('host.vars.invalid', 'foo'))
->filter(Filter::unequal('host.vars.missing', 'bar'));

$this->assertSql(
self::DOUBLE_UNEQUAL_RESULT,
$query->assembleSelect(),
['invalid', 'foo', 'missing', 'bar', 1]
);
}

public function testEqualAndUnequalCondition()
{
$query = Host::on($this->connection)
->columns('host.id')
->orderBy('host.id')
->filter(Filter::unequal('host.vars.invalid', 'bar'))
->filter(Filter::equal('host.vars.env', 'foo'));

$this->assertSql(
self::EQUAL_UNEQUAL_RESULT,
$query->assembleSelect(),
['invalid', 'bar', 1, 'env', 'foo', 1]
);
}

public function testDoubleEqualCondition()
{
$query = Host::on($this->connection)
->columns('host.id')
->orderBy('host.id')
->filter(Filter::equal('host.vars.env', 'foo'))
->filter(Filter::equal('host.vars.os', 'bar'));

$this->assertSql(
self::DOUBLE_EQUAL_RESULT,
$query->assembleSelect(),
['env', 'foo', 'os', 'bar', 2]
);
}
}

0 comments on commit b8a5849

Please sign in to comment.