fix(datagrid): refuse edits to server-owned columns at the model boundary - #2590
Merged
Conversation
|
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. |
…n the writability guard
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.
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
generatedColumnsis 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 throughrecordCellEdit, and the row inspector callsDataChangeManager.recordCellChangedirectly.Stage an edit to a generated column, a MongoDB
_id, or aGENERATED ALWAYS AS IDENTITYcolumn that way and save it alongside a legitimate edit:generateUpdateSQLfilters 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.recordCellEditalso refuses before touchingTableRows, 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
resolveDisplayMetadatare-read the liveTabSessionRegistrywhen 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
columnIdentityandgeneratedColumns. Because it was a cache hit, no schema fetch runs behind it to repair the mistake, and the wrong sets persist on the newTableRows: 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:), andresolveDisplayMetadatanever re-reads the session. Both decision sites (MainContentCoordinatorand the parameterized path) capture it.foreignKeysFetchedis preserved through the snapshot'scolumnForeignKeysbeing nil versus empty, so a tab that has not fetched its arrows still fetches them.Collapsing the inline and cached metadata into the one
metadataparameter 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.inlineMetadatapublishes emptygeneratedColumnsandcolumnIdentity, 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:
ResultColumnMeta.isAutoIncrementwas on the wire and being dropped. It now becomes.byDefaultidentity, 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 aDEFAULTpre-fill on every engine whose result metadata carries it.ParsedSchemaMetadata.isAuthoritativeis true only fromparseSchemaMetadata, andTableRows.hasAuthoritativeSchemacarries it. Add Row and Duplicate Row wait for it, in menu validation and inRowEditingCoordinatoritself, 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
isGeneratedcolumns, so SQL ServerIDENTITYcolumns 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;andOFF;. TheONis emitted lazily before the first batch, so a table with no rows gets no stray pair.Verified
build TableProtest(20 suites)build SQLExport / MSSQLDriver / PostgreSQLDriver / MySQLDriverlint TablePro Plugins TableProTestsdocsNew 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
pluginsaggregate still fails locally on the pre-existing OracleNIO@TaskLocalmacro issue, so the four plugin targets involved were built individually.The SQL Server
IDENTITY_INSERTbracket has no unit test becauseSQLExportPlugin.swiftis 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 neededgeneratefirst 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:
RowOperationsManager.insertParsedRowscallsrecordRowInsertion, notrecordCellChange, so a pasted identity or generated value stayed staged and displayed while the statement generator dropped it. Pasted rows now have those cells reset toDEFAULTbefore they are appended. My original description of this PR claimed paste was covered by the boundary guard; it was not.isColumnWritableon the grid also consults the driver'simmutableColumns, so MongoDB's_idand its equivalents could still be staged from the inspector. The model boundary now asks the same full question.MultiRowEditState.updateFieldstores 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.MainContentCoordinator.canAddRowand 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.applySchemaMetadatadropped the metadata whenever the user had switched to another result, and nothing re-fetches on the way back, sohasAuthoritativeSchemastayed false and Add Row was disabled permanently. That was a regression this PR introduced. Phase-two metadata is now applied to theResultSetit was fetched for even while that result is inactive.Codex also raised eight findings against
StructureColumnReorderHandler,ColumnReorderTypes,SQLiteColumnReorderPlanner,PostgreSQLPluginDriver+ColumnReorderandOraclePlugin. 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.