Skip to content

Commit

Permalink
[Discover] Migrate from index to dataSource in app state (#182321)
Browse files Browse the repository at this point in the history
## Summary

This PR replaces the `index` property of Discover's app state with a new
`dataSource` property, containing two initial types: `dataView` and
`esql`. The new data source abstraction will be used in the One Discover
data source context resolution process, as well as preparing Discover to
support additional data sources as needed in the future. Additionally,
it creates a clearer division in the Discover state between "data view
mode" and "ES|QL mode", where previously this was implicit based on
whether the `query` property was an ES|QL query vs KQL or Lucene.

The goal of this PR is to add initial support for the `dataSource`
property without introducing broader changes to the Discover state
management. However, these changes open up opportunities for future
improvements to simplify the state management, such as checking the data
source type to determine which mode Discover is in instead of always
checking the query directly. These types of enhancements can be built on
this foundation in followup PRs.

Part of #181963.

### Checklist

- [ ] Any text added follows [EUI's writing
guidelines](https://elastic.github.io/eui/#/guidelines/writing), uses
sentence case text and includes [i18n
support](https://github.com/elastic/kibana/blob/main/packages/kbn-i18n/README.md)
- [ ]
[Documentation](https://www.elastic.co/guide/en/kibana/master/development-documentation.html)
was added for features that require explanation or tutorials
- [x] [Unit or functional
tests](https://www.elastic.co/guide/en/kibana/master/development-tests.html)
were updated or added to match the most common scenarios
- [ ] [Flaky Test
Runner](https://ci-stats.kibana.dev/trigger_flaky_test_runner/1) was
used on any tests changed
- [ ] Any UI touched in this PR is usable by keyboard only (learn more
about [keyboard accessibility](https://webaim.org/techniques/keyboard/))
- [ ] Any UI touched in this PR does not create any new axe failures
(run axe in browser:
[FF](https://addons.mozilla.org/en-US/firefox/addon/axe-devtools/),
[Chrome](https://chrome.google.com/webstore/detail/axe-web-accessibility-tes/lhdoppojpmngadmnindnejefpokejbdd?hl=en-US))
- [ ] If a plugin configuration key changed, check if it needs to be
allowlisted in the cloud and added to the [docker
list](https://github.com/elastic/kibana/blob/main/src/dev/build/tasks/os_packages/docker_generator/resources/base/bin/kibana-docker)
- [ ] This renders correctly on smaller devices using a responsive
layout. (You can test this [in your
browser](https://www.browserstack.com/guide/responsive-testing-on-local-server))
- [ ] This was checked for [cross-browser
compatibility](https://www.elastic.co/support/matrix#matrix_browsers)

### For maintainers

- [ ] This was checked for breaking API changes and was [labeled
appropriately](https://www.elastic.co/guide/en/kibana/master/contributing.html#kibana-release-notes-process)
  • Loading branch information
davismcphee authored May 15, 2024
1 parent 70b1879 commit 872f010
Show file tree
Hide file tree
Showing 33 changed files with 544 additions and 220 deletions.
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>(
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,
];
});
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
Loading

0 comments on commit 872f010

Please sign in to comment.