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

Migrate legacy sort arrays on saved searches #43038

Merged
merged 8 commits into from
Aug 13, 2019
Merged
Show file tree
Hide file tree
Changes from all 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
23 changes: 23 additions & 0 deletions src/legacy/core_plugins/kibana/migrations/migrations.js
Original file line number Diff line number Diff line change
Expand Up @@ -365,6 +365,24 @@ function replaceMovAvgToMovFn(doc, logger) {
return doc;
}

function migrateSearchSortToNestedArray(doc) {
const sort = get(doc, 'attributes.sort');
if (!sort) return doc;

// Don't do anything if we already have a two dimensional array
if (Array.isArray(sort) && sort.length > 0 && Array.isArray(sort[0])) {
return doc;
}

return {
...doc,
attributes: {
...doc.attributes,
sort: [doc.attributes.sort],
}
};
}

const executeMigrations720 = flow(
migratePercentileRankAggregation,
migrateDateHistogramAggregation
Expand All @@ -376,6 +394,10 @@ const executeMigrations730 = flow(
replaceMovAvgToMovFn
);

const executeSearchMigrations740 = flow(
migrateSearchSortToNestedArray,
);

export const migrations = {
'index-pattern': {
'6.5.0': doc => {
Expand Down Expand Up @@ -530,5 +552,6 @@ export const migrations = {
migrateIndexPattern(doc);
return doc;
},
'7.4.0': executeSearchMigrations740,
},
};
52 changes: 52 additions & 0 deletions src/legacy/core_plugins/kibana/migrations/migrations.test.js
Original file line number Diff line number Diff line change
Expand Up @@ -1788,4 +1788,56 @@ Object {
/* eslint-enable max-len */
});
});

describe('7.4.0', function () {
const migration = migrations.search['7.4.0'];

test('transforms one dimensional sort arrays into two dimensional arrays', () => {
const doc = {
id: '123',
type: 'search',
attributes: {
sort: ['bytes', 'desc'],
},
};

const expected = {
id: '123',
type: 'search',
attributes: {
sort: [['bytes', 'desc']],
},
};

const migratedDoc = migration(doc);

expect(migratedDoc).toEqual(expected);
});

test('doesn\'t modify search docs that already have two dimensional sort arrays', () => {
const doc = {
id: '123',
type: 'search',
attributes: {
sort: [['bytes', 'desc']],
},
};

const migratedDoc = migration(doc);

expect(migratedDoc).toEqual(doc);
});

test('doesn\'t modify search docs that have no sort array', () => {
const doc = {
id: '123',
type: 'search',
attributes: {},
};

const migratedDoc = migration(doc);

expect(migratedDoc).toEqual(doc);
});
});
});
Original file line number Diff line number Diff line change
Expand Up @@ -38,11 +38,15 @@ describe('docTable', function () {
expect(getSort).to.be.a(Function);
});

it('should return an array of objects if passed a 2 item array', function () {
expect(getSort(['bytes', 'desc'], indexPattern)).to.eql([{ bytes: 'desc' }]);
it('should return an array of objects', function () {
expect(getSort([['bytes', 'desc']], indexPattern)).to.eql([{ bytes: 'desc' }]);

delete indexPattern.timeFieldName;
expect(getSort(['bytes', 'desc'], indexPattern)).to.eql([{ bytes: 'desc' }]);
expect(getSort([['bytes', 'desc']], indexPattern)).to.eql([{ bytes: 'desc' }]);
});

it('should passthrough arrays of objects', () => {
expect(getSort([{ bytes: 'desc' }], indexPattern)).to.eql([{ bytes: 'desc' }]);
});

it('should sort by the default when passed an unsortable field', function () {
Expand Down Expand Up @@ -74,7 +78,26 @@ describe('docTable', function () {
});

it('should return an array of arrays for sortable fields', function () {
expect(getSort.array(['bytes', 'desc'], indexPattern)).to.eql([[ 'bytes', 'desc' ]]);
expect(getSort.array([['bytes', 'desc']], indexPattern)).to.eql([[ 'bytes', 'desc' ]]);
});

it('should return an array of arrays from an array of elasticsearch sort objects', function () {
expect(getSort.array([{ bytes: 'desc' }], indexPattern)).to.eql([[ 'bytes', 'desc' ]]);
});

it('should sort by the default when passed an unsortable field', function () {
expect(getSort.array([{ 'non-sortable': 'asc' }], indexPattern)).to.eql([['time', 'desc']]);
expect(getSort.array([{ lol_nope: 'asc' }], indexPattern)).to.eql([['time', 'desc']]);

delete indexPattern.timeFieldName;
expect(getSort.array([{ 'non-sortable': 'asc' }], indexPattern)).to.eql([[ '_score', 'desc' ]]);
});

it('should sort by the default when passed an empty sort', () => {
expect(getSort.array([], indexPattern)).to.eql([['time', 'desc']]);

delete indexPattern.timeFieldName;
expect(getSort.array([], indexPattern)).to.eql([[ '_score', 'desc' ]]);
});
});
});
Original file line number Diff line number Diff line change
Expand Up @@ -29,24 +29,25 @@ function createSortObject(sortPair, indexPattern) {
const [ field, direction ] = sortPair;
return { [field]: direction };
}
else if (_.isPlainObject(sortPair) && isSortable(Object.keys(sortPair)[0], indexPattern)) {
Copy link
Contributor Author

Choose a reason for hiding this comment

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

I noticed a bug where getSort.array was being used with an Elasticsearch formatted sort object but we weren't actually handling that in the getSort code. So I updated this method to handle that case and added tests to make it more clear what all getSort.array is expected to do.

return sortPair;
}
else {
return undefined;
}
}

