Fixes #29137: Preserve data products across domain deletes#29138
Fixes #29137: Preserve data products across domain deletes#29138harshach wants to merge 6 commits into
Conversation
github-actions
Bot
added
backend
safe to test
✅ PR checks passedThe linked issue has a description and all required Shipping project fields set. Thanks! |
![greptile-apps[bot]](https://avatars.githubusercontent.com/in/867647?s=60&v=4){kind=small}
| } finally { | ||
| await movedDataProduct.delete(apiContext); | ||
| await originalDomain.delete(apiContext); | ||
| await vehicleDomain.delete(apiContext); | ||
| await afterAction(); | ||
| } |
There was a problem hiding this comment.
The established pattern in this spec (see the no-assets test at lines 309–326) wraps each cleanup call in its own try/catch and uses a fresh apiContext. Here, all four calls are sequential bare awaits inside a single finally. If movedDataProduct.delete(apiContext) throws for any reason (network hiccup, FQN lookup failure), neither vehicleDomain.delete nor afterAction() will execute, leaking the vehicle domain and the API context across tests and causing flakiness. Additionally, originalDomain.delete(apiContext) is called on an entity the test already hard-deleted (line ~380), so a 404 there could silently interrupt the remainder of cleanup if Domain.delete is ever tightened to surface the response status.
Jest test CoverageUI tests summary
|
![gitar-bot[bot]](https://avatars.githubusercontent.com/in/827041?s=60&v=4){kind=small}
harshach
changed the title
Code Review ✅ Approved 4 resolved / 4 findingsPrevents data product loss during domain deletion by synchronizing containment relationships and implementing a safe-detach cascade filter. All previously identified integration test and cleanup issues have been addressed. ✅ 4 resolved✅ Bug: Delete-cascade filter has side effects, runs on restore/soft-delete
✅ Edge Case: deletingDomainIds fallback may mis-retain DP when entered at non-root
✅ Quality: IT bypasses SdkClients, mutates relationships via repository directly
✅ Quality: Test toggles global multi-domain rule, risking concurrent-test flakiness
OptionsDisplay: compact → Showing less information. Comment with these commands to change:
Was this helpful? React with 👍 / 👎 | Gitar |
|
|
🟡 Playwright Results — all passed (9 flaky)✅ 4303 passed · ❌ 0 failed · 🟡 9 flaky · ⏭️ 88 skipped
🟡 9 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 |
1 participant
Describe your changes:
Fixes #29137
Fixes #29146
This PR now includes the backend fix plus repro coverage for the data product/domain delete issue.
domain -> dataProductCONTAINSrelationships when a data product domain is changed.Type of change:
Tests:
Backend integration tests
openmetadata-integration-tests/.test_changeDataProductDomain_thenDeleteOriginalDomain_preservesDataProducttest_deleteOneDomainForMultiDomainDataProduct_preservesDataProductUntilLastDomainPlaywright tests
openmetadata-ui/src/main/resources/ui/playwright/e2e/Features/DataProductDomainMigration.spec.ts.Local validation
mvn -pl openmetadata-service,openmetadata-integration-tests spotless:checkmvn -pl openmetadata-integration-tests -am -DskipTests installmvn -pl openmetadata-integration-tests '-Dit.test=DataProductResourceIT#test_changeDataProductDomain_thenDeleteOriginalDomain_preservesDataProduct+test_deleteOneDomainForMultiDomainDataProduct_preservesDataProductUntilLastDomain' -DfailIfNoTests=false failsafe:integration-test failsafe:verify\n\n#\n### Checklist:\n- [x] I have read the CONTRIBUTING document.\n- [x] My PR title isFixes <issue-number>: <short explanation>.\n- [x] My PR is linked to GitHub issues viaFixes #29137andFixes #29146above.\n- [x] I have added tests around the fixed behavior.\nSummary by Gitar
domainDeleteSubtreemechanism inDomainRepositoryto track hierarchy during recursive deletes.filterChildrenForDeleteCascadeto identify and detachDataProductentities from domains being deleted.updateDataProductDomainContainmentinDataProductRepositoryto manage relationship cleanup during domain transitions.EntityRepositoryto allow child relationship management duringhardDeleteoperations.MultiDomainRuletoggling in integration tests to restore state correctly instead of hardcodingtrue.This will update automatically on new commits.
Greptile Summary
This PR fixes a data loss bug where deleting a domain would cascade-delete data products that had already been migrated to another domain, because the CONTAINS relationship between the old domain and the data product was never removed when domains were updated. The fix adds CONTAINS relationship synchronization on domain change and a cascade-filter hook that retains data products with surviving domains during domain hard-deletes.
DataProductRepository: addsupdateDataProductDomainContainmentto sync CONTAINS relationships when a data product's domains change via PATCH/PUT, closing the gap that caused orphaned CONTAINS rows.DomainRepository: overridesprepareChildrenForHardDeleteCascade(both single-entity and bulk paths) to identify data products shared with surviving domains, detach them from only the deleting domains, and update their JSON+search index; uses aThreadLocalsubtree-set to avoid re-initialising context across recursive calls.EntityRepository: addsprepareChildrenForHardDeleteCascade/enterBulkHardDeleteCascadehooks (no-ops by default) called only whenhardDelete=true, keeping the soft-delete path unchanged.Confidence Score: 5/5
Safe to merge. The root cause (stale CONTAINS relationships not cleaned up on domain migration) is correctly fixed in both the update path and the delete-cascade path, with no regressions on soft-delete.
The three-layer fix is logically consistent: CONTAINS rows are now maintained on every domain change, the cascade filter correctly identifies data products with surviving domains and detaches them instead of deleting them, and the ThreadLocal subtree set is properly initialised and cleaned up in all entry points. The hardDelete guard prevents the cascade filter from running during soft-deletes, addressing the concern raised in the previous review thread. Two new integration tests and a Playwright repro test cover the fixed scenarios. The only observations are minor defensive-coding gaps.
openmetadata-service/src/main/java/org/openmetadata/service/jdbi3/DomainRepository.java — the two suggestions around null-safety and the unconditional storeEntity call in detachRetainedDataProductsFromDeletingDomains are worth a quick look before merge.
Important Files Changed
Reviews (5): Last reviewed commit: "Preserve multi-domain rule state in test..." | Re-trigger Greptile