fix(coordinator): compose structure and create table previews on the scope save runs on - #3071
Merged
Merged
Conversation
…scope save runs on
|
Preview deployment for your docs. Learn more about Mintlify Previews.
💡 Tip: Enable Automations to automatically generate PRs for you. |
…-scope # Conflicts: # CHANGELOG.md
…-scope # Conflicts: # CHANGELOG.md
This branch was successfully deployed
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.
Summary
Structure Preview SQL, the Create Table SQL Preview tab and a row import's new-table
CREATEall built their DDL on the unpinned session driver, while the statements that actually run are built on a driver pinned to the tab's scope. So a preview could name a different schema than Save, and a new table from an import could land in a different schema than its rows. Preview also skipped two things Save does: the primary key constraint name lookup, and the decision to rebuild the table instead of runningALTER.Every DDL caller now gets its text from one scoped composer, and Save runs exactly what was composed.
Root cause
Several plugins qualify DDL from the driver's own mutable state: PostgreSQL's
qualifiedTableName,generateDropIndexSQLandgenerateCreateTableSQLreadcore.currentSchema, and DuckDB, SQL Server and Cassandra do the same.pin()moves that state on every scoped session-driver read, and so does the toolbar schema picker. Save built its statements insidewithScopedDriveron the tab scope; Preview and Create Table tookdriver(for:)with no scope, passed no constraint name, and never askedStructureTableRebuildHandler.requiresRebuild. Nothing forced preview and save through the same composer.On top of that, the primary key name lookup was gated on a closed
DatabaseTypelist (postgresql,redshift,cockroachdb,duckdb) that PGlite was missing from, so a PGlite primary key change saved asDROP CONSTRAINT /* unknown constraint */.What changed
DatabaseManager+SchemaComposition(new):withSchemaComposer(scope:route:)leases the driver for a scope and hands over the pinned plugin driver. On top of it:schemaChangeStatements(tableName:changes:scope:),createTableStatements(plan:scope:)andcreateTableStatements(definition:scope:route:).PrimaryKeyConstraintLookup(new): a function taking a driver. It asksINFORMATION_SCHEMA.TABLE_CONSTRAINTSfor the key being dropped, in the driver's pinned schema, with the table name escaped by the driver itself. It only asks when an existing key is dropped and the engine has schemas. NoDatabaseTypelist.executeSchemaChanges(_ statements:databaseType:scope:)no longer composes; it authorizes, runs and records. Every caller and test moved to compose-then-execute in this commit.StructureSavePlan(new):.alter([SchemaStatement])or.rebuild(Prepared), decided once byStructureEditingSession.stagedSavePlan(for:).applyStagedChangesandpreviewStagedChanges(coordinator:)both read it.TableRebuildReviewRequestSave presents, with its caveats as the warning and Open in Query Editor, but with no action: Preview never offers to run anything.TableRebuildReviewRequestnow carries an optionalAction, and the sheet only shows it when the plan is runnable.CreateTableDraft(coordinator-owned per tab,@Published), recomposed after a 150ms pause keyed on the draft's inputs plus scope. The last composition stays on screen until the next one lands, a failed composition keeps it, a stale one is dropped by generation, and an unchanged draft is not composed again. The preview, the issue label andhasCreateTablePendingread it, so a remount no longer blanks them. Create Table composes fresh at press time and checks the plan's own issues synchronously first.StructureChangeManager'sobjectWillChange(theConnectionFormCoordinatorpattern).CreateTableViewobserves only the draft and theAnyChangeManagerwrapper, which does not forward, so a column, index or foreign key edit in the grid never re-evaluated the view: the SQL Preview, the issue label and the Create button's readiness stayed on the last state until the name field, a segment switch or a selection re-rendered it. This gap predates this PR. Before it, the view observed the same two objects and read the working rows directly inbody, so the old derived preview and readiness went stale on a grid edit the same way. Found by review of this PR and fixed in the follow-up commit.performImportresolves the browse scope once. The new table'sCREATEis composed onexecutionRoute(the route that runs it), and the same scope goes toImportService.importFile(scope:),prepareTable,clearRowsand the leased statements.ImportServiceno longer re-derivesbrowseScope;ImportDialogresolves it for its own import.docs/features/table-structure.mdx.No PluginKit change, so no ABI check.
Measured
PostgreSQL 17.11 (Homebrew, aarch64), two schemas each with an
orderstable and an indexi, the second table renamed tosales, sessionsearch_pathmoved to the other schema. Probe files:probe.sql(before) andprobe-after.sql(after), both in the lane directory.DROP INDEX "preview_scope"."i"dropped the other schema's index.DROP CONSTRAINT "sales_pkey"failed withconstraint "sales_pkey" of relation "sales" does not exist(the key staysorders_pkeyafter a rename).DROP CONSTRAINT /* unknown constraint */failed withsyntax error at or near ";".ALTER TABLE "preview_scope"."sales" ...failed withrelation "preview_scope.sales" does not exist.SELECT CONSTRAINT_NAME FROM INFORMATION_SCHEMA.TABLE_CONSTRAINTS WHERE TABLE_SCHEMA = 'preview_scope_reporting' AND TABLE_NAME = 'sales' AND CONSTRAINT_TYPE = 'PRIMARY KEY'returnsorders_pkey. On the pinned schema,DROP CONSTRAINT "orders_pkey",ADD PRIMARY KEY ("id", "v")andDROP INDEX "preview_scope_reporting"."i"all succeed, and the other schema'siis untouched.INFORMATION_SCHEMA.TABLE_CONSTRAINTSreports a synthesizedsales_id_pkey, andpg_constraint(the old query) reportsPRIMARY KEY(id). Neither matters:ALTER TABLE ... DROP CONSTRAINTanswersNot implemented Error: No support for that ALTER TABLE option yet!for any name, so DuckDB behaves as before.Tests
verify.sh testonStructureSavePlanTests PrimaryKeyConstraintLookupTests CreateTableDraftCompositionTests SchemaCompositionGuardTests DatabaseManagerSchemaChangeRoutingTests StructureEditingSessionTests TabCloseProtectionTests SchemaOperationRefusalTests: 78 executed, 78 passed (after the follow-up commit).StructureSavePlanTests(new, 12 cases): plan names the tab's schema on the pooled route and on a single-connection engine (PGlite) after pinning; Save runs exactly the planned statements; PK drop uses the server's name on PostgreSQL and PGlite and never contains/* unknown constraint */; a SQLite foreign key change plans the rebuild with the prepared script; Preview shows the ALTER statements; Preview shows a rebuild read-only with its caveats and runnability, for runnable and non-runnable plans, while Save's review offers the action only when runnable; a preview that lands after the tab left Structure, over another sheet, or for a session the selected tab no longer owns presents nothing.PrimaryKeyConstraintLookupTests(new, 5): query shape and schema, literal escaping, no query without a drop or without schemas, a failed catalog read leaves the name unknown.CreateTableDraftCompositionTests(new, 8): composes on the tab schema; the last statement stays until the next lands (session gate held across the recomposition); a failure keeps the last good statement; an unchanged draft is not composed again; an empty draft names what is missing with no connection; a grid edit throughCreateTableGridDelegate.dataGridDidEditCellon the Columns, Indexes and Foreign Keys tabs publishes the draft and changes its composition key.SchemaCompositionGuardTests(new, 2): noSchemaStatementGenerator(,CreateTableStatementComposer.compose(orgenerateCreateTableStatements(outside the scoped composers.SchemaSyncScriptBuilderis allow-listed as known unscoped collateral, not as a sanctioned composer.DatabaseManagerSchemaChangeRoutingTests: the 7 existing save tests moved to compose-then-execute; 4 new cases for the composer's driver on both routes, Create Table on the scope schema running unchanged on its own connection, and the importCREATEon the execution route.StructurePreviewSQLUITests(new UI test): on the SQLite sample, select Album's foreign key, remove it, pressCmd+Shift+P, and check the sheet offers Open in Query Editor with no run button and no "Unsupported schema operation". It compiles in theTableProUITeststarget; I did not run it locally because the lanes share one screen, so CI is its first run.What turns each red, measured by mutating the source and rerunning:
driver(for:)instead of a scoped lease: 14 cases red, including both scope tests,composesOnTheTabSchema,keepsTheLastStatementUntilTheNextLands,composerDriverSitsOnTheScopeSchema,importCreateTableComposesOnTheExecutionRoute,schemaGroupedEngineKeepsTableSchema.previewShowsTheRebuildReadOnlyred.unchangedDraftIsNotComposedAgainred.SchemaStatementGenerator(call site inTableStructureView+Schema:ddlIsComposedOnlyOnAScopedDriverred.primaryKeyDropUsesTheServerName(type: .pglite)red, PostgreSQL still green.requiresRebuild:sqliteForeignKeyPlansTheRebuildand bothpreviewShowsTheRebuildReadOnlyarguments red.composedbefore recomposing:keepsTheLastStatementUntilTheNextLandsandfailureKeepsTheLastStatementred.CreateTableDraft.init: all threegridEditPublishesTheDraftarguments red.Build PASS, lint 0 violations on all 20 changed Swift files, docs checks PASS.
Before / After
Screenshots to be added. States to capture:
reporting.saleswhile the toolbar schema ispublic, one column staged, Preview SQL open: before names"public"."sales", after names"reporting"."sales".ALTER TABLE orders RENAME TO sales, primary key changed, Preview SQL open: beforeDROP CONSTRAINT /* unknown constraint */, afterDROP CONSTRAINT "orders_pkey".-- Error generating SQL: Unsupported schema operationsheet, after the rebuild script with Open in Query Editor and Done.reportingwith the toolbar onpublic, SQL Preview segment: beforeCREATE TABLE "public"..., afterCREATE TABLE "reporting"...; then switch to another tab and back, and the preview is still there with no spinner.Critique points not taken
None. All six objections are resolved as described above. One note on the PK objection: the lookup is engine-agnostic through
INFORMATION_SCHEMA.TABLE_CONSTRAINTSrather than aDatabaseTypelist, which also means SQL Server now gets a real constraint name where it used to get/* unknown constraint */(Plugins/MSSQLDriverPlugin/MSSQLPluginDriver+DDL.swift:144). The query is written in uppercase so it matches catalog views under a case-sensitive collation. I could not run SQL Server here, so that path is unmeasured and is not claimed in the CHANGELOG.Deliberately not fixed here
TablePro/Core/Compare/SchemaSyncScriptBuilder.swift:112and:143. Allow-listed in the guard as known collateral.PrimaryKeyConstraintLookup.constraintName(tableName:changes:driver:)is the function it can call. Object Copy's PK naming is the same follow-up.TablePro/Views/Structure/CreateTableView.swift:321,:360,:361.TablePro/Views/Structure/StructureEditingSession+Apply.swift:57has the checkpreviewStagedChangesdoes not make.TablePro/Views/Main/Extensions/MainContentCoordinator+SQLPreview.swift:71.