Fix template queries loading and update getSampleQuery interface #8848

jowg-amazon · 2024-11-12T03:53:04Z

Description

Instead of loading template queries through saved objects api, this pr changes it so that we can load template queries in through the getSampleQuery function directly
Updates URL state when opening a saved query so datapicker state is correct
Updates getSampleQuery interface so it can also return a promise

 PASS  src/plugins/data/public/ui/saved_query_flyouts/open_saved_query_flyout.test.tsx
  OpenSavedQueryFlyout
    ✓ should render the flyout with correct tabs and content (58 ms)
    ✓ should filter saved queries based on search input (29 ms)
    ✓ should select a query when clicking on it and enable the "Open query" button (14 ms)
    ✓ should call handleQueryDelete when deleting a query (21 ms)
    ✓ should handle pagination controls correctly (43 ms)

Test Suites: 1 passed, 1 total
Tests:       5 passed, 5 total
Snapshots:   0 total
Time:        3.351 s
Ran all test suites matching /src\/plugins\/data\/public\/ui\/saved_query_flyouts\/open_saved_query_flyout.test.tsx/i.

Screen.Recording.2024-11-12.at.9.50.17.AM.mov

Issues Resolved

Screenshot

Testing the changes

Changelog

fix: Fix template queries loading and update getSampleQuery interface

Check List

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

codecov · 2024-11-12T04:08:38Z

Codecov Report

All modified and coverable lines are covered by tests ✅

Project coverage is 56.39%. Comparing base (76cf823) to head (c73d357).
Report is 1 commits behind head on main.

Additional details and impacted files

@@            Coverage Diff             @@
##             main    #8848      +/-   ##
==========================================
- Coverage   60.86%   56.39%   -4.48%     
==========================================
  Files        3800     1239    -2561     
  Lines       90818    25913   -64905     
  Branches    14307     4437    -9870     
==========================================
- Hits        55276    14613   -40663     
+ Misses      32028    10559   -21469     
+ Partials     3514      741    -2773

Flag	Coverage Δ
Linux_1	`?`
Linux_2	`56.39% <ø> (ø)`
Linux_3	`?`
Linux_4	`?`
Windows_1	`?`
Windows_2	`?`
Windows_3	`?`
Windows_4	`?`

Flags with carried forward coverage won't be shown. Click here to find out more.

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

Signed-off-by: Joanne Wang <[email protected]>

virajsanghvi

Only skimmed so far, but posting initial comments

src/plugins/data/public/query/query_string/dataset_service/types.ts

src/plugins/data/public/ui/saved_query_flyouts/open_saved_query_flyout.tsx

src/plugins/discover/public/application/components/no_results/no_results.tsx

virajsanghvi · 2024-11-12T18:50:25Z

src/plugins/data/public/ui/saved_query_flyouts/open_saved_query_flyout.tsx

