Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
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
Fix template queries loading and update getSampleQuery interface #8848
Fix template queries loading and update getSampleQuery interface #8848
Changes from 4 commits
0221592
5b4b3a8
0e07d48
10e8a9e
e14f0be
c73d357
File filter
Filter by extension
Conversations
Jump to
There are no files selected for viewing
Check warning on line 67 in src/plugins/data/public/ui/saved_query_flyouts/open_saved_query_flyout.tsx
Codecov / codecov/patch
src/plugins/data/public/ui/saved_query_flyouts/open_saved_query_flyout.tsx#L66-L67
Check warning on line 70 in src/plugins/data/public/ui/saved_query_flyouts/open_saved_query_flyout.tsx
Codecov / codecov/patch
src/plugins/data/public/ui/saved_query_flyouts/open_saved_query_flyout.tsx#L70
Check warning on line 72 in src/plugins/data/public/ui/saved_query_flyouts/open_saved_query_flyout.tsx
Codecov / codecov/patch
src/plugins/data/public/ui/saved_query_flyouts/open_saved_query_flyout.tsx#L72
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
apologies i believe i missed the original design PR for template queries.
Key Issues with Current Approach:
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?
Check warning on line 76 in src/plugins/data/public/ui/saved_query_flyouts/open_saved_query_flyout.tsx
Codecov / codecov/patch
src/plugins/data/public/ui/saved_query_flyouts/open_saved_query_flyout.tsx#L75-L76
Check warning on line 78 in src/plugins/data/public/ui/saved_query_flyouts/open_saved_query_flyout.tsx
Codecov / codecov/patch
src/plugins/data/public/ui/saved_query_flyouts/open_saved_query_flyout.tsx#L78
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
nit: Can we use enums or constants for the tabs and not raw strings?
Check warning on line 81 in src/plugins/data/public/ui/saved_query_flyouts/open_saved_query_flyout.tsx
Codecov / codecov/patch
src/plugins/data/public/ui/saved_query_flyouts/open_saved_query_flyout.tsx#L81
Check warning on line 83 in src/plugins/data/public/ui/saved_query_flyouts/open_saved_query_flyout.tsx
Codecov / codecov/patch
src/plugins/data/public/ui/saved_query_flyouts/open_saved_query_flyout.tsx#L83
Check warning on line 90 in src/plugins/data/public/ui/saved_query_flyouts/open_saved_query_flyout.tsx
Codecov / codecov/patch
src/plugins/data/public/ui/saved_query_flyouts/open_saved_query_flyout.tsx#L90
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
nit: why are we hardcoding height here?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
+1
Check warning on line 279 in src/plugins/data/public/ui/saved_query_flyouts/open_saved_query_flyout.tsx
Codecov / codecov/patch
src/plugins/data/public/ui/saved_query_flyouts/open_saved_query_flyout.tsx#L279
Check warning on line 300 in src/plugins/data/public/ui/saved_query_flyouts/open_saved_query_flyout.tsx
Codecov / codecov/patch
src/plugins/data/public/ui/saved_query_flyouts/open_saved_query_flyout.tsx#L300
Check warning on line 305 in src/plugins/data/public/ui/saved_query_flyouts/open_saved_query_flyout.tsx
Codecov / codecov/patch
src/plugins/data/public/ui/saved_query_flyouts/open_saved_query_flyout.tsx#L305
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
curious, why does this use setQuery over onQueryOpen?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@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.,
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Since this PR has been merged, I'll adjust this in a follow-up PR.
Check warning on line 307 in src/plugins/data/public/ui/saved_query_flyouts/open_saved_query_flyout.tsx
Codecov / codecov/patch
src/plugins/data/public/ui/saved_query_flyouts/open_saved_query_flyout.tsx#L307
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Nit: Instead of prop drilling
queryStringManager
throughout,savedQueryService
should ideally have been responsible for templates too.There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks for calling that out. I'll revert line 205 in create_search_bar.tsx.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Since this PR has been merged, I'll adjust this in a follow-up PR.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Addressed in #8864
Check warning on line 177 in src/plugins/discover/public/application/components/no_results/no_results.tsx
Codecov / codecov/patch
src/plugins/discover/public/application/components/no_results/no_results.tsx#L177
Check warning on line 190 in src/plugins/discover/public/application/components/no_results/no_results.tsx
Codecov / codecov/patch
src/plugins/discover/public/application/components/no_results/no_results.tsx#L190
Check warning on line 192 in src/plugins/discover/public/application/components/no_results/no_results.tsx
Codecov / codecov/patch
src/plugins/discover/public/application/components/no_results/no_results.tsx#L192
Check warning on line 194 in src/plugins/discover/public/application/components/no_results/no_results.tsx
Codecov / codecov/patch
src/plugins/discover/public/application/components/no_results/no_results.tsx#L194
Check warning on line 197 in src/plugins/discover/public/application/components/no_results/no_results.tsx
Codecov / codecov/patch
src/plugins/discover/public/application/components/no_results/no_results.tsx#L197
Check warning on line 203 in src/plugins/discover/public/application/components/no_results/no_results.tsx
Codecov / codecov/patch
src/plugins/discover/public/application/components/no_results/no_results.tsx#L203
Check warning on line 205 in src/plugins/discover/public/application/components/no_results/no_results.tsx
Codecov / codecov/patch
src/plugins/discover/public/application/components/no_results/no_results.tsx#L205
Check warning on line 207 in src/plugins/discover/public/application/components/no_results/no_results.tsx
Codecov / codecov/patch
src/plugins/discover/public/application/components/no_results/no_results.tsx#L207
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
nit, could create a helper function for setting sample queries that gets used in both places to prevent divergence
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
+1 having the savedQueryService take care of this would be ideal. Sample queries and templates are an extension of that
Check warning on line 209 in src/plugins/discover/public/application/components/no_results/no_results.tsx
Codecov / codecov/patch
src/plugins/discover/public/application/components/no_results/no_results.tsx#L209
Check warning on line 212 in src/plugins/discover/public/application/components/no_results/no_results.tsx
Codecov / codecov/patch
src/plugins/discover/public/application/components/no_results/no_results.tsx#L212
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Lets atleast log this?