Visitar URL original
fix(data): use explicit query comparison instead of lodash isEqual by poliglots · Pull Request #12562 · opensearch-project/OpenSearch-Dashboards · GitHub
Skip to content

fix(data): use explicit query comparison instead of lodash isEqual - #12562

Open
poliglots wants to merge 1 commit into
opensearch-project:mainfrom
poliglots:main
Open

poliglots wants to merge 1 commit into
opensearch-project:mainfrom
poliglots:main

Conversation

@poliglots

Copy link
Copy Markdown

Replace _.isEqual(payload.query, currentQuery) with a custom isEqualQuery helper that only compares the query and language fields.

The bug: _.isEqual treats an object with dataset: undefined as unequal to one that lacks the dataset key entirely. This caused the Discover tab's refresh button to silently stop triggering refetches after state transitions.

Fixes #12522

Co-Authored-By: Bug Analysis by @lbeltran-dev

Description

Fix the Discover tab's refresh button becoming non-functional after state transitions.

The root cause was _.isEqual(payload.query, currentQuery) returning false when payload.query had dataset: undefined
as an own enumerable property while currentQuery lacked the dataset key entirely. This is a known limitation of
deep equality checks — the presence of an undefined value is treated as structurally different from the absence of
the key.

The fix introduces a custom isEqualQuery helper that explicitly compares only the meaningful fields (query and
language), ignoring structural differences in optional properties. This ensures the refresh button correctly
detects when a query update is needed.

Issues Resolved

Fixes #12522

Screenshot

Testing the changes

  1. Unit tests: Run the new test suite for create_search_bar:

      yarn test:jest src/plugins/data/public/ui/search_bar/create_search_bar.test.ts

    All 8 tests pass, including the regression test for the dataset: undefined edge case.

  2. Full unit test suite:

      yarn test:jest

    All 3072 test suites pass (29608 tests), with no regressions from this change.

  3. Integration tests:

      yarn test:jest_integration

    Passes (the single pre-existing failure in basic_optimization.test.ts is unrelated to this PR).

  4. Manual verification:

    • Open OpenSearch Dashboards and navigate to the Discover tab.
    • Execute a search, then click the refresh button multiple times.
    • Verify that each refresh triggers a new data fetch (check the network tab or loading spinner).
    • Repeat after filtering by a time range and switching between query languages (KQL/PPL).

Check List

  • All tests pass
    • yarn test:jest
    • yarn test:jest_integration
  • New functionality includes testing.
  • New functionality has been documented.
  • Commits are signed per the DCO using --signoff

@github-actions

github-actions Bot commented Aug 10, 2026 •

Copy link
Copy Markdown
Contributor

PR Reviewer Guide 🔍

(Review updated until commit 82d92fe)

Here are some key observations to aid the review process:

🧪 PR contains tests
🔒 No security concerns identified
✅ No TODO sections
🔀 No multiple PR themes
⚡ Recommended focus areas for review

Behavior Change

The new isEqualQuery only compares query and language, ignoring other Query fields such as dataset. If a user changes only the dataset (keeping query text and language the same), isUpdate will now be false and the query state won't be updated via queryService.queryString.setQuery. This may cause dataset-only changes to be silently dropped on submit. Consider including dataset (compared in a way that treats missing key and undefined as equal) in the comparison.

export const isEqualQuery = (a: Query | undefined, b: Query | undefined): boolean => {
  if (a === b) return true;
  if (!a || !b) return false;
  return a.query === b.query && a.language === b.language;
};

@github-actions

github-actions Bot commented Aug 10, 2026 •

Copy link
Copy Markdown
Contributor

PR Code Suggestions ✨

Latest suggestions up to 82d92fe

Explore these optional code suggestions:

CategorySuggestion                                                                                                                                    Impact
Possible issue
Deep compare query field to handle objects

The query field on Query can be an object (e.g., DSL query), not just a string, so
using === for structural comparison will produce false negatives and incorrectly
flag updates on every submit. Use a deep equality check for the query field while
keeping strict equality for language.

src/plugins/data/public/ui/search_bar/create_search_bar.tsx [88-92]

 export const isEqualQuery = (a: Query | undefined, b: Query | undefined): boolean => {
   if (a === b) return true;
   if (!a || !b) return false;
-  return a.query === b.query && a.language === b.language;
+  return a.language === b.language && _.isEqual(a.query, b.query);
 };
Suggestion importance[1-10]: 8

__

Why: The Query.query field can be a string or object (DSL), so === would fail for object queries and cause spurious updates. Using _.isEqual for the query field is a meaningful correctness improvement.

Medium

