diff --git a/src/ORM/Filters/ExactMatchFilter.php b/src/ORM/Filters/ExactMatchFilter.php index f24dd0dbe..4a46a8589 100644 --- a/src/ORM/Filters/ExactMatchFilter.php +++ b/src/ORM/Filters/ExactMatchFilter.php @@ -4,7 +4,11 @@ namespace SilverStripe\ORM\Filters; use SilverStripe\ORM\DataQuery; use SilverStripe\ORM\DB; -use InvalidArgumentException; +use SilverStripe\Core\Config\Configurable; +use SilverStripe\ORM\DataObject; +use SilverStripe\Core\Injector\Injector; +use SilverStripe\ORM\FieldType\DBPrimaryKey; +use SilverStripe\ORM\FieldType\DBForeignKey; /** * Selects textual content with an exact match between columnname and keyword. @@ -14,6 +18,9 @@ use InvalidArgumentException; */ class ExactMatchFilter extends SearchFilter { + use Configurable; + + private static bool $use_placeholders_for_integer_ids = false; public function getSupportedModifiers() { @@ -133,17 +140,25 @@ class ExactMatchFilter extends SearchFilter } $connective = ''; + $setValuesToEmpty = false; if (empty($values)) { $predicate = ''; } elseif ($caseSensitive === null) { // For queries using the default collation (no explicit case) we can use the WHERE .. NOT IN .. syntax, // providing simpler SQL than many WHERE .. AND .. fragments. $column = $this->getDbName(); - $placeholders = DB::placeholders($values); - if ($inclusive) { - $predicate = "$column IN ($placeholders)"; + $usePlaceholders = $this->usePlaceholders($column, $values); + if ($usePlaceholders) { + $in = DB::placeholders($values); } else { - $predicate = "$column NOT IN ($placeholders)"; + // explicitly including space after comma to match the default for DB::placeholders + $in = implode(', ', $values); + $setValuesToEmpty = true; + } + if ($inclusive) { + $predicate = "$column IN ($in)"; + } else { + $predicate = "$column NOT IN ($in)"; } } else { // Generate reusable comparison clause @@ -194,7 +209,8 @@ class ExactMatchFilter extends SearchFilter } } - $clause = [$predicate => $values]; + $vals = $setValuesToEmpty ? [] : $values; + $clause = [$predicate => $vals]; return $this->aggregate ? $this->applyAggregate($query, $clause) : @@ -205,4 +221,44 @@ class ExactMatchFilter extends SearchFilter { return $this->getValue() === [] || $this->getValue() === null || $this->getValue() === ''; } + + /** + * Determine if we should use placeholders for the given column and values + * Current rules are to use placeholders for all values, unless all of these are true: + * - The column is a DBPrimaryKey or DBForeignKey + * - The values being filtered are all either integers or valid integer strings + * - Using placeholders for integer ids has been configured off + * + * Putting IDs directly into a where clause instead of using placehodlers was measured to be significantly + * faster when querying a large number of IDs e.g. over 1000 + */ + private function usePlaceholders(string $column, array $values): bool + { + if ($this->config()->get('use_placeholders_for_integer_ids')) { + return true; + } + // Ensure that the $column was created in the "Table"."Column" format + // by DataObjectSchema::sqlColumnForField() after first being called by SearchFilter::getDbName() + // earlier on in manyFilter(). Do this check to ensure that we're be safe and only not using + // placeholders in scenarios where we intend to not use them. + if (!preg_match('#^"(.+)"."(.+)"$#', $column, $matches)) { + return true; + } + $col = $matches[2]; + // Check if column is Primary or Foreign key, if it's not then we use placeholders + $schema = DataObject::getSchema(); + $fieldSpec = $schema->fieldSpec($this->model, $col); + $fieldObj = Injector::inst()->create($fieldSpec, $col); + if (!is_a($fieldObj, DBPrimaryKey::class) && !is_a($fieldObj, DBForeignKey::class)) { + return true; + } + // Validate that we're only using int ID's for the values + // We need to do this to protect against SQL injection + foreach ($values as $value) { + if (!ctype_digit((string) $value) || $value != (int) $value) { + return true; + } + } + return false; + } } diff --git a/tests/php/ORM/Filters/ExactMatchFilterTest.php b/tests/php/ORM/Filters/ExactMatchFilterTest.php new file mode 100644 index 000000000..400cd28fa --- /dev/null +++ b/tests/php/ORM/Filters/ExactMatchFilterTest.php @@ -0,0 +1,95 @@ +set(ExactMatchFilter::class, 'use_placeholders_for_integer_ids', $config); + [$idQueryUsesPlaceholders, $titleQueryUsesPlaceholders] = $this->usesPlaceholders($fn); + $this->assertSame($expectedID, $idQueryUsesPlaceholders); + $this->assertSame($expectedTitle, $titleQueryUsesPlaceholders); + } + + public function provideUsePlaceholders(): array + { + $ids = [1, 2, 3]; + $taskTitles = array_map(fn($i) => "Task $i", $ids); + return [ + 'primary key' => [ + 'expectedID' => false, + 'expectedTitle' => null, + 'config' => false, + 'fn' => fn() => Task::get()->byIDs($ids) + ], + 'primary key on relation' => [ + 'expectedID' => false, + 'expectedTitle' => null, + 'config' => false, + 'fn' => fn() => Project::get()->filter('Tasks.ID', $ids) + ], + 'foriegn key' => [ + 'expectedID' => false, + 'expectedTitle' => null, + 'config' => false, + 'fn' => fn() => Task::get()->filter(['ProjectID' => $ids]) + ], + 'regular column' => [ + 'expectedID' => null, + 'expectedTitle' => true, + 'config' => false, + 'fn' => fn() => Task::get()->filter(['Title' => $taskTitles]) + ], + 'primary key + regular column' => [ + 'expectedID' => false, + 'expectedTitle' => true, + 'config' => false, + 'fn' => fn() => Task::get()->filter([ + 'ID' => $ids, + 'Title' => $taskTitles + ]) + ], + 'primary key config enabled' => [ + 'expectedID' => true, + 'expectedTitle' => null, + 'config' => true, + 'fn' => fn() => Task::get()->byIDs($ids) + ], + 'non int values' => [ + 'expectedID' => true, + 'expectedTitle' => null, + 'config' => false, + 'fn' => fn() => Task::get()->filter(['ID' => ['a', 'b', 'c']]) + ], + ]; + } + + private function usesPlaceholders(callable $fn): array + { + // force showqueries on to view executed SQL via output-buffering + $list = $fn(); + $sql = $list->dataQuery()->sql(); + preg_match('#ID" IN \(([^\)]+)\)\)#', $sql, $matches); + $idQueryUsesPlaceholders = isset($matches[1]) ? $matches[1] === '?, ?, ?' : null; + preg_match('#"Title" IN \(([^\)]+)\)\)#', $sql, $matches); + $titleQueryUsesPlaceholders = isset($matches[1]) ? $matches[1] === '?, ?, ?' : null; + return [$idQueryUsesPlaceholders, $titleQueryUsesPlaceholders]; + } +} diff --git a/tests/php/ORM/Filters/ExactMatchFilterTest.yml b/tests/php/ORM/Filters/ExactMatchFilterTest.yml new file mode 100644 index 000000000..4ee2a9087 --- /dev/null +++ b/tests/php/ORM/Filters/ExactMatchFilterTest.yml @@ -0,0 +1,17 @@ +SilverStripe\ORM\Tests\Filters\ExactMatchFilterTest\Project: + project1: + Title: 'Project 1' + project2: + Title: 'Project 2' + project3: + Title: 'Project 3' +SilverStripe\ORM\Tests\Filters\ExactMatchFilterTest\Task: + task1: + Title: 'Task 1' + Project: =>SilverStripe\ORM\Tests\Filters\ExactMatchFilterTest\Project.project1 + task2: + Title: 'Task 2' + Project: =>SilverStripe\ORM\Tests\Filters\ExactMatchFilterTest\Project.project2 + task3: + Title: 'Task 3' + Project: =>SilverStripe\ORM\Tests\Filters\ExactMatchFilterTest\Project.project3 diff --git a/tests/php/ORM/Filters/ExactMatchFilterTest/Project.php b/tests/php/ORM/Filters/ExactMatchFilterTest/Project.php new file mode 100644 index 000000000..57ccb0b56 --- /dev/null +++ b/tests/php/ORM/Filters/ExactMatchFilterTest/Project.php @@ -0,0 +1,19 @@ + 'Varchar(255)', + ]; + + private static $has_many = [ + 'Tasks' => Task::class, + ]; +} diff --git a/tests/php/ORM/Filters/ExactMatchFilterTest/Task.php b/tests/php/ORM/Filters/ExactMatchFilterTest/Task.php new file mode 100644 index 000000000..de845b359 --- /dev/null +++ b/tests/php/ORM/Filters/ExactMatchFilterTest/Task.php @@ -0,0 +1,19 @@ + 'Varchar(255)', + ]; + + private static $has_one = [ + 'Project' => Project::class, + ]; +}