Fix Loading and Editing Saved Searches in Discover #8306

LDrago27 · 2024-09-24T00:40:49Z

Description

This PR aims at resolving the following issues.

Load Saved Search with Non index pattern Datasources
Change datasources for a Saved Search
Filters not showing up while a saved search is loaded
Filters not refreshing across page refresh

The CI group 6 failure for the PR has been addressed as part of this PR opensearch-project/opensearch-dashboards-functional-test#1585.

Issues Resolved

#8339
#8338

Screenshot

Testing the changes

Meeting.Recording.-.Sahoo.Suchit.Instant.Meeting.9.mp4

Meeting.Recording.-.Sahoo.Suchit.Instant.Meeting.10.mp4

Follow the steps mentioned in the video to test out the changes.

Changelog

skip

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-09-24T00:56:03Z

Codecov Report

Attention: Patch coverage is 14.28571% with 18 lines in your changes missing coverage. Please review.

Project coverage is 60.85%. Comparing base (e23f332) to head (d69bb15).
Report is 13 commits behind head on main.

Files with missing lines	Patch %	Lines
...ic/application/view_components/utils/use_search.ts	0.00%	6 Missing ⚠️
...on/index_patterns/index_patterns/index_patterns.ts	0.00%	4 Missing ⚠️
...ommon/search/search_source/create_search_source.ts	0.00%	1 Missing and 2 partials ⚠️
...plication/components/top_nav/get_top_nav_links.tsx	0.00%	2 Missing ⚠️
src/plugins/data/public/search/search_service.ts	0.00%	0 Missing and 1 partial ⚠️
...plication/components/top_nav/open_search_panel.tsx	0.00%	1 Missing ⚠️
...lication/utils/state_management/discover_slice.tsx	0.00%	1 Missing ⚠️

Additional details and impacted files

@@            Coverage Diff             @@
##             main    #8306      +/-   ##
==========================================
- Coverage   60.86%   60.85%   -0.02%     
==========================================
  Files        3793     3793              
  Lines       90367    90381      +14     
  Branches    14182    14189       +7     
==========================================
- Hits        55000    54998       -2     
- Misses      31890    31902      +12     
- Partials     3477     3481       +4

Flag	Coverage Δ
Linux_1	`29.09% <0.00%> (-0.01%)`	⬇️
Linux_2	`56.37% <0.00%> (-0.02%)`	⬇️
Linux_3	`37.68% <14.28%> (-0.01%)`	⬇️
Linux_4	`29.82% <0.00%> (-0.01%)`	⬇️
Windows_1	`29.10% <0.00%> (-0.01%)`	⬇️
Windows_2	`56.33% <0.00%> (-0.02%)`	⬇️
Windows_3	`37.68% <14.28%> (-0.01%)`	⬇️
Windows_4	`29.82% <0.00%> (-0.01%)`	⬇️

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.

kavilla · 2024-09-25T00:00:20Z

src/plugins/data/common/search/search_source/create_search_source.ts

@@ -28,6 +28,7 @@
 * under the License.
 */

+import { QueryStringContract } from 'src/plugins/data/public/';


unfortunately we should import public into common folder. the compiler might complain about this.

we should consider taking the required function from the query string service (cache dataset) and exporting it in the common folder so that it's reusable by the dataset service and search source service

Where would i see it complaining?

I am surprised too that CI is not failing.

src/plugins/data/common/search/search_source/create_search_source.ts

ashwin-pc

The changes looks good. Can we add some tests for the logic here. I want to make sure that things like "Filters in URL are higher priority than the filters in saved search" have test that will break if thats ever not true in future

src/plugins/discover/public/application/utils/state_management/discover_slice.tsx

src/plugins/discover/public/application/components/top_nav/open_search_panel.tsx

src/plugins/discover/public/application/components/top_nav/get_top_nav_links.tsx

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

src/plugins/data/public/ui/dataset_selector/index.tsx

src/plugins/data/common/search/search_source/create_search_source.ts

LDrago27 · 2024-10-07T20:32:54Z

The changes looks good. Can we add some tests for the logic here. I want to make sure that things like "Filters in URL are higher priority than the filters in saved search" have test that will break if thats ever not true in future

I have added couple of tests opensearch-project/opensearch-dashboards-functional-test#1585 . One to verify the above scenario and one to ensure we are resetting the existing filters when we are trying to create a new search.

sejli · 2024-10-08T22:45:24Z

src/plugins/data/public/ui/dataset_selector/index.tsx

  );

