fix(ui): preserve TagLabel server fields when saving tags (#28038)#28105
Conversation
TagsContainerV2.handleSave rebuilt each tag from an 8-field allowlist that silently dropped appliedBy, appliedAt, metadata, and reason — added to the TagLabel schema in #24817. The resulting JSON-Patch diff emitted spurious `remove /tags/N/appliedBy` ops, which the backend rejected with 500 when the path no longer existed at apply time (closes #28038). Pass every TagLabel schema field on the option payload through to the PATCH so server-managed fields survive the diff. The Jest schema-coverage test uses `Required<TagLabel>` so adding a new field to tagLabel.json forces the test fixture and assertion to cover it, preventing the same class of bug from recurring. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
|
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! |
There was a problem hiding this comment.
Pull request overview
This PR fixes tag saving in the UI so TagLabel server-managed fields are preserved when tags are edited, preventing unintended JSON Patch removals and related backend failures.
Changes:
- Preserves all current
TagLabelfields inTagsContainerV2.handleSave. - Adds Jest coverage for preserving applied fields and full
TagLabelpass-through. - Adds Playwright regression coverage for add-one/remove-one tag saves preserving
appliedBy.
Reviewed changes
Copilot reviewed 3 out of 3 changed files in this pull request and generated 1 comment.
| File | Description |
|---|---|
openmetadata-ui/src/main/resources/ui/src/components/Tag/TagsContainerV2/TagsContainerV2.tsx |
Updates tag save payload construction to retain server-managed fields. |
openmetadata-ui/src/main/resources/ui/src/components/Tag/TagsContainerV2/TagsContainerV2.test.tsx |
Adds unit tests for preserving TagLabel fields during save. |
openmetadata-ui/src/main/resources/ui/playwright/e2e/Pages/Tags.spec.ts |
Adds E2E regression test for simultaneous tag add/remove behavior. |
The two `as` casts those imports backed were removed; TypeScript inference covers the remaining flow. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
Code Review ✅ ApprovedPreserves TagLabel server-managed fields like appliedBy and appliedAt during UI save operations by removing the restrictive allowlist. Verified through new Jest fixtures and Playwright E2E tests to prevent future data loss regressions. OptionsDisplay: compact → Showing less information. Comment with these commands to change:
Was this helpful? React with 👍 / 👎 | Gitar |
|
🟡 Playwright Results — all passed (15 flaky)✅ 4071 passed · ❌ 0 failed · 🟡 15 flaky · ⏭️ 86 skipped
🟡 15 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 |
|
Changes have been cherry-picked to the 1.12.9 branch. |
…28105) * fix(ui): preserve TagLabel server fields when saving tags TagsContainerV2.handleSave rebuilt each tag from an 8-field allowlist that silently dropped appliedBy, appliedAt, metadata, and reason — added to the TagLabel schema in #24817. The resulting JSON-Patch diff emitted spurious `remove /tags/N/appliedBy` ops, which the backend rejected with 500 when the path no longer existed at apply time (closes #28038). Pass every TagLabel schema field on the option payload through to the PATCH so server-managed fields survive the diff. The Jest schema-coverage test uses `Required<TagLabel>` so adding a new field to tagLabel.json forces the test fixture and assertion to cover it, preventing the same class of bug from recurring. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com> * chore(ui): drop unused TagLabel/EntityTags imports from TagsContainerV2 The two `as` casts those imports backed were removed; TypeScript inference covers the remaining flow. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com> --------- Co-authored-by: Siddhant <siddhant@MacBook-Pro-751.local> Co-authored-by: Claude Opus 4.7 (1M context) <noreply@anthropic.com> (cherry picked from commit bba0bf8)
|
Changes have been cherry-picked to the 1.13 branch. |
…28105) * fix(ui): preserve TagLabel server fields when saving tags TagsContainerV2.handleSave rebuilt each tag from an 8-field allowlist that silently dropped appliedBy, appliedAt, metadata, and reason — added to the TagLabel schema in #24817. The resulting JSON-Patch diff emitted spurious `remove /tags/N/appliedBy` ops, which the backend rejected with 500 when the path no longer existed at apply time (closes #28038). Pass every TagLabel schema field on the option payload through to the PATCH so server-managed fields survive the diff. The Jest schema-coverage test uses `Required<TagLabel>` so adding a new field to tagLabel.json forces the test fixture and assertion to cover it, preventing the same class of bug from recurring. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com> * chore(ui): drop unused TagLabel/EntityTags imports from TagsContainerV2 The two `as` casts those imports backed were removed; TypeScript inference covers the remaining flow. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com> --------- Co-authored-by: Siddhant <siddhant@MacBook-Pro-751.local> Co-authored-by: Claude Opus 4.7 (1M context) <noreply@anthropic.com> (cherry picked from commit bba0bf8)



Summary
TagsContainerV2.handleSaveto pass every TagLabel schema field on the option payload through to the PATCH, so server-managed fields (appliedBy,appliedAt,metadata,reason) survive the JSON-Patch diff.500 Non-existing name/value pair in the object for key appliedBywhen adding one tag and removing another in the same save. The underlying cause was that the UI silently wipedappliedBy/appliedAton every save:handleSaverebuilt each tag from an 8-field allowlist that pre-dated those fields being added to the schema in Tagging explanation #24817.fast-json-patch.compare(old, new)then emittedremove /tags/N/appliedByops the UI never intended; the backend correctly refused them when the path no longer existed at apply time.A future PR adding another optional field to
TagLabelwould re-introduce the same class of bug. To prevent that:passes every TagLabel schema field through to onSelectionChange) builds aRequired<TagLabel>fixture. Adding a new field to the schema means the fixture won't compile until it's populated; once it is, the assertion loop catches ahandleSavethat doesn't pass it through.appliedBymatches the server-populated value before the edit. Together with the PATCH status check, this covers both halves of the bug: no 500 and no silent data loss.Test plan
yarn test src/components/Tag/TagsContainerV2/TagsContainerV2.test.tsx— 3/3 passyarn ui-checkstyle:changedon the touched src files — cleanyarn ui-checkstyle:playwright:changed playwright/e2e/Pages/Tags.spec.ts— cleannpx tsc --noEmit— no errors on changed filesAdds one tag and removes another in the same save preserves appliedBy on the kept tag) — passes againstyarn start(Vite dev server on :3000 with the live code). Test verifies (a) PATCH returns 200 and (b) the kept tag'sappliedBymatches the pre-edit server-populated value.appliedByis intact via API.