Merge pull request #10904 from creative-commoners/pulls/5/more-placeh…

…olders

ENH Do not use placeholders by default for foreignIDFilter()

src/ORM/DataList.php

@@ -38,6 +38,11 @@
  */
 class DataList extends ViewableData implements SS_List, Filterable, Sortable, Limitable
 {
+    /**
+     * Whether to use placeholders for integer IDs on Primary and Foriegn keys during a WHERE IN query
+     * It is significantly faster to not use placeholders
+     */
+    private static bool $use_placeholders_for_integer_ids = false;
 
     /**
      * The DataObject class name that this data list is querying

src/ORM/Filters/ExactMatchFilter.php

@@ -4,11 +4,11 @@
 
 use SilverStripe\ORM\DataQuery;
 use SilverStripe\ORM\DB;
-use SilverStripe\Core\Config\Configurable;
 use SilverStripe\ORM\DataObject;
 use SilverStripe\Core\Injector\Injector;
 use SilverStripe\ORM\FieldType\DBPrimaryKey;
 use SilverStripe\ORM\FieldType\DBForeignKey;
+use SilverStripe\ORM\DataList;
 
 /**
  * Selects textual content with an exact match between columnname and keyword.
@@ -18,9 +18,6 @@
  */
 class ExactMatchFilter extends SearchFilter
 {
-    use Configurable;
-
-    private static bool $use_placeholders_for_integer_ids = false;
 
     public function getSupportedModifiers()
     {
@@ -229,12 +226,12 @@ public function isEmpty()
      * - 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
+     * Putting IDs directly into a where clause instead of using placeholders 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')) {
+        if (DataList::config()->get('use_placeholders_for_integer_ids')) {
             return true;
         }
         // Ensure that the $column was created in the "Table"."Column" format

src/ORM/HasManyList.php

@@ -50,11 +50,12 @@ protected function foreignIDFilter($id = null)
         if ($id === null) {
             $id = $this->getForeignID();
         }
-
         // Apply relation filter
         $key = DataObject::getSchema()->sqlColumnForField($this->dataClass(), $this->getForeignKey());
         if (is_array($id)) {
-            return ["$key IN (" . DB::placeholders($id) . ")"  => $id];
+            $in = $this->prepareForeignIDsForWhereInClause($id);
+            $vals = str_contains($in, '?') ? $id : [];
+            return ["$key IN ($in)" => $vals];
         }
         if ($id !== null) {
             return [$key => $id];

src/ORM/ManyManyList.php

@@ -177,7 +177,9 @@ protected function foreignIDFilter($id = null)
         // Apply relation filter
         $key = "\"{$this->joinTable}\".\"{$this->foreignKey}\"";
         if (is_array($id)) {
-            return ["$key IN (" . DB::placeholders($id) . ")"  => $id];
+            $in = $this->prepareForeignIDsForWhereInClause($id);
+            $vals = str_contains($in, '?') ? $id : [];
+            return ["$key IN ($in)" => $vals];
         }
         if ($id !== null) {
             return [$key => $id];

src/ORM/RelationList.php

@@ -4,6 +4,7 @@
 
 use Exception;
 use Sminnee\CallbackList\CallbackList;
+use SilverStripe\ORM\DB;
 
 /**
  * A DataList that represents a relation.
@@ -159,4 +160,29 @@ public function forForeignID($id)
      * @return array Condition In array(SQL => parameters format)
      */
     abstract protected function foreignIDFilter($id = null);
+
+    /**
+     * Prepare an array of IDs for a 'WHERE IN` clause deciding if we should use placeholders
+     * Current rules are to use not use placeholders, unless:
+     * - SilverStripe\ORM\DataList.use_placeholders_for_integer_ids is set to false, or
+     * - Any of the IDs values being filtered are not integers or valid integer strings
+     *
+     * Putting IDs directly into a where clause instead of using placeholders was measured to be significantly
+     * faster when querying a large number of IDs e.g. over 1000
+     */
+    protected function prepareForeignIDsForWhereInClause(array $ids): string
+    {
+        if ($this->config()->get('use_placeholders_for_integer_ids')) {
+            return DB::placeholders($ids);
+        }
+        // Validate that we're only using int ID's for the IDs
+        // We need to do this to protect against SQL injection
+        foreach ($ids as $id) {
+            if (!ctype_digit((string) $id) || $id != (int) $id) {
+                return DB::placeholders($ids);
+            }
+        }
+        // explicitly including space after comma to match the default for DB::placeholders
+        return implode(', ', $ids);
+    }
 }

src/Security/Member_GroupSet.php

@@ -54,8 +54,9 @@ public function foreignIDFilter($id = null)
 
         // Add a filter to this DataList
         if (!empty($allGroupIDs)) {
-            $allGroupIDsPlaceholders = DB::placeholders($allGroupIDs);
-            return ["\"Group\".\"ID\" IN ($allGroupIDsPlaceholders)" => $allGroupIDs];
+            $in = $this->prepareForeignIDsForWhereInClause($allGroupIDs);
+            $vals = str_contains($in, '?') ? $allGroupIDs : [];
+            return ["\"Group\".\"ID\" IN ($in)" => $vals];
         }
 
         return ['"Group"."ID"' => 0];

tests/php/ORM/Filters/ExactMatchFilterTest.php

@@ -7,6 +7,7 @@
 use SilverStripe\ORM\Filters\ExactMatchFilter;
 use SilverStripe\ORM\Tests\Filters\ExactMatchFilterTest\Task;
 use SilverStripe\ORM\Tests\Filters\ExactMatchFilterTest\Project;
+use SilverStripe\ORM\DataList;
 
 class ExactMatchFilterTest extends SapphireTest
 {
@@ -22,7 +23,7 @@ class ExactMatchFilterTest extends SapphireTest
      */
     public function testUsePlaceholders(?bool $expectedID, ?bool $expectedTitle, bool $config, callable $fn): void
     {
-        Config::modify()->set(ExactMatchFilter::class, 'use_placeholders_for_integer_ids', $config);
+        Config::modify()->set(DataList::class, 'use_placeholders_for_integer_ids', $config);
         [$idQueryUsesPlaceholders, $titleQueryUsesPlaceholders] = $this->usesPlaceholders($fn);
         $this->assertSame($expectedID, $idQueryUsesPlaceholders);
         $this->assertSame($expectedTitle, $titleQueryUsesPlaceholders);

tests/php/ORM/HasManyListTest.php

@@ -8,6 +8,8 @@
 use SilverStripe\ORM\Tests\HasManyListTest\Company;
 use SilverStripe\ORM\Tests\HasManyListTest\CompanyCar;
 use SilverStripe\ORM\Tests\HasManyListTest\Employee;
+use SilverStripe\Core\Config\Config;
+use SilverStripe\ORM\DataList;
 
 class HasManyListTest extends SapphireTest
 {
@@ -189,4 +191,52 @@ public function testRemoveCallbackOnRemoveById()
         $relation->removeByID($remove->ID);
         $this->assertEquals([$remove->ID], $removedIds);
     }
+
+    /**
+     * @dataProvider provideForForeignIDPlaceholders
+     */
+    public function testForForeignIDPlaceholders(bool $config, bool $useInt, bool $expected): void
+    {
+        Config::modify()->set(DataList::class, 'use_placeholders_for_integer_ids', $config);
+        $team1 = $this->objFromFixture(Team::class, 'team1');
+        $team2 = $this->objFromFixture(Team::class, 'team2');
+        $comments1 = $team1->Comments();
+        $comments2 = $team2->Comments();
+        $ids = $useInt ? [$team1->ID, $team2->ID] : ['Lorem', 'Ipsum'];
+        $newCommentsList = $comments1->forForeignID($ids);
+        $sql = $newCommentsList->dataQuery()->sql();
+        preg_match('#ID" IN \(([^\)]+)\)\)#', $sql, $matches);
+        $usesPlaceholders = $matches[1] === '?, ?';
+        $this->assertSame($expected, $usesPlaceholders);
+        $expectedIDs = $useInt
+            ? array_values(array_merge($comments1->column('ID'), $comments2->column('ID')))
+            : [];
+        $this->assertSame($expectedIDs, $newCommentsList->column('ID'));
+    }
+
+    public function provideForForeignIDPlaceholders(): array
+    {
+        return [
+            'config false' => [
+                'config' => false,
+                'useInt' => true,
+                'expected' => false,
+            ],
+            'config false non-int' => [
+                'config' => false,
+                'useInt' => false,
+                'expected' => true,
+            ],
+            'config true' => [
+                'config' => true,
+                'useInt' => true,
+                'expected' => true,
+            ],
+            'config true non-int' => [
+                'config' => true,
+                'useInt' => false,
+                'expected' => true,
+            ],
+        ];
+    }
 }

tests/php/ORM/ManyManyListTest.php

@@ -13,6 +13,7 @@
 use SilverStripe\ORM\Tests\DataObjectTest\Team;
 use SilverStripe\ORM\Tests\ManyManyListTest\ExtraFieldsObject;
 use SilverStripe\ORM\Tests\ManyManyListTest\Product;
+use SilverStripe\ORM\DataList;
 
 class ManyManyListTest extends SapphireTest
 {
@@ -615,4 +616,52 @@ public function testRemoveCallbackOnRemoveAll()
         $relation->removeAll();
         $this->assertEquals(sort($remove), sort($removedIds));
     }
+
+    /**
+     * @dataProvider provideForForeignIDPlaceholders
+     */
+    public function testForForeignIDPlaceholders(bool $config, bool $useInt, bool $expected): void
+    {
+        Config::modify()->set(DataList::class, 'use_placeholders_for_integer_ids', $config);
+        $team1 = $this->objFromFixture(Team::class, 'team1');
+        $team2 = $this->objFromFixture(Team::class, 'team2');
+        $players1 = $team1->Players();
+        $players2 = $team2->Players();
+        $ids = $useInt ? [$team1->ID, $team2->ID] : ['Lorem', 'Ipsum'];
+        $newPlayersList = $players1->forForeignID($ids);
+        $sql = $newPlayersList->dataQuery()->sql();
+        preg_match('#ID" IN \(([^\)]+)\)\)#', $sql, $matches);
+        $usesPlaceholders = $matches[1] === '?, ?';
+        $this->assertSame($expected, $usesPlaceholders);
+        $expectedIDs = $useInt
+            ? array_values(array_merge($players1->column('ID'), $players2->column('ID')))
+            : [];
+        $this->assertEqualsCanonicalizing($expectedIDs, $newPlayersList->column('ID'));
+    }
+
+    public function provideForForeignIDPlaceholders(): array
+    {
+        return [
+            'config false' => [
+                'config' => false,
+                'useInt' => true,
+                'expected' => false,
+            ],
+            'config false non-int' => [
+                'config' => false,
+                'useInt' => false,
+                'expected' => true,
+            ],
+            'config true' => [
+                'config' => true,
+                'useInt' => true,
+                'expected' => true,
+            ],
+            'config true non-int' => [
+                'config' => true,
+                'useInt' => false,
+                'expected' => true,
+            ],
+        ];
+    }
 }

tests/php/ORM/ManyManyThroughListTest.php

@@ -15,6 +15,7 @@
 use SilverStripe\ORM\Tests\ManyManyThroughListTest\Locale;
 use SilverStripe\ORM\Tests\ManyManyThroughListTest\FallbackLocale;
 use SilverStripe\ORM\Tests\ManyManyThroughListTest\TestObject;
+use SilverStripe\ORM\DataList;
 
 class ManyManyThroughListTest extends SapphireTest
 {
@@ -514,4 +515,52 @@ public function testRemoveCallbackOnRemoveAll()
         $relation->removeAll();
         $this->assertEquals(sort($remove), sort($removedIds));
     }
+
+    /**
+     * @dataProvider provideForForeignIDPlaceholders
+     */
+    public function testForForeignIDPlaceholders(bool $config, bool $useInt, bool $expected): void
+    {
+        Config::modify()->set(DataList::class, 'use_placeholders_for_integer_ids', $config);
+        $parent1 = $this->objFromFixture(ManyManyThroughListTest\TestObject::class, 'parent1');
+        $parent2 = $this->objFromFixture(ManyManyThroughListTest\TestObject::class, 'parent2');
+        $items1 = $parent1->Items();
+        $items2 = $parent2->Items();
+        $ids = $useInt ? [$parent1->ID, $parent2->ID] : ['Lorem', 'Ipsum'];
+        $newItemsList = $items1->forForeignID($ids);
+        $sql = $newItemsList->dataQuery()->sql();
+        preg_match('#ID" IN \(([^\)]+)\)\)#', $sql, $matches);
+        $usesPlaceholders = $matches[1] === '?, ?';
+        $this->assertSame($expected, $usesPlaceholders);
+        $expectedIDs = $useInt
+            ? array_values(array_merge($items1->column('ID'), $items2->column('ID')))
+            : [];
+        $this->assertEqualsCanonicalizing($expectedIDs, $newItemsList->column('ID'));
+    }
+
+    public function provideForForeignIDPlaceholders(): array
+    {
+        return [
+            'config false' => [
+                'config' => false,
+                'useInt' => true,
+                'expected' => false,
+            ],
+            'config false non-int' => [
+                'config' => false,
+                'useInt' => false,
+                'expected' => true,
+            ],
+            'config true' => [
+                'config' => true,
+                'useInt' => true,
+                'expected' => true,
+            ],
+            'config true non-int' => [
+                'config' => true,
+                'useInt' => false,
+                'expected' => true,
+            ],
+        ];
+    }
 }

tests/php/Security/Member_GroupSetTest.php

@@ -0,0 +1,65 @@
+<?php
+
+namespace SilverStripe\Security\Tests;
+
+use SilverStripe\Dev\SapphireTest;
+use SilverStripe\Security\Tests\GroupTest\TestMember;
+use SilverStripe\Core\Config\Config;
+use SilverStripe\ORM\DataList;
+
+class Member_GroupSetTest extends SapphireTest
+{
+    protected static $fixture_file = 'GroupTest.yml';
+
+    protected static $extra_dataobjects = [
+        TestMember::class
+    ];
+
+    /**
+     * @dataProvider provideForForeignIDPlaceholders
+     */
+    public function testForForeignIDPlaceholders(bool $config, bool $useInt, bool $expected): void
+    {
+        Config::modify()->set(DataList::class, 'use_placeholders_for_integer_ids', $config);
+        $member1 = $this->objFromFixture(TestMember::class, 'parentgroupuser');
+        $member2 = $this->objFromFixture(TestMember::class, 'allgroupuser');
+        $groups1 = $member1->Groups();
+        $groups2 = $member2->Groups();
+        $ids = $useInt ? [$member1->ID, $member2->ID] : ['Lorem', 'Ipsum'];
+        $newGroupList = $groups1->forForeignID($ids);
+        $sql = $newGroupList->dataQuery()->sql();
+        preg_match('#ID" IN \(([^\)]+)\)\)#', $sql, $matches);
+        $usesPlaceholders = ($matches[1] ?? '') === '?, ?, ?, ?, ?' || str_contains($sql, '"Group"."ID" = ?');
+        $this->assertSame($expected, $usesPlaceholders);
+        $expectedIDs = $useInt
+            ? array_unique(array_merge($groups1->column('ID'), $groups2->column('ID')))
+            : [];
+        $this->assertEqualsCanonicalizing($expectedIDs, $newGroupList->column('ID'));
+    }
+
+    public function provideForForeignIDPlaceholders(): array
+    {
+        return [
+            'config false' => [
+                'config' => false,
+                'useInt' => true,
+                'expected' => false,
+            ],
+            'config false non-int' => [
+                'config' => false,
+                'useInt' => false,
+                'expected' => true,
+            ],
+            'config true' => [
+                'config' => true,
+                'useInt' => true,
+                'expected' => true,
+            ],
+            'config true non-int' => [
+                'config' => true,
+                'useInt' => false,
+                'expected' => true,
+            ],
+        ];
+    }
+}