Skip to content

Report children on a hierarchy attach - #10489

Open
gmazoyer wants to merge 3 commits into
stablefrom
gma-20260902-10485
Open

Report children on a hierarchy attach#10489
gmazoyer wants to merge 3 commits into
stablefrom
gma-20260902-10485

Conversation

@gmazoyer

@gmazoyer gmazoyer commented Sep 2, 2026

Copy link
Copy Markdown
Contributor

Why

A hierarchical schema declares parent and children under one identifier, parent__child. The peer changelog looked the other side of the edge up by identifier alone and kept the first match, which is parent. Attaching a child told its parent that its own parent had become the child, and the real change on children was never announced.

Every consumer of node events saw the wrong relationship. A computed attribute whose query reads children filters events on the fields it reads, so it dropped the event and never recomputed. A user action rule matching children never fired.

Closes #10485

Scope against the issue

The issue lists three scenarios. This PR fixes the first one. The other two need no change.

  • A new peer points its own parent at the node. Fixed and tested.
  • Update(data: {children: [...]}) from the parent side. Already worked. The primary changelog names that relationship directly, so it was never ambiguous. I verified the secondary changelog is identical before and after this change, and added a test to hold it.
  • Touching an unrelated attribute does not recompute. This is the selective recompute design, not a defect. The trigger narrows infrahub.field.name to the fields the transform reads.

What changed

  • A hierarchy peer now gets an event naming children, with the right cardinality.
  • The peer side is resolved by the mirrored direction: parent is outbound, so it maps to the inbound children.
  • Deleting a node that has both a parent and a child now reports both sides. The mapper that builds a deleted node's relationship changelog was keyed on the identifier, so the second hierarchy edge reused the first edge's mapper and overwrote it. A middle node reported a single parent entry pointing at its own child.
  • A cardinality-one peer that moved described its previous peer with the new peer's schema, so the relationship named for the old peer could be one it does not declare. The removed side is now resolved from the previous peer kind, as the removal branch already did.
  • A pair holding one relationship per identifier on each side keeps its current behaviour.

Merge and rebase were never affected. core/diff/query_parser.py already disambiguates the same way, so this follows an established pattern rather than a new one. RelationshipAdd and RelationshipRemove share the fixed function.

How to review

Start with peer_relationships in backend/infrahub/core/changelog/models.py, then the mapper key in backend/infrahub/core/query/relationship.py.

The fallback deserves the most attention. When no candidate mirrors the local direction, every candidate is returned, and the result is a guess. Two schemas reach it, both accepted by validation:

  • A third kind reuses the parent__child identifier with the default bidirectional direction. Validation only checks the peers the hierarchy pair itself declares, so it never sees this one. Both entries produced are schema-invalid: the changelog names a peer_kind the relationship does not allow.
  • A hierarchical node declares its own children under a different identifier, so it holds only parent under parent__child. The lone candidate does not mirror, and reporting it reproduces the original bug in a narrow shape.

Returning nothing instead would drop a real event, which is the failure this PR fixes, so the branch keeps every candidate and always logs a warning naming the peer kind, the identifier, the direction and the candidates. A user action rule on a falsely reported name still runs its generator, so that log line is the signal to alert on. Two unit params pin both shapes.

Deciding whether validate_identifiers should reject these schemas outright would make the branch dead code. That is a schema-validation change with its own compatibility risk, so it is not in this PR.

How to test

INFRAHUB_USE_TEST_CONTAINERS=1 uv run pytest backend/tests/component/core/changelog/ backend/tests/unit/core/changelog/ --neo4j

The hierarchy tests fail on the parent commit and pass here. Validated on release-1.10 as well: it sends the same wrong event, and only worked because the trigger carried no field filter before 1.11.0.

Impact & rollout

  • Backward compatibility: the event payload changes for a hierarchical attach, detach and delete. It was wrong before.
  • Config/env changes: none.

Checklist

  • Tests added/updated
  • Changelog entry added
  • External docs updated (if user-facing or ops-facing change)
  • Internal .md docs updated (internal knowledge and AI code tools knowledge)
  • I have reviewed AI generated content

@github-actions github-actions Bot added the group/backend Issue related to the backend (API Server, Git Agent) label Sep 2, 2026

@cubic-dev-ai cubic-dev-ai Bot left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

All reported issues were addressed across 3 files (changes from recent commits).

Shadow auto-approve: would not auto-approve because issues were found.

Re-trigger cubic

Comment thread backend/infrahub/core/changelog/models.py Outdated
@codspeed-hq

codspeed-hq Bot commented Sep 2, 2026

Copy link
Copy Markdown

Merging this PR will not alter performance

✅ 13 untouched benchmarks


