Repository navigation
fix: key-taking operations become free functions that honour dol wrappers (#1) - #2
Merged
Merged
Conversation
…pers (#1) Every non-dunder method of the stores that took a key (replace, batch, partition, add_container, add_database, CosmosDatabase.delete, CosmosAccount.delete) ran bound to the leaf under a dol wrapper and acted on the caller's unmapped key: under a key codec, replace overwrote another document, delete(force=True) emptied and dropped the wrong container or database, and partition() stripped every wrapper. They move to cosmodol.ops (re-exported): replace_item, batch_items, partition, add_container, delete_container, add_database, delete_database. Each takes the store first and maps the key through every layer with dol.inner_most_key (values through _data_of_obj). partition refuses a wrapped store unless given wrap=. batch_items takes (op, key[, value]) so its ids are mappable. The old method names raise AttributeError naming the replacement; a conformance test inventories every public method. Also: delete_container(force=True) now deletes items with the container's real partition key (the old cascade read a non-existent _partitionKey field), and FakeContainerProxy gains execute_item_batch. Same design as s3dol ADR-0011. Closes #1 Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
…iew of #2) - Operations on existing resources check k in store through the outer store first, so a filt_iter scope stops replace_item, batch read/replace/patch/ delete, and the forced delete_container/delete_database. - add_database returns the leaf's value instead of re-reading through the wrapper (which could raise after the database was created). - Forced container delete projects hierarchical (MultiHash) partition keys as a list and passes NonePartitionKeyValue for items without one. - batch_items accepts per-op SDK kwargs and names the SDK-triple mistake. - Conformance test also sees classmethods and overridden Mapping methods. - dol>=0.3.59 (inner_most_key exported). Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
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.
Closes #1.
What
Every store method that took a key ran bound to the leaf under a
dolwrapper and got the caller's unmapped key. They move tocosmodol.ops(re-exported fromcosmodol), each taking the store first and mapping the key through every layer withdol.inner_most_key:CosmosItems.replace(k, v, etag=)replace_item(store, k, v, etag=)(value via each layer's_data_of_obj)CosmosItems.batch(sdk_triples)batch_items(store, [(op, k[, v]), ...]): ids are now explicit, hence mappableCosmosPartitionedItems.partition(pk)partition(store, pk, *, wrap=None): refuses a wrapped store unlesswrap=says how to wrap the childCosmosDatabase.add_container(name, ...)add_container(db_store, name, ...)CosmosDatabase.delete(name, force=)delete_container(db_store, name, force=)CosmosAccount.add_database(name, ...)add_database(account_store, name, ...)CosmosAccount.delete(name, force=)delete_database(account_store, name, force=)The old names raise an
AttributeErrornaming the replacement (sohasattris False and class-wrapping still works).querystays a method (it takes SQL, not a key) and is now documented as speaking raw Cosmos ids/documents. A conformance test inventories every public method of the four store classes and fails on any new one, forcing the "does this take a key?" judgement at review. Design follows s3dol ADR-0011: free functions are handed the outer store, so they are also immune towrapped_self's lost-reference hole (tested with a temporary wrapper).Also fixed
delete_container(force=True)deleted each item withpartition_key=item.get("_partitionKey"), a field that does not exist unless the pk path is literally/_partitionKey; it now projects the container's real partition-key path.FakeContainerProxy.execute_item_batch(all-or-nothing) so batches are testable offline..with_filter(...)that was never implemented.Breaking
Pre-1.0 (0.0.x), no fleet dependents (no package in the fleet imports cosmodol). Existing tests that called the removed methods were migrated to the functions with the same assertions.
Tests
All issue repros, on both delegation routes (class-wrap and instance-wrap):
tests/test_ops.py.pytest --doctest-modules cosmodol: 63 passed (emulator tests deselected as before).Adversarial review
A fresh Opus reviewer read the diff and the issue and tried to break it. Verdict: ship-after-fixes, 4 confirmed defects, all fixed in the second commit:
filt_iter) were bypassed by destructive ops. Fixed: operations on existing resources first checkk in storethrough the outer store (replace_item, batch read/replace/patch/delete, forced delete_container/delete_database). The docstring now states exactly what a function can and cannot honour.add_databasecreated the database then raised when the wrapper hid the name. Fixed: returns the leaf's value.NonePartitionKeyValue.from dol import inner_most_keyneeds dol >= 0.3.59. Fixed: pinned.Also taken from its "plausible" list: per-op kwargs in
batch_items(ETag-conditional ops), a clear error for raw SDK triples, a conformance test that also sees classmethods and overridden Mapping methods, a clearerpartition()refusal. Deferred (not regressions; the old methods bypassed them too): write guards, value caches,preset=hooks, scope on creating ops → #3.Behaviour change worth noting:
delete_container(db, name)withoutforceisdel db[name]and refuses a non-empty container (the olddb.delete(name)dropped it).🤖 Generated with Claude Code