Skip to content

ENH Add index for default_sort on many_many tables #11785

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Merged
emteknetnz merged 1 commit into from
Jul 22, 2025
Merged

ENH Add index for default_sort on many_many tables #11785

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
30 changes: 28 additions & 2 deletions src/ORM/DataObject.php
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -3844,7 +3844,7 @@ public function requireTable()
}

// Build index list
$manymanyIndexes = [
$manyManyIndexes = [
$parentField => [
'type' => 'index',
'name' => $parentField,
Expand All @@ -3856,7 +3856,33 @@ public function requireTable()
'columns' => [$childField],
],
];
DB::require_table($tableOrClass, $manymanyFields, $manymanyIndexes, true, null, $extensions);
// Add index for sort. MySQL (and probably others) can only use a single index at a time,
Comment thread
GuySartorelli marked this conversation as resolved.
// so instead of adding a separate index for sort, add the relevant columns to the parent and
// child indexes.
$joinSort = Config::inst()->get($tableOrClass, 'default_sort');
if (is_string($joinSort) || is_array($joinSort)) {
$sortIndex = $schema->deriveIndexFromSort(
$tableOrClass,
array_keys($manymanyFields),
$joinSort,
DataObjectSchema::SORT_INDEX_MODE_COMPOSITE
);
if (isset($sortIndex['default_sort_composite'])) {
// We don't want to have the same column listed twice
$newParentCols = $sortIndex['default_sort_composite']['columns'];
if (str_starts_with($newParentCols[0], $parentField . ' ')) {
// Remove the reference without a direction - this allows for e.g. ParentID DESC in default sort
unset($manyManyIndexes[$parentField]['columns'][0]);
}
$manyManyIndexes[$parentField]['columns'] = array_merge($manyManyIndexes[$parentField]['columns'], $newParentCols);
$newChildCols = $sortIndex['default_sort_composite']['columns'];
if (str_starts_with($newChildCols[0], $childField . ' ')) {
unset($newChildCols[0]);
}
$manyManyIndexes[$childField]['columns'] = array_merge($manyManyIndexes[$childField]['columns'], $newChildCols);
}
}
DB::require_table($tableOrClass, $manymanyFields, $manyManyIndexes, true, null, $extensions);
}
}

