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

[Discover] Change default sort handling #85561

Merged
Merged
Show file tree
Hide file tree
Changes from 15 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
8 changes: 7 additions & 1 deletion src/plugins/discover/public/application/angular/discover.js
Original file line number Diff line number Diff line change
Expand Up @@ -65,11 +65,13 @@ import {
MODIFY_COLUMNS_ON_SWITCH,
SAMPLE_SIZE_SETTING,
SEARCH_ON_PAGE_LOAD_SETTING,
SORT_DEFAULT_ORDER_SETTING,
} from '../../../common';
import { loadIndexPattern, resolveIndexPattern } from '../helpers/resolve_index_pattern';
import { getTopNavLinks } from '../components/top_nav/get_top_nav_links';
import { updateSearchSource } from '../helpers/update_search_source';
import { calcFieldCounts } from '../helpers/calc_field_counts';
import { getDefaultSort } from './doc_table/lib/get_default_sort';

const services = getServices();

Expand Down Expand Up @@ -410,9 +412,13 @@ function discoverController($element, $route, $scope, $timeout, Promise, uiCapab

function getStateDefaults() {
const query = $scope.searchSource.getField('query') || data.query.queryString.getDefaultQuery();
const sort = getSortArray(savedSearch.sort, $scope.indexPattern);

return {
query,
sort: getSortArray(savedSearch.sort, $scope.indexPattern),
sort: !sort.length
? getDefaultSort($scope.indexPattern, config.get(SORT_DEFAULT_ORDER_SETTING, 'desc'))
: sort,
columns:
savedSearch.columns.length > 0
? savedSearch.columns
Expand Down
Original file line number Diff line number Diff line change
@@ -0,0 +1,43 @@
/*
* Licensed to Elasticsearch B.V. under one or more contributor
* license agreements. See the NOTICE file distributed with
* this work for additional information regarding copyright
* ownership. Elasticsearch B.V. licenses this file to you under
* the Apache License, Version 2.0 (the "License"); you may
* not use this file except in compliance with the License.
* You may obtain a copy of the License at
*
* http://www.apache.org/licenses/LICENSE-2.0
*
* Unless required by applicable law or agreed to in writing,
* software distributed under the License is distributed on an
* "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY
* KIND, either express or implied. See the License for the
* specific language governing permissions and limitations
* under the License.
*/
import { getDefaultSort } from './get_default_sort';
// @ts-ignore
import FixturesStubbedLogstashIndexPatternProvider from 'fixtures/stubbed_logstash_index_pattern';
import { IndexPattern } from '../../../../kibana_services';

describe('getDefaultSort function', function () {
let indexPattern: IndexPattern;
beforeEach(() => {
indexPattern = FixturesStubbedLogstashIndexPatternProvider() as IndexPattern;
});
test('should be a function', function () {
expect(typeof getDefaultSort === 'function').toBeTruthy();
});

test('should return default sort for an index pattern with timeFieldName', function () {
expect(getDefaultSort(indexPattern, 'desc')).toEqual([['time', 'desc']]);
expect(getDefaultSort(indexPattern, 'asc')).toEqual([['time', 'asc']]);
});

test('should return default sort for an index pattern without timeFieldName', function () {
delete indexPattern.timeFieldName;
expect(getDefaultSort(indexPattern, 'desc')).toEqual([]);
expect(getDefaultSort(indexPattern, 'asc')).toEqual([]);
});
});
Original file line number Diff line number Diff line change
Expand Up @@ -17,7 +17,6 @@
* under the License.
*/
import { IndexPattern } from '../../../../kibana_services';
// @ts-ignore
import { isSortable } from './get_sort';
import { SortOrder } from '../components/table_header/helpers';

Expand All @@ -26,12 +25,12 @@ import { SortOrder } from '../components/table_header/helpers';
* the default sort is returned depending of the index pattern
*/
export function getDefaultSort(
indexPattern: IndexPattern,
indexPattern: IndexPattern | undefined,
Copy link
Contributor

Choose a reason for hiding this comment

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

In what scenario would index pattern be undefined?

Copy link
Member Author

@kertal kertal Dec 18, 2020

Choose a reason for hiding this comment

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

It shouldn't. But when using this function in an embeddable, according the TypeScript it could be. So the decision was the edit in the function or in the embeddable. decided to modify here, since the function would work without an index pattern.

defaultSortOrder: string = 'desc'
): SortOrder[] {
if (indexPattern.timeFieldName && isSortable(indexPattern.timeFieldName, indexPattern)) {
if (indexPattern?.timeFieldName && isSortable(indexPattern.timeFieldName, indexPattern)) {
return [[indexPattern.timeFieldName, defaultSortOrder]];
} else {
return [['_score', defaultSortOrder]];
Copy link
Member Author

Choose a reason for hiding this comment

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

quick question @timroes , I don't think it makes much sense to sort by _score by default when it's an index pattern without timefield. this function now returna an empty default sorting, which would mean, that ES get's the info sort by _doc. I think that's a better choice here, would you agree?

Copy link
Member Author

Choose a reason for hiding this comment

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

@timroes update: changed the default to _score for index patterns without time field (so it is handled like before and nothing breaks)

return [];
}
}
Original file line number Diff line number Diff line change
@@ -0,0 +1,51 @@
/*
* Licensed to Elasticsearch B.V. under one or more contributor
* license agreements. See the NOTICE file distributed with
* this work for additional information regarding copyright
* ownership. Elasticsearch B.V. licenses this file to you under
* the Apache License, Version 2.0 (the "License"); you may
* not use this file except in compliance with the License.
* You may obtain a copy of the License at
*
* http://www.apache.org/licenses/LICENSE-2.0
*
* Unless required by applicable law or agreed to in writing,
* software distributed under the License is distributed on an
* "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY
* KIND, either express or implied. See the License for the
* specific language governing permissions and limitations
* under the License.
*/
import { getSortForSearchSource } from './get_sort_for_search_source';
// @ts-ignore
import FixturesStubbedLogstashIndexPatternProvider from 'fixtures/stubbed_logstash_index_pattern';
import { IndexPattern } from '../../../../kibana_services';
import { SortOrder } from '../components/table_header/helpers';

describe('getSortForSearchSource function', function () {
let indexPattern: IndexPattern;
beforeEach(() => {
indexPattern = FixturesStubbedLogstashIndexPatternProvider() as IndexPattern;
});
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, indexPattern)).toEqual([{ bytes: 'desc' }]);
expect(getSortForSearchSource(cols, indexPattern, 'asc')).toEqual([{ bytes: 'desc' }]);
delete indexPattern.timeFieldName;
expect(getSortForSearchSource(cols, indexPattern)).toEqual([{ bytes: 'desc' }]);
expect(getSortForSearchSource(cols, indexPattern, '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(cols, indexPattern)).toEqual([{ _doc: 'desc' }]);
expect(getSortForSearchSource(cols, indexPattern, 'asc')).toEqual([{ _doc: 'asc' }]);
delete indexPattern.timeFieldName;
expect(getSortForSearchSource(cols, indexPattern)).toEqual([{ _score: 'desc' }]);
expect(getSortForSearchSource(cols, indexPattern, 'asc')).toEqual([{ _score: 'asc' }]);
});
});
Original file line number Diff line number Diff line change
Expand Up @@ -19,7 +19,6 @@
import { EsQuerySortValue, IndexPattern } from '../../../../kibana_services';
import { SortOrder } from '../components/table_header/helpers';
import { getSort } from './get_sort';
import { getDefaultSort } from './get_default_sort';

/**
* Prepares sort for search source, that's sending the request to ES
Expand All @@ -33,10 +32,13 @@ export function getSortForSearchSource(
indexPattern?: IndexPattern,
defaultDirection: string = 'desc'
): EsQuerySortValue[] {
if (!sort || !indexPattern) {
return [];
} else if (Array.isArray(sort) && sort.length === 0) {
sort = getDefaultSort(indexPattern, defaultDirection);
if (!sort || !indexPattern || (Array.isArray(sort) && sort.length === 0)) {
if (indexPattern?.timeFieldName) {
// sorting by index order
return [{ _doc: defaultDirection } as EsQuerySortValue];
} else {
return [{ _score: defaultDirection } as EsQuerySortValue];
}
}
const { timeFieldName } = indexPattern;
return getSort(sort, indexPattern).map((sortPair: Record<string, string>) => {
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -48,6 +48,7 @@ import {
import { SEARCH_EMBEDDABLE_TYPE } from './constants';
import { SavedSearch } from '../..';
import { SAMPLE_SIZE_SETTING, SORT_DEFAULT_ORDER_SETTING } from '../../../common';
import { getDefaultSort } from '../angular/doc_table/lib/get_default_sort';

interface SearchScope extends ng.IScope {
columns?: string[];
Expand Down Expand Up @@ -200,6 +201,13 @@ export class SearchEmbeddable
const { searchSource } = this.savedSearch;
const indexPattern = (searchScope.indexPattern = searchSource.getField('index'))!;

if (!this.savedSearch.sort || !this.savedSearch.sort.length) {
this.savedSearch.sort = getDefaultSort(
indexPattern,
getServices().uiSettings.get(SORT_DEFAULT_ORDER_SETTING, 'desc')
);
}

const timeRangeSearchSource = searchSource.create();
timeRangeSearchSource.setField('filter', () => {
if (!this.searchScope || !this.input.timeRange) return;
Expand Down Expand Up @@ -341,7 +349,14 @@ export class SearchEmbeddable
// If there is column or sort data on the panel, that means the original columns or sort settings have
// been overridden in a dashboard.
searchScope.columns = this.input.columns || this.savedSearch.columns;
searchScope.sort = this.input.sort || this.savedSearch.sort;
const savedSearchSort =
this.savedSearch.sort && this.savedSearch.sort.length
? this.savedSearch.sort
: getDefaultSort(
this.searchScope?.indexPattern,
getServices().uiSettings.get(SORT_DEFAULT_ORDER_SETTING, 'desc')
);
searchScope.sort = this.input.sort || savedSearchSort;
searchScope.sharedItemTitle = this.panelTitle;

if (forceFetch || isFetchRequired) {
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -21,6 +21,7 @@ import { getSharingData } from './get_sharing_data';
import { IUiSettingsClient } from 'kibana/public';
import { createSearchSourceMock } from '../../../../data/common/search/search_source/mocks';
import { indexPatternMock } from '../../__mocks__/index_pattern';
import { SORT_DEFAULT_ORDER_SETTING } from '../../../common';

describe('getSharingData', () => {
test('returns valid data for sharing', async () => {
Expand All @@ -29,7 +30,10 @@ describe('getSharingData', () => {
searchSourceMock,
{ columns: [] },
({
get: () => {
get: (key: string) => {
if (key === SORT_DEFAULT_ORDER_SETTING) {
return 'desc';
}
return false;
},
} as unknown) as IUiSettingsClient,
Expand Down Expand Up @@ -57,7 +61,13 @@ describe('getSharingData', () => {
},
},
"script_fields": Object {},
"sort": Array [],
"sort": Array [
Object {
"_score": Object {
"order": "desc",
},
},
],
"stored_fields": undefined,
},
"index": "the-index-pattern-title",
Expand Down
2 changes: 1 addition & 1 deletion test/functional/apps/discover/_shared_links.js
Original file line number Diff line number Diff line change
Expand Up @@ -90,7 +90,7 @@ export default function ({ getService, getPageObjects }) {
":(from:'2015-09-19T06:31:44.000Z',to:'2015-09" +
"-23T18:31:44.000Z'))&_a=(columns:!(_source),filters:!(),index:'logstash-" +
"*',interval:auto,query:(language:kuery,query:'')" +
',sort:!())';
",sort:!(!('@timestamp',desc)))";
const actualUrl = await PageObjects.share.getSharedUrl();
// strip the timestamp out of each URL
expect(actualUrl.replace(/_t=\d{13}/, '_t=TIMESTAMP')).to.be(
Expand Down