fix(teams): prevent circular team hierarchy on create/update paths#30063
Conversation
validateHierarchy only walked team.getParents(), so a cycle introduced via team.getChildren() (the other direction of the PARENT_OF edge), or by naming the same team as both parent and child in one request, slipped through and later tripped the runtime guard in SubjectContext. Validate both edge directions and reject parent/child overlap in prepare(), which covers POST, PUT, PATCH, bulk create and CSV import. Adds integration tests for the child-ancestor and overlap cases. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
❌ PR checklist incompleteThis PR cannot be merged until the following are addressed on its linked issue:
The fields live on the linked issue in the Shipping project (open the issue → right sidebar → Projects). After you set them, re-run this check (or push a commit) — issue/project changes do not re-trigger it automatically. Maintainers can bypass this check by adding the |
github-actions
Bot
added
backend
safe to test
![greptile-apps[bot]](https://avatars.githubusercontent.com/in/867647?s=60&v=4){kind=small}
![gitar-bot[bot]](https://avatars.githubusercontent.com/in/827041?s=60&v=4){kind=small}
🟡 Playwright Results — all passed (29 flaky)✅ 4540 passed · ❌ 0 failed · 🟡 29 flaky · ⏭️ 95 skipped
🟡 29 flaky test(s) (passed on retry)
How to debug locally# Download playwright-test-results-<shard> artifact and unzip
npx playwright show-trace path/to/trace.zip # view trace |
Address PR review findings. A single create request with parents=[P] and children=[C] where C is an ancestor of P slipped through, because the per-edge walks read only persisted state and the new team has no id yet. Collect the team's ancestor names once by walking up from both its proposed (in-request) and persisted parents, then check each child against that set. This catches the create-time cross-edge cycle and replaces the per-child re-walk, cutting the children check from O(children*depth) to O(depth) DB queries. Removes the now-subsumed parent/child overlap helper. Adds an integration test for the create-time cross-edge cycle. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
Code Review ✅ Approved 2 resolved / 2 findingsTightens team hierarchy validation by implementing bidirectional checks in TeamRepository to prevent circular dependencies. Addresses issues with cross-edge cycles and redundant ancestor walking while successfully adding covering integration tests. ✅ 2 resolved✅ Edge Case: Cross-edge cycle from new parent+new child not caught
✅ Performance: Ancestor chain re-walked once per child
OptionsDisplay: compact → Showing less information. Comment with these commands to change the behavior for this request:
Was this helpful? React with 👍 / 👎 | Gitar |
|
Code Review ✅ Approved 2 resolved / 2 findingsTightens team hierarchy validation by implementing bidirectional checks in TeamRepository to prevent circular dependencies. Addresses issues with cross-edge cycles and redundant ancestor walking while successfully adding covering integration tests. ✅ 2 resolved✅ Edge Case: Cross-edge cycle from new parent+new child not caught
✅ Performance: Ancestor chain re-walked once per child
OptionsDisplay: compact → Showing less information. Comment with these commands to change the behavior for this request:
Was this helpful? React with 👍 / 👎 | Gitar |
…30063) * fix(teams): reject circular hierarchy from child edge and overlap validateHierarchy only walked team.getParents(), so a cycle introduced via team.getChildren() (the other direction of the PARENT_OF edge), or by naming the same team as both parent and child in one request, slipped through and later tripped the runtime guard in SubjectContext. Validate both edge directions and reject parent/child overlap in prepare(), which covers POST, PUT, PATCH, bulk create and CSV import. Adds integration tests for the child-ancestor and overlap cases. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com> * fix(teams): detect cross-edge cycles on create, walk ancestors once Address PR review findings. A single create request with parents=[P] and children=[C] where C is an ancestor of P slipped through, because the per-edge walks read only persisted state and the new team has no id yet. Collect the team's ancestor names once by walking up from both its proposed (in-request) and persisted parents, then check each child against that set. This catches the create-time cross-edge cycle and replaces the per-child re-walk, cutting the children check from O(children*depth) to O(depth) DB queries. Removes the now-subsumed parent/child overlap helper. Adds an integration test for the create-time cross-edge cycle. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com> --------- Co-authored-by: Claude Opus 4.8 (1M context) <noreply@anthropic.com> (cherry picked from commit 659f288)
Describe your changes:
Fixes #
TeamRepository.validateHierarchy()only walkedteam.getParents(), so a cycle introduced throughteam.getChildren()(the other direction of the samePARENT_OFedge) or by naming the same team as both a parent and a child in one request was persisted and later tripped the runtime "Circular dependency detected" guard inSubjectContext. This validates both edge directions and rejects parent/child overlap inprepare(), closing the gap for POST, PUT, PATCH, bulk create and CSV import (all paths funnel throughprepare()).Type of change:
High-level design:
N/A — small, self-contained change to one validation method.
Tests:
Use cases covered
Backend integration tests
openmetadata-integration-tests/.../TeamResourceIT.java:test_teamHierarchy_childCannotBeAncestorandtest_teamHierarchy_parentAndChildOverlapRejected.test_importCsv_circularDependency_*continue to pass (message contract preserved).Playwright (UI) tests
UI screen recording / screenshots:
Not applicable.
Checklist:
Greptile Summary
This PR prevents circular team hierarchies during team create and update flows. The main changes are:
Confidence Score: 5/5
This looks safe to merge.
Important Files Changed
Reviews (4): Last reviewed commit: "Merge branch 'main' into team-hierarchy-..." | Re-trigger Greptile
Context used (3)