mirror of
https://github.com/silverstripe/silverstripe-framework
synced 2024-10-22 14:05:37 +02:00
Merge pull request #10861 from creative-commoners/pulls/5/no-placeholders
ENH Do not use placeholders for int ID filters
This commit is contained in:
Guy Sartorelli
GitHub
commit
158d51a0a5
@ -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;
|
||||
}
|
||||
}
|
||||
|
||||
95
tests/php/ORM/Filters/ExactMatchFilterTest.php
Normal file
95
tests/php/ORM/Filters/ExactMatchFilterTest.php
Normal file
@ -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];
|
||||
}
|
||||
}
|
||||
17
tests/php/ORM/Filters/ExactMatchFilterTest.yml
Normal file
17
tests/php/ORM/Filters/ExactMatchFilterTest.yml
Normal file
@ -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
|
||||
19
tests/php/ORM/Filters/ExactMatchFilterTest/Project.php
Normal file
19
tests/php/ORM/Filters/ExactMatchFilterTest/Project.php
Normal file
@ -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,
|
||||
];
|
||||
}
|
||||
19
tests/php/ORM/Filters/ExactMatchFilterTest/Task.php
Normal file
19
tests/php/ORM/Filters/ExactMatchFilterTest/Task.php
Normal file
@ -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,
|
||||
];
|
||||
}
|
||||
Loading…
Reference in New Issue
Block a user