fix(structure): hold only the columns a save changes to having a name and a type - #3147
Merged
Merged
Conversation
|
Preview deployment for your docs. Learn more about Mintlify Previews.
💡 Tip: Enable Automations to automatically generate PRs for you. |
…a typeless column drop
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.
Root cause
StructureChangeManager.validate()checked every column inworkingColumnsand showed "Column must have a name and a data type" for any column whereisValidwas false.isValidneeds a name and a type that are not blank after trimming. The index, foreign key and check constraint blocks next to it skip rows that are not staged and rows pending deletion. The column block did neither, so untouched columns and columns struck through for deletion were checked too.Nothing read
canCommituntil #2694 added the Save guard in v0.73.0. Since then, every Structure save on a table with such a column shows "Some Changes Are Incomplete".These columns do exist:
CREATE TABLE notes (id INTEGER PRIMARY KEY, body)) with type'', and the SQLite, libSQL and Cloudflare D1 drivers pass that through.CREATE TABLE … AS SELECTgives no type to every selected expression that is not a plain column or aCAST.""and" ".""and" ", and the driver lists them as columns.columnNameshad the same problem. It was filtered onisValid, and it is also the list the index, foreign key and primary key checks use to ask whether a column exists. That caused four more failures:ADD COLUMN body INTEGERnext to a typelessbodypassed validation, then failed in SQLite with "duplicate column name".Fix
isValidanswered two questions at once. They are now separate:columnsAfterSaveis every working column not pending deletion, in whatever state its name and type are. Its names are the existence list for the index, foreign key and primary key checks.EditableColumnDefinition.isIncomplete(over:)answers it with two checks,hasSavableName(over:)andhasSavableDataType(over:). A name or type passes when it is filled in, or when the column was read without it. So:This is the rule
introducesNullDefaultOnNotNullalready uses.validateColumns(_:)applies it only to staged columns.Duplicate names are grouped over every kept column whose name passes
hasSavableName. That includes a blank name the table was read with: SQLite keeps""and" "as two columns, and renaming one onto the other fails withduplicate column name. A blank row not yet named, whether new or cleared, is left out of the grouping and reported as incomplete. Names are compared exactly, so""," "and" "are three different names, as they are in SQLite. A group blocks the save only when one of its columns is staged.The primary key check now runs only when the save changes the key. It matches names without regard to case, as the index and foreign key checks do. Every engine's
RENAME COLUMNcarries the key over to the new name. Dropping a key column is left to the database: MySQL and PostgreSQL allow it, and SQLite refuses withcannot drop PRIMARY KEY column.There is no PluginKit change and no new string.
Verified
StructureChangeValidationTests,ColumnDefinitionTests,SQLiteColumnDeclarationTestsandSQLiteTableRespecifierTests, plus these neighbours:AnyChangeManagerTests,IndexDefinitionPasteTests,SQLiteTableRebuildPlannerTests,StructureChangeGuardTestsStructureChangeManagerCatalogSpelling, ClusteredIndex, ForeignKeyLoad, IndexExpression, PK, UndoDelete and Undo testsStructureDeclaredTypeEditTests,StructureEditGateTests,StructureEditingSupportFieldDiffTestsStructureGridDelegateAddRowTests,StructureGridDelegateInspectorTests,StructureInspectorRowBuilderTestsStructureNullDefaultTests,StructureRowProviderTests,StructureRowProviderChangeStateTestsStructureSavePlanTests,StructureEditingSessionTests,StructureDiffEngineTestsStructureChangeManager.swiftfails 13 of the 32 cases inStructureChangeValidationTests:""to" ", which SQLite accepts and main refuses.renamingOntoALoadedBlankNameIsADuplicatealso fails on this branch's first revision, which dropped blank names from the duplicate check. It passes now.""."".CREATE TABLE n2(body NOT NULL, tag TEXT)givesbodytype''with notnull 1.RENAME COLUMN id TO note_idkeeps pk 1.ADD COLUMN body INTEGERnext to a typelessbodyfails withduplicate column name: body.DROP COLUMNon the key fails withcannot drop PRIMARY KEY column.RENAME COLUMN " " TO ""next to""fails withduplicate column name:.RENAME COLUMN "" TO " "next to" "succeeds.fetchColumnsreturns_id ObjectId,"" VARCHAR," " VARCHAR,a INTEGERandb INTEGER.$rename {" ": ""}fails withAn empty update path is not valid.verify.sh docs: pass.swiftlint --stricton the touched files: 0 violations, except 10contains_over_first_not_nilinSQLiteColumnDeclarationTests. The merge base has the same 10.Deliberately not fixed here
RENAME COLUMN b TO Anext toapasses validation, and SQLite then fails withduplicate column name: A. Ignoring case would refuse a legal PostgreSQL pair such as"Name"andname, so this needs identifier rules for each engine.idx_aand adding a newidx_ain one save is refused asDuplicate index name: idx_a.CREATE INDEX "" ON t(a), and editing such an index is refused as incomplete. No duplicate can slip through there, because a staged index with a blank name is always refused.ADD COLUMN d. Every other engine needs a type.Duplicate column name:with nothing after the colon, as SQLite's own error does.main. This fix only matters there once Structure field editing lands.StructureTypelessColumnUITestsseeds a restored SQLite session withCREATE TABLE notes (tag TEXT, body). It drops a column from the Structure tab, confirms Apply Changes, and checks the file withpragma_table_xinfo. The session fixture moved toTableProUITests/Support/SeededSQLiteSession.swift, andCloseTabBeforeFirstClickUITestsnow uses it. Neither UI suite has run locally. The first attempt timed out enabling automation mode, because a macOS UI Automation prompt was waiting on the machine, and it was not retried. Both need a CI run.