feat(plugins)!: order SQL exports by foreign key dependency and report what the order cannot fix (#2517) - #2607
Merged
Merged
Conversation
…t what the order cannot fix (#2517) Claude-Session: https://claude.ai/code/session_01S9ckdzeugurfqNmDpGU2M7
|
Preview deployment for your docs. Learn more about Mintlify Previews.
💡 Tip: Enable Workflows to automatically generate PRs for you. |
|
You have reached your Codex usage limits for code reviews. You can see your limits in the Codex usage dashboard. |
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Fixes #2517.
What was already there
The topological sort the issue asks for has been in the SQL export since #1126:
SQLExportPlugin.topologicallySortruns every export throughForeignKeyTopologicalSort, the CREATE and data phases follow that order, and the drop phase walks it backwards. So item 1 of the issue and item 3'sALTER TABLE … ADD CONSTRAINTwere 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.orderedreturns[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.warningsreachesExportService.state.warningMessageand stops there: nothing reads it, andTransferResultAlert.presentExportSuccesstook 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 throughpresentImportSuccess(result:).And item 3's premise does not hold.
writeFinalizationPhaseemittedALTER TABLE … ADD CONSTRAINTfor every foreign key regardless of engine, while most drivers'fetchTableDDLhands back the server's ownCREATE TABLE, which already declares them:SHOW CREATE TABLEcarriesCONSTRAINT … FOREIGN KEY, so the import fails with error 1826, duplicate foreign key constraint nameMSSQLPluginDriver+Schema.swiftappends the same clauses itselfsqlite_master.sqlkeeps the inlineREFERENCES, and SQLite has noALTER TABLE … ADD CONSTRAINTat all, so the statement is a syntax errorduckdb_tables(),GET_DDL, andSHOW CREATE TABLE/SHOW TABLEPostgreSQL 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:
The fix
PluginKit.
ForeignKeyTopologicalSortgainsOrderingandorder(_: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
ordersandcustomersreferencing each other andauditreferencingorders, all three come back unplaced, so a warning built on that set namesauditas part of a cycle it is not in, and the fallback can putauditbefore 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.tableDDLIncludesForeignKeyssays whetherfetchTableDDLalready declares them. It defaults tofalse, 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 totrue, andPluginExportDataSourcemirrors it for the export side.writeFinalizationPhasethen runs only where the DDL leaves foreign keys out.Redshift needed one more thing to be honest about the flag: its
SHOW TABLEpath declares foreign keys and its hand-built fallback did not, so the fallback now emits them too.The summary.
ExportState.warningsreplaceswarningMessage,presentExportSuccesstakes the warnings and mirrors the import alert ("Export completed with warnings",.warningstyle, the text ininformativeText), and the suppression checkbox is offered only on a clean run, because the alert a user turned off is the routine one.ExportDialogshows 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, becauseforeignKeyDisableStatements()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
tableDDLIncludesForeignKeysis 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 failBundle.loadAndReturnErrorin an older app.validateBundleVersionsonly rejectsdeclared > current, so without a bump that plugin is accepted and its driver then vanishes.currentPluginKitVersiongoes to 21 withminimumCompatiblePluginKitVersionleft at 19, so an older app refuses a new plugin cleanly instead. Every pluginInfo.plistis bumped to match. This is the same reasoning that shipped PluginKit 20 in #2597.CLAUDE.md:120-124still 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 21has to run before or with the app release, or users on the new app hitnoCompatibleBinaryuntil the registry catches up. It has not been run.Verification
generatebuild(app)testlint(12 paths)docsabi origin/mainordereduntouchedplugins(aggregate)AllPluginsfails locally on the vendored oracle-nio fork,macro expansion @TaskLocal:1:2: error: unknown attribute 'usableFromInlinenonisolated'in targetOracleNIO, which is a toolchain incompatibility that stops the aggregate for any change underPlugins/. 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:220referencesAXCell, a symbol in no Swift source.CLAUDE.mdis not in this diff.New
SQLExportForeignKeyOrderTestscovers 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.SQLExportPluginis added to the test target's sources, which is what makes those runnable at all.ForeignKeyTopologicalSortTestsgains three cases for cycle membership. The export tests snapshot and restore the plugin's stored settings, becauseSQLExportPlugin()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 behindtry?, 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
NSAlertthat only appears after a real export against a real server.https://claude.ai/code/session_01S9ckdzeugurfqNmDpGU2M7