Skip to content

Removed the special-case Tier filter logic that combined getTags API with aggregation#26826

Merged
Rohit0301 merged 7 commits intomainfrom
explore-tier-filter
Mar 31, 2026
Merged

Removed the special-case Tier filter logic that combined getTags API with aggregation#26826
Rohit0301 merged 7 commits intomainfrom
explore-tier-filter

Conversation

@Rohit0301
Copy link
Copy Markdown
Contributor

@Rohit0301 Rohit0301 commented Mar 27, 2026

Tier no longer calls the getTags API to fetch all tier tags and merge them with aggregation results. It now uses the standard aggregation flow like every other quick filter.

Describe your changes:

Screenshot 2026-03-27 at 11 04 43 PM

Fixes #26843

I worked on ... because ...

Type of change:

  • Bug fix
  • Improvement
  • New feature
  • Breaking change (fix or feature that would cause existing functionality to not work as expected)
  • Documentation

Checklist:

  • I have read the CONTRIBUTING document.
  • My PR title is Fixes <issue-number>: <short explanation>
  • I have commented on my code, particularly in hard-to-understand areas.
  • For JSON Schema changes: I updated the migration scripts or explained why it is not needed.

Summary by Gitar

  • Removed Tier filter special-case logic:
    • Eliminated getTags API call and manual tier option mapping in ExploreQuickFilters
    • Tier filter now uses aggregation-based options consistently with other filters
  • Updated test expectations:
    • Fixed Tier test IDs from Tier.TierX to lowercase tier.tierx format in e2e tests
    • Simplified tier filter count validation to use aggregation buckets directly

This will update automatically on new commits.

@Rohit0301 Rohit0301 self-assigned this Mar 27, 2026
@Rohit0301 Rohit0301 requested a review from a team as a code owner March 27, 2026 17:42
@Rohit0301 Rohit0301 added the safe to test Add this label to run secure Github workflows on PRs label Mar 27, 2026
@Rohit0301 Rohit0301 changed the title Removed the special-case Tier filter logic that combined getTags API … Removed the special-case Tier filter logic that combined getTags API with Mar 27, 2026
@github-actions
Copy link
Copy Markdown
Contributor

github-actions bot commented Mar 27, 2026

Jest test Coverage

UI tests summary

Lines Statements Branches Functions
Coverage: 64%
64.88% (58737/90524) 44.48% (30876/69408) 47.68% (9316/19535)

@github-actions
Copy link
Copy Markdown
Contributor

github-actions bot commented Mar 27, 2026

🟡 Playwright Results — all passed (18 flaky)

✅ 3427 passed · ❌ 0 failed · 🟡 18 flaky · ⏭️ 223 skipped

Shard Passed Failed Flaky Skipped
🟡 Shard 1 453 0 2 2
🟡 Shard 2 609 0 2 32
🟡 Shard 3 618 0 2 27
🟡 Shard 4 604 0 6 47
🟡 Shard 5 586 0 1 67
🟡 Shard 6 557 0 5 48
🟡 18 flaky test(s) (passed on retry)
  • Features/CustomizeDetailPage.spec.ts › Database - customization should work (shard 1, 1 retry)
  • Pages/UserCreationWithPersona.spec.ts › Create user with persona and verify on profile (shard 1, 1 retry)
  • Features/BulkEditEntity.spec.ts › Glossary (shard 2, 1 retry)
  • Features/BulkImport.spec.ts › Keyboard Delete selection (shard 2, 1 retry)
  • Features/Permissions/GlossaryPermissions.spec.ts › Team-based permissions work correctly (shard 3, 1 retry)
  • Flow/ExploreDiscovery.spec.ts › Should display deleted assets when showDeleted is checked and deleted is not present in queryFilter (shard 3, 1 retry)
  • Pages/Customproperties-part2.spec.ts › entityReferenceList shows item count, scrollable list, no expand toggle (shard 4, 1 retry)
  • Pages/DataContracts.spec.ts › Create Data Contract and validate for Dashboard (shard 4, 1 retry)
  • Pages/DataContractsSemanticRules.spec.ts › Validate Owner Rule Any_In (shard 4, 1 retry)
  • Pages/Domains.spec.ts › Rename domain with subdomains attached verifies subdomain accessibility (shard 4, 1 retry)
  • Pages/Domains.spec.ts › Verify Domain entity API calls do not include invalid domains field in glossary term assets (shard 4, 1 retry)
  • Pages/Entity.spec.ts › Glossary Term Add, Update and Remove for child entities (shard 4, 1 retry)
  • Pages/EntityDataConsumer.spec.ts › Tier Add, Update and Remove (shard 5, 1 retry)
  • Pages/Glossary.spec.ts › Glossary Term Update in Glossary Page should persist tree (shard 6, 1 retry)
  • Pages/ODCSImportExport.spec.ts › Multi-object ODCS contract - object selector shows all schema objects (shard 6, 1 retry)
  • Pages/Users.spec.ts › Permissions for table details page for Data Consumer (shard 6, 1 retry)
  • Pages/Users.spec.ts › Check permissions for Data Steward (shard 6, 1 retry)
  • VersionPages/EntityVersionPages.spec.ts › Directory (shard 6, 1 retry)

