ENH Do not use placeholders for int ID filters

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);
+            $usePlaceholders = $this->usePlaceholders($column, $values);
-            if ($inclusive) {
+            if ($usePlaceholders) {
-                $predicate = "$column IN ($placeholders)";
+                $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;
+    }
 }

tests/php/ORM/Filters/ExactMatchFilterTest.php

@@ -0,0 +1,95 @@
+<?php
+
+namespace SilverStripe\ORM\Tests\Filters;
+
+use SilverStripe\Core\Config\Config;
+use SilverStripe\Dev\SapphireTest;
+use SilverStripe\ORM\Filters\ExactMatchFilter;
+use SilverStripe\ORM\Tests\Filters\ExactMatchFilterTest\Task;
+use SilverStripe\ORM\Tests\Filters\ExactMatchFilterTest\Project;
+
+class ExactMatchFilterTest extends SapphireTest
+{
+    protected static $fixture_file = 'ExactMatchFilterTest.yml';
+
+    protected static $extra_dataobjects = [
+        Task::class,
+        Project::class,
+    ];
+
+    /**
+     * @dataProvider provideUsePlaceholders
+     */
+    public function testUsePlaceholders(?bool $expectedID, ?bool $expectedTitle, bool $config, callable $fn): void
+    {
+        Config::modify()->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];
+    }
+}

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

tests/php/ORM/Filters/ExactMatchFilterTest/Project.php

@@ -0,0 +1,19 @@
+<?php
+
+namespace SilverStripe\ORM\Tests\Filters\ExactMatchFilterTest;
+
+use SilverStripe\Dev\TestOnly;
+use SilverStripe\ORM\DataObject;
+
+class Project extends DataObject implements TestOnly
+{
+    private static $table_name = 'ExactMatchFilterTest_Project';
+
+    private static $db = [
+        'Title' => 'Varchar(255)',
+    ];
+
+    private static $has_many = [
+        'Tasks' => Task::class,
+    ];
+}

tests/php/ORM/Filters/ExactMatchFilterTest/Task.php

@@ -0,0 +1,19 @@
+<?php
+
+namespace SilverStripe\ORM\Tests\Filters\ExactMatchFilterTest;
+
+use SilverStripe\Dev\TestOnly;
+use SilverStripe\ORM\DataObject;
+
+class Task extends DataObject implements TestOnly
+{
+    private static $table_name = 'ExactMatchFilterTest_Task';
+
+    private static $db = [
+        'Title' => 'Varchar(255)',
+    ];
+
+    private static $has_one = [
+        'Project' => Project::class,
+    ];
+}