Skip to content

ENH Add index for default_sort on many_many tables#11785

Merged
emteknetnz merged 1 commit intosilverstripe:6from
creative-commoners:pulls/6/many-many-sort-index
Jul 22, 2025
Merged

ENH Add index for default_sort on many_many tables#11785
emteknetnz merged 1 commit intosilverstripe:6from
creative-commoners:pulls/6/many-many-sort-index

Conversation

@GuySartorelli
Copy link
Copy Markdown
Member

@GuySartorelli GuySartorelli commented Jul 17, 2025

default_sort on a DataObject results in an index being added. The same should happen with the join table for many_many relations.

Note that because MySQL (and probably others) can only use a single index at a time, I've opted to add the sort index columns into the existing child and parent ID indexes. This means we can filter by ID and sort by the default sort all using the same index.

If a different sort is applied, the first column (the parent or child ID) in the index can still be used for filtering.

Issue

Comment thread src/ORM/DataObjectSchema.php
/**
* 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

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

@GuySartorelli GuySartorelli force-pushed the pulls/6/many-many-sort-index branch from 4cfba15 to 4b579f8 Compare July 17, 2025 02:37
@GuySartorelli GuySartorelli marked this pull request as ready for review July 17, 2025 02:37
@GuySartorelli GuySartorelli marked this pull request as draft July 17, 2025 02:55
@GuySartorelli GuySartorelli force-pushed the pulls/6/many-many-sort-index branch from 4b579f8 to 3b8814a Compare July 17, 2025 03:48
Comment on lines +782 to +785
// 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());
Copy link
Copy Markdown
Member Author

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

@GuySartorelli GuySartorelli marked this pull request as ready for review July 17, 2025 03:51
Copy link
Copy Markdown
Member

@emteknetnz emteknetnz left a comment

Choose a reason for hiding this comment

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

Could you also rebase framework on to this? I attempted to do a kitchen sink CI run (just this PR, not any of the others) to double check this, though it complained that "Class Generated does not exist" - since there is no Generated class, instead DBGenerated, something's go wrong. I also go the same issue trying to run this PR locally.

Comment thread src/ORM/DataObject.php Outdated
Comment thread src/ORM/DataObject.php Outdated
Comment thread src/ORM/DataObject.php Outdated
Comment thread src/ORM/DataObjectSchema.php
Comment thread src/ORM/DataObject.php
Comment thread src/ORM/DataObjectSchema.php
@GuySartorelli
Copy link
Copy Markdown
Member Author

I attempted to do a kitchen sink CI run (just this PR, not any of the others) to double check this, though it complained that "Class Generated does not exist" - since there is no Generated class, instead DBGenerated, something's go wrong

Generated is the injector service used for the $db spec. But since the injector config isn't set (because, as you noted, I need to rebase), it just thinks it's a class name.
Rebasing will resolve that, so will do.

@GuySartorelli GuySartorelli force-pushed the pulls/6/many-many-sort-index branch 2 times, most recently from fc52832 to 4a1c447 Compare July 21, 2025 23:57
@GuySartorelli GuySartorelli force-pushed the pulls/6/many-many-sort-index branch from 4a1c447 to 18cf152 Compare July 22, 2025 00:06
Copy link
Copy Markdown
Member

@emteknetnz emteknetnz left a comment

Choose a reason for hiding this comment

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

I'm just having a little trouble validating this works, though it's probably more my lack on understanding rather than any thing in this PR

Seems like the DB indexes are the same whether or not this PR is present or not?

// app/MyDataObject.php
use SilverStripe\ORM\DataObject;

class MyDataObject extends DataObject
{
    private static $table_name = 'MyDataObject';

    private static $db = [
        'Foo' => 'Varchar'
    ];

    private static $many_many = [
        'MyOtherDataObjects' => MyOtherDataObject::class
    ];

    private static $default_sort = 'Foo ASC';
}
// app/MyOtherDataObject.php
use SilverStripe\ORM\DataObject;

class MyDataObject extends DataObject
{
    private static $table_name = 'MyDataObject';

    private static $db = [
        'Bar' => 'Varchar'
    ];

    private static $many_many = [
        'MyOtherDataObjects' => MyOtherDataObject::class
    ];

    private static $default_sort = 'Title ASC';
}

```text
# show indexes from MyDataObject;
+--------------+------------+-----------+--------------+-------------+-----------+-------------+----------+--------+------+------------+---------+---------------+---------+
| Table        | Non_unique | Key_name  | Seq_in_index | Column_name | Collation | Cardinality | Sub_part | Packed | Null | Index_type | Comment | Index_comment | Ignored |
+--------------+------------+-----------+--------------+-------------+-----------+-------------+----------+--------+------+------------+---------+---------------+---------+
| MyDataObject |          0 | PRIMARY   |            1 | ID          | A         |           0 |     NULL | NULL   |      | BTREE      |         |               | NO      |
| MyDataObject |          1 | Foo       |            1 | Foo         | A         |           0 |     NULL | NULL   | YES  | BTREE      |         |               | NO      |
| MyDataObject |          1 | ClassName |            1 | ClassName   | A         |           0 |     NULL | NULL   | YES  | BTREE      |         |               | NO      |
+--------------+------------+-----------+--------------+-------------+-----------+-------------+----------+--------+------+------------+---------+---------------+---------+

# show indexes from MyOtherDataObject;
+-------------------+------------+-----------+--------------+-------------+-----------+-------------+----------+--------+------+------------+---------+---------------+---------+
| Table             | Non_unique | Key_name  | Seq_in_index | Column_name | Collation | Cardinality | Sub_part | Packed | Null | Index_type | Comment | Index_comment | Ignored |
+-------------------+------------+-----------+--------------+-------------+-----------+-------------+----------+--------+------+------------+---------+---------------+---------+
| MyOtherDataObject |          0 | PRIMARY   |            1 | ID          | A         |           0 |     NULL | NULL   |      | BTREE      |         |               | NO      |
| MyOtherDataObject |          1 | Bar       |            1 | Bar         | A         |           0 |     NULL | NULL   | YES  | BTREE      |         |               | NO      |
| MyOtherDataObject |          1 | ClassName |            1 | ClassName   | A         |           0 |     NULL | NULL   | YES  | BTREE      |         |               | NO      |
+-------------------+------------+-----------+--------------+-------------+-----------+-------------+----------+--------+------+------------+---------+---------------+---------+

# show indexes from MyDataObject_MyOtherDataObjects;

+---------------------------------+------------+---------------------+--------------+---------------------+-----------+-------------+----------+--------+------+------------+---------+---------------+---------+
| Table                           | Non_unique | Key_name            | Seq_in_index | Column_name         | Collation | Cardinality | Sub_part | Packed | Null | Index_type | Comment | Index_comment | Ignored |
+---------------------------------+------------+---------------------+--------------+---------------------+-----------+-------------+----------+--------+------+------------+---------+---------------+---------+
| MyDataObject_MyOtherDataObjects |          0 | PRIMARY             |            1 | ID                  | A         |           0 |     NULL | NULL   |      | BTREE      |         |               | NO      |
| MyDataObject_MyOtherDataObjects |          1 | MyDataObjectID      |            1 | MyDataObjectID      | A         |           0 |     NULL | NULL   |      | BTREE      |         |               | NO      |
| MyDataObject_MyOtherDataObjects |          1 | MyOtherDataObjectID |            1 | MyOtherDataObjectID | A         |           0 |     NULL | NULL   |      | BTREE      |         |               | NO      |
+---------------------------------+------------+---------------------+--------------+---------------------+-----------+-------------+----------+--------+------+------------+---------+---------------+---------+

@GuySartorelli
Copy link
Copy Markdown
Member Author

GuySartorelli commented Jul 22, 2025

It's not the default sort for either model, it's for the many_many join table itself. It's documented in https://docs.silverstripe.org/en/6/developer_guides/model/relations/#:~:text=To%20ensure%20this,to%20your%20config

You need to add some YAML config (the extrafields can be in your PHP class but the sort has to be in YAML)

MyDataObject:
  many_many_extraFields:
    MyOtherDataObjects:
        SomeField: 'Int'

MyDataObject_MyOtherDataObjects:
  default_sort: 'SomeField ASC'

@GuySartorelli
Copy link
Copy Markdown
Member Author

Can you please also merge the other PRs which have been approved? None of them except the docs PR rely on this in any way.

Copy link
Copy Markdown
Member

@emteknetnz emteknetnz left a comment

Choose a reason for hiding this comment

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

Cool thanks, index now shows up with this PR

@emteknetnz emteknetnz merged commit 164c7a1 into silverstripe:6 Jul 22, 2025
12 checks passed
@emteknetnz emteknetnz deleted the pulls/6/many-many-sort-index branch July 22, 2025 05:04
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants