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

[Discover] Migrate from index to dataSource in app state #182321

Merged
merged 10 commits into from
May 15, 2024
22 changes: 21 additions & 1 deletion src/plugins/discover/common/app_locator.test.ts
Original file line number Diff line number Diff line change
Expand Up @@ -15,6 +15,7 @@ import { mockStorage } from '@kbn/kibana-utils-plugin/public/storage/hashed_item
import { FilterStateStore } from '@kbn/es-query';
import { DiscoverAppLocatorDefinition } from './app_locator';
import { SerializableRecord } from '@kbn/utility-types';
import { createDataViewDataSource, createEsqlDataSource } from './data_sources';

const dataViewId: string = 'c367b774-a4c2-11ea-bb37-0242ac130002';
const savedSearchId: string = '571aaf70-4c88-11e8-b3d7-01146121b73d';
Expand Down Expand Up @@ -63,7 +64,7 @@ describe('Discover url generator', () => {
const { _a, _g } = getStatesFromKbnUrl(path, ['_a', '_g']);

expect(_a).toEqual({
index: dataViewId,
dataSource: createDataViewDataSource({ dataViewId }),
});
expect(_g).toEqual(undefined);
});
Expand Down Expand Up @@ -104,6 +105,25 @@ describe('Discover url generator', () => {
expect(_g).toEqual(undefined);
});

test('can specify an ES|QL query', async () => {
const { locator } = await setup();
const { path } = await locator.getLocation({
dataViewId,
query: {
esql: 'SELECT * FROM test',
},
});
const { _a, _g } = getStatesFromKbnUrl(path, ['_a', '_g']);

expect(_a).toEqual({
dataSource: createEsqlDataSource(),
query: {
esql: 'SELECT * FROM test',
},
});
expect(_g).toEqual(undefined);
});

