Skip to content

fix(datagrid): refuse a save that would leave out a change its driver cannot write - #3149

Merged
datlechin merged 1 commit into
mainfrom
fix/datagrid-unexpressed-edits
Sep 26, 2026
Merged

datlechin merged 1 commit into
mainfrom
fix/datagrid-unexpressed-edits

Conversation

@datlechin

Copy link
Copy Markdown
Member

Root cause

The PluginKit contract had no way for a driver to say "I cannot write this change". PluginDatabaseDriver.generateStatements returns a flat list of statements, nothing ties a statement to the change it writes, and nil means "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:

  • A whole change. An empty new MongoDB document, or a new Redis row with no key.
  • One value inside a change the generator still writes. An Elasticsearch nested-leaf edit next to a normal field, a Redis Value edit on a list key next to a TTL edit, or an etcd Value set to NULL, which was written back as the old value.

The host took that list unchecked. RowChangeStatementFactory returned the plugin's statements as given, and buildRowWrites wrapped them with no row count. The only completeness checks (validate and 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, finishSuccessfulSave cleared 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. Only updateOne runs, and the app reports success.

The host path had the same hole for inserts. On a .generic dialect other than Databend, allDefaultsInsertStatement returns 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 the rowIndex of every change it writes. A change, or a value in one, that the driver cannot write is thrown as PluginRowWriteRefusal(rowIndex:reason:) rather than left out. nil still means the host generates.
  • A default for drivers that implement only generateStatements. It runs that once on the whole set and returns the statements unchanged, so MongoDB's deleteMany batching 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.
  • Both new types are non-frozen and have explicit public initializers.

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, _index or _score (_id typed into a new row still names the document). Also refuses a document with no _id, and a value JSON cannot hold, such as nan in a number field. Example reason: "'identifiers.type' is a field of a nested array. Edit the array in 'identifiers' instead."

  • Redis. Refuses:

    • a Value edit on any type but a string, or on a key whose type is unknown
    • a NULL Value, a Type edit, or a key renamed to NULL
    • a TTL that is not a whole number of seconds above 0 (-1 and NULL still run PERSIST)
    • a new key with no name, an unsupported type, or a non-numeric TTL

    A per-slot DEL names the rows in its slot. Database addressing keeps each write's rows, and its SELECTs 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 \"\" and 0 already did. Refuses an edit to Version, ModRevision or CreateRevision, and a key renamed to NULL or empty.

Host

  • RowWriteCoverage is 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.
  • AttributedStatement carries the RowIDs it writes, and delete chunks keep theirs. validate, with its hasPrefix(\"UPDATE\") check, and the separate delete count are gone.
  • Host gaps throw rowsNotIdentifiable(table, kind) as before. An all-DEFAULT insert the dialect cannot spell is now refused instead of dropped.
  • Driver gaps throw DataWriteError.changesNotWritable, which names the kind and the count: "Cannot save changes to 'items'. The driver cannot write a new row." A thrown refusal becomes changeRefused with 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."
  • The save banner adds "Nothing was saved, and every change is still pending. Undo what cannot be written, or make it with a query, then save again." Preview SQL shows the same error.
  • buildRowWrites generates once instead of twice. Driver steps keep expectedRowCount nil.
  • The inspector's multi-row save goes through the same rule. It now throws instead of writing only some of the selected rows.
  • Removed the uncalled 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:

    • Elasticsearch new row with only identifiers.type typed: POST {} and no refusal before; now refused with its reason.
    • etcd Value set to NULL: put k v1 (the old value) before; now put k \"\".
    • 200 rows × 20 edited cells through the default: 4,001 generateStatements calls before; now 201.
  • verify.sh build and verify.sh plugins (all 40 plugins) pass.

  • Lint: 0 violations on the 29 Swift files the commit changes.

  • verify.sh docs passes. localization.py plugins and localization.py verify return 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:

  • Live, Redis 8.10.2, running the commands this generator produced:

    • The mixed Value+TTL edit on a list key is refused and sends nothing. The list keeps a,b and TTL -1.
    • The TTL-only edit sends EXPIRE probe:list 600.
    • The per-slot deletes remove all three keys.
  • 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 through generateRowWrites. Round 3 found a new Redis key with a negative TTL other than -1 saved without its EXPIRE; 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 _id with 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 generateRowWrites remain. The host sees only whether a change got a statement, not whether each value in it was written. Known cases:

    • SurrealDB drops a DEFAULT, or an id/in/out edit, set next to another edit (SurrealStatementGenerator.swift:66). It also drops a record id typed into a new row that does not parse (:102).
    • MongoDB drops a truncated value (PR B).

    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:

    • An etcd Value edit on a leased key sends put without --lease, which detaches the lease.
    • Duplicating a row writes __DEFAULT__ into a plugin engine's key column: a Redis key named __DEFAULT__, or PUT /_doc/__DEFAULT__ on Elasticsearch.
    • The etcd revision columns were documented as read-only but accept edits. Save now refuses those edits; marking the columns generated would stop the edit at the cell.
  • 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 saveChanges headlessly with a driver stub instead.

@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, 4:29 PM

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

This branch was successfully deployed

1 active deployment
staging - docs — 0eab0989 Deployed Sep 26, 2026 by mintlify[bot]
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