Skip to content

Commit

Permalink
FilterProcessor: Ignore NULL results in NOT IN conditions
Browse files Browse the repository at this point in the history
  • Loading branch information
nilmerg committed Mar 12, 2024
1 parent 26b245d commit 8383627
Show file tree
Hide file tree
Showing 5 changed files with 175 additions and 0 deletions.
6 changes: 6 additions & 0 deletions src/Compat/FilterProcessor.php
Original file line number Diff line number Diff line change
Expand Up @@ -328,6 +328,12 @@ protected function requireAndResolveFilterColumns(Filter\Rule $filter, Query $qu

$subQuerySelect->having(["COUNT(DISTINCT $targetKeys) >= ?" => $count]);
$subQuerySelect->groupBy(array_values($subQuerySelect->getColumns()));

if ($negate) {
$subQuerySelect->where(array_map(function ($k) {
return $k . ' IS NOT NULL';
}, array_values($subQuerySelect->getColumns())));
}
}

// TODO: Qualification is only necessary since the `In` and `NotIn` conditions are ignored by
Expand Down
70 changes: 70 additions & 0 deletions tests/FilterProcessorTest.php
Original file line number Diff line number Diff line change
Expand Up @@ -4,10 +4,15 @@

use ipl\Orm\Compat\FilterProcessor;
use ipl\Orm\Query;
use ipl\Sql\Connection;
use ipl\Sql\Test\Databases;
use ipl\Stdlib\Filter;
use ipl\Tests\Orm\Lib\Model\Office;

class FilterProcessorTest extends \PHPUnit\Framework\TestCase
{
use Databases;

public function testUnequalDoesNotOverrideUnlike()
{
$query = new Query();
Expand Down Expand Up @@ -39,6 +44,9 @@ public function testUnequalDoesNotOverrideUnlike()
]]
]]
]]
]],
['AND', [
'sub_passenger_car.id IS NOT NULL'
]]
]],
$where[1][0][1][0][1]['(car.id NOT IN (?) OR car.id IS NULL)']->getWhere()
Expand All @@ -57,9 +65,71 @@ public function testUnequalDoesNotOverrideUnlike()
]]
]]
]]
]],
['AND', [
'sub_passenger_car.id IS NOT NULL'
]]
]],
$where[1][0][1][1][1]['(car.id NOT IN (?) OR car.id IS NULL)']->getWhere()
);
}

/**
* Test whether an unequal, that targets a to-many relation to which a link can only be established through an
* optional other relation, is built by the ORM in a way that coincidental matches are ignored
*
* This will fail if the ORM generates a NOT IN which uses a subquery that produces NULL values.
*
* @dataProvider databases
*/
public function testUnequalTargetingAnOptionalToManyRelationIgnoresFalsePositives(Connection $db)
{
$db->insert('office', ['id' => 1, 'city' => 'London']);
$db->insert('department', ['id' => 1, 'name' => 'Accounting']);
$db->insert('department', ['id' => 2, 'name' => 'Kitchen']);
$db->insert('employee', ['id' => 1, 'department_id' => 1, 'name' => 'Minnie', 'role' => 'CEO']); // remote
$db->insert(
'employee',
['id' => 2, 'department_id' => 2, 'office_id' => 1, 'name' => 'Goofy', 'role' => 'Developer']
);

// This POC uses inner joins to achieve the desired result
$offices = $db->prepexec(
'SELECT office.city FROM office'
. ' INNER JOIN employee e on e.office_id = office.id'
. ' INNER JOIN department d on e.department_id = d.id'
. ' WHERE d.name != ?'
. ' GROUP BY office.id'
. ' ORDER BY office.id',
['Accounting']
)->fetchAll();

$this->assertSame('London', $offices[0]['city'] ?? 'not found');

// The ORM will use a NOT IN and needs to ignore false positives explicitly
$offices = Office::on($db)
->columns(['office.city'])
->orderBy('office.id')
->filter(Filter::unequal('employee.department.name', 'Accounting'));
$results = iterator_to_array($offices);

$this->assertSame('London', $results[0]['city'] ?? 'not found');
}

protected function createSchema(Connection $db, string $driver): void
{
$db->exec('CREATE TABLE office (id INT PRIMARY KEY, city VARCHAR(255))');
$db->exec('CREATE TABLE department (id INT PRIMARY KEY, name VARCHAR(255))');
$db->exec(
'CREATE TABLE employee (id INT PRIMARY KEY, department_id INT,'
. ' office_id INT, name VARCHAR(255), role VARCHAR(255))'
);
}

protected function dropSchema(Connection $db, string $driver): void
{
$db->exec('DROP TABLE IF EXISTS employee');
$db->exec('DROP TABLE IF EXISTS department');
$db->exec('DROP TABLE IF EXISTS office');
}
}
32 changes: 32 additions & 0 deletions tests/Lib/Model/Department.php
Original file line number Diff line number Diff line change
@@ -0,0 +1,32 @@
<?php

namespace ipl\Tests\Orm\Lib\Model;

use ipl\Orm\Model;
use ipl\Orm\Relations;

class Department extends Model
{
public function getTableName()
{
return 'department';
}

public function getKeyName()
{
return 'id';
}

public function getColumns()
{
return [
'name'
];
}

public function createRelations(Relations $relations)
{
$relations->hasMany('employee', Employee::class)
->setJoinType('LEFT');
}
}
36 changes: 36 additions & 0 deletions tests/Lib/Model/Employee.php
Original file line number Diff line number Diff line change
@@ -0,0 +1,36 @@
<?php

namespace ipl\Tests\Orm\Lib\Model;

use ipl\Orm\Model;
use ipl\Orm\Relations;

class Employee extends Model
{
public function getTableName()
{
return 'employee';
}

public function getKeyName()
{
return 'id';
}

public function getColumns()
{
return [
'name',
'role',
'department_id',
'office_id'
];
}

public function createRelations(Relations $relations)
{
$relations->belongsTo('department', Department::class);
$relations->belongsTo('office', Office::class)
->setJoinType('LEFT');
}
}
31 changes: 31 additions & 0 deletions tests/Lib/Model/Office.php
Original file line number Diff line number Diff line change
@@ -0,0 +1,31 @@
<?php

namespace ipl\Tests\Orm\Lib\Model;

use ipl\Orm\Model;
use ipl\Orm\Relations;

class Office extends Model
{
public function getTableName()
{
return 'office';
}

public function getKeyName()
{
return 'id';
}

public function getColumns()
{
return [
'city'
];
}

public function createRelations(Relations $relations)
{
$relations->hasMany('employee', Employee::class);
}
}

0 comments on commit 8383627

Please sign in to comment.