Skip to content

fix(coordinator): read a foreign key's target through the scope that named it - #2802

Merged
datlechin merged 2 commits into
mainfrom
fix/foreign-key-target-reads
Sep 12, 2026
Merged

fix(coordinator): read a foreign key's target through the scope that named it#2802
datlechin merged 2 commits into
mainfrom
fix/foreign-key-target-reads

Conversation

@datlechin

Copy link
Copy Markdown
Member

Six defects in the foreign key and structure subsystem, all one class: a read or a statement resolving against whichever container the connection happens to be on, when the caller named another. Found while investigating #2769 and verified against a live MySQL 8.4.11 after both halves of that fix landed (#2797, #2798).

Two of them are regressions #2798 introduced, and they are the reason this is a PR rather than a note. A peer session found the Ref Table one and dropped its overlapping branch so this could land whole.

What was wrong

Nested chevrons in the row inspector pointed at the wrong database. ForeignKeyRowFetcher read the referenced row through a name it qualified itself, so the row was right, then read that row's own foreign keys through browseScope plus a hand-built DatabaseScope(database: origin, schema: referencedSchema). That scope shape is inert on an engine with no schema layer, so the nested read asked the sidebar's database. Where both databases had a same-named table, the crm row got a chevron that navigated into the app-side target.

Ref Columns went permanently empty after one failed read. try? await ... fetchColumns then columnCache[key] = (columns ?? []) stored a failure as "this table has no columns". Nothing retried, nothing logged, and loading: was cache == nil, so the menu showed only Custom… for the life of the tab with no error. It also read through browseScope rather than the tab's own scope, and keyed its cache on schema ?? schemaName, which is nil on a schema-less engine and collapses two databases to one entry.

Ref Table was empty on most engines. referencedTableNames read SchemaService.tables(for:schema:), whose perSchemaStates store is written only by runSchemaLoad. All four of its callers are gated on .hierarchicalSchema: SchemaRefreshService.swift:117 explicitly, and the three sidebar paths are schema-node expansions that only exist on such a tree. MySQL is .byDatabase and PostgreSQL and DuckDB are .bySchema, so that store is never written for them. The dropdown was therefore permanently empty on PostgreSQL, CockroachDB, Redshift, SQL Server and DuckDB, where the tab's schema is always set, and empty on MySQL, MariaDB and TiDB the moment a row named a Ref Schema, which is exactly the cross-database case. It worked only where both the tab schema and the row's Ref Schema are nil: SQLite, libSQL, D1.

New Trigger and Drop Trigger named the wrong database. Both built their SQL on DatabaseManager.driver(for:), the session driver, which follows the sidebar. #2798 made MySQL's createTriggerTemplate and generateDropTriggerSQL qualify with the connection's current database, so the template pre-filled ON \sidebar_db`.`customers`for a tab on another database, and Drop Trigger builtDROP TRIGGER `sidebar_db`.`trg`and then ran it on a pooled connection pinned somewhere else. Before #2798 those statements were unqualified and resolved against the executing connection, which was correct by accident.apply()already built its own drop SQL inside the lease;drop()` did not.

describe_table over MCP answered about two tables at once. It passed scope.schema to fetchColumns and dropped it for indexes, foreign keys, check constraints, row count and DDL, which fell back to the session's container. #2798 is what created the split: before it, every part read the same wrong place, so the answer was at least self-consistent. Measured on the live fixture: columns came back as id/display_name/region varchar(8) from one database while foreign_keys reported region -> tp2769_app.regions from the other, so one description had region as both an unindexed varchar and an indexed integer key.

The fix

Each read now starts from the scope its caller owns, and each statement is built on the driver that will run it.

  • ForeignKeyRowFetcher.fetch takes an origin: DatabaseScope, resolves the target through ForeignKeyTargetScope (the helper fix(datagrid): key a foreign key's target on the container its engine actually names #2797 added) and reads both the row and its nested keys through that one scope. It used to take the row off the raw session driver, and a qualified name only reaches inside the database the connection is already on, so once the tab and the sidebar drifted apart the row and its key map described two different tables. JSONRowSnapshot carries the scope instead of a bare connection id, filled from coordinator.scope(for: tab).
  • The picker and the preview now share one gridOriginScope on TableViewCoordinator. They were two expressions for one cell's target, which is how they drifted.
  • ForeignKeyReferenceMenus takes the tab's origin and a ScopedMetadataProviding seam, routes through ForeignKeyTargetScope, keys its cache on the resolved scope, and keeps failures in a separate set so a reopen retries and the menu says Couldn't read the referenced table instead of silently offering nothing. It no longer reads SchemaService at all: both lists now come from the tab's own driver, through one cache whose key carries the kind as well as the container, because a container's table list and one of its tables' column lists are both [String] and would otherwise overwrite each other.
  • TriggerEditing.drop builds its statement inside a lease on the target scope; TriggerDetailView.newTrigger reads its template the same way.
  • DatabaseDriver gains schema: overloads for fetchIndexes, fetchForeignKeys, fetchCheckConstraints, fetchApproximateRowCount, fetchTableDDL, fetchIndexDDL and fetchCommentDDL, each defaulting to the unqualified read exactly as fetchColumns already did, so no conformer changes. TableDDLComposer.fetchDDL takes a schema and passes it to all three of its reads, and MCP's describeTable passes scope.schema everywhere.

This is the app-level protocol, not PluginKit, so no ABI bump.

Verification

Step Result
verify.sh build PASS
verify.sh test (12 suites) PASS, 95 executed, 95 passed
verify.sh lint TablePro … PASS, 0 violations

Seven new cases in ForeignKeyReferenceMenusTests cover the Ref Table list coming from the driver with views excluded, the table and column lists not colliding in the cache, the tab-scope routing, a referenced schema resolving to a database on a schema-less engine, a failure being reported and retried rather than cached, a loaded list still being served from the cache, and two databases with the same table name not sharing an entry.

No UI automation: every one of these needs a live MySQL server with two databases and a cross-database foreign key, which does not run deterministically on CI.

One pre-existing lint violation sits at JSONRowSnapshotChangeTests.swift:63 (snapshot() == snapshot(), flagged as identical_operands). It is on a line this PR does not touch, and .swiftlint.yml scopes to TablePro, so CI never sees it. Left alone.

Codex reviewed the diff and found four issues; all four are addressed. One was a real defect I had left: the row read still went through the session driver while its nested keys came from the resolved scope, so the fix was half applied. The other two substantive ones were Ref Table swallowing its own failure the way Ref Columns used to, and the table list never being dropped on a schema refresh, so a created or renamed table did not appear until the tab closed. On the fourth, asking for the new /// blocks to be deleted under the no-comments rule, I trimmed the two that narrated caller history and kept the rest: that rule bans comments describing what code does, and this codebase records why a non-obvious choice was made in exactly this form.

Not in this PR

RowImportSheet reads its table list and column mapping off the unpinned session driver (:668, :674, :752, :758) while its writes lease the browse scope. Verified real against the live fixture: after following a cross-database foreign key, the tab's database and the session driver's diverge, so the sheet can map fields against one database's customers and insert into another's. Pre-existing, independent of this change, and large enough to deserve its own PR.

https://claude.ai/code/session_01SP8Cj5R28unz7YhwL2BtiM

@datlechin
datlechin merged commit e582ba0 into main Sep 12, 2026
5 checks passed
@datlechin
datlechin deleted the fix/foreign-key-target-reads branch September 12, 2026 12:20
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.

1 participant