fix(plugin-mongodb): rename and remove MongoDB fields from the Structure tab, carrying the validator along and refusing what would break an index, a view or a document - #3161
Open
datlechin wants to merge 2 commits into
Conversation
…ure tab, carrying the validator along and refusing what would break an index, a view or a document
|
Preview deployment for your docs. Learn more about Mintlify Previews.
💡 Tip: Enable Automations to automatically generate PRs for you. |
…e-field-edits # Conflicts: # CHANGELOG.md # TablePro/Core/Database/DatabaseManager+Schema.swift # TablePro/Core/Events/AppCommands.swift # TablePro/Core/Plugins/PluginMetadataRegistry+RegistryDefaults.swift # TablePro/Resources/Localizable.xcstrings # TablePro/Views/Main/Extensions/MainContentCoordinator+DatabaseObjectTools.swift # TablePro/Views/Main/Extensions/MainContentCoordinator+Refresh.swift # TablePro/Views/Structure/StructureEditingSession+Apply.swift # TablePro/Views/Structure/StructureEditingSession.swift # TablePro/Views/Structure/TableStructureView+ColumnReorder.swift # TableProTests/Core/Database/DatabaseManagerSchemaChangeRoutingTests.swift # docs/databases/mongodb.mdx # docs/features/table-structure.mdx
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.
Follows #3145, now merged. Based on
mainatc21dc512e.Closes the rename/remove half of #3132.
Root cause
Three defects made MongoDB fields impossible to rename or remove from the Structure tab.
supportsSchemaEditing = false, the driver had nogenerateModifyColumnSQLorgenerateDropColumnSQL, andPluginSchemaOperationhad no column cases, so no driver could refuse or answer a column change.$jsonSchema.properties, so renaming only on the documents leaves the validator requiring the old name and every renamed document invalid.updateMany(..., {$unset})as a plain write, although the Structure tab already marks the removal destructive.Older defects that affect every engine:
ALTER. On MongoDB it also covers the document checks, which can run for as long as the query timeout.updateManychanged before it stopped.refreshData. That reloaded the selected tab of each window in the database whatever table it showed, and asked it to discard its edits, while a background tab on the saved table, and the saving tab's own Data view behind Structure, kept the rows from before the save. A table rebuild and a column reorder sent the same signal for the whole connection.updateOneandupdateManyin the MongoDB shell dropped awriteConcernoption, andmongoc_client_command_simplesends a command with no write concern of its own, so the update went out with the server's default.Fix
Statements. A rename is
db.c.updateMany({"F": {"$exists": true}, "G": {"$exists": false}}, {"$rename": {"F": "G"}})and a removal isdb.c.updateMany({"F": {"$exists": true}}, {"$unset": {"F": ""}}). The filters keep every document atomic and every run idempotent. A document that already holds the new name is skipped, never overwritten, and pressing Save again after a stop finishes only what is left.Write concern. The connection's write concern is read from the libmongoc client once it connects (
w,journalandwtimeoutMS, so an imported connection string counts), and every statement names it:updateMany(..., {"writeConcern": {"w": "majority"}})anddb.runCommand({"collMod": ..., "validator": ..., "writeConcern": ...}). SQL Preview shows it. A concern that asks for no answer (w: 0withoutj: true) goes out asw: 1, because measured on 7.0.43 an unacknowledged update is answeredn: 0whatever it changed. The shell'supdatenow puts a statement'swriteConcernoption on the command. With no write concern configured, nothing is added and the server default applies, as before.Validator. When the validator is a
$jsonSchemathat declares the field inproperties,requiredor propertydependencies, the save starts with acollModthat renames or removes the field there too, keeping key order and the canonical EJSON values. A field only the validator declares can now be removed.A validator that uses the field anywhere the rewrite cannot reach refuses the save:
$exprand query operators;enumwhose entries are documents naming the old or the new name. A document matches such an entry only with exactly its names in its order, and$renamemoves the field to the end, so the rewrite cannot carry it along;$$ROOTor$$CURRENT, to something that reads its names (see the shared rule below).$whereand$functionalready did;patternPropertiespattern that matches the old or the new name;additionalPropertiesrule (anything buttrueor{}) that applies to the old or the new name becausepropertiesdoes not declare it.One whole-document rule for validators and views (
MongoWholeDocumentReads). A bare$$ROOTor$$CURRENTreads every field, except where it stays the document:$replaceRootand$replaceWithof it, directly or through$mergeObjects, a$setFieldor$unsetFieldwith a literal field name, or a branch of$cond,$switch,$ifNullor$let. A$getFieldwith a literal name reads that one field. Anywhere else, placed under a name, in an array, in a variable or handed to$objectToArrayor a comparison, it reads every field. A validator's$exprand a view's pipeline both answer through it, and a test holds them to the same answer for eleven expressions. This narrows one round-3 answer on purpose: a validator's$getField: {field: "a", input: "$$ROOT"}now readsaalone, as it does in a view.Refusals, before anything is written.
_idas source or target; empty, NUL,$-prefixed, dotted and__proto__names; any change to type, nullability, default or comment. These run ahead of fix(plugin-mongodb): create collections from New Table with their fields as a validator, read the fields back, and write values in their declared types #3145's own refusal, which still answers everything else.reviewSchemaChange:system.or missing collection;dropIndexcommand);$listSearchIndexes. The listing's failure is read by code. 6047401 (measured on 6.0.28 and 7.0.43 community) and 31082 SearchNotEnabled (measured on 8.2.12 community with nomongot) mean the server has no search: both refuse$searchthe same way. 40324, the unknown-stage code, no longer means none: an Atlas cluster older than the stage runs$searchover indexes it cannot list. The driver then runs[{$search: {exists: {path: "_id"}}}, {$limit: 1}]. A server that answers it with 40324, 6047401 or 31082 has no search (measured: 5.0.33 and 6.0.5 community answer 40324 to both); any answer, even an empty one, stops the save with "This server runs Atlas Search but cannot list its search indexes, so one that uses the field cannot be ruled out." Any other failure stops it too;viewOnchains and$lookup/$graphLookup/$unionWithat any depth, by any string or non-operator key naming it, or by the whole-document rule.The review keeps the collection's
listCollectionsentry as its newbasis.schemaChangeRefusalBeforeWriting, inside the lease that then writes:basis: validator, level, action, or a drop and recreate since the save was composed;moderate, only a document the old validator accepted counts;After writing, through
schemaChangeShortfallAfterWriting, which now receives the review:basis. This is the target-only race: a document another client wrote with only the new name after the checks and before thecollModpassed the old validator, which did not read that name, and fails the new one. The save fails with "The save did not finish: the updated validator of tr rejects the document with _id "late", most likely written by another client while the save ran. Fix that document, then save again." Undermoderate, only a document the old validator accepts as it now stands counts, as before writing.A read after writing that fails reports that the save ran and to save again.
Host.
StructureChangeManager.holdForSave()), and every staging path refuses while it does. The Structure tab shows Saving Changes….schemaChangeStatementsreturns aSchemaChangeScript(statements, operations, review).executeSchemaChangesasks the before-writing question just before the first statement and the after-writing question just after the last.tableDefinitionDidChange(table:schema:)areDatabaseDriverrequirements with defaults, bridged inPluginDriverAdapter+SchemaChangeChecks.swift. The manager calls them onDatabaseDriverwith no cast to the adapter, so any driver or test double answers them.withSchemaComposerkeeps its existing cast, becauseSchemaStatementGeneratortakes the plugin driver itself.DatabaseManager.reportTableDefinitionChange(table:in:)tells the session's own drivertableDefinitionDidChangeand sendsDatabaseObjectChange(kind: .structure)for the saved table. A table rebuild and a column reorder send it too. The scope-widerefreshDatais gone from all three. Each window, for every tab on that table:markStructureStale()). The saving tab's structure holds staged edits for as long as its save runs, and keeps them when the save stops partway, so it is left alone.Tabs on other tables are untouched.
tableDefinitionDidChangedrops the collection's inferred column kinds, field path kinds, identity kind and declared schema, in every database the driver keyed them under. Measured before: after a rename of a declaredcreatedon another connection, page 2 of the session's browse still listedcreatedbesidemade, from the declared schema a later page reuses.isDestructivejoins the text tier in the Safe Mode kind, so a field removal and a SQL column type change confirm likeDROP COLUMN.columnsAreSampled, true for MongoDB).Edit surface. The Structure tab offers Name and row removal on a collection. Type, Nullable, adding a field and index editing stay read-only. An older installed plugin keeps its own
supportsSchemaEditing = false.PluginKit (additive, pending kit 33). Adds
PluginSchemaOperation.modifyColumnand.dropColumnon the non-frozen enum, thePluginSchemaChangeReviewstruct (refusal,leadingStatements,basis), and four requirements with defaults:reviewSchemaChange(table:schema:operations:),schemaChangeRefusalBeforeWriting(table:schema:operations:review:),schemaChangeShortfallAfterWriting(table:schema:operations:review:)andtableDefinitionDidChange(table:schema:). None of these has shipped, so the after-writing requirement gainingreview:changes nothing a built plugin references.scripts/check-pluginkit-abi.sh b8b2fc7b4shows additions only. v0.75.0 ships kit 32, so there is no bump.Overlap with open branches
fix/refresh-written-table-tabs) makes the same move for every write:DatabaseObjectChangewith.rowsand.structure,refreshStructure(ofTabs:),markStructureStale(),forgetSchemaColumns,SchemaColumnStore.remove, plus freshness stamps and an origin tab. This branch adds the.structurecase and those four pieces under the same names and semantics, without the stamps. On merge, take that branch's versions and keepreportTableDefinitionChangeas the single sender, now feeding its.structurepath.tableDefinitionDidChangestays: that branch does not reach the driver's caches.fix/mongodb-shell-write-concern) sends the connection's write concern with every shell write,updateManyincluded, and honours a statement's own. It does not add one todb.runCommand, so thecollModneeds its explicit member either way. Both branches readwriteConcernErrorinMongoWriteFailure; on merge keep fix(plugin-mongodb): send the connection's write concern with every shell write and name the documents a failed write already changed #3151'sread(fromReply:)and this branch'sconcernFailure(fromReply:)beside it. On merge, take that branch'sMongoScriptCommandBuilder.updateand host write path over this branch's one-line pass-through; the statements' explicit concern is then the same one the connection would add. Both read the concern from libmongoc; foldingMongoWriteConcerninto itswriteConcernJson(client:)is a follow-up.fix/mongodb-views-and-index-order) makesfetchTablesreport views asVIEWandsystem.collections asSYSTEM TABLE. Nothing here duplicates it.Verified
Round 5. Codex found one P1 and four P2s:
CLAUDE.mdbumpscurrentPluginKitVersionat most once per release cycle and every later change reuses the pending number. v0.75.0 shipped 32,mainalready holds 33 for this cycle, and no build has shipped with 33, so no host exists that accepts 33 and lacks these symbols.heldSaveMakesTheRowReadOnly, which fails without the change).collMod'swriteConcernErrorpassed. The shell'sdb.runCommandnow throws on a reply carrying one, which is what mongosh's driver does for any command, so the save stops with the server's reason (commandConcernFailure). This replaces the round-4 note that it would not.$bsonSizefor a document the added bytes would take past MongoDB's limit and refuses with its_id, sinceupdateManywould stop at it partway on every retry (oversizePassCountsGrowth). A server before 4.4 has no$bsonSize; there the check steps aside and the docs say so.MongoFieldDependent, so before and after read the same rules (dependentAfterTheSave).Round 5 checks, on
mainatc21dc512e: the 20 test files this branch changes plusStructureGridDelegateInspectorTests,MongoWriteFailureTestsandStringCatalogIntegrityTests, 256 of 256; the app,MongoDBDriverandAllPluginsbuild; lint 0 violations on the changed Swift files; docs pass; four new plugin strings added throughlocalization.py plugins --add. The 16 MB and dependency paths were not run live this round.Round 4 and earlier:
This pass, on the tree amended into a5b3fc85b. One plugin-only line changed after the test runs (the search probe's success path returns
falsedirectly), andbuild MongoDBDrivercompiled it again:verify.sh test, 43 suites, 666 of 666: the 8 suites this pass touches (MongoFieldChangeAssessmentTests,MongoFieldDataProbeTests,MongoFieldReferencesTests,MongoSearchIndexTests,MongoFieldChangeTests,MongoScriptCommandBuilderTests,DatabaseManagerSchemaChangeRoutingTests,CatalogChangeWindowTests) and 35 neighbours (SQLSchemaProviderTests, whose mock gained the hooks;EvictionTests,MainContentCoordinatorRefreshTests,MainContentCoordinatorLazyLoadTests,DataRefreshScopeTests,SchemaColumnStoreTests,SchemaColumnStoreCancellationTests,ColumnFetchScopeTests, the Structure, Safe Mode, registry, Compare and MongoDB DDL, schema, generator, query builder and write suites, and three materialized view suites).viewReadsEveryFieldThroughTheWholeDocument,viewPassingTheDocumentOn,validatorAndViewAgree,documentEnumEntriesNameFields,listingStageUnknown,serverWithoutSearch,statementCarriesWriteConcern,unacknowledgedConcernIsRaised,validatorStatementCarriesWriteConcern,collectionCacheKeys,catalogChangedSinceComposed,violationAfterWriting,updateWriteConcern,schemaChangeReportsItsTable,sessionDriverForgetsTheTableDefinition,checkAfterWritingGetsTheComposedReview,databaseDriverAnswersThroughTheProtocol,structureChangeReachesEveryTabOnTheTable,structureChangeKeepsEditedRows,structureChangeSparesStagedEdits,structureChangeForgetsCachedColumns. One round-3 expectation changed with the shared rule: a validator's$getFieldof a literalqtyover$$ROOTno longer readsnote..rowshandling for a structure change, the adapter cast, no write concern on the statement or on the shell's update, the view walker without the whole-document rule, 40324 as no search, the cache key match, the enum rule, the after-writing pass replaying the steps), the 8 touched suites went 125 executed, 108 passed, 17 failed, each a new case above or a routing case the refresh change touches. The files were restored from a saved copy and checked by hash, and the suites then passed.verify.sh build(TablePro),build MongoDBDriverandplugins(all 40): pass.verify.sh linton the 35 changed Swift files: 0 violations.verify.sh docs: pass.localization.py plugins --addadded 3 strings, the key this branch no longer uses was removed, andpluginsandverifyare ok for both catalogs.verify.sh abi b8b2fc7b4on the clean amended tree: additions only, no line removed.probe_field-rename-remove_r5, dropped afterwards:slowms: -1. A Majority connection: before, thecollModinsystem.profileand theupdatein the server log carried no write concern; after, both carried{"w": "majority"}. A 2 connection on this standalone: before, the rename applied; after, the server refused theupdateManywith "[2] cannot use 'w' > 1 when a host is not replicated" and no document changed.w: 0throughmongoc_client_command_simple:{"n": 0, "nModified": 0, "ok": 1}for an update that changed 2 documents;w: 0, j: true:n: 2.{_id: "late", new: 42}between the check and thecollMod. Before: "applied", the document then failed the validator, and its next update failed with 121. After: "The save did not finish: the updated validator of tr rejects the document with _id "late" ...". With the document fixed, saving again applied.[{$project: {kv: {$objectToArray: "$$ROOT"}}}, {$project: {names: "$kv.k"}}]: before, renamingstatusapplied and the view's names changed from[_id, status, qty]to[_id, qty, state]; after, refused with "View keys reads status." A view[{$replaceWith: {$mergeObjects: [{note: ""}, "$$ROOT"]}}]letqtyrename in both.{$jsonSchema: {enum: [{_id: 1, old: "a"}, {_id: 2, old: "b"}]}}underwarn: before, the rename applied and both documents then failed the validator; after, the rename and the removal were refused.createdtomade: before[_id, made, n, created], after[_id, made, n].$listSearchIndexesand to$search; 6.0.28 and 7.0.43 answer 6047401 to both; 8.2.12 answers 31082 to both. A rename applied on each, before and after (6.0.5 after only). The fail-closed answer needs a server that knows$searchand not the listing stage, which no community build is; unit tests cover it.Deliberately not fixed here
Structure edit gate on views, time series and
system.collections. The gate reads the object kind the sidebar listed, and on this base MongoDB lists every namespace as a table. Locking these before an edit needs a new per-object driver question asked when the Structure tab loads, plus plumbing intoStructureEditGateand the grid delegate's own copy of it.fix/mongodb-views-and-index-orderlists views asVIEWandsystem.collections asSYSTEM TABLE, which MongoDB's.table-only edit matrix then locks with no further change. A time series collection stays a table there, so its Name and row removal stay on offer, and the save refuses it at SQL Preview and at Save, before anything is written.Residual races. MongoDB has no conditional
collModand no lock a client can take:collModis replaced by the save's. The docs list this under Limitations.collModis not overwritten. TheupdateManystatements are validated against it, and one that fails is reported as a failed save.moderate, the check after writing reads the old validator against a document as it now stands. A renamed document the old validator rejected only through a name it required,GamongrequiredwhilepropertiesdeclaredF, can be reported although the save left it as invalid as it found it.collModwhose write concern was not met has already changed the validator when the save stops; the message gives the server's reason, and the documents were not touched.Conservative refusals, accepted:
{$push: "$$ROOT"}in a view included. Rewrite those from a query tab.propertiesdoes not declare is refused.Carried from earlier rounds:
$getFieldin a view is covered by unit tests only: 7.0.43 rejects a view whose field argument is not a constant.$listSearchIndexesoutput the MongoDB manual documents; no Atlas cluster was reachable.db.createView, and itscreateIndexdropswildcardProjection,language_overrideand other options it does not pass through.fetchIndexestypes every MongoDB index BTREE and reads key paths in dictionary order, which is why index editing stays read-only.No UI test. The rename flow needs a live
mongod, and CI has no MongoDB server, so the live checks stand in for it. The tab refresh is covered atapplyObjectChangeandexecuteSchemaChanges;fix/refresh-written-table-tabsbringsTableChangeReloadUITestsfor the same reload on SQLite and replaces this mechanism on merge. A save that fails raises an error alert with no window under XCTest, which hangs the test host, so the failure path is covered at theDatabaseManagerlevel instead.