Skip to content

Conversation

@siddhant1
Copy link
Member

@siddhant1 siddhant1 commented Dec 27, 2025

Describe your changes:

Fixes

I worked on ... because ...


Summary by Gitar

  • Domain filtering refactor:
    • Changed from exact match to prefix matching on domains.fullyQualifiedName for hierarchical domain support (parent domains show child assets)
  • Code organization improvement:
    • Extracted withDomainFilter logic into DomainUtils.tsx utility module with comprehensive unit tests (DomainUtils.test.tsx - 585 lines)
  • Query structure modernization:
    • Replaced URL query string manipulation with Elasticsearch DSL query_filter parameter using prefix query syntax
  • E2E test coverage:
    • Added DomainFilterQueryFilter.spec.ts with 9 scenarios including 3-level domain hierarchy and filter persistence validation

This will update automatically on new commits.


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.

@github-actions
Copy link
Contributor

Hi there 👋 Thanks for your contribution!

The OpenMetadata team will review the PR shortly! Once it has been labeled as safe to test, the CI workflows
will start executing and we'll be able to make sure everything is working as expected.

Let us know if you need any help!

@github-actions
Copy link
Contributor

github-actions bot commented Dec 27, 2025

Jest test Coverage

UI tests summary

Lines Statements Branches Functions
Coverage: 64%
64.79% (51496/79484) 42.51% (25210/59302) 45.96% (7945/17285)

@gitar-bot
Copy link

gitar-bot bot commented Jan 1, 2026

🔍 CI failure analysis for f91cc51: 3 of 6 shards now fail (1, 2, 4). Shard 2 regression: 3 new failures in BulkImport and Glossary tests. All failures remain unrelated to domain filtering PR.

Issue

CI Regression: Shard 2 now fails with 3 new test failures. Total: 3 shards failing (1, 2, 4).

Current Status

Passing Shards (3/6): ✅

  • Shard 3: SUCCESS
  • Shard 5: SUCCESS
  • Shard 6: SUCCESS

Failing Shards (3/6): ❌

  • Shard 1: FAILURE (persistent - DataAssetRulesDisabled)
  • Shard 2: FAILURE (NEW - BulkImport + Glossary)
  • Shard 4: FAILURE (persistent - DataContracts)

Shard 2 NEW Failures

3 failed tests:

  1. playwright/e2e/Features/BulkImport.spec.ts:583:7 - "Bulk Import Export › Database Schema"

    • Error: expect(locator).toBeVisible() failed
    • Locator: getByTestId('select-owner-tabs') not found
  2. playwright/e2e/Features/Glossary/GlossaryAdvancedOperations.spec.ts:330:7 - "Glossary Advanced Operations › should remove domain from glossary"

    • Error: expect(locator).toBeVisible() failed
    • Locator: getByTestId('glossary-container') resolved to 8 elements (strict mode violation)
  3. playwright/e2e/Features/Glossary/GlossaryP3Tests.spec.ts:585:7 - "Glossary P3 Tests › should handle slow network gracefully"

    • Error: Test timeout or network issue

Plus 9 flaky tests (Database Schema, Glossary operations)

Shard 1 Persistent Failure

Test: DataAssetRulesDisabled.spec.ts:426:9

Error: page.click: Target page, context or browser has been closed

Shard 4 Persistent Failure

Test: DataContracts.spec.ts:134:9

Error: Expected "Failed" but got "Passed"

Analysis

All 3 shards test features completely unrelated to domain filtering:

  1. Shard 1: Data asset rules, bulk edit
  2. Shard 2: Bulk import, glossary operations (Note: "should remove domain from glossary" tests glossary-domain association, NOT domain filtering logic)
  3. Shard 4: Data contracts

This PR changes:

  • Domain filtering (how assets are filtered by domain)
  • Elasticsearch query structure
  • DomainUtils.tsx utility

The failures test:

  • Bulk import/export
  • Glossary domain assignment/removal (different from filtering)
  • Data contracts
  • Data asset rules

Key Point: "Glossary domain operations" (adding/removing domains to/from glossaries) is DIFFERENT from "domain filtering" (filtering assets by selected domain). This PR only affects filtering, not domain assignment.

Root Cause

High test instability: Tests passing/failing between runs indicates:

  • Infrastructure/timing issues
  • Test isolation problems
  • Not caused by PR code changes

Evidence:

  • Shard 2 passed in previous run, now fails with 3 tests
  • 9 additional flaky tests in shard 2
  • Same 2 tests (shards 1, 4) persistently fail
  • 50% success rate (3/6 shards pass)
Code Review 👍 Approved with suggestions

Well-structured refactor moving domain filtering to query_filter approach with comprehensive tests. Two minor issues from the previous review remain unaddressed.

Suggestions 💡 2 suggestions
Edge Case: Search index exclusion still only covers TAG index

📄 openmetadata-ui/src/main/resources/ui/src/utils/DomainUtils.tsx:89-91