/**
* Take a sorting array and make it into an object
* @param {array} sort 2 item array [fieldToSort, directionToSort]
* @param {array} sort two dimensional array [[fieldToSort, directionToSort]]
* or an array of objects [{fieldToSort: directionToSort}]
* @param {object} indexPattern used for determining default sort
* @returns {object} a sort object suitable for returning to elasticsearch
*/
export function getSort(sort, indexPattern, defaultSortOrder = 'desc') {

let sortObjects;
if (Array.isArray(sort)) {
if (sort.length > 0 && !Array.isArray(sort[0])) {
sort = [sort];
}
sortObjects = _.compact(sort.map((sortPair) => createSortObject(sortPair, indexPattern)));
}

Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -45,11 +45,11 @@ import { ISearchEmbeddable, SearchInput, SearchOutput } from './types';
interface SearchScope extends ng.IScope {
columns?: string[];
description?: string;
sort?: string[] | string[][];
sort?: string[][];
searchSource?: SearchSource;
sharedItemTitle?: string;
inspectorAdapters?: Adapters;
setSortOrder?: (sortPair: [string, string]) => void;
setSortOrder?: (sortPair: [[string, string]]) => void;
removeColumn?: (column: string) => void;
addColumn?: (column: string) => void;
moveColumn?: (column: string, index: number) => void;
Expand Down Expand Up @@ -264,9 +264,6 @@ export class SearchEmbeddable extends Embeddable<SearchInput, SearchOutput>
// been overridden in a dashboard.
searchScope.columns = this.input.columns || this.savedSearch.columns;
searchScope.sort = this.input.sort || this.savedSearch.sort;
if (searchScope.sort.length && !Array.isArray(searchScope.sort[0])) {
searchScope.sort = [searchScope.sort];
}
searchScope.sharedItemTitle = this.panelTitle;

if (
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -34,7 +34,7 @@ export interface SearchInput extends EmbeddableInput {
filters?: Filter[];
hidePanelTitles?: boolean;
columns?: string[];
sort?: string[];
sort?: string[][];
}

export interface SearchOutput extends EmbeddableOutput {
Expand Down
2 changes: 1 addition & 1 deletion src/legacy/core_plugins/kibana/public/discover/types.d.ts
Original file line number Diff line number Diff line change
Expand Up @@ -25,7 +25,7 @@ export interface SavedSearch {
searchSource: SearchSource;
description?: string;
columns: string[];
sort: string[];
sort: string[][];
destroy: () => void;
}
export interface SavedSearchLoader {
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -212,10 +212,10 @@ export const getSavedObjects = () => [
"taxful_total_price",
"total_quantity"
],
"sort": [
"sort": [[
"order_date",
"desc"
],
]],
"version": 1,
"kibanaSavedObjectMeta": {
"searchSourceJSON": "{\"index\":\"ff959d40-b880-11e8-a6d9-e546fe2bba5f\",\"highlightAll\":true,\"version\":true,\"query\":{\"query\":\"\",\"language\":\"kuery\"},\"filter\":[]}"
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -84,10 +84,10 @@ export const getSavedObjects = () => [
"Cancelled",
"FlightDelayType"
],
"sort": [
"sort": [[
"timestamp",
"desc"
],
]],
"version": 1,
"kibanaSavedObjectMeta": {
"searchSourceJSON": "{\"index\":\"d3d7af60-4c81-11e8-b3d7-01146121b73d\",\"highlightAll\":true,\"version\":true,\"query\":{\"language\":\"kuery\",\"query\":\"\"},\"filter\":[]}"
Expand Down