From 358cbc9ee56ff70cd50342746082ff07c7c2bf0b Mon Sep 17 00:00:00 2001 From: Steve Boyd Date: Wed, 9 Aug 2023 10:46:08 +1200 Subject: [PATCH] ENH Do not use placeholders by default for foreignIDFilter() --- src/ORM/DataList.php | 5 ++ src/ORM/Filters/ExactMatchFilter.php | 9 +-- src/ORM/HasManyList.php | 5 +- src/ORM/ManyManyList.php | 4 +- src/ORM/RelationList.php | 26 ++++++++ src/Security/Member_GroupSet.php | 5 +- .../php/ORM/Filters/ExactMatchFilterTest.php | 3 +- tests/php/ORM/HasManyListTest.php | 50 ++++++++++++++ tests/php/ORM/ManyManyListTest.php | 49 ++++++++++++++ tests/php/ORM/ManyManyThroughListTest.php | 49 ++++++++++++++ tests/php/Security/Member_GroupSetTest.php | 65 +++++++++++++++++++ 11 files changed, 258 insertions(+), 12 deletions(-) create mode 100644 tests/php/Security/Member_GroupSetTest.php diff --git a/src/ORM/DataList.php b/src/ORM/DataList.php index e2df7893e..3e784bb6d 100644 --- a/src/ORM/DataList.php +++ b/src/ORM/DataList.php @@ -38,6 +38,11 @@ use SilverStripe\ORM\ArrayList; */ 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 diff --git a/src/ORM/Filters/ExactMatchFilter.php b/src/ORM/Filters/ExactMatchFilter.php index 4a46a8589..7b2a47453 100644 --- a/src/ORM/Filters/ExactMatchFilter.php +++ b/src/ORM/Filters/ExactMatchFilter.php @@ -4,11 +4,11 @@ namespace SilverStripe\ORM\Filters; 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 @@ use SilverStripe\ORM\FieldType\DBForeignKey; */ class ExactMatchFilter extends SearchFilter { - use Configurable; - - private static bool $use_placeholders_for_integer_ids = false; public function getSupportedModifiers() { @@ -229,12 +226,12 @@ class ExactMatchFilter extends SearchFilter * - 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 diff --git a/src/ORM/HasManyList.php b/src/ORM/HasManyList.php index 5eb2b490e..2837ccdf6 100644 --- a/src/ORM/HasManyList.php +++ b/src/ORM/HasManyList.php @@ -50,11 +50,12 @@ class HasManyList extends RelationList 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]; diff --git a/src/ORM/ManyManyList.php b/src/ORM/ManyManyList.php index 2223196d1..a2aeb4ccf 100644 --- a/src/ORM/ManyManyList.php +++ b/src/ORM/ManyManyList.php @@ -177,7 +177,9 @@ class ManyManyList extends RelationList // 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]; diff --git a/src/ORM/RelationList.php b/src/ORM/RelationList.php index 0b26d3862..dfcf8a3ea 100644 --- a/src/ORM/RelationList.php +++ b/src/ORM/RelationList.php @@ -4,6 +4,7 @@ namespace SilverStripe\ORM; use Exception; use Sminnee\CallbackList\CallbackList; +use SilverStripe\ORM\DB; /** * A DataList that represents a relation. @@ -159,4 +160,29 @@ abstract class RelationList extends DataList implements Relation * @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); + } } diff --git a/src/Security/Member_GroupSet.php b/src/Security/Member_GroupSet.php index 058dfd586..10d134b4a 100644 --- a/src/Security/Member_GroupSet.php +++ b/src/Security/Member_GroupSet.php @@ -54,8 +54,9 @@ class Member_GroupSet extends ManyManyList // 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]; diff --git a/tests/php/ORM/Filters/ExactMatchFilterTest.php b/tests/php/ORM/Filters/ExactMatchFilterTest.php index 400cd28fa..25562cc84 100644 --- a/tests/php/ORM/Filters/ExactMatchFilterTest.php +++ b/tests/php/ORM/Filters/ExactMatchFilterTest.php @@ -7,6 +7,7 @@ use SilverStripe\Dev\SapphireTest; 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); diff --git a/tests/php/ORM/HasManyListTest.php b/tests/php/ORM/HasManyListTest.php index 83714ad02..143412b10 100644 --- a/tests/php/ORM/HasManyListTest.php +++ b/tests/php/ORM/HasManyListTest.php @@ -8,6 +8,8 @@ use SilverStripe\ORM\Tests\DataObjectTest\Team; 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 @@ class HasManyListTest extends SapphireTest $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, + ], + ]; + } } diff --git a/tests/php/ORM/ManyManyListTest.php b/tests/php/ORM/ManyManyListTest.php index 9b2bfb5ca..f29856568 100644 --- a/tests/php/ORM/ManyManyListTest.php +++ b/tests/php/ORM/ManyManyListTest.php @@ -13,6 +13,7 @@ use SilverStripe\ORM\Tests\DataObjectTest\Player; 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 @@ class ManyManyListTest extends SapphireTest $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, + ], + ]; + } } diff --git a/tests/php/ORM/ManyManyThroughListTest.php b/tests/php/ORM/ManyManyThroughListTest.php index 168f762d4..5fdd1c830 100644 --- a/tests/php/ORM/ManyManyThroughListTest.php +++ b/tests/php/ORM/ManyManyThroughListTest.php @@ -15,6 +15,7 @@ use SilverStripe\ORM\Tests\ManyManyThroughListTest\PolyJoinObject; 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 @@ class ManyManyThroughListTest extends SapphireTest $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, + ], + ]; + } } diff --git a/tests/php/Security/Member_GroupSetTest.php b/tests/php/Security/Member_GroupSetTest.php new file mode 100644 index 000000000..a7992a146 --- /dev/null +++ b/tests/php/Security/Member_GroupSetTest.php @@ -0,0 +1,65 @@ +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, + ], + ]; + } +}