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

Cancel discarded KQL value suggestion requests #51411

Merged
merged 9 commits into from
Dec 9, 2019
Original file line number Diff line number Diff line change
Expand Up @@ -18,6 +18,7 @@
*/

import { get, map } from 'lodash';
import { abortableRequestHandler } from '../../../../../elasticsearch/lib/abortable_request_handler';

export function registerValueSuggestions(server) {
const serverConfig = server.config();
Expand All @@ -26,7 +27,7 @@ export function registerValueSuggestions(server) {
server.route({
path: '/api/kibana/suggestions/values/{index}',
method: ['POST'],
handler: async function (req) {
handler: abortableRequestHandler(async function (signal, req) {
const { index } = req.params;
const { field: fieldName, query, boolFilter } = req.payload;
const { callWithRequest } = server.plugins.elasticsearch.getCluster('data');
Expand All @@ -46,7 +47,7 @@ export function registerValueSuggestions(server) {
);

try {
const response = await callWithRequest(req, 'search', { index, body });
const response = await callWithRequest(req, 'search', { index, body }, { signal });
const buckets = get(response, 'aggregations.suggestions.buckets')
|| get(response, 'aggregations.nestedSuggestions.suggestions.buckets')
|| [];
Expand All @@ -55,7 +56,7 @@ export function registerValueSuggestions(server) {
} catch (error) {
throw server.plugins.elasticsearch.handleESError(error);
}
},
}),
});
}

Expand Down
1 change: 1 addition & 0 deletions src/plugins/data/public/autocomplete_provider/types.ts
Original file line number Diff line number Diff line change
Expand Up @@ -40,6 +40,7 @@ export type GetSuggestions = (args: {
query: string;
selectionStart: number;
selectionEnd: number;
signal?: AbortSignal;
}) => Promise<AutocompleteSuggestion[]>;

/** @public **/
Expand Down
19 changes: 16 additions & 3 deletions src/plugins/data/public/suggestions_provider/value_suggestions.ts
Original file line number Diff line number Diff line change
Expand Up @@ -28,23 +28,36 @@ export function getSuggestionsProvider(
http: HttpServiceBase
): IGetSuggestions {
const requestSuggestions = memoize(
(index: string, field: IFieldType, query: string, boolFilter: any = []) => {
(
index: string,
field: IFieldType,
query: string,
boolFilter: any = [],
signal?: AbortSignal
) => {
return http.fetch(`/api/kibana/suggestions/values/${index}`, {
method: 'POST',
body: JSON.stringify({ query, field: field.name, boolFilter }),
signal,
});
},
resolver
);

return async (index: string, field: IFieldType, query: string, boolFilter?: any) => {
return async (
index: string,
field: IFieldType,
query: string,
boolFilter?: any,
signal?: AbortSignal
) => {
const shouldSuggestValues = uiSettings.get('filterEditor:suggestValues');
if (field.type === 'boolean') {
return [true, false];
} else if (!shouldSuggestValues || !field.aggregatable || field.type !== 'string') {
return [];
}
return await requestSuggestions(index, field, query, boolFilter);
return await requestSuggestions(index, field, query, boolFilter, signal);
};
}

Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -106,6 +106,7 @@ export class QueryStringInputUI extends Component<Props, State> {
public inputRef: HTMLInputElement | null = null;

private persistedLog: PersistedLog | undefined;
private abortController: AbortController | undefined;
private services = this.props.kibana.services;
private componentIsUnmounting = false;

Expand Down Expand Up @@ -163,12 +164,22 @@ export class QueryStringInputUI extends Component<Props, State> {
return;
}

const suggestions: AutocompleteSuggestion[] = await getAutocompleteSuggestions({
query: queryString,
selectionStart,
selectionEnd,
});
return [...suggestions, ...recentSearchSuggestions];
try {
if (this.abortController) this.abortController.abort();
this.abortController = new AbortController();
const suggestions: AutocompleteSuggestion[] = await getAutocompleteSuggestions({
query: queryString,
selectionStart,
selectionEnd,
signal: this.abortController.signal,
});
return [...suggestions, ...recentSearchSuggestions];
} catch (e) {
// TODO: Waiting on https://github.com/elastic/kibana/issues/51406 for a properly typed error
// Ignore aborted requests
if (e.message === 'The user aborted a request.') return;
throw e;
}
};

private getRecentSearchSuggestions = (query: string) => {
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -22,7 +22,7 @@ export const kueryProvider = ({ config, indexPatterns, boolFilter }) => {
return provider({ config, indexPatterns, boolFilter });
});

return function getSuggestions({ query, selectionStart, selectionEnd }) {
return function getSuggestions({ query, selectionStart, selectionEnd, signal }) {
Copy link
Contributor

Choose a reason for hiding this comment

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

// nit
Can\will the signal be used for anything but aborting?
If not, maybe call it abortSignal?

const cursoredQuery = `${query.substr(0, selectionStart)}${cursorSymbol}${query.substr(selectionEnd)}`;

let cursorNode;
Expand All @@ -34,7 +34,7 @@ export const kueryProvider = ({ config, indexPatterns, boolFilter }) => {

const { suggestionTypes = [] } = cursorNode;
const suggestionsByType = suggestionTypes.map(type => {
return getSuggestionsByType[type](cursorNode);
return getSuggestionsByType[type](cursorNode, signal);
});
return Promise.all(suggestionsByType)
.then(suggestionsByType => dedup(flatten(suggestionsByType)));
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -27,14 +27,14 @@ export function getSuggestionsProvider({ indexPatterns, boolFilter }) {
suffix,
fieldName,
nestedPath,
}) {
}, signal) {
const fullFieldName = nestedPath ? `${nestedPath}.${fieldName}` : fieldName;
const fields = allFields.filter(field => field.name === fullFieldName);
const query = `${prefix}${suffix}`.trim();
const { getSuggestions } = npStart.plugins.data;

const suggestionsByField = fields.map(field => {
return getSuggestions(field.indexPatternTitle, field, query, boolFilter).then(data => {
return getSuggestions(field.indexPatternTitle, field, query, boolFilter, signal).then(data => {
const quotedValues = data.map(value => typeof value === 'string' ? `"${escapeQuotes(value)}"` : `${value}`);
return wrapAsSuggestions(start, end, query, quotedValues);
});
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -112,7 +112,7 @@ describe('Kuery value suggestions', function () {
const spy = jest.spyOn(npStart.plugins.data, 'getSuggestions');
await getSuggestions({ fieldName, prefix, suffix });
expect(spy).toHaveBeenCalledTimes(1);
expect(spy).toBeCalledWith(expect.any(String), expect.any(Object), prefix + suffix, undefined);
expect(spy).toBeCalledWith(expect.any(String), expect.any(Object), prefix + suffix, undefined, undefined);
Copy link
Contributor

Choose a reason for hiding this comment

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

Maybe add a test where signal is defined as well?

});

test('should escape quotes in suggestions', async () => {
Expand Down