Previous suggestions

Suggestions up to commit 3aa8625
CategorySuggestion                                                                                                                                    Impact
General
Avoid conflating null with undefined

The comment above normalizedEqual claims that "real values and explicit undefined
are still distinguished", but by coalescing both undefined values to null,
normalizedEqual(undefined, undefined) returns true — which contradicts the comment
and the test case "should treat dataset: undefined as different from a defined
dataset" only passes because one side is defined. More importantly, this means null
and undefined are now conflated: normalizedEqual(null, undefined) returns true,
which may mask real differences if any Query field legitimately uses null. Consider
using a unique sentinel or checking a === undefined on both sides explicitly.

src/plugins/data/public/ui/search_bar/create_search_bar.tsx [106-110]

 const normalizedEqual = (a: unknown, b: unknown): boolean => {
-  const aVal = a === undefined ? null : a;
-  const bVal = b === undefined ? null : b;
-  return _.isEqual(aVal, bVal);
+  if (a === undefined && b === undefined) return true;
+  if (a === undefined || b === undefined) return false;
+  return _.isEqual(a, b);
 };
Suggestion importance[1-10]: 4

__

Why: The suggestion correctly identifies that normalizedEqual conflates null and undefined, which is a minor correctness concern. However, since Query fields like dataset and profile are unlikely to legitimately hold null, the practical impact is limited.

Low
Make query comparison future-proof for new fields

The Query type may have additional fields beyond query, language, dataset, and
profile; comparing only these four fields could miss legitimate updates for any
other fields. Consider iterating over the union of keys from both objects (excluding
known-normalized fields) to make the comparison future-proof, rather than relying on
the reminder comment to manually update this function.

src/plugins/data/public/ui/search_bar/create_search_bar.tsx [92-101]

 export const isEqualQuery = (a: Query | undefined, b: Query | undefined): boolean => {
   if (a === b) return true;
   if (!a || !b) return false;
-  return (
-    _.isEqual(a.query, b.query) &&
-    a.language === b.language &&
-    normalizedEqual(a.dataset, b.dataset) &&
-    normalizedEqual(a.profile, b.profile)
-  );
+  if (!_.isEqual(a.query, b.query)) return false;
+  if (a.language !== b.language) return false;
+  const normalizedKeys = new Set(['query', 'language']);
+  const allKeys = new Set([...Object.keys(a), ...Object.keys(b)]);
+  for (const key of allKeys) {
+    if (normalizedKeys.has(key)) continue;
+    if (!normalizedEqual((a as any)[key], (b as any)[key])) return false;
+  }
+  return true;
 };
Suggestion importance[1-10]: 3

__

Why: Making the comparison future-proof is a reasonable maintainability improvement, but the explicit field approach with a comment is already acceptable and the dynamic-key approach loses type safety and clarity.

Low
Suggestions up to commit 0f070fe
CategorySuggestion                                                                                                                                    Impact
Possible issue
Deep-compare dataset objects, not by reference

Comparing dataset with === only works for primitive dataset identifiers; if dataset
is an object (as Query.dataset typically is, e.g., { id, title, type }), reference
equality will almost always return false and incorrectly mark equal datasets as
different. Use a deep comparison for dataset instead.

src/plugins/data/public/ui/search_bar/create_search_bar.tsx [96]

 export const isEqualQuery = (a: Query | undefined, b: Query | undefined): boolean => {
   if (a === b) return true;
   if (!a || !b) return false;
   return (
     _.isEqual(a.query, b.query) &&
     a.language === b.language &&
-    (a.dataset ?? null) === (b.dataset ?? null) &&
+    _.isEqual(a.dataset ?? null, b.dataset ?? null) &&
     (a.profile ?? false) === (b.profile ?? false)
   );
 };
Suggestion importance[1-10]: 8

__

Why: In OpenSearch Dashboards, Query.dataset is typically an object (e.g., { id, title, type }), so using === would cause reference-inequality false negatives. Switching to _.isEqual for the dataset field is a meaningful correctness improvement.

Medium
Suggestions up to commit 82d92fe
CategorySuggestion                                                                                                                                    Impact
Possible issue
Deep-compare non-primitive query bodies

The query field of a Query object can be an object (e.g., DSL query) rather than a
primitive string, in which case a.query === b.query will incorrectly return false
for structurally-equal objects. Use a deep equality check for the query field to
handle both string and object query bodies correctly.

src/plugins/data/public/ui/search_bar/create_search_bar.tsx [88-92]

 export const isEqualQuery = (a: Query | undefined, b: Query | undefined): boolean => {
   if (a === b) return true;
   if (!a || !b) return false;
-  return a.query === b.query && a.language === b.language;
+  return _.isEqual(a.query, b.query) && a.language === b.language;
 };