📦 Download artifacts

How to debug locally
# Download playwright-test-results-<shard> artifact and unzip
npx playwright show-trace path/to/trace.zip    # view trace

@gitar-bot
Copy link
Copy Markdown

gitar-bot bot commented Mar 31, 2026

Code Review ⚠️ Changes requested 2 resolved / 5 findings

Removed special-case Tier filter logic that combined getTags API with aggregation, resolving the unused searchText variable issue. However, batch tag loading does not filter out certification tags (important finding) and applyCertification does not record appliedBy/appliedAt metadata (minor finding).

⚠️ Bug: Batch tag loading path does not filter out certification tags

The single-entity getTags(String fqn) method (line 4715-4721) correctly filters out certification tags from the regular tag list using getCertificationClassification(). However, the batch loading path batchFetchTags() (line ~9051) calls getTagsInternalBatch() directly without any certification tag filtering. This means:

  • GET single entity → certification tags are excluded from tags (correct)
  • LIST entities (batch) → certification tags leak into the tags field (incorrect)

Users will see different tag sets depending on whether they fetch one entity or list multiple entities.

Suggested fix
Apply the same certification filtering in `batchFetchTags()`. After fetching tags, remove any tags whose parent FQN matches `getCertificationClassification()`:

```java
String certClassification = getCertificationClassification();
if (certClassification != null) {
  tagsByEntity.values().forEach(tags ->
    tags.removeIf(tag -> certClassification.equals(
      FullyQualifiedName.getParentFQN(tag.getTagFQN()))));
}
```
💡 Quality: applyCertification does not record appliedBy/appliedAt

The applyCertification method passes null for both appliedAt and appliedBy parameters when calling applyTag() (line 4537-4538). The standard applyTags method (used for regular tags) correctly propagates tagLabel.getAppliedBy(). This means certification tags lose audit trail information about who applied them and when, which may complicate compliance/audit workflows.

Suggested fix
Pass the current user and timestamp:
```java
.applyTag(
    ...,
    new java.sql.Timestamp(System.currentTimeMillis()),
    Entity.getCollectionDAO().userDAO() != null
        ? SecurityContext.getCurrentUser() : null,
    metadata);
```
Or at minimum propagate from the incoming tagLabel if available.
💡 Bug: Missing icon for ML model service due to enum value mismatch

The refactored entityIconMapping removed the SearchIndex.ML_MODEL_SERVICE entry ('mlModelService') while keeping EntityType.MLMODEL_SERVICE ('mlmodelService'). These have different string values due to casing ('mlModelService' vs 'mlmodelService'). Any code path that passes the SearchIndex enum value (e.g., search aggregation results from the API) will fail to find an icon, returning null instead of MlModelIcon.

Suggested fix
Add the missing entry back to the entityIconMapping:

  [SearchIndex.ML_MODEL_SERVICE]: MlModelIcon,

