Fixes #24294: support re-parenting a Container via PATCH#28201
Conversation
Allow the PATCH API to update a Container's `parent`, cascading the FQN change to every descendant container, nested column FQN, tag-usage row, entity-link, policy condition, and search-index document — same shape as GlossaryTerm re-parenting. Scoped to same-StorageService moves; cross-service parents are rejected with HTTP 400. Adds parent-aware fluent SDK methods in Java (`Containers.under(...)`, `FluentContainer.withParent(...)`/`withoutParent()`) and Python (`Containers.set_parent`, `Containers.clear_parent`), unit tests for the validation logic, 11 integration tests covering the cascade and rejection paths, and 9 + 2 SDK tests. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
github-actions
Bot
added
backend
safe to test
![gitar-bot[bot]](https://avatars.githubusercontent.com/in/827041?s=60&v=4){kind=small}
The single-transaction cascade in updateParent locks every descendant row in storage_container_entity, rewrites their JSON, renames their tag_usage rows, and issues an Elasticsearch updateByQuery — all while holding row locks that block any concurrent write on the subtree. At ~10k+ descendants this becomes a multi-minute outage on the cluster. Add an indexed COUNT(*) preflight in ContainerUpdater.updateParent that short-circuits with HTTP 400 when the moved subtree exceeds openmetadata.container.maxReparentDescendants (default 10000), pointing the operator at the system property if they have measured the impact and accept it. Run BEFORE invalidateCacheForRenameCascade so a rejected request pays no cache-eviction cost. Tests: 5 new unit tests in ContainerRepositoryParentValidationTest cover under/at/over-limit and the system-property override; 2 new IT methods in ContainerResourceIT exercise the end-to-end reject path with a test-scoped low threshold and confirm at-limit moves still succeed. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
🟡 Playwright Results — all passed (10 flaky)✅ 4111 passed · ❌ 0 failed · 🟡 10 flaky · ⏭️ 86 skipped
🟡 10 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 |
…race (#24294) Four fixes from the gitar-bot review: 1. Performance — validateContainerParent short-circuits when the proposed parent id matches the original. Skips an unnecessary Entity.getEntity round-trip on every container PATCH/PUT that doesn't touch the parent field (description edits, tag adds, etc.). 2. Cycle detection — second-line ID-based ancestor-chain walk added in ContainerUpdater.validateAncestorChainCycle. Uses relationshipDAO.findFrom (direct DB) so a descendant with a briefly stale FQN can't bypass the FQN-prefix check. Visited-set bounded. 3. IT race condition — drop System.setProperty in patch_containerParent_rejectsOversizedSubtree_400 and patch_containerParent_allowsMoveAtConfiguredLimit_200. Add a package-accessible test-override field (set/clear via public static methods) plus @ResourceLock(MAX_REPARENT_DESCENDANTS_TEST_LOCK) to serialize any test that mutates the override, even though the class runs methods concurrently. 4. SQL build comment — document why ContainerDAO.updateFqn interpolates values via String.format (mirrors EntityDAO pattern, FQN values are server-computed, escapeApostrophe handles the only SQL metacharacter that can appear in a validated entity name). Tests: ContainerRepositoryParentValidationTest extended to 12 cases (adds parent-unchanged short-circuit assertion + override-priority coverage). Full ContainerResourceIT still 255/255. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
| // Always resolve parent — re-parenting via PATCH (#24294) requires the loaded entity to | ||
| // carry the current parent so the JSON Patch document can target an existing | ||
| // `/parent` member. Mirrors `GlossaryTermRepository.setFields`. `clearFields` below still | ||
| // strips parent from the response payload when the caller did not request it. | ||
| container.setParent(getContainerParent(container)); |
| /** | ||
| * Rewrite feed entity-links and field-relationships when a container's FQN changes (parent | ||
| * move). Mirrors {@code GlossaryTermRepository.updateEntityLinks}. Descendant container | ||
| * entity-links are also updated by walking children via {@link Relationship#CONTAINS}. | ||
| */ | ||
| private void updateEntityLinks(String oldFqn, String newFqn, Container updated) { | ||
| daoCollection.fieldRelationshipDAO().renameByToFQN(oldFqn, newFqn); | ||
|
|
||
| EntityLink newAbout = new EntityLink(CONTAINER, newFqn); | ||
| feedRepository.updateLegacyThreadsAbout(newAbout.getLinkString(), updated.getId().toString()); | ||
|
|
||
| List<EntityReference> children = | ||
| findTo(updated.getId(), CONTAINER, Relationship.CONTAINS, CONTAINER); | ||
| for (EntityReference child : children) { | ||
| EntityLink childAbout = new EntityLink(CONTAINER, child.getFullyQualifiedName()); | ||
| feedRepository.updateLegacyThreadsAbout(childAbout.getLinkString(), child.getId().toString()); | ||
| } | ||
| } |
Two findings from copilot-pull-request-reviewer: 1. setFields perf regression — restored the conditional fields.contains(FIELD_PARENT) ? getContainerParent(c) : c.getParent() guard. The unconditional load forced an extra entity_relationship lookup on every container GET, which is a measurable regression on the hot path. The PATCH flow still loads parent because CONTAINER_PATCH_FIELDS includes parent (so fields.contains is true there). Full ContainerResourceIT still 255/255. 2. updateEntityLinks shallow walk — previously only iterated direct children, leaving deep descendants' (grandchildren+) legacy feed thread entityLinks pointing at the old FQN and breaking activity- feed navigation after a multi-level move. Now takes the renamedContainers snapshot captured by invalidateCacheForRenameCascade and rewrites each descendant's entityLink by swapping the FQN prefix (oldFqn → newFqn) — consistent with the same prefix-substitution ContainerDAO.updateFqn applies to the JSON/fqnHash. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
Code Review ✅ Approved 4 resolved / 4 findingsEnables Container re-parenting via PATCH by cascading FQN updates, metadata migration, and relationship rewrites. Addresses race conditions, optimizes performance, and resolves cycle detection issues. ✅ 4 resolved✅ Performance: validateParent fires DB query on every update even when parent unchanged
✅ Bug: Cycle detection uses FQN prefix check, misses non-FQN-based cycles
✅ Bug: System.setProperty in concurrent tests causes race condition
✅ Security: updateFqn builds SQL via String.format with user-derived FQN values
OptionsDisplay: compact → Showing less information. Comment with these commands to change:
Was this helpful? React with 👍 / 👎 | Gitar |
|
Describe your changes:
Fixes #24294
Allow the PATCH API to update a Container's
parentso users can re-parent containers (and their entire subtree) without delete+recreate, preserving followers, likes, ownership, tags, and other metadata.Type of change:
High-level design:
ContainerRepository.restorePatchAttributesno longer wipesparent,CONTAINER_PATCH_FIELDSnow includes it, and a newContainerUpdater.updateParentruns the same FQN-cascade flowGlossaryTermRepository.updateNameAndParentuses — invalidate descendant caches, bulk-rewrite descendant FQNs, re-apply tags, renametag_usagetargets for classification + glossary sources, rewrite entity-links, rewrite policy conditions, swap theCONTAINSrelationship edge, and reindex the search documents viaupdateByFqnPrefix.ContainerDAO.updateFqnis overridden because the generic MySQLJSON_REPLACEonly rewrites$.fullyQualifiedNameand silently leavesdataModel.columns[*].fullyQualifiedNamepointing at the old parent.setFullyQualifiedNameno longer falls back to the relationship table whenparentis null in-memory — that fallback was silently restoring the cleared parent on every "promote to top-level" PATCH. Validation is extracted to a package-private static helpervalidateContainerParentand rejects cross-StorageService parents (HTTP 400), self-parent, and descendant-as-parent cycles. Java fluent SDK gainsContainers.create().under(...)(3 overloads) plusFluentContainer.withParent(...)/withParentFqn/withoutParent; Python fluent SDK gainsContainers.set_parentandContainers.clear_parentfollowing the existingTables.add_tagPATCH idiom —parentwas already inALLOWED_COMMON_PATCH_FIELDS. Scoped to same-StorageService moves; cross-service is explicitly rejected.Tests:
Use cases covered
dataModel.columns[*].fullyQualifiedNamenullto promote a container to top-level under its service; set a parent on a previously top-level containerchangeDescriptionwithparentrecorded and bumps the entity versionUnit tests
openmetadata-service/src/test/java/org/openmetadata/service/jdbi3/ContainerRepositoryParentValidationTest.java— 6 tests, all passBackend integration tests
openmetadata-integration-tests/src/test/java/org/openmetadata/it/tests/ContainerResourceIT.java— 11 new@Testmethods; full class is 253/253 passing (17 pre-existing skipped)Ingestion integration tests
ingestion/tests/unit/sdk/test_container_entity.pyfor the fluent SDK (14/14 pass)Playwright (UI) tests
Manual testing performed
mvn -DskipTests clean install(full build)mvn test -pl openmetadata-integration-tests -Dtest=ContainerResourceITagainst the spawned MySQL/OpenSearch test harness — 253/253 passingmvn spotless:checkandmake py_format_check— both cleanUI screen recording / screenshots:
Not applicable.
Checklist:
Fixes <issue-number>: <short explanation>Fixes #<issue-number>above.🤖 Generated with Claude Code
Summary by Gitar
setFieldsto resolve parent only when requested, reducing unnecessary database round-trips for standard GET requests.updateEntityLinksto process all descendants fromrenamedContainersinstead of just direct children, ensuring consistent FQN updates for deeply nested feed threads.This will update automatically on new commits.