Comparing gma-20260902-10485 (e98ca2d) with stable (369e89f)1

Open in CodSpeed

Footnotes

  1. No successful run was found on stable (ee269fe) during the generation of this report, so 369e89f was used instead as the comparison base. There might be some changes unrelated to this pull request in this report.

A hierarchical schema declares `parent` and `children` under one
identifier, `parent__child`. The peer changelog looked the other side
of the edge up by identifier alone and kept the first match, which is
`parent`. So attaching a child told its parent that its own `parent`
had become the child, and the real change on `children` was never
announced.

Every consumer of node mutation events saw the wrong relationship. A
computed attribute whose query reads `children` filters events on the
fields it reads, so it dropped the mislabelled event and never
recomputed. Before 1.11.0 the same wrong event still matched, because
the trigger carried no field filter at all.

Resolve the peer side by the mirrored direction instead: `parent` is
outbound, so it maps to the inbound `children`. A pair that holds one
relationship per identifier on each side keeps its current behaviour.
When the direction resolves nothing, report every candidate rather
than guess one.

Fixes #10485

@cubic-dev-ai cubic-dev-ai Bot left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

0 issues found across 3 files (changes from recent commits).

Confidence score: 5/5

  • Automated review surfaced no issues in the provided summaries.
  • No files require special attention.

Shadow auto-approve: would auto-approve. Fixes hierarchical attach/detach changelogs so the peer is told its children relationship moved instead of its own parent, with a component test pinning the corrected behavior. The change is bounded to peer-relationship resolution and covered by a failing-before test.

Re-trigger cubic

@cubic-dev-ai cubic-dev-ai Bot left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

0 issues found across 6 files (changes from recent commits).

Confidence score: 5/5

  • Automated review surfaced no issues in the provided summaries.
  • No files require special attention.

Shadow auto-approve: would auto-approve. Fixes hierarchy changelogs so attach/detach report the peer's mirrored children/parent and middle-node deletes report both sides, with tests pinning the corrected behavior. Bounded bug fix confined to relationship-changelog resolution.

Re-trigger cubic

@gmazoyer
gmazoyer force-pushed the gma-20260902-10485 branch 4 times, most recently from 211b020 to 96efefc Compare September 3, 2026 08:01
Both sides of a hierarchy share one identifier, so the peer side of an
edge cannot be resolved by identifier alone. Three defects followed from
that.

The mapper that builds a deleted node's relationship changelog was keyed
on the identifier, so the second hierarchy edge reused the first edge's
mapper and overwrote it. A node losing a parent and a child reported one
`parent` entry that pointed at the child. Key the mapper on the resolved
relationship instead.

A cardinality-one peer that moved described its previous peer with the
new peer's schema, so the relationship named for the old peer could be
one it does not declare. Resolve the removed side from the previous peer
kind, as the removal branch already does.

An unresolvable peer side is reported whole rather than guessed, and now
always logs. The warning used to fire only for two or more candidates,
which left the single wrong-direction candidate silent even though that
is the case that guesses.

Also record the shared-identifier rule in the schema knowledge page, and
correct two rows that stated the opposite.
@gmazoyer
gmazoyer force-pushed the gma-20260902-10485 branch 2 times, most recently from 467be87 to 9853c40 Compare September 3, 2026 09:31
@gmazoyer
gmazoyer marked this pull request as ready for review September 3, 2026 09:50
@gmazoyer
gmazoyer requested a review from a team as a code owner September 3, 2026 09:50


# A generic whose two members name their side of `shared__link` differently.
DIFFERING_PEER_NAMES: dict[str, Any] = {

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Very specific schema we don't have anywhere else, hence being inlined here.

Comment on lines +127 to +128
# A hierarchical node whose own children relationship uses a different identifier, so it holds
# only one side under `parent__child`.

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

do we actually allow this?

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Maybe not on purpose, but yes. If a node inherits a hierarchy but declares its own children with a different identifier, add_hierarchy_node skips generating children, so the node keeps only parent under parent__child. I checked it against a real registered schema and validation accepts it, and peer_relationships then returns that single non-mirroring parent. I made the comment say how the shape arises, since it was not obvious from the test alone.

@cubic-dev-ai cubic-dev-ai Bot left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

All reported issues were addressed across 1 file (changes from recent commits).

Shadow auto-approve: would not auto-approve because issues were found.
Tip: Review your code locally with the cubic CLI to iterate faster.

Re-trigger cubic

Comment thread backend/tests/unit/core/changelog/test_models.py
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

group/backend Issue related to the backend (API Server, Git Agent)

Projects

None yet

Development

Successfully merging this pull request may close these issues.

bug: TransformPython computed attribute never recomputes after initial catch-up when its query traverses a cardinality-many relationship

2 participants