Skip to content

fix: classify reverse-Reference traversal through entity inheritance#522

Open
hjotha wants to merge 1 commit intomendixlabs:mainfrom
hjotha:submit/find-by-attr-list-of-subtype
Open

fix: classify reverse-Reference traversal through entity inheritance#522
hjotha wants to merge 1 commit intomendixlabs:mainfrom
hjotha:submit/find-by-attr-list-of-subtype

Conversation

@hjotha
Copy link
Copy Markdown
Contributor

@hjotha hjotha commented May 5, 2026

Summary

addRetrieveAction decided whether retrieve $X from $V/M.Assoc was a reverse traversal (child-side → parent-side, returning a list) by comparing the start variable's entity type to the association's child entity name exactly. Inheritance broke that check: when the start variable was a subclass of the association's child entity, the comparison missed and the result variable was registered as the parent entity singleton instead of List of <parent>.

Downstream find($List, attr = value) then dispatched to FindByExpression instead of Find because resolveListOperationMember read the variable type as a non-list. Studio Pro authors Find for the simple equality shape; emitting FindByExpression triggers mx check CE0117 "Error in expression at List operation activity 'Find by expression'" because the runtime can no longer see the qualified attribute path the model expects.

Reproduction

A Reference association with Parent=Header / Child=Message. A microflow holds a $Request variable typed as a Request entity that extends Message. Traversing $Request/Module.Headers should yield List of Header:

$HeaderList = retrieve $Request/Module.Headers;
$Match = find($HeaderList, Key = 'Authorization');

Before this PR: mx check reports CE0117 at the Find by expression activity. After this PR: mx check reports 0 errors and the result variable is correctly typed as List of Module.Header.

Fix

New entityIsSubtypeOf helper performs an inheritance-aware walk (mirrors the existing resolveAttributeInEntityHierarchy traversal). The exact-match startVarType == childEntityQN checks in addRetrieveAction are replaced with entityIsSubtypeOf(startVarType, childEntityQN). The replacement covers both the expandReverseReference DatabaseRetrieveSource branch and the AssociationRetrieveSource fallback that registers the result type. Direct (non-inheriting) traversal still works the same way; the helper short-circuits on identity.

Test plan

  • New TestAddRetrieveAction_ReverseRefThroughInheritedChild — reverse Reference through an inherited child entity registers List of <parent>.
  • New TestAddRetrieveAction_ReverseRefDirectChild — guards the non-inheritance path: exact child match continues to register List of <parent>.
  • Existing reverse-Reference / owner-both / non-persistable-parent tests continue to pass (go test ./mdl/executor/).
  • go test ./... passes.
  • Roundtrip audit on the 20-microflow random sample that surfaced the bug: previously failing microflow now matches (mpr_check_failed → match), 19/19 unaffected microflows held their prior status — no regressions.

🤖 Generated with Claude Code

@github-actions
Copy link
Copy Markdown

github-actions Bot commented May 5, 2026

AI Code Review

Critical Issues

  • None found

Moderate Issues

  • None found

Minor Issues

  • Test placement: The new test file cmd_microflows_builder_retrieve_inherited_subtype_test.go is placed in mdl/executor/ alongside other microflow builder tests. While acceptable, consider whether it might be better grouped with other retrieve-specific tests if they exist (though none are visible in the diff).
  • Helper function documentation: The new entityIsSubtypeOf function lacks a comment explaining its return value semantics (e.g., "returns true if candidate is ancestor or subtype"). Though the implementation is clear, explicit documentation would aid maintenance.
  • Edge case handling: The helper returns false for empty strings or nil backend. While safe (preserves old behavior), adding a comment to clarify this is intentional would improve readability.

What Looks Good

  • Bug fix correctness: The inheritance-aware traversal logic correctly handles the reported scenario ($Request extending Message traversing to Headers), resolving the CE0117 error by properly typing the result as List of <parent>.
  • Test coverage:
    • New test TestAddRetrieveAction_ReverseRefThroughInheritedChild validates the inheritance case.
    • New test TestAddRetrieveAction_ReverseRefDirectChild guards against regressions in the direct-match path.
    • Existing tests continue to pass, and roundtrip audit on 20 microflows confirms no regressions.
  • Code quality:
    • The helper entityIsSubtypeOf mirrors the existing resolveAttributeInEntityHierarchy traversal pattern, ensuring consistency.
    • Short-circuits on identity (candidateQN == ancestorQN) for efficiency.
    • Uses a seen map to prevent infinite loops in generalization chains.
    • Changes are localized to the two places needing inheritance awareness (both expandReverseReference and AssociationRetrieveSource branches).
  • Atomicity: The PR addresses a single, well-defined bug without unrelated changes.
  • Robustness: Safe handling of nil backend and empty strings prevents panics.