test('can specify local and global filters', async () => {
const { locator } = await setup();
const { path } = await locator.getLocation({
Expand Down
25 changes: 8 additions & 17 deletions src/plugins/discover/common/app_locator.ts
Original file line number Diff line number Diff line change
Expand Up @@ -7,13 +7,15 @@
*/

import type { SerializableRecord } from '@kbn/utility-types';
import type { Filter, TimeRange, Query, AggregateQuery } from '@kbn/es-query';
import { Filter, TimeRange, Query, AggregateQuery, isOfAggregateQueryType } from '@kbn/es-query';
import type { GlobalQueryStateFromUrl, RefreshInterval } from '@kbn/data-plugin/public';
import type { LocatorDefinition, LocatorPublic } from '@kbn/share-plugin/public';
import type { DiscoverGridSettings } from '@kbn/saved-search-plugin/common';
import { DataViewSpec } from '@kbn/data-views-plugin/common';
import { setStateToKbnUrl } from '@kbn/kibana-utils-plugin/common';
import { VIEW_MODE } from './constants';
import type { DiscoverAppState } from '../public';
import { createDataViewDataSource, createEsqlDataSource } from './data_sources';

export const DISCOVER_APP_LOCATOR = 'DISCOVER_APP_LOCATOR';

Expand Down Expand Up @@ -150,32 +152,21 @@ export class DiscoverAppLocatorDefinition implements LocatorDefinition<DiscoverA
isAlertResults,
} = params;
const savedSearchPath = savedSearchId ? `view/${encodeURIComponent(savedSearchId)}` : '';
const appState: {
query?: Query | AggregateQuery;
filters?: Filter[];
index?: string;
columns?: string[];
grid?: DiscoverGridSettings;
interval?: string;
sort?: string[][];
savedQuery?: string;
viewMode?: string;
hideAggregatedPreview?: boolean;
breakdownField?: string;
} = {};
const appState: Partial<DiscoverAppState> = {};
const queryState: GlobalQueryStateFromUrl = {};
const { isFilterPinned } = await import('@kbn/es-query');

if (query) appState.query = query;
if (filters && filters.length) appState.filters = filters?.filter((f) => !isFilterPinned(f));
if (indexPatternId) appState.index = indexPatternId;
if (dataViewId) appState.index = dataViewId;
if (indexPatternId)
appState.dataSource = createDataViewDataSource({ dataViewId: indexPatternId });
if (dataViewId) appState.dataSource = createDataViewDataSource({ dataViewId });
if (isOfAggregateQueryType(query)) appState.dataSource = createEsqlDataSource();
if (columns) appState.columns = columns;
if (grid) appState.grid = grid;
if (savedQuery) appState.savedQuery = savedQuery;
if (sort) appState.sort = sort;
if (interval) appState.interval = interval;

if (timeRange) queryState.time = timeRange;
if (filters && filters.length) queryState.filters = filters?.filter((f) => isFilterPinned(f));
if (refreshInterval) queryState.refreshInterval = refreshInterval;
Expand Down
10 changes: 10 additions & 0 deletions src/plugins/discover/common/data_sources/index.ts
Original file line number Diff line number Diff line change
@@ -0,0 +1,10 @@
/*
* Copyright Elasticsearch B.V. and/or licensed to Elasticsearch B.V. under one
* or more contributor license agreements. Licensed under the Elastic License
* 2.0 and the Server Side Public License, v 1; you may not use this file except
* in compliance with, at your election, the Elastic License 2.0 or the Server
* Side Public License, v 1.
*/

export * from './utils';
export * from './types';
23 changes: 23 additions & 0 deletions src/plugins/discover/common/data_sources/types.ts
Original file line number Diff line number Diff line change
@@ -0,0 +1,23 @@
/*
* Copyright Elasticsearch B.V. and/or licensed to Elasticsearch B.V. under one
* or more contributor license agreements. Licensed under the Elastic License
* 2.0 and the Server Side Public License, v 1; you may not use this file except
* in compliance with, at your election, the Elastic License 2.0 or the Server
* Side Public License, v 1.
*/

export enum DataSourceType {
DataView = 'dataView',
Esql = 'esql',
}

export interface DataViewDataSource {
type: DataSourceType.DataView;
dataViewId: string;
}

export interface EsqlDataSource {
type: DataSourceType.Esql;
}

export type DiscoverDataSource = DataViewDataSource | EsqlDataSource;
27 changes: 27 additions & 0 deletions src/plugins/discover/common/data_sources/utils.ts
Original file line number Diff line number Diff line change
@@ -0,0 +1,27 @@
/*
* Copyright Elasticsearch B.V. and/or licensed to Elasticsearch B.V. under one
* or more contributor license agreements. Licensed under the Elastic License
* 2.0 and the Server Side Public License, v 1; you may not use this file except
* in compliance with, at your election, the Elastic License 2.0 or the Server
* Side Public License, v 1.
*/

import { DataSourceType, DataViewDataSource, DiscoverDataSource, EsqlDataSource } from './types';

export const createDataViewDataSource = ({
dataViewId,
}: {
dataViewId: string;
}): DataViewDataSource => ({
type: DataSourceType.DataView,
dataViewId,
});

export const createEsqlDataSource = (): EsqlDataSource => ({
type: DataSourceType.Esql,
});

export const isDataSourceType = <T extends DataSourceType>(
jughosta marked this conversation as resolved.
Show resolved Hide resolved
dataSource: DiscoverDataSource | undefined,
type: T
): dataSource is Extract<DiscoverDataSource, { type: T }> => dataSource?.type === type;
Original file line number Diff line number Diff line change
Expand Up @@ -25,6 +25,7 @@ import { DiscoverAppState } from '../../state_management/discover_app_state_cont
import { DiscoverCustomization, DiscoverCustomizationProvider } from '../../../../customizations';
import { createCustomizationService } from '../../../../customizations/customization_service';
import { DiscoverGrid } from '../../../../components/discover_grid';
import { createDataViewDataSource } from '../../../../../common/data_sources';

const customisationService = createCustomizationService();

Expand All @@ -39,7 +40,9 @@ async function mountComponent(fetchStatus: FetchStatus, hits: EsHitRecord[]) {
result: hits.map((hit) => buildDataTableRecord(hit, dataViewMock)),
}) as DataDocuments$;
const stateContainer = getDiscoverStateMock({});
stateContainer.appState.update({ index: dataViewMock.id });
stateContainer.appState.update({
dataSource: createDataViewDataSource({ dataViewId: dataViewMock.id! }),
});
stateContainer.dataState.data$.documents$ = documents$;

const props = {
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -108,46 +108,27 @@ function DiscoverDocumentsComponent({
const documents$ = stateContainer.dataState.data$.documents$;
const savedSearch = useSavedSearchInitial();
const { dataViews, capabilities, uiSettings, uiActions } = services;
const [
query,
sort,
rowHeight,
headerRowHeight,
rowsPerPage,
grid,
columns,
index,
sampleSizeState,
] = useAppStateSelector((state) => {
return [
state.query,
state.sort,
state.rowHeight,
state.headerRowHeight,
state.rowsPerPage,
state.grid,
state.columns,
state.index,
state.sampleSize,
];
});
const setExpandedDoc = useCallback(
(doc: DataTableRecord | undefined) => {
stateContainer.internalState.transitions.setExpandedDoc(doc);
},
[stateContainer]
);

const [query, sort, rowHeight, headerRowHeight, rowsPerPage, grid, columns, sampleSizeState] =
useAppStateSelector((state) => {
return [
state.query,
state.sort,
state.rowHeight,
state.headerRowHeight,
state.rowsPerPage,
state.grid,
state.columns,
state.sampleSize,
];
});
Comment on lines +111 to +123
Copy link
Member

Choose a reason for hiding this comment

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

let me share a thought that doesn't need to be applied here. We are subscribing to AppState which is synced to URL, AppState is also applied to the savedSearch state. We are subscribing to too much state of various sources in the UI, so this one could be changed to use useSavedSearch, so there are "just" dataState, savedSearchState and internalState being used.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Agreed about subscribing to too much. And tbh the redundancy in our state and having to keep it all in sync is itself an issue too. Ideally I'd like to simplify the state management to remove the duplication and need for explicit syncing, and work with a representation less coupled to the URL and saved search model, but of course lots of other things to take care of first 😅

const expandedDoc = useInternalStateSelector((state) => state.expandedDoc);

const isTextBasedQuery = useMemo(() => getRawRecordType(query) === RecordRawType.PLAIN, [query]);
const useNewFieldsApi = useMemo(() => !uiSettings.get(SEARCH_FIELDS_FROM_SOURCE), [uiSettings]);
const hideAnnouncements = useMemo(() => uiSettings.get(HIDE_ANNOUNCEMENTS), [uiSettings]);
const isLegacy = useMemo(
() => isLegacyTableEnabled({ uiSettings, isTextBasedQueryMode: isTextBasedQuery }),
[uiSettings, isTextBasedQuery]
);

const documentState = useDataState(documents$);
const isDataLoading =
documentState.fetchStatus === FetchStatus.LOADING ||
Expand All @@ -162,7 +143,8 @@ function DiscoverDocumentsComponent({
// 4. since the new sort by field isn't available in currentColumns EuiDataGrid is emitting a 'onSort', which is unsorting the grid
// 5. this is propagated to Discover's URL and causes an unwanted change of state to an unsorted state
// This solution switches to the loading state in this component when the URL index doesn't match the dataView.id
const isDataViewLoading = !isTextBasedQuery && dataView.id && index !== dataView.id;
const isDataViewLoading =
useInternalStateSelector((state) => state.isDataViewLoading) && !isTextBasedQuery;
const isEmptyDataResult =
isTextBasedQuery || !documentState.result || documentState.result.length === 0;
const rows = useMemo(() => documentState.result || [], [documentState.result]);
Expand All @@ -189,6 +171,13 @@ function DiscoverDocumentsComponent({
sort,
});

const setExpandedDoc = useCallback(
(doc: DataTableRecord | undefined) => {
stateContainer.internalState.transitions.setExpandedDoc(doc);
},
[stateContainer]
);

const onResizeDataGrid = useCallback(
(colSettings) => onResize(colSettings, stateContainer),
[stateContainer]
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -34,13 +34,14 @@ import { getDiscoverStateMock } from '../../../../__mocks__/discover_state.mock'
import { DiscoverMainProvider } from '../../state_management/discover_state_provider';
import { act } from 'react-dom/test-utils';
import { PanelsToggle } from '../../../../components/panels_toggle';
import { createDataViewDataSource } from '../../../../../common/data_sources';

function getStateContainer(savedSearch?: SavedSearch) {
const stateContainer = getDiscoverStateMock({ isTimeBased: true, savedSearch });
const dataView = savedSearch?.searchSource?.getField('index') as DataView;

stateContainer.appState.update({
index: dataView?.id,
dataSource: createDataViewDataSource({ dataViewId: dataView?.id! }),
interval: 'auto',
hideChart: false,
});
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -39,6 +39,7 @@ import { DiscoverMainProvider } from '../../state_management/discover_state_prov
import { act } from 'react-dom/test-utils';
import { ErrorCallout } from '../../../../components/common/error_callout';
import { PanelsToggle } from '../../../../components/panels_toggle';
import { createDataViewDataSource } from '../../../../../common/data_sources';

jest.mock('@elastic/eui', () => ({
...jest.requireActual('@elastic/eui'),
Expand Down Expand Up @@ -106,7 +107,11 @@ async function mountComponent(

session.getSession$.mockReturnValue(new BehaviorSubject('123'));

stateContainer.appState.update({ index: dataView.id, interval: 'auto', query });
stateContainer.appState.update({
dataSource: createDataViewDataSource({ dataViewId: dataView.id! }),
interval: 'auto',
query,
});
stateContainer.internalState.transitions.setDataView(dataView);

const props = {
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -329,7 +329,6 @@ export function DiscoverLayout({ stateContainer }: DiscoverLayoutProps) {
<TopNavMemoized
savedQuery={savedQuery}
stateContainer={stateContainer}
updateQuery={stateContainer.actions.onUpdateQuery}
textBasedLanguageModeErrors={textBasedLanguageModeErrors}
textBasedLanguageModeWarning={textBasedLanguageModeWarning}
onFieldEdited={onFieldEdited}
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -35,6 +35,7 @@ import { DiscoverMainProvider } from '../../state_management/discover_state_prov
import { getDiscoverStateMock } from '../../../../__mocks__/discover_state.mock';
import { PanelsToggle } from '../../../../components/panels_toggle';
import type { Storage } from '@kbn/kibana-utils-plugin/public';
import { createDataViewDataSource } from '../../../../../common/data_sources';

const mountComponent = async ({
hideChart = false,
Expand Down Expand Up @@ -97,7 +98,7 @@ const mountComponent = async ({
.getState()
.searchSource.getField('index') as DataView;
stateContainer.appState.update({
index: dataView?.id!,
dataSource: createDataViewDataSource({ dataViewId: dataView.id! }),
interval: 'auto',
hideChart,
columns: [],
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -73,7 +73,6 @@ function getProps(
return {
stateContainer,
savedQuery: '',
updateQuery: jest.fn(),
onFieldEdited: jest.fn(),
};
}
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -7,7 +7,6 @@
*/

import React, { useCallback, useEffect, useMemo, useRef } from 'react';
import type { AggregateQuery, Query, TimeRange } from '@kbn/es-query';
import { type DataView, DataViewType } from '@kbn/data-views-plugin/public';
import { DataViewPickerProps } from '@kbn/unified-search-plugin/public';
import { ENABLE_ESQL } from '@kbn/esql-utils';
Expand All @@ -29,10 +28,6 @@ import { useDiscoverTopNav } from './use_discover_topnav';

export interface DiscoverTopNavProps {
savedQuery?: string;
updateQuery: (
payload: { dateRange: TimeRange; query?: Query | AggregateQuery },
isUpdate?: boolean
) => void;
stateContainer: DiscoverStateContainer;
textBasedLanguageModeErrors?: Error;
textBasedLanguageModeWarning?: string;
Expand All @@ -44,7 +39,6 @@ export interface DiscoverTopNavProps {
export const DiscoverTopNav = ({
savedQuery,
stateContainer,
updateQuery,
textBasedLanguageModeErrors,
textBasedLanguageModeWarning,
onFieldEdited,
Expand Down Expand Up @@ -241,7 +235,7 @@ export const DiscoverTopNav = ({
{...topNavProps}
appName="discover"
indexPatterns={[dataView]}
onQuerySubmit={updateQuery}
onQuerySubmit={stateContainer.actions.onUpdateQuery}
onCancel={onCancelClick}
isLoading={isLoading}
onSavedQueryIdChange={updateSavedQueryId}
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -10,6 +10,7 @@ import { i18n } from '@kbn/i18n';
import type { DataView } from '@kbn/data-views-plugin/public';
import type { TopNavMenuData } from '@kbn/navigation-plugin/public';
import { setStateToKbnUrl } from '@kbn/kibana-utils-plugin/public';
import { omit } from 'lodash';
import type { DiscoverAppLocatorParams } from '../../../../../common';
import { showOpenSearchPanel } from './show_open_search_panel';
import { getSharingData, showPublicUrlSwitch } from '../../../../utils/get_sharing_data';
Expand Down Expand Up @@ -138,7 +139,7 @@ export const getTopNavLinks = ({

// Share -> Get links -> Snapshot
const params: DiscoverAppLocatorParams = {
...appState,
...omit(appState, 'dataSource'),
...(savedSearch.id ? { savedSearchId: savedSearch.id } : {}),
...(dataView?.isPersisted()
? { dataViewId: dataView?.id }
Expand Down