Skip to content

fix(datagrid): refuse edits to server-owned columns at the model boundary - #2590

Merged
datlechin merged 2 commits into
mainfrom
fix/non-writable-column-enforcement
Sep 1, 2026
Merged

fix(datagrid): refuse edits to server-owned columns at the model boundary#2590
datlechin merged 2 commits into
mainfrom
fix/non-writable-column-enforcement

Conversation

@datlechin

@datlechin datlechin commented Aug 31, 2026

Copy link
Copy Markdown
Member

Follows #2589, which is merged. These are the four defects that investigation found and reported rather than shipped. All four predate #2589, but three of them undermine the read-only guarantee it added, and the fourth is in a file it touched.

1. Paste, Fill Column and the row inspector bypassed the writability check

generatedColumns is the set the app must never write. #2589 gates the inline editor and the Set Value menu on it, but three paths reach the change manager without passing either gate: cell paste and Fill Column go through recordCellEdit, and the row inspector calls DataChangeManager.recordCellChange directly.

Stage an edit to a generated column, a MongoDB _id, or a GENERATED ALWAYS AS IDENTITY column that way and save it alongside a legitimate edit: generateUpdateSQL filters the forbidden one out of the SET clause, the legitimate one commits, and the successful save clears both. The user is told it worked and one of their edits is gone.

The fix is a guard at DataChangeManager.recordCellChange, the last gate before a change becomes pending and the only one every path crosses. recordCellEdit also refuses before touching TableRows, so the grid never paints a value no statement will carry, and Fill Column no longer appears on a column the server owns.

2. A cached rerun could adopt another result's column metadata

resolveDisplayMetadata re-read the live TabSessionRegistry when the result landed, but the decision to answer from cache was made much earlier, before the query ran. Nothing bound the two.

Rerun a cached result, select a pinned result before the rerun completes, and the rerun inherits that other result's columnIdentity and generatedColumns. Because it was a cache hit, no schema fetch runs behind it to repair the mistake, and the wrong sets persist on the new TableRows: writes to server-owned columns of the first table become possible, or legitimate values of it get suppressed.

The snapshot is now captured at the moment the cache decision is made, through ParsedSchemaMetadata.cached(rows:primaryKeyColumns:), and resolveDisplayMetadata never re-reads the session. Both decision sites (MainContentCoordinator and the parameterized path) capture it. foreignKeysFetched is preserved through the snapshot's columnForeignKeys being nil versus empty, so a tab that has not fetched its arrows still fetches them.

Collapsing the inline and cached metadata into the one metadata parameter kept the call sites at their existing arity. They are mutually exclusive by construction, since inline metadata is only built when the cache missed.

3. Rows were writable before the schema said which columns the server owns

QueryExecutor.inlineMetadata publishes empty generatedColumns and columnIdentity, and phase 1 makes the result editable before the schema fetch completes. Add Row in that window stages NULL into an identity column, and the later metadata update does not rewrite the staged row. A schema-fetch failure leaves the result in that state permanently.

Two changes:

  • Use the identity the result set does report. ResultColumnMeta.isAutoIncrement was on the wire and being dropped. It now becomes .byDefault identity, which is the safe reading: the result set says the server allocates the column, never that it would refuse an explicit value. That alone turns a NULL pre-fill into a DEFAULT pre-fill on every engine whose result metadata carries it.
  • Say when the metadata is authoritative. ParsedSchemaMetadata.isAuthoritative is true only from parseSchemaMetadata, and TableRows.hasAuthoritativeSchema carries it. Add Row and Duplicate Row wait for it, in menu validation and in RowEditingCoordinator itself, because the toolbar button and the right-click item do not go through menu validation.

Gating those two commands rather than the whole grid is deliberate: they are the paths that stage values from this metadata, and the schema fetch runs concurrently with the row fetch, so the wait is a fraction of a second on a first open and nothing at all afterwards. A cell edit made inside that same window is still possible; the change manager filters it at save as before. Closing that too would mean making every fresh result briefly read-only, which is a worse trade.

4. SQL Server exports could not be restored

