From 4e80b8210bd6a07b071e1c0af871cc3dcbb031bc Mon Sep 17 00:00:00 2001 From: Julia Rechkunova Date: Tue, 9 May 2023 13:05:57 +0200 Subject: [PATCH 01/49] [Discover] Improve date_nanos support for "search_after" pagination --- .../sorting/get_sort_for_search_source.ts | 1 + .../context/hooks/use_context_app_fetch.tsx | 11 ++++--- .../application/context/services/context.ts | 14 ++++----- .../utils/get_es_query_search_after.ts | 29 +++++-------------- .../context/utils/get_es_query_sort.ts | 11 +++++-- 5 files changed, 29 insertions(+), 37 deletions(-) diff --git a/src/plugins/discover/common/utils/sorting/get_sort_for_search_source.ts b/src/plugins/discover/common/utils/sorting/get_sort_for_search_source.ts index bcf0ccf4d0e30..a133a0dac9619 100644 --- a/src/plugins/discover/common/utils/sorting/get_sort_for_search_source.ts +++ b/src/plugins/discover/common/utils/sorting/get_sort_for_search_source.ts @@ -38,6 +38,7 @@ export function getSortForSearchSource( [timeFieldName]: { order: sortPair[timeFieldName], numeric_type: 'date_nanos', + format: 'strict_date_optional_time_nanos', }, } as EsQuerySortValue; } diff --git a/src/plugins/discover/public/application/context/hooks/use_context_app_fetch.tsx b/src/plugins/discover/public/application/context/hooks/use_context_app_fetch.tsx index 75ea1d1a56130..3a9fa8ff73151 100644 --- a/src/plugins/discover/public/application/context/hooks/use_context_app_fetch.tsx +++ b/src/plugins/discover/public/application/context/hooks/use_context_app_fetch.tsx @@ -23,6 +23,7 @@ import { AppState } from '../services/context_state'; import { getFirstSortableField } from '../utils/sorting'; import { useDiscoverServices } from '../../../hooks/use_discover_services'; import type { DataTableRecord } from '../../../types'; +import { getEsQuerySort } from '../utils/get_es_query_sort'; const createError = (statusKey: string, reason: FailureReason, error?: Error) => ({ [statusKey]: { value: LoadingStatus.FAILED, error, reason }, @@ -91,10 +92,12 @@ export function useContextAppFetch({ try { setState({ anchorStatus: { value: LoadingStatus.LOADING } }); - const sort = [ - { [dataView.timeFieldName!]: SortDirection.desc }, - { [tieBreakerField]: SortDirection.desc }, - ]; + const sort = getEsQuerySort( + dataView.timeFieldName!, + tieBreakerField, + SortDirection.desc, + dataView.isTimeNanosBased() + ); const anchor = await fetchAnchor(anchorId, dataView, searchSource, sort, useNewFieldsApi); setState({ anchor, anchorStatus: { value: LoadingStatus.LOADED } }); return anchor; diff --git a/src/plugins/discover/public/application/context/services/context.ts b/src/plugins/discover/public/application/context/services/context.ts index fc221200a5344..a697444fbf6dd 100644 --- a/src/plugins/discover/public/application/context/services/context.ts +++ b/src/plugins/discover/public/application/context/services/context.ts @@ -73,17 +73,15 @@ export async function fetchSurroundingDocs( break; } - const searchAfter = getEsQuerySearchAfter( - type, - documents, + const searchAfter = getEsQuerySearchAfter(type, documents, timeField, anchor); + + const sort = getEsQuerySort( timeField, - anchor, - nanos, - useNewFieldsApi + tieBreakerField, + sortDirToApply, + dataView.isTimeNanosBased() ); - const sort = getEsQuerySort(timeField, tieBreakerField, sortDirToApply, nanos); - const hits = await fetchHitsInInterval( searchSource, timeField, diff --git a/src/plugins/discover/public/application/context/utils/get_es_query_search_after.ts b/src/plugins/discover/public/application/context/utils/get_es_query_search_after.ts index bfec54c61e856..ab2a84c6a535e 100644 --- a/src/plugins/discover/public/application/context/utils/get_es_query_search_after.ts +++ b/src/plugins/discover/public/application/context/utils/get_es_query_search_after.ts @@ -19,33 +19,18 @@ export function getEsQuerySearchAfter( type: SurrDocType, documents: DataTableRecord[], timeFieldName: string, - anchor: DataTableRecord, - nanoSeconds: string, - useNewFieldsApi?: boolean + anchor: DataTableRecord ): EsQuerySearchAfter { if (documents.length) { // already surrounding docs -> first or last record is used const afterTimeRecIdx = type === SurrDocType.SUCCESSORS && documents.length ? documents.length - 1 : 0; - const afterTimeDoc = documents[afterTimeRecIdx]; - const afterTimeDocRaw = afterTimeDoc.raw; - let afterTimeValue = afterTimeDocRaw.sort?.[0] as string | number; - if (nanoSeconds) { - afterTimeValue = useNewFieldsApi - ? afterTimeDocRaw.fields?.[timeFieldName][0] - : afterTimeDocRaw._source?.[timeFieldName]; - } - return [afterTimeValue, afterTimeDoc.raw.sort?.[1] as string | number]; + const afterTimeDocRaw = documents[afterTimeRecIdx].raw; + return [ + afterTimeDocRaw.sort?.[0] as string | number, + afterTimeDocRaw.sort?.[1] as string | number, + ]; } - // if data_nanos adapt timestamp value for sorting, since numeric value was rounded by browser // ES search_after also works when number is provided as string - const searchAfter = new Array(2) as EsQuerySearchAfter; - searchAfter[0] = anchor.raw.sort?.[0] as string | number; - if (nanoSeconds) { - searchAfter[0] = useNewFieldsApi - ? anchor.raw.fields?.[timeFieldName][0] - : anchor.raw._source?.[timeFieldName]; - } - searchAfter[1] = anchor.raw.sort?.[1] as string | number; - return searchAfter; + return [anchor.raw.sort?.[0] as string | number, anchor.raw.sort?.[1] as string | number]; } diff --git a/src/plugins/discover/public/application/context/utils/get_es_query_sort.ts b/src/plugins/discover/public/application/context/utils/get_es_query_sort.ts index 2bbea70e16160..2731404ac6d18 100644 --- a/src/plugins/discover/public/application/context/utils/get_es_query_sort.ts +++ b/src/plugins/discover/public/application/context/utils/get_es_query_sort.ts @@ -14,19 +14,24 @@ import type { EsQuerySortValue, SortDirection } from '@kbn/data-plugin/public'; * @param timeField * @param tieBreakerField * @param sortDir - * @param nanos + * @param isTimeNanosBased */ export function getEsQuerySort( timeField: string, tieBreakerField: string, sortDir: SortDirection, - nanos?: string + isTimeNanosBased: boolean ): [EsQuerySortValue, EsQuerySortValue] { return [ { [timeField]: { order: sortDir, - format: nanos ? 'strict_date_optional_time_nanos' : 'strict_date_optional_time', + ...(isTimeNanosBased + ? { + format: 'strict_date_optional_time_nanos', + numeric_type: 'date_nanos', + } + : { format: 'strict_date_optional_time' }), }, }, { [tieBreakerField]: sortDir }, From e4e12693d82c1b53c1dfe3123cc5fce0fd004f5d Mon Sep 17 00:00:00 2001 From: Julia Rechkunova Date: Thu, 11 May 2023 17:40:33 +0200 Subject: [PATCH 02/49] [Discover] Allow to fetch more documents on Discover page --- .../components/layout/discover_documents.tsx | 11 ++ .../services/discover_data_state_container.ts | 56 +++++++-- .../application/main/utils/fetch_all.ts | 69 +++++++++++ .../discover_grid/discover_grid.scss | 4 + .../discover_grid/discover_grid.tsx | 34 +++--- .../discover_grid/discover_grid_footer.tsx | 115 ++++++++++++++++++ 6 files changed, 264 insertions(+), 25 deletions(-) create mode 100644 src/plugins/discover/public/components/discover_grid/discover_grid_footer.tsx diff --git a/src/plugins/discover/public/application/main/components/layout/discover_documents.tsx b/src/plugins/discover/public/application/main/components/layout/discover_documents.tsx index 3070f447712c5..5658c287f0a06 100644 --- a/src/plugins/discover/public/application/main/components/layout/discover_documents.tsx +++ b/src/plugins/discover/public/application/main/components/layout/discover_documents.tsx @@ -84,6 +84,7 @@ function DiscoverDocumentsComponent({ }) { const services = useDiscoverServices(); const documents$ = stateContainer.dataState.data$.documents$; + const totalHits$ = stateContainer.dataState.data$.totalHits$; const savedSearch = useSavedSearchInitial(); const { dataViews, capabilities, uiSettings } = services; const [query, sort, rowHeight, rowsPerPage, grid, columns, index] = useAppStateSelector( @@ -127,6 +128,14 @@ function DiscoverDocumentsComponent({ const isPlainRecord = useMemo(() => getRawRecordType(query) === RecordRawType.PLAIN, [query]); const rows = useMemo(() => documentState.result || [], [documentState.result]); + const totalHitsState = useDataState(totalHits$); + const canFetchMoreRecords = + !isPlainRecord && (totalHitsState.result || 0) > (documentState.result?.length || 0); + + const onFetchMoreRecords = useCallback(() => { + stateContainer.dataState.fetchMore(); + }, [stateContainer.dataState]); + const { columns: currentColumns, onAddColumn, @@ -258,6 +267,8 @@ function DiscoverDocumentsComponent({ savedSearchId={savedSearch.id} DocumentView={DiscoverGridFlyout} services={services} + canFetchMoreRecords={canFetchMoreRecords} + onFetchMoreRecords={onFetchMoreRecords} /> diff --git a/src/plugins/discover/public/application/main/services/discover_data_state_container.ts b/src/plugins/discover/public/application/main/services/discover_data_state_container.ts index 07e1f089bcd13..431e6e7352dfb 100644 --- a/src/plugins/discover/public/application/main/services/discover_data_state_container.ts +++ b/src/plugins/discover/public/application/main/services/discover_data_state_container.ts @@ -19,7 +19,7 @@ import { DiscoverSearchSessionManager } from './discover_search_session'; import { SEARCH_FIELDS_FROM_SOURCE, SEARCH_ON_PAGE_LOAD_SETTING } from '../../../../common'; import { FetchStatus } from '../../types'; import { validateTimeRange } from '../utils/validate_time_range'; -import { fetchAll } from '../utils/fetch_all'; +import { fetchAll, fetchMoreDocuments } from '../utils/fetch_all'; import { sendResetMsg } from '../hooks/use_saved_search_messages'; import { getFetch$ } from '../utils/get_fetch_observable'; import { DataTableRecord } from '../../../types'; @@ -36,7 +36,10 @@ export type DataDocuments$ = BehaviorSubject; export type DataTotalHits$ = BehaviorSubject; export type AvailableFields$ = BehaviorSubject; export type DataFetch$ = Observable<{ - reset: boolean; + options: { + reset: boolean; + fetchMore: boolean; + }; searchSessionId: string; }>; @@ -53,7 +56,7 @@ export enum RecordRawType { PLAIN = 'plain', } -export type DataRefetchMsg = 'reset' | undefined; +export type DataRefetchMsg = 'reset' | 'fetch_more' | undefined; export interface DataMsg { fetchStatus: FetchStatus; @@ -88,6 +91,10 @@ export interface DiscoverDataStateContainer { * Implicitly starting fetching data from ES */ fetch: () => void; + /** + * Fetch more data from ES + */ + fetchMore: () => void; /** * Observable emitting when a next fetch is triggered */ @@ -186,21 +193,20 @@ export function getDataStateContainer({ filter(() => validateTimeRange(timefilter.getTime(), toastNotifications)), tap(() => inspectorAdapters.requests.reset()), map((val) => ({ - reset: val === 'reset', + options: { + reset: val === 'reset', + fetchMore: val === 'fetch_more', + }, searchSessionId: searchSessionManager.getNextSearchSessionId(), })), share() ); let abortController: AbortController; + let abortControllerFetchMore: AbortController; function subscribe() { - const subscription = fetch$.subscribe(async ({ reset, searchSessionId }) => { - abortController?.abort(); - abortController = new AbortController(); - const prevAutoRefreshDone = autoRefreshDone; - - await fetchAll(dataSubjects, reset, { - abortController, + const subscription = fetch$.subscribe(async ({ options, searchSessionId }) => { + const commonFetchDeps = { initialFetchStatus: getInitialFetchStatus(), inspectorAdapters, searchSessionId, @@ -208,6 +214,28 @@ export function getDataStateContainer({ getAppState, savedSearch: getSavedSearch(), useNewFieldsApi: !uiSettings.get(SEARCH_FIELDS_FROM_SOURCE), + }; + + if (options.fetchMore) { + abortControllerFetchMore?.abort(); + abortControllerFetchMore = new AbortController(); + + await fetchMoreDocuments(dataSubjects, { + abortController: abortControllerFetchMore, + ...commonFetchDeps, + }); + return; + } + + abortControllerFetchMore?.abort(); + + abortController?.abort(); + abortController = new AbortController(); + const prevAutoRefreshDone = autoRefreshDone; + + await fetchAll(dataSubjects, options.reset, { + abortController, + ...commonFetchDeps, }); // If the autoRefreshCallback is still the same as when we started i.e. there was no newer call @@ -235,6 +263,11 @@ export function getDataStateContainer({ return refetch$; }; + const fetchMore = () => { + refetch$.next('fetch_more'); + return refetch$; + }; + const reset = (savedSearch: SavedSearch) => { const recordType = getRawRecordType(savedSearch.searchSource.getField('query')); sendResetMsg(dataSubjects, getInitialFetchStatus(), recordType); @@ -242,6 +275,7 @@ export function getDataStateContainer({ return { fetch: fetchQuery, + fetchMore, fetch$, data$: dataSubjects, refetch$, diff --git a/src/plugins/discover/public/application/main/utils/fetch_all.ts b/src/plugins/discover/public/application/main/utils/fetch_all.ts index f69399ab1d2bf..5c7df02e85956 100644 --- a/src/plugins/discover/public/application/main/utils/fetch_all.ts +++ b/src/plugins/discover/public/application/main/utils/fetch_all.ts @@ -131,6 +131,75 @@ export function fetchAll( } } +export function fetchMoreDocuments( + dataSubjects: SavedSearchData, + fetchDeps: FetchDeps +): Promise { + const { getAppState, services, savedSearch } = fetchDeps; + const searchSource = savedSearch.searchSource.createChild(); + + try { + const latestDocuments = dataSubjects.documents$.getValue().result || []; + const lastDocumentSort = latestDocuments[latestDocuments.length - 1]?.raw?.sort; + + if (!lastDocumentSort) { + return Promise.resolve(); + } + + // TODO: add tie breaker field + searchSource.setField('searchAfter', lastDocumentSort); + + const dataView = searchSource.getField('index')!; + const query = getAppState().query; + const recordRawType = getRawRecordType(query); + + if (recordRawType === RecordRawType.PLAIN) { + // not supported yet + // TODO? + return Promise.resolve(); + } + + // Update the base searchSource, base for all child fetches + updateVolatileSearchSource(searchSource, { + dataView, + services, + sort: getAppState().sort as SortOrder[], + }); + + // Mark subjects as loading + sendLoadingMsg(dataSubjects.documents$, { + recordRawType, + query, + result: dataSubjects.documents$.getValue().result, + }); + + // Start fetching all required requests + const response = fetchDocuments(searchSource, fetchDeps); + + // Handle results of the individual queries and forward the results to the corresponding dataSubjects + response + .then(({ records, textBasedQueryColumns }) => { + dataSubjects.documents$.next({ + fetchStatus: FetchStatus.COMPLETE, + result: [...(dataSubjects.documents$.getValue().result || []), ...records], + textBasedQueryColumns, + recordRawType, + query, + }); + }) + .catch((error) => { + // TODO? + // eslint-disable-next-line no-console + console.error(error); + }); + } catch (error) { + // eslint-disable-next-line no-console + console.error(error); + // We also want to return a resolved promise in an error case, since it just indicates we're done with querying. + } + return Promise.resolve(); +} + const fetchStatusByType = (subject: BehaviorSubject, type: string) => subject.pipe(map(({ fetchStatus }) => ({ type, fetchStatus }))); diff --git a/src/plugins/discover/public/components/discover_grid/discover_grid.scss b/src/plugins/discover/public/components/discover_grid/discover_grid.scss index a80f2cd6b5534..b9aa8a40c0b7f 100644 --- a/src/plugins/discover/public/components/discover_grid/discover_grid.scss +++ b/src/plugins/discover/public/components/discover_grid/discover_grid.scss @@ -75,6 +75,10 @@ padding: $euiSize / 2 $euiSize; margin-top: $euiSize / 4; text-align: center; + + &--withButton { + padding: 0 $euiSize; + } } .dscTable__flyoutHeader { diff --git a/src/plugins/discover/public/components/discover_grid/discover_grid.tsx b/src/plugins/discover/public/components/discover_grid/discover_grid.tsx index 35c51ab4c996c..9eaefdba66f45 100644 --- a/src/plugins/discover/public/components/discover_grid/discover_grid.tsx +++ b/src/plugins/discover/public/components/discover_grid/discover_grid.tsx @@ -46,6 +46,7 @@ import { SHOW_MULTIFIELDS, } from '../../../common'; import { DiscoverGridDocumentToolbarBtn } from './discover_grid_document_selection'; +import { DiscoverGridFooter } from './discover_grid_footer'; import { getShouldShowFieldHandler } from '../../utils/get_should_show_field_handler'; import type { DataTableRecord, ValueToStringConverter } from '../../types'; import { useRowHeightsOptions } from '../../hooks/use_row_heights_options'; @@ -205,6 +206,14 @@ export interface DiscoverGridProps { dataViewFieldEditor: DataViewFieldEditorStart; toastNotifications: ToastsStart; }; + /** + * Is number of fetched records less than the total number of found hits? + */ + canFetchMoreRecords?: boolean; + /** + * To fetch more + */ + onFetchMoreRecords?: () => void; } export const EuiDataGridMemoized = React.memo(EuiDataGrid); @@ -247,6 +256,8 @@ export const DiscoverGrid = ({ onFieldEdited, DocumentView, services, + canFetchMoreRecords, + onFetchMoreRecords, }: DiscoverGridProps) => { const { fieldFormats, toastNotifications, dataViewFieldEditor, uiSettings } = services; const dataGridRef = useRef(null); @@ -336,8 +347,6 @@ export const DiscoverGrid = ({ : undefined; }, [pagination, pageCount, isPaginationEnabled, onUpdateRowsPerPage]); - const isOnLastPage = paginationObj ? paginationObj.pageIndex === pageCount - 1 : false; - useEffect(() => { setPagination((paginationData) => paginationData.pageSize === currentPageSize @@ -386,7 +395,6 @@ export const DiscoverGrid = ({ /** * Render variables */ - const showDisclaimer = rowCount === sampleSize && isOnLastPage; const randomId = useMemo(() => htmlIdGenerator()(), []); const closeFieldEditor = useRef<() => void | undefined>(); @@ -615,17 +623,15 @@ export const DiscoverGrid = ({ gridStyle={GRID_STYLE} /> - {showDisclaimer && ( -

- -

- )} + {searchTitle && (

diff --git a/src/plugins/discover/public/components/discover_grid/discover_grid_footer.tsx b/src/plugins/discover/public/components/discover_grid/discover_grid_footer.tsx new file mode 100644 index 0000000000000..f0aa2b5c5eacd --- /dev/null +++ b/src/plugins/discover/public/components/discover_grid/discover_grid_footer.tsx @@ -0,0 +1,115 @@ +/* + * 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 React from 'react'; +import { FormattedMessage } from '@kbn/i18n-react'; +import classnames from 'classnames'; +import { EuiButtonEmpty } from '@elastic/eui'; + +export interface DiscoverGridFooterProps { + isLoading: boolean; + rowCount: number; + sampleSize: number; + pageIndex?: number; + pageCount: number; + canFetchMoreRecords?: boolean; + onFetchMoreRecords?: () => void; +} + +export const DiscoverGridFooter: React.FC = (props) => { + const { + isLoading, + rowCount, + sampleSize, + pageIndex, + pageCount, + canFetchMoreRecords, + onFetchMoreRecords, + } = props; + const isOnLastPage = pageIndex === pageCount - 1; + + if (!isOnLastPage) { + return null; + } + + // allow to fetch more records on Discover page + if (canFetchMoreRecords && onFetchMoreRecords) { + // TODO: define a limit + if (rowCount <= 3000 - sampleSize) { + return ( + + <> + {' '} + + + + + + ); + } + + return ; + } + + if (rowCount === sampleSize) { + // show only a message for embeddable + return ; + } + + return null; +}; + +interface DiscoverGridFooterContainerProps extends DiscoverGridFooterProps { + hasButton: boolean; +} + +const DiscoverGridFooterContainer: React.FC = ({ + hasButton, + rowCount, + sampleSize, + children, +}) => { + return ( +

+ {hasButton ? ( + + ) : ( + + )} + {children} +

+ ); +}; From d218e6eeb06d77ade6b0bd07ac07429e84e1a402 Mon Sep 17 00:00:00 2001 From: Julia Rechkunova Date: Fri, 12 May 2023 10:51:54 +0200 Subject: [PATCH 03/49] [Discover] Fix when the footer should appear --- .../components/layout/discover_documents.tsx | 20 ++++++++---- .../main/hooks/use_saved_search_messages.ts | 12 +++++++ .../application/main/utils/fetch_all.ts | 7 ++--- .../discover/public/application/types.ts | 1 + .../discover_grid/discover_grid.tsx | 31 ++++++++++++------- .../discover_grid/discover_grid_footer.tsx | 17 +++++----- 6 files changed, 56 insertions(+), 32 deletions(-) diff --git a/src/plugins/discover/public/application/main/components/layout/discover_documents.tsx b/src/plugins/discover/public/application/main/components/layout/discover_documents.tsx index 5658c287f0a06..45ff0c6cc081d 100644 --- a/src/plugins/discover/public/application/main/components/layout/discover_documents.tsx +++ b/src/plugins/discover/public/application/main/components/layout/discover_documents.tsx @@ -116,6 +116,7 @@ function DiscoverDocumentsComponent({ const documentState = useDataState(documents$); const isDataLoading = documentState.fetchStatus === FetchStatus.LOADING; + const isMoreDataLoading = documentState.fetchStatus === FetchStatus.LOADING_MORE; // This is needed to prevent EuiDataGrid pushing onSort because the data view has been switched. // 1. When switching the data view, the sorting in the URL is reset to the default sorting of the selected data view. // 2. The new sort param is already available in this component and propagated to the EuiDataGrid. @@ -129,12 +130,18 @@ function DiscoverDocumentsComponent({ const rows = useMemo(() => documentState.result || [], [documentState.result]); const totalHitsState = useDataState(totalHits$); - const canFetchMoreRecords = - !isPlainRecord && (totalHitsState.result || 0) > (documentState.result?.length || 0); + const totalHits = totalHitsState.result || 0; + const canFetchMoreRecords = !isPlainRecord && totalHits > (documentState.result?.length || 0); - const onFetchMoreRecords = useCallback(() => { - stateContainer.dataState.fetchMore(); - }, [stateContainer.dataState]); + const onFetchMoreRecords = useMemo( + () => + canFetchMoreRecords + ? () => { + stateContainer.dataState.fetchMore(); + } + : undefined, + [canFetchMoreRecords, stateContainer.dataState] + ); const { columns: currentColumns, @@ -242,6 +249,7 @@ function DiscoverDocumentsComponent({ expandedDoc={expandedDoc} dataView={dataView} isLoading={isDataLoading} + isLoadingMore={isMoreDataLoading} rows={rows} sort={(sort as SortOrder[]) || []} sampleSize={sampleSize} @@ -267,7 +275,7 @@ function DiscoverDocumentsComponent({ savedSearchId={savedSearch.id} DocumentView={DiscoverGridFlyout} services={services} - canFetchMoreRecords={canFetchMoreRecords} + totalHits={totalHits} onFetchMoreRecords={onFetchMoreRecords} /> diff --git a/src/plugins/discover/public/application/main/hooks/use_saved_search_messages.ts b/src/plugins/discover/public/application/main/hooks/use_saved_search_messages.ts index 060b12053c870..f23dc073a4b4f 100644 --- a/src/plugins/discover/public/application/main/hooks/use_saved_search_messages.ts +++ b/src/plugins/discover/public/application/main/hooks/use_saved_search_messages.ts @@ -71,6 +71,18 @@ export function sendLoadingMsg( } } +/** + * Send LOADING_MORE message via main observable + */ +export function sendLoadingMoreMsg(data$: BehaviorSubject) { + if (data$.getValue().fetchStatus !== FetchStatus.LOADING_MORE) { + data$.next({ + ...data$.getValue(), + fetchStatus: FetchStatus.LOADING_MORE, + } as T); + } +} + /** * Send ERROR message */ diff --git a/src/plugins/discover/public/application/main/utils/fetch_all.ts b/src/plugins/discover/public/application/main/utils/fetch_all.ts index 5c7df02e85956..feb223ec9d1b2 100644 --- a/src/plugins/discover/public/application/main/utils/fetch_all.ts +++ b/src/plugins/discover/public/application/main/utils/fetch_all.ts @@ -17,6 +17,7 @@ import { sendErrorMsg, sendErrorTo, sendLoadingMsg, + sendLoadingMoreMsg, sendResetMsg, } from '../hooks/use_saved_search_messages'; import { fetchDocuments } from './fetch_documents'; @@ -167,11 +168,7 @@ export function fetchMoreDocuments( }); // Mark subjects as loading - sendLoadingMsg(dataSubjects.documents$, { - recordRawType, - query, - result: dataSubjects.documents$.getValue().result, - }); + sendLoadingMoreMsg(dataSubjects.documents$); // Start fetching all required requests const response = fetchDocuments(searchSource, fetchDeps); diff --git a/src/plugins/discover/public/application/types.ts b/src/plugins/discover/public/application/types.ts index 798e0f350cc5f..9e6a880ebe288 100644 --- a/src/plugins/discover/public/application/types.ts +++ b/src/plugins/discover/public/application/types.ts @@ -9,6 +9,7 @@ export enum FetchStatus { UNINITIALIZED = 'uninitialized', LOADING = 'loading', + LOADING_MORE = 'loading_more', PARTIAL = 'partial', COMPLETE = 'complete', ERROR = 'error', diff --git a/src/plugins/discover/public/components/discover_grid/discover_grid.tsx b/src/plugins/discover/public/components/discover_grid/discover_grid.tsx index 9eaefdba66f45..e0482722d5ffa 100644 --- a/src/plugins/discover/public/components/discover_grid/discover_grid.tsx +++ b/src/plugins/discover/public/components/discover_grid/discover_grid.tsx @@ -83,6 +83,10 @@ export interface DiscoverGridProps { * Determines if data is currently loaded */ isLoading: boolean; + /** + * Determines if it's loading more data (next chunk) + */ + isLoadingMore?: boolean; /** * Function used to add a column in the document flyout */ @@ -207,9 +211,9 @@ export interface DiscoverGridProps { toastNotifications: ToastsStart; }; /** - * Is number of fetched records less than the total number of found hits? + * Number total hits from ES */ - canFetchMoreRecords?: boolean; + totalHits?: number; /** * To fetch more */ @@ -225,6 +229,7 @@ export const DiscoverGrid = ({ columns, dataView, isLoading, + isLoadingMore, expandedDoc, onAddColumn, filters, @@ -256,7 +261,7 @@ export const DiscoverGrid = ({ onFieldEdited, DocumentView, services, - canFetchMoreRecords, + totalHits, onFetchMoreRecords, }: DiscoverGridProps) => { const { fieldFormats, toastNotifications, dataViewFieldEditor, uiSettings } = services; @@ -623,15 +628,17 @@ export const DiscoverGrid = ({ gridStyle={GRID_STYLE} /> - + {!isLoading && typeof isLoadingMore === 'boolean' && ( + + )} {searchTitle && (

diff --git a/src/plugins/discover/public/components/discover_grid/discover_grid_footer.tsx b/src/plugins/discover/public/components/discover_grid/discover_grid_footer.tsx index f0aa2b5c5eacd..5232132367e0d 100644 --- a/src/plugins/discover/public/components/discover_grid/discover_grid_footer.tsx +++ b/src/plugins/discover/public/components/discover_grid/discover_grid_footer.tsx @@ -12,33 +12,33 @@ import classnames from 'classnames'; import { EuiButtonEmpty } from '@elastic/eui'; export interface DiscoverGridFooterProps { - isLoading: boolean; + isLoadingMore: boolean; rowCount: number; sampleSize: number; pageIndex?: number; pageCount: number; - canFetchMoreRecords?: boolean; + totalHits?: number; onFetchMoreRecords?: () => void; } export const DiscoverGridFooter: React.FC = (props) => { const { - isLoading, + isLoadingMore, rowCount, sampleSize, pageIndex, pageCount, - canFetchMoreRecords, + totalHits = 0, onFetchMoreRecords, } = props; - const isOnLastPage = pageIndex === pageCount - 1; + const isOnLastPage = pageIndex === pageCount - 1 && rowCount < totalHits; if (!isOnLastPage) { return null; } // allow to fetch more records on Discover page - if (canFetchMoreRecords && onFetchMoreRecords) { + if (onFetchMoreRecords) { // TODO: define a limit if (rowCount <= 3000 - sampleSize) { return ( @@ -46,7 +46,7 @@ export const DiscoverGridFooter: React.FC = (props) => <> {' '} = (props) => return ; } - if (rowCount === sampleSize) { + if (rowCount < totalHits) { // show only a message for embeddable return ; } @@ -82,7 +82,6 @@ interface DiscoverGridFooterContainerProps extends DiscoverGridFooterProps { const DiscoverGridFooterContainer: React.FC = ({ hasButton, rowCount, - sampleSize, children, }) => { return ( From 3254b988ac50a3fbae0881969657da2521809c7d Mon Sep 17 00:00:00 2001 From: Julia Rechkunova Date: Fri, 12 May 2023 12:05:44 +0200 Subject: [PATCH 04/49] [Discover] Experiment with a tie breaker field --- .../common/utils/sorting/get_es_query_sort.ts | 78 +++++++++++++++++++ .../sorting/get_sort_for_search_source.ts | 51 ++++++++---- .../context/hooks/use_context_app_fetch.tsx | 12 ++- .../application/context/services/context.ts | 2 +- .../context/utils/get_es_query_sort.ts | 39 ---------- .../components/layout/discover_documents.tsx | 6 +- .../application/main/utils/fetch_all.ts | 5 +- .../application/main/utils/fetch_documents.ts | 10 ++- .../main/utils/update_search_source.ts | 13 ++-- 9 files changed, 141 insertions(+), 75 deletions(-) create mode 100644 src/plugins/discover/common/utils/sorting/get_es_query_sort.ts delete mode 100644 src/plugins/discover/public/application/context/utils/get_es_query_sort.ts diff --git a/src/plugins/discover/common/utils/sorting/get_es_query_sort.ts b/src/plugins/discover/common/utils/sorting/get_es_query_sort.ts new file mode 100644 index 0000000000000..3b1a6aab21ea6 --- /dev/null +++ b/src/plugins/discover/common/utils/sorting/get_es_query_sort.ts @@ -0,0 +1,78 @@ +/* + * 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 type { EsQuerySortValue, SortDirection } from '@kbn/data-plugin/public'; +import type { DataView } from '@kbn/data-views-plugin/common'; +import type { IUiSettingsClient } from '@kbn/core-ui-settings-browser'; +import { CONTEXT_TIE_BREAKER_FIELDS_SETTING } from '../..'; + +/** + * Returns `EsQuerySort` which is used to sort records in the ES query + * https://www.elastic.co/guide/en/elasticsearch/reference/current/search-request-sort.html + * @param timeField + * @param tieBreakerField + * @param sortDir + * @param isTimeNanosBased + */ +export function getEsQuerySort( + timeField: string, + tieBreakerField: string, + sortDir: SortDirection, + isTimeNanosBased: boolean +): [EsQuerySortValue, EsQuerySortValue] { + return [ + getESQuerySortForTimeField(timeField, sortDir, isTimeNanosBased), + getESQuerySortForTieBreaker(tieBreakerField, sortDir), + ]; +} + +export function getESQuerySortForTimeField( + timeField: string, + sortDir: SortDirection, + isTimeNanosBased: boolean +): EsQuerySortValue { + return { + [timeField]: { + order: sortDir, + ...(isTimeNanosBased + ? { + format: 'strict_date_optional_time_nanos', + numeric_type: 'date_nanos', + } + : { format: 'strict_date_optional_time' }), + }, + }; +} + +export function getESQuerySortForTieBreaker( + tieBreakerField: string, + sortDir: SortDirection +): EsQuerySortValue { + return { [tieBreakerField]: sortDir }; +} + +/** + * The list of field names that are allowed for sorting, but not included in + * data view fields. + */ +const META_FIELD_NAMES: string[] = ['_seq_no', '_doc', '_uid']; + +/** + * Returns a field from the intersection of the set of sortable fields in the + * given data view and a given set of candidate field names. + */ +export function getTieBreakerField(dataView: DataView, uiSettings: IUiSettingsClient) { + const sortableFields = uiSettings + .get(CONTEXT_TIE_BREAKER_FIELDS_SETTING) + .filter( + (fieldName: string) => + META_FIELD_NAMES.includes(fieldName) || + (dataView.fields.getByName(fieldName) || { sortable: false }).sortable + ); + return sortableFields[0]; +} diff --git a/src/plugins/discover/common/utils/sorting/get_sort_for_search_source.ts b/src/plugins/discover/common/utils/sorting/get_sort_for_search_source.ts index a133a0dac9619..d4ba350d6791e 100644 --- a/src/plugins/discover/common/utils/sorting/get_sort_for_search_source.ts +++ b/src/plugins/discover/common/utils/sorting/get_sort_for_search_source.ts @@ -6,10 +6,17 @@ * Side Public License, v 1. */ -import type { DataView } from '@kbn/data-views-plugin/public'; -import type { EsQuerySortValue } from '@kbn/data-plugin/public'; +import type { DataView } from '@kbn/data-views-plugin/common'; +import type { EsQuerySortValue, SortDirection } from '@kbn/data-plugin/common'; import type { SortOrder } from '@kbn/saved-search-plugin/public'; +import type { IUiSettingsClient } from '@kbn/core-ui-settings-browser'; import { getSort } from './get_sort'; +import { + getESQuerySortForTimeField, + getESQuerySortForTieBreaker, + getTieBreakerField, +} from './get_es_query_sort'; +import { SORT_DEFAULT_ORDER_SETTING } from '../..'; /** * Prepares sort for search source, that's sending the request to ES @@ -19,10 +26,12 @@ import { getSort } from './get_sort'; * when there are indices with date and indices with date_nanos field */ export function getSortForSearchSource( - sort?: SortOrder[], - dataView?: DataView, - defaultDirection: string = 'desc' + sort: SortOrder[] | undefined, + dataView: DataView | undefined, + uiSettings: IUiSettingsClient ): EsQuerySortValue[] { + const defaultDirection = uiSettings.get(SORT_DEFAULT_ORDER_SETTING) || 'desc'; + if (!sort || !dataView || (Array.isArray(sort) && sort.length === 0)) { if (dataView?.timeFieldName) { // sorting by index order @@ -31,17 +40,31 @@ export function getSortForSearchSource( return [{ _score: defaultDirection } as EsQuerySortValue]; } } + const { timeFieldName } = dataView; - return getSort(sort, dataView).map((sortPair: Record) => { - if (dataView.isTimeNanosBased() && timeFieldName && sortPair[timeFieldName]) { - return { - [timeFieldName]: { - order: sortPair[timeFieldName], - numeric_type: 'date_nanos', - format: 'strict_date_optional_time_nanos', - }, - } as EsQuerySortValue; + const sortPairs = getSort(sort, dataView); + + const sortForSearchSource = sortPairs.map((sortPair: Record) => { + if (timeFieldName && sortPair[timeFieldName]) { + return getESQuerySortForTimeField( + timeFieldName, + sortPair[timeFieldName] as SortDirection, + dataView.isTimeNanosBased() + ); } return sortPair as EsQuerySortValue; }); + + // TODO: do we need to a tie breaker like this? + if (dataView.isTimeBased() && sortPairs.length) { + const firstSortPair = sortPairs[0]; + const firstPairSortDir = Array.isArray(firstSortPair) + ? firstSortPair[1] + : Object.values(firstSortPair)[0]; + sortForSearchSource.push( + getESQuerySortForTieBreaker(getTieBreakerField(dataView, uiSettings), firstPairSortDir) + ); + } + + return sortForSearchSource; } diff --git a/src/plugins/discover/public/application/context/hooks/use_context_app_fetch.tsx b/src/plugins/discover/public/application/context/hooks/use_context_app_fetch.tsx index 3a9fa8ff73151..be838f6353eeb 100644 --- a/src/plugins/discover/public/application/context/hooks/use_context_app_fetch.tsx +++ b/src/plugins/discover/public/application/context/hooks/use_context_app_fetch.tsx @@ -10,7 +10,6 @@ import { i18n } from '@kbn/i18n'; import { MarkdownSimple, toMountPoint, wrapWithTheme } from '@kbn/kibana-react-plugin/public'; import type { DataView } from '@kbn/data-views-plugin/public'; import { SortDirection } from '@kbn/data-plugin/public'; -import { CONTEXT_TIE_BREAKER_FIELDS_SETTING } from '../../../../common'; import { fetchAnchor } from '../services/anchor'; import { fetchSurroundingDocs, SurrDocType } from '../services/context'; import { @@ -20,10 +19,12 @@ import { LoadingStatus, } from '../services/context_query_state'; import { AppState } from '../services/context_state'; -import { getFirstSortableField } from '../utils/sorting'; import { useDiscoverServices } from '../../../hooks/use_discover_services'; import type { DataTableRecord } from '../../../types'; -import { getEsQuerySort } from '../utils/get_es_query_sort'; +import { + getTieBreakerField, + getEsQuerySort, +} from '../../../../common/utils/sorting/get_es_query_sort'; const createError = (statusKey: string, reason: FailureReason, error?: Error) => ({ [statusKey]: { value: LoadingStatus.FAILED, error, reason }, @@ -54,10 +55,7 @@ export function useContextAppFetch({ const searchSource = useMemo(() => { return data.search.searchSource.createEmpty(); }, [data.search.searchSource]); - const tieBreakerField = useMemo( - () => getFirstSortableField(dataView, config.get(CONTEXT_TIE_BREAKER_FIELDS_SETTING)), - [config, dataView] - ); + const tieBreakerField = useMemo(() => getTieBreakerField(dataView, config), [config, dataView]); const [fetchedState, setFetchedState] = useState( getInitialContextQueryState() diff --git a/src/plugins/discover/public/application/context/services/context.ts b/src/plugins/discover/public/application/context/services/context.ts index a697444fbf6dd..8253e018381f3 100644 --- a/src/plugins/discover/public/application/context/services/context.ts +++ b/src/plugins/discover/public/application/context/services/context.ts @@ -13,7 +13,7 @@ import { convertIsoToMillis, extractNanos } from '../utils/date_conversion'; import { fetchHitsInInterval } from '../utils/fetch_hits_in_interval'; import { generateIntervals } from '../utils/generate_intervals'; import { getEsQuerySearchAfter } from '../utils/get_es_query_search_after'; -import { getEsQuerySort } from '../utils/get_es_query_sort'; +import { getEsQuerySort } from '../../../../common/utils/sorting/get_es_query_sort'; import { DataTableRecord } from '../../../types'; export enum SurrDocType { diff --git a/src/plugins/discover/public/application/context/utils/get_es_query_sort.ts b/src/plugins/discover/public/application/context/utils/get_es_query_sort.ts deleted file mode 100644 index 2731404ac6d18..0000000000000 --- a/src/plugins/discover/public/application/context/utils/get_es_query_sort.ts +++ /dev/null @@ -1,39 +0,0 @@ -/* - * 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 type { EsQuerySortValue, SortDirection } from '@kbn/data-plugin/public'; - -/** - * Returns `EsQuerySort` which is used to sort records in the ES query - * https://www.elastic.co/guide/en/elasticsearch/reference/current/search-request-sort.html - * @param timeField - * @param tieBreakerField - * @param sortDir - * @param isTimeNanosBased - */ -export function getEsQuerySort( - timeField: string, - tieBreakerField: string, - sortDir: SortDirection, - isTimeNanosBased: boolean -): [EsQuerySortValue, EsQuerySortValue] { - return [ - { - [timeField]: { - order: sortDir, - ...(isTimeNanosBased - ? { - format: 'strict_date_optional_time_nanos', - numeric_type: 'date_nanos', - } - : { format: 'strict_date_optional_time' }), - }, - }, - { [tieBreakerField]: sortDir }, - ]; -} diff --git a/src/plugins/discover/public/application/main/components/layout/discover_documents.tsx b/src/plugins/discover/public/application/main/components/layout/discover_documents.tsx index 45ff0c6cc081d..40a180980fe8f 100644 --- a/src/plugins/discover/public/application/main/components/layout/discover_documents.tsx +++ b/src/plugins/discover/public/application/main/components/layout/discover_documents.tsx @@ -131,7 +131,11 @@ function DiscoverDocumentsComponent({ const totalHitsState = useDataState(totalHits$); const totalHits = totalHitsState.result || 0; - const canFetchMoreRecords = !isPlainRecord && totalHits > (documentState.result?.length || 0); + const canFetchMoreRecords = + !isPlainRecord && + rows.length > 0 && + totalHits > rows.length && + Boolean(rows[rows.length - 1].raw.sort?.length); const onFetchMoreRecords = useMemo( () => diff --git a/src/plugins/discover/public/application/main/utils/fetch_all.ts b/src/plugins/discover/public/application/main/utils/fetch_all.ts index feb223ec9d1b2..591d39c9c063e 100644 --- a/src/plugins/discover/public/application/main/utils/fetch_all.ts +++ b/src/plugins/discover/public/application/main/utils/fetch_all.ts @@ -140,14 +140,13 @@ export function fetchMoreDocuments( const searchSource = savedSearch.searchSource.createChild(); try { - const latestDocuments = dataSubjects.documents$.getValue().result || []; - const lastDocumentSort = latestDocuments[latestDocuments.length - 1]?.raw?.sort; + const lastDocuments = dataSubjects.documents$.getValue().result || []; + const lastDocumentSort = lastDocuments[lastDocuments.length - 1]?.raw?.sort; if (!lastDocumentSort) { return Promise.resolve(); } - // TODO: add tie breaker field searchSource.setField('searchAfter', lastDocumentSort); const dataView = searchSource.getField('index')!; diff --git a/src/plugins/discover/public/application/main/utils/fetch_documents.ts b/src/plugins/discover/public/application/main/utils/fetch_documents.ts index db092cb449057..bb32c227b9163 100644 --- a/src/plugins/discover/public/application/main/utils/fetch_documents.ts +++ b/src/plugins/discover/public/application/main/utils/fetch_documents.ts @@ -45,9 +45,13 @@ export const fetchDocuments = ( sessionId: searchSessionId, inspector: { adapter: inspectorAdapters.requests, - title: i18n.translate('discover.inspectorRequestDataTitleDocuments', { - defaultMessage: 'Documents', - }), + title: searchSource.getField('searchAfter') + ? i18n.translate('discover.inspectorRequestDataTitleMoreDocuments', { + defaultMessage: 'More documents', + }) + : i18n.translate('discover.inspectorRequestDataTitleDocuments', { + defaultMessage: 'Documents', + }), description: i18n.translate('discover.inspectorRequestDescriptionDocument', { defaultMessage: 'This request queries Elasticsearch to fetch the documents.', }), diff --git a/src/plugins/discover/public/application/main/utils/update_search_source.ts b/src/plugins/discover/public/application/main/utils/update_search_source.ts index 4152b57a34a0f..b138d559fb5cc 100644 --- a/src/plugins/discover/public/application/main/utils/update_search_source.ts +++ b/src/plugins/discover/public/application/main/utils/update_search_source.ts @@ -9,7 +9,7 @@ import { ISearchSource } from '@kbn/data-plugin/public'; import { DataViewType, DataView } from '@kbn/data-views-plugin/public'; import type { SortOrder } from '@kbn/saved-search-plugin/public'; -import { SEARCH_FIELDS_FROM_SOURCE, SORT_DEFAULT_ORDER_SETTING } from '../../../../common'; +import { SEARCH_FIELDS_FROM_SOURCE } from '../../../../common'; import { DiscoverServices } from '../../../build_services'; import { getSortForSearchSource } from '../../../utils/sorting'; @@ -29,13 +29,12 @@ export function updateVolatileSearchSource( } ) { const { uiSettings, data } = services; - const usedSort = getSortForSearchSource( - sort, - dataView, - uiSettings.get(SORT_DEFAULT_ORDER_SETTING) - ); const useNewFieldsApi = !uiSettings.get(SEARCH_FIELDS_FROM_SOURCE); - searchSource.setField('trackTotalHits', true).setField('sort', usedSort); + + const usedSort = getSortForSearchSource(sort, dataView, uiSettings); + searchSource.setField('sort', usedSort); + + searchSource.setField('trackTotalHits', true); if (dataView.type !== DataViewType.ROLLUP) { // Set the date range filter fields from timeFilter using the absolute format. Search sessions requires that it be converted from a relative range From 0c867cf7c49c311f15c4bf34132ed413dd0dc392 Mon Sep 17 00:00:00 2001 From: Julia Rechkunova Date: Fri, 12 May 2023 13:59:20 +0200 Subject: [PATCH 05/49] [Discover] Update other places --- .../public/embeddable/saved_search_embeddable.tsx | 3 ++- .../public/embeddable/utils/update_search_source.ts | 10 ++++++---- src/plugins/discover/public/utils/get_sharing_data.ts | 11 ++--------- 3 files changed, 10 insertions(+), 14 deletions(-) diff --git a/src/plugins/discover/public/embeddable/saved_search_embeddable.tsx b/src/plugins/discover/public/embeddable/saved_search_embeddable.tsx index 1317916588184..8d9b870c3a15a 100644 --- a/src/plugins/discover/public/embeddable/saved_search_embeddable.tsx +++ b/src/plugins/discover/public/embeddable/saved_search_embeddable.tsx @@ -195,7 +195,8 @@ export class SavedSearchEmbeddable { sampleSize: this.services.uiSettings.get(SAMPLE_SIZE_SETTING), defaultSort: this.services.uiSettings.get(SORT_DEFAULT_ORDER_SETTING), - } + }, + this.services.uiSettings ); // Log request to inspector diff --git a/src/plugins/discover/public/embeddable/utils/update_search_source.ts b/src/plugins/discover/public/embeddable/utils/update_search_source.ts index 87ee137aed796..8f20464e73b58 100644 --- a/src/plugins/discover/public/embeddable/utils/update_search_source.ts +++ b/src/plugins/discover/public/embeddable/utils/update_search_source.ts @@ -8,6 +8,7 @@ import type { DataView } from '@kbn/data-views-plugin/public'; import type { ISearchSource } from '@kbn/data-plugin/public'; import type { SortOrder } from '@kbn/saved-search-plugin/public'; +import type { IUiSettingsClient } from '@kbn/core-ui-settings-browser'; import { getSortForSearchSource } from '../../utils/sorting'; export const updateSearchSource = ( @@ -17,12 +18,13 @@ export const updateSearchSource = ( useNewFieldsApi: boolean, defaults: { sampleSize: number; - defaultSort: string; - } + defaultSort: string; // TODO: remove + }, + uiSettings: IUiSettingsClient ) => { - const { sampleSize, defaultSort } = defaults; + const { sampleSize } = defaults; searchSource.setField('size', sampleSize); - searchSource.setField('sort', getSortForSearchSource(sort, dataView, defaultSort)); + searchSource.setField('sort', getSortForSearchSource(sort, dataView, uiSettings)); if (useNewFieldsApi) { searchSource.removeField('fieldsFromSource'); const fields: Record = { field: '*', include_unmapped: 'true' }; diff --git a/src/plugins/discover/public/utils/get_sharing_data.ts b/src/plugins/discover/public/utils/get_sharing_data.ts index c9c8dd5c7bb1c..465aff3926f04 100644 --- a/src/plugins/discover/public/utils/get_sharing_data.ts +++ b/src/plugins/discover/public/utils/get_sharing_data.ts @@ -20,11 +20,7 @@ import { isEqualFilters, } from '../application/main/services/discover_app_state_container'; import { getSortForSearchSource } from './sorting'; -import { - DOC_HIDE_TIME_COLUMN_SETTING, - SEARCH_FIELDS_FROM_SOURCE, - SORT_DEFAULT_ORDER_SETTING, -} from '../../common'; +import { DOC_HIDE_TIME_COLUMN_SETTING, SEARCH_FIELDS_FROM_SOURCE } from '../../common'; /** * Preparing data to share the current state as link or CSV/Report @@ -40,10 +36,7 @@ export async function getSharingData( const index = searchSource.getField('index')!; let existingFilter = searchSource.getField('filter') as Filter[] | Filter | undefined; - searchSource.setField( - 'sort', - getSortForSearchSource(state.sort as SortOrder[], index, config.get(SORT_DEFAULT_ORDER_SETTING)) - ); + searchSource.setField('sort', getSortForSearchSource(state.sort as SortOrder[], index, config)); searchSource.removeField('filter'); searchSource.removeField('highlight'); From e868a12915be62e48d963a0de5c7f6f8fc7f21ca Mon Sep 17 00:00:00 2001 From: Julia Rechkunova Date: Fri, 12 May 2023 17:07:55 +0200 Subject: [PATCH 06/49] [Discover] Comment out the tie breaker --- .../get_sort_for_search_source.test.ts | 36 +++++++++++++------ .../sorting/get_sort_for_search_source.ts | 24 ++++++------- .../discover_grid/discover_grid_footer.tsx | 5 +-- .../utils/update_search_source.test.ts | 25 +++++++++++-- 4 files changed, 62 insertions(+), 28 deletions(-) diff --git a/src/plugins/discover/common/utils/sorting/get_sort_for_search_source.test.ts b/src/plugins/discover/common/utils/sorting/get_sort_for_search_source.test.ts index dd54b5d2a70a2..ba76a0c093ca4 100644 --- a/src/plugins/discover/common/utils/sorting/get_sort_for_search_source.test.ts +++ b/src/plugins/discover/common/utils/sorting/get_sort_for_search_source.test.ts @@ -8,33 +8,49 @@ import type { SortOrder } from '@kbn/saved-search-plugin/public'; import { getSortForSearchSource } from './get_sort_for_search_source'; import { stubDataView, stubDataViewWithoutTimeField } from '@kbn/data-plugin/common/stubs'; +import { coreMock } from '@kbn/core/public/mocks'; +import { SORT_DEFAULT_ORDER_SETTING } from '../..'; describe('getSortForSearchSource function', function () { + const core = coreMock.createStart(); + const uiSettings = core.uiSettings; + + const uiSettingWithAscSorting = coreMock.createStart().uiSettings; + jest + .spyOn(uiSettingWithAscSorting, 'get') + .mockImplementation((key) => (key === SORT_DEFAULT_ORDER_SETTING ? 'asc' : null)); + test('should be a function', function () { expect(typeof getSortForSearchSource === 'function').toBeTruthy(); }); test('should return an object to use for searchSource when columns are given', function () { const cols = [['bytes', 'desc']] as SortOrder[]; - expect(getSortForSearchSource(cols, stubDataView)).toEqual([{ bytes: 'desc' }]); - expect(getSortForSearchSource(cols, stubDataView, 'asc')).toEqual([{ bytes: 'desc' }]); + expect(getSortForSearchSource(cols, stubDataView, uiSettings)).toEqual([{ bytes: 'desc' }]); + expect(getSortForSearchSource(cols, stubDataView, uiSettingWithAscSorting)).toEqual([ + { bytes: 'desc' }, + ]); - expect(getSortForSearchSource(cols, stubDataViewWithoutTimeField)).toEqual([{ bytes: 'desc' }]); - expect(getSortForSearchSource(cols, stubDataViewWithoutTimeField, 'asc')).toEqual([ + expect(getSortForSearchSource(cols, stubDataViewWithoutTimeField, uiSettings)).toEqual([ { bytes: 'desc' }, ]); + expect( + getSortForSearchSource(cols, stubDataViewWithoutTimeField, uiSettingWithAscSorting) + ).toEqual([{ bytes: 'desc' }]); }); test('should return an object to use for searchSource when no columns are given', function () { const cols = [] as SortOrder[]; - expect(getSortForSearchSource(cols, stubDataView)).toEqual([{ _doc: 'desc' }]); - expect(getSortForSearchSource(cols, stubDataView, 'asc')).toEqual([{ _doc: 'asc' }]); + expect(getSortForSearchSource(cols, stubDataView, uiSettings)).toEqual([{ _doc: 'desc' }]); + expect(getSortForSearchSource(cols, stubDataView, uiSettingWithAscSorting)).toEqual([ + { _doc: 'asc' }, + ]); - expect(getSortForSearchSource(cols, stubDataViewWithoutTimeField)).toEqual([ + expect(getSortForSearchSource(cols, stubDataViewWithoutTimeField, uiSettings)).toEqual([ { _score: 'desc' }, ]); - expect(getSortForSearchSource(cols, stubDataViewWithoutTimeField, 'asc')).toEqual([ - { _score: 'asc' }, - ]); + expect( + getSortForSearchSource(cols, stubDataViewWithoutTimeField, uiSettingWithAscSorting) + ).toEqual([{ _score: 'asc' }]); }); }); diff --git a/src/plugins/discover/common/utils/sorting/get_sort_for_search_source.ts b/src/plugins/discover/common/utils/sorting/get_sort_for_search_source.ts index d4ba350d6791e..7fcee98e87112 100644 --- a/src/plugins/discover/common/utils/sorting/get_sort_for_search_source.ts +++ b/src/plugins/discover/common/utils/sorting/get_sort_for_search_source.ts @@ -13,8 +13,8 @@ import type { IUiSettingsClient } from '@kbn/core-ui-settings-browser'; import { getSort } from './get_sort'; import { getESQuerySortForTimeField, - getESQuerySortForTieBreaker, - getTieBreakerField, + // getESQuerySortForTieBreaker, + // getTieBreakerField, } from './get_es_query_sort'; import { SORT_DEFAULT_ORDER_SETTING } from '../..'; @@ -55,16 +55,16 @@ export function getSortForSearchSource( return sortPair as EsQuerySortValue; }); - // TODO: do we need to a tie breaker like this? - if (dataView.isTimeBased() && sortPairs.length) { - const firstSortPair = sortPairs[0]; - const firstPairSortDir = Array.isArray(firstSortPair) - ? firstSortPair[1] - : Object.values(firstSortPair)[0]; - sortForSearchSource.push( - getESQuerySortForTieBreaker(getTieBreakerField(dataView, uiSettings), firstPairSortDir) - ); - } + // TODO: do we need to have a tie breaker like this? + // if (dataView.isTimeBased() && sortPairs.length) { + // const firstSortPair = sortPairs[0]; + // const firstPairSortDir = Array.isArray(firstSortPair) + // ? firstSortPair[1] + // : Object.values(firstSortPair)[0]; + // sortForSearchSource.push( + // getESQuerySortForTieBreaker(getTieBreakerField(dataView, uiSettings), firstPairSortDir) + // ); + // } return sortForSearchSource; } diff --git a/src/plugins/discover/public/components/discover_grid/discover_grid_footer.tsx b/src/plugins/discover/public/components/discover_grid/discover_grid_footer.tsx index 5232132367e0d..a089fb84f8413 100644 --- a/src/plugins/discover/public/components/discover_grid/discover_grid_footer.tsx +++ b/src/plugins/discover/public/components/discover_grid/discover_grid_footer.tsx @@ -53,10 +53,7 @@ export const DiscoverGridFooter: React.FC = (props) => > diff --git a/src/plugins/discover/public/embeddable/utils/update_search_source.test.ts b/src/plugins/discover/public/embeddable/utils/update_search_source.test.ts index 779fcfffde778..a94a6127ac8f7 100644 --- a/src/plugins/discover/public/embeddable/utils/update_search_source.test.ts +++ b/src/plugins/discover/public/embeddable/utils/update_search_source.test.ts @@ -9,6 +9,13 @@ import { createSearchSourceMock } from '@kbn/data-plugin/common/search/search_so import { updateSearchSource } from './update_search_source'; import { dataViewMock } from '../../__mocks__/data_view'; import type { SortOrder } from '@kbn/saved-search-plugin/public'; +import { coreMock } from '@kbn/core/public/mocks'; +import { SORT_DEFAULT_ORDER_SETTING } from '../../../common'; + +const uiSettingWithAscSorting = coreMock.createStart().uiSettings; +jest + .spyOn(uiSettingWithAscSorting, 'get') + .mockImplementation((key) => (key === SORT_DEFAULT_ORDER_SETTING ? 'asc' : null)); describe('updateSearchSource', () => { const defaults = { @@ -18,7 +25,14 @@ describe('updateSearchSource', () => { it('updates a given search source', async () => { const searchSource = createSearchSourceMock({}); - updateSearchSource(searchSource, dataViewMock, [] as SortOrder[], false, defaults); + updateSearchSource( + searchSource, + dataViewMock, + [] as SortOrder[], + false, + defaults, + uiSettingWithAscSorting + ); expect(searchSource.getField('fields')).toBe(undefined); // does not explicitly request fieldsFromSource when not using fields API expect(searchSource.getField('fieldsFromSource')).toBe(undefined); @@ -26,7 +40,14 @@ describe('updateSearchSource', () => { it('updates a given search source with the usage of the new fields api', async () => { const searchSource = createSearchSourceMock({}); - updateSearchSource(searchSource, dataViewMock, [] as SortOrder[], true, defaults); + updateSearchSource( + searchSource, + dataViewMock, + [] as SortOrder[], + true, + defaults, + uiSettingWithAscSorting + ); expect(searchSource.getField('fields')).toEqual([{ field: '*', include_unmapped: 'true' }]); expect(searchSource.getField('fieldsFromSource')).toBe(undefined); }); From bead1dcdeb0401a15c74ef21ad920c4c83b99560 Mon Sep 17 00:00:00 2001 From: Julia Rechkunova Date: Fri, 12 May 2023 17:34:23 +0200 Subject: [PATCH 07/49] [Discover] Fix lint issues --- .../sorting/get_sort_for_search_source.ts | 4 ++-- .../layout/use_discover_histogram.test.tsx | 20 +++++++++++++++---- .../locator/searchsource_from_locator.ts | 2 +- 3 files changed, 19 insertions(+), 7 deletions(-) diff --git a/src/plugins/discover/common/utils/sorting/get_sort_for_search_source.ts b/src/plugins/discover/common/utils/sorting/get_sort_for_search_source.ts index 7fcee98e87112..5f9423db20c5b 100644 --- a/src/plugins/discover/common/utils/sorting/get_sort_for_search_source.ts +++ b/src/plugins/discover/common/utils/sorting/get_sort_for_search_source.ts @@ -28,9 +28,9 @@ import { SORT_DEFAULT_ORDER_SETTING } from '../..'; export function getSortForSearchSource( sort: SortOrder[] | undefined, dataView: DataView | undefined, - uiSettings: IUiSettingsClient + uiSettings: IUiSettingsClient | null ): EsQuerySortValue[] { - const defaultDirection = uiSettings.get(SORT_DEFAULT_ORDER_SETTING) || 'desc'; + const defaultDirection = uiSettings?.get(SORT_DEFAULT_ORDER_SETTING) || 'desc'; if (!sort || !dataView || (Array.isArray(sort) && sort.length === 0)) { if (dataView?.timeFieldName) { diff --git a/src/plugins/discover/public/application/main/components/layout/use_discover_histogram.test.tsx b/src/plugins/discover/public/application/main/components/layout/use_discover_histogram.test.tsx index d3dae1cf148dc..7edbddadeadd4 100644 --- a/src/plugins/discover/public/application/main/components/layout/use_discover_histogram.test.tsx +++ b/src/plugins/discover/public/application/main/components/layout/use_discover_histogram.test.tsx @@ -360,7 +360,10 @@ describe('useDiscoverHistogram', () => { describe('refetching', () => { it('should call refetch when savedSearchFetch$ is triggered', async () => { const savedSearchFetch$ = new Subject<{ - reset: boolean; + options: { + reset: boolean; + fetchMore: boolean; + }; searchSessionId: string; }>(); const stateContainer = getStateContainer(); @@ -372,14 +375,20 @@ describe('useDiscoverHistogram', () => { }); expect(api.refetch).not.toHaveBeenCalled(); act(() => { - savedSearchFetch$.next({ reset: false, searchSessionId: '1234' }); + savedSearchFetch$.next({ + options: { reset: false, fetchMore: false }, + searchSessionId: '1234', + }); }); expect(api.refetch).toHaveBeenCalled(); }); it('should skip the next refetch when hideChart changes from true to false', async () => { const savedSearchFetch$ = new Subject<{ - reset: boolean; + options: { + reset: boolean; + fetchMore: boolean; + }; searchSessionId: string; }>(); const stateContainer = getStateContainer(); @@ -396,7 +405,10 @@ describe('useDiscoverHistogram', () => { hook.rerender({ ...initialProps, hideChart: false }); }); act(() => { - savedSearchFetch$.next({ reset: false, searchSessionId: '1234' }); + savedSearchFetch$.next({ + options: { reset: false, fetchMore: false }, + searchSessionId: '1234', + }); }); expect(api.refetch).not.toHaveBeenCalled(); }); diff --git a/src/plugins/discover/server/locator/searchsource_from_locator.ts b/src/plugins/discover/server/locator/searchsource_from_locator.ts index 455efc968b534..3d2ab58767b58 100644 --- a/src/plugins/discover/server/locator/searchsource_from_locator.ts +++ b/src/plugins/discover/server/locator/searchsource_from_locator.ts @@ -147,7 +147,7 @@ export function searchSourceFromLocatorFactory(services: LocatorServicesDeps) { // Inject sort if (savedSearch.sort) { - const sort = getSortForSearchSource(savedSearch.sort as Array<[string, string]>, index); + const sort = getSortForSearchSource(savedSearch.sort as Array<[string, string]>, index, null); // TODO: is it correct that it was using a `desc` as default sort here? searchSource.setField('sort', sort); } From f4575d71cf7fee83bc9c3acb6257edd57c901531 Mon Sep 17 00:00:00 2001 From: Julia Rechkunova Date: Mon, 24 Jul 2023 16:01:58 +0200 Subject: [PATCH 08/49] [Discover] Update styles --- src/plugins/discover/common/constants.ts | 1 + .../discover_grid/discover_grid.scss | 12 ---- .../discover_grid/discover_grid_footer.tsx | 68 ++++++++++++------- 3 files changed, 44 insertions(+), 37 deletions(-) diff --git a/src/plugins/discover/common/constants.ts b/src/plugins/discover/common/constants.ts index 8e61baa8ba6fc..4a7e06c9e6642 100644 --- a/src/plugins/discover/common/constants.ts +++ b/src/plugins/discover/common/constants.ts @@ -6,6 +6,7 @@ * Side Public License, v 1. */ +export const MAX_LOADED_GRID_ROWS = 10000; export const DEFAULT_ROWS_PER_PAGE = 100; export const ROWS_PER_PAGE_OPTIONS = [10, 25, 50, DEFAULT_ROWS_PER_PAGE, 250, 500]; export enum VIEW_MODE { diff --git a/src/plugins/discover/public/components/discover_grid/discover_grid.scss b/src/plugins/discover/public/components/discover_grid/discover_grid.scss index d55ed9f3f34f0..991224cc6b9ba 100644 --- a/src/plugins/discover/public/components/discover_grid/discover_grid.scss +++ b/src/plugins/discover/public/components/discover_grid/discover_grid.scss @@ -65,18 +65,6 @@ min-height: 0; } -.dscDiscoverGrid__footer { - flex-shrink: 0; - background-color: $euiColorLightShade; - padding: $euiSize / 2 $euiSize; - margin-top: $euiSize / 4; - text-align: center; - - &--withButton { - padding: 0 $euiSize; - } -} - .dscTable__flyoutHeader { white-space: nowrap; } diff --git a/src/plugins/discover/public/components/discover_grid/discover_grid_footer.tsx b/src/plugins/discover/public/components/discover_grid/discover_grid_footer.tsx index a089fb84f8413..df89fdb9a210a 100644 --- a/src/plugins/discover/public/components/discover_grid/discover_grid_footer.tsx +++ b/src/plugins/discover/public/components/discover_grid/discover_grid_footer.tsx @@ -8,8 +8,9 @@ import React from 'react'; import { FormattedMessage } from '@kbn/i18n-react'; -import classnames from 'classnames'; -import { EuiButtonEmpty } from '@elastic/eui'; +import { EuiButtonEmpty, useEuiTheme } from '@elastic/eui'; +import { css } from '@emotion/react'; +import { MAX_LOADED_GRID_ROWS } from '../../../common/constants'; export interface DiscoverGridFooterProps { isLoadingMore: boolean; @@ -31,6 +32,7 @@ export const DiscoverGridFooter: React.FC = (props) => totalHits = 0, onFetchMoreRecords, } = props; + const { euiTheme } = useEuiTheme(); const isOnLastPage = pageIndex === pageCount - 1 && rowCount < totalHits; if (!isOnLastPage) { @@ -39,23 +41,25 @@ export const DiscoverGridFooter: React.FC = (props) => // allow to fetch more records on Discover page if (onFetchMoreRecords) { - // TODO: define a limit - if (rowCount <= 3000 - sampleSize) { + if (rowCount <= MAX_LOADED_GRID_ROWS - sampleSize) { return ( <> - {' '} + {'.'} ); @@ -81,30 +85,44 @@ const DiscoverGridFooterContainer: React.FC = rowCount, children, }) => { + const { euiTheme } = useEuiTheme(); + return (

- {hasButton ? ( - - ) : ( - - )} + + {hasButton ? ( + + ) : ( + + )} + {children}

); From f9adfa18419cb97ff18f6da0664413979be3e3b6 Mon Sep 17 00:00:00 2001 From: Julia Rechkunova Date: Mon, 24 Jul 2023 16:53:16 +0200 Subject: [PATCH 09/49] [Discover] Handle error cases --- .../main/hooks/use_saved_search_messages.ts | 16 ++++++++ .../application/main/utils/fetch_all.ts | 37 ++++++++----------- 2 files changed, 32 insertions(+), 21 deletions(-) diff --git a/src/plugins/discover/public/application/main/hooks/use_saved_search_messages.ts b/src/plugins/discover/public/application/main/hooks/use_saved_search_messages.ts index f23dc073a4b4f..d2ae99eab1be0 100644 --- a/src/plugins/discover/public/application/main/hooks/use_saved_search_messages.ts +++ b/src/plugins/discover/public/application/main/hooks/use_saved_search_messages.ts @@ -7,6 +7,7 @@ */ import type { BehaviorSubject } from 'rxjs'; +import type { DataTableRecord } from '@kbn/discover-utils/src/types'; import { FetchStatus } from '../../types'; import type { DataDocuments$, @@ -16,6 +17,7 @@ import type { SavedSearchData, } from '../services/discover_data_state_container'; import { RecordRawType } from '../services/discover_data_state_container'; + /** * Sends COMPLETE message to the main$ observable with the information * that no documents have been found, allowing Discover to show a no @@ -83,6 +85,20 @@ export function sendLoadingMoreMsg(data$: BehaviorSubject) } } +/** + * Finishing LOADING_MORE message + */ +export function sendLoadingMoreFinishedMsg(data$: DataDocuments$, moreRecords: DataTableRecord[]) { + const currentValue = data$.getValue(); + if (currentValue.fetchStatus === FetchStatus.LOADING_MORE) { + data$.next({ + ...currentValue, + fetchStatus: FetchStatus.COMPLETE, + result: [...(currentValue.result || []), ...moreRecords], + }); + } +} + /** * Send ERROR message */ diff --git a/src/plugins/discover/public/application/main/utils/fetch_all.ts b/src/plugins/discover/public/application/main/utils/fetch_all.ts index 34244e8f01891..b8196e136c908 100644 --- a/src/plugins/discover/public/application/main/utils/fetch_all.ts +++ b/src/plugins/discover/public/application/main/utils/fetch_all.ts @@ -20,6 +20,7 @@ import { sendErrorTo, sendLoadingMsg, sendLoadingMoreMsg, + sendLoadingMoreFinishedMsg, sendResetMsg, } from '../hooks/use_saved_search_messages'; import { fetchDocuments } from './fetch_documents'; @@ -163,6 +164,15 @@ export function fetchMoreDocuments( const searchSource = savedSearch.searchSource.createChild(); try { + const dataView = searchSource.getField('index')!; + const query = getAppState().query; + const recordRawType = getRawRecordType(query); + + if (recordRawType === RecordRawType.PLAIN) { + // not supported yet + return Promise.resolve(); + } + const lastDocuments = dataSubjects.documents$.getValue().result || []; const lastDocumentSort = lastDocuments[lastDocuments.length - 1]?.raw?.sort; @@ -172,15 +182,8 @@ export function fetchMoreDocuments( searchSource.setField('searchAfter', lastDocumentSort); - const dataView = searchSource.getField('index')!; - const query = getAppState().query; - const recordRawType = getRawRecordType(query); - - if (recordRawType === RecordRawType.PLAIN) { - // not supported yet - // TODO? - return Promise.resolve(); - } + // Mark subjects as loading + sendLoadingMoreMsg(dataSubjects.documents$); // Update the base searchSource, base for all child fetches updateVolatileSearchSource(searchSource, { @@ -189,32 +192,24 @@ export function fetchMoreDocuments( sort: getAppState().sort as SortOrder[], }); - // Mark subjects as loading - sendLoadingMoreMsg(dataSubjects.documents$); - // Start fetching all required requests const response = fetchDocuments(searchSource, fetchDeps); // Handle results of the individual queries and forward the results to the corresponding dataSubjects response - .then(({ records, textBasedQueryColumns }) => { - dataSubjects.documents$.next({ - fetchStatus: FetchStatus.COMPLETE, - result: [...(dataSubjects.documents$.getValue().result || []), ...records], - textBasedQueryColumns, - recordRawType, - query, - }); + .then(({ records }) => { + sendLoadingMoreFinishedMsg(dataSubjects.documents$, records); }) .catch((error) => { - // TODO? // eslint-disable-next-line no-console console.error(error); + sendLoadingMoreFinishedMsg(dataSubjects.documents$, []); }); } catch (error) { // eslint-disable-next-line no-console console.error(error); // We also want to return a resolved promise in an error case, since it just indicates we're done with querying. + sendLoadingMoreFinishedMsg(dataSubjects.documents$, []); } return Promise.resolve(); } From e6a9453aa24e467d760700c1fc79613c9a5cde2b Mon Sep 17 00:00:00 2001 From: Julia Rechkunova Date: Mon, 24 Jul 2023 16:56:29 +0200 Subject: [PATCH 10/49] [Discover] Fix types --- .../application/main/services/discover_data_state_container.ts | 2 +- src/plugins/discover/public/application/main/utils/fetch_all.ts | 2 +- 2 files changed, 2 insertions(+), 2 deletions(-) diff --git a/src/plugins/discover/public/application/main/services/discover_data_state_container.ts b/src/plugins/discover/public/application/main/services/discover_data_state_container.ts index 19803abc24f71..5b1900ffa2a0b 100644 --- a/src/plugins/discover/public/application/main/services/discover_data_state_container.ts +++ b/src/plugins/discover/public/application/main/services/discover_data_state_container.ts @@ -238,7 +238,7 @@ export function getDataStateContainer({ const prevAutoRefreshDone = autoRefreshDone; const fetchAllStartTime = window.performance.now(); - await fetchAll(dataSubjects, reset, { + await fetchAll(dataSubjects, options.reset, { abortController, ...commonFetchDeps, }); diff --git a/src/plugins/discover/public/application/main/utils/fetch_all.ts b/src/plugins/discover/public/application/main/utils/fetch_all.ts index b8196e136c908..f8e17c13f230d 100644 --- a/src/plugins/discover/public/application/main/utils/fetch_all.ts +++ b/src/plugins/discover/public/application/main/utils/fetch_all.ts @@ -198,7 +198,7 @@ export function fetchMoreDocuments( // Handle results of the individual queries and forward the results to the corresponding dataSubjects response .then(({ records }) => { - sendLoadingMoreFinishedMsg(dataSubjects.documents$, records); + sendLoadingMoreFinishedMsg(dataSubjects.documents$, records); // TODO: surface shard failures }) .catch((error) => { // eslint-disable-next-line no-console From 46a7a13a5b2274c634ef34262bf3569821c6f1b6 Mon Sep 17 00:00:00 2001 From: Julia Rechkunova Date: Mon, 24 Jul 2023 17:27:55 +0200 Subject: [PATCH 11/49] [Discover] Fix default sorting order for locator --- .../utils/sorting/get_sort_for_search_source.ts | 4 ++-- .../server/locator/searchsource_from_locator.ts | 13 +++++++++++-- 2 files changed, 13 insertions(+), 4 deletions(-) diff --git a/src/plugins/discover/common/utils/sorting/get_sort_for_search_source.ts b/src/plugins/discover/common/utils/sorting/get_sort_for_search_source.ts index 5f9423db20c5b..0ac92a5391ca4 100644 --- a/src/plugins/discover/common/utils/sorting/get_sort_for_search_source.ts +++ b/src/plugins/discover/common/utils/sorting/get_sort_for_search_source.ts @@ -28,9 +28,9 @@ import { SORT_DEFAULT_ORDER_SETTING } from '../..'; export function getSortForSearchSource( sort: SortOrder[] | undefined, dataView: DataView | undefined, - uiSettings: IUiSettingsClient | null + uiSettings: Pick ): EsQuerySortValue[] { - const defaultDirection = uiSettings?.get(SORT_DEFAULT_ORDER_SETTING) || 'desc'; + const defaultDirection = uiSettings.get(SORT_DEFAULT_ORDER_SETTING) || 'desc'; if (!sort || !dataView || (Array.isArray(sort) && sort.length === 0)) { if (dataView?.timeFieldName) { diff --git a/src/plugins/discover/server/locator/searchsource_from_locator.ts b/src/plugins/discover/server/locator/searchsource_from_locator.ts index 3d2ab58767b58..94a104dcd64a1 100644 --- a/src/plugins/discover/server/locator/searchsource_from_locator.ts +++ b/src/plugins/discover/server/locator/searchsource_from_locator.ts @@ -12,7 +12,7 @@ import { AggregateQuery, Filter, Query } from '@kbn/es-query'; import { SavedSearch } from '@kbn/saved-search-plugin/common'; import { getSavedSearch } from '@kbn/saved-search-plugin/server'; import { LocatorServicesDeps } from '.'; -import { DiscoverAppLocatorParams } from '../../common'; +import { DiscoverAppLocatorParams, SORT_DEFAULT_ORDER_SETTING } from '../../common'; import { getSortForSearchSource } from '../../common/utils/sorting'; import { getColumns } from './columns_from_locator'; @@ -147,7 +147,16 @@ export function searchSourceFromLocatorFactory(services: LocatorServicesDeps) { // Inject sort if (savedSearch.sort) { - const sort = getSortForSearchSource(savedSearch.sort as Array<[string, string]>, index, null); // TODO: is it correct that it was using a `desc` as default sort here? + const defaultSortingSetting = await services.uiSettings.get(SORT_DEFAULT_ORDER_SETTING); + const uiSettingsSyncReplacement = { + get: (key: string) => + key === SORT_DEFAULT_ORDER_SETTING ? defaultSortingSetting : undefined, + }; + const sort = getSortForSearchSource( + savedSearch.sort as Array<[string, string]>, + index, + uiSettingsSyncReplacement + ); searchSource.setField('sort', sort); } From b11ed0ac90274432d89cc2a7a6ceffa8145d7f1b Mon Sep 17 00:00:00 2001 From: Julia Rechkunova Date: Mon, 24 Jul 2023 17:39:37 +0200 Subject: [PATCH 12/49] [Discover] Add a comment --- .../discover/public/application/main/utils/fetch_documents.ts | 1 + 1 file changed, 1 insertion(+) diff --git a/src/plugins/discover/public/application/main/utils/fetch_documents.ts b/src/plugins/discover/public/application/main/utils/fetch_documents.ts index 704cc9f0148cb..9ddc31139c12f 100644 --- a/src/plugins/discover/public/application/main/utils/fetch_documents.ts +++ b/src/plugins/discover/public/application/main/utils/fetch_documents.ts @@ -23,6 +23,7 @@ export const fetchDocuments = ( searchSource: ISearchSource, { abortController, inspectorAdapters, searchSessionId, services }: FetchDeps ): Promise => { + // TODO: for "Load more" do we want to keep this value or override it with a fixed value (200)? searchSource.setField('size', services.uiSettings.get(SAMPLE_SIZE_SETTING)); searchSource.setField('trackTotalHits', false); searchSource.setField('highlightAll', true); From 87f57d3d9e557a8497fd3b13a4d0360a8d8022a4 Mon Sep 17 00:00:00 2001 From: Julia Rechkunova Date: Tue, 25 Jul 2023 12:46:35 +0200 Subject: [PATCH 13/49] [Discover] Add a tooltip when refresh interval is on --- .../discover_grid/discover_grid_footer.tsx | 35 ++++++++++++++++--- 1 file changed, 30 insertions(+), 5 deletions(-) diff --git a/src/plugins/discover/public/components/discover_grid/discover_grid_footer.tsx b/src/plugins/discover/public/components/discover_grid/discover_grid_footer.tsx index df89fdb9a210a..ccfe8571ac2ff 100644 --- a/src/plugins/discover/public/components/discover_grid/discover_grid_footer.tsx +++ b/src/plugins/discover/public/components/discover_grid/discover_grid_footer.tsx @@ -6,11 +6,13 @@ * Side Public License, v 1. */ -import React from 'react'; +import React, { useEffect, useState } from 'react'; import { FormattedMessage } from '@kbn/i18n-react'; -import { EuiButtonEmpty, useEuiTheme } from '@elastic/eui'; +import { EuiButtonEmpty, EuiToolTip, useEuiTheme } from '@elastic/eui'; import { css } from '@emotion/react'; +import { i18n } from '@kbn/i18n'; import { MAX_LOADED_GRID_ROWS } from '../../../common/constants'; +import { useDiscoverServices } from '../../hooks/use_discover_services'; export interface DiscoverGridFooterProps { isLoadingMore: boolean; @@ -32,6 +34,21 @@ export const DiscoverGridFooter: React.FC = (props) => totalHits = 0, onFetchMoreRecords, } = props; + const { data } = useDiscoverServices(); + const timefilter = data.query.timefilter.timefilter; + const [refreshInterval, setRefreshInterval] = useState(timefilter.getRefreshInterval()); + + useEffect(() => { + const subscriber = timefilter.getRefreshIntervalUpdate$().subscribe(() => { + setRefreshInterval(timefilter.getRefreshInterval()); + }); + + return () => subscriber.unsubscribe(); + }, [timefilter, setRefreshInterval]); + + const isRefreshIntervalOn = + refreshInterval && refreshInterval.pause === false && refreshInterval.value > 0; + const { euiTheme } = useEuiTheme(); const isOnLastPage = pageIndex === pageCount - 1 && rowCount < totalHits; @@ -44,8 +61,17 @@ export const DiscoverGridFooter: React.FC = (props) => if (rowCount <= MAX_LOADED_GRID_ROWS - sampleSize) { return ( - <> + = (props) => defaultMessage="Load more" /> - {'.'} - + ); } From 200b9780b4bd1b9b70d9252e8bb42dc18276f1f9 Mon Sep 17 00:00:00 2001 From: Julia Rechkunova Date: Tue, 25 Jul 2023 12:47:24 +0200 Subject: [PATCH 14/49] [Discover] Update code style --- .../public/components/discover_grid/discover_grid_footer.tsx | 5 +++-- 1 file changed, 3 insertions(+), 2 deletions(-) diff --git a/src/plugins/discover/public/components/discover_grid/discover_grid_footer.tsx b/src/plugins/discover/public/components/discover_grid/discover_grid_footer.tsx index ccfe8571ac2ff..564499944c350 100644 --- a/src/plugins/discover/public/components/discover_grid/discover_grid_footer.tsx +++ b/src/plugins/discover/public/components/discover_grid/discover_grid_footer.tsx @@ -46,8 +46,9 @@ export const DiscoverGridFooter: React.FC = (props) => return () => subscriber.unsubscribe(); }, [timefilter, setRefreshInterval]); - const isRefreshIntervalOn = - refreshInterval && refreshInterval.pause === false && refreshInterval.value > 0; + const isRefreshIntervalOn = Boolean( + refreshInterval && refreshInterval.pause === false && refreshInterval.value > 0 + ); const { euiTheme } = useEuiTheme(); const isOnLastPage = pageIndex === pageCount - 1 && rowCount < totalHits; From d89ec6bc821d77b8781bb8d3d3a9fe380b2943b0 Mon Sep 17 00:00:00 2001 From: Julia Rechkunova Date: Thu, 27 Jul 2023 17:21:48 +0200 Subject: [PATCH 15/49] [Discover] Show error toasts --- .../public/application/main/utils/fetch_all.ts | 16 ++++++++++++---- .../application/main/utils/fetch_documents.ts | 9 ++++++--- 2 files changed, 18 insertions(+), 7 deletions(-) diff --git a/src/plugins/discover/public/application/main/utils/fetch_all.ts b/src/plugins/discover/public/application/main/utils/fetch_all.ts index f8e17c13f230d..3ebdcfc8f5f02 100644 --- a/src/plugins/discover/public/application/main/utils/fetch_all.ts +++ b/src/plugins/discover/public/application/main/utils/fetch_all.ts @@ -10,6 +10,7 @@ import type { SavedSearch, SortOrder } from '@kbn/saved-search-plugin/public'; import { BehaviorSubject, filter, firstValueFrom, map, merge, scan } from 'rxjs'; import { reportPerformanceMetricEvent } from '@kbn/ebt-tools'; import { isEqual } from 'lodash'; +import { i18n } from '@kbn/i18n'; import type { DiscoverAppState } from '../services/discover_app_state_container'; import { updateVolatileSearchSource } from './update_search_source'; import { getRawRecordType } from './get_raw_record_type'; @@ -194,6 +195,7 @@ export function fetchMoreDocuments( // Start fetching all required requests const response = fetchDocuments(searchSource, fetchDeps); + // TODO: show it as a separate request in Inspect flyout // Handle results of the individual queries and forward the results to the corresponding dataSubjects response @@ -201,13 +203,19 @@ export function fetchMoreDocuments( sendLoadingMoreFinishedMsg(dataSubjects.documents$, records); // TODO: surface shard failures }) .catch((error) => { - // eslint-disable-next-line no-console - console.error(error); + services.toastNotifications.addError(error, { + title: i18n.translate('discover.fetchMore.fetchingErrorTitle', { + defaultMessage: 'Error fetching more documents', + }), + }); sendLoadingMoreFinishedMsg(dataSubjects.documents$, []); }); } catch (error) { - // eslint-disable-next-line no-console - console.error(error); + services.toastNotifications.addError(error, { + title: i18n.translate('discover.fetchMore.requestErrorTitle', { + defaultMessage: 'Error requesting more documents', + }), + }); // We also want to return a resolved promise in an error case, since it just indicates we're done with querying. sendLoadingMoreFinishedMsg(dataSubjects.documents$, []); } diff --git a/src/plugins/discover/public/application/main/utils/fetch_documents.ts b/src/plugins/discover/public/application/main/utils/fetch_documents.ts index 9ddc31139c12f..21da35f938019 100644 --- a/src/plugins/discover/public/application/main/utils/fetch_documents.ts +++ b/src/plugins/discover/public/application/main/utils/fetch_documents.ts @@ -23,7 +23,7 @@ export const fetchDocuments = ( searchSource: ISearchSource, { abortController, inspectorAdapters, searchSessionId, services }: FetchDeps ): Promise => { - // TODO: for "Load more" do we want to keep this value or override it with a fixed value (200)? + // TODO: for "Load more" do we want to keep this value or override it with a fixed value (e.g. 200)? searchSource.setField('size', services.uiSettings.get(SAMPLE_SIZE_SETTING)); searchSource.setField('trackTotalHits', false); searchSource.setField('highlightAll', true); @@ -36,18 +36,21 @@ export const fetchDocuments = ( searchSource.setOverwriteDataViewType(undefined); } const dataView = searchSource.getField('index')!; + const isFetchingMore = Boolean(searchSource.getField('searchAfter')); const executionContext = { - description: 'fetch documents', + description: isFetchingMore ? 'fetch more documents' : 'fetch documents', }; + // TODO: surface shard failures after PR #161271 + const fetch$ = searchSource .fetch$({ abortSignal: abortController.signal, sessionId: searchSessionId, inspector: { adapter: inspectorAdapters.requests, - title: searchSource.getField('searchAfter') + title: isFetchingMore ? i18n.translate('discover.inspectorRequestDataTitleMoreDocuments', { defaultMessage: 'More documents', }) From e10c82cb2c506bab422351e3476f991c8c05b9a3 Mon Sep 17 00:00:00 2001 From: kibanamachine <42973632+kibanamachine@users.noreply.github.com> Date: Thu, 27 Jul 2023 16:47:37 +0000 Subject: [PATCH 16/49] [CI] Auto-commit changed files from 'node scripts/eslint --no-cache --fix' --- src/plugins/discover/public/utils/get_sharing_data.ts | 6 +----- 1 file changed, 1 insertion(+), 5 deletions(-) diff --git a/src/plugins/discover/public/utils/get_sharing_data.ts b/src/plugins/discover/public/utils/get_sharing_data.ts index ab2d631c114bc..f007138beabdb 100644 --- a/src/plugins/discover/public/utils/get_sharing_data.ts +++ b/src/plugins/discover/public/utils/get_sharing_data.ts @@ -15,11 +15,7 @@ import type { } from '@kbn/data-plugin/public'; import type { Filter } from '@kbn/es-query'; import type { SavedSearch, SortOrder } from '@kbn/saved-search-plugin/public'; -import { - DOC_HIDE_TIME_COLUMN_SETTING, - SEARCH_FIELDS_FROM_SOURCE, - SORT_DEFAULT_ORDER_SETTING, -} from '@kbn/discover-utils'; +import { DOC_HIDE_TIME_COLUMN_SETTING, SEARCH_FIELDS_FROM_SOURCE } from '@kbn/discover-utils'; import { DiscoverAppState, isEqualFilters, From e38495aafca2a3cad6ed721d12c0f6e0aacb643b Mon Sep 17 00:00:00 2001 From: Julia Rechkunova Date: Wed, 2 Aug 2023 17:22:06 +0200 Subject: [PATCH 17/49] [Discover] Fix after merging main --- .../discover/public/application/main/utils/fetch_all.ts | 6 +++--- .../public/application/main/utils/fetch_documents.ts | 6 ++---- 2 files changed, 5 insertions(+), 7 deletions(-) diff --git a/src/plugins/discover/public/application/main/utils/fetch_all.ts b/src/plugins/discover/public/application/main/utils/fetch_all.ts index bcc54db2a03ed..47cf503118488 100644 --- a/src/plugins/discover/public/application/main/utils/fetch_all.ts +++ b/src/plugins/discover/public/application/main/utils/fetch_all.ts @@ -171,7 +171,7 @@ export function fetchMoreDocuments( dataSubjects: SavedSearchData, fetchDeps: FetchDeps ): Promise { - const { getAppState, services, savedSearch } = fetchDeps; + const { getAppState, getInternalState, services, savedSearch } = fetchDeps; const searchSource = savedSearch.searchSource.createChild(); try { @@ -201,16 +201,16 @@ export function fetchMoreDocuments( dataView, services, sort: getAppState().sort as SortOrder[], + customFilters: getInternalState().customFilters, }); // Start fetching all required requests const response = fetchDocuments(searchSource, fetchDeps); - // TODO: show it as a separate request in Inspect flyout // Handle results of the individual queries and forward the results to the corresponding dataSubjects response .then(({ records }) => { - sendLoadingMoreFinishedMsg(dataSubjects.documents$, records); // TODO: surface shard failures + sendLoadingMoreFinishedMsg(dataSubjects.documents$, records); }) .catch((error) => { services.toastNotifications.addError(error, { diff --git a/src/plugins/discover/public/application/main/utils/fetch_documents.ts b/src/plugins/discover/public/application/main/utils/fetch_documents.ts index adbb960bf0dea..5f35374a5f3d2 100644 --- a/src/plugins/discover/public/application/main/utils/fetch_documents.ts +++ b/src/plugins/discover/public/application/main/utils/fetch_documents.ts @@ -22,7 +22,6 @@ export const fetchDocuments = ( searchSource: ISearchSource, { abortController, inspectorAdapters, searchSessionId, services }: FetchDeps ): Promise => { - // TODO: for "Load more" do we want to keep this value or override it with a fixed value (e.g. 200)? searchSource.setField('size', services.uiSettings.get(SAMPLE_SIZE_SETTING)); searchSource.setField('trackTotalHits', false); searchSource.setField('highlightAll', true); @@ -41,15 +40,13 @@ export const fetchDocuments = ( description: isFetchingMore ? 'fetch more documents' : 'fetch documents', }; - // TODO: surface shard failures after PR #161271 - const fetch$ = searchSource .fetch$({ abortSignal: abortController.signal, sessionId: searchSessionId, inspector: { adapter: inspectorAdapters.requests, - title: isFetchingMore + title: isFetchingMore // TODO: show it as a separate request in Inspect flyout ? i18n.translate('discover.inspectorRequestDataTitleMoreDocuments', { defaultMessage: 'More documents', }) @@ -61,6 +58,7 @@ export const fetchDocuments = ( }), }, executionContext, + // TODO: after PR #161271, keep shard failures as toasts for "Load more" }) .pipe( filter((res) => isCompleteResponse(res)), From f7759a32c0b235da2a6eb9ab5c2ceef88d346536 Mon Sep 17 00:00:00 2001 From: Julia Rechkunova Date: Fri, 4 Aug 2023 13:29:50 +0200 Subject: [PATCH 18/49] [Discover] Update comment --- .../utils/sorting/get_sort_for_search_source.ts | 11 +++++------ 1 file changed, 5 insertions(+), 6 deletions(-) diff --git a/src/plugins/discover/common/utils/sorting/get_sort_for_search_source.ts b/src/plugins/discover/common/utils/sorting/get_sort_for_search_source.ts index c67109a6d546c..742e6cf4ed5ac 100644 --- a/src/plugins/discover/common/utils/sorting/get_sort_for_search_source.ts +++ b/src/plugins/discover/common/utils/sorting/get_sort_for_search_source.ts @@ -12,11 +12,7 @@ import type { SortOrder } from '@kbn/saved-search-plugin/public'; import type { IUiSettingsClient } from '@kbn/core-ui-settings-browser'; import { SORT_DEFAULT_ORDER_SETTING } from '@kbn/discover-utils'; import { getSort } from './get_sort'; -import { - getESQuerySortForTimeField, - // getESQuerySortForTieBreaker, - // getTieBreakerField, -} from './get_es_query_sort'; +import { getESQuerySortForTimeField } from './get_es_query_sort'; /** * Prepares sort for search source, that's sending the request to ES @@ -55,7 +51,10 @@ export function getSortForSearchSource( return sortPair as EsQuerySortValue; }); - // TODO: do we need to have a tie breaker like this? + // Do we need to have a tie breaker like this? We have it for Surrounding Documents page but not here yet. + // Introducing it here might become an unexpected change for users of Discover and also CSV reports + // as it will change the ordering of documents. + // if (dataView.isTimeBased() && sortPairs.length) { // const firstSortPair = sortPairs[0]; // const firstPairSortDir = Array.isArray(firstSortPair) From d50cd7e410468cf048e4326861cb2aed879a35b7 Mon Sep 17 00:00:00 2001 From: Julia Rechkunova Date: Fri, 4 Aug 2023 13:57:53 +0200 Subject: [PATCH 19/49] [Discover] Remove redundant param --- .../discover/public/embeddable/saved_search_embeddable.tsx | 2 -- .../public/embeddable/utils/update_search_source.test.ts | 1 - .../discover/public/embeddable/utils/update_search_source.ts | 1 - 3 files changed, 4 deletions(-) diff --git a/src/plugins/discover/public/embeddable/saved_search_embeddable.tsx b/src/plugins/discover/public/embeddable/saved_search_embeddable.tsx index b88043a6d7da5..b8009d1f83a7a 100644 --- a/src/plugins/discover/public/embeddable/saved_search_embeddable.tsx +++ b/src/plugins/discover/public/embeddable/saved_search_embeddable.tsx @@ -52,7 +52,6 @@ import { SAMPLE_SIZE_SETTING, SEARCH_FIELDS_FROM_SOURCE, SHOW_FIELD_STATISTICS, - SORT_DEFAULT_ORDER_SETTING, buildDataTableRecord, } from '@kbn/discover-utils'; import { VIEW_MODE } from '../../common/constants'; @@ -271,7 +270,6 @@ export class SavedSearchEmbeddable useNewFieldsApi, { sampleSize: this.services.uiSettings.get(SAMPLE_SIZE_SETTING), - defaultSort: this.services.uiSettings.get(SORT_DEFAULT_ORDER_SETTING), }, this.services.uiSettings ); diff --git a/src/plugins/discover/public/embeddable/utils/update_search_source.test.ts b/src/plugins/discover/public/embeddable/utils/update_search_source.test.ts index ccd8592c37a32..9e291feabaa01 100644 --- a/src/plugins/discover/public/embeddable/utils/update_search_source.test.ts +++ b/src/plugins/discover/public/embeddable/utils/update_search_source.test.ts @@ -20,7 +20,6 @@ jest describe('updateSearchSource', () => { const defaults = { sampleSize: 50, - defaultSort: 'asc', }; it('updates a given search source', async () => { diff --git a/src/plugins/discover/public/embeddable/utils/update_search_source.ts b/src/plugins/discover/public/embeddable/utils/update_search_source.ts index 8f20464e73b58..fae4c9e453212 100644 --- a/src/plugins/discover/public/embeddable/utils/update_search_source.ts +++ b/src/plugins/discover/public/embeddable/utils/update_search_source.ts @@ -18,7 +18,6 @@ export const updateSearchSource = ( useNewFieldsApi: boolean, defaults: { sampleSize: number; - defaultSort: string; // TODO: remove }, uiSettings: IUiSettingsClient ) => { From ebadf470bd9cac8656135359ea90f7e09ea9f7dc Mon Sep 17 00:00:00 2001 From: Julia Rechkunova Date: Fri, 4 Aug 2023 14:04:25 +0200 Subject: [PATCH 20/49] [Discover] Refactor params of getSortForSearchSource --- .../get_sort_for_search_source.test.ts | 46 +++++++++++++------ .../sorting/get_sort_for_search_source.ts | 14 ++++-- .../main/utils/update_search_source.ts | 2 +- .../embeddable/utils/update_search_source.ts | 2 +- .../discover/public/utils/get_sharing_data.ts | 11 +++-- .../locator/searchsource_from_locator.ts | 11 +++-- 6 files changed, 57 insertions(+), 29 deletions(-) diff --git a/src/plugins/discover/common/utils/sorting/get_sort_for_search_source.test.ts b/src/plugins/discover/common/utils/sorting/get_sort_for_search_source.test.ts index 95bf162e319bf..c94d49c60ba3e 100644 --- a/src/plugins/discover/common/utils/sorting/get_sort_for_search_source.test.ts +++ b/src/plugins/discover/common/utils/sorting/get_sort_for_search_source.test.ts @@ -26,31 +26,51 @@ describe('getSortForSearchSource function', function () { test('should return an object to use for searchSource when columns are given', function () { const cols = [['bytes', 'desc']] as SortOrder[]; - expect(getSortForSearchSource(cols, stubDataView, uiSettings)).toEqual([{ bytes: 'desc' }]); - expect(getSortForSearchSource(cols, stubDataView, uiSettingWithAscSorting)).toEqual([ + expect(getSortForSearchSource({ sort: cols, dataView: stubDataView, uiSettings })).toEqual([ { bytes: 'desc' }, ]); + expect( + getSortForSearchSource({ + sort: cols, + dataView: stubDataView, + uiSettings: uiSettingWithAscSorting, + }) + ).toEqual([{ bytes: 'desc' }]); - expect(getSortForSearchSource(cols, stubDataViewWithoutTimeField, uiSettings)).toEqual([ - { bytes: 'desc' }, - ]); expect( - getSortForSearchSource(cols, stubDataViewWithoutTimeField, uiSettingWithAscSorting) + getSortForSearchSource({ sort: cols, dataView: stubDataViewWithoutTimeField, uiSettings }) + ).toEqual([{ bytes: 'desc' }]); + expect( + getSortForSearchSource({ + sort: cols, + dataView: stubDataViewWithoutTimeField, + uiSettings: uiSettingWithAscSorting, + }) ).toEqual([{ bytes: 'desc' }]); }); test('should return an object to use for searchSource when no columns are given', function () { const cols = [] as SortOrder[]; - expect(getSortForSearchSource(cols, stubDataView, uiSettings)).toEqual([{ _doc: 'desc' }]); - expect(getSortForSearchSource(cols, stubDataView, uiSettingWithAscSorting)).toEqual([ - { _doc: 'asc' }, + expect(getSortForSearchSource({ sort: cols, dataView: stubDataView, uiSettings })).toEqual([ + { _doc: 'desc' }, ]); + expect( + getSortForSearchSource({ + sort: cols, + dataView: stubDataView, + uiSettings: uiSettingWithAscSorting, + }) + ).toEqual([{ _doc: 'asc' }]); - expect(getSortForSearchSource(cols, stubDataViewWithoutTimeField, uiSettings)).toEqual([ - { _score: 'desc' }, - ]); expect( - getSortForSearchSource(cols, stubDataViewWithoutTimeField, uiSettingWithAscSorting) + getSortForSearchSource({ sort: cols, dataView: stubDataViewWithoutTimeField, uiSettings }) + ).toEqual([{ _score: 'desc' }]); + expect( + getSortForSearchSource({ + sort: cols, + dataView: stubDataViewWithoutTimeField, + uiSettings: uiSettingWithAscSorting, + }) ).toEqual([{ _score: 'asc' }]); }); }); diff --git a/src/plugins/discover/common/utils/sorting/get_sort_for_search_source.ts b/src/plugins/discover/common/utils/sorting/get_sort_for_search_source.ts index 742e6cf4ed5ac..611f1d7646295 100644 --- a/src/plugins/discover/common/utils/sorting/get_sort_for_search_source.ts +++ b/src/plugins/discover/common/utils/sorting/get_sort_for_search_source.ts @@ -21,11 +21,15 @@ import { getESQuerySortForTimeField } from './get_es_query_sort'; * the addon of the numeric_type guarantees the right sort order * when there are indices with date and indices with date_nanos field */ -export function getSortForSearchSource( - sort: SortOrder[] | undefined, - dataView: DataView | undefined, - uiSettings: Pick -): EsQuerySortValue[] { +export function getSortForSearchSource({ + sort, + dataView, + uiSettings, +}: { + sort: SortOrder[] | undefined; + dataView: DataView | undefined; + uiSettings: Pick; +}): EsQuerySortValue[] { const defaultDirection = uiSettings.get(SORT_DEFAULT_ORDER_SETTING) || 'desc'; if (!sort || !dataView || (Array.isArray(sort) && sort.length === 0)) { diff --git a/src/plugins/discover/public/application/main/utils/update_search_source.ts b/src/plugins/discover/public/application/main/utils/update_search_source.ts index e825230704bd4..d3798ef77b7bf 100644 --- a/src/plugins/discover/public/application/main/utils/update_search_source.ts +++ b/src/plugins/discover/public/application/main/utils/update_search_source.ts @@ -34,7 +34,7 @@ export function updateVolatileSearchSource( const { uiSettings, data } = services; const useNewFieldsApi = !uiSettings.get(SEARCH_FIELDS_FROM_SOURCE); - const usedSort = getSortForSearchSource(sort, dataView, uiSettings); + const usedSort = getSortForSearchSource({ sort, dataView, uiSettings }); searchSource.setField('sort', usedSort); searchSource.setField('trackTotalHits', true); diff --git a/src/plugins/discover/public/embeddable/utils/update_search_source.ts b/src/plugins/discover/public/embeddable/utils/update_search_source.ts index fae4c9e453212..36cb203d1ffa2 100644 --- a/src/plugins/discover/public/embeddable/utils/update_search_source.ts +++ b/src/plugins/discover/public/embeddable/utils/update_search_source.ts @@ -23,7 +23,7 @@ export const updateSearchSource = ( ) => { const { sampleSize } = defaults; searchSource.setField('size', sampleSize); - searchSource.setField('sort', getSortForSearchSource(sort, dataView, uiSettings)); + searchSource.setField('sort', getSortForSearchSource({ sort, dataView, uiSettings })); if (useNewFieldsApi) { searchSource.removeField('fieldsFromSource'); const fields: Record = { field: '*', include_unmapped: 'true' }; diff --git a/src/plugins/discover/public/utils/get_sharing_data.ts b/src/plugins/discover/public/utils/get_sharing_data.ts index f007138beabdb..4b34cff05db67 100644 --- a/src/plugins/discover/public/utils/get_sharing_data.ts +++ b/src/plugins/discover/public/utils/get_sharing_data.ts @@ -31,12 +31,15 @@ export async function getSharingData( services: { uiSettings: IUiSettingsClient; data: DataPublicPluginStart }, isPlainRecord?: boolean ) { - const { uiSettings: config, data } = services; + const { uiSettings, data } = services; const searchSource = currentSearchSource.createCopy(); const index = searchSource.getField('index')!; let existingFilter = searchSource.getField('filter') as Filter[] | Filter | undefined; - searchSource.setField('sort', getSortForSearchSource(state.sort as SortOrder[], index, config)); + searchSource.setField( + 'sort', + getSortForSearchSource({ sort: state.sort as SortOrder[], dataView: index, uiSettings }) + ); searchSource.removeField('filter'); searchSource.removeField('highlight'); @@ -50,7 +53,7 @@ export async function getSharingData( if (columns && columns.length > 0) { // conditionally add the time field column: let timeFieldName: string | undefined; - const hideTimeColumn = config.get(DOC_HIDE_TIME_COLUMN_SETTING); + const hideTimeColumn = uiSettings.get(DOC_HIDE_TIME_COLUMN_SETTING); if (!hideTimeColumn && index && index.timeFieldName && !isPlainRecord) { timeFieldName = index.timeFieldName; } @@ -91,7 +94,7 @@ export async function getSharingData( * Otherwise, the requests will ask for all fields, even if only a few are really needed. * Discover does not set fields, since having all fields is needed for the UI. */ - const useFieldsApi = !config.get(SEARCH_FIELDS_FROM_SOURCE); + const useFieldsApi = !uiSettings.get(SEARCH_FIELDS_FROM_SOURCE); if (useFieldsApi) { searchSource.removeField('fieldsFromSource'); const fields = columns.length diff --git a/src/plugins/discover/server/locator/searchsource_from_locator.ts b/src/plugins/discover/server/locator/searchsource_from_locator.ts index 492fdf0d96abb..9d9f1e924fe01 100644 --- a/src/plugins/discover/server/locator/searchsource_from_locator.ts +++ b/src/plugins/discover/server/locator/searchsource_from_locator.ts @@ -153,11 +153,12 @@ export function searchSourceFromLocatorFactory(services: LocatorServicesDeps) { get: (key: string) => key === SORT_DEFAULT_ORDER_SETTING ? defaultSortingSetting : undefined, }; - const sort = getSortForSearchSource( - savedSearch.sort as Array<[string, string]>, - index, - uiSettingsSyncReplacement - ); + + const sort = getSortForSearchSource({ + sort: savedSearch.sort as Array<[string, string]>, + dataView: index, + uiSettings: uiSettingsSyncReplacement, + }); searchSource.setField('sort', sort); } From 8c5f2f210558877b99d5ddd75b69273aaf52fdf7 Mon Sep 17 00:00:00 2001 From: Julia Rechkunova Date: Fri, 4 Aug 2023 14:15:01 +0200 Subject: [PATCH 21/49] [Discover] Refactor params of get_es_query_sort --- .../common/utils/sorting/get_es_query_sort.ts | 56 +++++++++++-------- .../sorting/get_sort_for_search_source.ts | 10 ++-- .../context/hooks/use_context_app_fetch.tsx | 27 +++++---- .../application/context/services/context.ts | 12 ++-- 4 files changed, 60 insertions(+), 45 deletions(-) diff --git a/src/plugins/discover/common/utils/sorting/get_es_query_sort.ts b/src/plugins/discover/common/utils/sorting/get_es_query_sort.ts index 54960698119c4..ffe200f24a545 100644 --- a/src/plugins/discover/common/utils/sorting/get_es_query_sort.ts +++ b/src/plugins/discover/common/utils/sorting/get_es_query_sort.ts @@ -14,30 +14,39 @@ import { CONTEXT_TIE_BREAKER_FIELDS_SETTING } from '@kbn/discover-utils'; /** * Returns `EsQuerySort` which is used to sort records in the ES query * https://www.elastic.co/guide/en/elasticsearch/reference/current/search-request-sort.html - * @param timeField - * @param tieBreakerField * @param sortDir + * @param timeFieldName + * @param tieBreakerFieldName * @param isTimeNanosBased */ -export function getEsQuerySort( - timeField: string, - tieBreakerField: string, - sortDir: SortDirection, - isTimeNanosBased: boolean -): [EsQuerySortValue, EsQuerySortValue] { +export function getEsQuerySort({ + sortDir, + timeFieldName, + tieBreakerFieldName, + isTimeNanosBased, +}: { + sortDir: SortDirection; + timeFieldName: string; + tieBreakerFieldName: string; + isTimeNanosBased: boolean; +}): [EsQuerySortValue, EsQuerySortValue] { return [ - getESQuerySortForTimeField(timeField, sortDir, isTimeNanosBased), - getESQuerySortForTieBreaker(tieBreakerField, sortDir), + getESQuerySortForTimeField({ sortDir, timeFieldName, isTimeNanosBased }), + getESQuerySortForTieBreaker({ sortDir, tieBreakerFieldName }), ]; } -export function getESQuerySortForTimeField( - timeField: string, - sortDir: SortDirection, - isTimeNanosBased: boolean -): EsQuerySortValue { +export function getESQuerySortForTimeField({ + sortDir, + timeFieldName, + isTimeNanosBased, +}: { + sortDir: SortDirection; + timeFieldName: string; + isTimeNanosBased: boolean; +}): EsQuerySortValue { return { - [timeField]: { + [timeFieldName]: { order: sortDir, ...(isTimeNanosBased ? { @@ -49,11 +58,14 @@ export function getESQuerySortForTimeField( }; } -export function getESQuerySortForTieBreaker( - tieBreakerField: string, - sortDir: SortDirection -): EsQuerySortValue { - return { [tieBreakerField]: sortDir }; +export function getESQuerySortForTieBreaker({ + sortDir, + tieBreakerFieldName, +}: { + sortDir: SortDirection; + tieBreakerFieldName: string; +}): EsQuerySortValue { + return { [tieBreakerFieldName]: sortDir }; } /** @@ -66,7 +78,7 @@ const META_FIELD_NAMES: string[] = ['_seq_no', '_doc', '_uid']; * Returns a field from the intersection of the set of sortable fields in the * given data view and a given set of candidate field names. */ -export function getTieBreakerField(dataView: DataView, uiSettings: IUiSettingsClient) { +export function getTieBreakerFieldName(dataView: DataView, uiSettings: IUiSettingsClient) { const sortableFields = uiSettings .get(CONTEXT_TIE_BREAKER_FIELDS_SETTING) .filter( diff --git a/src/plugins/discover/common/utils/sorting/get_sort_for_search_source.ts b/src/plugins/discover/common/utils/sorting/get_sort_for_search_source.ts index 611f1d7646295..f6cea7c89311c 100644 --- a/src/plugins/discover/common/utils/sorting/get_sort_for_search_source.ts +++ b/src/plugins/discover/common/utils/sorting/get_sort_for_search_source.ts @@ -46,11 +46,11 @@ export function getSortForSearchSource({ const sortForSearchSource = sortPairs.map((sortPair: Record) => { if (timeFieldName && sortPair[timeFieldName]) { - return getESQuerySortForTimeField( + return getESQuerySortForTimeField({ + sortDir: sortPair[timeFieldName] as SortDirection, timeFieldName, - sortPair[timeFieldName] as SortDirection, - dataView.isTimeNanosBased() - ); + isTimeNanosBased: dataView.isTimeNanosBased(), + }); } return sortPair as EsQuerySortValue; }); @@ -65,7 +65,7 @@ export function getSortForSearchSource({ // ? firstSortPair[1] // : Object.values(firstSortPair)[0]; // sortForSearchSource.push( - // getESQuerySortForTieBreaker(getTieBreakerField(dataView, uiSettings), firstPairSortDir) + // getESQuerySortForTieBreaker(getTieBreakerFieldName(dataView, uiSettings), firstPairSortDir) // ); // } diff --git a/src/plugins/discover/public/application/context/hooks/use_context_app_fetch.tsx b/src/plugins/discover/public/application/context/hooks/use_context_app_fetch.tsx index 7061622783e0f..ab11697dddf25 100644 --- a/src/plugins/discover/public/application/context/hooks/use_context_app_fetch.tsx +++ b/src/plugins/discover/public/application/context/hooks/use_context_app_fetch.tsx @@ -22,7 +22,7 @@ import { import { AppState } from '../services/context_state'; import { useDiscoverServices } from '../../../hooks/use_discover_services'; import { - getTieBreakerField, + getTieBreakerFieldName, getEsQuerySort, } from '../../../../common/utils/sorting/get_es_query_sort'; @@ -55,7 +55,10 @@ export function useContextAppFetch({ const searchSource = useMemo(() => { return data.search.searchSource.createEmpty(); }, [data.search.searchSource]); - const tieBreakerField = useMemo(() => getTieBreakerField(dataView, config), [config, dataView]); + const tieBreakerFieldName = useMemo( + () => getTieBreakerFieldName(dataView, config), + [config, dataView] + ); const [fetchedState, setFetchedState] = useState( getInitialContextQueryState() @@ -70,7 +73,7 @@ export function useContextAppFetch({ defaultMessage: 'Unable to load the anchor document', }); - if (!tieBreakerField) { + if (!tieBreakerFieldName) { setState(createError('anchorStatus', FailureReason.INVALID_TIEBREAKER)); toastNotifications.addDanger({ title: errorTitle, @@ -90,12 +93,12 @@ export function useContextAppFetch({ try { setState({ anchorStatus: { value: LoadingStatus.LOADING } }); - const sort = getEsQuerySort( - dataView.timeFieldName!, - tieBreakerField, - SortDirection.desc, - dataView.isTimeNanosBased() - ); + const sort = getEsQuerySort({ + sortDir: SortDirection.desc, + timeFieldName: dataView.timeFieldName!, + tieBreakerFieldName, + isTimeNanosBased: dataView.isTimeNanosBased(), + }); const anchor = await fetchAnchor(anchorId, dataView, searchSource, sort, useNewFieldsApi); setState({ anchor, anchorStatus: { value: LoadingStatus.LOADED } }); return anchor; @@ -107,7 +110,7 @@ export function useContextAppFetch({ }); } }, [ - tieBreakerField, + tieBreakerFieldName, setState, toastNotifications, dataView, @@ -136,7 +139,7 @@ export function useContextAppFetch({ type, dataView, anchor, - tieBreakerField, + tieBreakerFieldName, SortDirection.desc, count, filters, @@ -159,7 +162,7 @@ export function useContextAppFetch({ filterManager, appState, fetchedState.anchor, - tieBreakerField, + tieBreakerFieldName, setState, dataView, toastNotifications, diff --git a/src/plugins/discover/public/application/context/services/context.ts b/src/plugins/discover/public/application/context/services/context.ts index f599e65a322b1..50ef5dba1ae1b 100644 --- a/src/plugins/discover/public/application/context/services/context.ts +++ b/src/plugins/discover/public/application/context/services/context.ts @@ -75,12 +75,12 @@ export async function fetchSurroundingDocs( const searchAfter = getEsQuerySearchAfter(type, documents, timeField, anchor); - const sort = getEsQuerySort( - timeField, - tieBreakerField, - sortDirToApply, - dataView.isTimeNanosBased() - ); + const sort = getEsQuerySort({ + timeFieldName: timeField, + tieBreakerFieldName: tieBreakerField, + sortDir: sortDirToApply, + isTimeNanosBased: dataView.isTimeNanosBased(), + }); const hits = await fetchHitsInInterval( searchSource, From 08281ea8c33c2d24dacd9d5367887642bb203c8e Mon Sep 17 00:00:00 2001 From: Julia Rechkunova Date: Fri, 4 Aug 2023 15:18:39 +0200 Subject: [PATCH 22/49] [Discover] Don't update histogram for "Load more" in the grid --- .../main/components/layout/use_discover_histogram.ts | 5 ++++- 1 file changed, 4 insertions(+), 1 deletion(-) diff --git a/src/plugins/discover/public/application/main/components/layout/use_discover_histogram.ts b/src/plugins/discover/public/application/main/components/layout/use_discover_histogram.ts index 35a0e24c99b55..692954f14973c 100644 --- a/src/plugins/discover/public/application/main/components/layout/use_discover_histogram.ts +++ b/src/plugins/discover/public/application/main/components/layout/use_discover_histogram.ts @@ -280,7 +280,10 @@ export const useDiscoverHistogram = ({ textBasedFetchComplete$.pipe(map(() => 'discover')) ).pipe(debounceTime(50)); } else { - fetch$ = stateContainer.dataState.fetch$.pipe(map(() => 'discover')); + fetch$ = stateContainer.dataState.fetch$.pipe( + filter(({ options }) => !options.fetchMore), // don't update histogram for "Load more" in the grid + map(() => 'discover') + ); } const subscription = fetch$.subscribe((source) => { From 13d5b0af3bbd4d806cee3da67644c17201b2ec45 Mon Sep 17 00:00:00 2001 From: Julia Rechkunova Date: Fri, 4 Aug 2023 15:36:40 +0200 Subject: [PATCH 23/49] [Discover] Fix footer for Surrounding Documents and Dashboard pages --- .../discover_grid/discover_grid.tsx | 23 ++++++++++--------- .../discover_grid/discover_grid_footer.tsx | 4 ++-- .../public/embeddable/saved_search_grid.tsx | 1 + 3 files changed, 15 insertions(+), 13 deletions(-) diff --git a/src/plugins/discover/public/components/discover_grid/discover_grid.tsx b/src/plugins/discover/public/components/discover_grid/discover_grid.tsx index 599b34229900d..f90025e695a76 100644 --- a/src/plugins/discover/public/components/discover_grid/discover_grid.tsx +++ b/src/plugins/discover/public/components/discover_grid/discover_grid.tsx @@ -696,17 +696,18 @@ export const DiscoverGrid = ({ gridStyle={GRID_STYLE} /> - {!isLoading && typeof isLoadingMore === 'boolean' && ( - - )} + {!isLoading && + isPaginationEnabled && ( // we hide the footer for Surrounding Documents page + + )} {searchTitle && (

diff --git a/src/plugins/discover/public/components/discover_grid/discover_grid_footer.tsx b/src/plugins/discover/public/components/discover_grid/discover_grid_footer.tsx index 564499944c350..9cd00919d2f13 100644 --- a/src/plugins/discover/public/components/discover_grid/discover_grid_footer.tsx +++ b/src/plugins/discover/public/components/discover_grid/discover_grid_footer.tsx @@ -15,7 +15,7 @@ import { MAX_LOADED_GRID_ROWS } from '../../../common/constants'; import { useDiscoverServices } from '../../hooks/use_discover_services'; export interface DiscoverGridFooterProps { - isLoadingMore: boolean; + isLoadingMore?: boolean; rowCount: number; sampleSize: number; pageIndex?: number; @@ -58,7 +58,7 @@ export const DiscoverGridFooter: React.FC = (props) => } // allow to fetch more records on Discover page - if (onFetchMoreRecords) { + if (onFetchMoreRecords && typeof isLoadingMore === 'boolean') { if (rowCount <= MAX_LOADED_GRID_ROWS - sampleSize) { return ( diff --git a/src/plugins/discover/public/embeddable/saved_search_grid.tsx b/src/plugins/discover/public/embeddable/saved_search_grid.tsx index 075a3ca930235..fa21c9868468d 100644 --- a/src/plugins/discover/public/embeddable/saved_search_grid.tsx +++ b/src/plugins/discover/public/embeddable/saved_search_grid.tsx @@ -29,6 +29,7 @@ export function DiscoverGridEmbeddable(props: DiscoverGridEmbeddableProps) { > Date: Fri, 4 Aug 2023 16:58:02 +0200 Subject: [PATCH 24/49] [Discover] Add unit tests --- .../utils/sorting/get_es_query_sort.test.ts | 144 ++++++++++++++++++ .../common/utils/sorting/get_es_query_sort.ts | 17 ++- 2 files changed, 153 insertions(+), 8 deletions(-) create mode 100644 src/plugins/discover/common/utils/sorting/get_es_query_sort.test.ts diff --git a/src/plugins/discover/common/utils/sorting/get_es_query_sort.test.ts b/src/plugins/discover/common/utils/sorting/get_es_query_sort.test.ts new file mode 100644 index 0000000000000..e0b6923f8807a --- /dev/null +++ b/src/plugins/discover/common/utils/sorting/get_es_query_sort.test.ts @@ -0,0 +1,144 @@ +/* + * 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 { SortDirection } from '@kbn/data-plugin/common'; +import { createStubDataView } from '@kbn/data-views-plugin/common/data_view.stub'; +import { + getEsQuerySort, + getESQuerySortForTieBreaker, + getESQuerySortForTimeField, + getTieBreakerFieldName, +} from './get_es_query_sort'; +import { CONTEXT_TIE_BREAKER_FIELDS_SETTING } from '@kbn/discover-utils'; +import { IUiSettingsClient } from '@kbn/core-ui-settings-browser'; + +const dataView = createStubDataView({ + spec: { + id: 'logstash-*', + fields: { + test_field: { + name: 'test_field', + type: 'string', + esTypes: ['keyword'], + aggregatable: true, + searchable: true, + }, + test_field_not_sortable: { + name: 'test_field_not_sortable', + type: 'string', + esTypes: ['keyword'], + aggregatable: false, + searchable: false, + }, + }, + title: 'logstash-*', + timeFieldName: '@timestamp', + }, +}); + +describe('get_es_query_sort', function () { + test('getEsQuerySort should return sort params', function () { + expect( + getEsQuerySort({ + sortDir: SortDirection.desc, + timeFieldName: 'testTimeField', + isTimeNanosBased: false, + tieBreakerFieldName: 'testTieBreakerField', + }) + ).toStrictEqual([ + { testTimeField: { format: 'strict_date_optional_time', order: 'desc' } }, + { testTieBreakerField: 'desc' }, + ]); + + expect( + getEsQuerySort({ + sortDir: SortDirection.asc, + timeFieldName: 'testTimeField', + isTimeNanosBased: true, + tieBreakerFieldName: 'testTieBreakerField', + }) + ).toStrictEqual([ + { + testTimeField: { + format: 'strict_date_optional_time_nanos', + numeric_type: 'date_nanos', + order: 'asc', + }, + }, + { testTieBreakerField: 'asc' }, + ]); + }); + + test('getESQuerySortForTimeField should return time field as sort param', function () { + expect( + getESQuerySortForTimeField({ + sortDir: SortDirection.desc, + timeFieldName: 'testTimeField', + isTimeNanosBased: false, + }) + ).toStrictEqual({ + testTimeField: { + format: 'strict_date_optional_time', + order: 'desc', + }, + }); + + expect( + getESQuerySortForTimeField({ + sortDir: SortDirection.asc, + timeFieldName: 'testTimeField', + isTimeNanosBased: true, + }) + ).toStrictEqual({ + testTimeField: { + format: 'strict_date_optional_time_nanos', + numeric_type: 'date_nanos', + order: 'asc', + }, + }); + }); + + test('getESQuerySortForTieBreaker should return tie breaker as sort param', function () { + expect( + getESQuerySortForTieBreaker({ + sortDir: SortDirection.desc, + tieBreakerFieldName: 'testTieBreaker', + }) + ).toStrictEqual({ testTieBreaker: 'desc' }); + }); + + test('getTieBreakerFieldName should return a correct tie breaker', function () { + expect( + getTieBreakerFieldName(dataView, { + get: (key) => (key === CONTEXT_TIE_BREAKER_FIELDS_SETTING ? ['_doc'] : undefined), + } as IUiSettingsClient) + ).toBe('_doc'); + + expect( + getTieBreakerFieldName(dataView, { + get: (key) => + key === CONTEXT_TIE_BREAKER_FIELDS_SETTING + ? ['test_field_not_sortable', '_doc'] + : undefined, + } as IUiSettingsClient) + ).toBe('_doc'); + + expect( + getTieBreakerFieldName(dataView, { + get: (key) => + key === CONTEXT_TIE_BREAKER_FIELDS_SETTING ? ['test_field', '_doc'] : undefined, + } as IUiSettingsClient) + ).toBe('test_field'); + + expect( + getTieBreakerFieldName(dataView, { + get: (key) => undefined, + } as IUiSettingsClient) + ).toBeUndefined(); + }); +}); diff --git a/src/plugins/discover/common/utils/sorting/get_es_query_sort.ts b/src/plugins/discover/common/utils/sorting/get_es_query_sort.ts index ffe200f24a545..b1587781a4284 100644 --- a/src/plugins/discover/common/utils/sorting/get_es_query_sort.ts +++ b/src/plugins/discover/common/utils/sorting/get_es_query_sort.ts @@ -78,13 +78,14 @@ const META_FIELD_NAMES: string[] = ['_seq_no', '_doc', '_uid']; * Returns a field from the intersection of the set of sortable fields in the * given data view and a given set of candidate field names. */ -export function getTieBreakerFieldName(dataView: DataView, uiSettings: IUiSettingsClient) { - const sortableFields = uiSettings - .get(CONTEXT_TIE_BREAKER_FIELDS_SETTING) - .filter( - (fieldName: string) => - META_FIELD_NAMES.includes(fieldName) || - (dataView.fields.getByName(fieldName) || { sortable: false }).sortable - ); +export function getTieBreakerFieldName( + dataView: DataView, + uiSettings: Pick +) { + const sortableFields = (uiSettings.get(CONTEXT_TIE_BREAKER_FIELDS_SETTING) || []).filter( + (fieldName: string) => + META_FIELD_NAMES.includes(fieldName) || + (dataView.fields.getByName(fieldName) || { sortable: false }).sortable + ); return sortableFields[0]; } From c193c644a08145f323bfc381c05f37ace2a81376 Mon Sep 17 00:00:00 2001 From: Julia Rechkunova Date: Mon, 7 Aug 2023 11:18:58 +0200 Subject: [PATCH 25/49] [Discover] Add tests for the footer --- .../discover_grid_footer.test.tsx | 138 ++++++++++++++++++ .../discover_grid/discover_grid_footer.tsx | 2 +- 2 files changed, 139 insertions(+), 1 deletion(-) create mode 100644 src/plugins/discover/public/components/discover_grid/discover_grid_footer.test.tsx diff --git a/src/plugins/discover/public/components/discover_grid/discover_grid_footer.test.tsx b/src/plugins/discover/public/components/discover_grid/discover_grid_footer.test.tsx new file mode 100644 index 0000000000000..a4bfe84b68126 --- /dev/null +++ b/src/plugins/discover/public/components/discover_grid/discover_grid_footer.test.tsx @@ -0,0 +1,138 @@ +/* + * 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 React from 'react'; +import { mountWithIntl } from '@kbn/test-jest-helpers'; +import { findTestSubject } from '@elastic/eui/lib/test'; +import { KibanaContextProvider } from '@kbn/kibana-react-plugin/public'; +import { DiscoverGridFooter } from './discover_grid_footer'; +import { discoverServiceMock } from '../../__mocks__/services'; + +describe('DiscoverGridFooter', function () { + it('should not render anything when not on the last page', async () => { + const component = mountWithIntl( + + + + ); + expect(component.isEmptyRender()).toBe(true); + }); + + it('should not render anything yet when all rows shown', async () => { + const component = mountWithIntl( + + + + ); + expect(component.isEmptyRender()).toBe(true); + }); + + it('should render a message for the last page', async () => { + const component = mountWithIntl( + + + + ); + expect(findTestSubject(component, 'discoverTableFooter').text()).toBe( + 'Search results are limited to 500 documents. Add more search terms to narrow your search.' + ); + expect(findTestSubject(component, 'dscGridSampleSizeFetchMoreLink').exists()).toBe(false); + }); + + it('should render a message and the button for the last page', async () => { + const mockLoadMore = jest.fn(); + + const component = mountWithIntl( + + + + ); + expect(findTestSubject(component, 'discoverTableFooter').text()).toBe( + 'Search results are limited to 500 documents.Load more' + ); + + const button = findTestSubject(component, 'dscGridSampleSizeFetchMoreLink'); + expect(button.exists()).toBe(true); + + button.simulate('click'); + + expect(mockLoadMore).toHaveBeenCalledTimes(1); + }); + + it('should render a disabled button when loading more', async () => { + const mockLoadMore = jest.fn(); + + const component = mountWithIntl( + + + + ); + expect(findTestSubject(component, 'discoverTableFooter').text()).toBe( + 'Search results are limited to 500 documents.Load more' + ); + + const button = findTestSubject(component, 'dscGridSampleSizeFetchMoreLink'); + expect(button.exists()).toBe(true); + expect(button.prop('disabled')).toBe(true); + + button.simulate('click'); + + expect(mockLoadMore).not.toHaveBeenCalled(); + }); + + it('should render a message when max total limit is reached', async () => { + const component = mountWithIntl( + + + + ); + expect(findTestSubject(component, 'discoverTableFooter').text()).toBe( + 'Search results are limited to 10000 documents. Add more search terms to narrow your search.' + ); + }); +}); diff --git a/src/plugins/discover/public/components/discover_grid/discover_grid_footer.tsx b/src/plugins/discover/public/components/discover_grid/discover_grid_footer.tsx index 9cd00919d2f13..16de45b012cf1 100644 --- a/src/plugins/discover/public/components/discover_grid/discover_grid_footer.tsx +++ b/src/plugins/discover/public/components/discover_grid/discover_grid_footer.tsx @@ -18,7 +18,7 @@ export interface DiscoverGridFooterProps { isLoadingMore?: boolean; rowCount: number; sampleSize: number; - pageIndex?: number; + pageIndex?: number; // starts from 0 pageCount: number; totalHits?: number; onFetchMoreRecords?: () => void; From c73ef317d75d1717d1adc4dade785cc62cd58fd9 Mon Sep 17 00:00:00 2001 From: Julia Rechkunova Date: Mon, 7 Aug 2023 11:47:17 +0200 Subject: [PATCH 26/49] [Discover] Add functional tests --- .../apps/discover/group2/_data_grid_footer.ts | 106 ++++++++++++++++++ test/functional/apps/discover/group2/index.ts | 1 + 2 files changed, 107 insertions(+) create mode 100644 test/functional/apps/discover/group2/_data_grid_footer.ts diff --git a/test/functional/apps/discover/group2/_data_grid_footer.ts b/test/functional/apps/discover/group2/_data_grid_footer.ts new file mode 100644 index 0000000000000..010b8fc1d3287 --- /dev/null +++ b/test/functional/apps/discover/group2/_data_grid_footer.ts @@ -0,0 +1,106 @@ +/* + * 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 expect from '@kbn/expect'; +import { FtrProviderContext } from '../ftr_provider_context'; + +const FOOTER_SELECTOR = 'discoverTableFooter'; +const LOAD_MORE_SELECTOR = 'dscGridSampleSizeFetchMoreLink'; + +export default function ({ getService, getPageObjects }: FtrProviderContext) { + const esArchiver = getService('esArchiver'); + const kibanaServer = getService('kibanaServer'); + const PageObjects = getPageObjects(['common', 'discover', 'timePicker']); + const defaultSettings = { defaultIndex: 'logstash-*' }; + const testSubjects = getService('testSubjects'); + const retry = getService('retry'); + const security = getService('security'); + + describe('discover data grid footer', function describeIndexTests() { + before(async () => { + await security.testUser.setRoles(['kibana_admin', 'test_logstash_reader']); + await esArchiver.loadIfNeeded('test/functional/fixtures/es_archiver/logstash_functional'); + await kibanaServer.importExport.load('test/functional/fixtures/kbn_archiver/discover'); + }); + + after(async () => { + await kibanaServer.importExport.unload('test/functional/fixtures/kbn_archiver/discover'); + await esArchiver.unload('test/functional/fixtures/es_archiver/logstash_functional'); + await kibanaServer.savedObjects.cleanStandardList(); + await kibanaServer.uiSettings.replace({}); + }); + + beforeEach(async function () { + await PageObjects.timePicker.setDefaultAbsoluteRangeViaUiSettings(); + await kibanaServer.uiSettings.update(defaultSettings); + await PageObjects.common.navigateToApp('discover'); + await PageObjects.discover.waitUntilSearchingHasFinished(); + }); + + it('should show footer only for the last page and allow to load more', async () => { + // footer is not shown + await testSubjects.missingOrFail(FOOTER_SELECTOR); + + // go to next page + await testSubjects.click('pagination-button-next'); + // footer is not shown yet + await retry.try(async function () { + await testSubjects.missingOrFail(FOOTER_SELECTOR); + }); + + // go to the last page + await testSubjects.click('pagination-button-4'); + // footer is shown now + await retry.try(async function () { + await testSubjects.existOrFail(FOOTER_SELECTOR); + }); + expect(await testSubjects.getVisibleText(FOOTER_SELECTOR)).to.be( + 'Search results are limited to 500 documents.\nLoad more' + ); + + // there is no other pages to see + await testSubjects.missingOrFail('pagination-button-5'); + + // press "Load more" + await testSubjects.click(LOAD_MORE_SELECTOR); + await PageObjects.discover.waitUntilSearchingHasFinished(); + + // more pages appeared and the footer is gone + await retry.try(async function () { + await testSubjects.missingOrFail(FOOTER_SELECTOR); + }); + await testSubjects.exists('pagination-button-5'); + }); + + it('should disable "Load more" button when refresh interval is on', async () => { + // go to the last page + await testSubjects.click('pagination-button-4'); + await retry.try(async function () { + await testSubjects.existOrFail(FOOTER_SELECTOR); + }); + + expect(await testSubjects.isEnabled(LOAD_MORE_SELECTOR)).to.be(true); + + // enable the refresh interval + await PageObjects.timePicker.startAutoRefresh(10); + + // the button is disabled now + await retry.waitFor('disabled state', async function () { + return (await testSubjects.isEnabled(LOAD_MORE_SELECTOR)) === false; + }); + + // disable the refresh interval + await PageObjects.timePicker.pauseAutoRefresh(); + + // the button is enabled again + await retry.waitFor('enabled state', async function () { + return (await testSubjects.isEnabled(LOAD_MORE_SELECTOR)) === true; + }); + }); + }); +} diff --git a/test/functional/apps/discover/group2/index.ts b/test/functional/apps/discover/group2/index.ts index d6a0aeb9cd9ec..17562157f444e 100644 --- a/test/functional/apps/discover/group2/index.ts +++ b/test/functional/apps/discover/group2/index.ts @@ -30,6 +30,7 @@ export default function ({ getService, loadTestFile }: FtrProviderContext) { loadTestFile(require.resolve('./_data_grid_doc_table')); loadTestFile(require.resolve('./_data_grid_copy_to_clipboard')); loadTestFile(require.resolve('./_data_grid_pagination')); + loadTestFile(require.resolve('./_data_grid_footer')); loadTestFile(require.resolve('./_adhoc_data_views')); loadTestFile(require.resolve('./_sql_view')); loadTestFile(require.resolve('./_indexpattern_with_unmapped_fields')); From b2d4ba6f142fd5d10fed8d127d5b78afcc5fa5de Mon Sep 17 00:00:00 2001 From: Julia Rechkunova Date: Mon, 7 Aug 2023 12:34:29 +0200 Subject: [PATCH 27/49] [Discover] Add functional tests for date nanos --- .../discover_grid/discover_grid_footer.tsx | 10 +- .../apps/discover/group2/_data_grid_footer.ts | 244 +++++++++++++----- 2 files changed, 190 insertions(+), 64 deletions(-) diff --git a/src/plugins/discover/public/components/discover_grid/discover_grid_footer.tsx b/src/plugins/discover/public/components/discover_grid/discover_grid_footer.tsx index 16de45b012cf1..540c7102bd424 100644 --- a/src/plugins/discover/public/components/discover_grid/discover_grid_footer.tsx +++ b/src/plugins/discover/public/components/discover_grid/discover_grid_footer.tsx @@ -11,6 +11,7 @@ import { FormattedMessage } from '@kbn/i18n-react'; import { EuiButtonEmpty, EuiToolTip, useEuiTheme } from '@elastic/eui'; import { css } from '@emotion/react'; import { i18n } from '@kbn/i18n'; +import { ES_FIELD_TYPES, KBN_FIELD_TYPES } from '@kbn/field-types'; import { MAX_LOADED_GRID_ROWS } from '../../../common/constants'; import { useDiscoverServices } from '../../hooks/use_discover_services'; @@ -112,6 +113,11 @@ const DiscoverGridFooterContainer: React.FC = children, }) => { const { euiTheme } = useEuiTheme(); + const { fieldFormats } = useDiscoverServices(); + + const formattedRowCount = fieldFormats + .getDefaultInstance(KBN_FIELD_TYPES.NUMBER, [ES_FIELD_TYPES.INTEGER]) + .convert(rowCount); return (

= id="discover.gridSampleSize.lastPageDescription" defaultMessage="Search results are limited to {rowCount} documents." values={{ - rowCount, + rowCount: formattedRowCount, }} /> ) : ( @@ -144,7 +150,7 @@ const DiscoverGridFooterContainer: React.FC = id="discover.gridSampleSize.limitDescription" defaultMessage="Search results are limited to {sampleSize} documents. Add more search terms to narrow your search." values={{ - sampleSize: rowCount, + sampleSize: formattedRowCount, }} /> )} diff --git a/test/functional/apps/discover/group2/_data_grid_footer.ts b/test/functional/apps/discover/group2/_data_grid_footer.ts index 010b8fc1d3287..a1d2ff0294d23 100644 --- a/test/functional/apps/discover/group2/_data_grid_footer.ts +++ b/test/functional/apps/discover/group2/_data_grid_footer.ts @@ -15,91 +15,211 @@ const LOAD_MORE_SELECTOR = 'dscGridSampleSizeFetchMoreLink'; export default function ({ getService, getPageObjects }: FtrProviderContext) { const esArchiver = getService('esArchiver'); const kibanaServer = getService('kibanaServer'); - const PageObjects = getPageObjects(['common', 'discover', 'timePicker']); + const dataGrid = getService('dataGrid'); + const PageObjects = getPageObjects(['common', 'discover', 'timePicker', 'unifiedFieldList']); const defaultSettings = { defaultIndex: 'logstash-*' }; const testSubjects = getService('testSubjects'); const retry = getService('retry'); const security = getService('security'); - describe('discover data grid footer', function describeIndexTests() { - before(async () => { - await security.testUser.setRoles(['kibana_admin', 'test_logstash_reader']); - await esArchiver.loadIfNeeded('test/functional/fixtures/es_archiver/logstash_functional'); - await kibanaServer.importExport.load('test/functional/fixtures/kbn_archiver/discover'); - }); - - after(async () => { - await kibanaServer.importExport.unload('test/functional/fixtures/kbn_archiver/discover'); - await esArchiver.unload('test/functional/fixtures/es_archiver/logstash_functional'); - await kibanaServer.savedObjects.cleanStandardList(); - await kibanaServer.uiSettings.replace({}); - }); + describe('discover data grid footer', function () { + describe('time field with date type', function () { + before(async () => { + await security.testUser.setRoles(['kibana_admin', 'test_logstash_reader']); + await esArchiver.loadIfNeeded('test/functional/fixtures/es_archiver/logstash_functional'); + await kibanaServer.importExport.load('test/functional/fixtures/kbn_archiver/discover'); + }); - beforeEach(async function () { - await PageObjects.timePicker.setDefaultAbsoluteRangeViaUiSettings(); - await kibanaServer.uiSettings.update(defaultSettings); - await PageObjects.common.navigateToApp('discover'); - await PageObjects.discover.waitUntilSearchingHasFinished(); - }); + after(async () => { + await kibanaServer.importExport.unload('test/functional/fixtures/kbn_archiver/discover'); + await esArchiver.unload('test/functional/fixtures/es_archiver/logstash_functional'); + await kibanaServer.savedObjects.cleanStandardList(); + await kibanaServer.uiSettings.replace({}); + }); - it('should show footer only for the last page and allow to load more', async () => { - // footer is not shown - await testSubjects.missingOrFail(FOOTER_SELECTOR); + beforeEach(async function () { + await PageObjects.timePicker.setDefaultAbsoluteRangeViaUiSettings(); + await kibanaServer.uiSettings.update(defaultSettings); + await PageObjects.common.navigateToApp('discover'); + await PageObjects.discover.waitUntilSearchingHasFinished(); + }); - // go to next page - await testSubjects.click('pagination-button-next'); - // footer is not shown yet - await retry.try(async function () { + it('should show footer only for the last page and allow to load more', async () => { + // footer is not shown await testSubjects.missingOrFail(FOOTER_SELECTOR); - }); - // go to the last page - await testSubjects.click('pagination-button-4'); - // footer is shown now - await retry.try(async function () { - await testSubjects.existOrFail(FOOTER_SELECTOR); + // go to next page + await testSubjects.click('pagination-button-next'); + // footer is not shown yet + await retry.try(async function () { + await testSubjects.missingOrFail(FOOTER_SELECTOR); + }); + + // go to the last page + await testSubjects.click('pagination-button-4'); + // footer is shown now + await retry.try(async function () { + await testSubjects.existOrFail(FOOTER_SELECTOR); + }); + expect(await testSubjects.getVisibleText(FOOTER_SELECTOR)).to.be( + 'Search results are limited to 500 documents.\nLoad more' + ); + + // there is no other pages to see + await testSubjects.missingOrFail('pagination-button-5'); + + // press "Load more" + await testSubjects.click(LOAD_MORE_SELECTOR); + await PageObjects.discover.waitUntilSearchingHasFinished(); + + // more pages appeared and the footer is gone + await retry.try(async function () { + await testSubjects.missingOrFail(FOOTER_SELECTOR); + }); + + // go to the last page + await testSubjects.click('pagination-button-9'); + expect(await testSubjects.getVisibleText(FOOTER_SELECTOR)).to.be( + 'Search results are limited to 1,000 documents.\nLoad more' + ); + + // press "Load more" + await testSubjects.click(LOAD_MORE_SELECTOR); + await PageObjects.discover.waitUntilSearchingHasFinished(); + + // more pages appeared and the footer is gone + await retry.try(async function () { + await testSubjects.missingOrFail(FOOTER_SELECTOR); + }); + + // go to the last page + await testSubjects.click('pagination-button-14'); + expect(await testSubjects.getVisibleText(FOOTER_SELECTOR)).to.be( + 'Search results are limited to 1,500 documents.\nLoad more' + ); }); - expect(await testSubjects.getVisibleText(FOOTER_SELECTOR)).to.be( - 'Search results are limited to 500 documents.\nLoad more' - ); - // there is no other pages to see - await testSubjects.missingOrFail('pagination-button-5'); + it('should disable "Load more" button when refresh interval is on', async () => { + // go to the last page + await testSubjects.click('pagination-button-4'); + await retry.try(async function () { + await testSubjects.existOrFail(FOOTER_SELECTOR); + }); - // press "Load more" - await testSubjects.click(LOAD_MORE_SELECTOR); - await PageObjects.discover.waitUntilSearchingHasFinished(); + expect(await testSubjects.isEnabled(LOAD_MORE_SELECTOR)).to.be(true); - // more pages appeared and the footer is gone - await retry.try(async function () { - await testSubjects.missingOrFail(FOOTER_SELECTOR); + // enable the refresh interval + await PageObjects.timePicker.startAutoRefresh(10); + + // the button is disabled now + await retry.waitFor('disabled state', async function () { + return (await testSubjects.isEnabled(LOAD_MORE_SELECTOR)) === false; + }); + + // disable the refresh interval + await PageObjects.timePicker.pauseAutoRefresh(); + + // the button is enabled again + await retry.waitFor('enabled state', async function () { + return (await testSubjects.isEnabled(LOAD_MORE_SELECTOR)) === true; + }); }); - await testSubjects.exists('pagination-button-5'); }); - it('should disable "Load more" button when refresh interval is on', async () => { - // go to the last page - await testSubjects.click('pagination-button-4'); - await retry.try(async function () { - await testSubjects.existOrFail(FOOTER_SELECTOR); + describe('time field with date nano type', function () { + before(async () => { + await security.testUser.setRoles(['kibana_admin', 'kibana_date_nanos']); + await esArchiver.loadIfNeeded('test/functional/fixtures/es_archiver/date_nanos'); + await kibanaServer.importExport.load('test/functional/fixtures/kbn_archiver/date_nanos'); }); - expect(await testSubjects.isEnabled(LOAD_MORE_SELECTOR)).to.be(true); - - // enable the refresh interval - await PageObjects.timePicker.startAutoRefresh(10); + after(async () => { + await esArchiver.unload('test/functional/fixtures/es_archiver/date_nanos'); + await kibanaServer.importExport.unload('test/functional/fixtures/kbn_archiver/date_nanos'); + await kibanaServer.savedObjects.cleanStandardList(); + await kibanaServer.uiSettings.replace({}); + }); - // the button is disabled now - await retry.waitFor('disabled state', async function () { - return (await testSubjects.isEnabled(LOAD_MORE_SELECTOR)) === false; + beforeEach(async function () { + await PageObjects.common.setTime({ + from: 'Sep 10, 2015 @ 00:00:00.000', + to: 'Sep 30, 2019 @ 00:00:00.000', + }); + await kibanaServer.uiSettings.update({ + defaultIndex: 'date-nanos', + 'discover:sampleSize': 4, + 'discover:sampleRowsPerPage': 2, + }); + await PageObjects.common.navigateToApp('discover'); + await PageObjects.discover.waitUntilSearchingHasFinished(); }); - // disable the refresh interval - await PageObjects.timePicker.pauseAutoRefresh(); + it('should work for date nanos too', async () => { + await PageObjects.unifiedFieldList.clickFieldListItemAdd('_id'); + await PageObjects.discover.waitUntilSearchingHasFinished(); + + expect(await dataGrid.getRowsText()).to.eql([ + 'Sep 22, 2019 @ 23:50:13.253123345AU_x3-TaGFA8no6QjiSJ', + 'Sep 18, 2019 @ 06:50:13.000000104AU_x3-TaGFA8no6Qjis104Z', + ]); + + // footer is not shown + await testSubjects.missingOrFail(FOOTER_SELECTOR); - // the button is enabled again - await retry.waitFor('enabled state', async function () { - return (await testSubjects.isEnabled(LOAD_MORE_SELECTOR)) === true; + // go to the last page + await testSubjects.click('pagination-button-1'); + await retry.try(async function () { + await testSubjects.existOrFail(FOOTER_SELECTOR); + }); + expect(await testSubjects.getVisibleText(FOOTER_SELECTOR)).to.be( + 'Search results are limited to 4 documents.\nLoad more' + ); + expect(await dataGrid.getRowsText()).to.eql([ + 'Sep 18, 2019 @ 06:50:13.000000103BU_x3-TaGFA8no6Qjis103Z', + 'Sep 18, 2019 @ 06:50:13.000000102AU_x3-TaGFA8no6Qji102Z', + ]); + + // there is no other pages to see yet + await testSubjects.missingOrFail('pagination-button-2'); + + // press "Load more" + await testSubjects.click(LOAD_MORE_SELECTOR); + await PageObjects.discover.waitUntilSearchingHasFinished(); + + // more pages appeared and the footer is gone + await retry.try(async function () { + await testSubjects.missingOrFail(FOOTER_SELECTOR); + }); + + // go to the last page + await testSubjects.click('pagination-button-3'); + expect(await testSubjects.getVisibleText(FOOTER_SELECTOR)).to.be( + 'Search results are limited to 8 documents.\nLoad more' + ); + + expect(await dataGrid.getRowsText()).to.eql([ + 'Sep 18, 2019 @ 06:50:13.000000000CU_x3-TaGFA8no6QjiSX000Z', + 'Sep 18, 2019 @ 06:50:12.999999999AU_x3-TaGFA8no6Qj999Z', + ]); + + // press "Load more" + await testSubjects.click(LOAD_MORE_SELECTOR); + await PageObjects.discover.waitUntilSearchingHasFinished(); + + // more pages appeared and the footer is gone + await retry.try(async function () { + await testSubjects.missingOrFail(FOOTER_SELECTOR); + }); + + // go to the last page + await testSubjects.click('pagination-button-4'); + await retry.try(async function () { + await testSubjects.missingOrFail(FOOTER_SELECTOR); + }); + + expect(await dataGrid.getRowsText()).to.eql([ + 'Sep 19, 2015 @ 06:50:13.000100001AU_x3-TaGFA8no000100001Z', + ]); }); }); }); From ef345102f8dc77f4d8d68929a6a5dd6e6d88d12c Mon Sep 17 00:00:00 2001 From: kibanamachine <42973632+kibanamachine@users.noreply.github.com> Date: Mon, 7 Aug 2023 10:39:43 +0000 Subject: [PATCH 28/49] [CI] Auto-commit changed files from 'node scripts/lint_ts_projects --fix' --- src/plugins/discover/tsconfig.json | 3 ++- 1 file changed, 2 insertions(+), 1 deletion(-) diff --git a/src/plugins/discover/tsconfig.json b/src/plugins/discover/tsconfig.json index ab6d47d6a86d1..443867566ff83 100644 --- a/src/plugins/discover/tsconfig.json +++ b/src/plugins/discover/tsconfig.json @@ -64,7 +64,8 @@ "@kbn/shared-ux-utility", "@kbn/core-application-browser", "@kbn/core-saved-objects-server", - "@kbn/discover-utils" + "@kbn/discover-utils", + "@kbn/field-types" ], "exclude": [ "target/**/*" From 2b97c3d322c723fe38354c13387e03fe8c9e2d64 Mon Sep 17 00:00:00 2001 From: Julia Rechkunova Date: Mon, 7 Aug 2023 20:27:47 +0200 Subject: [PATCH 29/49] [Discover] Extract the code --- .../common/utils/sorting/get_es_query_sort.ts | 11 +++ .../components/layout/discover_documents.tsx | 24 ++----- .../layout/use_fetch_more_records.ts | 70 +++++++++++++++++++ .../services/discover_data_state_container.ts | 1 + 4 files changed, 87 insertions(+), 19 deletions(-) create mode 100644 src/plugins/discover/public/application/main/components/layout/use_fetch_more_records.ts diff --git a/src/plugins/discover/common/utils/sorting/get_es_query_sort.ts b/src/plugins/discover/common/utils/sorting/get_es_query_sort.ts index b1587781a4284..8f6ee1d8c788e 100644 --- a/src/plugins/discover/common/utils/sorting/get_es_query_sort.ts +++ b/src/plugins/discover/common/utils/sorting/get_es_query_sort.ts @@ -36,6 +36,12 @@ export function getEsQuerySort({ ]; } +/** + * Prepares "sort" structure for a time field for next ES request + * @param sortDir + * @param timeFieldName + * @param isTimeNanosBased + */ export function getESQuerySortForTimeField({ sortDir, timeFieldName, @@ -58,6 +64,11 @@ export function getESQuerySortForTimeField({ }; } +/** + * Prepares "sort" structure for a tie breaker for next ES request + * @param sortDir + * @param tieBreakerFieldName + */ export function getESQuerySortForTieBreaker({ sortDir, tieBreakerFieldName, diff --git a/src/plugins/discover/public/application/main/components/layout/discover_documents.tsx b/src/plugins/discover/public/application/main/components/layout/discover_documents.tsx index 6d53c096a0453..8171c095f5af1 100644 --- a/src/plugins/discover/public/application/main/components/layout/discover_documents.tsx +++ b/src/plugins/discover/public/application/main/components/layout/discover_documents.tsx @@ -45,6 +45,7 @@ import { getRawRecordType } from '../../utils/get_raw_record_type'; import { DiscoverGridFlyout } from '../../../../components/discover_grid/discover_grid_flyout'; import { DocViewer } from '../../../../services/doc_views/components/doc_viewer'; import { useSavedSearchInitial } from '../../services/discover_state_provider'; +import { useFetchMoreRecords } from './use_fetch_more_records'; const containerStyles = css` position: relative; @@ -85,7 +86,6 @@ function DiscoverDocumentsComponent({ }) { const services = useDiscoverServices(); const documents$ = stateContainer.dataState.data$.documents$; - const totalHits$ = stateContainer.dataState.data$.totalHits$; const savedSearch = useSavedSearchInitial(); const { dataViews, capabilities, uiSettings, uiActions } = services; const [query, sort, rowHeight, rowsPerPage, grid, columns, index] = useAppStateSelector( @@ -119,7 +119,6 @@ function DiscoverDocumentsComponent({ const isDataLoading = documentState.fetchStatus === FetchStatus.LOADING || documentState.fetchStatus === FetchStatus.PARTIAL; - const isMoreDataLoading = documentState.fetchStatus === FetchStatus.LOADING_MORE; const isTextBasedQuery = useMemo(() => getRawRecordType(query) === RecordRawType.PLAIN, [query]); // This is needed to prevent EuiDataGrid pushing onSort because the data view has been switched. // It's just necessary for non-text-based query lang requests since they don't have a partial result state, that's @@ -135,23 +134,10 @@ function DiscoverDocumentsComponent({ isTextBasedQuery || !documentState.result || documentState.result.length === 0; const rows = useMemo(() => documentState.result || [], [documentState.result]); - const totalHitsState = useDataState(totalHits$); - const totalHits = totalHitsState.result || 0; - const canFetchMoreRecords = - !isTextBasedQuery && - rows.length > 0 && - totalHits > rows.length && - Boolean(rows[rows.length - 1].raw.sort?.length); - - const onFetchMoreRecords = useMemo( - () => - canFetchMoreRecords - ? () => { - stateContainer.dataState.fetchMore(); - } - : undefined, - [canFetchMoreRecords, stateContainer.dataState] - ); + const { isMoreDataLoading, totalHits, onFetchMoreRecords } = useFetchMoreRecords({ + isTextBasedQuery, + stateContainer, + }); const { columns: currentColumns, diff --git a/src/plugins/discover/public/application/main/components/layout/use_fetch_more_records.ts b/src/plugins/discover/public/application/main/components/layout/use_fetch_more_records.ts new file mode 100644 index 0000000000000..e9b6c1c017853 --- /dev/null +++ b/src/plugins/discover/public/application/main/components/layout/use_fetch_more_records.ts @@ -0,0 +1,70 @@ +/* + * 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 { useMemo } from 'react'; +import { FetchStatus } from '../../../types'; +import { useDataState } from '../../hooks/use_data_state'; +import type { DiscoverStateContainer } from '../../services/discover_state'; + +/** + * Params for the hook + */ +export interface UseFetchMoreRecordsParams { + isTextBasedQuery: boolean; + stateContainer: DiscoverStateContainer; +} + +/** + * Return type for the hook + */ +export interface UseFetchMoreRecordsResult { + isMoreDataLoading: boolean; + totalHits: number; + onFetchMoreRecords: (() => void) | undefined; +} + +/** + * Checks if more records can be loaded and returns a handler for it + * @param isTextBasedQuery + * @param stateContainer + */ +export const useFetchMoreRecords = ({ + isTextBasedQuery, + stateContainer, +}: UseFetchMoreRecordsParams): UseFetchMoreRecordsResult => { + const documents$ = stateContainer.dataState.data$.documents$; + const totalHits$ = stateContainer.dataState.data$.totalHits$; + const documentState = useDataState(documents$); + const totalHitsState = useDataState(totalHits$); + + const rows = documentState.result || []; + const isMoreDataLoading = documentState.fetchStatus === FetchStatus.LOADING_MORE; + + const totalHits = totalHitsState.result || 0; + const canFetchMoreRecords = + !isTextBasedQuery && + rows.length > 0 && + totalHits > rows.length && + Boolean(rows[rows.length - 1].raw.sort?.length); + + const onFetchMoreRecords = useMemo( + () => + canFetchMoreRecords + ? () => { + stateContainer.dataState.fetchMore(); + } + : undefined, + [canFetchMoreRecords, stateContainer.dataState] + ); + + return { + isMoreDataLoading, + totalHits, + onFetchMoreRecords, + }; +}; diff --git a/src/plugins/discover/public/application/main/services/discover_data_state_container.ts b/src/plugins/discover/public/application/main/services/discover_data_state_container.ts index c04d231241ade..413446b281551 100644 --- a/src/plugins/discover/public/application/main/services/discover_data_state_container.ts +++ b/src/plugins/discover/public/application/main/services/discover_data_state_container.ts @@ -264,6 +264,7 @@ export function getDataStateContainer({ return () => { abortController?.abort(); + abortControllerFetchMore?.abort(); subscription.unsubscribe(); }; } From a57eafa030f56deafba0cd1519111ef31f53fee8 Mon Sep 17 00:00:00 2001 From: Julia Rechkunova Date: Thu, 10 Aug 2023 16:18:04 +0200 Subject: [PATCH 30/49] [Discover] Add a test for fetchMore --- .../layout/use_discover_histogram.test.tsx | 32 +++++++++++++++++++ 1 file changed, 32 insertions(+) diff --git a/src/plugins/discover/public/application/main/components/layout/use_discover_histogram.test.tsx b/src/plugins/discover/public/application/main/components/layout/use_discover_histogram.test.tsx index 2c082237fd4a0..e4a2a674c0f77 100644 --- a/src/plugins/discover/public/application/main/components/layout/use_discover_histogram.test.tsx +++ b/src/plugins/discover/public/application/main/components/layout/use_discover_histogram.test.tsx @@ -414,5 +414,37 @@ describe('useDiscoverHistogram', () => { }); expect(api.refetch).not.toHaveBeenCalled(); }); + + it('should skip the next refetch when fetching more', async () => { + const savedSearchFetch$ = new Subject<{ + options: { + reset: boolean; + fetchMore: boolean; + }; + searchSessionId: string; + }>(); + const stateContainer = getStateContainer(); + stateContainer.dataState.fetch$ = savedSearchFetch$; + const { hook } = await renderUseDiscoverHistogram({ stateContainer }); + const api = createMockUnifiedHistogramApi(); + act(() => { + hook.result.current.ref(api); + }); + act(() => { + savedSearchFetch$.next({ + options: { reset: false, fetchMore: true }, + searchSessionId: '1234', + }); + }); + expect(api.refetch).not.toHaveBeenCalled(); + + act(() => { + savedSearchFetch$.next({ + options: { reset: false, fetchMore: false }, + searchSessionId: '1234', + }); + }); + expect(api.refetch).toHaveBeenCalled(); + }); }); }); From 5d10203af084e0b4542704bef6124c4d6acb6796 Mon Sep 17 00:00:00 2001 From: Julia Rechkunova Date: Thu, 10 Aug 2023 16:51:02 +0200 Subject: [PATCH 31/49] [Discover] Changes to abort logic --- .../main/services/discover_data_state_container.ts | 6 +++--- 1 file changed, 3 insertions(+), 3 deletions(-) diff --git a/src/plugins/discover/public/application/main/services/discover_data_state_container.ts b/src/plugins/discover/public/application/main/services/discover_data_state_container.ts index feb1a6704b480..5faf0e243c252 100644 --- a/src/plugins/discover/public/application/main/services/discover_data_state_container.ts +++ b/src/plugins/discover/public/application/main/services/discover_data_state_container.ts @@ -228,10 +228,11 @@ export function getDataStateContainer({ useNewFieldsApi: !uiSettings.get(SEARCH_FIELDS_FROM_SOURCE), }; + abortController?.abort(); + abortControllerFetchMore?.abort(); + if (options.fetchMore) { - abortControllerFetchMore?.abort(); abortControllerFetchMore = new AbortController(); - await fetchMoreDocuments(dataSubjects, { abortController: abortControllerFetchMore, ...commonFetchDeps, @@ -239,7 +240,6 @@ export function getDataStateContainer({ return; } - abortController?.abort(); abortController = new AbortController(); const prevAutoRefreshDone = autoRefreshDone; From 358c755c570970814bf0344faf5ceaaa3d7acec4 Mon Sep 17 00:00:00 2001 From: Julia Rechkunova Date: Fri, 11 Aug 2023 12:18:09 +0200 Subject: [PATCH 32/49] [Discover] Add `_doc` as a tie breaker --- packages/kbn-generate-csv/src/generate_csv.ts | 3 ++ .../common/utils/sorting/get_es_query_sort.ts | 5 +++ .../sorting/get_sort_for_search_source.ts | 33 +++++++++++-------- 3 files changed, 28 insertions(+), 13 deletions(-) diff --git a/packages/kbn-generate-csv/src/generate_csv.ts b/packages/kbn-generate-csv/src/generate_csv.ts index 3e4d324dcfced..ef973d9ef50d2 100644 --- a/packages/kbn-generate-csv/src/generate_csv.ts +++ b/packages/kbn-generate-csv/src/generate_csv.ts @@ -356,6 +356,9 @@ export class CsvGenerator { // set the latest pit, which could be different from the last request searchSource.setField('pit', { id: pitId, keep_alive: settings.scroll.duration }); + // TODO: should we skip `_doc` (the tie breaker from Discover) from searchSource `sort` field when generating CSV? + // According to docs, the default `_shard_doc` is a better option when PIT is used. + const results = await this.doSearch(searchSource, settings, searchAfter); const { hits } = results; diff --git a/src/plugins/discover/common/utils/sorting/get_es_query_sort.ts b/src/plugins/discover/common/utils/sorting/get_es_query_sort.ts index 8f6ee1d8c788e..5fb4fa3d01a0b 100644 --- a/src/plugins/discover/common/utils/sorting/get_es_query_sort.ts +++ b/src/plugins/discover/common/utils/sorting/get_es_query_sort.ts @@ -79,6 +79,11 @@ export function getESQuerySortForTieBreaker({ return { [tieBreakerFieldName]: sortDir }; } +/** + * The default tie breaker for Discover + */ +export const DEFAULT_TIE_BREAKER_NAME = '_doc'; + /** * The list of field names that are allowed for sorting, but not included in * data view fields. diff --git a/src/plugins/discover/common/utils/sorting/get_sort_for_search_source.ts b/src/plugins/discover/common/utils/sorting/get_sort_for_search_source.ts index f6cea7c89311c..f1e507a40446f 100644 --- a/src/plugins/discover/common/utils/sorting/get_sort_for_search_source.ts +++ b/src/plugins/discover/common/utils/sorting/get_sort_for_search_source.ts @@ -12,7 +12,11 @@ import type { SortOrder } from '@kbn/saved-search-plugin/public'; import type { IUiSettingsClient } from '@kbn/core-ui-settings-browser'; import { SORT_DEFAULT_ORDER_SETTING } from '@kbn/discover-utils'; import { getSort } from './get_sort'; -import { getESQuerySortForTimeField } from './get_es_query_sort'; +import { + getESQuerySortForTimeField, + getESQuerySortForTieBreaker, + DEFAULT_TIE_BREAKER_NAME, +} from './get_es_query_sort'; /** * Prepares sort for search source, that's sending the request to ES @@ -55,19 +59,22 @@ export function getSortForSearchSource({ return sortPair as EsQuerySortValue; }); - // Do we need to have a tie breaker like this? We have it for Surrounding Documents page but not here yet. - // Introducing it here might become an unexpected change for users of Discover and also CSV reports - // as it will change the ordering of documents. + // NB: Changing the tie breaker here can affect CSV reports + // as it will change the ordering of returned documents. - // if (dataView.isTimeBased() && sortPairs.length) { - // const firstSortPair = sortPairs[0]; - // const firstPairSortDir = Array.isArray(firstSortPair) - // ? firstSortPair[1] - // : Object.values(firstSortPair)[0]; - // sortForSearchSource.push( - // getESQuerySortForTieBreaker(getTieBreakerFieldName(dataView, uiSettings), firstPairSortDir) - // ); - // } + if (timeFieldName && sortPairs.length) { + const firstSortPair = sortPairs[0]; + const firstPairSortDir = Array.isArray(firstSortPair) + ? firstSortPair[1] + : Object.values(firstSortPair)[0]; + + sortForSearchSource.push( + getESQuerySortForTieBreaker({ + sortDir: firstPairSortDir, + tieBreakerFieldName: DEFAULT_TIE_BREAKER_NAME, + }) + ); + } return sortForSearchSource; } From 5413e5e84905159e9527f5951b39b4471e5d2156 Mon Sep 17 00:00:00 2001 From: Julia Rechkunova Date: Fri, 11 Aug 2023 13:21:52 +0200 Subject: [PATCH 33/49] [Discover] Update tests --- .../utils/sorting/get_sort_for_search_source.test.ts | 7 ++++--- .../common/utils/sorting/get_sort_for_search_source.ts | 2 +- 2 files changed, 5 insertions(+), 4 deletions(-) diff --git a/src/plugins/discover/common/utils/sorting/get_sort_for_search_source.test.ts b/src/plugins/discover/common/utils/sorting/get_sort_for_search_source.test.ts index c94d49c60ba3e..be18119b88e6f 100644 --- a/src/plugins/discover/common/utils/sorting/get_sort_for_search_source.test.ts +++ b/src/plugins/discover/common/utils/sorting/get_sort_for_search_source.test.ts @@ -28,6 +28,7 @@ describe('getSortForSearchSource function', function () { const cols = [['bytes', 'desc']] as SortOrder[]; expect(getSortForSearchSource({ sort: cols, dataView: stubDataView, uiSettings })).toEqual([ { bytes: 'desc' }, + { _doc: 'desc' }, ]); expect( getSortForSearchSource({ @@ -35,18 +36,18 @@ describe('getSortForSearchSource function', function () { dataView: stubDataView, uiSettings: uiSettingWithAscSorting, }) - ).toEqual([{ bytes: 'desc' }]); + ).toEqual([{ bytes: 'desc' }, { _doc: 'desc' }]); expect( getSortForSearchSource({ sort: cols, dataView: stubDataViewWithoutTimeField, uiSettings }) - ).toEqual([{ bytes: 'desc' }]); + ).toEqual([{ bytes: 'desc' }, { _doc: 'desc' }]); expect( getSortForSearchSource({ sort: cols, dataView: stubDataViewWithoutTimeField, uiSettings: uiSettingWithAscSorting, }) - ).toEqual([{ bytes: 'desc' }]); + ).toEqual([{ bytes: 'desc' }, { _doc: 'desc' }]); }); test('should return an object to use for searchSource when no columns are given', function () { diff --git a/src/plugins/discover/common/utils/sorting/get_sort_for_search_source.ts b/src/plugins/discover/common/utils/sorting/get_sort_for_search_source.ts index f1e507a40446f..3629367ec04a3 100644 --- a/src/plugins/discover/common/utils/sorting/get_sort_for_search_source.ts +++ b/src/plugins/discover/common/utils/sorting/get_sort_for_search_source.ts @@ -62,7 +62,7 @@ export function getSortForSearchSource({ // NB: Changing the tie breaker here can affect CSV reports // as it will change the ordering of returned documents. - if (timeFieldName && sortPairs.length) { + if (sortPairs.length) { const firstSortPair = sortPairs[0]; const firstPairSortDir = Array.isArray(firstSortPair) ? firstSortPair[1] From a2cb79f246e3f91082e254d6ff8dade908ffe14c Mon Sep 17 00:00:00 2001 From: Julia Rechkunova Date: Fri, 11 Aug 2023 13:55:40 +0200 Subject: [PATCH 34/49] [Discover] Fix for CSV --- .../sorting/get_sort_for_search_source.test.ts | 15 +++++++++++++-- .../utils/sorting/get_sort_for_search_source.ts | 8 ++++---- .../discover/public/utils/get_sharing_data.ts | 7 ++++++- .../server/locator/searchsource_from_locator.ts | 1 + 4 files changed, 24 insertions(+), 7 deletions(-) diff --git a/src/plugins/discover/common/utils/sorting/get_sort_for_search_source.test.ts b/src/plugins/discover/common/utils/sorting/get_sort_for_search_source.test.ts index be18119b88e6f..7889db50de1fc 100644 --- a/src/plugins/discover/common/utils/sorting/get_sort_for_search_source.test.ts +++ b/src/plugins/discover/common/utils/sorting/get_sort_for_search_source.test.ts @@ -30,6 +30,7 @@ describe('getSortForSearchSource function', function () { { bytes: 'desc' }, { _doc: 'desc' }, ]); + expect( getSortForSearchSource({ sort: cols, @@ -38,16 +39,26 @@ describe('getSortForSearchSource function', function () { }) ).toEqual([{ bytes: 'desc' }, { _doc: 'desc' }]); + expect( + getSortForSearchSource({ + sort: cols, + dataView: stubDataView, + uiSettings: uiSettingWithAscSorting, + skipTieBreaker: true, + }) + ).toEqual([{ bytes: 'desc' }]); + expect( getSortForSearchSource({ sort: cols, dataView: stubDataViewWithoutTimeField, uiSettings }) - ).toEqual([{ bytes: 'desc' }, { _doc: 'desc' }]); + ).toEqual([{ bytes: 'desc' }]); + expect( getSortForSearchSource({ sort: cols, dataView: stubDataViewWithoutTimeField, uiSettings: uiSettingWithAscSorting, }) - ).toEqual([{ bytes: 'desc' }, { _doc: 'desc' }]); + ).toEqual([{ bytes: 'desc' }]); }); test('should return an object to use for searchSource when no columns are given', function () { diff --git a/src/plugins/discover/common/utils/sorting/get_sort_for_search_source.ts b/src/plugins/discover/common/utils/sorting/get_sort_for_search_source.ts index 3629367ec04a3..457bed88c140b 100644 --- a/src/plugins/discover/common/utils/sorting/get_sort_for_search_source.ts +++ b/src/plugins/discover/common/utils/sorting/get_sort_for_search_source.ts @@ -29,10 +29,12 @@ export function getSortForSearchSource({ sort, dataView, uiSettings, + skipTieBreaker = false, }: { sort: SortOrder[] | undefined; dataView: DataView | undefined; uiSettings: Pick; + skipTieBreaker?: boolean; }): EsQuerySortValue[] { const defaultDirection = uiSettings.get(SORT_DEFAULT_ORDER_SETTING) || 'desc'; @@ -59,10 +61,8 @@ export function getSortForSearchSource({ return sortPair as EsQuerySortValue; }); - // NB: Changing the tie breaker here can affect CSV reports - // as it will change the ordering of returned documents. - - if (sortPairs.length) { + // This tie breaker is skipped for CSV reports as it uses PIT + if (!skipTieBreaker && timeFieldName && sortPairs.length) { const firstSortPair = sortPairs[0]; const firstPairSortDir = Array.isArray(firstSortPair) ? firstSortPair[1] diff --git a/src/plugins/discover/public/utils/get_sharing_data.ts b/src/plugins/discover/public/utils/get_sharing_data.ts index 4b34cff05db67..edb1abc8a4b9e 100644 --- a/src/plugins/discover/public/utils/get_sharing_data.ts +++ b/src/plugins/discover/public/utils/get_sharing_data.ts @@ -38,7 +38,12 @@ export async function getSharingData( searchSource.setField( 'sort', - getSortForSearchSource({ sort: state.sort as SortOrder[], dataView: index, uiSettings }) + getSortForSearchSource({ + sort: state.sort as SortOrder[], + dataView: index, + uiSettings, + skipTieBreaker: true, + }) ); searchSource.removeField('filter'); diff --git a/src/plugins/discover/server/locator/searchsource_from_locator.ts b/src/plugins/discover/server/locator/searchsource_from_locator.ts index 9d9f1e924fe01..a585de0752031 100644 --- a/src/plugins/discover/server/locator/searchsource_from_locator.ts +++ b/src/plugins/discover/server/locator/searchsource_from_locator.ts @@ -158,6 +158,7 @@ export function searchSourceFromLocatorFactory(services: LocatorServicesDeps) { sort: savedSearch.sort as Array<[string, string]>, dataView: index, uiSettings: uiSettingsSyncReplacement, + skipTieBreaker: true, }); searchSource.setField('sort', sort); } From c8728d491d73a4a118fc7d6e1e9ee73edc362873 Mon Sep 17 00:00:00 2001 From: Julia Rechkunova Date: Fri, 11 Aug 2023 14:04:56 +0200 Subject: [PATCH 35/49] [Discover] Invert the parameter --- .../get_sort_for_search_source.test.ts | 21 +++++++++++++------ .../sorting/get_sort_for_search_source.ts | 6 +++--- .../main/utils/update_search_source.ts | 2 +- .../embeddable/utils/update_search_source.ts | 5 ++++- .../discover/public/utils/get_sharing_data.ts | 1 - .../locator/searchsource_from_locator.ts | 1 - 6 files changed, 23 insertions(+), 13 deletions(-) diff --git a/src/plugins/discover/common/utils/sorting/get_sort_for_search_source.test.ts b/src/plugins/discover/common/utils/sorting/get_sort_for_search_source.test.ts index 7889db50de1fc..6e5fbab6bbc39 100644 --- a/src/plugins/discover/common/utils/sorting/get_sort_for_search_source.test.ts +++ b/src/plugins/discover/common/utils/sorting/get_sort_for_search_source.test.ts @@ -26,16 +26,21 @@ describe('getSortForSearchSource function', function () { test('should return an object to use for searchSource when columns are given', function () { const cols = [['bytes', 'desc']] as SortOrder[]; - expect(getSortForSearchSource({ sort: cols, dataView: stubDataView, uiSettings })).toEqual([ - { bytes: 'desc' }, - { _doc: 'desc' }, - ]); + expect( + getSortForSearchSource({ + sort: cols, + dataView: stubDataView, + uiSettings, + includeTieBreaker: true, + }) + ).toEqual([{ bytes: 'desc' }, { _doc: 'desc' }]); expect( getSortForSearchSource({ sort: cols, dataView: stubDataView, uiSettings: uiSettingWithAscSorting, + includeTieBreaker: true, }) ).toEqual([{ bytes: 'desc' }, { _doc: 'desc' }]); @@ -44,12 +49,16 @@ describe('getSortForSearchSource function', function () { sort: cols, dataView: stubDataView, uiSettings: uiSettingWithAscSorting, - skipTieBreaker: true, }) ).toEqual([{ bytes: 'desc' }]); expect( - getSortForSearchSource({ sort: cols, dataView: stubDataViewWithoutTimeField, uiSettings }) + getSortForSearchSource({ + sort: cols, + dataView: stubDataViewWithoutTimeField, + uiSettings, + includeTieBreaker: true, + }) ).toEqual([{ bytes: 'desc' }]); expect( diff --git a/src/plugins/discover/common/utils/sorting/get_sort_for_search_source.ts b/src/plugins/discover/common/utils/sorting/get_sort_for_search_source.ts index 457bed88c140b..965bd7f8fec73 100644 --- a/src/plugins/discover/common/utils/sorting/get_sort_for_search_source.ts +++ b/src/plugins/discover/common/utils/sorting/get_sort_for_search_source.ts @@ -29,12 +29,12 @@ export function getSortForSearchSource({ sort, dataView, uiSettings, - skipTieBreaker = false, + includeTieBreaker = false, }: { sort: SortOrder[] | undefined; dataView: DataView | undefined; uiSettings: Pick; - skipTieBreaker?: boolean; + includeTieBreaker?: boolean; }): EsQuerySortValue[] { const defaultDirection = uiSettings.get(SORT_DEFAULT_ORDER_SETTING) || 'desc'; @@ -62,7 +62,7 @@ export function getSortForSearchSource({ }); // This tie breaker is skipped for CSV reports as it uses PIT - if (!skipTieBreaker && timeFieldName && sortPairs.length) { + if (includeTieBreaker && timeFieldName && sortPairs.length) { const firstSortPair = sortPairs[0]; const firstPairSortDir = Array.isArray(firstSortPair) ? firstSortPair[1] diff --git a/src/plugins/discover/public/application/main/utils/update_search_source.ts b/src/plugins/discover/public/application/main/utils/update_search_source.ts index d3798ef77b7bf..c5556ab73ab5e 100644 --- a/src/plugins/discover/public/application/main/utils/update_search_source.ts +++ b/src/plugins/discover/public/application/main/utils/update_search_source.ts @@ -34,7 +34,7 @@ export function updateVolatileSearchSource( const { uiSettings, data } = services; const useNewFieldsApi = !uiSettings.get(SEARCH_FIELDS_FROM_SOURCE); - const usedSort = getSortForSearchSource({ sort, dataView, uiSettings }); + const usedSort = getSortForSearchSource({ sort, dataView, uiSettings, includeTieBreaker: true }); searchSource.setField('sort', usedSort); searchSource.setField('trackTotalHits', true); diff --git a/src/plugins/discover/public/embeddable/utils/update_search_source.ts b/src/plugins/discover/public/embeddable/utils/update_search_source.ts index 36cb203d1ffa2..d3885ee210f9c 100644 --- a/src/plugins/discover/public/embeddable/utils/update_search_source.ts +++ b/src/plugins/discover/public/embeddable/utils/update_search_source.ts @@ -23,7 +23,10 @@ export const updateSearchSource = ( ) => { const { sampleSize } = defaults; searchSource.setField('size', sampleSize); - searchSource.setField('sort', getSortForSearchSource({ sort, dataView, uiSettings })); + searchSource.setField( + 'sort', + getSortForSearchSource({ sort, dataView, uiSettings, includeTieBreaker: true }) + ); if (useNewFieldsApi) { searchSource.removeField('fieldsFromSource'); const fields: Record = { field: '*', include_unmapped: 'true' }; diff --git a/src/plugins/discover/public/utils/get_sharing_data.ts b/src/plugins/discover/public/utils/get_sharing_data.ts index edb1abc8a4b9e..89a161416189d 100644 --- a/src/plugins/discover/public/utils/get_sharing_data.ts +++ b/src/plugins/discover/public/utils/get_sharing_data.ts @@ -42,7 +42,6 @@ export async function getSharingData( sort: state.sort as SortOrder[], dataView: index, uiSettings, - skipTieBreaker: true, }) ); diff --git a/src/plugins/discover/server/locator/searchsource_from_locator.ts b/src/plugins/discover/server/locator/searchsource_from_locator.ts index a585de0752031..9d9f1e924fe01 100644 --- a/src/plugins/discover/server/locator/searchsource_from_locator.ts +++ b/src/plugins/discover/server/locator/searchsource_from_locator.ts @@ -158,7 +158,6 @@ export function searchSourceFromLocatorFactory(services: LocatorServicesDeps) { sort: savedSearch.sort as Array<[string, string]>, dataView: index, uiSettings: uiSettingsSyncReplacement, - skipTieBreaker: true, }); searchSource.setField('sort', sort); } From 8d6308809f3e2c7b8026a885dc083426874a0ab0 Mon Sep 17 00:00:00 2001 From: Julia Rechkunova Date: Fri, 11 Aug 2023 14:07:59 +0200 Subject: [PATCH 36/49] [Discover] Remove the comment --- packages/kbn-generate-csv/src/generate_csv.ts | 3 --- 1 file changed, 3 deletions(-) diff --git a/packages/kbn-generate-csv/src/generate_csv.ts b/packages/kbn-generate-csv/src/generate_csv.ts index ef973d9ef50d2..3e4d324dcfced 100644 --- a/packages/kbn-generate-csv/src/generate_csv.ts +++ b/packages/kbn-generate-csv/src/generate_csv.ts @@ -356,9 +356,6 @@ export class CsvGenerator { // set the latest pit, which could be different from the last request searchSource.setField('pit', { id: pitId, keep_alive: settings.scroll.duration }); - // TODO: should we skip `_doc` (the tie breaker from Discover) from searchSource `sort` field when generating CSV? - // According to docs, the default `_shard_doc` is a better option when PIT is used. - const results = await this.doSearch(searchSource, settings, searchAfter); const { hits } = results; From 70e40617282b40935a2a9b4732f3ba35796f6855 Mon Sep 17 00:00:00 2001 From: Julia Rechkunova Date: Mon, 14 Aug 2023 10:57:38 +0200 Subject: [PATCH 37/49] [Discover] Send a performance metric for fetchMore --- .../main/services/discover_data_state_container.ts | 7 +++++++ 1 file changed, 7 insertions(+) diff --git a/src/plugins/discover/public/application/main/services/discover_data_state_container.ts b/src/plugins/discover/public/application/main/services/discover_data_state_container.ts index 5faf0e243c252..e68218cb0865c 100644 --- a/src/plugins/discover/public/application/main/services/discover_data_state_container.ts +++ b/src/plugins/discover/public/application/main/services/discover_data_state_container.ts @@ -233,10 +233,17 @@ export function getDataStateContainer({ if (options.fetchMore) { abortControllerFetchMore = new AbortController(); + + const fetchMoreStartTime = window.performance.now(); await fetchMoreDocuments(dataSubjects, { abortController: abortControllerFetchMore, ...commonFetchDeps, }); + const fetchMoreDuration = window.performance.now() - fetchMoreStartTime; + reportPerformanceMetricEvent(services.analytics, { + eventName: 'discoverFetchMore', + duration: fetchMoreDuration, + }); return; } From b31b568dfd9a5d0dba0eee8c226bd7ab62fd73b2 Mon Sep 17 00:00:00 2001 From: Julia Rechkunova Date: Mon, 14 Aug 2023 11:28:25 +0200 Subject: [PATCH 38/49] [Discover] Update error handling --- .../application/main/utils/fetch_all.ts | 44 ++++++------------- 1 file changed, 13 insertions(+), 31 deletions(-) diff --git a/src/plugins/discover/public/application/main/utils/fetch_all.ts b/src/plugins/discover/public/application/main/utils/fetch_all.ts index 585e33851bf75..0ade1f436a8f8 100644 --- a/src/plugins/discover/public/application/main/utils/fetch_all.ts +++ b/src/plugins/discover/public/application/main/utils/fetch_all.ts @@ -10,7 +10,6 @@ import type { SavedSearch, SortOrder } from '@kbn/saved-search-plugin/public'; import { BehaviorSubject, filter, firstValueFrom, map, merge, scan } from 'rxjs'; import { reportPerformanceMetricEvent } from '@kbn/ebt-tools'; import { isEqual } from 'lodash'; -import { i18n } from '@kbn/i18n'; import type { DiscoverAppState } from '../services/discover_app_state_container'; import { updateVolatileSearchSource } from './update_search_source'; import { getRawRecordType } from './get_raw_record_type'; @@ -168,36 +167,36 @@ export function fetchAll( } } -export function fetchMoreDocuments( +export async function fetchMoreDocuments( dataSubjects: SavedSearchData, fetchDeps: FetchDeps ): Promise { - const { getAppState, getInternalState, services, savedSearch } = fetchDeps; - const searchSource = savedSearch.searchSource.createChild(); - try { + const { getAppState, getInternalState, services, savedSearch } = fetchDeps; + const searchSource = savedSearch.searchSource.createChild(); + const dataView = searchSource.getField('index')!; const query = getAppState().query; const recordRawType = getRawRecordType(query); if (recordRawType === RecordRawType.PLAIN) { // not supported yet - return Promise.resolve(); + return; } const lastDocuments = dataSubjects.documents$.getValue().result || []; const lastDocumentSort = lastDocuments[lastDocuments.length - 1]?.raw?.sort; if (!lastDocumentSort) { - return Promise.resolve(); + return; } searchSource.setField('searchAfter', lastDocumentSort); - // Mark subjects as loading + // Mark as loading sendLoadingMoreMsg(dataSubjects.documents$); - // Update the base searchSource, base for all child fetches + // Update the searchSource updateVolatileSearchSource(searchSource, { dataView, services, @@ -205,32 +204,15 @@ export function fetchMoreDocuments( customFilters: getInternalState().customFilters, }); - // Start fetching all required requests - const response = fetchDocuments(searchSource, fetchDeps); + // Fetch more documents + const { records } = await fetchDocuments(searchSource, fetchDeps); - // Handle results of the individual queries and forward the results to the corresponding dataSubjects - response - .then(({ records }) => { - sendLoadingMoreFinishedMsg(dataSubjects.documents$, records); - }) - .catch((error) => { - services.toastNotifications.addError(error, { - title: i18n.translate('discover.fetchMore.fetchingErrorTitle', { - defaultMessage: 'Error fetching more documents', - }), - }); - sendLoadingMoreFinishedMsg(dataSubjects.documents$, []); - }); + // Update the state and finish the loading state + sendLoadingMoreFinishedMsg(dataSubjects.documents$, records); } catch (error) { - services.toastNotifications.addError(error, { - title: i18n.translate('discover.fetchMore.requestErrorTitle', { - defaultMessage: 'Error requesting more documents', - }), - }); - // We also want to return a resolved promise in an error case, since it just indicates we're done with querying. sendLoadingMoreFinishedMsg(dataSubjects.documents$, []); + sendErrorMsg(dataSubjects.main$, error); } - return Promise.resolve(); } const fetchStatusByType = (subject: BehaviorSubject, type: string) => From 3fdf229fe1371a95bc50b63e7c156d148441590a Mon Sep 17 00:00:00 2001 From: Julia Rechkunova Date: Mon, 14 Aug 2023 13:54:50 +0200 Subject: [PATCH 39/49] [Discover] Update warnings handling --- .../main/hooks/use_saved_search_messages.ts | 31 +++++++++++++------ .../application/main/utils/fetch_all.ts | 12 +++++-- .../application/main/utils/fetch_documents.ts | 5 ++- 3 files changed, 33 insertions(+), 15 deletions(-) diff --git a/src/plugins/discover/public/application/main/hooks/use_saved_search_messages.ts b/src/plugins/discover/public/application/main/hooks/use_saved_search_messages.ts index d2ae99eab1be0..8a49ea7098c7c 100644 --- a/src/plugins/discover/public/application/main/hooks/use_saved_search_messages.ts +++ b/src/plugins/discover/public/application/main/hooks/use_saved_search_messages.ts @@ -8,6 +8,7 @@ import type { BehaviorSubject } from 'rxjs'; import type { DataTableRecord } from '@kbn/discover-utils/src/types'; +import type { SearchResponseInterceptedWarning } from '@kbn/search-response-warnings'; import { FetchStatus } from '../../types'; import type { DataDocuments$, @@ -76,25 +77,37 @@ export function sendLoadingMsg( /** * Send LOADING_MORE message via main observable */ -export function sendLoadingMoreMsg(data$: BehaviorSubject) { - if (data$.getValue().fetchStatus !== FetchStatus.LOADING_MORE) { - data$.next({ - ...data$.getValue(), +export function sendLoadingMoreMsg(documents$: DataDocuments$) { + if (documents$.getValue().fetchStatus !== FetchStatus.LOADING_MORE) { + documents$.next({ + ...documents$.getValue(), fetchStatus: FetchStatus.LOADING_MORE, - } as T); + }); } } /** * Finishing LOADING_MORE message */ -export function sendLoadingMoreFinishedMsg(data$: DataDocuments$, moreRecords: DataTableRecord[]) { - const currentValue = data$.getValue(); +export function sendLoadingMoreFinishedMsg( + documents$: DataDocuments$, + { + moreRecords, + interceptedWarnings, + }: { + moreRecords: DataTableRecord[]; + interceptedWarnings: SearchResponseInterceptedWarning[] | undefined; + } +) { + const currentValue = documents$.getValue(); if (currentValue.fetchStatus === FetchStatus.LOADING_MORE) { - data$.next({ + documents$.next({ ...currentValue, fetchStatus: FetchStatus.COMPLETE, - result: [...(currentValue.result || []), ...moreRecords], + result: moreRecords?.length + ? [...(currentValue.result || []), ...moreRecords] + : currentValue.result, + interceptedWarnings, }); } } diff --git a/src/plugins/discover/public/application/main/utils/fetch_all.ts b/src/plugins/discover/public/application/main/utils/fetch_all.ts index 0ade1f436a8f8..4eefb9b7fe2eb 100644 --- a/src/plugins/discover/public/application/main/utils/fetch_all.ts +++ b/src/plugins/discover/public/application/main/utils/fetch_all.ts @@ -205,12 +205,18 @@ export async function fetchMoreDocuments( }); // Fetch more documents - const { records } = await fetchDocuments(searchSource, fetchDeps); + const { records, interceptedWarnings } = await fetchDocuments(searchSource, fetchDeps); // Update the state and finish the loading state - sendLoadingMoreFinishedMsg(dataSubjects.documents$, records); + sendLoadingMoreFinishedMsg(dataSubjects.documents$, { + moreRecords: records, + interceptedWarnings, + }); } catch (error) { - sendLoadingMoreFinishedMsg(dataSubjects.documents$, []); + sendLoadingMoreFinishedMsg(dataSubjects.documents$, { + moreRecords: [], + interceptedWarnings: undefined, + }); sendErrorMsg(dataSubjects.main$, error); } } diff --git a/src/plugins/discover/public/application/main/utils/fetch_documents.ts b/src/plugins/discover/public/application/main/utils/fetch_documents.ts index 58622befa4399..b75f8ce7b583f 100644 --- a/src/plugins/discover/public/application/main/utils/fetch_documents.ts +++ b/src/plugins/discover/public/application/main/utils/fetch_documents.ts @@ -37,7 +37,6 @@ export const fetchDocuments = ( } const dataView = searchSource.getField('index')!; const isFetchingMore = Boolean(searchSource.getField('searchAfter')); - const disableShardFailureWarning = isFetchingMore ? false : DISABLE_SHARD_FAILURE_WARNING; const executionContext = { description: isFetchingMore ? 'fetch more documents' : 'fetch documents', @@ -61,7 +60,7 @@ export const fetchDocuments = ( }), }, executionContext, - disableShardFailureWarning, + disableShardFailureWarning: DISABLE_SHARD_FAILURE_WARNING, }) .pipe( filter((res) => isCompleteResponse(res)), @@ -77,7 +76,7 @@ export const fetchDocuments = ( services, adapter, options: { - disableShardFailureWarning, + disableShardFailureWarning: DISABLE_SHARD_FAILURE_WARNING, }, }) : []; From 3752fdc51c931743d622f2de0808cc5eeaf75de3 Mon Sep 17 00:00:00 2001 From: Julia Rechkunova Date: Mon, 14 Aug 2023 14:55:59 +0200 Subject: [PATCH 40/49] [Discover] Merge isLoading and isLoadingMore props into loadingState prop --- .../context/context_app_content.tsx | 8 +++--- .../components/layout/discover_documents.tsx | 17 +++++++----- .../discover_grid/discover_grid.test.tsx | 4 +-- .../discover_grid/discover_grid.tsx | 27 ++++++++++--------- .../doc_table/doc_table_embeddable.tsx | 4 +-- .../saved_search_embeddable_base.tsx | 4 +-- .../saved_search_embeddable_component.tsx | 11 +++++--- .../public/embeddable/saved_search_grid.tsx | 17 +++++++----- src/plugins/discover/server/ui_settings.ts | 2 +- 9 files changed, 56 insertions(+), 38 deletions(-) diff --git a/src/plugins/discover/public/application/context/context_app_content.tsx b/src/plugins/discover/public/application/context/context_app_content.tsx index fa9ed3b96c9ff..c72b7f366e5ce 100644 --- a/src/plugins/discover/public/application/context/context_app_content.tsx +++ b/src/plugins/discover/public/application/context/context_app_content.tsx @@ -6,7 +6,7 @@ * Side Public License, v 1. */ -import React, { useState, Fragment, useMemo, useCallback } from 'react'; +import React, { Fragment, useCallback, useMemo, useState } from 'react'; import { FormattedMessage } from '@kbn/i18n-react'; import { EuiHorizontalRule, EuiSpacer, EuiText } from '@elastic/eui'; import type { DataView } from '@kbn/data-views-plugin/public'; @@ -15,13 +15,13 @@ import type { SortOrder } from '@kbn/saved-search-plugin/public'; import { CellActionsProvider } from '@kbn/cell-actions'; import type { DataTableRecord } from '@kbn/discover-utils/types'; import { - SearchResponseWarnings, type SearchResponseInterceptedWarning, + SearchResponseWarnings, } from '@kbn/search-response-warnings'; import { CONTEXT_STEP_SETTING, DOC_HIDE_TIME_COLUMN_SETTING } from '@kbn/discover-utils'; import { LoadingStatus } from './services/context_query_state'; import { ActionBar } from './components/action_bar/action_bar'; -import { DiscoverGrid } from '../../components/discover_grid/discover_grid'; +import { DataLoadingState, DiscoverGrid } from '../../components/discover_grid/discover_grid'; import { DocViewFilterFn } from '../../services/doc_views/doc_views_types'; import { AppState } from './services/context_state'; import { SurrDocType } from './services/context'; @@ -169,7 +169,7 @@ export function ContextAppContent({ rows={rows} dataView={dataView} expandedDoc={expandedDoc} - isLoading={isAnchorLoading} + loadingState={isAnchorLoading ? DataLoadingState.loading : DataLoadingState.loaded} sampleSize={0} sort={sort as SortOrder[]} isSortEnabled={false} diff --git a/src/plugins/discover/public/application/main/components/layout/discover_documents.tsx b/src/plugins/discover/public/application/main/components/layout/discover_documents.tsx index 648cdcaada8c1..a378d25fb04de 100644 --- a/src/plugins/discover/public/application/main/components/layout/discover_documents.tsx +++ b/src/plugins/discover/public/application/main/components/layout/discover_documents.tsx @@ -24,15 +24,15 @@ import { SearchResponseWarnings } from '@kbn/search-response-warnings'; import { DOC_HIDE_TIME_COLUMN_SETTING, DOC_TABLE_LEGACY, + HIDE_ANNOUNCEMENTS, SAMPLE_SIZE_SETTING, SEARCH_FIELDS_FROM_SOURCE, - HIDE_ANNOUNCEMENTS, } from '@kbn/discover-utils'; import { useInternalStateSelector } from '../../services/discover_internal_state_container'; import { useAppStateSelector } from '../../services/discover_app_state_container'; import { useDiscoverServices } from '../../../../hooks/use_discover_services'; import { DocViewFilterFn } from '../../../../services/doc_views/doc_views_types'; -import { DiscoverGrid } from '../../../../components/discover_grid/discover_grid'; +import { DataLoadingState, DiscoverGrid } from '../../../../components/discover_grid/discover_grid'; import { FetchStatus } from '../../../types'; import { useColumns } from '../../../../hooks/use_data_grid_columns'; import { RecordRawType } from '../../services/discover_data_state_container'; @@ -57,7 +57,7 @@ const progressStyle = css` `; const DocTableInfiniteMemoized = React.memo(DocTableInfinite); -const DataGridMemoized = React.memo(DiscoverGrid); +const DiscoverGridMemoized = React.memo(DiscoverGrid); // export needs for testing export const onResize = ( @@ -251,13 +251,18 @@ function DiscoverDocumentsComponent({ - @@ -632,7 +635,7 @@ export const DiscoverGrid = ({ return (

- {!isLoading && + {loadingState !== DataLoadingState.loading && isPaginationEnabled && ( // we hide the footer for Surrounding Documents page void; @@ -79,7 +79,7 @@ export const DocTableEmbeddable = (props: DocTableEmbeddableProps) => { ); const shouldShowLimitedResultsWarning = useMemo( - () => !hasNextPage && props.rows.length < props.totalHitCount, + () => !hasNextPage && props.totalHitCount && props.rows.length < props.totalHitCount, [hasNextPage, props.rows.length, props.totalHitCount] ); diff --git a/src/plugins/discover/public/embeddable/saved_search_embeddable_base.tsx b/src/plugins/discover/public/embeddable/saved_search_embeddable_base.tsx index b41c70676c754..6c05451ec5a79 100644 --- a/src/plugins/discover/public/embeddable/saved_search_embeddable_base.tsx +++ b/src/plugins/discover/public/embeddable/saved_search_embeddable_base.tsx @@ -20,7 +20,7 @@ const containerStyles = css` export interface SavedSearchEmbeddableBaseProps { isLoading: boolean; - totalHitCount: number; + totalHitCount?: number; prepend?: React.ReactElement; append?: React.ReactElement; dataTestSubj?: string; @@ -55,7 +55,7 @@ export const SavedSearchEmbeddableBase: React.FC > {Boolean(prepend) && {prepend}} - {Boolean(totalHitCount) && ( + {!!totalHitCount && ( diff --git a/src/plugins/discover/public/embeddable/saved_search_embeddable_component.tsx b/src/plugins/discover/public/embeddable/saved_search_embeddable_component.tsx index 03ab602cd760a..fd28f3114211f 100644 --- a/src/plugins/discover/public/embeddable/saved_search_embeddable_component.tsx +++ b/src/plugins/discover/public/embeddable/saved_search_embeddable_component.tsx @@ -8,7 +8,11 @@ import React from 'react'; import { AggregateQuery, Query } from '@kbn/es-query'; -import { DiscoverGridEmbeddable, DiscoverGridEmbeddableProps } from './saved_search_grid'; +import { + DiscoverGridEmbeddable, + DiscoverGridEmbeddableProps, + DataLoadingState, +} from './saved_search_grid'; import { DiscoverDocTableEmbeddable } from '../components/doc_table/create_doc_table_embeddable'; import { DocTableEmbeddableProps } from '../components/doc_table/doc_table_embeddable'; import { isTextBasedQuery } from '../application/main/utils/is_text_based_query'; @@ -32,14 +36,15 @@ export function SavedSearchEmbeddableComponent({ const isPlainRecord = isTextBasedQuery(query); return ( ); } return ( - Record', }, }), + requiresPageReload: true, category: ['discover'], schema: schema.boolean(), metric: { @@ -206,7 +207,6 @@ export const getUiSettings: (docLinks: DocLinksServiceSetup) => Record Date: Tue, 15 Aug 2023 10:42:18 +0200 Subject: [PATCH 41/49] [Discover] Remove the redundant argument --- .../public/application/context/services/context.ts | 2 +- .../context/utils/get_es_query_search_after.ts | 10 ++++------ 2 files changed, 5 insertions(+), 7 deletions(-) diff --git a/src/plugins/discover/public/application/context/services/context.ts b/src/plugins/discover/public/application/context/services/context.ts index 387e63f4f6fe9..df47356394821 100644 --- a/src/plugins/discover/public/application/context/services/context.ts +++ b/src/plugins/discover/public/application/context/services/context.ts @@ -88,7 +88,7 @@ export async function fetchSurroundingDocs( break; } - const searchAfter = getEsQuerySearchAfter(type, rows, timeField, anchor); + const searchAfter = getEsQuerySearchAfter(type, rows, anchor); const sort = getEsQuerySort({ timeFieldName: timeField, diff --git a/src/plugins/discover/public/application/context/utils/get_es_query_search_after.ts b/src/plugins/discover/public/application/context/utils/get_es_query_search_after.ts index a2f12643931fb..18792cb6a7ff8 100644 --- a/src/plugins/discover/public/application/context/utils/get_es_query_search_after.ts +++ b/src/plugins/discover/public/application/context/utils/get_es_query_search_after.ts @@ -17,15 +17,13 @@ import { SurrDocType } from '../services/context'; */ export function getEsQuerySearchAfter( type: SurrDocType, - documents: DataTableRecord[], - timeFieldName: string, + rows: DataTableRecord[], anchor: DataTableRecord ): EsQuerySearchAfter { - if (documents.length) { + if (rows.length) { // already surrounding docs -> first or last record is used - const afterTimeRecIdx = - type === SurrDocType.SUCCESSORS && documents.length ? documents.length - 1 : 0; - const afterTimeDocRaw = documents[afterTimeRecIdx].raw; + const afterTimeRecIdx = type === SurrDocType.SUCCESSORS && rows.length ? rows.length - 1 : 0; + const afterTimeDocRaw = rows[afterTimeRecIdx].raw; return [ afterTimeDocRaw.sort?.[0] as string | number, afterTimeDocRaw.sort?.[1] as string | number, From ee11a09f6104aa499e70be097122857376abbb31 Mon Sep 17 00:00:00 2001 From: Julia Rechkunova Date: Tue, 15 Aug 2023 11:15:35 +0200 Subject: [PATCH 42/49] [Discover] Add tests to useFetchMoreRecords hook --- .../layout/use_fetch_more_records.test.tsx | 134 ++++++++++++++++++ 1 file changed, 134 insertions(+) create mode 100644 src/plugins/discover/public/application/main/components/layout/use_fetch_more_records.test.tsx diff --git a/src/plugins/discover/public/application/main/components/layout/use_fetch_more_records.test.tsx b/src/plugins/discover/public/application/main/components/layout/use_fetch_more_records.test.tsx new file mode 100644 index 0000000000000..3b30214f1b2de --- /dev/null +++ b/src/plugins/discover/public/application/main/components/layout/use_fetch_more_records.test.tsx @@ -0,0 +1,134 @@ +/* + * 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 { BehaviorSubject } from 'rxjs'; +import { renderHook } from '@testing-library/react-hooks'; +import { buildDataTableRecord } from '@kbn/discover-utils'; +import { dataViewMock, esHitsMock } from '@kbn/discover-utils/src/__mocks__'; +import { useFetchMoreRecords } from './use_fetch_more_records'; +import { getDiscoverStateMock } from '../../../../__mocks__/discover_state.mock'; +import { DataDocuments$, DataTotalHits$ } from '../../services/discover_data_state_container'; +import { FetchStatus } from '../../../types'; + +describe('useFetchMoreRecords', () => { + const records = esHitsMock.map((hit) => buildDataTableRecord(hit, dataViewMock)); + + const getStateContainer = ({ + fetchStatus, + loadedRecordsCount, + totalRecordsCount, + }: { + fetchStatus: FetchStatus; + loadedRecordsCount: number; + totalRecordsCount: number; + }) => { + const rows = records.slice(0, loadedRecordsCount); + if (rows.length) { + rows[rows.length - 1].raw.sort = [1000]; + } + const stateContainer = getDiscoverStateMock({ isTimeBased: true }); + stateContainer.dataState.data$.documents$ = new BehaviorSubject({ + fetchStatus, + result: records.slice(0, loadedRecordsCount), + }) as DataDocuments$; + stateContainer.dataState.data$.totalHits$ = new BehaviorSubject({ + fetchStatus, + result: totalRecordsCount, + }) as DataTotalHits$; + + return stateContainer; + }; + + it('should not be allowed if all records are already loaded', async () => { + const { + result: { current }, + } = renderHook(() => + useFetchMoreRecords({ + isTextBasedQuery: false, + stateContainer: getStateContainer({ + fetchStatus: FetchStatus.COMPLETE, + loadedRecordsCount: 3, + totalRecordsCount: 3, + }), + }) + ); + expect(current.onFetchMoreRecords).toBeUndefined(); + expect(current.isMoreDataLoading).toBe(false); + expect(current.totalHits).toBe(3); + }); + + it('should be allowed when there are more records to load', async () => { + const { + result: { current }, + } = renderHook(() => + useFetchMoreRecords({ + isTextBasedQuery: false, + stateContainer: getStateContainer({ + fetchStatus: FetchStatus.COMPLETE, + loadedRecordsCount: 3, + totalRecordsCount: 5, + }), + }) + ); + expect(current.onFetchMoreRecords).toBeDefined(); + expect(current.isMoreDataLoading).toBe(false); + expect(current.totalHits).toBe(5); + }); + + it('should not be allowed when there is no initial documents', async () => { + const { + result: { current }, + } = renderHook(() => + useFetchMoreRecords({ + isTextBasedQuery: false, + stateContainer: getStateContainer({ + fetchStatus: FetchStatus.COMPLETE, + loadedRecordsCount: 0, + totalRecordsCount: 5, + }), + }) + ); + expect(current.onFetchMoreRecords).toBeUndefined(); + expect(current.isMoreDataLoading).toBe(false); + expect(current.totalHits).toBe(5); + }); + + it('should return loading status correctly', async () => { + const { + result: { current }, + } = renderHook(() => + useFetchMoreRecords({ + isTextBasedQuery: false, + stateContainer: getStateContainer({ + fetchStatus: FetchStatus.LOADING_MORE, + loadedRecordsCount: 3, + totalRecordsCount: 5, + }), + }) + ); + expect(current.onFetchMoreRecords).toBeDefined(); + expect(current.isMoreDataLoading).toBe(true); + expect(current.totalHits).toBe(5); + }); + + it('should not be allowed for text-based queries', async () => { + const { + result: { current }, + } = renderHook(() => + useFetchMoreRecords({ + isTextBasedQuery: true, + stateContainer: getStateContainer({ + fetchStatus: FetchStatus.COMPLETE, + loadedRecordsCount: 3, + totalRecordsCount: 5, + }), + }) + ); + expect(current.onFetchMoreRecords).toBeUndefined(); + }); +}); From 63ec44c5ba62ee3956af0363f65176cb219a4b4d Mon Sep 17 00:00:00 2001 From: Julia Rechkunova Date: Tue, 15 Aug 2023 11:40:20 +0200 Subject: [PATCH 43/49] [Discover] Add tests for sendLoadingMoreMsg and sendLoadingMoreFinishedMsg --- .../hooks/use_saved_search_messages.test.ts | 71 ++++++++++++++++++- 1 file changed, 70 insertions(+), 1 deletion(-) diff --git a/src/plugins/discover/public/application/main/hooks/use_saved_search_messages.test.ts b/src/plugins/discover/public/application/main/hooks/use_saved_search_messages.test.ts index fb0c5a7b2b3e0..44b01f681ca8d 100644 --- a/src/plugins/discover/public/application/main/hooks/use_saved_search_messages.test.ts +++ b/src/plugins/discover/public/application/main/hooks/use_saved_search_messages.test.ts @@ -11,13 +11,22 @@ import { sendErrorMsg, sendErrorTo, sendLoadingMsg, + sendLoadingMoreMsg, + sendLoadingMoreFinishedMsg, sendNoResultsFoundMsg, sendPartialMsg, } from './use_saved_search_messages'; import { FetchStatus } from '../../types'; import { BehaviorSubject } from 'rxjs'; -import { DataMainMsg, RecordRawType } from '../services/discover_data_state_container'; +import { + DataDocumentsMsg, + DataMainMsg, + RecordRawType, +} from '../services/discover_data_state_container'; import { filter } from 'rxjs/operators'; +import { dataViewMock, esHitsMock } from '@kbn/discover-utils/src/__mocks__'; +import { buildDataTableRecord } from '@kbn/discover-utils'; +import { searchResponseWarningsMock } from '@kbn/search-response-warnings/src/__mocks__/search_response_warnings'; describe('test useSavedSearch message generators', () => { test('sendCompleteMsg', (done) => { @@ -69,6 +78,66 @@ describe('test useSavedSearch message generators', () => { recordRawType: RecordRawType.DOCUMENT, }); }); + test('sendLoadingMoreMsg', (done) => { + const documents$ = new BehaviorSubject({ + fetchStatus: FetchStatus.COMPLETE, + }); + documents$.subscribe((value) => { + if (value.fetchStatus !== FetchStatus.COMPLETE) { + expect(value.fetchStatus).toBe(FetchStatus.LOADING_MORE); + done(); + } + }); + sendLoadingMoreMsg(documents$); + }); + test('sendLoadingMoreFinishedMsg', (done) => { + const records = esHitsMock.map((hit) => buildDataTableRecord(hit, dataViewMock)); + const initialRecords = [records[0], records[1]]; + const moreRecords = [records[2], records[3]]; + + const documents$ = new BehaviorSubject({ + fetchStatus: FetchStatus.LOADING_MORE, + result: initialRecords, + }); + documents$.subscribe((value) => { + if (value.fetchStatus !== FetchStatus.LOADING_MORE) { + expect(value.fetchStatus).toBe(FetchStatus.COMPLETE); + expect(value.result).toStrictEqual([...initialRecords, ...moreRecords]); + expect(value.interceptedWarnings).toHaveLength(searchResponseWarningsMock.length); + done(); + } + }); + sendLoadingMoreFinishedMsg(documents$, { + moreRecords, + interceptedWarnings: searchResponseWarningsMock.map((warning) => ({ + originalWarning: warning, + })), + }); + }); + test('sendLoadingMoreFinishedMsg after an exception', (done) => { + const records = esHitsMock.map((hit) => buildDataTableRecord(hit, dataViewMock)); + const initialRecords = [records[0], records[1]]; + + const documents$ = new BehaviorSubject({ + fetchStatus: FetchStatus.LOADING_MORE, + result: initialRecords, + interceptedWarnings: searchResponseWarningsMock.map((warning) => ({ + originalWarning: warning, + })), + }); + documents$.subscribe((value) => { + if (value.fetchStatus !== FetchStatus.LOADING_MORE) { + expect(value.fetchStatus).toBe(FetchStatus.COMPLETE); + expect(value.result).toBe(initialRecords); + expect(value.interceptedWarnings).toBeUndefined(); + done(); + } + }); + sendLoadingMoreFinishedMsg(documents$, { + moreRecords: [], + interceptedWarnings: undefined, + }); + }); test('sendErrorMsg', (done) => { const main$ = new BehaviorSubject({ fetchStatus: FetchStatus.PARTIAL }); main$.subscribe((value) => { From 2837cf696f3045f3f6224404352b8e9ca3105f45 Mon Sep 17 00:00:00 2001 From: Julia Rechkunova Date: Tue, 15 Aug 2023 14:16:35 +0200 Subject: [PATCH 44/49] [Discover] Add tests for fetchMoreDocuments --- .../src/__mocks__/es_hits.ts | 5 + .../layout/use_fetch_more_records.test.tsx | 8 +- .../hooks/use_saved_search_messages.test.ts | 6 +- .../discover_data_state_container.test.ts | 55 ++++++++++- .../application/main/utils/fetch_all.test.ts | 97 ++++++++++++++++++- 5 files changed, 158 insertions(+), 13 deletions(-) diff --git a/packages/kbn-discover-utils/src/__mocks__/es_hits.ts b/packages/kbn-discover-utils/src/__mocks__/es_hits.ts index a23bb1af0ceaf..84891fec5ab10 100644 --- a/packages/kbn-discover-utils/src/__mocks__/es_hits.ts +++ b/packages/kbn-discover-utils/src/__mocks__/es_hits.ts @@ -49,3 +49,8 @@ export const esHitsMock = [ }, }, ]; + +export const esHitsMockWithSort = esHitsMock.map((hit) => ({ + ...hit, + sort: [hit._source.date], // some `sort` param should be specified for "fetch more" to work +})); diff --git a/src/plugins/discover/public/application/main/components/layout/use_fetch_more_records.test.tsx b/src/plugins/discover/public/application/main/components/layout/use_fetch_more_records.test.tsx index 3b30214f1b2de..6851344538686 100644 --- a/src/plugins/discover/public/application/main/components/layout/use_fetch_more_records.test.tsx +++ b/src/plugins/discover/public/application/main/components/layout/use_fetch_more_records.test.tsx @@ -9,14 +9,14 @@ import { BehaviorSubject } from 'rxjs'; import { renderHook } from '@testing-library/react-hooks'; import { buildDataTableRecord } from '@kbn/discover-utils'; -import { dataViewMock, esHitsMock } from '@kbn/discover-utils/src/__mocks__'; +import { dataViewMock, esHitsMockWithSort } from '@kbn/discover-utils/src/__mocks__'; import { useFetchMoreRecords } from './use_fetch_more_records'; import { getDiscoverStateMock } from '../../../../__mocks__/discover_state.mock'; import { DataDocuments$, DataTotalHits$ } from '../../services/discover_data_state_container'; import { FetchStatus } from '../../../types'; describe('useFetchMoreRecords', () => { - const records = esHitsMock.map((hit) => buildDataTableRecord(hit, dataViewMock)); + const records = esHitsMockWithSort.map((hit) => buildDataTableRecord(hit, dataViewMock)); const getStateContainer = ({ fetchStatus, @@ -27,10 +27,6 @@ describe('useFetchMoreRecords', () => { loadedRecordsCount: number; totalRecordsCount: number; }) => { - const rows = records.slice(0, loadedRecordsCount); - if (rows.length) { - rows[rows.length - 1].raw.sort = [1000]; - } const stateContainer = getDiscoverStateMock({ isTimeBased: true }); stateContainer.dataState.data$.documents$ = new BehaviorSubject({ fetchStatus, diff --git a/src/plugins/discover/public/application/main/hooks/use_saved_search_messages.test.ts b/src/plugins/discover/public/application/main/hooks/use_saved_search_messages.test.ts index 44b01f681ca8d..7b96c2673f3eb 100644 --- a/src/plugins/discover/public/application/main/hooks/use_saved_search_messages.test.ts +++ b/src/plugins/discover/public/application/main/hooks/use_saved_search_messages.test.ts @@ -24,7 +24,7 @@ import { RecordRawType, } from '../services/discover_data_state_container'; import { filter } from 'rxjs/operators'; -import { dataViewMock, esHitsMock } from '@kbn/discover-utils/src/__mocks__'; +import { dataViewMock, esHitsMockWithSort } from '@kbn/discover-utils/src/__mocks__'; import { buildDataTableRecord } from '@kbn/discover-utils'; import { searchResponseWarningsMock } from '@kbn/search-response-warnings/src/__mocks__/search_response_warnings'; @@ -91,7 +91,7 @@ describe('test useSavedSearch message generators', () => { sendLoadingMoreMsg(documents$); }); test('sendLoadingMoreFinishedMsg', (done) => { - const records = esHitsMock.map((hit) => buildDataTableRecord(hit, dataViewMock)); + const records = esHitsMockWithSort.map((hit) => buildDataTableRecord(hit, dataViewMock)); const initialRecords = [records[0], records[1]]; const moreRecords = [records[2], records[3]]; @@ -115,7 +115,7 @@ describe('test useSavedSearch message generators', () => { }); }); test('sendLoadingMoreFinishedMsg after an exception', (done) => { - const records = esHitsMock.map((hit) => buildDataTableRecord(hit, dataViewMock)); + const records = esHitsMockWithSort.map((hit) => buildDataTableRecord(hit, dataViewMock)); const initialRecords = [records[0], records[1]]; const documents$ = new BehaviorSubject({ diff --git a/src/plugins/discover/public/application/main/services/discover_data_state_container.test.ts b/src/plugins/discover/public/application/main/services/discover_data_state_container.test.ts index ccb3f4d39a877..33920a45baf94 100644 --- a/src/plugins/discover/public/application/main/services/discover_data_state_container.test.ts +++ b/src/plugins/discover/public/application/main/services/discover_data_state_container.test.ts @@ -5,15 +5,28 @@ * in compliance with, at your election, the Elastic License 2.0 or the Server * Side Public License, v 1. */ -import { Subject } from 'rxjs'; +import { BehaviorSubject, Subject } from 'rxjs'; import { waitFor } from '@testing-library/react'; +import { buildDataTableRecord } from '@kbn/discover-utils'; +import { dataViewMock, esHitsMockWithSort } from '@kbn/discover-utils/src/__mocks__'; import { discoverServiceMock } from '../../../__mocks__/services'; import { savedSearchMockWithSQL } from '../../../__mocks__/saved_search'; import { FetchStatus } from '../../types'; import { setUrlTracker } from '../../../kibana_services'; import { urlTrackerMock } from '../../../__mocks__/url_tracker.mock'; -import { RecordRawType } from './discover_data_state_container'; +import { DataDocuments$, RecordRawType } from './discover_data_state_container'; import { getDiscoverStateMock } from '../../../__mocks__/discover_state.mock'; +import { fetchDocuments } from '../utils/fetch_documents'; + +jest.mock('../utils/fetch_documents', () => ({ + fetchDocuments: jest.fn().mockResolvedValue({ records: [] }), +})); + +jest.mock('@kbn/ebt-tools', () => ({ + reportPerformanceMetricEvent: jest.fn(), +})); + +const mockFetchDocuments = fetchDocuments as unknown as jest.MockedFunction; setUrlTracker(urlTrackerMock); describe('test getDataStateContainer', () => { @@ -84,4 +97,42 @@ describe('test getDataStateContainer', () => { expect(stateContainer.dataState.data$.main$.getValue().recordRawType).toBe(RecordRawType.PLAIN); }); + + test('refetch$ accepts "fetch_more" signal', (done) => { + const records = esHitsMockWithSort.map((hit) => buildDataTableRecord(hit, dataViewMock)); + const initialRecords = [records[0], records[1]]; + const moreRecords = [records[2], records[3]]; + + mockFetchDocuments.mockResolvedValue({ records: moreRecords }); + + const stateContainer = getDiscoverStateMock({ isTimeBased: true }); + stateContainer.dataState.data$.documents$ = new BehaviorSubject({ + fetchStatus: FetchStatus.COMPLETE, + result: initialRecords, + }) as DataDocuments$; + + const dataState = stateContainer.dataState; + + const unsubscribe = dataState.subscribe(); + + expect(dataState.data$.documents$.value.result).toEqual(initialRecords); + + let hasLoadingMoreStarted = false; + + stateContainer.dataState.data$.documents$.subscribe((value) => { + if (value.fetchStatus === FetchStatus.LOADING_MORE) { + hasLoadingMoreStarted = true; + return; + } + + if (hasLoadingMoreStarted && value.fetchStatus === FetchStatus.COMPLETE) { + expect(value.result).toEqual([...initialRecords, ...moreRecords]); + + unsubscribe(); + done(); + } + }); + + dataState.refetch$.next('fetch_more'); + }); }); diff --git a/src/plugins/discover/public/application/main/utils/fetch_all.test.ts b/src/plugins/discover/public/application/main/utils/fetch_all.test.ts index 1be1be3053ea6..7f19ec3a23695 100644 --- a/src/plugins/discover/public/application/main/utils/fetch_all.test.ts +++ b/src/plugins/discover/public/application/main/utils/fetch_all.test.ts @@ -12,7 +12,7 @@ import { SearchSource } from '@kbn/data-plugin/public'; import { RequestAdapter } from '@kbn/inspector-plugin/common'; import { savedSearchMock } from '../../../__mocks__/saved_search'; import { discoverServiceMock } from '../../../__mocks__/services'; -import { fetchAll } from './fetch_all'; +import { fetchAll, fetchMoreDocuments } from './fetch_all'; import { DataAvailableFieldsMsg, DataDocumentsMsg, @@ -24,7 +24,9 @@ import { import { fetchDocuments } from './fetch_documents'; import { fetchSql } from './fetch_sql'; import { buildDataTableRecord } from '@kbn/discover-utils'; -import { dataViewMock } from '@kbn/discover-utils/src/__mocks__'; +import { dataViewMock, esHitsMockWithSort } from '@kbn/discover-utils/src/__mocks__'; +import { searchResponseWarningsMock } from '@kbn/search-response-warnings/src/__mocks__/search_response_warnings'; + jest.mock('./fetch_documents', () => ({ fetchDocuments: jest.fn().mockResolvedValue([]), })); @@ -288,4 +290,95 @@ describe('test fetchAll', () => { }, ]); }); + + describe('fetchMoreDocuments', () => { + const records = esHitsMockWithSort.map((hit) => buildDataTableRecord(hit, dataViewMock)); + const initialRecords = [records[0], records[1]]; + const moreRecords = [records[2], records[3]]; + + const interceptedWarnings = searchResponseWarningsMock.map((warning) => ({ + originalWarning: warning, + })); + + test('should add more records', async () => { + const collectDocuments = subjectCollector(subjects.documents$); + const collectMain = subjectCollector(subjects.main$); + mockFetchDocuments.mockResolvedValue({ records: moreRecords, interceptedWarnings }); + subjects.documents$.next({ + fetchStatus: FetchStatus.COMPLETE, + recordRawType: RecordRawType.DOCUMENT, + result: initialRecords, + }); + fetchMoreDocuments(subjects, deps); + await waitForNextTick(); + + expect(await collectDocuments()).toEqual([ + { fetchStatus: FetchStatus.UNINITIALIZED }, + { + fetchStatus: FetchStatus.COMPLETE, + recordRawType: RecordRawType.DOCUMENT, + result: initialRecords, + }, + { + fetchStatus: FetchStatus.LOADING_MORE, + recordRawType: RecordRawType.DOCUMENT, + result: initialRecords, + }, + { + fetchStatus: FetchStatus.COMPLETE, + recordRawType: RecordRawType.DOCUMENT, + result: [...initialRecords, ...moreRecords], + interceptedWarnings, + }, + ]); + expect(await collectMain()).toEqual([ + { + fetchStatus: FetchStatus.UNINITIALIZED, + }, + ]); + }); + + test('should handle exceptions', async () => { + const collectDocuments = subjectCollector(subjects.documents$); + const collectMain = subjectCollector(subjects.main$); + mockFetchDocuments.mockRejectedValue({ msg: 'This query failed' }); + subjects.documents$.next({ + fetchStatus: FetchStatus.COMPLETE, + recordRawType: RecordRawType.DOCUMENT, + result: initialRecords, + }); + fetchMoreDocuments(subjects, deps); + await waitForNextTick(); + + expect(await collectDocuments()).toEqual([ + { fetchStatus: FetchStatus.UNINITIALIZED }, + { + fetchStatus: FetchStatus.COMPLETE, + recordRawType: RecordRawType.DOCUMENT, + result: initialRecords, + }, + { + fetchStatus: FetchStatus.LOADING_MORE, + recordRawType: RecordRawType.DOCUMENT, + result: initialRecords, + }, + { + fetchStatus: FetchStatus.COMPLETE, + recordRawType: RecordRawType.DOCUMENT, + result: initialRecords, + }, + ]); + expect(await collectMain()).toEqual([ + { + fetchStatus: FetchStatus.UNINITIALIZED, + }, + { + error: { + msg: 'This query failed', + }, + fetchStatus: 'error', + }, + ]); + }); + }); }); From 729475596714556e7599687cc3c8cf0af5cecf0a Mon Sep 17 00:00:00 2001 From: Julia Rechkunova Date: Tue, 15 Aug 2023 16:14:51 +0200 Subject: [PATCH 45/49] [Discover] Skip abort errors --- src/plugins/discover/public/application/main/utils/fetch_all.ts | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/src/plugins/discover/public/application/main/utils/fetch_all.ts b/src/plugins/discover/public/application/main/utils/fetch_all.ts index 4eefb9b7fe2eb..bf2bc0c1eb1b0 100644 --- a/src/plugins/discover/public/application/main/utils/fetch_all.ts +++ b/src/plugins/discover/public/application/main/utils/fetch_all.ts @@ -217,7 +217,7 @@ export async function fetchMoreDocuments( moreRecords: [], interceptedWarnings: undefined, }); - sendErrorMsg(dataSubjects.main$, error); + sendErrorTo(dataSubjects.main$)(error); } } From 453fbbdf908236bf06a05b999cfa3f2a5902b112 Mon Sep 17 00:00:00 2001 From: Julia Rechkunova Date: Tue, 15 Aug 2023 17:52:11 +0200 Subject: [PATCH 46/49] [Discover] Reuse current search session id for "Load more" requests --- .../discover_data_state_container.test.ts | 23 +++++++++++++++++++ .../services/discover_data_state_container.ts | 4 +++- .../main/services/discover_search_session.ts | 7 ++++++ 3 files changed, 33 insertions(+), 1 deletion(-) diff --git a/src/plugins/discover/public/application/main/services/discover_data_state_container.test.ts b/src/plugins/discover/public/application/main/services/discover_data_state_container.test.ts index 33920a45baf94..d3f600dcfaff8 100644 --- a/src/plugins/discover/public/application/main/services/discover_data_state_container.test.ts +++ b/src/plugins/discover/public/application/main/services/discover_data_state_container.test.ts @@ -41,6 +41,11 @@ describe('test getDataStateContainer', () => { }); test('refetch$ triggers a search', async () => { const stateContainer = getDiscoverStateMock({ isTimeBased: true }); + jest.spyOn(stateContainer.searchSessionManager, 'getNextSearchSessionId'); + jest.spyOn(stateContainer.searchSessionManager, 'getCurrentSearchSessionId'); + expect( + stateContainer.searchSessionManager.getNextSearchSessionId as jest.Mock + ).not.toHaveBeenCalled(); discoverServiceMock.data.query.timefilter.timefilter.getTime = jest.fn(() => { return { from: '2021-05-01T20:00:00Z', to: '2021-05-02T20:00:00Z' }; @@ -59,6 +64,15 @@ describe('test getDataStateContainer', () => { expect(dataState.data$.totalHits$.value.result).toBe(0); expect(dataState.data$.documents$.value.result).toEqual([]); + + // gets a new search session id + expect( + stateContainer.searchSessionManager.getNextSearchSessionId as jest.Mock + ).toHaveBeenCalled(); + expect( + stateContainer.searchSessionManager.getCurrentSearchSessionId as jest.Mock + ).not.toHaveBeenCalled(); + unsubscribe(); }); @@ -111,6 +125,11 @@ describe('test getDataStateContainer', () => { result: initialRecords, }) as DataDocuments$; + jest.spyOn(stateContainer.searchSessionManager, 'getCurrentSearchSessionId'); + expect( + stateContainer.searchSessionManager.getCurrentSearchSessionId as jest.Mock + ).not.toHaveBeenCalled(); + const dataState = stateContainer.dataState; const unsubscribe = dataState.subscribe(); @@ -127,6 +146,10 @@ describe('test getDataStateContainer', () => { if (hasLoadingMoreStarted && value.fetchStatus === FetchStatus.COMPLETE) { expect(value.result).toEqual([...initialRecords, ...moreRecords]); + // it uses the same current search session id + expect( + stateContainer.searchSessionManager.getCurrentSearchSessionId as jest.Mock + ).toHaveBeenCalled(); unsubscribe(); done(); diff --git a/src/plugins/discover/public/application/main/services/discover_data_state_container.ts b/src/plugins/discover/public/application/main/services/discover_data_state_container.ts index e68218cb0865c..f243d9884ca9e 100644 --- a/src/plugins/discover/public/application/main/services/discover_data_state_container.ts +++ b/src/plugins/discover/public/application/main/services/discover_data_state_container.ts @@ -208,7 +208,9 @@ export function getDataStateContainer({ reset: val === 'reset', fetchMore: val === 'fetch_more', }, - searchSessionId: searchSessionManager.getNextSearchSessionId(), + searchSessionId: + (val === 'fetch_more' && searchSessionManager.getCurrentSearchSessionId()) || + searchSessionManager.getNextSearchSessionId(), })), share() ); diff --git a/src/plugins/discover/public/application/main/services/discover_search_session.ts b/src/plugins/discover/public/application/main/services/discover_search_session.ts index c6c64ed89c10a..c7379e706cf71 100644 --- a/src/plugins/discover/public/application/main/services/discover_search_session.ts +++ b/src/plugins/discover/public/application/main/services/discover_search_session.ts @@ -70,6 +70,13 @@ export class DiscoverSearchSessionManager { return searchSessionIdFromURL ?? this.deps.session.start(); } + /** + * Get current search session id + */ + getCurrentSearchSessionId() { + return this.deps.session.getSessionId(); + } + /** * Removes Discovers {@link SEARCH_SESSION_ID_QUERY_PARAM} from the URL * @param replace - methods to change the URL From 6c5e60c0b7f0bbedbc215a6f14e50e41d70083c2 Mon Sep 17 00:00:00 2001 From: Julia Rechkunova Date: Wed, 16 Aug 2023 10:08:11 +0200 Subject: [PATCH 47/49] [Discover] Don't send search session id for "Load more" requests --- .../discover/public/application/main/utils/fetch_documents.ts | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/src/plugins/discover/public/application/main/utils/fetch_documents.ts b/src/plugins/discover/public/application/main/utils/fetch_documents.ts index b75f8ce7b583f..767163259304f 100644 --- a/src/plugins/discover/public/application/main/utils/fetch_documents.ts +++ b/src/plugins/discover/public/application/main/utils/fetch_documents.ts @@ -45,7 +45,7 @@ export const fetchDocuments = ( const fetch$ = searchSource .fetch$({ abortSignal: abortController.signal, - sessionId: searchSessionId, + sessionId: isFetchingMore ? undefined : searchSessionId, inspector: { adapter: inspectorAdapters.requests, title: isFetchingMore // TODO: show it as a separate request in Inspect flyout From 37a609522ece6d33b75bb527321628a5a1b01616 Mon Sep 17 00:00:00 2001 From: Julia Rechkunova Date: Wed, 16 Aug 2023 10:53:21 +0200 Subject: [PATCH 48/49] [Discover] Refactor the default sort dir argument --- .../get_sort_for_search_source.test.ts | 40 ++++++------ .../sorting/get_sort_for_search_source.ts | 8 +-- .../main/utils/update_search_source.ts | 9 ++- .../embeddable/saved_search_embeddable.tsx | 5 +- .../utils/update_search_source.test.ts | 64 ++++++++++++------- .../embeddable/utils/update_search_source.ts | 14 ++-- .../discover/public/utils/get_sharing_data.ts | 8 ++- .../locator/searchsource_from_locator.ts | 8 +-- 8 files changed, 91 insertions(+), 65 deletions(-) diff --git a/src/plugins/discover/common/utils/sorting/get_sort_for_search_source.test.ts b/src/plugins/discover/common/utils/sorting/get_sort_for_search_source.test.ts index 6e5fbab6bbc39..415cf7ca9e76e 100644 --- a/src/plugins/discover/common/utils/sorting/get_sort_for_search_source.test.ts +++ b/src/plugins/discover/common/utils/sorting/get_sort_for_search_source.test.ts @@ -6,20 +6,10 @@ * Side Public License, v 1. */ import type { SortOrder } from '@kbn/saved-search-plugin/public'; -import { SORT_DEFAULT_ORDER_SETTING } from '@kbn/discover-utils'; import { getSortForSearchSource } from './get_sort_for_search_source'; import { stubDataView, stubDataViewWithoutTimeField } from '@kbn/data-plugin/common/stubs'; -import { coreMock } from '@kbn/core/public/mocks'; describe('getSortForSearchSource function', function () { - const core = coreMock.createStart(); - const uiSettings = core.uiSettings; - - const uiSettingWithAscSorting = coreMock.createStart().uiSettings; - jest - .spyOn(uiSettingWithAscSorting, 'get') - .mockImplementation((key) => (key === SORT_DEFAULT_ORDER_SETTING ? 'asc' : null)); - test('should be a function', function () { expect(typeof getSortForSearchSource === 'function').toBeTruthy(); }); @@ -30,7 +20,7 @@ describe('getSortForSearchSource function', function () { getSortForSearchSource({ sort: cols, dataView: stubDataView, - uiSettings, + defaultSortDir: 'desc', includeTieBreaker: true, }) ).toEqual([{ bytes: 'desc' }, { _doc: 'desc' }]); @@ -39,7 +29,7 @@ describe('getSortForSearchSource function', function () { getSortForSearchSource({ sort: cols, dataView: stubDataView, - uiSettings: uiSettingWithAscSorting, + defaultSortDir: 'asc', includeTieBreaker: true, }) ).toEqual([{ bytes: 'desc' }, { _doc: 'desc' }]); @@ -48,7 +38,7 @@ describe('getSortForSearchSource function', function () { getSortForSearchSource({ sort: cols, dataView: stubDataView, - uiSettings: uiSettingWithAscSorting, + defaultSortDir: 'asc', }) ).toEqual([{ bytes: 'desc' }]); @@ -56,7 +46,7 @@ describe('getSortForSearchSource function', function () { getSortForSearchSource({ sort: cols, dataView: stubDataViewWithoutTimeField, - uiSettings, + defaultSortDir: 'desc', includeTieBreaker: true, }) ).toEqual([{ bytes: 'desc' }]); @@ -65,32 +55,40 @@ describe('getSortForSearchSource function', function () { getSortForSearchSource({ sort: cols, dataView: stubDataViewWithoutTimeField, - uiSettings: uiSettingWithAscSorting, + defaultSortDir: 'asc', }) ).toEqual([{ bytes: 'desc' }]); }); test('should return an object to use for searchSource when no columns are given', function () { const cols = [] as SortOrder[]; - expect(getSortForSearchSource({ sort: cols, dataView: stubDataView, uiSettings })).toEqual([ - { _doc: 'desc' }, - ]); expect( getSortForSearchSource({ sort: cols, dataView: stubDataView, - uiSettings: uiSettingWithAscSorting, + defaultSortDir: 'desc', + }) + ).toEqual([{ _doc: 'desc' }]); + expect( + getSortForSearchSource({ + sort: cols, + dataView: stubDataView, + defaultSortDir: 'asc', }) ).toEqual([{ _doc: 'asc' }]); expect( - getSortForSearchSource({ sort: cols, dataView: stubDataViewWithoutTimeField, uiSettings }) + getSortForSearchSource({ + sort: cols, + dataView: stubDataViewWithoutTimeField, + defaultSortDir: 'desc', + }) ).toEqual([{ _score: 'desc' }]); expect( getSortForSearchSource({ sort: cols, dataView: stubDataViewWithoutTimeField, - uiSettings: uiSettingWithAscSorting, + defaultSortDir: 'asc', }) ).toEqual([{ _score: 'asc' }]); }); diff --git a/src/plugins/discover/common/utils/sorting/get_sort_for_search_source.ts b/src/plugins/discover/common/utils/sorting/get_sort_for_search_source.ts index 965bd7f8fec73..efd6c8b29cd30 100644 --- a/src/plugins/discover/common/utils/sorting/get_sort_for_search_source.ts +++ b/src/plugins/discover/common/utils/sorting/get_sort_for_search_source.ts @@ -9,8 +9,6 @@ import type { DataView } from '@kbn/data-views-plugin/common'; import type { EsQuerySortValue, SortDirection } from '@kbn/data-plugin/common'; import type { SortOrder } from '@kbn/saved-search-plugin/public'; -import type { IUiSettingsClient } from '@kbn/core-ui-settings-browser'; -import { SORT_DEFAULT_ORDER_SETTING } from '@kbn/discover-utils'; import { getSort } from './get_sort'; import { getESQuerySortForTimeField, @@ -28,15 +26,15 @@ import { export function getSortForSearchSource({ sort, dataView, - uiSettings, + defaultSortDir, includeTieBreaker = false, }: { sort: SortOrder[] | undefined; dataView: DataView | undefined; - uiSettings: Pick; + defaultSortDir: string; includeTieBreaker?: boolean; }): EsQuerySortValue[] { - const defaultDirection = uiSettings.get(SORT_DEFAULT_ORDER_SETTING) || 'desc'; + const defaultDirection = defaultSortDir || 'desc'; if (!sort || !dataView || (Array.isArray(sort) && sort.length === 0)) { if (dataView?.timeFieldName) { diff --git a/src/plugins/discover/public/application/main/utils/update_search_source.ts b/src/plugins/discover/public/application/main/utils/update_search_source.ts index c5556ab73ab5e..64c5e1586bdc4 100644 --- a/src/plugins/discover/public/application/main/utils/update_search_source.ts +++ b/src/plugins/discover/public/application/main/utils/update_search_source.ts @@ -10,7 +10,7 @@ import { ISearchSource } from '@kbn/data-plugin/public'; import { DataViewType, DataView } from '@kbn/data-views-plugin/public'; import { Filter } from '@kbn/es-query'; import type { SortOrder } from '@kbn/saved-search-plugin/public'; -import { SEARCH_FIELDS_FROM_SOURCE } from '@kbn/discover-utils'; +import { SEARCH_FIELDS_FROM_SOURCE, SORT_DEFAULT_ORDER_SETTING } from '@kbn/discover-utils'; import { DiscoverServices } from '../../../build_services'; import { getSortForSearchSource } from '../../../utils/sorting'; @@ -34,7 +34,12 @@ export function updateVolatileSearchSource( const { uiSettings, data } = services; const useNewFieldsApi = !uiSettings.get(SEARCH_FIELDS_FROM_SOURCE); - const usedSort = getSortForSearchSource({ sort, dataView, uiSettings, includeTieBreaker: true }); + const usedSort = getSortForSearchSource({ + sort, + dataView, + defaultSortDir: uiSettings.get(SORT_DEFAULT_ORDER_SETTING), + includeTieBreaker: true, + }); searchSource.setField('sort', usedSort); searchSource.setField('trackTotalHits', true); diff --git a/src/plugins/discover/public/embeddable/saved_search_embeddable.tsx b/src/plugins/discover/public/embeddable/saved_search_embeddable.tsx index cfdba5d56b3eb..b3be230355e5d 100644 --- a/src/plugins/discover/public/embeddable/saved_search_embeddable.tsx +++ b/src/plugins/discover/public/embeddable/saved_search_embeddable.tsx @@ -56,6 +56,7 @@ import { SAMPLE_SIZE_SETTING, SEARCH_FIELDS_FROM_SOURCE, SHOW_FIELD_STATISTICS, + SORT_DEFAULT_ORDER_SETTING, buildDataTableRecord, } from '@kbn/discover-utils'; import { VIEW_MODE, DISABLE_SHARD_FAILURE_WARNING } from '../../common/constants'; @@ -275,8 +276,8 @@ export class SavedSearchEmbeddable useNewFieldsApi, { sampleSize: this.services.uiSettings.get(SAMPLE_SIZE_SETTING), - }, - this.services.uiSettings + sortDir: this.services.uiSettings.get(SORT_DEFAULT_ORDER_SETTING), + } ); // Log request to inspector diff --git a/src/plugins/discover/public/embeddable/utils/update_search_source.test.ts b/src/plugins/discover/public/embeddable/utils/update_search_source.test.ts index 9e291feabaa01..6d440d89cf413 100644 --- a/src/plugins/discover/public/embeddable/utils/update_search_source.test.ts +++ b/src/plugins/discover/public/embeddable/utils/update_search_source.test.ts @@ -7,31 +7,28 @@ */ import { createSearchSourceMock } from '@kbn/data-plugin/common/search/search_source/mocks'; import { updateSearchSource } from './update_search_source'; -import { dataViewMock } from '@kbn/discover-utils/src/__mocks__'; +import { + buildDataViewMock, + dataViewMock, + shallowMockedFields, +} from '@kbn/discover-utils/src/__mocks__'; import type { SortOrder } from '@kbn/saved-search-plugin/public'; -import { coreMock } from '@kbn/core/public/mocks'; -import { SORT_DEFAULT_ORDER_SETTING } from '@kbn/discover-utils'; -const uiSettingWithAscSorting = coreMock.createStart().uiSettings; -jest - .spyOn(uiSettingWithAscSorting, 'get') - .mockImplementation((key) => (key === SORT_DEFAULT_ORDER_SETTING ? 'asc' : null)); +const dataViewMockWithTimeField = buildDataViewMock({ + name: 'the-data-view', + fields: shallowMockedFields, + timeFieldName: '@timestamp', +}); describe('updateSearchSource', () => { const defaults = { sampleSize: 50, + sortDir: 'asc', }; it('updates a given search source', async () => { const searchSource = createSearchSourceMock({}); - updateSearchSource( - searchSource, - dataViewMock, - [] as SortOrder[], - false, - defaults, - uiSettingWithAscSorting - ); + updateSearchSource(searchSource, dataViewMock, [] as SortOrder[], false, defaults); expect(searchSource.getField('fields')).toBe(undefined); // does not explicitly request fieldsFromSource when not using fields API expect(searchSource.getField('fieldsFromSource')).toBe(undefined); @@ -39,15 +36,38 @@ describe('updateSearchSource', () => { it('updates a given search source with the usage of the new fields api', async () => { const searchSource = createSearchSourceMock({}); + updateSearchSource(searchSource, dataViewMock, [] as SortOrder[], true, defaults); + expect(searchSource.getField('fields')).toEqual([{ field: '*', include_unmapped: 'true' }]); + expect(searchSource.getField('fieldsFromSource')).toBe(undefined); + }); + + it('updates a given search source with sort field', async () => { + const searchSource1 = createSearchSourceMock({}); + updateSearchSource(searchSource1, dataViewMock, [] as SortOrder[], true, defaults); + expect(searchSource1.getField('sort')).toEqual([{ _score: 'asc' }]); + + const searchSource2 = createSearchSourceMock({}); + updateSearchSource(searchSource2, dataViewMockWithTimeField, [] as SortOrder[], true, { + sampleSize: 50, + sortDir: 'desc', + }); + expect(searchSource2.getField('sort')).toEqual([{ _doc: 'desc' }]); + + const searchSource3 = createSearchSourceMock({}); updateSearchSource( - searchSource, - dataViewMock, - [] as SortOrder[], + searchSource3, + dataViewMockWithTimeField, + [['bytes', 'desc']] as SortOrder[], true, - defaults, - uiSettingWithAscSorting + defaults ); - expect(searchSource.getField('fields')).toEqual([{ field: '*', include_unmapped: 'true' }]); - expect(searchSource.getField('fieldsFromSource')).toBe(undefined); + expect(searchSource3.getField('sort')).toEqual([ + { + bytes: 'desc', + }, + { + _doc: 'desc', + }, + ]); }); }); diff --git a/src/plugins/discover/public/embeddable/utils/update_search_source.ts b/src/plugins/discover/public/embeddable/utils/update_search_source.ts index d3885ee210f9c..0215a26e649b0 100644 --- a/src/plugins/discover/public/embeddable/utils/update_search_source.ts +++ b/src/plugins/discover/public/embeddable/utils/update_search_source.ts @@ -8,7 +8,6 @@ import type { DataView } from '@kbn/data-views-plugin/public'; import type { ISearchSource } from '@kbn/data-plugin/public'; import type { SortOrder } from '@kbn/saved-search-plugin/public'; -import type { IUiSettingsClient } from '@kbn/core-ui-settings-browser'; import { getSortForSearchSource } from '../../utils/sorting'; export const updateSearchSource = ( @@ -18,14 +17,19 @@ export const updateSearchSource = ( useNewFieldsApi: boolean, defaults: { sampleSize: number; - }, - uiSettings: IUiSettingsClient + sortDir: string; + } ) => { - const { sampleSize } = defaults; + const { sampleSize, sortDir } = defaults; searchSource.setField('size', sampleSize); searchSource.setField( 'sort', - getSortForSearchSource({ sort, dataView, uiSettings, includeTieBreaker: true }) + getSortForSearchSource({ + sort, + dataView, + defaultSortDir: sortDir, + includeTieBreaker: true, + }) ); if (useNewFieldsApi) { searchSource.removeField('fieldsFromSource'); diff --git a/src/plugins/discover/public/utils/get_sharing_data.ts b/src/plugins/discover/public/utils/get_sharing_data.ts index 89a161416189d..9074d9d681f7e 100644 --- a/src/plugins/discover/public/utils/get_sharing_data.ts +++ b/src/plugins/discover/public/utils/get_sharing_data.ts @@ -15,7 +15,11 @@ import type { } from '@kbn/data-plugin/public'; import type { Filter } from '@kbn/es-query'; import type { SavedSearch, SortOrder } from '@kbn/saved-search-plugin/public'; -import { DOC_HIDE_TIME_COLUMN_SETTING, SEARCH_FIELDS_FROM_SOURCE } from '@kbn/discover-utils'; +import { + DOC_HIDE_TIME_COLUMN_SETTING, + SEARCH_FIELDS_FROM_SOURCE, + SORT_DEFAULT_ORDER_SETTING, +} from '@kbn/discover-utils'; import { DiscoverAppState, isEqualFilters, @@ -41,7 +45,7 @@ export async function getSharingData( getSortForSearchSource({ sort: state.sort as SortOrder[], dataView: index, - uiSettings, + defaultSortDir: uiSettings.get(SORT_DEFAULT_ORDER_SETTING), }) ); diff --git a/src/plugins/discover/server/locator/searchsource_from_locator.ts b/src/plugins/discover/server/locator/searchsource_from_locator.ts index 9d9f1e924fe01..70a723ddb5c54 100644 --- a/src/plugins/discover/server/locator/searchsource_from_locator.ts +++ b/src/plugins/discover/server/locator/searchsource_from_locator.ts @@ -148,16 +148,12 @@ export function searchSourceFromLocatorFactory(services: LocatorServicesDeps) { // Inject sort if (savedSearch.sort) { - const defaultSortingSetting = await services.uiSettings.get(SORT_DEFAULT_ORDER_SETTING); - const uiSettingsSyncReplacement = { - get: (key: string) => - key === SORT_DEFAULT_ORDER_SETTING ? defaultSortingSetting : undefined, - }; + const defaultSortDir = await services.uiSettings.get(SORT_DEFAULT_ORDER_SETTING); const sort = getSortForSearchSource({ sort: savedSearch.sort as Array<[string, string]>, dataView: index, - uiSettings: uiSettingsSyncReplacement, + defaultSortDir, }); searchSource.setField('sort', sort); } From cad0221016b9963511550418bf125c67820d7615 Mon Sep 17 00:00:00 2001 From: Julia Rechkunova Date: Thu, 17 Aug 2023 10:53:45 +0200 Subject: [PATCH 49/49] [Discover] Add a test for search session id --- .../main/utils/fetch_documents.test.ts | 44 +++++++++++++++++++ 1 file changed, 44 insertions(+) diff --git a/src/plugins/discover/public/application/main/utils/fetch_documents.test.ts b/src/plugins/discover/public/application/main/utils/fetch_documents.test.ts index e8ae34aacd917..f850b283b3491 100644 --- a/src/plugins/discover/public/application/main/utils/fetch_documents.test.ts +++ b/src/plugins/discover/public/application/main/utils/fetch_documents.test.ts @@ -16,6 +16,7 @@ import { FetchDeps } from './fetch_all'; import type { EsHitRecord } from '@kbn/discover-utils/types'; import { buildDataTableRecord } from '@kbn/discover-utils'; import { dataViewMock } from '@kbn/discover-utils/src/__mocks__'; +import { createSearchSourceMock } from '@kbn/data-plugin/common/search/search_source/mocks'; const getDeps = () => ({ @@ -48,4 +49,47 @@ describe('test fetchDocuments', () => { new Error('Oh noes!') ); }); + + test('passes a correct session id', async () => { + const deps = getDeps(); + const hits = [ + { _id: '1', foo: 'bar' }, + { _id: '2', foo: 'baz' }, + ] as unknown as EsHitRecord[]; + const documents = hits.map((hit) => buildDataTableRecord(hit, dataViewMock)); + + // regular search source + + const searchSourceRegular = createSearchSourceMock({ index: dataViewMock }); + searchSourceRegular.fetch$ = () => + of({ rawResponse: { hits: { hits } } } as IKibanaSearchResponse>); + + jest.spyOn(searchSourceRegular, 'fetch$'); + + expect(fetchDocuments(searchSourceRegular, deps)).resolves.toEqual({ + records: documents, + }); + + expect(searchSourceRegular.fetch$ as jest.Mock).toHaveBeenCalledWith( + expect.objectContaining({ sessionId: deps.searchSessionId }) + ); + + // search source with `search_after` for "Load more" requests + + const searchSourceForLoadMore = createSearchSourceMock({ index: dataViewMock }); + searchSourceForLoadMore.setField('searchAfter', ['100']); + + searchSourceForLoadMore.fetch$ = () => + of({ rawResponse: { hits: { hits } } } as IKibanaSearchResponse>); + + jest.spyOn(searchSourceForLoadMore, 'fetch$'); + + expect(fetchDocuments(searchSourceForLoadMore, deps)).resolves.toEqual({ + records: documents, + }); + + expect(searchSourceForLoadMore.fetch$ as jest.Mock).toHaveBeenCalledWith( + expect.objectContaining({ sessionId: undefined }) + ); + }); });