Skip to content
New issue

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

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

Already on GitHub? Sign in to your account

[Search Source] Fix retrieval of unmapped fields; Add field filters #89837

Merged
merged 20 commits into from
Feb 15, 2021
Merged
Show file tree
Hide file tree
Changes from 17 commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
69 changes: 69 additions & 0 deletions src/plugins/data/common/search/search_source/search_source.test.ts
Original file line number Diff line number Diff line change
Expand Up @@ -28,6 +28,7 @@ const mockSource2 = { excludes: ['bar-*'] };

const indexPattern = ({
title: 'foo',
fields: [{ name: 'foo-bar' }, { name: 'field1' }, { name: 'field2' }],
getComputedFields,
getSourceFiltering: () => mockSource,
} as unknown) as IndexPattern;
Expand Down Expand Up @@ -518,6 +519,54 @@ describe('SearchSource', () => {
expect(request.script_fields).toEqual({ hello: {} });
});

test('request all fields except the ones specified with source filters', async () => {
searchSource.setField('index', ({
...indexPattern,
getComputedFields: () => ({
storedFields: [],
scriptFields: [],
docvalueFields: [],
}),
} as unknown) as IndexPattern);
searchSource.setField('fields', ['hello', 'foo']);

const request = await searchSource.getSearchRequestBody();
expect(request.fields).toEqual(['hello']);
});

test('request all fields from index pattern except the ones specified with source filters', async () => {
searchSource.setField('index', ({
...indexPattern,
getComputedFields: () => ({
storedFields: [],
scriptFields: [],
docvalueFields: [],
}),
} as unknown) as IndexPattern);
searchSource.setField('fields', ['*']);

const request = await searchSource.getSearchRequestBody();
expect(request.fields).toEqual([{ field: 'field1' }, { field: 'field2' }]);
});

test('request all fields from index pattern except the ones specified with source filters with unmapped_fields option', async () => {
searchSource.setField('index', ({
...indexPattern,
getComputedFields: () => ({
storedFields: [],
scriptFields: [],
docvalueFields: [],
}),
} as unknown) as IndexPattern);
searchSource.setField('fields', [{ field: '*', include_unmapped: 'true' }]);

const request = await searchSource.getSearchRequestBody();
expect(request.fields).toEqual([
{ field: 'field1', include_unmapped: 'true' },
{ field: 'field2', include_unmapped: 'true' },
]);
});

