diff --git a/src/ORM/DataObject.php b/src/ORM/DataObject.php index 4e91d1bda24..3783b92f86d 100644 --- a/src/ORM/DataObject.php +++ b/src/ORM/DataObject.php @@ -3844,7 +3844,7 @@ public function requireTable() } // Build index list - $manymanyIndexes = [ + $manyManyIndexes = [ $parentField => [ 'type' => 'index', 'name' => $parentField, @@ -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, + // 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); } } diff --git a/src/ORM/DataObjectSchema.php b/src/ORM/DataObjectSchema.php index 7d7b66f650f..f30ee2deaa8 100644 --- a/src/ORM/DataObjectSchema.php +++ b/src/ORM/DataObjectSchema.php @@ -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 */ public function tableName($class) { @@ -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 + { + $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 ?? '')); + $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') { + 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; } diff --git a/tests/php/ORM/ManyManyListTest.php b/tests/php/ORM/ManyManyListTest.php index 20cb288bcd6..a1644c570c5 100644 --- a/tests/php/ORM/ManyManyListTest.php +++ b/tests/php/ORM/ManyManyListTest.php @@ -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 { @@ -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()); + } + } + public function testFilteringOnPreviouslyJoinedTable() { /** @var ManyManyListTest\Category $category */