Add ODPS Support for Data Products + Custom Intake Forms (1.13)#29154
Conversation
Cherry-picked from ea438e0 (main). The backend (IntakeForm entity/table/repository/resource, ODPSConverter, DataProduct ODPS fields), generated schemas/types, IntakeForms designer page, ODPSImportModal, DataProductMetadataModal, and the ODPS data-product API applied as authored. 1.13 adaptation — the original commit was built on three main-only features absent from 1.13; those entanglements were resolved by keeping 1.13's architecture and layering only the ODPS/intake-form additions: - AddDomainForm: 1.13 still uses the legacy Ant Design form (main migrated it to react-hook-form in #26951). The data-product fields (dataProductType, visibility, portfolioPriority, reviewers), intake-form required-field rules, and dynamic required custom-property (extension) fields were re-implemented on the antd FieldProp/getField pattern instead of HookForm. - DataProductsDetailsPage: kept 1.13's version and added only the ODPS import and metadata-edit modals; dropped the main-only Request-Data-Access drawer and activity/task-count utilities it referenced. - Settings: added only the Intake Forms menu entry/route; the Workflow Definitions and Task Forms entries/routes (and their components) do not exist in 1.13 and were excluded. - CollectionDAO/DomainRepository: dropped main-only Announcement/TaskFormSchema imports and the dry-run-impact helpers (BulkResponse.withHasSideEffects / filterDataProductsByDomain) that bled into the conflict; only IntakeFormDAO was added. Verified: openmetadata-service compiles and test-compiles; AddDomainForm unit tests pass (13); changed UI files pass tsc/eslint/prettier; i18n locales synced. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
…llow-up) Adds a Playwright suite for the ODPS + data-product-metadata flow (which had zero E2E coverage) and fixes four backend bugs it surfaced. All bugs are present on main too — the feature code (ODPSConverter, DataProductRepository, DataProductResource import handlers) is unchanged since #27600 and none were fixed by a later commit, so these fixes should be forward-ported. Bugs fixed: 1. DataProduct PATCH never persisted dataProductType/visibility/portfolioPriority. DataProductUpdater.entitySpecificUpdate() recorded changes only for name and domains, so change-consolidation reverted the ODPS-aligned scalar fields on store: the PATCH response showed the value but a subsequent GET returned null. The metadata-edit modal silently no-opped. Added recordChange() for the three fields (reviewers were already handled by updateReviewers()). 2. ODPS import always returned 400 "Unknown custom field odpsMetadata". ODPSConverter.fromODPS wrote the raw document into extension.odpsMetadata, but that key is not a registered custom property, so validateExtension rejected every import. toODPS never reads it back (export is driven by native fields), 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 the ?domain= FQN had no id (the create authorization context and relationship storage need it). buildDataProductFromODPS now resolves the domain via Entity.getEntityReferenceByName. 4. ODPS create-from-import returned 409 (duplicate key) because the converter built a bare entity with no id, so the generated primary-key column was null. The normal create path sets the id via EntityMapper.copy; buildDataProductFromODPS now assigns a fresh UUID (merge/replace overwrite it with the existing id). UI: completed the AddDomainForm adaptation by rendering required entity-reference custom properties as a user/team picker (was a text fallback); single entityReference values are unwrapped from the picker's array shape so the API receives a bare object. Tests: - New playwright/e2e/Pages/DataProductODPS.spec.ts (8 tests): ODPS export, validate (valid + invalid), export→rename→import round-trip, merge strategy, and the UI export-menu / import-modal / metadata-edit-modal flows. - IntakeForm.spec.ts: realigned AddDomainForm assertions to 1.13's legacy antd form (add-domain testid, MUI required-asterisk label) so the cherry-picked intake-form enforcement tests pass against the adapted form. Verified on a fresh local stack: ODPS 8/8, IntakeForm 10/10, DataProducts CRUD spot-check green; openmetadata-service compiles + ODPSConverterTest passes. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
Intake forms support Domain, DataProduct and GlossaryTerm (all enforced by IntakeFormValidator in their repositories), but E2E only covered DataProduct. Mirror that coverage for Domain (AddDomainForm rendered with type=DOMAIN): - Designer creates a Domain intake form via the UI (entityType=domain). - "Domain" option disabled in the add menu once a form exists. - A required native field (displayName) blocks Domain create client-side and the backend rejects the direct API create with 400. - A required custom property renders on the Domain create form and blocks submit when left empty. - A required entity-reference custom property renders as a user picker and the Domain is created with the bare-object extension payload (isolated in its own describe for a fresh page, same MUIUserTeamSelect reason as the DataProduct entity-ref test). 15/15 IntakeForm tests pass on the local stack. 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
❌ UI Checkstyle Failed❌ ESLint + Prettier + Organise Imports (src)One or more source files have linting or formatting issues. Affected files
❌ I18n SyncTranslation locale files are out of sync with Affected files
Fix locally (fast — only checks files changed in this branch): make ui-checkstyle-changed |
|
The Python checkstyle failed. Please run You can install the pre-commit hooks with |
![greptile-apps[bot]](https://avatars.githubusercontent.com/in/867647?s=60&v=4){kind=small}
| public Response validateODPSYaml(String yamlContent) { | ||
| try { | ||
| ODPSDataProduct odps = YAML_MAPPER.readValue(yamlContent, ODPSDataProduct.class); | ||
| ODPSConverter.validateRequiredODPSFields(odps); | ||
| JsonNode summary = summarizeODPS(odps); | ||
| return Response.ok(summary).build(); | ||
| } catch (JsonProcessingException e) { | ||
| throw new IllegalArgumentException("Invalid ODPS YAML content: " + e.getMessage(), e); | ||
| } | ||
| } |
There was a problem hiding this comment.
validateODPSYaml is the only non-GET endpoint in this resource that omits @Context SecurityContext securityContext. All other mutating endpoints — including the adjacent import and create-or-update YAML endpoints — receive a SecurityContext so the caller identity can be audited. Without it, this endpoint relies entirely on upstream filter-level auth with no ability to enforce role-based checks or log the caller. If framework auth is bypassed (e.g. dev/test deployment without filter), the endpoint accepts and parses arbitrary YAML from any caller with no trace.
1. ODPS merge/replace wiped owners/domains/experts/reviewers/certification on an existing product. findExistingByName loaded the product with only "id,name,version", so the lazy relationship fields came back null; smartMerge/fullReplace then copied those nulls and the updater recorded their removal — and since domains is required, a PUT against an existing product either 400'd (missing domain) or stripped governance fields. Load the existing product with EXPORT_FIELDS (the same set used for export) so those fields are hydrated before merging. 2. ODPSImportModal's name guard matched the first `name:` key via regex, which in an ODPS document can hit an SLA dimension or tag rather than the product name (product.details.<lang>.name). Parse the YAML with js-yaml and read that path (en, else the first declared language) — robust to key order and nesting. Extends the merge E2E test to assert domains + owners survive the merge. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com> (cherry picked from commit ac251943e3b79692c41d3e9b3daad67f90854de9)
Jest test CoverageUI tests summary
|
|
The Python checkstyle failed. Please run You can install the pre-commit hooks with |
The previous import-modal test only covered a round-trip YAML whose name matches the product, so it passed with either the old first-`name:` regex or the new js-yaml parser. Add a test that pastes a YAML with a decoy top-level `name:` matching the current product but a different real product name at product.details.en.name — the hardened guard must read that path and block the import (no PUT fires, modal stays open). Fails with the old regex, passes now. 9/9 ODPS tests green. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
![gitar-bot[bot]](https://avatars.githubusercontent.com/in/827041?s=60&v=4){kind=small}
| await page.getByTestId('odps-import-submit').click(); | ||
|
|
||
| // The guard short-circuits before any API call: no PUT, modal stays open. | ||
| await expect(async () => { | ||
| expect(putFired).toBe(false); | ||
| }).toPass({ timeout: 3000, intervals: [300] }); | ||
| page.off('response', listener); | ||
| await expect(page.getByTestId('odps-import-modal')).toBeVisible(); |
There was a problem hiding this comment.
💡 Quality: Name-guard test assertion passes instantly, weakening regression value
The toPass block at lines 388-390 asserts expect(putFired).toBe(false). Because putFired starts as false, this assertion succeeds on the very first poll (essentially immediately after submit is clicked), so the timeout: 3000 / intervals: [300] settings never come into play. toPass retries until the assertion passes — it does not keep re-checking to confirm the condition stays true for the whole window. As a result, if the name guard regressed and a PUT to /dataProducts/odps/yaml did fire (a network round-trip takes some milliseconds), the test would likely have already passed and the regression would go undetected. The test therefore provides much weaker protection than the comment implies.
To actually verify no PUT fires, wait a fixed interval before asserting (so the network call would have had time to happen), e.g. await page.waitForTimeout(2000) then expect(putFired).toBe(false). Pairing it with the existing modal-still-visible / error-message assertion makes the regression signal robust.
Wait a fixed window so a regressed PUT would have time to fire before asserting.:
page.on('response', listener);
await page.getByTestId('odps-import-submit').click();
// The guard short-circuits before any API call: give a real PUT time to
// fire, then confirm none did and the modal stayed open.
await page.waitForTimeout(2000);
expect(putFired).toBe(false);
page.off('response', listener);
await expect(page.getByTestId('odps-import-modal')).toBeVisible();
Was this helpful? React with 👍 / 👎
|
The Python checkstyle failed. Please run You can install the pre-commit hooks with |
|
Code Review 👍 Approved with suggestions 2 resolved / 3 findingsImplements ODPS support and Custom Intake Forms by adapting main-branch features to the 1.13 architecture, including critical bug fixes for data-product persistence. Address the validation logic in 💡 Quality: Name-guard test assertion passes instantly, weakening regression value📄 openmetadata-ui/src/main/resources/ui/playwright/e2e/Pages/DataProductODPS.spec.ts:385-392 The To actually verify no PUT fires, wait a fixed interval before asserting (so the network call would have had time to happen), e.g. Wait a fixed window so a regressed PUT would have time to fire before asserting.✅ 2 resolved✅ Bug: ODPS merge/replace wipes domains/owners on existing products
✅ Edge Case: ODPS YAML name guard relies on fragile regex / line order
🤖 Prompt for agentsOptionsDisplay: compact → Showing less information. Comment with these commands to change:
Was this helpful? React with 👍 / 👎 | Gitar |
1 participant
Cherry-picks the ODPS / Data Products + Custom Intake Forms feature (#27600) from
mainonto1.13, plus fixes for several bugs the feature shipped with and a large E2E coverage expansion.What's included
Feature port (cherry-pick of
ea438e03b6) —IntakeFormentity/table/repository/resource,ODPSConverter, DataProduct ODPS fields, generated schemas/types, IntakeForms designer page,ODPSImportModal,DataProductMetadataModal, ODPS data-product API.1.13 adaptation — the original commit was built on three
main-only features absent from 1.13 (the HookFormAddDomainFormmigration #26951, the Request-Data-Access drawer, and task-redesign activity counts). Resolved by keeping 1.13's architecture and layering only the ODPS/intake additions:AddDomainFormstays on 1.13's legacy Ant Design form; the data-product fields, intake-form required rules, and dynamic required custom-property (incl. entity-reference) fields were re-implemented on the antdFieldProp/getFieldpattern.DataProductsDetailsPagekeeps 1.13's version + the ODPS import/metadata modals; dropped the main-only DAR drawer and task-count utilities.Bug fixes (also present on
main— see companion PR)dataProductType/visibility/portfolioPriority(consolidation reverted unrecorded fields) — addedrecordChangein the updater.Unknown custom field odpsMetadata— the converter wrote a non-registered extension key; removed it (export never reads it back).?domain=had no id; now resolved.Test coverage
DataProductODPS.spec.ts(8 tests): export, validate (valid + invalid), export→rename→import round-trip, merge strategy, and the UI export-menu / import-modal / metadata-edit-modal flows.IntakeForm.spec.ts: realigned to 1.13's legacy form, plus new Domain enforcement coverage (designer create, option-disabled, required-native-field block, required custom property render+block, entity-reference picker) mirroring the existing DataProduct tests.Verified on a fresh local stack: ODPS 8/8, IntakeForm 15/15, DataProducts CRUD spot-check green;
openmetadata-servicecompiles +ODPSConverterTestpasses.🤖 Generated with Claude Code
Greptile Summary
This PR cherry-picks the ODPS / Data Products + Custom Intake Forms feature from
mainto the1.13branch, adapting it to 1.13's Ant Design form architecture, and includes four targeted bug fixes (DataProduct PATCH not persisting ODPS scalar fields, 400 on ODPS import from unregistered extension key, 409 from domain reference without ID, and 409 from missing entity ID on create-from-import).IntakeFormRepository/Resource/Mapper/Validator,ODPSConverter(ODPS v4.1 ↔ OM DataProduct), 6 ODPS import/export/validate endpoints onDataProductResource, custom entity-reference validation inEntityUtil, and DB migrations forintake_form_entity.IntakeFormsPagedesigner,ODPSImportModal(with proper YAML-parsed name extraction replacing the prior regex),DataProductMetadataModal, and intake-form required-field enforcement wired intoAddDomainForm.DataProductUpdater.entitySpecificUpdatenow callsrecordChangefordataProductType/visibility/portfolioPriority;EXPORT_FIELDSincludesextensionso merge/replace preserve custom properties; domain reference is resolved to a full entity reference; imported entities receive a UUID before creation.Confidence Score: 5/5
Safe to merge; the four stated bug fixes are correct and the new ODPS/IntakeForm code paths are well-structured with appropriate fail-closed validation.
The extension-null fix (EXPORT_FIELDS now includes extension), the domain-reference resolution, the UUID assignment on import-create, and the recordChange additions in DataProductUpdater all look correct. The IntakeFormValidator's fail-closed design and the js-yaml-based name extraction in ODPSImportModal address prior fragility. Fresh findings are confined to a tag-replacement semantic in smartMerge and a possible rename block in ensureUniquePerEntityType — neither affects correctness of the core create/update/import flows.
ODPSConverter.smartMerge (tag replacement semantics) and IntakeFormRepository.ensureUniquePerEntityType (rename guard logic) are worth a second look before this ships.
Important Files Changed
Sequence Diagram
%%{init: {'theme': 'neutral'}}%% sequenceDiagram participant UI as ODPSImportModal (UI) participant API as DataProductResource participant Conv as ODPSConverter participant Repo as DataProductRepository participant IFV as IntakeFormValidator participant IFRepo as IntakeFormRepository UI->>API: "POST /odps/yaml?domain=... (YAML)" API->>Conv: fromODPS(odps, lang) Conv-->>API: DataProduct (bare, name sanitized) API->>API: setId(UUID.randomUUID()) API->>API: resolveEntityReferenceByName(domain) API->>Repo: create(dataProduct) Repo->>IFV: validate(entity, dataProduct) IFV->>IFRepo: findEnabledForEntityType(dataProduct) IFRepo-->>IFV: IntakeForm or null alt IntakeForm configured and enabled IFV->>IFV: checkRequiredFields(entity, form) alt Missing required fields IFV-->>Repo: IllegalArgumentException 400 end end Repo-->>API: DataProduct created API-->>UI: 200 DataProduct Note over UI,API: PUT /odps/yaml merge or replace UI->>API: "PUT /odps/yaml?strategy=merge (YAML)" API->>Conv: fromODPS to buildDataProductFromODPS API->>Repo: getByName(sanitizedName, EXPORT_FIELDS) alt Existing product found API->>Conv: smartMerge(existing, imported) Conv-->>API: merged id existing extension existing API->>Repo: createOrUpdate(merged) else New product API->>Repo: create(imported) 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 UI as ODPSImportModal (UI) participant API as DataProductResource participant Conv as ODPSConverter participant Repo as DataProductRepository participant IFV as IntakeFormValidator participant IFRepo as IntakeFormRepository UI->>API: "POST /odps/yaml?domain=... (YAML)" API->>Conv: fromODPS(odps, lang) Conv-->>API: DataProduct (bare, name sanitized) API->>API: setId(UUID.randomUUID()) API->>API: resolveEntityReferenceByName(domain) API->>Repo: create(dataProduct) Repo->>IFV: validate(entity, dataProduct) IFV->>IFRepo: findEnabledForEntityType(dataProduct) IFRepo-->>IFV: IntakeForm or null alt IntakeForm configured and enabled IFV->>IFV: checkRequiredFields(entity, form) alt Missing required fields IFV-->>Repo: IllegalArgumentException 400 end end Repo-->>API: DataProduct created API-->>UI: 200 DataProduct Note over UI,API: PUT /odps/yaml merge or replace UI->>API: "PUT /odps/yaml?strategy=merge (YAML)" API->>Conv: fromODPS to buildDataProductFromODPS API->>Repo: getByName(sanitizedName, EXPORT_FIELDS) alt Existing product found API->>Conv: smartMerge(existing, imported) Conv-->>API: merged id existing extension existing API->>Repo: createOrUpdate(merged) else New product API->>Repo: create(imported) endReviews (3): Last reviewed commit: "test(odps): add regression test for the ..." | Re-trigger Greptile