fix: scope glossary term duplicate-name check to parent, not whole glossary (#30144)#30209
Conversation
…ry (open-metadata#30144) checkDuplicateTerms() rejected a new glossary term whenever any term with the same name existed anywhere in the glossary tree, since its SQL query matched on glossaryHash.% (all descendants) instead of the term's actual parent. This incorrectly blocked legitimate terms such as "Banking.Account.Balance" and "Banking.Loan.Balance", which have distinct FQNs and should both be creatable. Scope the check to direct children of the term's parent (or the glossary root for top-level terms) instead of the entire glossary subtree.
❌ 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! |
Addresses review feedback on open-metadata#30209: the call to getGlossaryTermCountIgnoreCaseUnderParent(parentFqn, parentFqn, ...) passes the same FQN twice on purpose (each @BindConcat parameter applies a different wildcard suffix), not by mistake. Add comments at the DAO method and call site to make that explicit.
|
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 ✅ ApprovedRestricts the duplicate glossary term name check to direct siblings by matching only the immediate parent's FQN. This prevents false positive validation errors when different parents contain terms with the same leaf name. OptionsDisplay: compact → Showing less information. Comment with these commands to change the behavior for this request:
Was this helpful? React with 👍 / 👎 | Gitar |
Fixes #30144
Root cause
GlossaryTermRepository.checkDuplicateTerms()rejects a new glossary termif any term with the same name (case-insensitive) exists anywhere in the
glossary, via a SQL query that matches
fqnHash LIKE glossaryHash.%—i.e. the entire glossary subtree at any depth, not just the term's direct
siblings. Since glossary term uniqueness is actually defined by FQN
(
glossary.parent.term), this over-broad check incorrectly rejects twoterms that legitimately share a leaf name under different parents, e.g.
Banking.Account.BalanceandBanking.Loan.Balance.What changed
getGlossaryTermCountIgnoreCaseUnderParentinCollectionDAO,scoped to direct children of a given parent FQN (using a
LIKE/NOT LIKEpair to match one level deep and exclude grandchildren).
checkDuplicateTermsinGlossaryTermRepositoryto check againstthe term's actual parent (or the glossary root for top-level terms)
instead of the whole glossary tree.
This is a minimal, targeted fix — the sibling
checkDuplicateTermsForUpdatemethod has the same scoping pattern but wasn't touched, since it isn't on
the path this issue reports and is out of scope for this change.
Test evidence
Added
GlossaryResourceIT#test_createGlossaryTerm_sameNameUnderDifferentParents_succeeds,which creates two top-level terms ("Account", "Loan") and then a child term
named "Balance" under each. Verified fail → pass locally:
AssertionFailedError: ... A term with the name 'Balance' already exists in '<glossary>' glossary.Account.BalanceandLoan.Balancearecreated successfully with correct FQNs.
Also ran the full
GlossaryResourceIT+GlossaryTermResourceITsuites(524 tests) with the fix applied — all passed, no regressions (including
existing true-duplicate-name rejection behavior for same-parent terms).
spotless:checkpasses clean on the touched modules.Prepared with AI assistance (Claude Code, Anthropic), reviewed for correctness before submission.
Greptile Summary
This PR scopes the glossary term duplicate-name check from the entire glossary tree to direct siblings only, fixing a bug where two terms with the same leaf name under different parents (e.g.
Banking.Account.BalancevsBanking.Loan.Balance) were incorrectly rejected as duplicates.CollectionDAO.java: AddsgetGlossaryTermCountIgnoreCaseUnderParent, which uses aLIKE parentHash.%/NOT LIKE parentHash.%.%pair to match only direct children of a given parent, relying on the fact that FQN hash segments are hex strings containing no.characters.GlossaryTermRepository.java: UpdatescheckDuplicateTermsto deriveparentFqnfrom the term's direct parent (or the glossary root for top-level terms) and calls the new scoped DAO method.GlossaryResourceIT.java: Adds an integration test that verifiesBalancecan be created under two different parents in the same glossary, confirming the regression is fixed.Confidence Score: 5/5
Safe to merge — the fix is narrow and well-targeted, touching only the create-path duplicate check with no changes to update/delete paths or existing query behavior.
The SQL LIKE/NOT LIKE approach is sound given that FQN hash segments are hex strings with no dots, so
.unambiguously acts as a segment separator. The@BindConcatannotation processes each parameter independently, so the two bindings produce correctly distinct suffixes. The!updateguard in the repository ensures the new method is only invoked on create. The integration test directly reproduces the reported failure and verifies the fix, and the PR description confirms no regressions in the broader test suite.No files require special attention.
Important Files Changed
getGlossaryTermCountIgnoreCaseUnderParentwith a LIKE/NOT LIKE pair scoped to direct children; the two@BindConcatparameters are intentionally distinct (different wildcard suffixes) and the design is well-documented.entity.getParent() != nullto determine the effective parent FQN and calls the new scoped DAO method; only affects the create path (!updateguard is unchanged).try/catch+returnpattern correctly handles compiler reachability for theloanBalancevariable and is a valid testing idiom.Sequence Diagram
%%{init: {'theme': 'neutral'}}%% sequenceDiagram participant Client participant GlossaryTermRepository participant CollectionDAO participant DB as glossary_term_entity Client->>GlossaryTermRepository: create(GlossaryTerm entity) GlossaryTermRepository->>GlossaryTermRepository: checkDuplicateTerms(entity) Note over GlossaryTermRepository: parentFqn = entity.getParent()?.fqn<br/>OR entity.getGlossary().fqn GlossaryTermRepository->>CollectionDAO: getGlossaryTermCountIgnoreCaseUnderParent(parentFqn, parentFqn, name) Note over CollectionDAO: @BindConcat builds:<br/>parentHash = hash(parentFqn) + ".%"<br/>parentHashNested = hash(parentFqn) + ".%.%" CollectionDAO->>DB: "SELECT COUNT(*) WHERE fqnHash LIKE parentHash AND fqnHash NOT LIKE parentHashNested AND LOWER(name) = LOWER(:termName)" DB-->>CollectionDAO: count (siblings with same name only) CollectionDAO-->>GlossaryTermRepository: count alt "count > 0" GlossaryTermRepository-->>Client: 400 duplicateGlossaryTerm else "count == 0" GlossaryTermRepository-->>Client: 200 created 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 Client participant GlossaryTermRepository participant CollectionDAO participant DB as glossary_term_entity Client->>GlossaryTermRepository: create(GlossaryTerm entity) GlossaryTermRepository->>GlossaryTermRepository: checkDuplicateTerms(entity) Note over GlossaryTermRepository: parentFqn = entity.getParent()?.fqn<br/>OR entity.getGlossary().fqn GlossaryTermRepository->>CollectionDAO: getGlossaryTermCountIgnoreCaseUnderParent(parentFqn, parentFqn, name) Note over CollectionDAO: @BindConcat builds:<br/>parentHash = hash(parentFqn) + ".%"<br/>parentHashNested = hash(parentFqn) + ".%.%" CollectionDAO->>DB: "SELECT COUNT(*) WHERE fqnHash LIKE parentHash AND fqnHash NOT LIKE parentHashNested AND LOWER(name) = LOWER(:termName)" DB-->>CollectionDAO: count (siblings with same name only) CollectionDAO-->>GlossaryTermRepository: count alt "count > 0" GlossaryTermRepository-->>Client: 400 duplicateGlossaryTerm else "count == 0" GlossaryTermRepository-->>Client: 200 created endComments Outside Diff (1)
openmetadata-service/src/main/java/org/openmetadata/service/jdbi3/GlossaryTermRepository.java, line 2041-2055 (link)checkDuplicateTermsForUpdatehas the same over-broad scoping bugcheckDuplicateTermsForUpdatestill callsgetGlossaryTermCountIgnoreCaseExcludingIdwith the glossary FQN, which matches any term anywhere in the glossary tree. RenamingBanking.Loan.Balancewould be incorrectly rejected ifBanking.Account.Balancealready exists, even though they are distinct terms with different FQNs. The PR description calls this out as out-of-scope, but it's worth tracking as a follow-up since the user-visible failure mode (rename rejected with "a term with this name already exists") would be just as confusing as the create-path bug fixed here.Reviews (2): Last reviewed commit: "docs(glossary): clarify intentional dupl..." | Re-trigger Greptile