fix(datagrid): refuse a save that would leave out a change its driver cannot write - #3149
Merged
Merged
Conversation
|
Preview deployment for your docs. Learn more about Mintlify Previews.
💡 Tip: Enable Automations to automatically generate PRs for you. |
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.
Root cause
The PluginKit contract had no way for a driver to say "I cannot write this change".
PluginDatabaseDriver.generateStatementsreturns a flat list of statements, nothing ties a statement to the change it writes, andnilmeans "the host generates". So a generator that met a change it could not express could only leave it out and log a warning. Generators did this at two levels:The host took that list unchecked.
RowChangeStatementFactoryreturned the plugin's statements as given, andbuildRowWriteswrapped them with no row count. The only completeness checks (validateand the delete count) were on the host-SQL path, which a plugin save never reaches. So in a mixed save the plan was not empty: it ran,finishSuccessfulSavecleared the queue and the undo stack, and the reload removed the dropped change from the grid. #3132 is that save: edit a MongoDB document and add a row with every field empty. OnlyupdateOneruns, and the app reports success.The host path had the same hole for inserts. On a
.genericdialect other than Databend,allDefaultsInsertStatementreturns nil, and nothing counted inserts.A statement's text cannot prove it writes a given value. So the host can check coverage per change, and only the driver can refuse a value.
Fix
PluginKit (additive, pending kit 33, no bump)
generateRowWrites(...) throws -> [PluginRowWrite]?. Each write names therowIndexof every change it writes. A change, or a value in one, that the driver cannot write is thrown asPluginRowWriteRefusal(rowIndex:reason:)rather than left out.nilstill means the host generates.generateStatements. It runs that once on the whole set and returns the statements unchanged, so MongoDB'sdeleteManybatching is kept. It then runs it once per change, with only that change's row, to learn which changes produce a statement. That is 1 + N calls, whatever the number of edited cells. The default holds a driver to every change, not to every value in one.Drivers that left out values now refuse them, each through its own
generateRowWrites, one write per change:Elasticsearch. Refuses a value typed into a nested leaf, whether the document is existing or new. Refuses a new row that carries a leaf value while its array is empty, and an edit to
_id,_indexor_score(_idtyped into a new row still names the document). Also refuses a document with no_id, and a value JSON cannot hold, such asnanin a number field. Example reason: "'identifiers.type' is a field of a nested array. Edit the array in 'identifiers' instead."Redis. Refuses:
-1and NULL still runPERSIST)A per-slot
DELnames the rows in its slot. Database addressing keeps each write's rows, and itsSELECTs name none.etcd. A NULL Value is written as an empty value (etcd stores no NULL, and a new row already did this) instead of as the old value. A NULL Lease on its own removes the lease, as
\"\"and0already did. Refuses an edit to Version, ModRevision or CreateRevision, and a key renamed to NULL or empty.Host
RowWriteCoverageis the one rule for both generators. Every pending change needs a statement (an update with cell changes, a row still marked inserted, a row still marked deleted), or the save is refused before anything runs.AttributedStatementcarries theRowIDs it writes, and delete chunks keep theirs.validate, with itshasPrefix(\"UPDATE\")check, and the separate delete count are gone.rowsNotIdentifiable(table, kind)as before. An all-DEFAULT insert the dialect cannot spell is now refused instead of dropped.DataWriteError.changesNotWritable, which names the kind and the count: "Cannot save changes to 'items'. The driver cannot write a new row." A thrown refusal becomeschangeRefusedwith the kind of change and the driver's reason: "Cannot save the edited row in 'persons'. 'identifiers.type' is a field of a nested array. Edit the array in 'identifiers' instead."buildRowWritesgenerates once instead of twice. Driver steps keepexpectedRowCountnil.PluginDriverAdapter.pluginGenerateStatements.Verified
Tests: 24 suites, 286 of 286 passed. They include RowChangeStatementFactoryCoverageTests, SidebarSaveCoverageTests, SaveCompletionTests, and the Elasticsearch, Redis and etcd generator suites.
Mutation run: with the default put back to a per-cell probe, the Elasticsearch insert check off, etcd's old-value fallback back, and Redis skipping a non-string Value, 9 of 54 cases failed, all of them the new tests.
A swiftc harness compiled the previous commit's PluginKit and generators, then this tree's:
identifiers.typetyped:POST {}and no refusal before; now refused with its reason.put k v1(the old value) before; nowput k \"\".generateStatementscalls before; now 201.verify.sh buildandverify.sh plugins(all 40 plugins) pass.Lint: 0 violations on the 29 Swift files the commit changes.
verify.sh docspasses.localization.py pluginsandlocalization.py verifyreturn ok.ABI against the merge base: only additions (the requirement and its default,
PluginRowWrite,PluginRowWriteRefusal). v0.75.0 ships kit 32; main and every plugin Info.plist are already on 33.Live, MongoDB 7.0.43, real MongoDB plugin sources against this PluginKit:
updateOnenaming row 0, and the server held both changes.Live, Redis 8.10.2, running the commands this generator produced:
a,band TTL-1.EXPIRE probe:list 600.Codex
review, three rounds. Round 1 found row-level coverage passing a partial update. A per-value probe answered it, and round 2 showed that probe was heuristic by nature, so it was taken out: the default is row-level again, and the generators known to drop single values (Elasticsearch, Redis, etcd) now refuse them throughgenerateRowWrites. Round 3 found a new Redis key with a negative TTL other than -1 saved without itsEXPIRE; that is refused now, with a test.Deliberately not fixed here
MongoDB adoption (PR B). Write
insertOne({})for an empty document, and refuse a truncated value or a missing_idwith a reason. It waits for Can't create a database collection from the visual editor #3131, which rewrites the same generator and its tests. Until then an empty new MongoDB document is refused rather than saved.Value-level omissions by generators that have not adopted
generateRowWritesremain. The host sees only whether a change got a statement, not whether each value in it was written. Known cases:id/in/outedit, set next to another edit (SurrealStatementGenerator.swift:66). It also drops a record id typed into a new row that does not parse (:102).The earlier audit of the other generators found no other value they leave out of a change they still write.
BigQuery's, Spanner's and Kafka's use of nil as a refusal is left for PR C.
Found while reading, not measured:
putwithout--lease, which detaches the lease.__DEFAULT__into a plugin engine's key column: a Redis key named__DEFAULT__, orPUT /_doc/__DEFAULT__on Elasticsearch.Merge note. Can't create a database collection from the visual editor #3131 and feat(plugin-mongodb): insert MongoDB documents written as Extended JSON #3140 also append to Localizable.xcstrings and CHANGELOG.md. Whichever lands second fixes the conflict at the catalog's tail by hand.
No UI test: SQLite is the only engine UI tests reach without a server, and it writes every change and every value, so no UI flow reaches a refusal. CI has no MongoDB, Redis, Elasticsearch or etcd. SaveCompletionTests drives
saveChangesheadlessly with a driver stub instead.