M2M: Handle intermediate objects created during transform#4849
Draft
aziemchawdhary-gs wants to merge 26 commits into
Draft
M2M: Handle intermediate objects created during transform#4849aziemchawdhary-gs wants to merge 26 commits into
aziemchawdhary-gs wants to merge 26 commits into
Conversation
Repros infinite recursion in meta::pure::functions::meta::allNestedProperties when class graph contains a bidirectional non-association reference. Observed failure: [ERROR] PureTestBuilder$PureTestCase.testAllNestedPropertiesTerminatesOnCycle » StackOverflow at core_pure_corefunctions_metaExtension.Root_meta_pure_functions_meta_allNestedProperties_Class_1__AbstractProperty_MANY_(core_pure_corefunctions_metaExtension.java:491) (recursive frames via CompiledSupport.mapToManyOverMany -> lambda -> allNestedProperties)
Threads a Map<String, Class<Any>> visited set through the recursion, keyed on elementToPath() for stable identity. Public arity-1 entry seeds an empty map; the private overload checks and short-circuits on already-visited classes. Tests run: 1006, Failures: 0, Errors: 0.
Repros the buildPropertyPathUptoOwner bug where the navigation map is keyed by Type but $prop.owner can be an Association. The fixture uses a CompanyEmployee source with an intermediate that exposes a Person, then accesses Person.address (defined on association Person_Address). buildPropertyPathUptoOwner walks back from .street; when it reaches the address node, ownerClass(address) = Person (not source CompanyEmployee), the buggy branch fires, and $t->get($prop.owner) looks up an Association in a Type-keyed map -> empty -> ->toOne() fails. Observed failure: testIntermediateOverAssociation(org.finos.legend.pure.m3.execution.test.PureTestBuilder$PureTestCase) <<< ERROR! Execution error at (resource:/core/pure/graphFetch/graphExtension.pure line:745 column:72), "Cannot cast a collection of size 0 to multiplicity [1]" at Root_meta_pure_graphFetch_buildPropertyPathUptoOwner_PropertyPathTree_1__Class_1__Map_1__PropertyPathTree_1_
- Look up the navigation map by Class (via ownerClass()) instead of $prop.owner, which is PackageableElement and may be an Association for association-owned properties. The map is keyed by Type, so an Association lookup silently returned empty and the subsequent ->toOne() crashed with "Cannot cast a collection of size 0 to multiplicity [1]" (the failure mode demonstrated by Task 3's test testIntermediateOverAssociation). - Add a depth guard (max 64) with a clear assertion message so an unreachable owner produces a diagnosable error instead of a stack overflow. - Drop the dangling $prop.owner comment and the dead newclz locals. Tests run: 1007, Failures: 0, Errors: 0.
…igation
The Order class has two properties typed Customer (primaryCustomer and
secondaryCustomer). The mapping wraps primaryCustomer through an
intermediate while accessing secondaryCustomer directly. The current
childPropertiesMap is a Map<Type, AbstractProperty> keyed by Customer
that overwrites on collision, so when buildPropertyPathUptoOwner needs
to navigate back to Order via the intermediate, it picks whichever of
{primaryCustomer, secondaryCustomer} happened to win the map insertion
- which is not in general the intended primaryCustomer.
Observed failure (testCollidingReturnTypes):
expected: 'Order(amount, primaryCustomer(name), secondaryCustomer(name))'
actual: 'Order(amount, secondaryCustomer(name))'
The primaryCustomer branch is dropped because Customer's slot in
childPropertiesMap was overwritten by secondaryCustomer.
childPropertiesMap is now Map<Type, List<AbstractProperty<Any>>> built via groupBy on the property return type, so Order with two Customer-typed properties (primaryCustomer + secondaryCustomer) keeps both entries instead of one overwriting the other. buildPropertyPathUptoOwner now consults a deterministic picker (pickIntermediateProperty) that prefers an exact return-type match and breaks ties by lexicographic property name, so the navigation back to owner is reproducible across runs/Pure compiler versions. findSubTreeWithOwnerOrPropertyTreesFromOwner's signature is updated to take the List-valued map; behaviour is unchanged (still only checks isNotEmpty). Used the orElse(list([])).values workaround for Map::get to defensively handle missing keys, as suggested in the task plan. Fixes the failing test testCollidingReturnTypes added in Task 5. Tests run: 1008, Failures: 0, Errors: 0.
meta::pure::functions::collection::groupBy already returns Map<K, List<X>>[1] natively. The keyValues -> map -> newMap pipeline introduced in Task 6 was a no-op rewrap. Direct groupBy result is the expected Map<Type, List<AbstractProperty<Any>>>[1]. Tests run: 1008, Failures: 0, Errors: 0.
testSourceAccessViaSupertypeProperty exercises a direct Dog->DogTarget mapping where Dog.species is inherited from Animal (supertype). The PropertyPathNode arm of findSubTreeWithOwnerOrPropertyTreesFromOwner already handles this case, so this test passes today - kept as a regression guard. testIntermediateOverSubtype is the failing case: a KennelIntermediate slot is typed at Animal (the supertype), but the mapping's source is Dog (the subtype). When the algorithm encounters an Animal-typed Class node in the property tree and tries to match it against owner Dog, the current check $clz->isStrictSubType($ownerClass) (Animal->isStrictSubType(Dog)) is false, so the supertype-rooted subtree is not recognised and the walk-up fails. Observed failure: testIntermediateOverSubtype - expected 'Dog\n(\n breed\n species\n)' but got 'Dog\n(\n breed\n)' - the species node is missing because the Animal-rooted property subtree was not recognised as belonging to source Dog.
…e directions The Class arm and PropertyPathNode arm previously only accepted X->isStrictSubType(ownerClass) - i.e. the tree class is a subtype of owner. The pre-branch behaviour also accepted the supertype direction (ownerClass->isStrictSubType(X)). This commit reinstates that direction on both arms so a tree whose root class is a supertype of owner is still recognised as a subtree-of-owner. Note: testIntermediateOverSubtype still fails after this change - it relies on isTreeRootOwner accepting the supertype direction as well, which is fixed by Task 10.
supertype-typed slot is not handled The fixture is kept as a placeholder for follow-up investigation. The supertype-direction case requires more careful changes to buildPropertyPathUptoOwner's recursion than Task 10's planned isTreeRootOwner tweak alone provides: making isTreeRootOwner return false on empty treeOwner regresses three other tests that rely on the silent legacy-fallback. The right design needs to thread a supertype-aware terminator through buildPropertyPathUptoOwner that preserves the navigation rather than early-returning the leaf sub-tree.
…rees found If findSubTreeWithOwnerOrPropertyTreesFromOwner returns empty, building a fresh ^PropertyPathTree with empty children silently produces an empty source tree and loses information. Add an inner check: when no owner-belonging subtrees are found, fall back to the legacy propertyTreeToGraphFetchTree($owner) path instead. Tests run: 1009, Failures: 0, Errors: 0.
Tests run: 1009, Failures: 0, Errors: 0.
- testIntermediateWithSubtypeTarget (Task 13): M2M subtype mapping where TgtCircle's source goes through an intermediate. Currently the subType branch's children are not enriched into the source tree; marked test.ToFix. - testUnionWithIntermediateMember (Task 14): union mapping where one member uses an intermediate, the other doesn't. Operation set handling in calculateSourceTree is a documented gap (see TODO in testOnSourceRoot.pure); marked test.ToFix. - testTwoHopIntermediate (Task 15): two-level intermediate chain; exercises buildPropertyPathUptoOwner recursion. Passes. - testUnreachableOwnerGivesClearError (Task 16): negative path - intermediate references a foreign class with no route back to the source. The unreachable-owner assertion isn't currently reached in this configuration; marked test.ToFix. Test_Pure_Core: 1010 tests run (baseline 1009 + 1 newly-passing test; 3 marked test.ToFix following the existing testIntermediateOverSubtype precedent in the same file).
Mappings whose root class is an Operation (e.g. *Tgt: Operation { union(a, b) })
previously crashed calculateSourceTree with "Match failure: OperationSetImplementation"
because the top-level dispatch only had a PureInstanceSetImplementation arm.
Resolve the operation to its constituent PureInstanceSetImplementations, compute a
source tree per branch, and combine using the first branch as the base with the
remaining branches attached as subTypeTrees. Drops the test.ToFix marker on
testUnionWithIntermediateMember.
For a mapping whose transform routes through an intermediate class that has no property path back to the source class (e.g. wrap-and-leave-foreign pattern), calculateSourceTree previously took the isTreeRootOwner permissive fallback and silently returned a degenerate source tree. Add an owner-reachability precondition at the top of enrichSourceTreeNodeForProperty: when the property tree references classes the owner can neither share hierarchy with nor reach via findSubTreeWithOwnerOrPropertyTreesFromOwner, raise the same diagnostic buildPropertyPathUptoOwner would have raised. Gated on $setImplementation.srcClass == $owner so the check is inert during operation-set fold dispatch into sibling subtrees (where srcClass != owner is the legitimate cross-branch shape). Drops the test.ToFix marker on testUnreachableOwnerGivesClearError. Also hoists childPropertiesMap / propertyTreesBelongingToOwner out of the !isTreeRootOwner branch to share with the precondition.
…class
A target tree like `TgtCatalog { shapes { area }, shapes->subType(@TgtCircle) { radius } }`
where the property mapping (`shapes : $src.shapes`) targets only the base class
(TgtShape) was silently dropping the subType subtree: the propertyMappings filter
required the mapping's target to be a (reflexive) subtype of the requested
subType, which excludes the base mapping.
Two changes in enrichSourceTreeNodeForProperty:
- Accept the property mapping when the requested subType is a subtype of the
mapping's target class (in addition to the existing direction). Mappings like
per-subset b[b1]/b[b2] still match only their own subset; single-default
mappings now also flow through to handle the subtype subtree.
- When tgtPgft.subType is set, use it as the lookup class for childSIs so the
subtype's set implementation (e.g. *TgtCircle) is picked up, not the
property's natural return type's set impl.
Drops the test.ToFix marker on testIntermediateWithSubtypeTarget.
For a mapping whose source class is a subtype (Dog) of an intermediate slot's
return type (Animal), accessing an inherited property (species) via the
intermediate (`$src->wrap().animal.species`) raised:
No intermediate-class property of Dog returns Animal - cannot build path
back to owner.
The two contributing checks both treated owner-direction as the only valid
shape:
- buildPropertyPathUptoOwner's guard accepted propOwner == owner and
propOwner subtype-of-owner but not owner subtype-of-propOwner. The
inheritance direction (species defined on Animal, accessed on Dog) hit
the candidate lookup and asserted with no candidates.
- addPropertyGraphFetchTrees' PropertyPathNode arm only accepted node.class
== ownerClass or subtype-of-ownerClass, so even if the path reached the
synthesised tree the inherited leaf was filtered out (the doc's Attempt 1
symptom: silently produces `Dog { breed }`).
Fixing both directions together: the buildPropertyPathUptoOwner guard now
also accepts owner subtype-of-propOwner (the inheritance case — leave pTree
in place), and addPropertyGraphFetchTrees mirrors the Class-node arm's
bidirectional check for PropertyPathNode so the inherited property attaches
directly under owner. Drops test.ToFix on testIntermediateOverSubtype.
|
Test Results 1 095 files - 31 1 095 suites - 31 3h 6m 48s ⏱️ - 1h 37m 47s Results for commit 2904199. ± Comparison against base commit e6f76b0. ♻️ This comment has been updated with latest results. |
testUnionSubTypeSourceWithType (propertyUnion.pure) failed with "No intermediate-class property of _S_PersonC returns _S_Person - cannot build path back to owner." even though _S_Person is the supertype of _S_PersonC and should be excluded from the precondition's non-trivial-tree-classes set. Cause: Pure's lazy evaluation can hand back distinct Class objects for the same type when navigated from different roots. The existing isStrictSubType helper in this file explicitly switches to element-path comparison for the same reason. The Gap 4 precondition was still using object-identity `contains`, so the supertype reference encountered in the property tree did not match anything in the owner's generalisations and slipped through into nonTrivialTreeClasses. Compare by elementToPath() for: owner-vs-tree-class equality, the owner-generalisations contains check, the reverse generalisations check, and the setImpl.srcClass == owner gate. Adds testUnionSubtypeSourceReads- InheritedProperty as a focused regression covering the same shape in Test_Pure_Core.
aziemchawdhary-gs
force-pushed
the
sourceTreeIntermediate
branch
from
16b07e9 to
7f4c93c
Compare
testNewOperatorInMapping (functionInMapping.pure) failed with
"No intermediate-class property of Firm returns PersonB - cannot build
path back to owner." for the mapping `count : ^PersonB(...).middleNames->size()`.
The transform constructs a literal PersonB and reads its empty middleNames
list — no source data is required, so the expected source tree is `Firm {}`
and the legacy fallback's empty output is correct.
My owner-reachability precondition was relying on
findSubTreeWithOwnerOrPropertyTreesFromOwner, which only matches tree
classes against childPropertiesMap by exact-class equality. PersonB is not
in Firm's childPropertiesMap (Firm.employees returns Person, not PersonB),
so the lookup missed the inheritance reachability and the precondition
fired on a benign literal-construction case.
Align with the followups doc's exact condition: also accept when any of
owner's class-typed-property return types shares hierarchy (in either
direction, via element path) with a non-trivial tree class. PersonB
shares hierarchy with Person (subtype-of) and is therefore considered
reachable — precondition stays inert. Gap 4's genuine unreachable case
(SrcRoot has zero class-typed properties at all) still raises because the
ownerClassTypedReturnTypes set is empty.
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.
No description provided.