Expand Down
121 changes: 72 additions & 49 deletions src/ORM/DataObjectSchema.php
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -167,7 +167,7 @@ public function sqlColumnForField($class, $field, $tablePrefix = null)
*
* @param string $class
*
* @return string Returns the table name, or null if there is no table
* @return string|null Returns the table name, or null if there is no table
Comment thread
GuySartorelli marked this conversation as resolved.
*/
public function tableName($class)
{
Expand Down Expand Up @@ -713,67 +713,90 @@ protected function buildCustomDatabaseIndexes($class)

protected function buildSortDatabaseIndexes($class)
{
$indexMode = $this->getSortIndexMode($class);
if ($indexMode === DataObjectSchema::SORT_INDEX_MODE_NONE) {
$sort = Config::inst()->get($class, 'default_sort', Config::UNINHERITED);
if (!is_string($sort) && !is_array($sort)) {
return [];
}
return $this->deriveIndexFromSort(
DataObjectSchema::tableName($class) ?? '',
array_keys($this->databaseFields($class, false)),
$sort,
$this->getSortIndexMode($class)
);
}

/**
* Derive the index spec for default_sort, e.g. for a DataObject table or for a many_many join table.
*/
public function deriveIndexFromSort(string $tableName, array $fieldNames, string|array $sort, string $indexMode): array
Copy link
Copy Markdown
Member Author

@GuySartorelli GuySartorelli Jul 17, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Logic for this method is just all pulled out of buildSortDatabaseIndexes() so we can reuse it in DataObject for many_many join tables

{
$indexModes = [
DataObjectSchema::SORT_INDEX_MODE_NONE,
DataObjectSchema::SORT_INDEX_MODE_BOTH,
DataObjectSchema::SORT_INDEX_MODE_COMPOSITE,
DataObjectSchema::SORT_INDEX_MODE_SINGLE,
];
if (!in_array($indexMode, $indexModes)) {
throw new InvalidArgumentException('$indexMode must be one of the DataObjectSchema::SORT_INDEX_MODE_* constant values');
}

if (empty($sort) || $indexMode === DataObjectSchema::SORT_INDEX_MODE_NONE) {
return [];
}

$shouldAddToComposite = in_array($indexMode, [DataObjectSchema::SORT_INDEX_MODE_BOTH, DataObjectSchema::SORT_INDEX_MODE_COMPOSITE]);
$shouldAddToSingle = in_array($indexMode, [DataObjectSchema::SORT_INDEX_MODE_BOTH, DataObjectSchema::SORT_INDEX_MODE_SINGLE]);
$sort = Config::inst()->get($class, 'default_sort', Config::UNINHERITED);
$compositeCols = [];
$indexes = [];

if ($sort && (is_string($sort) || is_array($sort))) {
$sort = $this->normaliseSort($sort);
$compositeCols = [];
foreach ($sort as $value) {
try {
list ($table, $column, $dir) = $this->parseSortColumn(trim($value ?? ''));
$table = trim($table ?? '', '"');
$column = trim($column ?? '', '"');
// Skip and stop grabbing composite columns if the sort column is on a different table
if ($table && strtolower($table ?? '') !== strtolower(DataObjectSchema::tableName($class) ?? '')) {
$shouldAddToComposite = false;
continue;
}
// ID is always the primary key, so we don't need a new index for it.
if ($column === 'ID') {
if ($shouldAddToComposite) {
// We still need to include it in the composite index if it's part-way through
$compositeCols[$column] = "$column $dir";
}
continue;
}
// Skip and stop grabbing composite columns if this isn't a column in the database.
if (!$this->databaseField($class, $column, false)) {
$shouldAddToComposite = false;
continue;
}
// Add indexes as appropriate
if ($shouldAddToSingle) {
$indexes[$column] = [
'type' => 'index',
'columns' => [$column],
];
}
$sort = $this->normaliseSort($sort);
foreach ($sort as $value) {
try {
list ($table, $column, $dir) = $this->parseSortColumn(trim($value ?? ''));
Comment thread
GuySartorelli marked this conversation as resolved.
$table = trim($table ?? '', '"');
$column = trim($column ?? '', '"');
// Skip and stop grabbing composite columns if the sort column is on a different table
if ($table && strtolower($table ?? '') !== strtolower($tableName)) {
$shouldAddToComposite = false;
continue;
}
// ID is always the primary key, so we don't need a new index for it.
if ($column === 'ID') {
if ($shouldAddToComposite) {
// We still need to include it in the composite index if it's part-way through
$compositeCols[$column] = "$column $dir";
}
} catch (InvalidArgumentException $e) {
continue;
}
// Skip and stop grabbing composite columns if this isn't a column in the database.
if (!in_array($column, $fieldNames)) {
$shouldAddToComposite = false;
continue;
}
// Add indexes as appropriate
if ($shouldAddToSingle) {
$indexes[$column] = [
'type' => 'index',
'columns' => [$column],
];
}
if ($shouldAddToComposite) {
$compositeCols[$column] = "$column $dir";
}
} catch (InvalidArgumentException $e) {
}
}

// If ID is last, we can omit it since that gets implicitly added to all indexes
if (array_key_last($compositeCols) === 'ID') {
unset($compositeCols['ID']);
}
// Add a composite index if we either didn't already add a single column or have multiple columns.
if (!empty($compositeCols) && (!$shouldAddToSingle || count($compositeCols) > 1)) {
$indexes['default_sort_composite'] = [
'type' => 'index',
'columns' => array_values($compositeCols),
];
}
// If ID is last, we can omit it since that gets implicitly added to all indexes
if (array_key_last($compositeCols) === 'ID') {
Comment thread
GuySartorelli marked this conversation as resolved.
unset($compositeCols['ID']);
}
// Add a composite index if we either didn't already add a single column or have multiple columns.
if (!empty($compositeCols) && (!$shouldAddToSingle || count($compositeCols) > 1)) {
$indexes['default_sort_composite'] = [
'type' => 'index',
'columns' => array_values($compositeCols),
];
}
return $indexes;
}
Expand Down
68 changes: 68 additions & 0 deletions tests/php/ORM/ManyManyListTest.php
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -17,6 +17,7 @@
use PHPUnit\Framework\Attributes\DataProvider;
use SilverStripe\Core\Injector\Injector;
use SilverStripe\Dev\CliDebugView;
use SilverStripe\ORM\DB;

class ManyManyListTest extends SapphireTest
{
Expand Down Expand Up @@ -718,6 +719,73 @@ public function testSortByExtraFieldsDefaultSort()
$this->assertEquals('A', $reverseSecond->Reference);
}

public static function provideDefaultSortIndexes(): array
{
return [
'string sort' => [
'defaultSort' => 'Reference ASC',
'newParentColumns' => ['Reference ASC'],
'newChildColumns' => ['Reference ASC'],
],
'array sort' => [
'defaultSort' => ['Reference' => 'DESC'],
'newParentColumns' => ['Reference DESC'],
'newChildColumns' => ['Reference DESC'],
],
'parent column not duplicated' => [
'defaultSort' => ['ManyManyListTest_ExtraFieldsID', 'Reference' => 'DESC'],
'newParentColumns' => ['Reference DESC'],
'newChildColumns' => ['ManyManyListTest_ExtraFieldsID ASC', 'Reference DESC'],
],
'child column not duplicated' => [
'defaultSort' => ['ChildID' => 'DESC', 'Reference'],
'newParentColumns' => ['ChildID DESC', 'Reference ASC'],
'newChildColumns' => ['Reference ASC'],
],
];
}

#[DataProvider('provideDefaultSortIndexes')]
public function testDefaultSortIndexes(string|array $defaultSort, array $newParentColumns, array $newChildColumns): void
{
$expectedParentSpec = [
'name' => 'ManyManyListTest_ExtraFieldsID',
'columns' => ['ManyManyListTest_ExtraFieldsID ASC'],
'type' => 'index',
];
$expectedChildSpec = [
'name' => 'ChildID',
'columns' => ['ChildID ASC'],
'type' => 'index',
];

// Indexes should only have a single column each by default
$indexes = DB::get_schema()->indexList('ManyManyListTest_ExtraFields_Clients');
// Use assertEqualsCanonicalizing because the order doesn't matter
// and the indexes in the `columns` array are different.
$this->assertEqualsCanonicalizing($expectedParentSpec, $indexes['ManyManyListTest_ExtraFieldsID']);
$this->assertEqualsCanonicalizing($expectedChildSpec, $indexes['ChildID']);

// Set default_sort config and rebuild the table
Config::inst()->set('ManyManyListTest_ExtraFields_Clients', 'default_sort', $defaultSort);
$obj = new ManyManyListTest\ExtraFieldsObject();
DB::get_schema()->schemaUpdate(fn () => $obj->requireTable());

// Index should be there now.
$expectedParentSpec['columns'] = array_merge($expectedParentSpec['columns'], $newParentColumns);
$expectedChildSpec['columns'] = array_merge($expectedChildSpec['columns'], $newChildColumns);
try {
$indexes = DB::get_schema()->indexList('ManyManyListTest_ExtraFields_Clients');
$this->assertEqualsCanonicalizing($expectedParentSpec, $indexes['ManyManyListTest_ExtraFieldsID']);
$this->assertEqualsCanonicalizing($expectedChildSpec, $indexes['ChildID']);
} finally {
// Indexes aren't included in transactions, which means they aren't reset after the test is torn down.
// Because of that, we need to reset the index manually by rebuilding the table.
Config::inst()->set('ManyManyListTest_ExtraFields_Clients', 'default_sort', null);
DB::get_schema()->schemaUpdate(fn () => $obj->requireTable());
Comment on lines +782 to +785
Copy link
Copy Markdown
Member Author

@GuySartorelli GuySartorelli Jul 17, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I've referenced this in #11786

}
}

public function testFilteringOnPreviouslyJoinedTable()
{
/** @var ManyManyListTest\Category $category */
Expand Down