Fix #788: make MemoryStore equality reflect data isolation - #1123
Open
rkingsbury wants to merge 6 commits into
Open
Fix #788: make MemoryStore equality reflect data isolation#1123rkingsbury wants to merge 6 commits into
rkingsbury wants to merge 6 commits into
Conversation
…gomock-ng MemoryStore now probes host:port (default localhost:27017) on connect and, if a MongoDB server answers, stores data in a real, ephemeral MongoDB database for full MongoDB compatibility and performance. Each instance uses a unique maggma_memory_<uuid> database that is dropped automatically (via weakref finalizer) when the store is garbage collected or the interpreter exits, so the user never has to set up or clean up a database. If no server is reachable the store transparently falls back to an in-process mongomock-ng database. - close() is now a no-op that leaves the store usable, preserving the historical mongomock behavior relied upon by the builders (pymongo raises on use-after-close). Use connect(force_reset=True) to discard contents. - JSONStore/FileStore inherit the behavior via a shared _connect_collection; MontyStore gets its own close() so its disk-backed client still closes. - Adds backward-compatible host/port/mongoclient_kwargs/server_selection_timeout_ms kwargs; server_selection_timeout_ms=0 forces the mongomock backend. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
Addresses GitHub AI security warnings about except clauses that only pass without an explanatory comment (the two best-effort ephemeral-database drops). Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
Previously MemoryStore.close() was a no-op so that callers could keep querying a "closed" store. This makes close() actually disconnect: querying a closed store now raises StoreError, and the store must be reconnected first. Data is preserved across a close()/connect() cycle -- the real-MongoDB backend keeps it in the ephemeral server-side database (dropped by the finalizer on GC/exit), and the mongomock backend keeps its in-process client alive -- so reconnect restores the contents while connect(force_reset=True) starts fresh. The Builder framework already only queries stores before closing them (run/serial/multi all do connect -> get_items -> update_targets -> finalize, and MapBuilder.finalize queries before super().finalize()). The only query-after-close was in the builder test suite, which relied on the old no-op close(); those tests now reconnect the target before asserting, matching the existing pattern in test_copy_builder.test_run. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
test_close expected an AttributeError from querying a closed S3Store (because S3Store.close() sets s3_bucket=None). With MemoryStore.close() now being a genuine close, the S3 index is genuinely closed and raises StoreError when queried, before the s3_bucket access is reached. Expect StoreError, which is the more principled signal that the store was closed. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
…ation Two MemoryStores instantiated with the same collection_name are isolated (they do not share data), yet MemoryStore.__eq__ compared only collection_name and returned True. Equality now reflects the actual data source: each MemoryStore has a unique ephemeral database (_database), and __eq__/__hash__ include it, so distinct instances are unequal while a store still equals itself, matching the "same data source" semantics of MongoStore. FileStore is backed by an on-disk directory and previously inherited MemoryStore's equality; it now compares by its tracked path so two FileStores for the same directory remain equal. A class-level _database=None default keeps subclasses that skip MemoryStore.__init__ (MontyStore) working. Adds regression tests for the isolated-and-unequal MemoryStore case and for FileStore path-based equality. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
![github-advanced-security[bot]](https://avatars.githubusercontent.com/in/57789?s=60&v=4){kind=small}
The materialsproject#788 fix made MemoryStore.__eq__ depend on the unique per-instance _database, but that id was regenerated on every from_dict, so a store no longer equaled its own serialized round-trip copy. MultiStore.add_store caches a round-tripped copy and later matches stores by equality (get_store_index), so the lookup returned None and self._stores[None] raised "TypeError: list indices must be integers or slices, not NoneType" in the multistore query/count tests. _database is now an (internal) init parameter, so it is carried through as_dict/from_dict: a round-tripped store keeps the same identity and compares equal to the original, while a freshly constructed store still gets its own new identity (preserving the materialsproject#788 fix). Adds a regression test for the round-trip equality. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
Codecov Report❌ Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## main #1123 +/- ##
==========================================
+ Coverage 69.53% 69.93% +0.40%
==========================================
Files 47 47
Lines 4103 4171 +68
==========================================
+ Hits 2853 2917 +64
- Misses 1250 1254 +4 ☔ View full report in Codecov by Harness. 🚀 New features to boost your workflow:
|
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
2 participants
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 #788.
Two
MemoryStores instantiated with the samecollection_nameare isolated — they do not share data — yetMemoryStore.__eq__compared onlycollection_nameand returnedTrue. That inconsistency is what #788 reports.This makes equality reflect the actual data source, consistent with
MongoStore(which is equal only when it points at the same physical database/collection). EachMemoryStoreis backed by its own unique ephemeral database (_database), so:MemoryStores — even with the samecollection_name— are not equal (and hash differently);Details
MemoryStore.__eq__/__hash__now include the unique per-instance_database. A class-level_database = Nonedefault keeps subclasses that don't callMemoryStore.__init__(i.e.MontyStore) working.FileStoreis backed by an on-disk directory, not isolated in-memory data, so it previously inheritedMemoryStore's equality incorrectly (allFileStores sharedcollection_name="file_store", so any two compared equal while hashing differently — a broken__eq__/__hash__invariant). It now compares by its trackedpath, so twoFileStores for the same directory are equal and two for different directories are not.JSONStorealready compared bypathsand is unchanged.Tests
test_memory_store_eq_isolated— two same-collection_nameMemoryStores are isolated (count()2 vs 0) and therefore unequal with differing hashes; a store equals itself.test_file_store_eq—FileStores for the same directory are equal (and hash-equal); different directories are not.Verified on both backends:
tests/stores/test_mongolike.py+tests/stores/test_file_store.py+tests/builderspass (53 withmongod; new tests also pass on the mongomock fallback), andtests/stores/test_aws.pyis unaffected.Note
This branches off #1121 and will show that PR's commits until it merges; review/merge #1121 first. It intentionally addresses #788 separately from #1121, per discussion there.
🤖 Generated with Claude Code