Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

[Reporting] Set Fields into the SearchSource in Discover's getSharingData #123412

Merged
merged 16 commits into from
Feb 8, 2022
Merged

[Reporting] Set Fields into the SearchSource in Discover's getSharingData #123412

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
16 commits
Select commit Hold shift + click to select a range
c32a3b7
[Reporting/CSV] set fields on the searchSource for querying performance
tsullivan Jan 24, 2022
2e13480
simplify generate_csv
tsullivan Jan 24, 2022
c090bab
update outdated comment
tsullivan Jan 24, 2022
10df2d3
fix generate_csv test
tsullivan Jan 25, 2022
005606b
simplify get_sharing_data
tsullivan Jan 25, 2022
b0bae75
fix api integration tests to match pre-existing snapshots
tsullivan Jan 25, 2022
a1b6963
Merge branch 'main' into fix/123187
kibanamachine Jan 26, 2022
8ed8d3a
Apply suggestions from code review
tsullivan Jan 27, 2022
8d0dfe3
use config in condition
tsullivan Jan 27, 2022
3124ef0
Merge remote-tracking branch 'elastic/main' into fix/123187
tsullivan Jan 27, 2022
84cb1cc
add more testing
tsullivan Jan 27, 2022
c3bca2c
test change clarify
tsullivan Jan 27, 2022
f446486
Merge branch 'main' into fix/123187
kibanamachine Jan 28, 2022
369c596
Merge branch 'main' into fix/123187
kibanamachine Feb 2, 2022
9223061
Merge remote-tracking branch 'elastic/main' into fix/123187
tsullivan Feb 2, 2022
bc72252
Merge branch 'main' into fix/123187
kibanamachine Feb 7, 2022
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
94 changes: 93 additions & 1 deletion src/plugins/discover/public/utils/get_sharing_data.test.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -11,7 +11,11 @@ import type { DataView } from 'src/plugins/data/common';
import type { DiscoverServices } from '../build_services';
import { dataPluginMock } from '../../../data/public/mocks';
import { createSearchSourceMock } from '../../../data/common/search/search_source/mocks';
import { DOC_HIDE_TIME_COLUMN_SETTING, SORT_DEFAULT_ORDER_SETTING } from '../../common';
import {
DOC_HIDE_TIME_COLUMN_SETTING,
SORT_DEFAULT_ORDER_SETTING,
SEARCH_FIELDS_FROM_SOURCE,
} from '../../common';
import { indexPatternMock } from '../__mocks__/index_pattern';
import { getSharingData, showPublicUrlSwitch } from './get_sharing_data';