The exporter excludes only isGenerated columns, so SQL Server IDENTITY columns are written into each INSERT with their explicit values. SQL Server rejects those statements unless the table is opened for it first, so the export completed successfully and restored nothing.

Each SQL Server table's rows are now bracketed with SET IDENTITY_INSERT <table> ON; and OFF;. The ON is emitted lazily before the first batch, so a table with no rows gets no stray pair.

Verified

Step Result
build TablePro PASS
test (20 suites) PASS, 327 cases, 0 failed
build SQLExport / MSSQLDriver / PostgreSQLDriver / MySQLDriver PASS
lint TablePro Plugins TableProTests 0 violations
docs PASS

New tests: DataChangeManagerNonWritableTests (refusal, and that a refused edit leaves a legitimate one recorded alongside it intact), CachedSchemaMetadataTests (the snapshot carries every write-relevant field, unfetched foreign keys stay absent rather than becoming an empty answer), SchemaMetadataAuthoritativenessTests (parsed schema authoritative, result metadata not, auto-increment carried as a writable identity).

No PluginKit change, so no ABI check is required and no version bump.

The plugins aggregate still fails locally on the pre-existing OracleNIO @TaskLocal macro issue, so the four plugin targets involved were built individually.

The SQL Server IDENTITY_INSERT bracket has no unit test because SQLExportPlugin.swift is not in the test target's sources; it is compile-checked by the plugin build. Verifying it end to end needs a live SQL Server, which no harness here can provision.

No UI automation, for the same reason as #2589: these flows need a live PostgreSQL or SQL Server connection with identity columns.

Verified after rebasing onto the merged main, which needed generate first because #2586 added source files.

Review round

Codex read the diff cold and found five more gaps in this change, all fixed in the second commit:

  • Paste never reached the boundary. RowOperationsManager.insertParsedRows calls recordRowInsertion, not recordCellChange, so a pasted identity or generated value stayed staged and displayed while the statement generator dropped it. Pasted rows now have those cells reset to DEFAULT before they are appended. My original description of this PR claimed paste was covered by the boundary guard; it was not.
  • The boundary was narrower than the grid's. isColumnWritable on the grid also consults the driver's immutableColumns, so MongoDB's _id and its equivalents could still be staged from the inspector. The model boundary now asks the same full question.
  • The inspector could not undo a refusal. MultiRowEditState.updateField stores the pending value before calling the change manager, and a void refusal leaves it there, so the field reads as edited and Save never clears it. Server-owned fields are now shown without an editor instead.
  • Only the Edit menu was gated on schema readiness. The toolbar's Add Row goes through MainContentCoordinator.canAddRow and the grid's Duplicate through its own context menu; both stayed enabled and hit a guard that silently returned. Readiness now lives in the shared predicate and in the grid menu.
  • A skipped phase-two apply left a result inert for good. applySchemaMetadata dropped the metadata whenever the user had switched to another result, and nothing re-fetches on the way back, so hasAuthoritativeSchema stayed false and Add Row was disabled permanently. That was a regression this PR introduced. Phase-two metadata is now applied to the ResultSet it was fetched for even while that result is inactive.

Codex also raised eight findings against StructureColumnReorderHandler, ColumnReorderTypes, SQLiteColumnReorderPlanner, PostgreSQLPluginDriver+ColumnReorder and OraclePlugin. Those belong to #2586, which merged while this branch was in flight and which the review picked up because its base branch had been deleted. They are not touched here.

One residual is deliberate: between phase one and phase two, inline editing, Set Value and Fill Column remain enabled on a result whose non-writable set is not yet known. A cell edit made in that window is filtered at save exactly as before this PR. Closing it would mean making every fresh result briefly read-only, which is a worse trade than the sub-second window it removes.

@mintlify

mintlify Bot commented Aug 31, 2026

Copy link
Copy Markdown

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

Project Status Preview Updated (UTC)
TablePro 🟢 Ready View Preview Aug 31, 2026, 10:51 PM

💡 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 c46cb8b into main Sep 1, 2026
12 of 14 checks passed
@datlechin
datlechin deleted the fix/non-writable-column-enforcement branch September 1, 2026 03:43
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