Suggestion importance[1-10]: 7

__

Why: The Query.query field type in OpenSearch Dashboards can be a string or an object (DSL), so using strict === may miss structurally equal objects. Using _.isEqual for the query field is a valid correctness improvement, though the practical impact depends on how the search bar is used.

Medium
Suggestions up to commit 2055775
CategorySuggestion                                                                                                                                    Impact
Possible issue
Test the actual implementation, not a copy

The test imports isEqualQuery but never uses it, and instead re-implements the logic
inline as isEqualQueryImpl. This means the tests do not actually verify the
production code — any future regression in create_search_bar.tsx would not be
caught. Either export isEqualQuery from the source module and test it directly, or
remove the unused import and the misleading comment.

src/plugins/data/public/ui/search_bar/create_search_bar.test.ts [46-59]

 import { isEqualQuery } from './create_search_bar';
 
-// Note: isEqualQuery is not exported by default, so we need to test
-// the behavior through the module. However, since it's a module-level
-// const, we can test the logic inline here to match the implementation.
+// Test the actual exported implementation to catch regressions.
+const isEqualQueryImpl = isEqualQuery;
 
-const isEqualQueryImpl = (
-  a: { query: string; language: string } | undefined,
-  b: { query: string; language: string } | undefined
-): boolean => {
-  if (a === b) return true;
-  if (!a || !b) return false;
-  return a.query === b.query && a.language === b.language;
-};
-
Suggestion importance[1-10]: 8

__

Why: The suggestion correctly identifies a significant issue: the test imports isEqualQuery but tests a local re-implementation instead, meaning regressions in the production code would not be caught. This undermines the test's purpose.

Medium

Replace _.isEqual(payload.query, currentQuery) with a custom isEqualQuery
helper that only compares the query and language fields.

The bug: _.isEqual treats an object with dataset: undefined as unequal
to one that lacks the dataset key entirely. This caused the Discover tab's
refresh button to silently stop triggering refetches after state transitions.

Fixes opensearch-project#12522

Co-Authored-By: Bug Analysis by @lbeltran-dev
Signed-off-by: Shatrughan Rai <polyglot.dev@outlook.com>
@github-actions

Copy link
Copy Markdown
Contributor

Persistent review updated to latest commit 82d92fe

poliglots added a commit to poliglots/OpenSearch-Dashboards that referenced this pull request Aug 21, 2026
…lQuery

The previous isEqualQuery used === for the query field, which fails
when query is a DSL object (two structurally-equal objects are always
different). It also ignored dataset and profile fields entirely, meaning
changing the dataset while keeping the same query text would not trigger
a refresh.

- Use _.isEqual() for the query field to handle both string and DSL
  object bodies correctly
- Add normalization for dataset (?? null) so undefined and missing
  keys are equivalent, but real value differences are still detected
- Add normalization for profile (?? false) with the same behavior

Addresses PR review feedback for opensearch-project#12562

Signed-off-by: Shatrughan Rai <polyglot.dev@outlook.com>
@github-actions

Copy link
Copy Markdown
Contributor

Persistent review updated to latest commit 0f070fe

poliglots added a commit to poliglots/OpenSearch-Dashboards that referenced this pull request Aug 21, 2026
…lQuery

The previous isEqualQuery used === for the query field, which fails
when query is a DSL object (two structurally-equal objects are always
different). It also ignored dataset and profile fields entirely, meaning
changing the dataset while keeping the same query text would not trigger
a refresh.

- Use _.isEqual() for the query field to handle both string and DSL
  object bodies correctly
- Add normalization for dataset (?? null) so undefined and missing
  keys are equivalent, but real value differences are still detected
- Add normalization for profile (?? false) with the same behavior

Addresses PR review feedback for opensearch-project#12562

Signed-off-by: Shatrughan Rai <polyglot.dev@outlook.com>
@github-actions

Copy link
Copy Markdown
Contributor

Persistent review updated to latest commit 3aa8625

@github-actions

Copy link
Copy Markdown
Contributor

Persistent review updated to latest commit 82d92fe

@github-actions

github-actions Bot commented Sep 9, 2026 •

Copy link
Copy Markdown
Contributor

🔗 Workflow run · commit 3aa8625e3528e97347adadda92b6ef64d6fd159f

This branch has not been deployed

No deployments
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Projects

None yet

Development

Successfully merging this pull request may close these issues.

[BUG] Refresh button in Discover tab intermittently stops triggering data refetch

1 participant