tonyair/silverstripe-framework
1
0
Fork 0
mirror of https://github.com/silverstripe/silverstripe-framework synced 2024-10-22 14:05:37 +02:00
Code Issues Projects Releases Wiki Activity

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

Browse Source 
ENH Do not use placeholders by default for foreignIDFilter()
This commit is contained in:
Guy Sartorelli 2023-08-09 11:58:26 +12:00 committed by GitHub
commit fd2b76d38b
No known key found for this signature in database
GPG Key ID: 4AEE18F83AFDEB23
11 changed files with 258 additions and 12 deletions
Show Stats Download Patch File Download Diff File Expand all files Collapse all files

5
src/ORM/DataList.php
View File

@ -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

9
src/ORM/Filters/ExactMatchFilter.php
View File

@ -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

5
src/ORM/HasManyList.php
View File

@ -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];

4
src/ORM/ManyManyList.php
View File

@ -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];

26
src/ORM/RelationList.php
View File

@ -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);
}
}

5
src/Security/Member_GroupSet.php
View File

@ -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];

3
tests/php/ORM/Filters/ExactMatchFilterTest.php
View File

@ -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);

50
tests/php/ORM/HasManyListTest.php
View File

@ -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,
],
];
}
}

49
tests/php/ORM/ManyManyListTest.php
View File

@ -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,
],
];
}
}

49
tests/php/ORM/ManyManyThroughListTest.php
View File

@ -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,
],
];
}
}

65
tests/php/Security/Member_GroupSetTest.php Normal file
View File

@ -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,
],
];
}
}