Fix ODPS import + data-product metadata bugs (#27600)

[harshach]

The ODPS / Data Products feature (#27600) shipped with several bugs in its import, metadata-edit, and merge paths. They were surfaced by a new Playwright suite for the ODPS flow (which had no E2E coverage). All fixes were verified end-to-end on a live stack (via the 1.13 backport, companion PR #29154); this lands the same fixes on main.

Bugs fixed

1. DataProduct PATCH never persisted dataProductType/visibility/portfolioPriority. DataProductUpdater.entitySpecificUpdate() recorded changes only for name and domains, so change-consolidation reverted these ODPS-aligned scalar fields on store — the PATCH response showed the value but a subsequent GET returned null, and the metadata-edit modal silently no-opped. Added recordChange for the three fields.

2. ODPS import always returned 400 "Unknown custom field odpsMetadata". ODPSConverter.fromODPS wrote the raw document into extension.odpsMetadata, which is not a registered custom property, so validateExtension rejected every import. toODPS never reads it back, so the write was dead weight — removed it; merge/replace now preserve the existing product's real custom properties.

3. ODPS import returned 409 because the domain reference built from ?domain= had no id. Resolved via Entity.getEntityReferenceByName.

4. ODPS create-from-import returned 409 (null primary key) because the converted entity had no id. Now assigns a fresh UUID like the normal create path.

5. ODPS merge/replace wiped owners/domains/experts/reviewers/certification on an existing product. findExistingByName loaded it with only id,name,version, so the lazy relationship fields came back null and smartMerge/fullReplace copied those nulls — and since domains is required, a PUT against an existing product either 400'd or stripped governance fields. Now loads with EXPORT_FIELDS so those fields are hydrated before merging.

6. ODPSImportModal name guard matched the first name: key via regex, which can hit an SLA dimension or tag rather than the product name (product.details.<lang>.name). Now parses the YAML with js-yaml and reads that path.

Tests

New DataProductODPS.spec.ts (8 tests): export, validate (valid + invalid), export→rename→import round-trip, merge preserves domain + owners (regression guard for #5), and the UI export-menu / import-modal / metadata-edit-modal flows.

Verified on a fresh local stack: ODPS 8/8 green. (Excludes the 1.13-only legacy-form changes from #29154, since main uses the HookForm AddDomainForm.)

🤖 Generated with Claude Code

---

Summary by Gitar

• DataProduct fixes:
  • Fixed PATCH persistence for dataProductType, visibility, and portfolioPriority in DataProductUpdater.
  • Removed extension.odpsMetadata to resolve 400 errors and ensure compatibility with custom field registration.
  • Added Entity.getEntityReferenceByName to fix 409 domain conflicts and UUID assignment for import-created entities.
  • Hydrated relationships using EXPORT_FIELDS during smartMerge/fullReplace to prevent data loss of governance fields.
  • Replaced regex with js-yaml in ODPSImportModal for accurate product name extraction.
• Core entity/container improvements:
  • Optimized container cache invalidation and batch-fetched derived tags for bulk indexing operations.
  • Refactored EntityRepository caching to use weighted memory-based limits with CacheConfiguration.
  • Added regression testing for ODPS import name guards in DataProductODPS.spec.ts.

_{This will update automatically on new commits.}

[gitar-bot]

⚠️ Edge Case: ODPS tag import fails for tags not already in OpenMetadata

ODPSConverter.fromODPS maps every free-form ODPS tag string into a TagLabel with tagFQN=, source=CLASSIFICATION (wrapTagsAsLabels). These labels flow into create()/createOrUpdate(). During entity preparation, TagLabelUtil.applyTagCommonFields calls getTag(tagFQN) → Entity.getEntityByName(Entity.TAG, ...), which throws EntityNotFoundException when the tagFQN does not correspond to an existing classification tag. ODPS documents authored on a foreign system will routinely carry tags that don't exist in this OpenMetadata instance, so any such import will fail (404/400) rather than importing the product without the unknown tags. Consider resolving tags leniently (e.g. drop unknown tags, or only attach those that resolve via applyTagCommonFieldsGracefully) so a stray tag string doesn't abort the whole import.

Only import tags that resolve to existing OM tags, dropping unknown ones.:

// in fromODPS, replace dp.setTags(wrapTagsAsLabels(...)) with a filtered list
List<TagLabel> labels = wrapTagsAsLabels(details.getTags());
labels.removeIf(l -> {
  try { TagLabelUtil.applyTagCommonFields(l); return false; }
  catch (Exception e) { LOG.warn("Skipping unknown ODPS tag {}", l.getTagFQN()); return true; }
});
if (!labels.isEmpty()) dp.setTags(labels);

• Apply fix

_{Check the box to apply the fix or reply for a change | Was this helpful? React with 👍 / 👎}

[gitar-bot]

💡 Bug: ODPS merge/replace silently ignores ?domain= parameter

In mergeOrCreateDataProductFromODPS the imported product is built with the domain resolved from ?domain= (buildDataProductFromODPS), but both smartMerge and fullReplace copy domains from the existing product (merged.setDomains(existing.getDomains())). As a result, when a caller does PUT /odps?domain=&strategy=replace against an existing product to move it to a different domain, the supplied domain is silently discarded. If preserving the existing domain is intentional (governance), that is fine, but the endpoint accepts and documents the domain query param, so the no-op may surprise callers. Consider documenting that domain changes are not applied on merge/replace, or honoring an explicit domain override.

_{Was this helpful? React with 👍 / 👎}

[github-actions]

The Python checkstyle failed.

Please run make py_format and py_format_check in the root of your repository and commit the changes to this PR.
You can also use pre-commit to automate the Python code formatting.

You can install the pre-commit hooks with make install_test precommit_install.

[greptile-apps]

Too many files changed for review. (2462 files found, 100 file limit)

[gitar-bot]

⚠️ Bug: Name-guard test passes immediately, never verifies PUT is blocked

The regression test intends to prove the hardened name guard short-circuits before any API call. It does this with:

let putFired = false;
...
await page.getByTestId('odps-import-submit').click();
await expect(async () => {
  expect(putFired).toBe(false);
}).toPass({ timeout: 3000, intervals: [300] });

expect(...).toPass() resolves as soon as the inner assertion succeeds. Since putFired is false at the moment of the first poll (immediately after the click), the assertion passes instantly on the very first iteration — before the browser has any chance to issue the PUT. The timeout: 3000 / intervals: [300] therefore do nothing: the test would also pass even if the guard were broken and a PUT fired ~100ms later. The test gives false confidence and is not a real regression guard for bug #6.

To actually verify no PUT occurs, wait a fixed settle period (or race a waitForResponse against a timeout) and only then assert. For example:

await page.getByTestId('odps-import-submit').click();
// give the app time to (incorrectly) fire a PUT
await page.waitForTimeout(2000);
expect(putFired).toBe(false);
await expect(page.getByTestId('odps-import-modal')).toBeVisible();

Alternatively assert positively that the guard surfaced a validation error in the modal, which is a stronger signal than the absence of a network call.

Wait a fixed settle period before asserting so a wrongly-fired PUT is actually observed.:

page.on('response', listener);
await page.getByTestId('odps-import-submit').click();

// Give the app time to (incorrectly) fire a PUT, then assert none did.
await page.waitForTimeout(2000);
expect(putFired).toBe(false);
page.off('response', listener);
await expect(page.getByTestId('odps-import-modal')).toBeVisible();

• Apply fix

_{Check the box to apply the fix or reply for a change | Was this helpful? React with 👍 / 👎}

[gitar-bot]

Code Review ⚠️ Changes requested 0 resolved / 3 findings

Addresses several ODPS import and data-product metadata bugs, but changes are requested due to persistent issues with tag imports, domain parameter handling during merge, and incomplete regression test validation.

⚠️ Edge Case: ODPS tag import fails for tags not already in OpenMetadata

📄 openmetadata-service/src/main/java/org/openmetadata/service/util/ODPSConverter.java:168-169 📄 openmetadata-service/src/main/java/org/openmetadata/service/util/ODPSConverter.java:210-221 📄 openmetadata-service/src/main/java/org/openmetadata/service/resources/domains/DataProductResource.java:1650-1658 📄 openmetadata-service/src/main/java/org/openmetadata/service/resources/domains/DataProductResource.java:1845-1859

ODPSConverter.fromODPS maps every free-form ODPS tag string into a TagLabel with tagFQN=, source=CLASSIFICATION (wrapTagsAsLabels). These labels flow into create()/createOrUpdate(). During entity preparation, TagLabelUtil.applyTagCommonFields calls getTag(tagFQN) → Entity.getEntityByName(Entity.TAG, ...), which throws EntityNotFoundException when the tagFQN does not correspond to an existing classification tag. ODPS documents authored on a foreign system will routinely carry tags that don't exist in this OpenMetadata instance, so any such import will fail (404/400) rather than importing the product without the unknown tags. Consider resolving tags leniently (e.g. drop unknown tags, or only attach those that resolve via applyTagCommonFieldsGracefully) so a stray tag string doesn't abort the whole import.

Only import tags that resolve to existing OM tags, dropping unknown ones.

// in fromODPS, replace dp.setTags(wrapTagsAsLabels(...)) with a filtered list
List<TagLabel> labels = wrapTagsAsLabels(details.getTags());
labels.removeIf(l -> {
  try { TagLabelUtil.applyTagCommonFields(l); return false; }
  catch (Exception e) { LOG.warn("Skipping unknown ODPS tag {}", l.getTagFQN()); return true; }
});
if (!labels.isEmpty()) dp.setTags(labels);

⚠️ Bug: Name-guard test passes immediately, never verifies PUT is blocked

📄 openmetadata-ui/src/main/resources/ui/playwright/e2e/Pages/DataProductODPS.spec.ts:375-389

The regression test intends to prove the hardened name guard short-circuits before any API call. It does this with:

let putFired = false;
...
await page.getByTestId('odps-import-submit').click();
await expect(async () => {
  expect(putFired).toBe(false);
}).toPass({ timeout: 3000, intervals: [300] });

expect(...).toPass() resolves as soon as the inner assertion succeeds. Since putFired is false at the moment of the first poll (immediately after the click), the assertion passes instantly on the very first iteration — before the browser has any chance to issue the PUT. The timeout: 3000 / intervals: [300] therefore do nothing: the test would also pass even if the guard were broken and a PUT fired ~100ms later. The test gives false confidence and is not a real regression guard for bug #6.

To actually verify no PUT occurs, wait a fixed settle period (or race a waitForResponse against a timeout) and only then assert. For example:

await page.getByTestId('odps-import-submit').click();
// give the app time to (incorrectly) fire a PUT
await page.waitForTimeout(2000);
expect(putFired).toBe(false);
await expect(page.getByTestId('odps-import-modal')).toBeVisible();

Alternatively assert positively that the guard surfaced a validation error in the modal, which is a stronger signal than the absence of a network call.

Wait a fixed settle period before asserting so a wrongly-fired PUT is actually observed.

page.on('response', listener);
await page.getByTestId('odps-import-submit').click();

// Give the app time to (incorrectly) fire a PUT, then assert none did.
await page.waitForTimeout(2000);
expect(putFired).toBe(false);
page.off('response', listener);
await expect(page.getByTestId('odps-import-modal')).toBeVisible();

💡 Bug: ODPS merge/replace silently ignores ?domain= parameter

📄 openmetadata-service/src/main/java/org/openmetadata/service/resources/domains/DataProductResource.java:1845-1859 📄 openmetadata-service/src/main/java/org/openmetadata/service/util/ODPSConverter.java:297 📄 openmetadata-service/src/main/java/org/openmetadata/service/util/ODPSConverter.java:330

In mergeOrCreateDataProductFromODPS the imported product is built with the domain resolved from ?domain= (buildDataProductFromODPS), but both smartMerge and fullReplace copy domains from the existing product (merged.setDomains(existing.getDomains())). As a result, when a caller does PUT /odps?domain=&strategy=replace against an existing product to move it to a different domain, the supplied domain is silently discarded. If preserving the existing domain is intentional (governance), that is fine, but the endpoint accepts and documents the domain query param, so the no-op may surprise callers. Consider documenting that domain changes are not applied on merge/replace, or honoring an explicit domain override.

🤖 Prompt for agents

Code Review: Addresses several ODPS import and data-product metadata bugs, but changes are requested due to persistent issues with tag imports, domain parameter handling during merge, and incomplete regression test validation.

1. ⚠️ Edge Case: ODPS tag import fails for tags not already in OpenMetadata
   Files: openmetadata-service/src/main/java/org/openmetadata/service/util/ODPSConverter.java:168-169, openmetadata-service/src/main/java/org/openmetadata/service/util/ODPSConverter.java:210-221, openmetadata-service/src/main/java/org/openmetadata/service/resources/domains/DataProductResource.java:1650-1658, openmetadata-service/src/main/java/org/openmetadata/service/resources/domains/DataProductResource.java:1845-1859

   ODPSConverter.fromODPS maps every free-form ODPS tag string into a TagLabel with tagFQN=<raw string>, source=CLASSIFICATION (wrapTagsAsLabels). These labels flow into create()/createOrUpdate(). During entity preparation, TagLabelUtil.applyTagCommonFields calls getTag(tagFQN) → Entity.getEntityByName(Entity.TAG, ...), which throws EntityNotFoundException when the tagFQN does not correspond to an existing classification tag. ODPS documents authored on a foreign system will routinely carry tags that don't exist in this OpenMetadata instance, so any such import will fail (404/400) rather than importing the product without the unknown tags. Consider resolving tags leniently (e.g. drop unknown tags, or only attach those that resolve via applyTagCommonFieldsGracefully) so a stray tag string doesn't abort the whole import.

   Fix (Only import tags that resolve to existing OM tags, dropping unknown ones.):
   // in fromODPS, replace dp.setTags(wrapTagsAsLabels(...)) with a filtered list
   List<TagLabel> labels = wrapTagsAsLabels(details.getTags());
   labels.removeIf(l -> {
     try { TagLabelUtil.applyTagCommonFields(l); return false; }
     catch (Exception e) { LOG.warn("Skipping unknown ODPS tag {}", l.getTagFQN()); return true; }
   });
   if (!labels.isEmpty()) dp.setTags(labels);

2. 💡 Bug: ODPS merge/replace silently ignores ?domain= parameter
   Files: openmetadata-service/src/main/java/org/openmetadata/service/resources/domains/DataProductResource.java:1845-1859, openmetadata-service/src/main/java/org/openmetadata/service/util/ODPSConverter.java:297, openmetadata-service/src/main/java/org/openmetadata/service/util/ODPSConverter.java:330

   In mergeOrCreateDataProductFromODPS the imported product is built with the domain resolved from ?domain= (buildDataProductFromODPS), but both smartMerge and fullReplace copy domains from the existing product (merged.setDomains(existing.getDomains())). As a result, when a caller does PUT /odps?domain=<new>&strategy=replace against an existing product to move it to a different domain, the supplied domain is silently discarded. If preserving the existing domain is intentional (governance), that is fine, but the endpoint accepts and documents the domain query param, so the no-op may surprise callers. Consider documenting that domain changes are not applied on merge/replace, or honoring an explicit domain override.

3. ⚠️ Bug: Name-guard test passes immediately, never verifies PUT is blocked
   Files: openmetadata-ui/src/main/resources/ui/playwright/e2e/Pages/DataProductODPS.spec.ts:375-389

   The regression test intends to prove the hardened name guard short-circuits before any API call. It does this with:
   
   ```
   let putFired = false;
   ...
   await page.getByTestId('odps-import-submit').click();
   await expect(async () => {
     expect(putFired).toBe(false);
   }).toPass({ timeout: 3000, intervals: [300] });
   ```
   
   `expect(...).toPass()` resolves as soon as the inner assertion succeeds. Since `putFired` is `false` at the moment of the first poll (immediately after the click), the assertion passes instantly on the very first iteration — before the browser has any chance to issue the PUT. The `timeout: 3000` / `intervals: [300]` therefore do nothing: the test would also pass even if the guard were broken and a PUT fired ~100ms later. The test gives false confidence and is not a real regression guard for bug #6.
   
   To actually verify no PUT occurs, wait a fixed settle period (or race a `waitForResponse` against a timeout) and only then assert. For example:
   
   ```
   await page.getByTestId('odps-import-submit').click();
   // give the app time to (incorrectly) fire a PUT
   await page.waitForTimeout(2000);
   expect(putFired).toBe(false);
   await expect(page.getByTestId('odps-import-modal')).toBeVisible();
   ```
   
   Alternatively assert positively that the guard surfaced a validation error in the modal, which is a stronger signal than the absence of a network call.

   Fix (Wait a fixed settle period before asserting so a wrongly-fired PUT is actually observed.):
   page.on('response', listener);
   await page.getByTestId('odps-import-submit').click();
   
   // Give the app time to (incorrectly) fire a PUT, then assert none did.
   await page.waitForTimeout(2000);
   expect(putFired).toBe(false);
   page.off('response', listener);
   await expect(page.getByTestId('odps-import-modal')).toBeVisible();

Options

Display: compact → Showing less information.

Comment with these commands to change:

Compact

gitar display:verbose

_{Was this helpful? React with 👍 / 👎 | Gitar}

[github-actions]

The Python checkstyle failed.

Please run make py_format and py_format_check in the root of your repository and commit the changes to this PR.
You can also use pre-commit to automate the Python code formatting.

You can install the pre-commit hooks with make install_test precommit_install.