From 3184e53f2777381d27c327451c8f9fa58bf373dd Mon Sep 17 00:00:00 2001 From: Liza Katz Date: Wed, 24 Jul 2019 09:08:01 +0300 Subject: [PATCH] Filtering on a search cell first inclusive then exclusive gives bad results - #39802 (#41754) * Resolves #39802 * Fixed tests that didn't detect the bug * Don't expose filter manager's filters, but a copy. * getPartitionedFilters to use getFilters (for clone) * Adjust filter gen tests to invert filters now calling addFilters * remove invertFilter method --- .../filter_manager/filter_manager.test.ts | 41 ++++++++----------- .../filter/filter_manager/filter_manager.ts | 12 +++--- .../filter/filter_manager/lib/map_phrase.js | 6 ++- .../__tests__/action_add_filter.js | 1 - .../__tests__/filter_generator.js | 21 ++++++---- .../public/filter_manager/filter_generator.js | 3 +- .../ui/public/filter_manager/query_filter.js | 1 - 7 files changed, 45 insertions(+), 40 deletions(-) diff --git a/src/legacy/core_plugins/data/public/filter/filter_manager/filter_manager.test.ts b/src/legacy/core_plugins/data/public/filter/filter_manager/filter_manager.test.ts index 5f894cc8d8a9e..d27b0eab0e34a 100644 --- a/src/legacy/core_plugins/data/public/filter/filter_manager/filter_manager.test.ts +++ b/src/legacy/core_plugins/data/public/filter/filter_manager/filter_manager.test.ts @@ -398,8 +398,8 @@ describe('filter_manager', () => { }); test('should fire the update and fetch events', async function() { - const updateStub = sinon.stub(); - const fetchStub = sinon.stub(); + const updateStub = jest.fn(); + const fetchStub = jest.fn(); filterManager.getUpdates$().subscribe({ next: updateStub, @@ -416,8 +416,8 @@ describe('filter_manager', () => { expect(globalStateStub.save.callCount).toBe(1); // this time, events should be emitted - expect(fetchStub.called); - expect(updateStub.called); + expect(fetchStub).toBeCalledTimes(1); + expect(updateStub).toBeCalledTimes(1); }); }); @@ -595,8 +595,8 @@ describe('filter_manager', () => { }); test('should fire the update and fetch events', async function() { - const updateStub = sinon.stub(); - const fetchStub = sinon.stub(); + const updateStub = jest.fn(); + const fetchStub = jest.fn(); await filterManager.addFilters(readyFilters, false); @@ -611,8 +611,8 @@ describe('filter_manager', () => { filterManager.removeFilter(readyFilters[0]); // this time, events should be emitted - expect(fetchStub.called); - expect(updateStub.called); + expect(fetchStub).toBeCalledTimes(1); + expect(updateStub).toBeCalledTimes(1); }); test('should remove matching filters', async function() { @@ -664,19 +664,12 @@ describe('filter_manager', () => { }); describe('invert', () => { - test('invert to disabled', async () => { - const f1 = getFilter(FilterStateStore.GLOBAL_STATE, false, false, 'age', 34); - filterManager.invertFilter(f1); - expect(f1.meta.negate).toBe(true); - filterManager.invertFilter(f1); - expect(f1.meta.negate).toBe(false); - }); - - test('should fire the update and fetch events', function() { - const updateStub = sinon.stub(); - const fetchStub = sinon.stub(); + test('should fire the update and fetch events', async function() { + await filterManager.addFilters(readyFilters); + expect(filterManager.getFilters()).toHaveLength(3); - filterManager.addFilters(readyFilters); + const updateStub = jest.fn(); + const fetchStub = jest.fn(); filterManager.getUpdates$().subscribe({ next: updateStub, }); @@ -685,9 +678,11 @@ describe('filter_manager', () => { next: fetchStub, }); - filterManager.invertFilter(readyFilters[1]); - expect(fetchStub.called); - expect(updateStub.called); + readyFilters[1].meta.negate = !readyFilters[1].meta.negate; + await filterManager.addFilters(readyFilters[1]); + expect(filterManager.getFilters()).toHaveLength(3); + expect(fetchStub).toBeCalledTimes(1); + expect(updateStub).toBeCalledTimes(1); }); }); diff --git a/src/legacy/core_plugins/data/public/filter/filter_manager/filter_manager.ts b/src/legacy/core_plugins/data/public/filter/filter_manager/filter_manager.ts index 16d927d9bb5c0..6c472ab76f9c0 100644 --- a/src/legacy/core_plugins/data/public/filter/filter_manager/filter_manager.ts +++ b/src/legacy/core_plugins/data/public/filter/filter_manager/filter_manager.ts @@ -105,7 +105,7 @@ export class FilterManager { /* Getters */ public getFilters() { - return this.filters; + return _.cloneDeep(this.filters); } public getAppFilters() { @@ -119,7 +119,7 @@ export class FilterManager { } public getPartitionedFilters(): PartitionedFilters { - return FilterManager.partitionFilters(this.filters); + return FilterManager.partitionFilters(this.getFilters()); } public getUpdates$() { @@ -137,6 +137,10 @@ export class FilterManager { filters = [filters]; } + if (filters.length === 0) { + return; + } + const { uiSettings } = npSetup.core; if (pinFilterStatus === undefined) { pinFilterStatus = uiSettings.get('filters:pinnedByDefault'); @@ -178,10 +182,6 @@ export class FilterManager { } } - public invertFilter(filter: Filter) { - filter.meta.negate = !filter.meta.negate; - } - public async removeAll() { await this.setFilters([]); } diff --git a/src/legacy/core_plugins/data/public/filter/filter_manager/lib/map_phrase.js b/src/legacy/core_plugins/data/public/filter/filter_manager/lib/map_phrase.js index 697c8250f7fed..b7ee58e9078ba 100644 --- a/src/legacy/core_plugins/data/public/filter/filter_manager/lib/map_phrase.js +++ b/src/legacy/core_plugins/data/public/filter/filter_manager/lib/map_phrase.js @@ -36,7 +36,11 @@ function getParams(filter, indexPattern) { // for example a user might manually edit the url or the index pattern's ID might change due to // external factors e.g. a reindex. We only need the index in order to grab the field formatter, so we fallback // on displaying the raw value if the index or field is invalid. - const value = (indexPattern && indexPattern.fields.byName[key]) ? indexPattern.fields.byName[key].format.convert(query) : query; + const value = ( + indexPattern && + indexPattern.fields && + indexPattern.fields.byName[key] + ) ? indexPattern.fields.byName[key].format.convert(query) : query; return { type, key, value, params }; } diff --git a/src/legacy/core_plugins/kibana/public/context/query_parameters/__tests__/action_add_filter.js b/src/legacy/core_plugins/kibana/public/context/query_parameters/__tests__/action_add_filter.js index b4161e51bd979..631a36547c984 100644 --- a/src/legacy/core_plugins/kibana/public/context/query_parameters/__tests__/action_add_filter.js +++ b/src/legacy/core_plugins/kibana/public/context/query_parameters/__tests__/action_add_filter.js @@ -71,7 +71,6 @@ describe('context app', function () { function createQueryFilterStub() { return { addFilters: sinon.stub(), - invertFilter: sinon.stub(), getAppFilters: sinon.stub(), }; } diff --git a/src/legacy/ui/public/filter_manager/__tests__/filter_generator.js b/src/legacy/ui/public/filter_manager/__tests__/filter_generator.js index a7fd90653fd9c..e7752b1e4b906 100644 --- a/src/legacy/ui/public/filter_manager/__tests__/filter_generator.js +++ b/src/legacy/ui/public/filter_manager/__tests__/filter_generator.js @@ -24,6 +24,7 @@ import expect from '@kbn/expect'; import ngMock from 'ng_mock'; import { getFilterGenerator } from '..'; import { FilterBarQueryFilterProvider } from '../../filter_manager/query_filter'; +import { uniqFilters } from '../../../../core_plugins/data/public/filter/filter_manager/lib/uniq_filters'; import { getPhraseScript } from '@kbn/es-query'; let queryFilter; let filterGen; @@ -63,10 +64,7 @@ describe('Filter Manager', function () { sinon.stub(queryFilter, 'getAppFilters').callsFake(() => appState.filters); sinon.stub(queryFilter, 'addFilters').callsFake((filters) => { if (!Array.isArray(filters)) filters = [filters]; - appState.filters = appState.filters.concat(filters); - }); - sinon.stub(queryFilter, 'invertFilter').callsFake((filter) => { - filter.meta.negate = !filter.meta.negate; + appState.filters = uniqFilters(appState.filters.concat(filters)); }); })); @@ -116,7 +114,10 @@ describe('Filter Manager', function () { // NOTE: negating exists filters also forces disabled to false filterGen.add('myField', 1, '-', 'myIndex'); - checkAddFilters(0, null, 1); + checkAddFilters(1, [{ + meta: { index: 'myIndex', negate: true, disabled: false }, + query: { match: { myField: { query: 1, type: 'phrase' } } } + }], 1); expect(appState.filters).to.have.length(1); filterGen.add('_exists_', 'myField', '+', 'myIndex'); @@ -127,7 +128,10 @@ describe('Filter Manager', function () { expect(appState.filters).to.have.length(2); filterGen.add('_exists_', 'myField', '-', 'myIndex'); - checkAddFilters(0, null, 3); + checkAddFilters(1, [{ + meta: { index: 'myIndex', negate: true, disabled: false }, + exists: { field: 'myField' } + }], 3); expect(appState.filters).to.have.length(2); const scriptedField = { name: 'scriptedField', scripted: true, script: 1, lang: 'painless' }; @@ -139,7 +143,10 @@ describe('Filter Manager', function () { expect(appState.filters).to.have.length(3); filterGen.add(scriptedField, 1, '-', 'myIndex'); - checkAddFilters(0, null, 5); + checkAddFilters(1, [{ + meta: { index: 'myIndex', negate: true, disabled: false, field: 'scriptedField' }, + script: getPhraseScript(scriptedField, 1) + }], 5); expect(appState.filters).to.have.length(3); }); diff --git a/src/legacy/ui/public/filter_manager/filter_generator.js b/src/legacy/ui/public/filter_manager/filter_generator.js index ff034e22aa370..6345f2bad72da 100644 --- a/src/legacy/ui/public/filter_manager/filter_generator.js +++ b/src/legacy/ui/public/filter_manager/filter_generator.js @@ -54,8 +54,9 @@ export function getFilterGenerator(queryFilter) { if (existing) { existing.meta.disabled = false; if (existing.meta.negate !== negate) { - queryFilter.invertFilter(existing); + existing.meta.negate = !existing.meta.negate; } + newFilters.push(existing); return; } diff --git a/src/legacy/ui/public/filter_manager/query_filter.js b/src/legacy/ui/public/filter_manager/query_filter.js index 19c6e9f5053cc..f62bde07781f7 100644 --- a/src/legacy/ui/public/filter_manager/query_filter.js +++ b/src/legacy/ui/public/filter_manager/query_filter.js @@ -33,7 +33,6 @@ export function FilterBarQueryFilterProvider(getAppState, globalState) { queryFilter.getAppFilters = filterManager.getAppFilters.bind(filterManager); queryFilter.getGlobalFilters = filterManager.getGlobalFilters.bind(filterManager); queryFilter.removeFilter = filterManager.removeFilter.bind(filterManager); - queryFilter.invertFilter = filterManager.invertFilter.bind(filterManager); queryFilter.addFilters = filterManager.addFilters.bind(filterManager); queryFilter.setFilters = filterManager.setFilters.bind(filterManager); queryFilter.addFiltersAndChangeTimeFilter = filterManager.addFiltersAndChangeTimeFilter.bind(filterManager);