Skip to content
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 Loading and Editing Saved Searches in Discover #8306

Open
wants to merge 4 commits into
base: main
Choose a base branch
from
Open
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
Original file line number Diff line number Diff line change
Expand Up @@ -234,6 +234,14 @@
indexPatternCache.set(id, indexPattern);
};

isPresentInCache(id: string) {
const indexPattern = indexPatternCache.get(id);

Check warning on line 238 in src/plugins/data/common/index_patterns/index_patterns/index_patterns.ts

View check run for this annotation

Codecov / codecov/patch

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

Added line #L238 was not covered by tests
if (indexPattern) {
return true;

Check warning on line 240 in src/plugins/data/common/index_patterns/index_patterns/index_patterns.ts

View check run for this annotation

Codecov / codecov/patch

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

Added line #L240 was not covered by tests
}
return false;

Check warning on line 242 in src/plugins/data/common/index_patterns/index_patterns/index_patterns.ts

View check run for this annotation

Codecov / codecov/patch

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

Added line #L242 was not covered by tests
Comment on lines +238 to +242
Copy link
Collaborator

Choose a reason for hiding this comment

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

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

}

/**
* Get default index pattern
*/
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -28,6 +28,7 @@
* under the License.
*/

import { QueryStringContract } from 'src/plugins/data/public/';
Copy link
Member

Choose a reason for hiding this comment

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

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

Copy link
Member

Choose a reason for hiding this comment

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

Where would i see it complaining?

Copy link
Collaborator

Choose a reason for hiding this comment

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

I am surprised too that CI is not failing.

import { migrateLegacyQuery } from './migrate_legacy_query';
import { SearchSource, SearchSourceDependencies } from './search_source';
import { IndexPatternsContract } from '../../index_patterns/index_patterns';
Expand Down Expand Up @@ -56,6 +57,18 @@
) => async (searchSourceFields: SearchSourceFields = {}) => {
const fields = { ...searchSourceFields };

// When we load a saved search and the saved search contains a non index pattern data source this step creates the temperary index patterns and sets the appriopriate query
if (
searchSourceDependencies.queryStringService &&
AMoo-Miki marked this conversation as resolved.
Show resolved Hide resolved
fields.query?.dataset &&
fields.query?.dataset?.type !== 'INDEX_PATTERN' &&
Copy link
Collaborator

Choose a reason for hiding this comment

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

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

!indexPatterns.isPresentInCache(fields.query.dataset.id)
) {
await searchSourceDependencies.queryStringService

Check warning on line 67 in src/plugins/data/common/search/search_source/create_search_source.ts

View check run for this annotation

Codecov / codecov/patch

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

Added line #L67 was not covered by tests
.getDatasetService()
.cacheDataset(fields.query?.dataset);
LDrago27 marked this conversation as resolved.
Show resolved Hide resolved
Copy link
Collaborator

Choose a reason for hiding this comment

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

does this do the right thing if dataset is undefined?

}

// hydrating index pattern
if (fields.index && typeof fields.index === 'string') {
fields.index = await indexPatterns.get(searchSourceFields.index as any);
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -83,6 +83,7 @@
import { setWith } from '@elastic/safer-lodash-set';
import { stringify } from '@osd/std';
import { uniqueId, uniq, extend, pick, difference, omit, isObject, keys, isFunction } from 'lodash';
import { QueryStringContract } from 'src/plugins/data/public';
import { normalizeSortRequest } from './normalize_sort_request';
import { filterDocvalueFields } from './filter_docvalue_fields';
import { fieldWildcardFilter } from '../../../../opensearch_dashboards_utils/common';
Expand Down Expand Up @@ -157,6 +158,7 @@ export interface SearchSourceDependencies extends FetchHandlers {
set: (dataFrame: IDataFrame) => void;
clear: () => void;
};
queryStringService?: QueryStringContract;
}

/** @public **/
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -31,6 +31,7 @@
import { BehaviorSubject } from 'rxjs';
import { IndexPatternsContract } from '../../index_patterns/index_patterns';
import { SearchSourceService, SearchSourceDependencies } from './';
import { QueryStringContract } from 'src/plugins/data/public';

describe('SearchSource service', () => {
let dependencies: jest.Mocked<SearchSourceDependencies>;
Expand All @@ -45,6 +46,7 @@ describe('SearchSource service', () => {
callMsearch: jest.fn(),
loadingCount$: new BehaviorSubject(0),
},
queryStringService: (jest.fn() as unknown) as jest.Mocked<QueryStringContract>,
};
});

Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -31,7 +31,7 @@
import { BehaviorSubject } from 'rxjs';
import { skip } from 'rxjs/operators';
import { CoreStart, NotificationsSetup } from 'opensearch-dashboards/public';
import { debounce, isEqual } from 'lodash';
import { isEqual } from 'lodash';
import { i18n } from '@osd/i18n';
import { Dataset, DataStorage, Query, TimeRange, UI_SETTINGS } from '../../../common';
import { createHistory, QueryHistory } from './query_history';
Expand Down Expand Up @@ -106,8 +106,8 @@ export class QueryStringManager {
}
}