Recommendation

Approve. The PR correctly fixes the inheritance-related bug in reverse Reference traversal, includes comprehensive tests, maintains backward compatibility, and follows the project's architectural patterns. No blocking issues were identified. Minor documentation improvements are suggested but not required for merge.


Automated review via OpenRouter (Nemotron Super 120B) — workflow source

addRetrieveAction decided whether `retrieve $X from $V/M.Assoc` was a
reverse traversal (child-side → parent-side, returning a list) by
comparing the start variable's entity type to the association's child
entity name *exactly*. Inheritance broke that check: when the start
variable was a subclass of the association's child entity (e.g. a
HttpRequest variable traversing a HttpHeaders association whose Child
is HttpMessage), the comparison missed and the result variable was
typed as the parent entity *singleton* instead of `List of <parent>`.

Downstream `find($List, attr = value)` then dispatched to
`FindByExpression` instead of `Find` because resolveListOperationMember
read the variable type as a non-list. Studio Pro authors `Find` for the
simple equality shape; emitting `FindByExpression` triggers `mx check`
CE0117 "Error in expression at List operation activity 'Find by
expression'" because the runtime can no longer see the qualified
attribute path the model expects.

Introduce entityIsSubtypeOf, an inheritance-aware walk (mirrors the
existing resolveAttributeInEntityHierarchy traversal), and replace the
exact-match `startVarType == childEntityQN` checks in addRetrieveAction
with `entityIsSubtypeOf(startVarType, childEntityQN)`. The replacement
covers both the expandReverseReference DatabaseRetrieveSource branch
and the AssociationRetrieveSource fallback that registers the result
type. Direct (non-inheriting) traversal still works the same way; the
helper short-circuits on identity.

Two regression tests:
  - reverse Reference through an inherited child registers
    `List of <parent>` (covers the original bug)
  - reverse Reference with an exact child match still registers
    `List of <parent>` (guards the non-inheritance path)

Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
@hjotha hjotha force-pushed the submit/find-by-attr-list-of-subtype branch from 1833b52 to 8a983ac Compare May 5, 2026 14:47
@github-actions
Copy link
Copy Markdown

github-actions Bot commented May 5, 2026

AI Code Review

Review Summary

This PR fixes a bug in reverse association traversal when the start variable is a subtype of the association's child entity. The fix introduces an inheritance-aware helper function entityIsSubtypeOf to replace exact entity name matches. The changes are localized to the microflow executor's retrieve action handling.

Critical Issues

  • None found

Moderate Issues

  • None found

Minor Issues

  • None found

What Looks Good

  • Bug Fix Correctness: The solution properly handles inheritance chains by walking the generalization hierarchy, mirroring the existing resolveAttributeInEntityHierarchy pattern.
  • Test Coverage: Added two comprehensive unit tests:
    • TestAddRetrieveAction_ReverseRefThroughInheritedChild verifies the fix for inheritance cases
    • TestAddRetrieveAction_ReverseRefDirectChild guards against regressions in the direct match path
  • Code Quality:
    • Helper function is well-commented and follows existing patterns
    • Changes are minimal and focused (only 4 call sites updated)
    • No unnecessary modifications or refactorings
  • Validation: Author reports successful roundtrip audit on 20-microflow sample showing the previously failing case now passes with no regressions
  • Checklist Compliance:
    • No MDL syntax changes (so syntax/full-stack consistency checks don't apply)
    • Tests are proper unit tests without time.Sleep
    • Change is scoped to a single bug fix
    • No duplication with existing functionality

Recommendation

Approve the PR. The fix correctly addresses the inheritance issue in reverse association traversal, is well-tested, and maintains backward compatibility for non-inheritance cases. The solution follows established patterns in the codebase.


Automated review via OpenRouter (Nemotron Super 120B) — workflow source

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants