Skip to content

feat(plugins)!: order SQL exports by foreign key dependency and report what the order cannot fix (#2517) - #2607

Merged
datlechin merged 1 commit into
mainfrom
feat/sql-export-fk-ordering
Sep 2, 2026
Merged

feat(plugins)!: order SQL exports by foreign key dependency and report what the order cannot fix (#2517)#2607
datlechin merged 1 commit into
mainfrom
feat/sql-export-fk-ordering

Conversation

@datlechin

Copy link
Copy Markdown
Member

Fixes #2517.

What was already there

The topological sort the issue asks for has been in the SQL export since #1126: SQLExportPlugin.topologicallySort runs every export through ForeignKeyTopologicalSort, the CREATE and data phases follow that order, and the drop phase walks it backwards. So item 1 of the issue and item 3's ALTER TABLE … ADD CONSTRAINT were both in place. What was missing is item 2, and one thing the issue could not have known about.

Root cause

Two holes, both variations on the export being unable to tell you it wrote something questionable.

The sort cannot report a cycle. ForeignKeyTopologicalSort.ordered returns [Table], so a caller cannot tell a parent-first order from a partial one. On a cycle it silently appended the tables it could not place in alphabetical identifier order, discarding the order the export tree gave them.

There is no export summary to report it in. ExportFormatResult.warnings reaches ExportService.state.warningMessage and stops there: nothing reads it, and TransferResultAlert.presentExportSuccess took no result and showed a fixed "Export completed". Every warning the SQL export already produced (a DDL fetch that failed, a metadata fetch that failed, an export spanning two schemas) has been invisible since it was written. The import side already does this properly through presentImportSuccess(result:).

And item 3's premise does not hold. writeFinalizationPhase emitted ALTER TABLE … ADD CONSTRAINT for every foreign key regardless of engine, while most drivers' fetchTableDDL hands back the server's own CREATE TABLE, which already declares them:

  • MySQL and MariaDB: SHOW CREATE TABLE carries CONSTRAINT … FOREIGN KEY, so the import fails with error 1826, duplicate foreign key constraint name
  • SQL Server: MSSQLPluginDriver+Schema.swift appends the same clauses itself
  • SQLite, libSQL, Cloudflare D1: sqlite_master.sql keeps the inline REFERENCES, and SQLite has no ALTER TABLE … ADD CONSTRAINT at all, so the statement is a syntax error
  • DuckDB, Snowflake, CockroachDB, Redshift: same, from duckdb_tables(), GET_DDL, and SHOW CREATE TABLE / SHOW TABLE

PostgreSQL and Oracle are the two whose builders leave foreign keys out. PostgreSQL's constraint query filters con.contype IN ('p', 'u', 'c') and Oracle's emits columns only, which is exactly why the phase exists.

Verified with a probe rather than from the docs:

$ sqlite3 probe.db "SELECT sql FROM sqlite_master WHERE name='orders';"
CREATE TABLE orders (id INTEGER PRIMARY KEY, customer_id INTEGER REFERENCES customers(id))

$ sqlite3 probe.db 'ALTER TABLE "orders" ADD CONSTRAINT "fk_orders_0" FOREIGN KEY ("customer_id") REFERENCES "customers" ("id");'
Parse error: near "FOREIGN": syntax error

The fix

PluginKit. ForeignKeyTopologicalSort gains Ordering and order(_:foreignKeysByTable:childrenFirst:), which returns the ordering alongside the tables a cycle left unorderable. ordered(...) keeps its exact signature and delegates, so the existing symbol is untouched.

Kahn's algorithm cannot answer this on its own, because the set of tables it fails to place also holds every table that merely descends from a cycle. With orders and customers referencing each other and audit referencing orders, all three come back unplaced, so a warning built on that set names audit as part of a cycle it is not in, and the fallback can put audit before its own parent. The sort now groups tables into strongly connected components with an iterative Tarjan pass and orders the component graph instead. Only a component holding more than one table is unorderable; its members keep their input order, and a descendant is its own component that still lands after the cycle.

The capability. PluginDatabaseDriver.tableDDLIncludesForeignKeys says whether fetchTableDDL already declares them. It defaults to false, which is what every driver shipped before this existed, so a plugin that is not rebuilt behaves exactly as it does today. The nine drivers listed above override it to true, and PluginExportDataSource mirrors it for the export side. writeFinalizationPhase then runs only where the DDL leaves foreign keys out.

Redshift needed one more thing to be honest about the flag: its SHOW TABLE path declares foreign keys and its hand-built fallback did not, so the fallback now emits them too.

The summary. ExportState.warnings replaces warningMessage, presentExportSuccess takes the warnings and mirrors the import alert ("Export completed with warnings", .warning style, the text in informativeText), and the suppression checkbox is offered only on a clean run, because the alert a user turned off is the routine one. ExportDialog shows the alert even when the success dialog is suppressed, if there is something to say.

The cycle itself is reported twice: as a warning in that summary, and as a -- note near the top of the dump, so the file still says so after the dialog is gone. The warning states what the file is rather than prescribing a remedy, because foreignKeyDisableStatements() is nil on SQL Server, Oracle, Snowflake and DuckDB: telling every user to import with the checks off would be wrong on the engines that cannot turn them off. All four of the export's warnings are localized while they are here, since this is the change that first puts them in front of a user.

PluginKit ABI 21

tableDDLIncludesForeignKeys is a new protocol requirement. It has a default, so an already-built plugin keeps loading, but a plugin rebuilt against it hard-references the new method descriptor and the default-implementation symbol, and would fail Bundle.loadAndReturnError in an older app. validateBundleVersions only rejects declared > current, so without a bump that plugin is accepted and its driver then vanishes. currentPluginKitVersion goes to 21 with minimumCompatiblePluginKitVersion left at 19, so an older app refuses a new plugin cleanly instead. Every plugin Info.plist is bumped to match. This is the same reasoning that shipped PluginKit 20 in #2597.

CLAUDE.md:120-124 still says an additive requirement needs no bump. That is true only old-plugin-in-new-host, and it is worth correcting separately.

scripts/release-all-plugins.sh 21 has to run before or with the app release, or users on the new app hit noCompatibleBinary until the registry catches up. It has not been run.

Verification

Step Result
generate PASS
build (app) PASS
test PASS, 61 executed and 61 passed across 10 suites, then 37 and 16 on the re-runs
lint (12 paths) PASS, 0 violations
docs PASS
abi origin/main additive: 14 additions, 0 removals, ordered untouched
plugins (aggregate) blocked before it reached this branch's code, see below

AllPlugins fails locally on the vendored oracle-nio fork, macro expansion @TaskLocal:1:2: error: unknown attribute 'usableFromInlinenonisolated' in target OracleNIO, which is a toolchain incompatibility that stops the aggregate for any change under Plugins/. Every plugin target this branch touches was built on its own instead, and all ten pass: TableProPluginKit, SQLExport, MySQLDriver, PostgreSQLDriver, SQLiteDriver, CloudflareD1DriverPlugin, DuckDBDriver, LibSQLDriverPlugin, MSSQLDriver, SnowflakeDriverPlugin. CI runs the aggregate on its own toolchain.

One lint finding is pre-existing and not from this branch: CLAUDE.md:220 references AXCell, a symbol in no Swift source. CLAUDE.md is not in this diff.

New SQLExportForeignKeyOrderTests covers parent-before-child creation, child-before-parent drops, the cycle warning in both the summary and the dump, the input-order fallback, a descendant of a cycle not being reported as part of it, a silent acyclic run, and both sides of the capability gate. SQLExportPlugin is added to the test target's sources, which is what makes those runnable at all. ForeignKeyTopologicalSortTests gains three cases for cycle membership. The export tests snapshot and restore the plugin's stored settings, because SQLExportPlugin() reads the app's real defaults and a developer with gzip enabled would otherwise get a compressed file the assertions cannot read.

A second model reviewed the diff: codex review, which raised six findings. Four are fixed here (the cycle-vs-descendant set, Redshift swallowing a failed foreign key lookup behind try?, the warning prescribing a remedy some engines lack, and the test reading real preferences), one is extended beyond what it asked (localizing all four warnings, not only the new one), and one is declined with its reason in the section above.

No UI automation: the export flow needs a live database connection and a save panel, so it does not run deterministically under TableProUITests.

No screenshots: the visible change is the text of an NSAlert that only appears after a real export against a real server.

https://claude.ai/code/session_01S9ckdzeugurfqNmDpGU2M7

@mintlify

mintlify Bot commented Sep 2, 2026

Copy link
Copy Markdown

Preview deployment for your docs. Learn more about Mintlify Previews.

Project Status Preview Updated (UTC)
TablePro 🟢 Ready View Preview Sep 2, 2026, 10:32 AM

💡 Tip: Enable Workflows to automatically generate PRs for you.

@chatgpt-codex-connector

Copy link
Copy Markdown

You have reached your Codex usage limits for code reviews. You can see your limits in the Codex usage dashboard.

@datlechin
datlechin merged commit 03e9f58 into main Sep 2, 2026
14 checks passed
@datlechin
datlechin deleted the feat/sql-export-fk-ordering branch September 2, 2026 11:28
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.

Order SQL exports by foreign key dependency

1 participant