Skip to content

fix(migration): accept ITable from getTable() in addMailboxKey - #13459

Open
ChristophWurst wants to merge 5 commits into
mainfrom
fix/migration-gettable-itable-type
Open

fix(migration): accept ITable from getTable() in addMailboxKey#13459
ChristophWurst wants to merge 5 commits into
mainfrom
fix/migration-gettable-itable-type

Conversation

@ChristophWurst

Copy link
Copy Markdown
Member

OCP's ISchemaWrapper::getTable() now returns OCP\DB\Schema\ITable instead of Doctrine\DBAL\Schema\Table on newer server versions. Widen the addMailboxKey() parameter types to Table|ITable so the migration works on both, and suppress the not-yet-vendored ITable stub in psalm.

Assisted-by: Claude:claude-opus-4-8

Ref nextcloud/server#63013

馃 AI (if applicable)

  • The content of this PR was partly or fully generated using AI

OCP's ISchemaWrapper::getTable() now returns OCP\DB\Schema\ITable
instead of Doctrine\DBAL\Schema\Table on newer server versions. Widen
the addMailboxKey() parameter types to Table|ITable so the migration
works on both, and suppress the not-yet-vendored ITable stub in psalm.

Assisted-by: Claude:claude-opus-4-8
Signed-off-by: Christoph Wurst <1374172+ChristophWurst@users.noreply.github.com>
@ChristophWurst

Copy link
Copy Markdown
Member Author

/backport to stable5.11

@ChristophWurst

Copy link
Copy Markdown
Member Author

/backport to stable5.10

@ChristophWurst
ChristophWurst removed the request for review from GretaD August 11, 2026 07:23
@ChristophWurst

Copy link
Copy Markdown
Member Author

There are more changes necessary to make Psalm happy, but there are also legit gaps.

  1. There is no replacement to enumerate all foreign keys:
    $fks = $attachmentsTable->getForeignKeys();
  2. ModifyColumn changed in a breaking way. It's not possible to be compatible with 34 and 35 at the same time:
    $recipientsTable->modifyColumn('id', [
    'type' => Type::getType(Types::BIGINT),
    ]);

@CarlSchwan

Copy link
Copy Markdown
Member

There are more changes necessary to make Psalm happy, but there are also legit gaps.

1. There is no replacement to enumerate all foreign keys: https://github.com/nextcloud/mail/blob/1968041ebcbd120bfce97be65d40fe636428252b/lib/Migration/Version1130Date20220412111833.php#L116

This should still work, since getForeignKeys is not implemented in Table, it's forwarding the method call to the DBAL Table via the __call in OC\DB\Schema\Table

2. ModifyColumn changed in a breaking way. It's not possible to be compatible with 34 and 35 at the same time: https://github.com/nextcloud/mail/blob/1968041ebcbd120bfce97be65d40fe636428252b/lib/Migration/Version1130Date20220412111833.php#L129-L131

Same, modifyColumn in OCP\DB\Schema, has some more strict type hints but at runtime this should still work as it is just forwarding the arguments.

Signed-off-by: Christoph Wurst <1374172+ChristophWurst@users.noreply.github.com>
Signed-off-by: Christoph Wurst <1374172+ChristophWurst@users.noreply.github.com>
@CarlSchwan

Copy link
Copy Markdown
Member

Looking at the latest psalm issues, I probably went to far for requiring non empty lowercase string for index. I will change that

Signed-off-by: Christoph Wurst <1374172+ChristophWurst@users.noreply.github.com>
Signed-off-by: Christoph Wurst <1374172+ChristophWurst@users.noreply.github.com>
@nextcloud-command nextcloud-command added the AI assisted This PR contains AI-assisted commits label Aug 12, 2026
@CarlSchwan

Copy link
Copy Markdown
Member

Last psalm issue seems to be a false positive from psalm

*/
foreach ($mailboxesTable->getIndexes() as $index) {
if ($index->isUnique() && $index->spansColumns(['account_id', 'name'])) {
if ($index->isUnique() && $index->getColumns() === ['account_id', 'name']) {

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Would be nice to also expose spansColumns because it ignores the order.

As hardening we should array diff expected and actual columns and test for an empty 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.

But the other matters. An index on [a,b] is very different from [b,a].

For this specific case we only need to catch the condition where the ordering is [account_id, name], right?

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Projects

None yet

Development

Successfully merging this pull request may close these issues.

5 participants