Skip to content

fix(magicmapper): prevent _uuid index collisions across dynamic tables#1612

Merged
SudoThijn merged 3 commits into
developmentfrom
fix/magic-mapper-unamed-uuid
May 22, 2026
Merged

fix(magicmapper): prevent _uuid index collisions across dynamic tables#1612
SudoThijn merged 3 commits into
developmentfrom
fix/magic-mapper-unamed-uuid

Conversation

@SudoThijn
Copy link
Copy Markdown
Contributor

@SudoThijn SudoThijn commented May 21, 2026

Summary

  • MagicMapper::createTable now emits a named CONSTRAINT ... UNIQUE (...) so MySQL/MariaDB no longer auto-name the constraint/index after the column.
  • Adds repair migration Version1Date20260521120000 to drop the bare _uuid index on every existing oc_openregister_table_*.

Bug

Installing any app (first reported with doriath) failed with:

InvalidArgumentException: Index name "_uuid" for table "oc_openregister_table_1_5"
collides with the constraint on table "oc_openregister_table_1_2".

Root cause: MagicMapper::createTable built CREATE TABLE ... , UNIQUE (`_uuid`) without naming the constraint. MySQL/MariaDB then used the column name (_uuid) as the index name. Every dynamic table accumulated a bare _uuid index alongside the correctly-prefixed {table}_uuid_idx from createTableIndexes. Nextcloud's MigrationService::ensureUniqueNamesConstraints throws fatally on duplicate index names across tables, but only at install time (isInstalling=true) — so the broken state silently accumulated and the next app:enable blew up.

Fix

  1. Name the constraint. lib/Db/MagicMapper.php — the inline UNIQUE now uses CONSTRAINT `{tableName}_{col}_uq` UNIQUE (...), with the matching double-quoted variant for PostgreSQL. New dynamic tables get a table-scoped, collision-free constraint name.
  2. Repair existing installs. lib/Migration/Version1Date20260521120000.phppostSchemaChange queries INFORMATION_SCHEMA.STATISTICS (MySQL/MariaDB) or pg_indexes (PostgreSQL) for oc_openregister_table_* tables that still carry a bare _uuid index and drops it. The correctly-prefixed {table}_uuid_idx (also unique, also on _uuid) stays in place, so column uniqueness and PG ON CONFLICT are preserved.