@@ -179,7 +197,13 @@ export function OpenSavedQueryFlyout({
        onChange={onChange}
      />
      <EuiSpacer />
-      {queriesOnCurrentPage.length > 0 ? (
+      {isLoading ? (
+        <EuiFlexGroup justifyContent="center" alignItems="center" style={{ height: '200px' }}>


nit: why are we hardcoding height here?

virajsanghvi · 2024-11-12T18:52:41Z

src/plugins/data/public/ui/saved_query_flyouts/open_saved_query_flyout.tsx

+                      query: selectedQuery.attributes.query.query,
+                      language: selectedQuery.attributes.query.language,
+                    };
+                    queryStringManager.setQuery(updatedQuery);


curious, why does this use setQuery over onQueryOpen?

Similar to this line (link), we wanted to preserve the data source info in the URL state when the saved query isn't associated with a data source. onQueryOpen clears the selected data source.

This is the line that concerns me a little and want to know more about why we need this. What are we trying to do here?

There are 2 states at play here. Query manager state and the local searchbar state. onQueryOpen updates the search bar state and not the query state because the act of opening a saved query today does not update the query state until the user has decided to fire the query (similar to how you dont want typing the query to update query state until you are ready to fire the query)

Updating the query manager state here for templates changes this behaviour for users when it comes to templates, because now it means that template queries DO change the query state even if the user has not been given the opportunity to confirm that thats the query that they want to run.

This change will not affect datasources that dont run the query by default, but for indices and index patterns, this means that opening a saved query template runs the query automatically while non templates do not.

@ashwin-pc That's good to know. Given that context, it sounds like the way forward would involve retrieving the dataset from the query state when the saved query doesn't have a dataset (i.e., when the saved query is a template, or when the saved query is not associated with a dataset), and applying it to the saved query before calling onQueryOpen. Does that sound appropriate?
E.g.,

onClick={() => { if (selectedQuery) { if ( // Template queries are not associated with data sources. Apply data source from current query selectedQuery.attributes.isTemplate || // Associating a saved query with a data source is optional. If no data source is present, apply the current source. !selectedQuery.attributes.query.dataset?.dataSource ) { selectedQuery.attributes.query.dataset = queryStringManager?.getQuery().dataset; } onQueryOpen(selectedQuery); onClose(); } }}

Since this PR has been merged, I'll adjust this in a follow-up PR.

src/plugins/discover/public/application/components/no_results/no_results.tsx

virajsanghvi · 2024-11-12T18:55:59Z

src/plugins/discover/public/application/components/no_results/no_results.tsx

+        if (Array.isArray(sampleQueriesResponse)) {
+          setSampleQueries([...sampleQueriesResponse, ...newSampleQueries]);


nit, could create a helper function for setting sample queries that gets used in both places to prevent divergence

+1 having the savedQueryService take care of this would be ideal. Sample queries and templates are an extension of that

src/plugins/discover/public/application/components/no_results/no_results.tsx

Signed-off-by: Joanne Wang <[email protected]>

Signed-off-by: Riya Saxena <[email protected]>

virajsanghvi

For some reason codecov is no longer loading, but think the two main things are covered at this point.

I restarted some failed CI that I think is unrelated, but please validate.

ashwin-pc

Did we sanity test this with the new UI with different datasources and languages?

ashwin-pc · 2024-11-13T11:56:59Z

src/plugins/data/public/ui/saved_query_flyouts/open_saved_query_flyout.tsx

@@ -179,7 +197,13 @@ export function OpenSavedQueryFlyout({
        onChange={onChange}
      />
      <EuiSpacer />
-      {queriesOnCurrentPage.length > 0 ? (
+      {isLoading ? (
+        <EuiFlexGroup justifyContent="center" alignItems="center" style={{ height: '200px' }}>


ashwin-pc · 2024-11-13T12:03:18Z

src/plugins/data/public/ui/saved_query_flyouts/open_saved_query_flyout.tsx

+    setIsLoading(true);
+    const query = queryStringManager.getQuery();
+    let templateQueries: any[] = [];
+
+    // fetch sample query based on dataset type
+    if (query?.dataset?.type) {
+      templateQueries =
+        (await queryStringManager
+          .getDatasetService()
+          ?.getType(query.dataset.type)
+          ?.getSampleQueries?.()) || [];
+
+      // Check if any sample query has isTemplate set to true
+      const hasTemplates = templateQueries.some((q) => q?.attributes?.isTemplate);
+      setHasTemplateQueries(hasTemplates);
+    }
+
+    // Set queries based on the current tab
+    if (currentTabIdRef.current === 'mutable-saved-queries') {
+      const allQueries = await savedQueryService.getAllSavedQueries();
+      const mutableSavedQueries = allQueries.filter((q) => !q.attributes.isTemplate);
+      if (currentTabIdRef.current === 'mutable-saved-queries') {
+        setSavedQueries(mutableSavedQueries);
+      }
+    } else if (currentTabIdRef.current === 'template-saved-queries') {
+      setSavedQueries(templateQueries);
+    }
+    setIsLoading(false);
+  }, [savedQueryService, currentTabIdRef, setSavedQueries, queryStringManager]);


Does this mean that now while my sample queries load my saved query flyout is unusable? Also if thats true, whats the user experience when these queries take time to load or fail?

ashwin-pc · 2024-11-13T12:05:07Z

src/plugins/data/public/ui/saved_query_flyouts/open_saved_query_flyout.tsx

+    if (currentTabIdRef.current === 'mutable-saved-queries') {
+      const allQueries = await savedQueryService.getAllSavedQueries();
+      const mutableSavedQueries = allQueries.filter((q) => !q.attributes.isTemplate);
+      if (currentTabIdRef.current === 'mutable-saved-queries') {
+        setSavedQueries(mutableSavedQueries);
+      }
+    } else if (currentTabIdRef.current === 'template-saved-queries') {


nit: Can we use enums or constants for the tabs and not raw strings?

ashwin-pc · 2024-11-13T12:19:51Z

src/plugins/data/public/ui/saved_query_flyouts/open_saved_query_flyout.tsx

+                      query: selectedQuery.attributes.query.query,
+                      language: selectedQuery.attributes.query.language,
+                    };
+                    queryStringManager.setQuery(updatedQuery);


This is the line that concerns me a little and want to know more about why we need this. What are we trying to do here?

There are 2 states at play here. Query manager state and the local searchbar state. onQueryOpen updates the search bar state and not the query state because the act of opening a saved query today does not update the query state until the user has decided to fire the query (similar to how you dont want typing the query to update query state until you are ready to fire the query)

Updating the query manager state here for templates changes this behaviour for users when it comes to templates, because now it means that template queries DO change the query state even if the user has not been given the opportunity to confirm that thats the query that they want to run.

This change will not affect datasources that dont run the query by default, but for indices and index patterns, this means that opening a saved query template runs the query automatically while non templates do not.

ashwin-pc · 2024-11-13T12:22:02Z

src/plugins/data/public/ui/saved_query_management/saved_query_management_component.tsx

@@ -256,6 +258,7 @@ export function SavedQueryManagementComponent({
                  onClose={() => openSavedQueryFlyout?.close().then()}
                  onQueryOpen={onLoad}
                  handleQueryDelete={handleDelete}
+                  queryStringManager={queryStringManager}


Nit: Instead of prop drilling queryStringManager throughout, savedQueryService should ideally have been responsible for templates too.

ashwin-pc · 2024-11-13T12:24:33Z

src/plugins/discover/public/application/components/no_results/no_results.tsx

+        if (Array.isArray(sampleQueriesResponse)) {
+          setSampleQueries([...sampleQueriesResponse, ...newSampleQueries]);


+1 having the savedQueryService take care of this would be ideal. Sample queries and templates are an extension of that

ashwin-pc · 2024-11-13T12:25:32Z

src/plugins/discover/public/application/components/no_results/no_results.tsx

+              }
+            })
+            .catch((error: any) => {
+              // noop


Lets atleast log this?

kavilla

i would avoid injecting a new prop to global component. will add more comments to the pr

kavilla · 2024-11-13T13:31:38Z

src/plugins/data/public/ui/search_bar/search_bar.tsx

@@ -467,6 +474,7 @@ class SearchBarUI extends Component<SearchBarProps, State> {
            useSaveQueryMenu={useSaveQueryMenu}
            isQueryEditorControl={isQueryEditorControl}
            saveQuery={this.onSave}
+            queryStringManager={this.props.queryStringManager}


the query string manager being pass in which will impact the state of the components in search bar which is also utilized by other plugins besides the Discover plugin (like dashboards and visualizations) so i would watch out and generally avoid touching this file.

this already was able to access the query string manager like this:

https://github.com/opensearch-project/OpenSearch-Dashboards/blob/main/src/plugins/data/public/ui/search_bar/search_bar.tsx#L123

this should be available in similar locations within this ui

Thanks for calling that out. I'll revert line 205 in create_search_bar.tsx.

Since this PR has been merged, I'll adjust this in a follow-up PR.

Addressed in #8864

kavilla · 2024-11-13T13:35:54Z

src/plugins/data/public/ui/saved_query_flyouts/open_saved_query_flyout.tsx

-  }, [savedQueryService, selectedTabId, setSavedQueries]);
+    setIsLoading(true);
+    const query = queryStringManager.getQuery();
+    let templateQueries: any[] = [];


we should just be passing around a Query object.

kavilla · 2024-11-13T14:07:35Z

src/plugins/data/public/ui/saved_query_flyouts/open_saved_query_flyout.tsx

+    }
+
+    // Set queries based on the current tab
+    if (currentTabIdRef.current === 'mutable-saved-queries') {


apologies i believe i missed the original design PR for template queries.

Key Issues with Current Approach:

Saved Query Purpose Change

Historically, saved queries stored exactly what users typed

Used for cross-application consistency

one major use case: Create query in Discover, apply same query in Dashboards against many saved searches and visualizations

Cross-Plugin Impact

Potential unexpected behavior in Dashboards when using templated queries

Need to verify behavior after dataset type selection

Other plugins may not be aware of template functionality

Suggested Alternative Structure

Make saved queries and saved query templates separate entities

Create new saved object type: "saved query template"

Keep existing saved query behavior intact

Share UI components between both types

This maintains backwards compatibility while cleanly separating template functionality from core saved query behavior.

that said might be a little too late. but have we verified existing saved query impact? as of today with this new ui toggled on? do we need to add a saved object migration for saved queries? could we verify the impact to other plugins for example the dashboards plugin?

Overriding due to a parallel conversation where some of the existing issues are an existing bug. This is not the final implimentation and requires changes to incorporate the appropriate fixes. The added property should be fine since it does not modify how the existing properties and saved searches work

ashwin-pc

Conditionally approving it to unblock testing the saved query template changes. Saved query as a feature is still broken. That needs to be fixed in a separate PR.

* update sample query impl and fix saved query load Signed-off-by: Joanne Wang <[email protected]> * Changeset file for PR #8848 created/updated * Changeset file for PR #8848 created/updated * add loading spinner Signed-off-by: Joanne Wang <[email protected]> * check if tab is shown based on if sample query isTemplate Signed-off-by: Joanne Wang <[email protected]> * added UTs for svaed queries flyout Signed-off-by: Riya Saxena <[email protected]> --------- Signed-off-by: Joanne Wang <[email protected]> Signed-off-by: Riya Saxena <[email protected]> Co-authored-by: opensearch-changeset-bot[bot] <154024398+opensearch-changeset-bot[bot]@users.noreply.github.com> Co-authored-by: Riya Saxena <[email protected]> (cherry picked from commit 25d3de7) Signed-off-by: github-actions[bot] <github-actions[bot]@users.noreply.github.com>

…) (#8859) * update sample query impl and fix saved query load * Changeset file for PR #8848 created/updated * Changeset file for PR #8848 created/updated * add loading spinner * check if tab is shown based on if sample query isTemplate * added UTs for svaed queries flyout --------- (cherry picked from commit 25d3de7) Signed-off-by: Joanne Wang <[email protected]> Signed-off-by: Riya Saxena <[email protected]> Signed-off-by: github-actions[bot] <github-actions[bot]@users.noreply.github.com> Co-authored-by: github-actions[bot] <github-actions[bot]@users.noreply.github.com> Co-authored-by: opensearch-changeset-bot[bot] <154024398+opensearch-changeset-bot[bot]@users.noreply.github.com> Co-authored-by: Riya Saxena <[email protected]>

github-actions bot added the first-time-contributor label Nov 12, 2024

opensearch-changeset-bot bot added a commit to jowg-amazon/OpenSearch-Dashboards that referenced this pull request Nov 12, 2024

Changeset file for PR opensearch-project#8848 created/updated

eca8670

update sample query impl and fix saved query load

0221592

Signed-off-by: Joanne Wang <[email protected]>

jowg-amazon force-pushed the sample_query branch from eca8670 to 0221592 Compare November 12, 2024 06:41

Changeset file for PR opensearch-project#8848 created/updated

5b4b3a8

jowg-amazon changed the title ~~Fix saved query loading and update getSampleQuery interface~~ Fix template queries loading and update getSampleQuery interface Nov 12, 2024

opensearch-changeset-bot bot and others added 2 commits November 12, 2024 07:14

Changeset file for PR opensearch-project#8848 created/updated

0e07d48

add loading spinner

10e8a9e

Signed-off-by: Joanne Wang <[email protected]>

virajsanghvi reviewed Nov 12, 2024

View reviewed changes

jowg-amazon and others added 2 commits November 12, 2024 12:45

check if tab is shown based on if sample query isTemplate

e14f0be

Signed-off-by: Joanne Wang <[email protected]>

added UTs for svaed queries flyout

c73d357

Signed-off-by: Riya Saxena <[email protected]>

virajsanghvi approved these changes Nov 13, 2024

View reviewed changes

ashwin-pc reviewed Nov 13, 2024

View reviewed changes

kavilla previously requested changes Nov 13, 2024

View reviewed changes

kavilla reviewed Nov 13, 2024

View reviewed changes

ashwin-pc approved these changes Nov 13, 2024

View reviewed changes

ashwin-pc merged commit 25d3de7 into opensearch-project:main Nov 13, 2024
67 of 70 checks passed

amsiglan added the backport 2.x label Nov 13, 2024

opensearch-trigger-bot bot mentioned this pull request Nov 13, 2024

[Backport 2.x] Fix template queries loading and update getSampleQuery interface #8859

Merged

AWSHurneyt mentioned this pull request Nov 14, 2024

Adjusted source of QueryStringManager functions for flyout. #8864

Open

7 tasks

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Fix template queries loading and update getSampleQuery interface #8848

Fix template queries loading and update getSampleQuery interface #8848

jowg-amazon commented Nov 12, 2024 •

edited

Loading

codecov bot commented Nov 12, 2024 •

edited

Loading

virajsanghvi left a comment

virajsanghvi Nov 12, 2024

ashwin-pc Nov 13, 2024

virajsanghvi Nov 12, 2024

AWSHurneyt Nov 12, 2024

ashwin-pc Nov 13, 2024

AWSHurneyt Nov 13, 2024

AWSHurneyt Nov 13, 2024

virajsanghvi Nov 12, 2024

ashwin-pc Nov 13, 2024

virajsanghvi left a comment

ashwin-pc left a comment

ashwin-pc Nov 13, 2024

ashwin-pc Nov 13, 2024

ashwin-pc Nov 13, 2024

ashwin-pc Nov 13, 2024

ashwin-pc Nov 13, 2024

ashwin-pc Nov 13, 2024

ashwin-pc Nov 13, 2024

kavilla left a comment

kavilla Nov 13, 2024

AWSHurneyt Nov 13, 2024

AWSHurneyt Nov 13, 2024

AWSHurneyt Nov 14, 2024

kavilla Nov 13, 2024

kavilla Nov 13, 2024 •

edited

Loading

ashwin-pc left a comment

		if (Array.isArray(sampleQueriesResponse)) {
		setSampleQueries([...sampleQueriesResponse, ...newSampleQueries]);

Fix template queries loading and update getSampleQuery interface #8848

Fix template queries loading and update getSampleQuery interface #8848

Conversation

jowg-amazon commented Nov 12, 2024 • edited Loading

Description

Issues Resolved

Screenshot

Testing the changes

Changelog

Check List

codecov bot commented Nov 12, 2024 • edited Loading

Codecov Report

virajsanghvi left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

virajsanghvi left a comment

Choose a reason for hiding this comment

ashwin-pc left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

kavilla left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

kavilla Nov 13, 2024 • edited Loading

Choose a reason for hiding this comment

ashwin-pc left a comment

Choose a reason for hiding this comment

jowg-amazon commented Nov 12, 2024 •

edited

Loading

codecov bot commented Nov 12, 2024 •

edited

Loading

kavilla Nov 13, 2024 •

edited

Loading