fix: route MongoBaseStore.persist_data through __setitem__, not the leaf - #13
Merged
Merged
Conversation
persist_data was the one value-side method MongoBaseStore did not override, so it was delegated leaf-bound: it bypassed both _id_of_key and _data_of_obj and issued a whole-document replace_one(upsert=True) built from the outer (untransformed) object. On any store built with set_key_and_data_fields this silently destroyed every field not present in the outer shape and rewrote the document in the wrong shape. Add MongoBaseStore.persist_data alongside its six siblings (contains_value/iter_values/contains_item/iter_items/append/extend), routing through self[key] = data so the wrapper's own key/data transforms apply. key defaults to being inferred from data[ID] for backward compatibility. Closes #11 Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>
Member
Author
|
Note (added after merge, per updated crowsnest review policy): this change was self-reviewed only — no independent adversarial review was run before merging. Flagging for the record; the fix itself (routing persist_data through setitem) is additive-only and covered by hosted CI, but a second pass would be prudent if this is revisited. |
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
1 participant
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.
Summary
Fixes the destructive gap identified in #11 (part 1 of 2 — the docs/design gap in part 2 was independently resolved by #10/mongodol/views.py's bulk-read resolver, a different and more general mechanism than the one #11 originally proposed for that part; see triage comment for detail).
persist_datawas the one value-side methodMongoBaseStoredid not override (unlike its six siblings:contains_value/iter_values/contains_item/iter_items/append/extend). Being delegated leaf-bound, it bypassed both_id_of_keyand_data_of_objand issued a whole-documentreplace_one(upsert=True)built straight from the outer (untransformed) object — silently destroying every field not present in the outer shape on any store built withset_key_and_data_fields.Fix
Adds
MongoBaseStore.persist_data(data, key=None), routing throughself[key] = dataso the wrapper's own key/data transforms apply — the same pattern as its six siblings.keydefaults to being inferred fromdata[ID]for backward compatibility with the previous call signature (persist_data(data)), matching the exact remediation#11proposed and verified against the leaf/wrapper delegation mechanics.Test plan
wads ci-local(format + lint) green; the test leg needs a live mongo, unavailable in this sandbox — deferred to hosted CI (mongo:5.0 service container), public repo.gh pr checks --watchgreen before merge.Closes #11
🤖 Generated with Claude Code
https://claude.ai/code/session_011HSBVhDjRU4apSLcRkavv9