along with EntityType.MLMODEL_SERVICE (which is already present).
✅ 2 resolved
Quality: Unused searchText variable left after tier logic removal

📄 openmetadata-ui/src/main/resources/ui/src/components/Explore/ExploreQuickFilters.tsx:60 📄 openmetadata-ui/src/main/resources/ui/src/components/Explore/ExploreQuickFilters.tsx:70
The searchText variable is still destructured from the parsed URL query params (line 60) but is no longer used anywhere in the component. It was previously passed as the 9th argument to getAggregationOptions in the tier-specific code path that was removed. This is dead code that should be cleaned up.

Quality: New tierWithoutAsset tag not cleaned up in test afterAll

📄 openmetadata-ui/src/main/resources/ui/playwright/e2e/Features/ExploreQuickFilters.spec.ts:36-39 📄 openmetadata-ui/src/main/resources/ui/playwright/e2e/Features/ExploreQuickFilters.spec.ts:49
The test file creates a tierWithoutAsset tag in test.beforeAll (line 49) but there is no test.afterAll block to clean it up. Note that the pre-existing tier, table, and domain entities also lack afterAll cleanup — but since this PR adds a new entity (tierWithoutAsset), it should ensure proper teardown to avoid leaving test artifacts.

🤖 Prompt for agents
Code Review: Removed special-case Tier filter logic that combined getTags API with aggregation, resolving the unused searchText variable issue. However, batch tag loading does not filter out certification tags (important finding) and applyCertification does not record appliedBy/appliedAt metadata (minor finding).

1. ⚠️ Bug: Batch tag loading path does not filter out certification tags

   The single-entity `getTags(String fqn)` method (line 4715-4721) correctly filters out certification tags from the regular tag list using `getCertificationClassification()`. However, the batch loading path `batchFetchTags()` (line ~9051) calls `getTagsInternalBatch()` directly without any certification tag filtering. This means:
   
   - **GET single entity** → certification tags are excluded from `tags` (correct)
   - **LIST entities (batch)** → certification tags leak into the `tags` field (incorrect)
   
   Users will see different tag sets depending on whether they fetch one entity or list multiple entities.

   Suggested fix:
   Apply the same certification filtering in `batchFetchTags()`. After fetching tags, remove any tags whose parent FQN matches `getCertificationClassification()`:
   
   ```java
   String certClassification = getCertificationClassification();
   if (certClassification != null) {
     tagsByEntity.values().forEach(tags ->
       tags.removeIf(tag -> certClassification.equals(
         FullyQualifiedName.getParentFQN(tag.getTagFQN()))));
   }
   ```

2. 💡 Quality: applyCertification does not record appliedBy/appliedAt

   The `applyCertification` method passes `null` for both `appliedAt` and `appliedBy` parameters when calling `applyTag()` (line 4537-4538). The standard `applyTags` method (used for regular tags) correctly propagates `tagLabel.getAppliedBy()`. This means certification tags lose audit trail information about who applied them and when, which may complicate compliance/audit workflows.

   Suggested fix:
   Pass the current user and timestamp:
   ```java
   .applyTag(
       ...,
       new java.sql.Timestamp(System.currentTimeMillis()),
       Entity.getCollectionDAO().userDAO() != null
           ? SecurityContext.getCurrentUser() : null,
       metadata);
   ```
   Or at minimum propagate from the incoming tagLabel if available.

3. 💡 Bug: Missing icon for ML model service due to enum value mismatch

   The refactored `entityIconMapping` removed the `SearchIndex.ML_MODEL_SERVICE` entry ('mlModelService') while keeping `EntityType.MLMODEL_SERVICE` ('mlmodelService'). These have different string values due to casing ('mlModelService' vs 'mlmodelService'). Any code path that passes the SearchIndex enum value (e.g., search aggregation results from the API) will fail to find an icon, returning `null` instead of `MlModelIcon`.

   Suggested fix:
   Add the missing entry back to the entityIconMapping:
   
     [SearchIndex.ML_MODEL_SERVICE]: MlModelIcon,
   
   along with EntityType.MLMODEL_SERVICE (which is already present).