Expand All @@ -23,6 +27,9 @@ describe('getSharingData', () => {
data: dataPluginMock.createStartContract(),
uiSettings: {
get: (key: string) => {
if (key === SEARCH_FIELDS_FROM_SOURCE) {
return false;
}
if (key === SORT_DEFAULT_ORDER_SETTING) {
return 'desc';
}
Expand Down Expand Up @@ -64,6 +71,91 @@ describe('getSharingData', () => {
`);
});

test('getSearchSource does not add fields to the searchSource', async () => {
const index = { ...indexPatternMock } as DataView;
index.timeFieldName = 'cool-timefield';
const searchSourceMock = createSearchSourceMock({ index });
const { getSearchSource } = await getSharingData(searchSourceMock, {}, services);
expect(getSearchSource()).toMatchInlineSnapshot(`
Object {
"index": "the-index-pattern-id",
"sort": Array [
Object {
"_doc": "desc",
},
],
}
`);
});

test(`getSearchSource does not add fields to the searchSource with 'discover:searchFieldsFromSource=true'`, async () => {
const originalGet = services.uiSettings.get;
services.uiSettings = {
get: (key: string, ...args: unknown[]) => {
if (key === SEARCH_FIELDS_FROM_SOURCE) {
return true;
}
return originalGet(key, ...args);
},
} as unknown as IUiSettingsClient;
const index = { ...indexPatternMock } as DataView;
index.timeFieldName = 'cool-timefield';
const searchSourceMock = createSearchSourceMock({ index });
const { getSearchSource } = await getSharingData(
searchSourceMock,
{
columns: [
'cool-field-1',
'cool-field-2',
'cool-field-3',
'cool-field-4',
'cool-field-5',
'cool-field-6',
],
},
services
);
expect(getSearchSource()).toMatchInlineSnapshot(`
Object {
"index": "the-index-pattern-id",
"sort": Array [
Object {
"_doc": "desc",
},
],
}
`);
});

test('getSearchSource does add fields to the searchSource when columns are selected', async () => {
const index = { ...indexPatternMock } as DataView;
index.timeFieldName = 'cool-timefield';
const searchSourceMock = createSearchSourceMock({ index });
const { getSearchSource } = await getSharingData(
searchSourceMock,
{
columns: [
'cool-field-1',
'cool-field-2',
'cool-field-3',
'cool-field-4',
'cool-field-5',
'cool-field-6',
],
},
services
);
expect(getSearchSource().fields).toStrictEqual([
'cool-timefield',
'cool-field-1',
'cool-field-2',
'cool-field-3',
'cool-field-4',
'cool-field-5',
'cool-field-6',
]);
});

test('fields have prepended timeField', async () => {
const index = { ...indexPatternMock } as DataView;
index.timeFieldName = 'cool-timefield';
Expand Down
15 changes: 14 additions & 1 deletion src/plugins/discover/public/utils/get_sharing_data.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -10,7 +10,11 @@ import type { Capabilities } from 'kibana/public';
import type { IUiSettingsClient } from 'kibana/public';
import type { DataPublicPluginStart } from 'src/plugins/data/public';
import type { Filter, ISearchSource, SerializedSearchSourceFields } from 'src/plugins/data/common';
import { DOC_HIDE_TIME_COLUMN_SETTING, SORT_DEFAULT_ORDER_SETTING } from '../../common';
import {
DOC_HIDE_TIME_COLUMN_SETTING,
SEARCH_FIELDS_FROM_SOURCE,
SORT_DEFAULT_ORDER_SETTING,
} from '../../common';
import type { SavedSearch, SortOrder } from '../services/saved_searches';
import { getSortForSearchSource } from '../components/doc_table';
import { AppState } from '../application/main/services/discover_state';
Expand Down Expand Up @@ -72,6 +76,15 @@ export async function getSharingData(
searchSource.setField('filter', filter);
}

/*
* For downstream querying performance, the searchSource object must have fields set.
* 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);
if (useFieldsApi && columns.length) {
searchSource.setField('fields', columns);
}
return searchSource.getSerializedFields(true);
},
columns,
Expand Down
6 changes: 1 addition & 5 deletions x-pack/plugins/reporting/public/panel_actions/get_csv_panel_action.tsx
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -75,11 +75,7 @@ export class ReportingCsvPanelAction implements ActionDefinition<ActionContext>
public async getSearchSource(savedSearch: SavedSearch, _embeddable: ISearchEmbeddable) {
const [{ uiSettings }, { data }] = await this.startServices$.pipe(first()).toPromise();
const { getSharingData } = await loadSharingDataHelpers();
return await getSharingData(
savedSearch.searchSource,
savedSearch, // TODO: get unsaved state (using embeddable.searchScope): https://github.com/elastic/kibana/issues/43977
Copy link
Member Author

Choose a reason for hiding this comment

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

outdated comment

{ uiSettings, data }
);
return await getSharingData(savedSearch.searchSource, savedSearch, { uiSettings, data });
}

public isCompatible = async (context: ActionContext) => {
Expand Down
6 changes: 3 additions & 3 deletions ...server/export_types/csv_searchsource/generate_csv/__snapshots__/generate_csv.test.ts.snap
View file
Open in desktop

Some generated files are not rendered by default. Learn more about how customized files appear on GitHub.

87 changes: 24 additions & 63 deletions .../plugins/reporting/server/export_types/csv_searchsource/generate_csv/generate_csv.test.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -69,21 +69,6 @@ const mockDataClientSearchDefault = jest.fn().mockImplementation(
},
})
);
const mockSearchSourceGetFieldDefault = jest.fn().mockImplementation((key: string) => {
switch (key) {
case 'fields':
return ['date', 'ip', 'message'];
Copy link
Member Author

Choose a reason for hiding this comment

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

This mock is not needed, as the Reporting code is no longer calling getField('fields') to get the column names.

case 'index':
return {
fields: {
getByName: jest.fn().mockImplementation(() => []),
getByType: jest.fn().mockImplementation(() => []),
},
metaFields: ['_id', '_index', '_type', '_score'],
getFormatterForField: jest.fn(),
};
}
});

const mockFieldFormatsRegistry = {
deserialize: jest
Expand Down Expand Up @@ -123,14 +108,26 @@ beforeEach(async () => {
})
);

searchSourceMock.getField = mockSearchSourceGetFieldDefault;
searchSourceMock.getField = jest.fn((key: string) => {
switch (key) {
case 'index':
return {
fields: {
getByName: jest.fn(() => []),
getByType: jest.fn(() => []),
},
metaFields: ['_id', '_index', '_type', '_score'],
getFormatterForField: jest.fn(),
};
}
});
});

const logger = createMockLevelLogger();

it('formats an empty search result to CSV content', async () => {
const generateCsv = new CsvGenerator(
createMockJob({}),
createMockJob({ columns: ['date', 'ip', 'message'] }),
Copy link
Member Author

@tsullivan tsullivan Jan 25, 2022 •
edited
Loading

Choose a reason for hiding this comment

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

All of the mock jobs in the test had to be updated to explicitly set the set of columns in the parameters. Previously, columns could be empty since it was assumed that the fields were set into the SearchSource in the browser.

This PR does set the fields into the SearchSource in the browser, but not for most use cases. If Reporting tries to get its CSV columns using searchSource.getField('fields'), most likely it will not get usable field names.

Existing POST URLs that were generated in 7.13+ should still yield the same results. The tests don't yield the same results, because they took shortcuts by assuming the set of fields could be retrieved from the searchSource.

mockConfig,
{
es: mockEsClient,
Expand Down Expand Up @@ -170,7 +167,7 @@ it('formats a search result to CSV content', async () => {
})
);
const generateCsv = new CsvGenerator(
createMockJob({}),
createMockJob({ columns: ['date', 'ip', 'message'] }),
mockConfig,
{
es: mockEsClient,
Expand All @@ -193,12 +190,6 @@ it('formats a search result to CSV content', async () => {
const HITS_TOTAL = 100;

it('calculates the bytes of the content', async () => {
searchSourceMock.getField = jest.fn().mockImplementation((key: string) => {
if (key === 'fields') {
return ['message'];
}
return mockSearchSourceGetFieldDefault(key);
});
mockDataClient.search = jest.fn().mockImplementation(() =>
Rx.of({
rawResponse: {
Expand All @@ -215,7 +206,7 @@ it('calculates the bytes of the content', async () => {
);

const generateCsv = new CsvGenerator(
createMockJob({}),
createMockJob({ columns: ['message'] }),
mockConfig,
{
es: mockEsClient,
Expand Down Expand Up @@ -267,7 +258,7 @@ it('warns if max size was reached', async () => {
);

const generateCsv = new CsvGenerator(
createMockJob({}),
createMockJob({ columns: ['date', 'ip', 'message'] }),
mockConfig,
{
es: mockEsClient,
Expand Down Expand Up @@ -321,7 +312,7 @@ it('uses the scrollId to page all the data', async () => {
});

const generateCsv = new CsvGenerator(
createMockJob({}),
createMockJob({ columns: ['date', 'ip', 'message'] }),
mockConfig,
{
es: mockEsClient,
Expand Down Expand Up @@ -361,12 +352,6 @@ it('uses the scrollId to page all the data', async () => {

describe('fields from job.searchSource.getFields() (7.12 generated)', () => {
it('cells can be multi-value', async () => {
searchSourceMock.getField = jest.fn().mockImplementation((key: string) => {
if (key === 'fields') {
return ['_id', 'sku'];
}
return mockSearchSourceGetFieldDefault(key);
});
mockDataClient.search = jest.fn().mockImplementation(() =>
Rx.of({
rawResponse: {
Expand All @@ -388,7 +373,7 @@ describe('fields from job.searchSource.getFields() (7.12 generated)', () => {
);

const generateCsv = new CsvGenerator(
createMockJob({ searchSource: {} }),
createMockJob({ searchSource: {}, columns: ['_id', 'sku'] }),
mockConfig,
{
es: mockEsClient,
Expand All @@ -409,12 +394,6 @@ describe('fields from job.searchSource.getFields() (7.12 generated)', () => {
});

it('provides top-level underscored fields as columns', async () => {
searchSourceMock.getField = jest.fn().mockImplementation((key: string) => {
if (key === 'fields') {
return ['_id', '_index', 'date', 'message'];
}
return mockSearchSourceGetFieldDefault(key);
});
mockDataClient.search = jest.fn().mockImplementation(() =>
Rx.of({
rawResponse: {
Expand Down Expand Up @@ -445,6 +424,7 @@ describe('fields from job.searchSource.getFields() (7.12 generated)', () => {
fields: ['_id', '_index', '@date', 'message'],
filter: [],
},
columns: ['_id', '_index', 'date', 'message'],
}),
mockConfig,
{
Expand All @@ -468,12 +448,6 @@ describe('fields from job.searchSource.getFields() (7.12 generated)', () => {
});

it('sorts the fields when they are to be used as table column names', async () => {
searchSourceMock.getField = jest.fn().mockImplementation((key: string) => {
if (key === 'fields') {
return ['*'];
}
return mockSearchSourceGetFieldDefault(key);
});
mockDataClient.search = jest.fn().mockImplementation(() =>
Rx.of({
rawResponse: {
Expand Down Expand Up @@ -620,13 +594,7 @@ describe('fields from job.columns (7.13+ generated)', () => {
expect(content).toMatchSnapshot();
});

it('empty columns defaults to using searchSource.getFields()', async () => {
searchSourceMock.getField = jest.fn().mockImplementation((key: string) => {
if (key === 'fields') {
return ['product'];
}
return mockSearchSourceGetFieldDefault(key);
});
it('default column names come from tabify', async () => {
mockDataClient.search = jest.fn().mockImplementation(() =>
Rx.of({
rawResponse: {
Expand Down Expand Up @@ -694,7 +662,7 @@ describe('formulas', () => {
);

const generateCsv = new CsvGenerator(
createMockJob({}),
createMockJob({ columns: ['date', 'ip', 'message'] }),
mockConfig,
{
es: mockEsClient,
Expand Down Expand Up @@ -736,15 +704,8 @@ describe('formulas', () => {
})
);

searchSourceMock.getField = jest.fn().mockImplementation((key: string) => {
if (key === 'fields') {
return ['date', 'ip', TEST_FORMULA];
}
return mockSearchSourceGetFieldDefault(key);
});

const generateCsv = new CsvGenerator(
createMockJob({}),
createMockJob({ columns: ['date', 'ip', TEST_FORMULA] }),
mockConfig,
{
es: mockEsClient,
Expand Down Expand Up @@ -797,7 +758,7 @@ describe('formulas', () => {
);

const generateCsv = new CsvGenerator(
createMockJob({}),
createMockJob({ columns: ['date', 'ip', 'message'] }),
mockConfig,
{
es: mockEsClient,
Expand Down
Loading