The domain filter exclusion logic at lines 89-91 only checks for SearchIndex.TAG. Other search indices that may not have domain associations (like DATA_PRODUCT, GLOSSARY, GLOSSARY_TERM, or classification indices) could be affected by this filter inappropriately.

Consider expanding the exclusion list:

const excludedIndices = [
  SearchIndex.TAG,
  SearchIndex.GLOSSARY_TERM,
  // Add other domain-agnostic indices as needed
];
if (excludedIndices.includes(config.params?.index)) {
  return config;
}

This was identified in the previous review and remains unaddressed.

Code Quality: Use .some() instead of .reduce() for readability

📄 openmetadata-ui/src/main/resources/ui/src/utils/DomainUtils.tsx:77-82

The shouldNotIntercept logic uses .reduce() for a boolean check, which is less readable than .some():

const shouldNotIntercept = [
  '/domain',
  '/auth/logout',
  '/auth/refresh',
].reduce((prev, curr) => {
  return prev || currentPath.startsWith(curr);
}, false);

This can be simplified to:

const shouldNotIntercept = [
  '/domain',
  '/auth/logout',
  '/auth/refresh',
].some((path) => currentPath.startsWith(path));

This was identified in the previous review and remains unaddressed.

Resolved ✅ 3 resolved
Code Quality: Debug console.log statement should be removed from E2E test

📄 openmetadata-ui/src/main/resources/ui/playwright/e2e/Features/DomainFilterQueryFilter.spec.ts:257
The E2E test file contains a debug console.log statement:\n\ntypescript\nconsole.log((subDomainTable.entityResponseData as any).domains)\n\n\nThis was identified in a previous review but remains unaddressed. Debug statements should be removed before merging to keep test output clean and maintain code quality.\n\nSuggested fix: Remove the console.log statement.

Edge Case: Prefix matching may unintentionally match unrelated domains

📄 openmetadata-ui/src/main/resources/ui/src/utils/DomainUtils.tsx:113-115
The new implementation uses Elasticsearch prefix matching on domains.fullyQualifiedName instead of exact matching. While this correctly supports subdomain filtering (parent domain matches child subdomain assets), it may also cause false positives.

Example scenario:

  • Domain A: engineering
  • Domain B: engineering-platform (separate domain, not a subdomain)

When user selects engineering, the prefix query { "prefix": { "domains.fullyQualifiedName": "engineering" } } will match both:

  • Assets in engineering (correct)
  • Assets in engineering-platform (incorrect - this is a different domain)

Previous implementation:
Used exact matching with escaped special characters: domains.fullyQualifiedName:"engineering" which would NOT match engineering-platform.

Suggested fix:
Consider using a more precise approach that handles the subdomain hierarchy delimiter. For example:

{
  bool: {
    should: [
      { term: { 'domains.fullyQualifiedName': activeDomain } },
      { prefix: { 'domains.fullyQualifiedName': activeDomain + '.' } }
    ],
    minimum_should_match: 1
  }
}

This would match the exact domain OR any subdomain (prefixed with the domain name followed by a dot separator).

Edge Case: Prefix matching may cause false positives for similar domain names

📄 openmetadata-ui/src/main/resources/ui/src/utils/DomainUtils.tsx:96-107
The prefix query on domains.fullyQualifiedName (line 96-98 in DomainUtils.tsx) is designed to support hierarchical domain filtering (e.g., "engineering" matches "engineering.backend"). However, this can cause false positives for similarly-named but unrelated domains.

Example: If you have domains "engineering" and "engineering-v2" (not a subdomain), selecting "engineering" would incorrectly also show assets from "engineering-v2" because the prefix matches.

Impact: Users may see assets from unintended domains that happen to share a name prefix.

Suggested fix: Consider using a more specific pattern that accounts for the domain separator. For example:

  • Use a bool query with should clauses for exact match OR prefix with separator: engineering OR engineering.
  • Or use a regexp/wildcard query that requires a . after the domain name for subdomain matches
filter.query.bool.must.push({
  bool: {
    should: [
      { term: { 'domains.fullyQualifiedName': activeDomain } },
      { prefix: { 'domains.fullyQualifiedName': `${activeDomain}.` } }
    ],
    minimum_should_match: 1
  }
});

What Works Well

The refactor from query string manipulation to Elasticsearch query_filter is a significant improvement - it properly handles domain hierarchy with term+prefix matching, ensuring subdomain assets are included when a parent domain is selected. The comprehensive unit tests and E2E tests provide strong coverage for the new filtering behavior.

Tip

Comment Gitar fix CI or enable auto-apply: gitar auto-apply:on

Options

Auto-apply is off Gitar will not commit updates to this branch.
✅ Code review is on Gitar will review this change.
Display: compact Hiding non-applicable rules.

Comment with these commands to change:

Auto-apply ✅ Code review Compact
gitar auto-apply:on         
gitar code-review:off         
gitar display:verbose         

Was this helpful? React with 👍 / 👎 | This comment will update automatically (Docs)

@sonarqubecloud
Copy link

sonarqubecloud bot commented Jan 1, 2026

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 UI UI specific issues

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants