fix(knowledge): prevent cyclic page reparenting in Context Center#29552
fix(knowledge): prevent cyclic page reparenting in Context Center#29552sonika-shah wants to merge 2 commits into
Conversation
Moving a KnowledgePage under itself or one of its own descendants was accepted and rewrote the subtree's fully-qualified names into an ever-growing self-referential chain (a.b.a.b.a...), corrupting every page in the subtree. The corrupted FQNs later surface in the UI as the banner "page instance for <fqn> not found". Add a cycle guard in KnowledgePageRepository.prepare() that rejects a reparent when the target parent is the page itself or one of its descendants, throwing a 400 (mirrors GlossaryTermRepository). Valid moves at every level - nesting, cross-parent subtree cascade, leaf->top, same-level reparent, parent->child - are unaffected. Adds CatalogExceptionMessage.invalidPageMove and unit tests for the cycle-detection logic.
❌ 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
![gitar-bot[bot]](https://avatars.githubusercontent.com/in/827041?s=60&v=4){kind=small}
![greptile-apps[bot]](https://avatars.githubusercontent.com/in/867647?s=60&v=4){kind=small}
| private void validateParentHierarchy(Page page) { | ||
| EntityReference parentRef = page.getParent(); | ||
| if (parentRef != null && parentRef.getId() != null) { |
There was a problem hiding this comment.
prepare() call — validateParentHierarchy fetches the parent from the database even when update=false (creates) or when the parent hasn't changed. On create, page.getFullyQualifiedName() is always null so isCyclicParentMove can never return true, yet the getEntityReferenceById lookup still fires. Compare GlossaryTermRepository, which gates the equivalent check on existingTerm != null (i.e., only on updates). Skipping the check when the page has no FQN yet avoids this overhead.
| private void validateParentHierarchy(Page page) { | |
| EntityReference parentRef = page.getParent(); | |
| if (parentRef != null && parentRef.getId() != null) { | |
| private void validateParentHierarchy(Page page) { | |
| // A new page has no FQN yet and cannot be a descendant of anything; skip the check. | |
| if (page.getFullyQualifiedName() == null) { | |
| return; | |
| } | |
| EntityReference parentRef = page.getParent(); | |
| if (parentRef != null && parentRef.getId() != null) { |
🔴 Playwright Results — 2 failure(s), 36 flaky✅ 4437 passed · ❌ 2 failed · 🟡 36 flaky · ⏭️ 39 skipped
Genuine Failures (failed on all attempts)❌
|
…er pages Adds KnowledgePageMoveIT covering the full reparent matrix end-to-end: nesting at each level, move of a top-level page under a parent, subtree FQN cascade (single and multi-level), leaf -> top, same-level reparent, and the two cycle cases (move under self / under own descendant) which must return 400 and leave the FQNs untouched. Complements the KnowledgePageRepositoryTest unit coverage for the cycle guard.
Code Review ✅ Approved 1 resolved / 1 findingsAdds a cycle-detection guard to ✅ 1 resolved✅ Quality: Cyclic-move guard lacks integration test for the 400 path
OptionsDisplay: compact → Showing less information. Comment with these commands to change:
Was this helpful? React with 👍 / 👎 | Gitar |
|
What
Prevent cyclic reparenting of Context Center pages (
KnowledgePage). Moving a page under itself or one of its own descendants is now rejected with a400instead of silently corrupting the subtree.Symptom
Users hit a banner in the Context Center:
page instance for <fqn> not found. That string is the raw backend 404 (CatalogExceptionMessage.entityNotFound("page", fqn)) surfaced by the UI. It appears when the UI requests a page by its computedparentFQN.childNamebut the stored page has a different FQN — i.e. a stored-vs-computed FQN mismatch.Root cause
Context Center "Articles" are
KnowledgePageentities (pageType=Article) served at/v1/contextCenter/pages.Normal reparenting already works correctly at every level — nesting, cross-parent subtree cascade, leaf→top, same-level, parent→child (
KnowledgePageUpdater.updateParentrecomputes the FQN and cascades to descendants). That part is not the bug.The bug: there was no guard against moving a page under its own descendant. The move was accepted (
200) and the FQN rewrite cascaded into an ever-growing, self-referential chain. Example after movingOther(an ancestor ofA) underA:Every page in the subtree is corrupted, and the resulting FQNs no longer match what the UI computes → the "not found" banner.
GlossaryTermRepositoryalready guards the equivalent move (invalidGlossaryTermMove+ descendant check);KnowledgePageRepositorydid not.Fix
KnowledgePageRepository.prepare()now callsvalidateParentHierarchy(), which rejects the reparent when the new parent is the page itself or a descendant of it (FullyQualifiedName.isParent(parentFqn, pageFqn)), throwingIllegalArgumentException→400.isCyclicParentMove(pageId, pageFqn, parentId, parentFqn).CatalogExceptionMessage.invalidPageMove(page, newParent).Legitimate moves are unaffected — the guard only triggers when the target parent is the moved page or one of its descendants.
Testing
Unit —
KnowledgePageRepositoryTest(7 cases): self, direct child, deep descendant → rejected; unrelated parent, top-level, shared-name-prefix sibling, new-page-without-FQN → allowed. Green.End-to-end (local build, 7-scenario move matrix):
Global FQN-consistency sweep over all created pages:
inconsistent: 1(before) →inconsistent: 0(after).Note
This only prevents new corruption. Pages already corrupted by a previous cyclic move keep their bad FQNs until re-moved.
Greptile Summary
This PR adds a cycle-prevention guard to
KnowledgePageRepository.prepare()that rejects any reparent move where the target parent is the page itself or one of its own descendants, throwingIllegalArgumentException(HTTP 400) instead of silently cascading a corrupt FQN chain. The fix mirrors the equivalent guard already present inGlossaryTermRepositoryandContainerRepository.KnowledgePageRepository.prepare()now callsvalidateParentHierarchy(), which fetches the parent from the DB and delegates to the pureisCyclicParentMove()helper; the helper checks identity by UUID and usesFullyQualifiedName.isParentto detect descendant relationships via dot-boundary-aware prefix matching.CatalogExceptionMessage.invalidPageMoveadds a consistent error message alongside the existinginvalidGlossaryTermMove/invalidContainerMovemessages.KnowledgePageRepositoryTest) exercising all cycle/non-cycle combinations and a full integration test suite (KnowledgePageMoveIT) covering create nesting, cascaded renames, leaf promotion, and cycle rejection end-to-end.Confidence Score: 5/5
Safe to merge — the guard is purely additive, correctly rejects only the cyclic case, and leaves all legitimate reparent paths unaffected.
The cycle-detection logic is correct: UUID identity check catches self-moves, and FullyQualifiedName.isParent(parentFqn, pageFqn) (dot-boundary-aware prefix match) catches descendant moves, using the same argument order as the equivalent GlossaryTermRepository guard. Null guards on both FQN and ID prevent false positives on creates. Unit and integration tests cover all relevant branches including the shared-name-prefix sibling edge case.
No files require special attention.
Important Files Changed
Sequence Diagram
%%{init: {'theme': 'neutral'}}%% sequenceDiagram participant Client participant PageResource participant KnowledgePageRepository participant DB Client->>PageResource: "PATCH /v1/contextCenter/pages/{id}" PageResource->>KnowledgePageRepository: patch(original, updated) KnowledgePageRepository->>KnowledgePageRepository: "prepare(page, update=true)" KnowledgePageRepository->>KnowledgePageRepository: validateParentHierarchy(page) alt parentRef present KnowledgePageRepository->>DB: getEntityReferenceById(parentRef.getId()) DB-->>KnowledgePageRepository: parent (with FQN) KnowledgePageRepository->>KnowledgePageRepository: isCyclicParentMove(pageId, pageFqn, parentId, parentFqn) alt "pageId == parentId OR isParent(parentFqn, pageFqn)" KnowledgePageRepository-->>PageResource: IllegalArgumentException (400) PageResource-->>Client: 400 Bad Request else valid move KnowledgePageRepository->>KnowledgePageRepository: KnowledgePageUpdater.updateParent() KnowledgePageRepository->>DB: update FQN + cascade to descendants DB-->>KnowledgePageRepository: ok KnowledgePageRepository-->>PageResource: updated Page PageResource-->>Client: 200 OK end end%%{init: {'theme': 'base', 'themeVariables': {"darkMode": true, "background": "#0d1117", "primaryColor": "#21262d", "primaryTextColor": "#e6edf3", "primaryBorderColor": "#8b949e", "lineColor": "#8b949e", "textColor": "#e6edf3", "edgeLabelBackground": "#161b22", "actorBkg": "#21262d", "actorBorder": "#8b949e", "actorTextColor": "#e6edf3", "actorLineColor": "#8b949e", "signalColor": "#8b949e", "signalTextColor": "#e6edf3", "noteBkgColor": "#373320", "noteBorderColor": "#d4a72c", "noteTextColor": "#f0e6c0", "labelBoxBkgColor": "#21262d", "labelBoxBorderColor": "#8b949e", "labelTextColor": "#e6edf3", "loopTextColor": "#e6edf3", "activationBkgColor": "#30363d", "activationBorderColor": "#8b949e"}}}%% sequenceDiagram participant Client participant PageResource participant KnowledgePageRepository participant DB Client->>PageResource: "PATCH /v1/contextCenter/pages/{id}" PageResource->>KnowledgePageRepository: patch(original, updated) KnowledgePageRepository->>KnowledgePageRepository: "prepare(page, update=true)" KnowledgePageRepository->>KnowledgePageRepository: validateParentHierarchy(page) alt parentRef present KnowledgePageRepository->>DB: getEntityReferenceById(parentRef.getId()) DB-->>KnowledgePageRepository: parent (with FQN) KnowledgePageRepository->>KnowledgePageRepository: isCyclicParentMove(pageId, pageFqn, parentId, parentFqn) alt "pageId == parentId OR isParent(parentFqn, pageFqn)" KnowledgePageRepository-->>PageResource: IllegalArgumentException (400) PageResource-->>Client: 400 Bad Request else valid move KnowledgePageRepository->>KnowledgePageRepository: KnowledgePageUpdater.updateParent() KnowledgePageRepository->>DB: update FQN + cascade to descendants DB-->>KnowledgePageRepository: ok KnowledgePageRepository-->>PageResource: updated Page PageResource-->>Client: 200 OK end endReviews (2): Last reviewed commit: "test(knowledge): add move-scenario integ..." | Re-trigger Greptile