Skip to content

fix(structure): hold only the columns a save changes to having a name and a type - #3147

Merged
datlechin merged 2 commits into
mainfrom
fix/structure-untouched-column-validation
Sep 26, 2026
Merged

datlechin merged 2 commits into
mainfrom
fix/structure-untouched-column-validation

Conversation

@datlechin

Copy link
Copy Markdown
Member

Root cause

StructureChangeManager.validate() checked every column in workingColumns and showed "Column must have a name and a data type" for any column where isValid was false. isValid needs 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 canCommit until #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:

  • SQLite reports a column declared without a type (CREATE TABLE notes (id INTEGER PRIMARY KEY, body)) with type '', and the SQLite, libSQL and Cloudflare D1 drivers pass that through.
  • CREATE TABLE … AS SELECT gives no type to every selected expression that is not a plain column or a CAST.
  • SQLite accepts columns named "" and " ".
  • MongoDB 7 stores fields named "" and " ", and the driver lists them as columns.

columnNames had the same problem. It was filtered on isValid, 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:

  • A primary key on a typeless column reported "Primary key references a column that does not exist" on every save.
  • An index on a typeless column was refused.
  • ADD COLUMN body INTEGER next to a typeless body passed validation, then failed in SQLite with "duplicate column name".
  • The primary key check ran even when the save did not change the key. After a rename, the loaded key still named the old column, so the save was refused on every engine. Deleting a key column was refused the same way.

Fix

isValid answered two questions at once. They are now separate:

  • Does this column still exist after the save? columnsAfterSave is 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.
  • Is this column complete enough to write? EditableColumnDefinition.isIncomplete(over:) answers it with two checks, hasSavableName(over:) and hasSavableDataType(over:). A name or type passes when it is filled in, or when the column was read without it. So:
    • A new column needs a name and a type.
    • An edited column may not lose a name or type it was read with.
    • An edit never has to supply a name or type the column did not have when it was read.

This is the rule introducesNullDefaultOnNotNull already 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 with duplicate 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 COLUMN carries the key over to the new name. Dropping a key column is left to the database: MySQL and PostgreSQL allow it, and SQLite refuses with cannot drop PRIMARY KEY column.

There is no PluginKit change and no new string.

Verified

  • Unit tests: 27 suites, 369 executed, 369 passed. The suites are StructureChangeValidationTests, ColumnDefinitionTests, SQLiteColumnDeclarationTests and SQLiteTableRespecifierTests, plus these neighbours:
    • AnyChangeManagerTests, IndexDefinitionPasteTests, SQLiteTableRebuildPlannerTests, StructureChangeGuardTests
    • StructureChangeManager CatalogSpelling, ClusteredIndex, ForeignKeyLoad, IndexExpression, PK, UndoDelete and Undo tests
    • StructureDeclaredTypeEditTests, StructureEditGateTests, StructureEditingSupportFieldDiffTests
    • StructureGridDelegateAddRowTests, StructureGridDelegateInspectorTests, StructureInspectorRowBuilderTests
    • StructureNullDefaultTests, StructureRowProviderTests, StructureRowProviderChangeStateTests
    • StructureSavePlanTests, StructureEditingSessionTests, StructureDiffEngineTests
  • Main's StructureChangeManager.swift fails 13 of the 32 cases in StructureChangeValidationTests:
    • Untouched, renamed, NOT NULL and deleted typeless columns.
    • An untouched empty-named MongoDB-shaped field.
    • A primary key on a typeless column.
    • An index on a typeless column.
    • A duplicate of a typeless column.
    • An untouched duplicate pair.
    • A renamed or deleted primary key column.
    • A rename onto a blank name the table holds. Main refuses it as incomplete, not as a duplicate.
    • A rename from "" to " ", which SQLite accepts and main refuses.
  • renamingOntoALoadedBlankNameIsADuplicate also fails on this branch's first revision, which dropped blank names from the duplicate check. It passes now.
  • These cases refuse the save both before and after the fix:
    • A blank added column.
    • Two blank added columns, reported as incomplete, not duplicate.
    • A blank added column beside a loaded "".
    • A cleared type.
    • A cleared name, including on a typeless column and beside a loaded "".
    • A rename onto another column's name.
  • SQLite 3.54.0:
    • CREATE TABLE n2(body NOT NULL, tag TEXT) gives body type '' with notnull 1.
    • RENAME COLUMN id TO note_id keeps pk 1.
    • ADD COLUMN body INTEGER next to a typeless body fails with duplicate column name: body.
    • DROP COLUMN on the key fails with cannot drop PRIMARY KEY column.
    • RENAME COLUMN " " TO "" next to "" fails with duplicate column name: .
    • RENAME COLUMN "" TO " " next to " " succeeds.
  • MongoDB 7.0.43, reading through the real plugin sources: fetchColumns returns _id ObjectId, "" VARCHAR, " " VARCHAR, a INTEGER and b INTEGER. $rename {" ": ""} fails with An empty update path is not valid.
  • App build: pass. verify.sh docs: pass.
  • swiftlint --strict on the touched files: 0 violations, except 10 contains_over_first_not_nil in SQLiteColumnDeclarationTests. The merge base has the same 10.

Deliberately not fixed here

  • Column names are compared with case. RENAME COLUMN b TO A next to a passes validation, and SQLite then fails with duplicate column name: A. Ignoring case would refuse a legal PostgreSQL pair such as "Name" and name, so this needs identifier rules for each engine.
  • Deleted indexes and check constraints still count as duplicates. Deleting idx_a and adding a new idx_a in one save is refused as Duplicate index name: idx_a.
  • Blank index names. SQLite accepts 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.
  • Adding a column with no type is still refused, although SQLite accepts ADD COLUMN d. Every other engine needs a type.
  • Clearing a column's name to whitespace is still refused, although SQLite accepts a whitespace-only name.
  • The duplicate message for a blank name reads Duplicate column name: with nothing after the colon, as SQLite's own error does.
  • MongoDB schema editing is off on main. This fix only matters there once Structure field editing lands.

StructureTypelessColumnUITests seeds a restored SQLite session with CREATE TABLE notes (tag TEXT, body). It drops a column from the Structure tab, confirms Apply Changes, and checks the file with pragma_table_xinfo. The session fixture moved to TableProUITests/Support/SeededSQLiteSession.swift, and CloseTabBeforeFirstClickUITests now 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.

@mintlify

mintlify Bot commented Sep 26, 2026 •

Copy link
Copy Markdown

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

Project Status Preview Updated
TablePro 🟢 Ready View Preview Sep 26, 2026, 10:59 AM

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

@datlechin
datlechin merged commit c21dc51 into main Sep 26, 2026
14 checks passed
@datlechin
datlechin deleted the fix/structure-untouched-column-validation branch September 26, 2026 14:23
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