-
Notifications
You must be signed in to change notification settings - Fork 1.6k
add prefix matching in domain #25002
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
base: main
Are you sure you want to change the base?
Conversation
|
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! |
openmetadata-ui/src/main/resources/ui/src/components/Auth/AuthProviders/AuthProvider.tsx
Outdated
Show resolved
Hide resolved
openmetadata-ui/src/main/resources/ui/src/components/Auth/AuthProviders/AuthProvider.tsx
Outdated
Show resolved
Hide resolved
6876654 to
58833bf
Compare
openmetadata-ui/src/main/resources/ui/playwright/e2e/Features/DomainFilterQueryFilter.spec.ts
Show resolved
Hide resolved
🔍 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.IssueCI Regression: Shard 2 now fails with 3 new test failures. Total: 3 shards failing (1, 2, 4). Current StatusPassing Shards (3/6): ✅
Failing Shards (3/6): ❌
Shard 2 NEW Failures3 failed tests:
Plus 9 flaky tests (Database Schema, Glossary operations) Shard 1 Persistent FailureTest: Error: Shard 4 Persistent FailureTest: Error: Expected "Failed" but got "Passed" AnalysisAll 3 shards test features completely unrelated to domain filtering:
This PR changes:
The failures test:
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 CauseHigh test instability: Tests passing/failing between runs indicates:
Evidence:
Code Review 👍 Approved with suggestionsWell-structured refactor moving domain filtering to query_filter approach with comprehensive tests. Two minor issues from the previous review remain unaddressed. Suggestions 💡 2 suggestionsEdge 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 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 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 resolvedCode Quality: Debug console.log statement should be removed from E2E test
Edge Case: Prefix matching may unintentionally match unrelated domains
Edge Case: Prefix matching may cause false positives for similar domain names
What Works WellThe 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 OptionsAuto-apply is off Gitar will not commit updates to this branch. Comment with these commands to change:
Was this helpful? React with 👍 / 👎 | This comment will update automatically (Docs) |
|



Describe your changes:
Fixes
I worked on ... because ...
Summary by Gitar
domains.fullyQualifiedNamefor hierarchical domain support (parent domains show child assets)withDomainFilterlogic intoDomainUtils.tsxutility module with comprehensive unit tests (DomainUtils.test.tsx- 585 lines)query_filterparameter usingprefixquery syntaxDomainFilterQueryFilter.spec.tswith 9 scenarios including 3-level domain hierarchy and filter persistence validationThis will update automatically on new commits.
Type of change:
Checklist:
Fixes <issue-number>: <short explanation>