fix(mongo): CRUD collection routing (targetEntity->type->name), selector must not be shadowed#190
Merged
Merged
Conversation
…RDBMS parity) The RDBMS exporter's update/upsert/delete resolve the target table via resolve_target_entity_from_metadata (targetEntity -> type -> name), so a plain target='db.update' iterate that carries only the statement name works. The mongo exporter instead demanded explicit type/selector metadata and raised "'type' or 'selector' statement's attribute is missing" - a converted store-update iterate (<iterate source='db' sourceEntity='db_order' target='db.update'>) has neither. All three mongo CRUD ops now route through one _routing() helper mirroring the RDBMS precedence, preserving any selector filter. Fixes the shop-mongodb demo's aggregate write-back and unblocks the mongodb-delete/upserter demos.
Review of the previous commit (be3e296) before push found a real regression it introduced: MongoDBExporter._routing() unconditionally injected a name-based META_TARGET_ENTITY fallback via dict.setdefault, even when the write metadata already carried a META_SELECTOR. Since MongoDBClient.update/upsert/delete all dispatch "if META_TARGET_ENTITY in query" before checking for a selector, this silently redirected every selector-driven update/upsert/delete whose statement name doesn't match the selector's own collection - the common real-world idiom of naming a cleanup/delete step for what it does, not for the collection it targets (name="delete", name="cleanup", ...), used throughout the existing test suite. 7 of the pre-existing mongo tests failed as a result (confirmed by running them, not by static reading) - test_mongodb_reference (all 3), test_mongodb_consumer, test_mongodb_cross_collection, test_mongodb_cyclic_source, test_mongodb_with_type - each because a cleanup/delete step silently no-oped on the real collection while "succeeding" against a bogus one, leaving stale data for the next run. _routing() now only injects the name-based fallback when NEITHER targetEntity NOR selector is already present - a selector always resolves its own collection. Also fixed the one existing unit test that was supposed to guard this (test_selector_filter_preserved_alongside_resolved_collection): its own fixture used the same string for both the statement name and the selector's collection, so the assertion held even under the buggy code and could not have caught this. Replaced with a discriminating case using different names, and confirmed by temporarily reverting the fix - the new test fails exactly as expected, the 7 previously-broken tests fail exactly as before, and mypy/ruff stay clean either way.
Neither the routing precedence (name -> type -> targetEntity for writes, selector -> sourceEntity -> type for reads) nor the "selector must not be shadowed" invariant had DSL-level coverage before this - only Python unit tests on MongoDBExporter._routing() in isolation, which is exactly what let the previous commit's regression through (its own fixture coincidentally used a matching name/collection). This exercises every combination through the real engine against a live mongo: - INSERT: name-only fallback, type wins over name, targetEntity wins over type. - UPDATE: name-only fallback via a genuinely metadata-free write (sourceEntity drives the read but is never added to write metadata, mirroring the actual reported gap - "<iterate source='db' sourceEntity='db_order' target='db.update'>"), and selector routing survives a mismatched statement name (the regression just fixed). - UPSERT / DELETE: selector routing survives a mismatched statement name, including upsert's insert-new-doc path. - READ (source side): sourceEntity wins over type, type-only works, selector wins regardless of a mismatched sourceEntity (type+selector together is itself a model-validation error, so that combination isn't reachable). Verified discriminating: temporarily reverted the fix and confirmed exactly the 3 selector-routing tests fail (the other 3, which don't depend on the fix, still pass) - this test would have caught the regression the previous commit introduced.
|
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
On top of #189: the RDBMS exporter's update/upsert/delete resolve their target table via
resolve_target_entity_from_metadata(targetEntity -> type -> name), so a plaintarget='db.update'iterate carrying only the statement name works. The mongo exporter instead demanded explicittype/selectormetadata and raised'type' or 'selector' statement's attribute is missing— a converted<iterate source='db' sourceEntity='db_order' target='db.update'>has neither. Fixes the shop-mongo demo's aggregate write-back and unblocks the mongodb-delete/upserter demos.A real regression was found and fixed before this went further (second commit): the first fix's
_routing()helper unconditionally injected a name-basedtargetEntityfallback viadict.setdefault, even when the metadata already carried aselector. SinceMongoDBClient.update/upsert/deleteall checktargetEntitybefore falling through toselector, this silently redirected every selector-driven write whose statement name doesn't match the selector's own collection — the common idiom of naming a cleanup/delete step for what it does (name="delete",name="cleanup"), used throughout the existing test suite. 7 pre-existing tests failed as a result (confirmed by running them, not by reading the diff):test_mongodb_reference(all 3),test_mongodb_consumer,test_mongodb_cross_collection,test_mongodb_cyclic_source,test_mongodb_with_type— each because a cleanup step silently no-oped on the real collection, leaking stale data into the next run._routing()now only injects the name-based fallback when neithertargetEntitynorselectoris already present. The one existing unit test meant to guard this had a fixture where the statement name coincidentally matched the selector's collection, so it passed under the buggy code too — replaced with a discriminating case (different names) and verified by temporarily reverting the fix.Third commit: a DSL E2E matrix (
test_mongodb_routing_matrix/) proving the routing precedence end to end against a live mongo — insert (name/type/targetEntity precedence), update/upsert/delete (name-only fallback + selector-not-shadowed), and the read/source side (selector -> sourceEntity -> type, deliberately no name fallback). Verified discriminating the same way: reverting the fix fails exactly the 3 selector-routing cases.Test plan
pytest tests_ce/unit_tests/test_mongo_crud_routing.py— 4/4 passedRUNTIME_ENVIRONMENT=development pytestacross all mongo external-service folders + the newtest_mongodb_routing_matrix— 65 passed, 1 skippedmypy datamimic_ceandruff check— clean