Claude finished @Yeraze's task —— View job

PR Review: fix(migration): prevent UNIQUE constraint crash in migration 103

Summary: This is a targeted, well-scoped fix for a real crash in migration 103 (consolidate_mqtt_channels) affecting users upgrading to 4.12.0 with specific permission configurations.

Code Quality ✅

The fix is clean and idiomatic for each database backend:

• SQLite (103_consolidate_mqtt_channels.ts:68-72): Uses a sub-select within the DELETE to remove conflicting dup-side permission rows before the UPDATE. Correct approach.
• PostgreSQL (lines 163-166): Mirrors the SQLite pattern with correct quoted "userId" / "channelDatabaseId" camelCase column names. Consistent with PG dialect requirements.
• MySQL (lines 249-252): Uses the derived table workaround (AS tmp) to circumvent MySQL Error 1093. This is the standard and correct fix for this MySQL limitation.

The comments clearly explain why the DELETE is needed before the UPDATE, which is important for future maintainers.

Correctness ✅

The fix is logically sound:

1. DELETE the dup-side permission row for any user who already has a row on keep
2. UPDATE remaining dup rows (users who only had dup access) to point to keep
3. DELETE the dup channel row

This preserves the invariant that users who had access to either duplicate retain access to the canonical keeper. No permission data is lost — users on keep already have access, and users on dup (not keep) are correctly migrated.

Test Coverage ✅

The new test (lines 135-168) correctly covers the collision scenario:

• Seeds user 99 with permissions on both id=1 (keeper) and id=2 (dup) — exactly the bug scenario
• Asserts no UNIQUE constraint exception is thrown
• Asserts exactly one permission row remains, pointing to the keeper

One minor observation: the collision test creates a new in-memory DB (db2) without calling seedDb, which means it only tests the minimal schema needed for the collision. This is intentional and reasonable — it isolates the fix without depending on the broader fixture. The test is clearly documented and works correctly.

Potential Issues

Minor: The test at line 138 uses const db2 = new Database(':memory:') outside of beforeEach, so db2 won't be automatically closed/cleaned up if the test suite grows. In practice this is fine for in-memory SQLite (it's garbage collected), but it's worth noting for consistency.

No security concerns. The DELETE uses parameterized queries consistently across all three backends. No raw string concatenation or user-controlled input.

No performance concerns. The DELETE sub-select runs once per (dup, keep) pair, which is the same cardinality as the existing permission UPDATE. No new full-table scans are introduced.

Verdict

LGTM. The fix is minimal, correct, well-tested for all three database backends, and properly handles the edge case. The MySQL derived-table workaround is the right approach. The PR description also helpfully includes a manual workaround for users already stuck.