Test plan

  • Run repair migration against a broken DB — Dropped bare _uuid index on 9 of 9 dynamic openregister table(s).
  • INFORMATION_SCHEMA.STATISTICS shows only {table}_uuid_idx per dynamic table afterwards.
  • occ app:enable doriath succeeds (previously failed with the collision exception).
  • Verify on PostgreSQL (no PG environment available here — migration's PG path uses pg_indexes lookup + per-index DROP INDEX IF EXISTS and only matches ^_uuid(\d+)?$ names, but please confirm in a PG setup before merging).
  • Confirm a freshly-created dynamic table on MySQL shows CONSTRAINT openregister_table_X_Y_uuid_uq UNIQUE (`_uuid`) in SHOW CREATE TABLE.

…collisions

MagicMapper::createTable emitted `UNIQUE (col)` inline without a name, so
MySQL/MariaDB auto-named the constraint/index after the column. Every
dynamic `oc_openregister_table_*` ended up with an index literally called
`_uuid`, which collides across tables and trips Nextcloud's
ensureUniqueNamesConstraints check on the next app install
(InvalidArgumentException: Index name "_uuid" ... collides with the
constraint on table ...).

- Emit `CONSTRAINT `{tableName}_{col}_uq` UNIQUE (...)` (and the PG
  double-quoted variant) so the index name is table-scoped.
- Add repair migration Version1Date20260521120000 that drops the bare
  `_uuid` index on every existing dynamic table. The correctly-prefixed
  `{table}_uuid_idx` remains, so uniqueness and PG ON CONFLICT keep
  working.
@github-actions
Copy link
Copy Markdown
Contributor

Quality Report — ConductionNL/openregister @ a8eb4ff

Check PHP Vue Security License Tests
lint
phpcs
phpmd
psalm
phpstan
phpmetrics
eslint
stylelint
composer ✅ 162/162
npm ✅ 532/532
PHPUnit ⏭️
Newman ⏭️
Playwright ⏭️

Quality workflow — 2026-05-21 12:38 UTC

Download the full PDF report from the workflow artifacts.

Comment thread lib/Migration/Version1Date20260521120000.php Outdated
Comment thread lib/Migration/Version1Date20260521120000.php
Comment thread lib/Migration/Version1Date20260521120000.php
Comment thread lib/Db/MagicMapper.php
Comment thread lib/Migration/Version1Date20260521120000.php Outdated
WilcoLouwerse
WilcoLouwerse previously approved these changes May 21, 2026
Copy link
Copy Markdown
Contributor

@WilcoLouwerse WilcoLouwerse left a comment

Choose a reason for hiding this comment

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

APPROVE — Thorough review.

Fix is correct and well-scoped: names the inline UNIQUE constraint ({tableName}_{col}_uq) so MySQL/MariaDB no longer auto-name the index after the column, and pairs it with an idempotent repair migration that drops the bare _uuid index on existing broken installs. Verified locally:

  • php -l clean on both files
  • phpstan clean on PR files (99 pre-existing errors all in other files)
  • composer phpcs — 19 errors on the new migration file (18 auto-fixable via composer phpcs:fix + 1 manual on line 113)
  • ✅ Smoke-checked SQL — MySQL ALTER TABLE … DROP INDEX _uuid is correct; PG pg_indexes lookup + DROP INDEX IF EXISTS is standard; column quoting flows correctly per-platform via the existing $colName branching at MagicMapper.php:2582-2585

No blockers. Six inline comments — 3× 🟡 (phpcs, PG asymmetry, PG path unverified per your own test plan), 3× 🟢 (hardcoded oc_ prefix, redundant unique indexes).

Nothing in the 🟡 list changes the verdict, but the phpcs failure is the project's own CI signal — worth running composer phpcs:fix before merge to keep the file green.

SudoThijn added 2 commits May 22, 2026 09:34
Address PR #1612 review comment: `findAffectedTables` matched the
bare `_uuid` index exactly, while `postSchemaChange` used a wider
`LIKE '\_uuid%'` pattern. A PG table that only had `_uuid1` (the
on-collision auto-suffix) would never be surfaced for repair,
defeating the broader matching downstream.

Both queries now agree on the LIKE pattern; the strict
`^_uuid(\d+)?$` regex at drop time still gates the actual DROP.
Address PR #1612 review comment about CI phpcs failures.

- Use named parameter on internal call: `findAffectedTables(isPostgres: $isPostgres)`
- Auto-fix 18 remaining formatting violations via `phpcbf`
  (blank lines around functions, equals alignment, multi-line call
  brace placement, `//end ...` markers, equals alignment)
- Tidy continuation indent on the multi-line `$output` calls phpcbf
  reformatted

Verified locally: `vendor/bin/phpcs` on the file now reports zero
errors.
Copy link
Copy Markdown
Contributor

@WilcoLouwerse WilcoLouwerse left a comment

Choose a reason for hiding this comment

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

Re-review verdict: ✅ APPROVE (Quick mode)

Both actionable prior findings have been addressed by the new commits:

  • 🟡 phpcs CI failure (named parameter + 18 auto-fix violations) — resolved in d5b023f
  • 🟡 PG asymmetry between findAffectedTables and postSchemaChangeresolved in 9213278 (both queries now share the LIKE '\_uuid%' ESCAPE '\\' pattern; strict ^_uuid(\d+)?$ regex at drop time preserves safety)

Resolved as non-blocking (not blockers; tracked as follow-ups or verification asks):

No new issues identified on the additional diff. CI Newman API Test Suite is failing due to a pre-existing infrastructure issue (config.platform PHP 8.1 vs composer.json requiring ^8.3) unrelated to this PR — not a required check per the Development ruleset.

@SudoThijn SudoThijn merged commit 4926098 into development May 22, 2026
1 check failed
@SudoThijn SudoThijn deleted the fix/magic-mapper-unamed-uuid branch May 22, 2026 08:46
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