test('returns all scripted fields when one fields entry is *', async () => {
searchSource.setField('index', ({
...indexPattern,
Expand Down Expand Up @@ -836,5 +885,25 @@ describe('SearchSource', () => {
expect(references[1].type).toEqual('index-pattern');
expect(JSON.parse(searchSourceJSON).filter[0].meta.indexRefName).toEqual(references[1].name);
});

test('mvt geoshape layer test', async () => {
// @ts-expect-error TS won't like using this field name, but technically it's possible.
searchSource.setField('docvalue_fields', ['prop1']);
searchSource.setField('source', ['geometry']);
searchSource.setField('fieldsFromSource', ['geometry', 'prop1']);
searchSource.setField('index', ({
...indexPattern,
getSourceFiltering: () => ({ excludes: [] }),
getComputedFields: () => ({
storedFields: ['*'],
scriptFields: {},
docvalueFields: [],
}),
} as unknown) as IndexPattern);
const request = await searchSource.getSearchRequestBody();
expect(request.stored_fields).toEqual(['geometry', 'prop1']);
expect(request.docvalue_fields).toEqual(['prop1']);
expect(request._source).toEqual(['geometry']);
});
});
});
84 changes: 72 additions & 12 deletions src/plugins/data/common/search/search_source/search_source.ts
Original file line number Diff line number Diff line change
Expand Up @@ -59,12 +59,13 @@
*/

import { setWith } from '@elastic/safer-lodash-set';
majagrubic marked this conversation as resolved.
Show resolved Hide resolved
import { uniqueId, keyBy, pick, difference, omit, isObject, isFunction } from 'lodash';
import { uniqueId, keyBy, pick, difference, omit, isFunction, isEqual } from 'lodash';
import { map, switchMap, tap } from 'rxjs/operators';
import { defer, from } from 'rxjs';
import { isObject } from 'rxjs/internal-compatibility';
import { normalizeSortRequest } from './normalize_sort_request';
import { fieldWildcardFilter } from '../../../../kibana_utils/common';
import { IIndexPattern } from '../../index_patterns';
import { IIndexPattern, IndexPattern, IndexPatternField } from '../../index_patterns';
import { ISearchGeneric, ISearchOptions } from '../..';
import type {
ISearchSource,
Expand Down Expand Up @@ -99,6 +100,8 @@ export interface SearchSourceDependencies extends FetchHandlers {
search: ISearchGeneric;
}

const META_FIELDS = ['_type', '_source'];
majagrubic marked this conversation as resolved.
Show resolved Hide resolved

/** @public **/
export class SearchSource {
private id: string = uniqueId('data_source');
Expand Down Expand Up @@ -500,10 +503,63 @@ export class SearchSource {
}
}

private checkMatch(sourceFilters: string[], field: string) {
return sourceFilters.some((sourceFilter) => field.match(sourceFilter));
}

private getFieldsWithoutSourceFilters(
index: IndexPattern | undefined,
bodyFields: SearchFieldValue[]
) {
if (!index) {
return bodyFields;
}
const { fields } = index;
const sourceFilters = index.getSourceFiltering();
if (!sourceFilters || sourceFilters.excludes?.length === 0 || bodyFields.length === 0) {
return bodyFields;
}
const sourceFiltersValues = sourceFilters.excludes;
const wildcardField = bodyFields.find(
(el: SearchFieldValue) => el === '*' || (el as Record<string, string>).field === '*'
);
if (!wildcardField) {
// we already have an explicit list of fields, so we just remove source filters from that list
return bodyFields.filter((fld: SearchFieldValue) => {
if (typeof fld === 'string') {
return !this.checkMatch(sourceFiltersValues, fld) && !META_FIELDS.includes(fld);
}
if (!fld.hasOwnProperty('field')) {
return false;
}
const searchFieldValue = fld as Record<string, string>;
return (
!this.checkMatch(sourceFiltersValues, searchFieldValue.field) &&
!META_FIELDS.includes(searchFieldValue.field)
);
});
}
// we need to get the list of fields from an index pattern
const fieldsToInclude = fields.filter((field: IndexPatternField) => {
return !this.checkMatch(sourceFiltersValues, field.name) && !META_FIELDS.includes(field.name);
majagrubic marked this conversation as resolved.
Show resolved Hide resolved
});
return fieldsToInclude.map((fld: IndexPatternField) => {
const fieldToInclude: Record<string, string> = {
field: fld.name,
};
if (wildcardField && wildcardField.hasOwnProperty('include_unmapped')) {
fieldToInclude.include_unmapped = (wildcardField as Record<
string,
string
>).include_unmapped;
}
return fieldToInclude;
});
}

private flatten() {
const { getConfig } = this.dependencies;
const searchRequest = this.mergeProps();

searchRequest.body = searchRequest.body || {};
const { body, index, query, filters, highlightAll } = searchRequest;
searchRequest.indexType = this.getIndexType(index);
Expand Down Expand Up @@ -539,7 +595,7 @@ export class SearchSource {
if (!body.hasOwnProperty('_source')) {
body._source = sourceFilters;
}
if (body._source.excludes) {
if (body._source) {
Copy link
Member

Choose a reason for hiding this comment

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

This will always evaluate to true and can probably be removed. (If body._source is undefined, we assign it on L596 above, and index.getSourceFiltering() will always be truthy).

const filter = fieldWildcardFilter(
body._source.excludes,
getConfig(UI_SETTINGS.META_FIELDS)
Expand Down Expand Up @@ -579,17 +635,20 @@ export class SearchSource {
const remainingFields = difference(uniqFieldNames, [
...Object.keys(body.script_fields),
...Object.keys(body.runtime_mappings),
]).filter(Boolean);
]).filter((remainingField) => {
if (!remainingField) return false;
if (!body._source || !body._source.excludes) return true;
return !body._source.excludes.includes(remainingField);
});

// only include unique values
body.stored_fields = [...new Set(remainingFields)];

// only include unique values
if (fieldsFromSource.length) {
// include remaining fields in _source
setWith(body, '_source.includes', remainingFields, (nsValue) =>
isObject(nsValue) ? {} : nsValue
);

if (!isEqual(remainingFields, fieldsFromSource)) {
setWith(body, '_source.includes', remainingFields, (nsValue) =>
isObject(nsValue) ? {} : nsValue
);
}
// if items that are in the docvalueFields are provided, we should
// make sure those are added to the fields API unless they are
// already set in docvalue_fields
Expand All @@ -614,6 +673,7 @@ export class SearchSource {
// if items that are in the docvalueFields are provided, we should
// inject the format from the computed fields if one isn't given
const docvaluesIndex = keyBy(filteredDocvalueFields, 'field');
body.fields = this.getFieldsWithoutSourceFilters(index, body.fields);
Copy link
Member

@lukeelmers lukeelmers Feb 11, 2021

Choose a reason for hiding this comment

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

I haven't run this, but I'm wondering why the logic for getFieldsWithoutSourceFilters wouldn't be colocated with the other source filtering stuff starting on L599. They are doing very similar things in applying filters to body.fields, and it seems could be combined (unless there's a reason we need the "unfiltered" values between L599 and here?)

[Edit] Apologies, just realized I'm repeating myself and already mentioned it in the thread above. Been a long week! 😬

Copy link
Contributor Author

@majagrubic majagrubic Feb 11, 2021

Choose a reason for hiding this comment

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

It's been a long week for all of us, don't worry 😂 Thanks for the comments, will address this soon.

body.fields = body.fields.map((fld: SearchFieldValue) => {
majagrubic marked this conversation as resolved.
Show resolved Hide resolved
const fieldName = getFieldName(fld);
if (Object.keys(docvaluesIndex).includes(fieldName)) {
Expand Down
2 changes: 1 addition & 1 deletion x-pack/test/functional/apps/maps/mvt_scaling.js
Original file line number Diff line number Diff line change
Expand Up @@ -30,7 +30,7 @@ export default function ({ getPageObjects, getService }) {

//Source should be correct
expect(mapboxStyle.sources[VECTOR_SOURCE_ID].tiles[0]).to.equal(
'/api/maps/mvt/getTile?x={x}&y={y}&z={z}&geometryFieldName=geometry&index=geo_shapes*&requestBody=(_source:(includes:!(geometry,prop1)),docvalue_fields:!(prop1),query:(bool:(filter:!((match_all:())),must:!(),must_not:!(),should:!())),runtime_mappings:(),script_fields:(),size:10000,stored_fields:!(geometry,prop1))&geoFieldType=geo_shape'
'/api/maps/mvt/getTile?x={x}&y={y}&z={z}&geometryFieldName=geometry&index=geo_shapes*&requestBody=(_source:!(geometry),docvalue_fields:!(prop1),query:(bool:(filter:!((match_all:())),must:!(),must_not:!(),should:!())),runtime_mappings:(),script_fields:(),size:10000,stored_fields:!(geometry,prop1))&geoFieldType=geo_shape'
);

//Should correctly load meta for style-rule (sigma is set to 1, opacity to 1)
Expand Down