Fixes 32792: let CreateTableRequest carry certification directly - #32793
Fixes 32792: let CreateTableRequest carry certification directly#32793zak-nuccio wants to merge 2 commits into
Conversation
CreateTableRequest had no certification field, so database metadata ingestion had to create/update a table first and follow up with a separate JSON Patch to /certification once the entity existed. That patch is also invisible to sourceHash, so a source system change that is only a certification update produces the same hash as the previous run and the bulk fast-path skips it, silently dropping the update. - createTable.json: add optional certification (AssetCertification). - TableMapper: pass CreateTable.certification through to the entity. - EntityRepository.updateCertification: the bot-overwrite guard used to revert any bot PUT once a certification existed, regardless of what the request contained (a workaround for CreateTableRequest never carrying a value). Narrow it to only preserve certification when the request omits it, matching the overrideMetadata escape hatch already used for description/owners/domains on the same class. An explicit certification value from a bot is now applied instead of reverted. - generate_source_hash needs no change: it hashes the full request via model_dump(), so certification is automatically included once it is a model field. Tests: source-hash stability/change coverage in test_source_hash.py, and four EntityRepositoryCertificationTest cases exercising the guard directly (omit-preserves, explicit-applies, overrideMetadata-clears, non-bot-clears).
❌ 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 |
|
Hi there 👋 Thanks for your contribution! The OpenMetadata team will review the PR shortly! Once it has been labeled as Let us know if you need any help! |
![gitar-bot[bot]](https://avatars.githubusercontent.com/in/827041?s=60&v=4){kind=small}
There was a problem hiding this comment.
🟡 Changes recommended
The schema change is not reflected in committed UI-generated TypeScript types (e.g., generated CreateTable/CreateTableRequest models still lack certification), leaving generated artifacts out of sync.
Once you've addressed the issues Copilot identified, you can request another Copilot review.
Pull request overview
This PR enables database ingestion to set/update table certification in the same PUT /tables request by adding certification to the CreateTable request model, ensuring certification-only changes affect sourceHash and are not skipped by the bulk fast-path.
Changes:
- Add optional
certification(AssetCertification) to theCreateTableRequestJSON schema. - Thread
certificationinto table entity creation viaTableMapper. - Refine
EntityRepository.EntityUpdater.updateCertification()to preserve existing certification only when a bot PUT omits the field (andoverrideMetadata=false), while allowing explicit bot-provided certification values. - Add Java unit tests for the updated certification guard and Python unit tests asserting
sourceHashchanges with certification payload changes.
File summaries
| File | Description |
|---|---|
| openmetadata-spec/src/main/resources/json/schema/api/data/createTable.json | Adds optional certification field to the CreateTable request schema. |
| openmetadata-service/src/main/java/org/openmetadata/service/resources/databases/TableMapper.java | Maps request certification onto the Table entity during create/update mapping. |
| openmetadata-service/src/main/java/org/openmetadata/service/jdbi3/EntityRepository.java | Narrows bot PUT “preserve certification” guard to omission-only + honors overrideMetadata. |
| openmetadata-service/src/test/java/org/openmetadata/service/jdbi3/EntityRepositoryCertificationTest.java | Adds unit tests covering the new guard behavior (bot omit/explicit, overrideMetadata, human). |
| ingestion/tests/unit/utils/test_source_hash.py | Adds tests verifying generate_source_hash() changes/stability with certification added/changed/omitted. |
Review details
- Files reviewed: 5/5 changed files
- Comments generated: 1
- Review effort level: Lite
💡 Add a code-review agent skill or configure MCP servers for context-aware, tailored reviews. Learn more in the docs.
| "type": "string", | ||
| "minLength": 1, | ||
| "maxLength": 32 | ||
| }, | ||
| "certification": { | ||
| "description": "Certification for a table", | ||
| "$ref": "../../type/assetCertification.json" | ||
| } |
The backend always recomputes AssetCertification.appliedDate/expiryDate server-side from AssetCertificationSettings when a certification is applied, ignoring whatever the request sent for those two fields. Only tagLabel reflects a real change. Hashing appliedDate/expiryDate would destabilize sourceHash on every ingestion run if a connector ever populates them with a run-time-relative value, defeating the bulk fast-path this PR's certification support relies on. Scoped to the certification field only (not a global key strip), since expiryDate is also a legitimate, meaningful field on regular tag metadata (TagLabelMetadata) that should stay part of the hash.
|
Hi there 👋 Thanks for your contribution! The OpenMetadata team will review the PR shortly! Once it has been labeled as Let us know if you need any help! |
Code Review ✅ Approved 1 resolved / 1 findingsAdds optional ✅ 1 resolved✅ Edge Case: Certification appliedDate/expiryDate can destabilize sourceHash
OptionsDisplay: compact → Showing less information. Comment with these commands to change the behavior for this request:
Was this helpful? React with 👍 / 👎 | Powered by Gitar — free for open source |
There was a problem hiding this comment.
🟡 Changes recommended
The CreateTable schema change needs the corresponding committed UI-generated TypeScript schema outputs regenerated/updated to keep spec-derived clients in sync.
Once you've addressed the issues Copilot identified, you can request another Copilot review.
Review details
Suppressed comments (2)
openmetadata-spec/src/main/resources/json/schema/api/data/createTable.json:119
- The CreateTable JSON schema now includes the new optional
certificationfield, but the committed UI-generated TypeScript types still defineCreateTablewithoutcertification. This will leave UI/client type generation out of sync with the spec; please regenerate and commit the updated TS schema outputs underopenmetadata-ui/src/main/resources/ui/src/generated/(e.g.,.../api/data/createTable.ts).
},
"certification": {
"description": "Certification for a table",
"$ref": "../../type/assetCertification.json"
}
ingestion/tests/unit/utils/test_source_hash.py:567
- This line exceeds the repo's Ruff line-length (120) and will likely fail formatting/lint checks; wrap the
AssetCertification(...)call across multiple lines.
certification=AssetCertification(tagLabel=tag_label, appliedDate=1800000000000, expiryDate=1999999999999),
- Files reviewed: 6/6 changed files
- Comments generated: 1
- Review effort level: Lite
| name="test_table", | ||
| databaseSchema="service.db.schema", | ||
| columns=[Column(name="id", dataType=DataType.INT)], | ||
| certification=AssetCertification(tagLabel=tag_label, appliedDate=1700000000000, expiryDate=1731536000000), |
Describe your changes:
Fixes #32792
CreateTableRequesthad nocertificationfield, so database metadata ingestion had to create/update a table first and follow up with a separate JSON Patch to/certification. That patch is invisible tosourceHash, so a source-system change that is only a certification update produces the same hash as the previous run and the bulk fast-path silently skips it.This adds an optional
certification(AssetCertification) field toCreateTableRequest, threads it throughTableMapper, and narrows the bot-overwrite guard inEntityRepository.updateCertificationso a bot's explicit certification value is applied instead of always being reverted.Type of change:
High-level design:
EntityRepository.updateCertification()had a guard that reverted any bot PUT once a certification existed, regardless of what the request contained — a workaround forCreateTableRequestnever carrying a certification value, so every bot PUT implicitly meant "no certification" and would otherwise have wiped it on every re-sync.Now that a bot can supply an explicit value, the guard only needs to protect against omission:
This mirrors the
overrideMetadataescape hatch already used fordescription/owners/domainson the same class, rather than introducing a new mechanism:overrideMetadata=true-> bypasses the guard entirely, consistent with the other protected fields.No change was needed to
generate_source_hash(): it hashes the full request viamodel_dump(), so certification is automatically included in the hash once it's a model field.Alternatives considered:
certificationfromsourceHashas a special case — rejected, since a certification-only source change would still look "unchanged" to the bulk fast path.sourceHashblind spot.Tests:
Use cases covered
CreateTableRequest.certificationon table create -> certification is applied.certificationon a later run -> existing certification (e.g. set through the UI) is preserved, not wiped.certificationvalue on a later run -> the new value is applied andsourceHashdiffers, so the bulk fast-path doesn't skip it.overrideMetadata=truestill clears certification when the request omits it, matching existing behaviour for other protected fields.Unit tests
openmetadata-service/src/test/java/org/openmetadata/service/jdbi3/EntityRepositoryCertificationTest.java— 4 new tests exercisingupdateCertification()'s guard directly (bot-omit-preserves, bot-explicit-applies, bot-omit-with-overrideMetadata-clears, human-omit-clears).ingestion/tests/unit/utils/test_source_hash.py— certification-added/changed/omitted/equivalent-payload hash stability tests forgenerate_source_hash.Backend integration tests
PUT /tablesalready acceptsCreateTableRequest).Ingestion integration tests
Manual testing performed
EntityRepositoryCertificationTestsuite (32/32 passing) and the fullorg.openmetadata.service.jdbi3.**package (616/616 passing).ingestion/tests/unit/utils/test_source_hash.pyandingestion/tests/unit/topology/test_runner.py(all passing) after regenerating Python models from the updated schema.mvn spotless:applyandmvn spotless:checkclean onopenmetadata-service.UI screen recording / screenshots:
Not applicable.
Checklist:
Fixes <issue-number>: <short explanation>Fixes #<issue-number>above.certificationis a new optional field onCreateTableRequest, so no migration is needed (existing stored data is unaffected).