Filtering on a search cell first inclusive then exclusive gives bad r…

…esults - elastic#39802 (elastic#41754) * Resolves elastic#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
lizozom · Jul 24, 2019 · 3184e53 · 3184e53
1 parent a8cbd1f
commit 3184e53
Show file tree

Hide file tree

Showing 7 changed files with 45 additions and 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
@@ -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
@@ -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
@@ -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/...legacy/core_plugins/kibana/public/context/query_parameters/__tests__/action_add_filter.js b/...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
@@ -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
@@ -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
@@ -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);