Options

Display: compact → Showing less information.

Comment with these commands to change:

Compact
gitar display:verbose         

Was this helpful? React with 👍 / 👎 | Gitar

@sonarqubecloud
Copy link
Copy Markdown

yan-3005 added a commit that referenced this pull request Mar 31, 2026
…pliedBy audit trail

- batchFetchTags() now filters out certification tags (matching the existing
  single-entity getTags() behavior), preventing cert tags from appearing in
  the tags field when entities are fetched via the LIST endpoint
- applyCertification() propagates tagLabel.getAppliedBy() instead of null,
  preserving the audit trail of who applied the certification
- updateCertification() sets appliedBy from updatingUser before calling
  applyCertification(), so the author is recorded on PATCH/PUT
- applyCertificationBatch() includes appliedBy from the incoming tagLabel
  when building the batch TagLabel
- Unit tests: batchFetchTagsFiltersCertificationTags,
  applyCertificationPropagatesAppliedBy, applyCertificationBatchPropagatesAppliedBy
- Integration test: test_certificationTagNotLeakingIntoTagsField verifies
  both GET (single) and LIST (batch) paths exclude cert tags from tags field

Fixes findings from Gitar bot review on #26826

Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
@Rohit0301 Rohit0301 merged commit 4a67801 into main Mar 31, 2026
45 checks passed
@Rohit0301 Rohit0301 deleted the explore-tier-filter branch March 31, 2026 07:17
Rohit0301 added a commit that referenced this pull request Mar 31, 2026
yan-3005 added a commit that referenced this pull request Mar 31, 2026
…sing appliedBy audit trail (#26876)

* Fix certification tags leaking into tags field on LIST and missing appliedBy audit trail

- batchFetchTags() now filters out certification tags (matching the existing
  single-entity getTags() behavior), preventing cert tags from appearing in
  the tags field when entities are fetched via the LIST endpoint
- applyCertification() propagates tagLabel.getAppliedBy() instead of null,
  preserving the audit trail of who applied the certification
- updateCertification() sets appliedBy from updatingUser before calling
  applyCertification(), so the author is recorded on PATCH/PUT
- applyCertificationBatch() includes appliedBy from the incoming tagLabel
  when building the batch TagLabel
- Unit tests: batchFetchTagsFiltersCertificationTags,
  applyCertificationPropagatesAppliedBy, applyCertificationBatchPropagatesAppliedBy
- Integration test: test_certificationTagNotLeakingIntoTagsField verifies
  both GET (single) and LIST (batch) paths exclude cert tags from tags field

Fixes findings from Gitar bot review on #26826

Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>

* Address Copilot review: use server-side updatedBy and avoid unnecessary list copy

- applyCertification/applyCertificationBatch: use entity.getUpdatedBy()
  instead of tagLabel.getAppliedBy() so the audit trail reflects the
  authenticated server-side user, preventing client-provided appliedBy
  from being trusted/spoofed
- batchFetchTags: removeIf directly on the mutable list returned by
  populateTagLabel instead of defensively copying on every entity
- Update unit tests to set updatedBy on the entity (not on the tagLabel)

Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>

* Fix certification column missing on all list/service views

Now that batchFetchTags() correctly filters certification tags from the
tags field (matching single-entity getTags() behavior), list views can
no longer rely on cert tags leaking into tags as a fallback. Request
the dedicated certification field explicitly instead.

- Add TabSpecificField.CERTIFICATION to commonTableFields — fixes
  ServiceDetailsPage (all 12 fetch functions), SchemaTablesTab,
  DataModelsTable, and ServiceVersionPage.fetchDirectories in one shot
- Fix ServiceVersionPage: 8 fetch functions (databases, topics,
  dashboards, pipelines, mlModels, containers, searchIndexes,
  collections) that manually specify OWNERS,TAGS — each updated to
  include CERTIFICATION
- DatabaseSchemaTable: remove now-redundant explicit CERTIFICATION
  addition since commonTableFields already includes it

Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>

* Address Copilot review: rename misleading test names and fix lint

- Rename applyCertificationPropagatesAppliedBy to
  applyCertificationUsesEntityUpdatedByAsAppliedBy
- Rename applyCertificationBatchPropagatesAppliedBy to
  applyCertificationBatchUsesEntityUpdatedByAsAppliedBy
  Names now accurately reflect that entity.getUpdatedBy() (server-side)
  is the source of appliedBy, not the incoming tagLabel field
- Run yarn ui-checkstyle and mvn spotless:apply to fix formatting

Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>

* Fix JDBI3 positional param error for PostgreSQL JSONB key-existence operator

The `?` operator in `json::jsonb ? 'certification'` was interpreted by JDBI3
as a positional parameter, causing UnableToCreateStatementException during
the v1125 certification migration on PostgreSQL. Escape with `??` so JDBI3
passes a literal `?` to the database driver.

Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>

---------

Co-authored-by: Claude Sonnet 4.6 <noreply@anthropic.com>
yan-3005 added a commit that referenced this pull request Apr 1, 2026
…sing appliedBy audit trail (#26876)

* Fix certification tags leaking into tags field on LIST and missing appliedBy audit trail

- batchFetchTags() now filters out certification tags (matching the existing
  single-entity getTags() behavior), preventing cert tags from appearing in
  the tags field when entities are fetched via the LIST endpoint
- applyCertification() propagates tagLabel.getAppliedBy() instead of null,
  preserving the audit trail of who applied the certification
- updateCertification() sets appliedBy from updatingUser before calling
  applyCertification(), so the author is recorded on PATCH/PUT
- applyCertificationBatch() includes appliedBy from the incoming tagLabel
  when building the batch TagLabel
- Unit tests: batchFetchTagsFiltersCertificationTags,
  applyCertificationPropagatesAppliedBy, applyCertificationBatchPropagatesAppliedBy
- Integration test: test_certificationTagNotLeakingIntoTagsField verifies
  both GET (single) and LIST (batch) paths exclude cert tags from tags field

Fixes findings from Gitar bot review on #26826

Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>

* Address Copilot review: use server-side updatedBy and avoid unnecessary list copy

- applyCertification/applyCertificationBatch: use entity.getUpdatedBy()
  instead of tagLabel.getAppliedBy() so the audit trail reflects the
  authenticated server-side user, preventing client-provided appliedBy
  from being trusted/spoofed
- batchFetchTags: removeIf directly on the mutable list returned by
  populateTagLabel instead of defensively copying on every entity
- Update unit tests to set updatedBy on the entity (not on the tagLabel)

Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>

* Fix certification column missing on all list/service views

Now that batchFetchTags() correctly filters certification tags from the
tags field (matching single-entity getTags() behavior), list views can
no longer rely on cert tags leaking into tags as a fallback. Request
the dedicated certification field explicitly instead.

- Add TabSpecificField.CERTIFICATION to commonTableFields — fixes
  ServiceDetailsPage (all 12 fetch functions), SchemaTablesTab,
  DataModelsTable, and ServiceVersionPage.fetchDirectories in one shot
- Fix ServiceVersionPage: 8 fetch functions (databases, topics,
  dashboards, pipelines, mlModels, containers, searchIndexes,
  collections) that manually specify OWNERS,TAGS — each updated to
  include CERTIFICATION
- DatabaseSchemaTable: remove now-redundant explicit CERTIFICATION
  addition since commonTableFields already includes it

Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>

* Address Copilot review: rename misleading test names and fix lint

- Rename applyCertificationPropagatesAppliedBy to
  applyCertificationUsesEntityUpdatedByAsAppliedBy
- Rename applyCertificationBatchPropagatesAppliedBy to
  applyCertificationBatchUsesEntityUpdatedByAsAppliedBy
  Names now accurately reflect that entity.getUpdatedBy() (server-side)
  is the source of appliedBy, not the incoming tagLabel field
- Run yarn ui-checkstyle and mvn spotless:apply to fix formatting

Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>

* Fix JDBI3 positional param error for PostgreSQL JSONB key-existence operator

The `?` operator in `json::jsonb ? 'certification'` was interpreted by JDBI3
as a positional parameter, causing UnableToCreateStatementException during
the v1125 certification migration on PostgreSQL. Escape with `??` so JDBI3
passes a literal `?` to the database driver.

Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>

---------

Co-authored-by: Claude Sonnet 4.6 <noreply@anthropic.com>
(cherry picked from commit 2ef1420)
ulixius9 pushed a commit that referenced this pull request Apr 6, 2026
…sing appliedBy audit trail (#26876)

* Fix certification tags leaking into tags field on LIST and missing appliedBy audit trail

- batchFetchTags() now filters out certification tags (matching the existing
  single-entity getTags() behavior), preventing cert tags from appearing in
  the tags field when entities are fetched via the LIST endpoint
- applyCertification() propagates tagLabel.getAppliedBy() instead of null,
  preserving the audit trail of who applied the certification
- updateCertification() sets appliedBy from updatingUser before calling
  applyCertification(), so the author is recorded on PATCH/PUT
- applyCertificationBatch() includes appliedBy from the incoming tagLabel
  when building the batch TagLabel
- Unit tests: batchFetchTagsFiltersCertificationTags,
  applyCertificationPropagatesAppliedBy, applyCertificationBatchPropagatesAppliedBy
- Integration test: test_certificationTagNotLeakingIntoTagsField verifies
  both GET (single) and LIST (batch) paths exclude cert tags from tags field

Fixes findings from Gitar bot review on #26826

Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>

* Address Copilot review: use server-side updatedBy and avoid unnecessary list copy

- applyCertification/applyCertificationBatch: use entity.getUpdatedBy()
  instead of tagLabel.getAppliedBy() so the audit trail reflects the
  authenticated server-side user, preventing client-provided appliedBy
  from being trusted/spoofed
- batchFetchTags: removeIf directly on the mutable list returned by
  populateTagLabel instead of defensively copying on every entity
- Update unit tests to set updatedBy on the entity (not on the tagLabel)

Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>

* Fix certification column missing on all list/service views

Now that batchFetchTags() correctly filters certification tags from the
tags field (matching single-entity getTags() behavior), list views can
no longer rely on cert tags leaking into tags as a fallback. Request
the dedicated certification field explicitly instead.

- Add TabSpecificField.CERTIFICATION to commonTableFields — fixes
  ServiceDetailsPage (all 12 fetch functions), SchemaTablesTab,
  DataModelsTable, and ServiceVersionPage.fetchDirectories in one shot
- Fix ServiceVersionPage: 8 fetch functions (databases, topics,
  dashboards, pipelines, mlModels, containers, searchIndexes,
  collections) that manually specify OWNERS,TAGS — each updated to
  include CERTIFICATION
- DatabaseSchemaTable: remove now-redundant explicit CERTIFICATION
  addition since commonTableFields already includes it

Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>

* Address Copilot review: rename misleading test names and fix lint

- Rename applyCertificationPropagatesAppliedBy to
  applyCertificationUsesEntityUpdatedByAsAppliedBy
- Rename applyCertificationBatchPropagatesAppliedBy to
  applyCertificationBatchUsesEntityUpdatedByAsAppliedBy
  Names now accurately reflect that entity.getUpdatedBy() (server-side)
  is the source of appliedBy, not the incoming tagLabel field
- Run yarn ui-checkstyle and mvn spotless:apply to fix formatting

Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>

* Fix JDBI3 positional param error for PostgreSQL JSONB key-existence operator

The `?` operator in `json::jsonb ? 'certification'` was interpreted by JDBI3
as a positional parameter, causing UnableToCreateStatementException during
the v1125 certification migration on PostgreSQL. Escape with `??` so JDBI3
passes a literal `?` to the database driver.

Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>

---------

Co-authored-by: Claude Sonnet 4.6 <noreply@anthropic.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

safe to test Add this label to run secure Github workflows on PRs

Projects

None yet

Development

Successfully merging this pull request may close these issues.

Removed the special-case Tier filter logic that combined getTags API with aggregation

2 participants