Skip to content

Conversation

@aniketkatkar97
Copy link
Member

@aniketkatkar97 aniketkatkar97 commented Jan 12, 2026

Fixes #17001
Fixes #21600
Fixes #22939
Fixes https://github.com/open-metadata/openmetadata-collate/issues/2539

This pull request introduces several improvements and refactors to the advanced search functionality, operator constants, and internationalization files. The main highlights are the refactoring of custom property handling in advanced search, the addition of new search operators, and the expansion of translation keys in multiple languages.

Advanced Search Refactoring and Improvements

  • Refactored the logic for handling custom properties in advanced search by replacing inline logic with a reusable processEntityTypeFields utility function, simplifying the code and improving maintainability in AdvanceSearchProvider.component.tsx. [1] [2] [3]
  • Updated the SQL query formatting logic to clarify value assignment and improve readability in AdvanceSearchProvider.component.tsx.

Operator Constants Enhancements

  • Added new operators (is_null, is_not_null) to MULTISELECT_FIELD_OPERATORS and NUMBER_FIELD_OPERATORS, and defined NUMBER_FIELD_OPERATORS to better support null checks and range queries in AdvancedSearch.constants.ts.

Type and Interface Updates

  • Cleaned up and clarified type definitions related to aggregation buckets by removing the AggregationEntry and Bucket interface from search.interface.ts, and importing Bucket from the Models module instead. [1] [2]

Internationalization (i18n) Updates

  • Added new translation keys for "start" and "end" (and their usage in date/time contexts) in English, Arabic, German, Spanish, French, and Galician language files. [1] [2] [3] [4] [5] [6] [7] [8] [9] [10] [11] [12] [13] [14] [15]
  • Fixed untranslated warning messages by ensuring the domain asset migration warning is now localized in all supported languages. [1] [2] [3] [4] [5]

@github-actions
Copy link
Contributor

github-actions bot commented Jan 12, 2026

Jest test Coverage

UI tests summary

Lines Statements Branches Functions
Coverage: 65%
66.01% (54676/82836) 44.29% (27711/62569) 47.77% (8585/17972)

Comment on lines 414 to +424
}

