Fixes 31756: return parent glossary terms on child fields; stop delete cascade stripping manual labels - #31834
Fixes 31756: return parent glossary terms on child fields; stop delete cascade stripping manual labels#31834mohityadav766 wants to merge 3 commits into
Conversation
…delete cascade stripping manual labels Bug B of #31756: a glossary term on a table was visible on its columns only in the search index. The entity API returned each column's own tags, so the column panel and Explore disagreed. populateEntityFieldTags now projects the parent's glossary terms onto every field on read. It is the single funnel for field tag hydration, so table columns, dashboard data model columns and API endpoint schema fields are all covered, and getFlattenedEntityField already recurses into nested columns. The parent's own tag_usage row now arrives in the existing getTagsByPrefix call ("%" instead of ".%"), so this costs no extra query. A field carrying the term itself keeps MANUAL, because tagLabelMatch ignores labelType and mergeTags gives the field's own label precedence. The projected label is PROPAGATED, not DERIVED as the issue proposed. DERIVED means "recomputed on read from the glossary term's own classification tags": applyTags, applyTagsAdd and applyTagsBatchMultiTarget strip it on write and addDerivedTags strips and regenerates it on read, so a DERIVED label cannot survive a round trip. PROPAGATED is the schema's own marker for a propagated label and is untouched by both paths. The search propagation sites are switched to match so the index and the entity API agree. Also fixes the child delete script matching on tagFQN alone, which stripped the term from a column that had it applied manually. It now only removes labels the system propagated, accepting legacy DERIVED alongside PROPAGATED so labels written by earlier releases are still cleaned up without a reindex. The block was duplicated between the delete and update scripts; it is now shared. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
github-actions
Bot
added
backend
safe to test
![gitar-bot[bot]](https://avatars.githubusercontent.com/in/827041?s=60&v=4){kind=small}
| private String deleteTagLabelListBlock() { | ||
| return String.format( | ||
| """ | ||
| if (ctx._source.tags != null && params.tagDeleted != null) { | ||
| for (int i = ctx._source.tags.size() - 1; i >= 0; i--) { | ||
| for (int j = 0; j < params.tagDeleted.size(); j++) { | ||
| if (ctx._source.tags[i].tagFQN.equalsIgnoreCase(params.tagDeleted[j].tagFQN)) { | ||
| ctx._source.tags.remove(i); | ||
| break; | ||
| def existingTag = ctx._source.tags[i]; | ||
| boolean systemApplied = existingTag.labelType == null | ||
| || existingTag.labelType.equalsIgnoreCase('%s') | ||
| || existingTag.labelType.equalsIgnoreCase('%s'); | ||
| if (systemApplied) { | ||
| for (int j = 0; j < params.tagDeleted.size(); j++) { | ||
| if (existingTag.tagFQN.equalsIgnoreCase(params.tagDeleted[j].tagFQN)) { | ||
| ctx._source.tags.remove(i); | ||
| break; | ||
| } |
There was a problem hiding this comment.
💡 Edge Case: Delete script treats null labelType as system-applied
deleteTagLabelListBlock marks a tag systemApplied when existingTag.labelType == null. Any indexed tag whose labelType field is absent/null will be removed by the cascade even if it was a manually applied label, re-introducing the very stripping bug this PR fixes for legacy/edge documents that lack an explicit labelType. Consider requiring an explicit PROPAGATED/DERIVED match instead of treating null as system-applied, or confirm all indexed tags always carry a labelType.
Was this helpful? React with 👍 / 👎
❌ 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 |
![greptile-apps[bot]](https://avatars.githubusercontent.com/in/867647?s=60&v=4){kind=small}
There was a problem hiding this comment.
Pull request overview
This PR addresses parts of #31756 by making inherited glossary terms visible on child fields in the entity API response (so API and search index agree), and by tightening the search-index tag delete/update scripts to avoid stripping manually-applied labels during cascades.
Changes:
- Project a parent entity’s glossary term tags onto all descendant fields at read time via
Entity.populateEntityFieldTags, usingLabelType.PROPAGATED. - Update
SearchRepositoryTAG_LABEL_LIST propagation scripts to stampPROPAGATEDand to only delete system-applied labels (accepting legacyDERIVEDfor cleanup). - Add unit tests for the parent→field tag projection helper.
Reviewed changes
Copilot reviewed 3 out of 3 changed files in this pull request and generated 3 comments.
| File | Description |
|---|---|
| openmetadata-service/src/main/java/org/openmetadata/service/Entity.java | Fetches parent + descendant tag_usage rows in one query and merges parent glossary terms into each field’s tags on read. |
| openmetadata-service/src/main/java/org/openmetadata/service/search/SearchRepository.java | Switches propagated labelType to PROPAGATED and refactors/guards the delete block to preserve manual labels. |
| openmetadata-service/src/test/java/org/openmetadata/service/EntityPropagatedTagsTest.java | Adds targeted unit tests validating read-time projection behavior and merge precedence. |
💡 Add a code-review agent skill or configure MCP servers for context-aware, tailored reviews. Learn more in the docs.
| def existingTag = ctx._source.tags[i]; | ||
| boolean systemApplied = existingTag.labelType == null | ||
| || existingTag.labelType.equalsIgnoreCase('%s') | ||
| || existingTag.labelType.equalsIgnoreCase('%s'); |
| // Fetch All tags belonging to Prefix. "%" rather than ".%" so the parent's own row arrives in | ||
| // the same query — its glossary terms are projected onto every field below. The DAO returns | ||
| // null | ||
| // for an entity type that does not support tags. |
| /** | ||
| * Removes a parent's tags from a child doc, but only the labels the system itself propagated. A | ||
| * child that carries the same term MANUAL (a column explicitly tagged with the term the table also | ||
| * carries) keeps it — matching on tagFQN alone used to strip it. DERIVED is accepted alongside | ||
| * PROPAGATED so labels written by earlier releases, which stamped DERIVED, are still cleaned up | ||
| * without requiring a reindex first. | ||
| */ | ||
| private String deleteTagLabelListBlock() { | ||
| return String.format( | ||
| """ | ||
| if (ctx._source.tags != null && params.tagDeleted != null) { | ||
| for (int i = ctx._source.tags.size() - 1; i >= 0; i--) { | ||
| for (int j = 0; j < params.tagDeleted.size(); j++) { | ||
| if (ctx._source.tags[i].tagFQN.equalsIgnoreCase(params.tagDeleted[j].tagFQN)) { | ||
| ctx._source.tags.remove(i); | ||
| break; | ||
| def existingTag = ctx._source.tags[i]; | ||
| boolean systemApplied = existingTag.labelType == null | ||
| || existingTag.labelType.equalsIgnoreCase('%s') | ||
| || existingTag.labelType.equalsIgnoreCase('%s'); | ||
| if (systemApplied) { | ||
| for (int j = 0; j < params.tagDeleted.size(); j++) { | ||
| if (existingTag.tagFQN.equalsIgnoreCase(params.tagDeleted[j].tagFQN)) { | ||
| ctx._source.tags.remove(i); | ||
| break; | ||
| } | ||
| } | ||
| } | ||
| } | ||
| } | ||
| """ | ||
| + SearchClient.TAG_RESEPARATION_SCRIPT; | ||
| """, | ||
| TagLabel.LabelType.PROPAGATED.value(), TagLabel.LabelType.DERIVED.value()); | ||
| } | ||
|
|
||
| private String generateDeleteTagLabelListScript() { | ||
| return deleteTagLabelListBlock() + SearchClient.TAG_RESEPARATION_SCRIPT; | ||
| } |
…ected label being persisted Adds GlossaryTagChildPropagationIT for #31756 and fixes a defect it found in the previous commit. The defect: every never-persist filter on the write path tested only for DERIVED, so a projected PROPAGATED label could be written into tag_usage on a round trip. A client that GETs a table — whose columns now carry projected labels — and PUTs it back unchanged would pin the label in place, and it would then survive the parent's term being removed. That is the phantom tag this change set exists to remove, reintroduced through the new label. All seven write-path sites now route through TagLabelUtil.isSystemGenerated, which covers DERIVED and PROPAGATED together. The read path in addDerivedTags is deliberately left alone: the projection depends on PROPAGATED surviving there. The IT verifies against a real stack that a term on a table is returned on its top-level and nested columns as PROPAGATED and disappears when the table's term is removed, and that a column carrying the term itself keeps MANUAL, is not duplicated, and survives the table's term being removed. Its third case is @disabled because it reproduces an open half of #31756 that this change set does not fix. SearchRepository logs "update with propagation - type: table", so the gate opens and propagateInheritedFieldsToChildren runs, yet the column doc — present in column_search_index and resolvable by fqnParts — still has tags: []. The label never reaches child search docs on the add path, so removal is never reached. The javadoc records the evidence and a labelled, unproven hypothesis. It is disabled rather than deleted so the repro survives. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
Code Review 👍 Approved with suggestions 1 resolved / 2 findingsProjects parent glossary terms onto entity fields as propagated labels and updates search deletion scripts to preserve manual child labels. Consider tightening the delete script's null labelType handling to avoid accidentally removing unannotated records. 💡 Edge Case: Delete script treats null labelType as system-applied📄 openmetadata-service/src/main/java/org/openmetadata/service/search/SearchRepository.java:3248-3262
✅ 1 resolved✅ Bug: PROPAGATED column tags can be persisted on table write
🤖 Prompt for agentsOptionsDisplay: 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 |
| case TAG_LABEL_LIST -> { | ||
| List<TagLabel> tagLabels = | ||
| JsonUtils.readOrConvertValues(field.getNewValue(), TagLabel.class); | ||
| tagLabels.forEach(t -> t.setLabelType(TagLabel.LabelType.DERIVED)); | ||
| tagLabels.forEach(t -> t.setLabelType(TagLabel.LabelType.PROPAGATED)); | ||
| data.put("tagAdded", tagLabels); | ||
| script.append(generateAddTagLabelListScript()); |
|
🔴 Playwright Results — workflow failedValidated commit ✅ 1278 passed · ❌ 2 failed · 🟡 1 flaky · ⏭️ 0 skipped · 🧰 0 lifecycle flaky PerformanceBlocking targets: ✅ met · Optimization targets: 🟡 in progress Shard-job maxima below are not the full workflow wall time; the linked run includes build, fixture, planning, and reporting. 🕒 Full workflow signal wall (to summary) 1h 4m 21s ⏱️ Max setup 6m 28s · max shard execution 19m 30s · max shard-job elapsed before upload 25m 51s · reporting 8s 🌐 200.88 requests/attempt · 2.12 app boots/UI scenario · 22.48% common-shard skew Optimization targets still in progress:
Genuine Failures (failed on all attempts)❌
|
Describe your changes:
Fixes #31756
I worked on Bug B of #31756 — a glossary term applied to a table was only ever visible on its columns in the search index. The entity API returned each column's own tags, so the column tag panel showed nothing while Explore and the glossary Assets tab showed the term.
Entity.populateEntityFieldTagsnow projects the parent's glossary terms onto every field on read. This is the single funnel for field tag hydration, so table columns, dashboard-data-model columns and API-endpoint schema fields are all covered, andgetFlattenedEntityFieldalready recurses — nested columns are covered by construction. The parent's owntag_usagerow now arrives in the existinggetTagsByPrefixcall ("%"instead of".%"), so this costs no extra query. A field that carries the term itself keepsMANUAL:tagLabelMatchcompares tagFQN+source and ignores labelType, somergeTagsgives the field's own label precedence.TagLabelUtil.isSystemGenerated— projections must never be persisted. All seven never-persist filters on the write path previously tested only forDERIVED, so a projectedPROPAGATEDlabel could be written intotag_usageon a round trip: GET a table (columns now carrying projected labels) → PUT it back unchanged → the projection becomes a stored row that outlives the parent's term. That is exactly the phantom tag this PR exists to remove, reintroduced through the new label. Found by writing the IT. The read path inaddDerivedTagsis deliberately untouched — the projection depends onPROPAGATEDsurviving there.SearchRepository— propagation sites stamp the same label as the entity API, and the child delete script no longer matches ontagFQNalone (which stripped the term from a column that had it applied manually). The delete block duplicated between the delete and update scripts is factored out.PROPAGATED, notDERIVEDThe issue's acceptance criteria ask for
labelType=DERIVED. That cannot work.DERIVEDalready means "recomputed on read from the glossary term's own classification tags":EntityRepository.applyTags:5907,applyTagsAdd:5948,applyTagsAddInFlushAndDeferRdf:9469,CollectionDAO.applyTagsBatchMultiTarget:7676TagLabelUtil.addDerivedTags:289/addDerivedTagsWithPreFetched:375, fed bygetDerivedTagsBatch(CollectionDAO:7033)A
DERIVEDlabel cannot survive a round trip, and relaxing the filter is not an option — it could not distinguish a stale label from a term whose classification tags changed (must drop) from a projected parent label (must keep).LabelType.PROPAGATEDis the schema's own marker, had zero write usages, and is already wired tolabel.propagatedinAdvancedSearch.constants.ts. User-visible: the chip on an inherited column tag reads "Propagated". @yan-3005 — this changes your acceptance criteria; flagging for your call.Projection on read, not stored rows
Deliberate: columns ingested after the table was tagged inherit automatically, no backfill migration, and no ~1 row per column per term of write amplification.
Type of change:
High-level design:
Two defects, fixed at shared choke points rather than per-caller.
Entity.populateEntityFieldTags, the one function every field-tag hydration path routes through (~10 call sites acrossTableRepository,DashboardDataModelRepository,APIEndpointRepository,TestCaseRepository), rather than patchingTableRepository.setFieldsalone.generateDeleteTagLabelListScriptandgenerateUpdateTagLabelListScript. Extracted todeleteTagLabelListBlock()and given a labelType guard once, so both scripts get the fix.Backward compatibility. Deployed indices carry
labelType: "Derived"from earlier releases, so the new delete block accepts bothPROPAGATEDand legacyDERIVEDas system-applied — existing labels are still cleaned up without a reindex. Writes going forward arePROPAGATEDonly. No schema change:LabelType.PROPAGATEDalready exists intagLabel.jsonand the generated enums, so nomake generateand no migration.Alternatives rejected. (a) Persisting one
tag_usagerow per child per term — needs a backfill, a second write path for children created later, and heavy write amplification on wide tables. (b) Relaxing theDERIVEDfilter — conflates two meanings on one enum value.Tests:
Use cases covered
GET /tables/{id}?fields=columns,tagsMANUAL, is not duplicated, and survives the table's term being removedDERIVEDlabels on the parent are not projectedUnit tests
openmetadata-service/src/test/java/org/openmetadata/service/EntityPropagatedTagsTest.java— 7 tests,Tests run: 7, Failures: 0, Errors: 0PROPAGATED; the parent's own label is not mutated (defensive copy — without it the table's own tags flip toPROPAGATED); classification +DERIVEDnot projected; null/empty; a field's ownMANUALwins; and thatPROPAGATED/DERIVEDare non-persistable whileMANUAL/AUTOMATEDremain persistable.Entity.java/SearchRepository.java— both are very large and this PR touches a small slice of each. The 90%-on-changed-class target is not demonstrated.Backend integration tests
openmetadata-integration-tests/src/test/java/org/openmetadata/it/tests/GlossaryTagChildPropagationIT.java—Tests run: 3, Failures: 0, Errors: 0, Skipped: 1,BUILD SUCCESSagainst a Testcontainers stack (MySQL + Elasticsearch + server).PROPAGATEDthen gone after removal; a column's ownMANUALlabel wins, is not duplicated, and survives the table's term being removed.Ingestion integration tests
Playwright (UI) tests
playwright/e2e/Pages/Glossary.spec.ts:1197asserts an asset count now inflated by propagated column entries.Manual testing performed
mvn install -pl openmetadata-service→BUILD SUCCESS;mvn test -Dtest=EntityPropagatedTagsTest→ 7/7mvn test -pl openmetadata-integration-tests -Dtest=GlossaryTagChildPropagationIT→BUILD SUCCESS, 2 passed / 1 skippedmvn spotless:apply→ clean, only files in this PR touchedUI screen recording / screenshots:
Not applicable — no UI files changed.
🔍 New finding: the cascade never reaches child search docs on the add path
The issue describes delete as the broken direction. Writing the IT showed add is broken too, which is why the third test is
@Disabledrather than deleted — the repro is worth keeping.Observed on a real stack: after the term is applied to the table,
SearchRepositorylogsSearch index update with propagation - type: table(propagate: 7ms), sorequiresPropagationopened the gate andpropagateInheritedFieldsToChildrenran — yet the column doc, present incolumn_search_indexand resolvable byfqnParts, still hastags: []. Removal is never even reached.Leading hypothesis, explicitly not proven:
EntityIndexCapabilityRegistryis populated fromEntity.registerEntity, andtableColumnis a pseudo-type with no repository, so it has no registered capability.IndexMappingValidatoralready warns that a child alias in that state is skipped by soft-delete propagation; the descriptor-driven cascade may be missing it the same way.Also noticed while reading that log line:
IndexMappingValidatoremits its warning with unsubstituted%splaceholders (Parent '%s' declares child alias '%s' ...), so it never says which parent or alias. Trivial, unrelated, worth a separate fix.Correction to my earlier claim
An earlier revision of this description asserted that removing a table's tag already cleared descendant column ES docs "via the column doc rebuild". That was inference from code and the IT disproves it. A tags-only change takes
updateTableColumnsInheritedFields— a script update-by-query that never touchestags— so there is no rebuild to ride on. Status table below is corrected.Status against #31756's acceptance criteria
GET /tables/{fqn}returns columns with the parent's termPROPAGATED), IT-verified incl. nestedMANUALterm is not clobberedpopulateEntityFieldTags, and the cascade never fires from the bulk APIs becauseupdateEntity(ref)nulls the ChangeDescription (SearchRepository:1879)CsvUtil.java:295/CSV.utils.tsx:121filter onDERIVEDonly, soPROPAGATEDneeds adding or export/re-import turns it intoMANUALAlso outstanding, found while tracing:
EntityUtil.populateEntityReferences's return value is discarded at all four bulk call sites (GlossaryTermRepository:1106,1403,TagRepository:517,677) — it prunes orphans from a copy while the loop iterates the original. And the UI keys "non-removable tag" onDerivedonly (AsyncSelectList.tsx:239,TreeAsyncSelectList.tsx:262), so a user can hand-delete a propagated tag that then reappears.Checklist:
Fixes <issue-number>: <short explanation>Fixes #<issue-number>above.Scope note: this was opened as a draft and has since been marked ready by a maintainer. Three of the issue's acceptance criteria remain unaddressed and the search-side cascade is a known-failing repro rather than a fix, so please review it as a partial fix for Bug B rather than a close-out of #31756. Happy to split the remainder into a follow-up.
🤖 Generated with Claude Code
Greptile Summary
This PR projects parent glossary terms onto entity fields as propagated labels, prevents projected labels from being persisted, and preserves manually applied child labels during search propagation.
Confidence Score: 5/5
The PR appears safe to merge because no blocking failure remains.
No blocking failure remains.
Important Files Changed
Sequence Diagram
Reviews (2): Last reviewed commit: "test(tags): cover the child glossary ter..." | Re-trigger Greptile
Context used: