Skip to content

Fixes #27482: Advanced Search between operator ignores range bounds for number custom properties#27525

Open
mohitjeswani01 wants to merge 8 commits intoopen-metadata:mainfrom
mohitjeswani01:fix-adv-search-between-27482
Open

Fixes #27482: Advanced Search between operator ignores range bounds for number custom properties#27525
mohitjeswani01 wants to merge 8 commits intoopen-metadata:mainfrom
mohitjeswani01:fix-adv-search-between-27482

Conversation

@mohitjeswani01
Copy link
Copy Markdown

@mohitjeswani01 mohitjeswani01 commented Apr 19, 2026

Description:

Fixes #27482

What changes did you make?
In QueryBuilderElasticsearchFormatUtils.js, the buildEsRule function passed only value[0] (the lower bound) to buildExtensionQuery for all extension field operators. For the between operator, RAQB stores the two inputs as a flat array [from, to], so value[1] (the upper bound) was silently dropped before reaching the Elasticsearch range query builder.

Fixed by introducing an isBetweenOp check that passes the full value array for between/not_between operators and value[0] for all others — no change to existing operator behaviour.

Why did you make them?
The filter was ignoring the specified range and returning all entities that had the custom property set regardless of value. For example, filtering between 1 and 5 returned an entity with value 100. A secondary effect was that subsequent Apply clicks did not re-trigger the search API, because the broken query was structurally identical every time and was short-circuited by the isEqual guard in loadTree.

How did you test your changes?
Added a new unit test file QueryBuilderElasticsearchFormatUtils.test.js with two tests that drive elasticSearchFormat end-to-end using a minimal RAQB tree stub and AntdConfig:

  • between [5, 20] → asserts gte: 5 and lte: 20 both present in the nested range query
  • not_between [10, 50] → asserts the range is wrapped in a must_not clause with correct bounds

Both tests pass. ✅

Screenshot 2026-04-19 162820

Type of change:

  • Bug fix

Checklist:

  • I have read the CONTRIBUTING document.
  • My PR title is Fixes #27482: Advanced Search between operator ignores range bounds for number custom properties
  • 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.
  • I have added a test that covers the exact scenario we are fixing. For complex issues, comment the issue number in the test for future reference.

Summary by Gitar

  • Test updates:
    • Cleaned up E2E setup logic in AdvanceSearchCustomProperty.spec.ts to improve test reliability and maintainability.

This will update automatically on new commits.

Copilot AI review requested due to automatic review settings April 19, 2026 15:27
@mohitjeswani01 mohitjeswani01 requested a review from a team as a code owner April 19, 2026 15:27
@github-actions
Copy link
Copy Markdown
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
Copy Markdown
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!

Copy link
Copy Markdown
Contributor

Copilot AI left a comment

Choose a reason for hiding this comment

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

Pull request overview

Fixes Advanced Search Elasticsearch query generation for extension (custom property) numeric range filters by ensuring between / not_between operators pass both bounds through to the nested range query builder, preventing silently dropped upper bounds and stale-query short-circuiting.

Changes:

  • Update buildEsRule to pass the full [from, to] array to buildExtensionQuery for between / not_between operators (retain prior scalar behavior for all other operators).
  • Add Jest coverage validating that both gte and lte are present for between, and that not_between is properly wrapped in a must_not clause.

Reviewed changes

Copilot reviewed 2 out of 2 changed files in this pull request and generated no comments.

File Description
openmetadata-ui/src/main/resources/ui/src/utils/QueryBuilderElasticsearchFormatUtils.js Fixes extension-field range handling by preserving both bounds for between / not_between.
openmetadata-ui/src/main/resources/ui/src/utils/QueryBuilderElasticsearchFormatUtils.test.js Adds regression tests for extension numeric between and not_between query generation.

@mohitjeswani01
Copy link
Copy Markdown
Author

hi @harshach sir could you please add a safe to test label ? thanks 🙏

@harshach harshach added the safe to test Add this label to run secure Github workflows on PRs label Apr 19, 2026
@harshach
Copy link
Copy Markdown
Collaborator

@mohitjeswani01 this requires playwright tests as well

@mohitjeswani01
Copy link
Copy Markdown
Author

@mohitjeswani01 this requires playwright tests as well

Thanks for the review @harshach sir , i will fix the checkstyle and also add playwrite test for the between UI flow. Will push the update shortly.

@github-actions
Copy link
Copy Markdown
Contributor

github-actions bot commented Apr 19, 2026

Jest test Coverage

UI tests summary

Lines Statements Branches Functions
Coverage: 63%
63.79% (59796/93727) 43.76% (31542/72076) 46.77% (9457/20216)

Copilot AI review requested due to automatic review settings April 19, 2026 17:48
Copilot AI review requested due to automatic review settings April 19, 2026 18:23
Copy link
Copy Markdown
Contributor

Copilot AI left a comment

Choose a reason for hiding this comment

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

Pull request overview

Copilot reviewed 3 out of 3 changed files in this pull request and generated 1 comment.

Copilot AI review requested due to automatic review settings April 19, 2026 18:34
@gitar-bot
Copy link
Copy Markdown

gitar-bot bot commented Apr 19, 2026

Code Review ✅ Approved

Advanced search logic now correctly respects range bounds for number-based custom properties, fixing the off-by-one error during filtering. No issues found.

Options

Display: compact → Showing less information.

Comment with these commands to change:

Compact
gitar display:verbose         

Was this helpful? React with 👍 / 👎 | Gitar

@mohitjeswani01
Copy link
Copy Markdown
Author

mohitjeswani01 commented Apr 19, 2026

@harshach The logic is fully locked in, E2E tests are complete, and I've resolved the Copilot review suggestions.

However, I am hitting a cross-platform loop with the lint-playwright CI job. The organize-imports-cli tool is sorting the imports slightly differently on my Windows/WSL local environment compared to the GitHub Actions Linux runner. Every time I run the auto-fixers locally and push, the Linux runner reorganizes them and intentionally fails the checkstyle.

Since the actual E2E logic is sound, could you please 🙏 run make ui-checkstyle-changed on your end to lock in the Linux import order and bypass the check ? as i am on a 8gb ram machine😅, Let me know if you need me to adjust any actual test logic!

Copy link
Copy Markdown
Contributor

Copilot AI left a comment

Choose a reason for hiding this comment

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

Pull request overview

Copilot reviewed 3 out of 3 changed files in this pull request and generated no new comments.

@github-actions
Copy link
Copy Markdown
Contributor

❌ UI Checkstyle Failed

❌ Playwright — ESLint + Prettier + Organise Imports

One or more Playwright test files have linting or formatting issues.

Affected files
  • openmetadata-ui/src/main/resources/ui/playwright/e2e/Features/AdvanceSearchCustomProperty.spec.ts

Fix locally (fast — only checks files changed in this branch):

make ui-checkstyle-changed

@sonarqubecloud
Copy link
Copy Markdown

@github-actions
Copy link
Copy Markdown
Contributor

🔴 Playwright Results — 3 failure(s), 21 flaky

✅ 3664 passed · ❌ 3 failed · 🟡 21 flaky · ⏭️ 89 skipped

Shard Passed Failed Flaky Skipped
🟡 Shard 1 480 0 1 4
🟡 Shard 2 650 0 2 7
🔴 Shard 3 655 3 3 1
🟡 Shard 4 626 0 8 27
✅ Shard 5 611 0 0 42
🟡 Shard 6 642 0 7 8

Genuine Failures (failed on all attempts)

Features/RestoreEntityInheritedFields.spec.ts › Validate restore with Inherited domain and data products assigned (shard 3)
Error: Waiting for data product "PW Data Product bdf5c474" to appear after save

�[2mexpect(�[22m�[31mreceived�[39m�[2m).�[22mtoBe�[2m(�[22m�[32mexpected�[39m�[2m) // Object.is equality�[22m

Expected: �[32mtrue�[39m
Received: �[31mfalse�[39m

Call Log:
- Timeout 60000ms exceeded while waiting on the predicate
Flow/AdvanceSearchFilter/CustomPropertyAdvanceSeach.spec.ts › DateTime CP with all operators (shard 3)
Error: �[2mexpect(�[22m�[31mlocator�[39m�[2m).�[22mtoBeVisible�[2m(�[22m�[2m)�[22m failed

Locator: getByTestId('table-data-card_pw-dashboard-service-20130358.pw-dashboard-35b7cd61')
Expected: visible
Timeout: 15000ms
Error: element(s) not found

Call log:
�[2m  - Expect "toBeVisible" with timeout 15000ms�[22m
�[2m  - waiting for getByTestId('table-data-card_pw-dashboard-service-20130358.pw-dashboard-35b7cd61')�[22m

Flow/AdvanceSearchFilter/CustomPropertyAdvanceSeach.spec.ts › Date CP with all operators (shard 3)
Error: �[2mexpect(�[22m�[31mlocator�[39m�[2m).�[22mtoBeVisible�[2m(�[22m�[2m)�[22m failed

Locator: getByTestId('table-data-card_pw-dashboard-service-e29261be.pw-dashboard-0e8c0309')
Expected: visible
Timeout: 15000ms
Error: element(s) not found

Call log:
�[2m  - Expect "toBeVisible" with timeout 15000ms�[22m
�[2m  - waiting for getByTestId('table-data-card_pw-dashboard-service-e29261be.pw-dashboard-0e8c0309')�[22m

🟡 21 flaky test(s) (passed on 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/Glossary/GlossaryWorkflow.spec.ts › should inherit reviewers from glossary when term is created (shard 2, 1 retry)
  • Features/RestoreEntityInheritedFields.spec.ts › Validate restore with Inherited domain and data products assigned (shard 3, 1 retry)
  • Features/RTL.spec.ts › Verify Following widget functionality (shard 3, 1 retry)
  • Flow/NotificationAlerts.spec.ts › Multiple Filters Alert (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 Table (shard 4, 2 retries)
  • Pages/DataContracts.spec.ts › Create Data Contract and validate for Api Collection (shard 4, 1 retry)
  • Pages/DataContracts.spec.ts › Create Data Contract and validate for Database (shard 4, 1 retry)
  • Pages/DataContractsSemanticRules.spec.ts › Validate Description Rule Contains (shard 4, 1 retry)
  • Pages/DataContractsSemanticRules.spec.ts › Validate Description Rule Is_Not_Set (shard 4, 1 retry)
  • Pages/Domains.spec.ts › Data Product announcement create, edit & delete (shard 4, 1 retry)
  • Pages/Entity.spec.ts › Inactive Announcement create & delete (shard 4, 1 retry)
  • Pages/HyperlinkCustomProperty.spec.ts › should accept valid http and https URLs (shard 6, 1 retry)
  • Pages/Lineage/LineageFilters.spec.ts › Verify lineage schema filter selection (shard 6, 1 retry)
  • Pages/Login.spec.ts › Refresh should work (shard 6, 1 retry)
  • Pages/ODCSImportExport.spec.ts › Multi-object ODCS contract - object selector shows all schema objects (shard 6, 1 retry)
  • Pages/Tag.spec.ts › Verify Owner Add Delete (shard 6, 1 retry)
  • Pages/UserDetails.spec.ts › Admin user can edit teams from the user profile (shard 6, 1 retry)
  • Pages/Users.spec.ts › Permissions for table details page for Data Consumer (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

@mohitjeswani01
Copy link
Copy Markdown
Author

@harshach Just a heads-up, the Postgresql PR Playwright E2E Tests job had a few failures (CustomPropertyAdvanceSeach.spec.ts, RestoreEntityInheritedFields.spec.ts, and RTL.spec.ts).

Looking at the traces, these appear to be unrelated CI flakes (async indexing timeouts and a test-data collision where a user email already exists), completely isolated from the QueryBuilderElasticsearchFormatUtils changes in this PR.

Could you re-trigger the failed Playwright jobs when you have a moment, or let me know if you'd prefer me to investigate them anyway?

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

3 participants