public getUpdates$ = () => {
return this.query$.asObservable().pipe(skip(1));
public getUpdates$ = (defaultSkip = 1) => {
return this.query$.asObservable().pipe(skip(defaultSkip));
};

public getQuery = (): Query => {
Expand Down
3 changes: 3 additions & 0 deletions src/plugins/data/public/search/search_service.ts
Original file line number Diff line number Diff line change
Expand Up @@ -207,6 +207,9 @@ export class SearchService implements Plugin<ISearchSetup, ISearchStart> {
loadingCount$,
},
df: dfService,
queryStringService: uiSettings.get(UI_SETTINGS.QUERY_ENHANCEMENTS_ENABLED)
? getQueryService().queryString
: undefined,
};

return {
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -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';
Copy link
Collaborator

Choose a reason for hiding this comment

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

Suggested change
import { of } from 'rxjs';


jest.mock('../../../../opensearch_dashboards_react/public', () => ({
useOpenSearchDashboards: jest.fn(),
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -44,7 +44,7 @@ export const QueryLanguageSelector = (props: QueryLanguageSelectorProps) => {
: undefined;

useEffect(() => {
const subscription = queryString.getUpdates$().subscribe((query: Query) => {
const subscription = queryString.getUpdates$(0).subscribe((query: Query) => {
if (query.language !== currentLanguage) {
setCurrentLanguage(query.language);
}
Expand Down
7 changes: 3 additions & 4 deletions src/plugins/data/public/ui/query_editor/query_editor.tsx
Original file line number Diff line number Diff line change
Expand Up @@ -240,11 +240,10 @@ export default class QueryEditorUI extends Component<Props, State> {

if (!isEqual(this.props.query.dataset, prevQuery.dataset)) {
if (this.inputRef) {
const newQuery = this.queryString.getInitialQuery();
const newQueryString = newQuery.query;
if (this.inputRef.getValue() !== newQueryString) {
const newQueryString = this.props.query.query;
if (this.inputRef.getValue() !== newQueryString && typeof newQueryString === 'string') {
ashwin-pc marked this conversation as resolved.
Show resolved Hide resolved
Copy link
Collaborator

Choose a reason for hiding this comment

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

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

this.inputRef.setValue(newQueryString);
this.onSubmit(newQuery);
this.onSubmit(this.props.query);
}
}
}
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -54,6 +54,7 @@
defaultMessage: 'New Search',
}),
run() {
query.filterManager.setFilters([]); // resetting the filters while we are loading a new search

Check warning on line 57 in src/plugins/discover/public/application/components/top_nav/get_top_nav_links.tsx

View check run for this annotation

Codecov / codecov/patch

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

Added line #L57 was not covered by tests
core.application.navigateToApp('discover', {
path: '#/',
});
Expand Down Expand Up @@ -306,6 +307,7 @@
defaultMessage: 'New',
}),
run() {
query.filterManager.setFilters([]); // resetting the filters while we are loading a new search

Check warning on line 310 in src/plugins/discover/public/application/components/top_nav/get_top_nav_links.tsx

View check run for this annotation

Codecov / codecov/patch

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

Added line #L310 was not covered by tests
LDrago27 marked this conversation as resolved.
Show resolved Hide resolved
core.application.navigateToApp('discover', {
path: '#/',
});
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -92,6 +92,7 @@
},
]}
onChoose={(id) => {
data.query.filterManager.setFilters([]); // resetting the filters

Check warning on line 95 in src/plugins/discover/public/application/components/top_nav/open_search_panel.tsx

View check run for this annotation

Codecov / codecov/patch

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

Added line #L95 was not covered by tests
LDrago27 marked this conversation as resolved.
Show resolved Hide resolved
application.navigateToApp('discover', { path: `#/view/${id}` });
onClose();
}}
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -11,7 +11,11 @@ import { RootState, DefaultViewState } from '../../../../../data_explorer/public
import { buildColumns } from '../columns';
import * as utils from './common';
import { SortOrder } from '../../../saved_searches/types';
import { DEFAULT_COLUMNS_SETTING, PLUGIN_ID } from '../../../../common';
import {
DEFAULT_COLUMNS_SETTING,
PLUGIN_ID,
QUERY_ENHANCEMENT_ENABLED_SETTING,
} from '../../../../common';

export interface DiscoverState {
/**
Expand Down Expand Up @@ -88,9 +92,12 @@ export const getPreloadedState = async ({
preloadedState.state.sort = savedSearchInstance.sort;
preloadedState.state.savedSearch = savedSearchInstance.id;
const indexPatternId = savedSearchInstance.searchSource.getField('index')?.id;

// If the query enhancement is enabled don't add the indexpattern id to the root since they will be migrated into the _q state
preloadedState.root = {
metadata: {
indexPattern: indexPatternId,
...(indexPatternId &&
!config.get(QUERY_ENHANCEMENT_ENABLED_SETTING) && { indexPattern: indexPatternId }),
view: PLUGIN_ID,
},
};
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -12,7 +12,7 @@
import { useLocation } from 'react-router-dom';
import { RequestAdapter } from '../../../../../inspector/public';
import { DiscoverViewServices } from '../../../build_services';
import { search } from '../../../../../data/public';
import { QueryState, search } from '../../../../../data/public';
import { validateTimeRange } from '../../helpers/validate_time_range';
import { updateSearchSource } from './update_search_source';
import { useIndexPattern } from './use_index_pattern';
Expand Down Expand Up @@ -354,16 +354,21 @@
const savedSearchInstance = await getSavedSearchById(savedSearchId);
setSavedSearch(savedSearchInstance);

// if saved search does not exist, do not atempt to sync filters and query from savedObject
if (!savedSearch) {
// if saved search does not exist, or the URL has filter tyhen don't sync the saved search state with that
if (!savedSearchInstance || !savedSearchId) {
return;
}

// sync initial app filters from savedObject to filterManager
const filters = cloneDeep(savedSearchInstance.searchSource.getOwnField('filter'));
// Sync Query from the saved search
const query =
savedSearchInstance.searchSource.getField('query') ||
data.query.queryString.getDefaultQuery();

data.query.queryString.setQuery(query);

Check warning on line 367 in src/plugins/discover/public/application/view_components/utils/use_search.ts

View check run for this annotation

Codecov / codecov/patch

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

Added line #L367 was not covered by tests

// Sync Filters from the saved search
const filters = cloneDeep(savedSearchInstance.searchSource.getOwnField('filter'));

Check warning on line 370 in src/plugins/discover/public/application/view_components/utils/use_search.ts

View check run for this annotation

Codecov / codecov/patch

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

Added line #L370 was not covered by tests

const actualFilters = [];

if (filters !== undefined) {
Expand All @@ -373,8 +378,11 @@
}
}

filterManager.setAppFilters(actualFilters);
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 (!Array.isArray(urlFilters) || urlFilters.length === 0) {
filterManager.setAppFilters(actualFilters);

Check warning on line 384 in src/plugins/discover/public/application/view_components/utils/use_search.ts

View check run for this annotation

Codecov / codecov/patch

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

Added line #L384 was not covered by tests
Copy link
Collaborator

Choose a reason for hiding this comment

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

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

}

if (savedSearchInstance?.id) {
chrome.recentlyAccessed.add(
Expand Down
2 changes: 1 addition & 1 deletion src/plugins/query_enhancements/public/plugin.tsx
Original file line number Diff line number Diff line change
Expand Up @@ -96,7 +96,7 @@ export class QueryEnhancementsPlugin
title: 'SQL',
search: sqlSearchInterceptor,
getQueryString: (query: Query) => {
return `SELECT * FROM ${queryString.getQuery().dataset?.title} LIMIT 10`;
return `SELECT * FROM ${query.dataset?.title} LIMIT 10`;
},
fields: {
filterable: false,
Expand Down
Loading