if (value && Array.isArray(value[0])) {
// Check if this is a multiselect equals operator that should use AND logic
const useAndLogic =
operator === 'multiselect_equals' ||
operator === 'multiselect_not_equals';

return {
bool: {
should: value[0].map((val) =>
[useAndLogic ? 'must' : 'should']: value[0].map((val) =>
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

⚠️ Bug: Behavioral change: multiselect_equals now uses AND instead of OR

Details

The change from should to must for multiselect_equals and multiselect_not_equals operators fundamentally changes the search behavior:

  • Before: When selecting multiple values (e.g., tags A and B), items matching A OR B would be returned
  • After: When selecting multiple values, only items matching A AND B will be returned

This is a breaking behavioral change that may significantly impact user expectations for multiselect filters. While this might be the intended fix for one of the referenced issues, it's important to verify:

  1. This matches the expected behavior from the issue requirements
  2. Existing users/documentation reflect this change
  3. Consider if this should be configurable based on use case

The same change appears in both elasticSearchFormat and elasticSearchFormatForJSONLogic functions, which is consistent.


Was this helpful? React with 👍 / 👎

@gitar-bot
Copy link

gitar-bot bot commented Jan 22, 2026

The validate-json-yaml CI failure is unrelated to this PR's changes.

Details

The failure is caused by openmetadata-ui/src/main/resources/ui/playwright/test-data/odcs-examples/invalid-malformed.json, which is an intentionally malformed test fixture file that was added in commit c235910 (merged January 20, 2026) as part of PR #25132.

The validation script at scripts/validate_json_yaml.sh:8 needs to be updated to exclude the odcs-examples test directory from validation, similar to how .vscode and great_expectations/resources are already excluded.

This is a pre-existing issue in the main branch and affects all PRs currently, not just this one.

@gitar-bot
Copy link

gitar-bot bot commented Jan 22, 2026

🔍 CI failure analysis for 84fb1fd: CI failures show 37 tests timing out on Time Interval custom properties UI interactions, along with 17 flaky tests for other custom property types (Email, Timestamp, Entity Reference, Date, DateTime) and domain/entity operations.

Issue 1: Custom Property Advanced Search Test Failure (Related to PR)

The playwright test CustomProperty Dashboard Filter is failing because the PR changes how string custom properties are formatted in advanced search queries.

Root Cause

The PR modified AdvancedSearchClassBase.ts to add a .keyword suffix to custom property field names for Elasticsearch queries. In the getCustomPropertiesSubFields method (lines 1233-1256 of the diff), the code now adds .keyword suffix for string types:

let subfieldsKey: string;
if (
  field.type === 'array<entityReference>' ||
  field.type === 'entityReference'
) {
  subfieldsKey = field.name + '.displayName.keyword';
} else if (
  field.type === 'number' ||
  field.type === 'integer' ||
  field.type === 'timestamp'
) {
  subfieldsKey = field.name;
} else {
  subfieldsKey = field.name + '.keyword';  // String types get .keyword suffix
}

The test at playwright/e2e/Flow/AdvanceSearchFilter/CustomPropertyAdvanceSeach.spec.ts:180-182 creates a String type custom property and expects the filter text to display:

extension.dashboard.pwCustomPropertyDashboardTest04ed6f68 = 'dashboardcustomproperty_12d6bc70'

But the actual output now includes the .keyword suffix:

extension.dashboard.pwCustomPropertyDashboardTest04ed6f68.keyword = 'dashboardcustomproperty_12d6bc70'

Details

This failure is directly related to the PR changes. The PR refactored custom property handling in advanced search and introduced logic to append .keyword to string custom properties for proper Elasticsearch field matching. This is a correct change for the query execution, but the SQL-like display format shown to users now also includes .keyword, which breaks the test expectation.

The solution is to update the test expectation to include the .keyword suffix, or alternatively, modify the SQL display formatter to strip the .keyword suffix when presenting the query to users (while keeping it in the actual Elasticsearch query).


Issue 2: Time Interval Custom Properties Tests - Widespread Timeout Failures (Likely Related to PR)

37 tests failed with timeout errors (540000ms exceeded) when attempting to add Time Interval custom properties across 18 entity types (container, dashboard, database, databaseSchema, glossaryTerm, mlmodel, pipeline, searchIndex, storedProcedure, table, topic, apiCollection, apiEndpoint, dataProduct, domain, dashboardDataModel, metric, chart).

Root Cause

The PR introduces significant refactoring to custom property handling in advanced search:

  • Extracted inline custom property processing logic into a new processEntityTypeFields utility function
  • Modified AdvancedSearchClassBase.ts to handle custom property subfields differently
  • Added new operators (is_null, is_not_null) to various field operators
  • Changed the way custom properties are structured in the advanced search query builder

All 37 failures show the same pattern:

Error: locator.waitFor: Test timeout of 540000ms exceeded.

This suggests the Time Interval custom property UI component is either:

  1. Not loading at all (broken by the refactoring)
  2. Stuck in an infinite loading state
  3. Unable to find required fields due to the structural changes in how custom properties are processed

Details

This failure pattern is likely related to the PR changes. The refactoring moves custom property handling logic from AdvanceSearchProvider.component.tsx into a separate processEntityTypeFields utility function in AdvancedSearchUtils.ts. If this refactoring introduced a bug specific to Time Interval type custom properties (e.g., missing field mappings, incorrect type handling, or broken UI component rendering), it could explain why:

  • All Time Interval tests fail consistently (37 failures)
  • Other custom property types show flaky behavior (17 flaky tests)
  • The failures occur during UI interaction (locator.waitFor timeout)

The solution should be to investigate the processEntityTypeFields function and verify that Time Interval custom properties are correctly handled in the refactored code, particularly checking:

  • Field type mapping for 'timeInterval' type
  • UI widget selection for Time Interval properties
  • Data structure compatibility with the query builder

Issue 3: JSON/YAML Validation Failure (Unrelated to PR)

The validate-json-yaml CI job failed while checking openmetadata-ui/src/main/resources/ui/playwright/test-data/odcs-examples/invalid-malformed.json.

Root Cause

The validation script scripts/validate_json_yaml.sh validates all JSON files in the repository but does not exclude test fixtures that are intentionally malformed. The file invalid-malformed.json is a legitimate test fixture with intentional invalid JSON syntax (missing comma on line 4) used for negative testing of ODCS import functionality.

The file is documented in the test data README as:

  • Description: Invalid JSON syntax
  • Expected Error: JSON parse error

Details

This failure is unrelated to the PR changes. The PR modifies advanced search functionality, utilities, and translations but does not touch invalid-malformed.json. This test file has existed in the repository and serves as a test fixture to verify the application correctly handles malformed JSON during ODCS contract imports.

The validation script currently excludes only .vscode and great_expectations/resources directories (line 6 of scripts/validate_json_yaml.sh):

EXCLUDED_DIRS=".vscode|great_expectations/resources"

The solution is to expand the exclusion pattern to include test data directories:

EXCLUDED_DIRS=".vscode|great_expectations/resources|playwright/test-data"

This will allow intentionally invalid test fixtures to remain in the repository for testing purposes while still validating production JSON/YAML files.


Issue 4: Flaky Tests - Custom Properties and Entity Operations (Possibly Related to PR)

17 flaky tests failed intermittently across various custom property types and entity operations:

  • Custom property types: Email, Timestamp, Entity Reference, Date, DateTime (all for tableColumn)
  • Entity operations: Domain rename, data contracts, tag/glossary preservation in column panels

Root Cause

These tests failed with various timeout and element visibility issues:

  • Error: page.click: Target page, context or browser has been closed
  • Error: page.waitForResponse: Test timeout of 60000ms exceeded
  • Error: expect(locator).toBeVisible() failed
  • Error: expect(locator).toContainText(expected) failed
  • Error: expect(locator).not.toHaveText(expected) failed

Details

These failures are possibly related to the PR changes. While the error patterns suggest test flakiness, the fact that:

  1. Most flaky tests involve custom properties (5 out of 17)
  2. The PR significantly refactored custom property handling
  3. The tests are passing in some retries but failing in others

suggests that the refactoring may have introduced timing issues or race conditions in how custom properties are loaded and displayed in the UI. The custom property processing now happens through a different code path (processEntityTypeFields), which may have different timing characteristics.

The remaining flaky tests (domain rename, data contracts, entity operations) appear to be environmental issues in the CI run, as they are unrelated to advanced search or custom property functionality modified by this PR.

Code Review 👍 Approved with suggestions 1 resolved / 3 findings

Comprehensive advanced search improvements for custom properties with good test coverage. The behavioral change from OR to AND logic for multiselect_equals is intentional but should be verified with product requirements.

⚠️ Bug: Behavioral change: multiselect_equals now uses AND instead of OR

📄 openmetadata-ui/src/main/resources/ui/src/utils/QueryBuilderElasticsearchFormatUtils.js:414-424

The change from should to must for multiselect_equals and multiselect_not_equals operators fundamentally changes the search behavior:

  • Before: When selecting multiple values (e.g., tags A and B), items matching A OR B would be returned
  • After: When selecting multiple values, only items matching A AND B will be returned

This is a breaking behavioral change that may significantly impact user expectations for multiselect filters. While this might be the intended fix for one of the referenced issues, it's important to verify:

  1. This matches the expected behavior from the issue requirements
  2. Existing users/documentation reflect this change
  3. Consider if this should be configurable based on use case

The same change appears in both elasticSearchFormat and elasticSearchFormatForJSONLogic functions, which is consistent.

More details 💡 1 suggestion ✅ 1 resolved
💡 Bug: entityReference displayName may not always have .keyword field

📄 openmetadata-ui/src/main/resources/ui/src/utils/AdvancedSearchClassBase.ts:1239-1244

The change from field.name + '.displayName' to field.name + '.displayName.keyword' assumes the Elasticsearch mapping always has a .keyword subfield for displayName.

While this is likely correct for most configurations, if the ES mapping doesn't include a keyword subfield for displayName (e.g., if it's mapped as keyword directly or uses a different analyzer setup), this could cause search failures.

Consider:

  1. Verifying the ES mapping schema supports .displayName.keyword for all entityReference custom properties
  2. Adding a fallback or documentation note about required mappings
Quality: Missing test for early return when getCustomPropertiesSubFields returns empty

📄 openmetadata-ui/src/main/resources/ui/src/utils/AdvancedSearchUtils.tsx:520-523
The processCustomPropertyField function processes results from getCustomPropertiesSubFields but doesn't explicitly handle the case when the result might be empty or undefined (beyond what the forEach handles).

While getCustomPropertiesSubFields is designed to always return valid data, adding defensive handling for edge cases would improve robustness:

const result = advancedSearchClassBase.getCustomPropertiesSubFields(field);
if (!result || (Array.isArray(result) && result.length === 0)) {
  return;
}

The tests cover various scenarios but could benefit from a test case where getCustomPropertiesSubFields returns an empty array.

What Works Well

Excellent refactoring of custom property handling with reusable processEntityTypeFields utility. Comprehensive test coverage added for new functionality including edge cases. Type safety improvements throughout with proper ESQueryClause types replacing any.

Recommendations

Verify with product/users that the multiselect_equals behavior change (from OR to AND logic) matches expected search semantics - this could be breaking for existing saved searches expecting OR 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.
Display: compact → Showing less information.

Comment with these commands to change:

Auto-apply Compact
gitar auto-apply:on         
gitar display:verbose         

Was this helpful? React with 👍 / 👎 | Gitar

@sonarqubecloud
Copy link

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 To release Will cherry-pick this PR into the release branch UI UI specific issues

Projects

None yet

2 participants