+  useEffect(() => {
+    const subscription = queryString.getUpdates$(0).subscribe((query: Query) => {
+      if (query.dataset !== selectedDataset) {


We recently removed this useEffect() in #8347 to avoid duplicate calls. Does this if condition avoid the duplicate calls? Are we adding this back in so it picks up updates from state sync?

@sejli The reasoning behind adding this , is when we are using to load the state from the saved Object it loads it without using the dataset selector component. Therefore although the actual query state has been updated but since the dataset selector is not triggered, the dataset selector content is not updated. The if condition is to avoid multiple updates.

src/plugins/data/common/search/search_source/create_search_source.ts

AMoo-Miki · 2024-10-15T23:27:22Z

src/plugins/discover/public/application/view_components/utils/use_search.ts

-      data.query.queryString.setQuery(query);
+      // Filters in URL are higher priority than the filters in saved search
+      const urlFilters = (osdUrlStateStorage.get('_q') as QueryState)?.filters ?? [];
+      if (!urlFilters || urlFilters.length === 0) {


This is untrusted input; right?

Suggested change

if (!urlFilters || urlFilters.length === 0) {

if (!Array.isArray(urlFilters) || urlFilters.length === 0) {

AMoo-Miki · 2024-10-15T23:28:56Z

src/plugins/discover/public/application/view_components/utils/use_search.ts

+      // Filters in URL are higher priority than the filters in saved search
+      const urlFilters = (osdUrlStateStorage.get('_q') as QueryState)?.filters ?? [];
+      if (!urlFilters || urlFilters.length === 0) {
+        filterManager.setAppFilters(actualFilters);


wouldn't we want to make sure to call filterManager.setAppFilters([]); when urlFilters is bad?

LDrago27 · 2024-10-18T18:01:34Z

Will add additional unit tests & concerns in a follow up PR.

virajsanghvi · 2024-10-18T19:01:20Z

src/plugins/data/common/index_patterns/index_patterns/index_patterns.ts

+    const indexPattern = indexPatternCache.get(id);
+    if (indexPattern) {
+      return true;
+    }
+    return false;


nit: return !!indexPatternCache.get(id);

virajsanghvi · 2024-10-18T19:06:44Z

src/plugins/data/common/search/search_source/create_search_source.ts

+  if (
+    searchSourceDependencies.queryStringService &&
+    fields.query?.dataset &&
+    fields.query?.dataset?.type !== 'INDEX_PATTERN' &&


nit: at this point, don't need ? in ?.dataset

virajsanghvi · 2024-10-18T19:07:12Z

src/plugins/data/common/search/search_source/create_search_source.ts

+  ) {
+    await searchSourceDependencies.queryStringService
+      .getDatasetService()
+      .cacheDataset(fields.query?.dataset);


does this do the right thing if dataset is undefined?

virajsanghvi · 2024-10-18T19:10:22Z

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

-        const newQueryString = newQuery.query;
-        if (this.inputRef.getValue() !== newQueryString) {
+        const newQueryString = this.props.query.query;
+        if (this.inputRef.getValue() !== newQueryString && typeof newQueryString === 'string') {


is inputRef.getValue() behind prop because its a controlled input (e.g. its value is defined by props)?

Signed-off-by: Suchit Sahoo <[email protected]>

Signed-off-by: Ashwin P Chandran <[email protected]>

AMoo-Miki · 2024-10-19T20:26:37Z

src/plugins/data/public/ui/dataset_selector/index.test.tsx

@@ -9,6 +9,7 @@ import { DatasetSelector as ConnectedDatasetSelector } from './index';
 import { DatasetSelector } from './dataset_selector';
 import { useOpenSearchDashboards } from '../../../../opensearch_dashboards_react/public';
 import { Dataset } from '../../../common';
+import { of } from 'rxjs';


Suggested change

import { of } from 'rxjs';

StefanStroebel · 2024-10-21T17:55:39Z

Can we get an ETA when this will be approved and Merged?

ohenr · 2024-10-31T07:55:02Z

Any news on this?

github-actions bot added all-star-contributor Skip-Changelog PRs that are too trivial to warrant a changelog or release notes entry labels Sep 24, 2024

kavilla reviewed Sep 25, 2024

View reviewed changes

src/plugins/data/common/search/search_source/create_search_source.ts Outdated Show resolved Hide resolved

LDrago27 force-pushed the savedSearchEditFix branch 2 times, most recently from cbeb3b5 to 5828bfb Compare October 3, 2024 19:11

LDrago27 added discover-next v2.18.0 backport 2.x labels Oct 3, 2024

LDrago27 marked this pull request as ready for review October 3, 2024 19:15

LDrago27 requested review from ananzh, AMoo-Miki, ashwin-pc, joshuarrrr, abbyhu2000, zengyan-amazon, zhongnansu, manasvinibs, ZilongX, Flyingliuhub, curq, bandinib-amzn, SuZhou-Joe, ruanyl, BionIT, xinruiba, zhyuanqi, mengweieric and virajsanghvi as code owners October 3, 2024 19:15

LDrago27 force-pushed the savedSearchEditFix branch 3 times, most recently from 138437d to 70a911a Compare October 3, 2024 22:39

This was linked to issues Oct 4, 2024

[BUG] Filters persist when creating new searches or switching indices #8338

Open

[BUG] Filters stored in saved search are not loaded #8339

Open

LDrago27 mentioned this pull request Oct 4, 2024

Updated the Discover Saved Search Tests opensearch-project/opensearch-dashboards-functional-test#1585

Open

1 task

LDrago27 force-pushed the savedSearchEditFix branch from 02317e8 to 97a42ef Compare October 4, 2024 19:35

ashwin-pc reviewed Oct 4, 2024

View reviewed changes

LDrago27 force-pushed the savedSearchEditFix branch from 97a42ef to 2ab3400 Compare October 7, 2024 20:30

sejli reviewed Oct 8, 2024

View reviewed changes

LDrago27 force-pushed the savedSearchEditFix branch from 2ab3400 to 6788fdd Compare October 15, 2024 22:09

AMoo-Miki reviewed Oct 15, 2024

View reviewed changes

src/plugins/data/common/search/search_source/create_search_source.ts Show resolved Hide resolved

AMoo-Miki reviewed Oct 15, 2024

View reviewed changes

LDrago27 force-pushed the savedSearchEditFix branch from 6788fdd to 8fbfc88 Compare October 18, 2024 17:58

virajsanghvi reviewed Oct 18, 2024

View reviewed changes

LDrago27 force-pushed the savedSearchEditFix branch from 8fbfc88 to 5bd3466 Compare October 18, 2024 19:17

AMoo-Miki force-pushed the savedSearchEditFix branch from 5bd3466 to 9c8e62f Compare October 19, 2024 00:53

LDrago27 added 3 commits October 19, 2024 16:51

Fix Saved Search Load for Non Index pattern Datasources

9b0d9ec

Signed-off-by: Suchit Sahoo <[email protected]>

Fixed Test failures

454a507

Signed-off-by: Suchit Sahoo <[email protected]>

Fix Filter Manager Bugs in Saved Search

27f7856

Signed-off-by: Suchit Sahoo <[email protected]>

ashwin-pc force-pushed the savedSearchEditFix branch from 9c8e62f to 27f7856 Compare October 19, 2024 16:59

fix validation

d69bb15

Signed-off-by: Ashwin P Chandran <[email protected]>

AMoo-Miki reviewed Oct 19, 2024

View reviewed changes

kavilla added v2.19.0 and removed v2.18.0 labels Oct 24, 2024

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Fix Loading and Editing Saved Searches in Discover #8306

Fix Loading and Editing Saved Searches in Discover #8306

LDrago27 commented Sep 24, 2024 •

edited

Loading

codecov bot commented Sep 24, 2024 •

edited

Loading

kavilla Sep 25, 2024

ashwin-pc Oct 4, 2024

AMoo-Miki Oct 15, 2024

ashwin-pc left a comment

LDrago27 commented Oct 7, 2024

sejli Oct 8, 2024

LDrago27 Oct 10, 2024

AMoo-Miki Oct 15, 2024

AMoo-Miki Oct 15, 2024

LDrago27 commented Oct 18, 2024 •

edited

Loading

virajsanghvi Oct 18, 2024

virajsanghvi Oct 18, 2024

virajsanghvi Oct 18, 2024

virajsanghvi Oct 18, 2024

AMoo-Miki Oct 19, 2024

StefanStroebel commented Oct 21, 2024

ohenr commented Oct 31, 2024

	if (!urlFilters \|\| urlFilters.length === 0) {
	if (!Array.isArray(urlFilters) \|\| urlFilters.length === 0) {

Fix Loading and Editing Saved Searches in Discover #8306

Are you sure you want to change the base?

Fix Loading and Editing Saved Searches in Discover #8306

Conversation

LDrago27 commented Sep 24, 2024 • edited Loading

Description

Issues Resolved

Screenshot

Testing the changes

Changelog

Check List

codecov bot commented Sep 24, 2024 • edited Loading

Codecov Report

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

ashwin-pc left a comment

Choose a reason for hiding this comment

LDrago27 commented Oct 7, 2024

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

LDrago27 commented Oct 18, 2024 • edited Loading

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

StefanStroebel commented Oct 21, 2024

ohenr commented Oct 31, 2024

LDrago27 commented Sep 24, 2024 •

edited

Loading

codecov bot commented Sep 24, 2024 •

edited

Loading

LDrago27 commented Oct 18, 2024 •

edited

Loading