Support parent-scoped asset counts and assets listing for glossary terms#26801
Support parent-scoped asset counts and assets listing for glossary terms#26801sonika-shah wants to merge 2 commits intomainfrom
Conversation
sonika-shah
added
safe to test
![gitar-bot[bot]](https://avatars.githubusercontent.com/in/827041?s=60&v=4){kind=small}
openmetadata-service/src/main/java/org/openmetadata/service/jdbi3/GlossaryTermRepository.java
Show resolved
Hide resolved
openmetadata-service/src/main/java/org/openmetadata/service/jdbi3/GlossaryTermRepository.java
Outdated
Show resolved
Hide resolved
There was a problem hiding this comment.
Pull request overview
Adds an optional parent scope filter to glossary-term asset count and asset listing APIs, enabling callers (e.g., UI) to request results limited to a specific glossary or glossary-term subtree.
Changes:
- Add
parentquery parameter to/v1/glossaryTerms/assets/counts,/v1/glossaryTerms/{id}/assets, and/v1/glossaryTerms/name/{fqn}/assets. - Extend
GlossaryTermRepositorymethods to acceptparentand apply parent-scope filtering. - Add integration tests covering the new
parentquery parameter on these endpoints.
Reviewed changes
Copilot reviewed 3 out of 3 changed files in this pull request and generated 6 comments.
| File | Description |
|---|---|
| openmetadata-service/src/main/java/org/openmetadata/service/resources/glossary/GlossaryTermResource.java | Exposes the new parent query parameter on glossary term asset count/listing endpoints. |
| openmetadata-service/src/main/java/org/openmetadata/service/jdbi3/GlossaryTermRepository.java | Implements parent-scope filtering for asset counts and asset listing queries. |
| openmetadata-integration-tests/src/test/java/org/openmetadata/it/tests/GlossaryTermResourceIT.java | Adds integration tests intended to validate parent-filtered asset counts and asset listing behavior. |
| List<GlossaryTerm> allGlossaryTerms = | ||
| listAll(getFields("fullyQualifiedName"), new ListFilter(null)); | ||
|
|
||
| if (parent != null && !parent.isEmpty()) { | ||
| allGlossaryTerms = | ||
| allGlossaryTerms.stream() | ||
| .filter(term -> FullyQualifiedName.isParent(term.getFullyQualifiedName(), parent)) | ||
| .collect(Collectors.toList()); |
There was a problem hiding this comment.
getAllGlossaryTermsWithAssetsCount(parent) still loads all glossary terms via listAll(...) and then filters in-memory when parent is provided. For large catalogs this can be unnecessarily expensive and defeats the purpose of a scoped query. Consider fetching only the nested terms directly (e.g., via daoCollection.glossaryTermDAO().getNestedTerms(parent) / an FQN-prefix DAO query) when parent is non-empty, and then run the count loop on that smaller set.
| List<GlossaryTerm> allGlossaryTerms = | |
| listAll(getFields("fullyQualifiedName"), new ListFilter(null)); | |
| if (parent != null && !parent.isEmpty()) { | |
| allGlossaryTerms = | |
| allGlossaryTerms.stream() | |
| .filter(term -> FullyQualifiedName.isParent(term.getFullyQualifiedName(), parent)) | |
| .collect(Collectors.toList()); | |
| List<GlossaryTerm> allGlossaryTerms; | |
| if (parent != null && !parent.isEmpty()) { | |
| // Fetch only the nested terms under the given parent to avoid loading all glossary terms | |
| allGlossaryTerms = daoCollection.glossaryTermDAO().getNestedTerms(parent); | |
| } else { | |
| allGlossaryTerms = listAll(getFields("fullyQualifiedName"), new ListFilter(null)); |
| "Filter by parent glossary or glossary term FQN. " | ||
| + "When provided, only returns assets for children whose FQN starts with this value.") |
There was a problem hiding this comment.
The parent query param description here says it "returns assets for children whose FQN starts with this value", but this endpoint returns assets for the requested term ({id}) and only uses parent as an additional scope filter. The current wording is misleading in the generated OpenAPI docs; please update it to describe the actual behavior (e.g., only return results if the term is within the provided parent scope).
| "Filter by parent glossary or glossary term FQN. " | |
| + "When provided, only returns assets for children whose FQN starts with this value.") | |
| "Optional scope filter by parent glossary or glossary term FQN. " | |
| + "When provided, assets are returned only if the requested glossary term's FQN is within this parent hierarchy (for example, its FQN starts with the given value).") |
| "Filter by parent glossary or glossary term FQN. " | ||
| + "When provided, only returns assets for children whose FQN starts with this value.") |
There was a problem hiding this comment.
The parent query param description here says it "returns assets for children whose FQN starts with this value", but this endpoint returns assets for the requested term ({fqn}) and only uses parent as an additional scope filter. Please adjust the description to match the actual behavior so the OpenAPI docs are accurate.
| "Filter by parent glossary or glossary term FQN. " | |
| + "When provided, only returns assets for children whose FQN starts with this value.") | |
| "Optional scope filter by parent glossary or glossary term FQN. " | |
| + "Assets are always returned for the requested term `{fqn}`; when `parent` is provided, " | |
| + "only assets associated to that term within the specified parent hierarchy " | |
| + "(children whose FQN starts with this value) are included.") |
| // Get counts with parent=glossary1 — should only include glossary1's terms | ||
| String countsWithParent = getAssetCounts(client, glossary1.getFullyQualifiedName()); | ||
| assertNotNull(countsWithParent); | ||
| assertFalse( | ||
| countsWithParent.contains(term3.getFullyQualifiedName()), | ||
| "Should not contain terms from glossary2 when filtering by glossary1"); | ||
|
|
||
| // Get counts without parent — should include all terms | ||
| String countsWithoutParent = getAssetCounts(client, null); | ||
| assertNotNull(countsWithoutParent); |
There was a problem hiding this comment.
These tests don’t strongly validate the new parent filtering behavior. For example, countsWithParent is only asserted to not contain term3, which would also pass if the endpoint returned {} or an empty body; and term1/term2 are created but never asserted. Consider parsing the response JSON into a map and asserting that (a) term1 and term2 keys are present, (b) term3 is absent, and (c) the filtered response differs from the unfiltered response in the expected way.
| // Query child term assets with parent filter matching its parent — should succeed | ||
| String result = | ||
| getTermAssetsById(client, childTerm.getId().toString(), parentTerm.getFullyQualifiedName()); | ||
| assertNotNull(result); | ||
|
|
||
| // Query parent term assets with parent filter = glossary — should succeed | ||
| String parentResult = | ||
| getTermAssetsById(client, parentTerm.getId().toString(), glossary.getFullyQualifiedName()); | ||
| assertNotNull(parentResult); | ||
|
|
||
| // Query child term assets with a non-matching parent — should return empty | ||
| String emptyResult = | ||
| getTermAssetsById(client, childTerm.getId().toString(), "nonexistent.glossary"); | ||
| assertNotNull(emptyResult); | ||
| } |
There was a problem hiding this comment.
get_termAssetsById_withParentFilter doesn’t currently assert the parent-filter behavior. No assets are tagged with the terms, so both the matching-parent and non-matching-parent calls can return empty results and still pass (the test only checks assertNotNull). To actually cover the new logic, create at least one real asset (e.g., a table) tagged with childTerm, then parse the paging response and assert the asset is returned when parent matches and that the result total/data is empty when parent does not match.
| // Query child term assets by name with parent filter matching — should succeed | ||
| String result = | ||
| getTermAssetsByName( | ||
| client, childTerm.getFullyQualifiedName(), parentTerm.getFullyQualifiedName()); | ||
| assertNotNull(result); | ||
|
|
||
| // Query with non-matching parent — should return empty | ||
| String emptyResult = | ||
| getTermAssetsByName(client, childTerm.getFullyQualifiedName(), "nonexistent.glossary"); | ||
| assertNotNull(emptyResult); |
There was a problem hiding this comment.
get_termAssetsByName_withParentFilter has the same issue as the ID-based test: it doesn’t tag any assets and only asserts non-null responses, so it won’t fail if the parent filter is ignored. Add a tagged asset for childTerm and assert (by parsing the paging response) that results are returned only when parent matches and are empty when it doesn’t.
OpenMetadata Service New-Code Coverage❌ FAIL. Required changed-line coverage:
Files below threshold:
Only changed executable lines under |
| boolean useParentScope = | ||
| parent != null && !parent.isEmpty() && FullyQualifiedName.isParent(fqn, parent); | ||
|
|
||
| InheritedFieldQuery query = | ||
| InheritedFieldQuery.forGlossaryTerm(term.getFullyQualifiedName(), offset, limit); | ||
| useParentScope | ||
| ? InheritedFieldQuery.forGlossaryTermChildren(parent, offset, limit) | ||
| : InheritedFieldQuery.forGlossaryTerm(fqn, offset, limit); |
There was a problem hiding this comment.
⚠️ Bug: Parent filter silently ignored when term is not under parent
The old code had a guard clause: if parent is provided but the term's FQN is not a child of parent, return an empty result list. This guard was removed in the refactor. Now when parent is provided but FullyQualifiedName.isParent(fqn, parent) is false, useParentScope is false and the code falls through to forGlossaryTerm(fqn, ...), returning the term's assets as if no parent filter was specified at all.
This is a behavioral regression — callers that pass a parent parameter expect it to act as a scope filter, but now an unrelated term's assets are returned instead of an empty list.
Suggested fix:
String fqn = term.getFullyQualifiedName();
boolean parentProvided = parent != null && !parent.isEmpty();
boolean isChild = parentProvided && FullyQualifiedName.isParent(fqn, parent);
if (parentProvided && !isChild) {
return new ResultList<>(new ArrayList<>(), null, null, 0);
}
InheritedFieldQuery query =
isChild
? InheritedFieldQuery.forGlossaryTermChildren(parent, offset, limit)
: InheritedFieldQuery.forGlossaryTerm(fqn, offset, limit);
Was this helpful? React with 👍 / 👎 | Reply gitar fix to apply this suggestion
Code Review
|
| Auto-apply | Compact |
|
|
Was this helpful? React with 👍 / 👎 | Gitar
|
🟡 Playwright Results — all passed (17 flaky)✅ 3397 passed · ❌ 0 failed · 🟡 17 flaky · ⏭️ 217 skipped
🟡 17 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 |
Describe your changes:
Fixes #3241
Type of change:
Checklist:
Fixes <issue-number>: <short explanation>