[BUG] Data fetching twice on discover timefilter change (#55279) (#55369)

* Fix bug #54887
 - Filters are not only fetch once on timefilter change
 - Make sure that discover doesn't fetch data when a disabled filter is changed
 - Support compareFilters on an array of filters.
 - Added tests to compare filters
 - Exctracted sortFilters and added tests to it.

* code review + FilterCompareOptions

* Remove sort by

Co-authored-by: Elastic Machine <elasticmachine@users.noreply.github.com>

Co-authored-by: Elastic Machine <elasticmachine@users.noreply.github.com>

src/legacy/core_plugins/data/public/filter/filter_manager/filter_state_manager.ts

@@ -21,6 +21,13 @@ import _ from 'lodash';
 import { State } from 'ui/state_management/state';
 import { FilterManager, esFilters } from '../../../../../../plugins/data/public';
 
+import {
+  compareFilters,
+  COMPARE_ALL_OPTIONS,
+  // this whole file will soon be deprecated by new state management.
+  // eslint-disable-next-line @kbn/eslint/no-restricted-paths
+} from '../../../../../../plugins/data/public/query/filter_manager/lib/compare_filters';
+
 type GetAppStateFunc = () => State | undefined | null;
 
 /**
@@ -63,8 +70,16 @@ export class FilterStateManager {
       const globalFilters = this.globalState.filters || [];
       const appFilters = (appState && appState.filters) || [];
 
-      const globalFilterChanged = !_.isEqual(this.filterManager.getGlobalFilters(), globalFilters);
-      const appFilterChanged = !_.isEqual(this.filterManager.getAppFilters(), appFilters);
+      const globalFilterChanged = !compareFilters(
+        this.filterManager.getGlobalFilters(),
+        globalFilters,
+        COMPARE_ALL_OPTIONS
+      );
+      const appFilterChanged = !compareFilters(
+        this.filterManager.getAppFilters(),
+        appFilters,
+        COMPARE_ALL_OPTIONS
+      );
       const filterStateChanged = globalFilterChanged || appFilterChanged;
 
       if (!filterStateChanged) return;

src/legacy/core_plugins/kibana/public/discover/np_ready/angular/discover.js

@@ -637,7 +637,7 @@ function discoverController(
 
       // fetch data when filters fire fetch event
       subscriptions.add(
-        subscribeWithScope($scope, filterManager.getUpdates$(), {
+        subscribeWithScope($scope, filterManager.getFetches$(), {
           next: $scope.fetch,
         })
       );

src/plugins/data/public/query/filter_manager/filter_manager.ts

@@ -22,7 +22,8 @@ import { Subject } from 'rxjs';
 
 import { IUiSettingsClient } from 'src/core/public';
 
-import { compareFilters } from './lib/compare_filters';
+import { compareFilters, COMPARE_ALL_OPTIONS } from './lib/compare_filters';
+import { sortFilters } from './lib/sort_filters';
 import { mapAndFlattenFilters } from './lib/map_and_flatten_filters';
 import { uniqFilters } from './lib/uniq_filters';
 import { onlyDisabledFiltersChanged } from './lib/only_disabled';
@@ -76,20 +77,9 @@ export class FilterManager {
   }
 
   private handleStateUpdate(newFilters: esFilters.Filter[]) {
-    // global filters should always be first
+    newFilters.sort(sortFilters);
 
-    newFilters.sort(({ $state: a }: esFilters.Filter, { $state: b }: esFilters.Filter): number => {
-      if (a!.store === b!.store) {
-        return 0;
-      } else {
-        return a!.store === esFilters.FilterStateStore.GLOBAL_STATE &&
-          b!.store !== esFilters.FilterStateStore.GLOBAL_STATE
-          ? -1
-          : 1;
-      }
-    });
-
-    const filtersUpdated = !_.isEqual(this.filters, newFilters);
+    const filtersUpdated = !compareFilters(this.filters, newFilters, COMPARE_ALL_OPTIONS);
     const updatedOnlyDisabledFilters = onlyDisabledFiltersChanged(newFilters, this.filters);
 
     this.filters = newFilters;

src/plugins/data/public/query/filter_manager/lib/compare_filters.test.ts

@@ -17,7 +17,7 @@
  * under the License.
  */
 
-import { compareFilters } from './compare_filters';
+import { compareFilters, COMPARE_ALL_OPTIONS } from './compare_filters';
 import { esFilters } from '../../../../common';
 
 describe('filter manager utilities', () => {
@@ -83,5 +83,134 @@ describe('filter manager utilities', () => {
 
       expect(compareFilters(f1, f2)).toBeTruthy();
     });
+
+    test('should compare filters array to non array', () => {
+      const f1 = esFilters.buildQueryFilter(
+        { _type: { match: { query: 'apache', type: 'phrase' } } },
+        'index',
+        ''
+      );
+
+      const f2 = esFilters.buildQueryFilter(
+        { _type: { match: { query: 'mochi', type: 'phrase' } } },
+        'index',
+        ''
+      );
+
+      expect(compareFilters([f1, f2], f1)).toBeFalsy();
+    });
+
+    test('should compare filters array to partial array', () => {
+      const f1 = esFilters.buildQueryFilter(
+        { _type: { match: { query: 'apache', type: 'phrase' } } },
+        'index',
+        ''
+      );
+
+      const f2 = esFilters.buildQueryFilter(
+        { _type: { match: { query: 'mochi', type: 'phrase' } } },
+        'index',
+        ''
+      );
+
+      expect(compareFilters([f1, f2], [f1])).toBeFalsy();
+    });
+
+    test('should compare filters array to exact array', () => {
+      const f1 = esFilters.buildQueryFilter(
+        { _type: { match: { query: 'apache', type: 'phrase' } } },
+        'index',
+        ''
+      );
+
+      const f2 = esFilters.buildQueryFilter(
+        { _type: { match: { query: 'mochi', type: 'phrase' } } },
+        'index',
+        ''
+      );
+
+      expect(compareFilters([f1, f2], [f1, f2])).toBeTruthy();
+    });
+
+    test('should compare array of duplicates, ignoring meta attributes', () => {
+      const f1 = esFilters.buildQueryFilter(
+        { _type: { match: { query: 'apache', type: 'phrase' } } },
+        'index1',
+        ''
+      );
+      const f2 = esFilters.buildQueryFilter(
+        { _type: { match: { query: 'apache', type: 'phrase' } } },
+        'index2',
+        ''
+      );
+
+      expect(compareFilters([f1], [f2])).toBeTruthy();
+    });
+
+    test('should compare array of duplicates, ignoring $state attributes', () => {
+      const f1 = {
+        $state: { store: esFilters.FilterStateStore.APP_STATE },
+        ...esFilters.buildQueryFilter(
+          { _type: { match: { query: 'apache', type: 'phrase' } } },
+          'index',
+          ''
+        ),
+      };
+      const f2 = {
+        $state: { store: esFilters.FilterStateStore.GLOBAL_STATE },
+        ...esFilters.buildQueryFilter(
+          { _type: { match: { query: 'apache', type: 'phrase' } } },
+          'index',
+          ''
+        ),
+      };
+
+      expect(compareFilters([f1], [f2])).toBeTruthy();
+    });
+
+    test('should compare duplicates with COMPARE_ALL_OPTIONS should check store', () => {
+      const f1 = {
+        $state: { store: esFilters.FilterStateStore.APP_STATE },
+        ...esFilters.buildQueryFilter(
+          { _type: { match: { query: 'apache', type: 'phrase' } } },
+          'index',
+          ''
+        ),
+      };
+      const f2 = {
+        $state: { store: esFilters.FilterStateStore.GLOBAL_STATE },
+        ...esFilters.buildQueryFilter(
+          { _type: { match: { query: 'apache', type: 'phrase' } } },
+          'index',
+          ''
+        ),
+      };
+
+      expect(compareFilters([f1], [f2], COMPARE_ALL_OPTIONS)).toBeFalsy();
+    });
+
+    test('should compare duplicates with COMPARE_ALL_OPTIONS should not check key and value ', () => {
+      const f1 = {
+        $state: { store: esFilters.FilterStateStore.GLOBAL_STATE },
+        ...esFilters.buildQueryFilter(
+          { _type: { match: { query: 'apache', type: 'phrase' } } },
+          'index',
+          ''
+        ),
+      };
+      const f2 = {
+        $state: { store: esFilters.FilterStateStore.GLOBAL_STATE },
+        ...esFilters.buildQueryFilter(
+          { _type: { match: { query: 'apache', type: 'phrase' } } },
+          'index',
+          ''
+        ),
+      };
+
+      f2.meta.key = 'wassup';
+      f2.meta.value = 'dog';
+
+      expect(compareFilters([f1], [f2], COMPARE_ALL_OPTIONS)).toBeTruthy();
+    });
   });
 });

src/plugins/data/public/query/filter_manager/lib/compare_filters.ts

@@ -17,32 +17,64 @@
  * under the License.
  */
 
-import { defaults, isEqual, omit } from 'lodash';
+import { defaults, isEqual, omit, map } from 'lodash';
 import { esFilters } from '../../../../common';
 
+export interface FilterCompareOptions {
+  disabled?: boolean;
+  negate?: boolean;
+  state?: boolean;
+}
+
 /**
- * Compare two filters to see if they match
+ * Include disabled, negate and store when comparing filters
+ */
+export const COMPARE_ALL_OPTIONS: FilterCompareOptions = {
+  disabled: true,
+  negate: true,
+  state: true,
+};
+
+const mapFilter = (
+  filter: esFilters.Filter,
+  comparators: FilterCompareOptions,
+  excludedAttributes: string[]
+) => {
+  const cleaned: esFilters.FilterMeta = omit(filter, excludedAttributes);
+
+  if (comparators.negate) cleaned.negate = filter.meta && Boolean(filter.meta.negate);
+  if (comparators.disabled) cleaned.disabled = filter.meta && Boolean(filter.meta.disabled);
+
+  return cleaned;
+};
+
+const mapFilterArray = (
+  filters: esFilters.Filter[],
+  comparators: FilterCompareOptions,
+  excludedAttributes: string[]
+) => {
+  return map(filters, (filter: esFilters.Filter) =>
+    mapFilter(filter, comparators, excludedAttributes)
+  );
+};
+
+/**
+ * Compare two filters or filter arrays to see if they match.
+ * For filter arrays, the assumption is they are sorted.
  *
- * @param {object} first The first filter to compare
- * @param {object} second The second filter to compare
- * @param {object} comparatorOptions Parameters to use for comparison
+ * @param {esFilters.Filter | esFilters.Filter[]} first The first filter or filter array to compare
+ * @param {esFilters.Filter | esFilters.Filter[]} second The second filter or filter array to compare
+ * @param {FilterCompareOptions} comparatorOptions Parameters to use for comparison
  *
  * @returns {bool} Filters are the same
  */
 export const compareFilters = (
-  first: esFilters.Filter,
-  second: esFilters.Filter,
-  comparatorOptions: any = {}
+  first: esFilters.Filter | esFilters.Filter[],
+  second: esFilters.Filter | esFilters.Filter[],
+  comparatorOptions: FilterCompareOptions = {}
 ) => {
-  let comparators: any = {};
-  const mapFilter = (filter: esFilters.Filter) => {
-    const cleaned: esFilters.FilterMeta = omit(filter, excludedAttributes);
+  let comparators: FilterCompareOptions = {};
 
-    if (comparators.negate) cleaned.negate = filter.meta && Boolean(filter.meta.negate);
-    if (comparators.disabled) cleaned.disabled = filter.meta && Boolean(filter.meta.disabled);
-
-    return cleaned;
-  };
   const excludedAttributes: string[] = ['$$hashKey', 'meta'];
 
   comparators = defaults(comparatorOptions || {}, {
@@ -53,5 +85,17 @@ export const compareFilters = (
 
   if (!comparators.state) excludedAttributes.push('$state');
 
-  return isEqual(mapFilter(first), mapFilter(second));
+  if (Array.isArray(first) && Array.isArray(second)) {
+    return isEqual(
+      mapFilterArray(first, comparators, excludedAttributes),
+      mapFilterArray(second, comparators, excludedAttributes)
+    );
+  } else if (!Array.isArray(first) && !Array.isArray(second)) {
+    return isEqual(
+      mapFilter(first, comparators, excludedAttributes),
+      mapFilter(second, comparators, excludedAttributes)
+    );
+  } else {
+    return false;
+  }
 };

src/plugins/data/public/query/filter_manager/lib/dedup_filters.ts

@@ -18,7 +18,7 @@
  */
 
 import { filter, find } from 'lodash';
-import { compareFilters } from './compare_filters';
+import { compareFilters, FilterCompareOptions } from './compare_filters';
 import { esFilters } from '../../../../common';
 
 /**
@@ -33,7 +33,7 @@ import { esFilters } from '../../../../common';
 export const dedupFilters = (
   existingFilters: esFilters.Filter[],
   filters: esFilters.Filter[],
-  comparatorOptions: any = {}
+  comparatorOptions: FilterCompareOptions = {}
 ) => {
   if (!Array.isArray(filters)) {
     filters = [filters];

src/plugins/data/public/query/filter_manager/lib/only_disabled.ts

@@ -17,8 +17,9 @@
  * under the License.
  */
 
-import { filter, isEqual } from 'lodash';
+import { filter } from 'lodash';
 import { esFilters } from '../../../../common';
+import { compareFilters, COMPARE_ALL_OPTIONS } from './compare_filters';
 
 const isEnabled = (f: esFilters.Filter) => f && f.meta && !f.meta.disabled;
 
@@ -35,5 +36,5 @@ export const onlyDisabledFiltersChanged = (
   const newEnabledFilters = filter(newFilters || [], isEnabled);
   const oldEnabledFilters = filter(oldFilters || [], isEnabled);
 
-  return isEqual(oldEnabledFilters, newEnabledFilters);
+  return compareFilters(oldEnabledFilters, newEnabledFilters, COMPARE_ALL_OPTIONS);
 };

src/plugins/data/public/query/filter_manager/lib/sort_filters.test.ts

@@ -0,0 +1,91 @@
+/*
+ * 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 { sortFilters } from './sort_filters';
+import { esFilters } from '../../../../common';
+
+describe('sortFilters', () => {
+  describe('sortFilters()', () => {
+    test('Not sort two application level filters', () => {
+      const f1 = {
+        $state: { store: esFilters.FilterStateStore.APP_STATE },
+        ...esFilters.buildQueryFilter(
+          { _type: { match: { query: 'apache', type: 'phrase' } } },
+          'index',
+          ''
+        ),
+      };
+      const f2 = {
+        $state: { store: esFilters.FilterStateStore.APP_STATE },
+        ...esFilters.buildQueryFilter(
+          { _type: { match: { query: 'apache', type: 'phrase' } } },
+          'index',
+          ''
+        ),
+      };
+
+      const filters = [f1, f2].sort(sortFilters);
+      expect(filters[0]).toBe(f1);
+    });
+
+    test('Not sort two global level filters', () => {
+      const f1 = {
+        $state: { store: esFilters.FilterStateStore.GLOBAL_STATE },
+        ...esFilters.buildQueryFilter(
+          { _type: { match: { query: 'apache', type: 'phrase' } } },
+          'index',
+          ''
+        ),
+      };
+      const f2 = {
+        $state: { store: esFilters.FilterStateStore.GLOBAL_STATE },
+        ...esFilters.buildQueryFilter(
+          { _type: { match: { query: 'apache', type: 'phrase' } } },
+          'index',
+          ''
+        ),
+      };
+
+      const filters = [f1, f2].sort(sortFilters);
+      expect(filters[0]).toBe(f1);
+    });
+
+    test('Move global level filter to the beginning of the array', () => {
+      const f1 = {
+        $state: { store: esFilters.FilterStateStore.APP_STATE },
+        ...esFilters.buildQueryFilter(
+          { _type: { match: { query: 'apache', type: 'phrase' } } },
+          'index',
+          ''
+        ),
+      };
+      const f2 = {
+        $state: { store: esFilters.FilterStateStore.GLOBAL_STATE },
+        ...esFilters.buildQueryFilter(
+          { _type: { match: { query: 'apache', type: 'phrase' } } },
+          'index',
+          ''
+        ),
+      };
+
+      const filters = [f1, f2].sort(sortFilters);
+      expect(filters[0]).toBe(f2);
+    });
+  });
+});

src/plugins/data/public/query/filter_manager/lib/sort_filters.ts

@@ -0,0 +1,42 @@
+/*
+ * 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 { esFilters } from '../../../../common';
+
+/**
+ * Sort filters according to their store - global filters go first
+ *
+ * @param {object} first The first filter to compare
+ * @param {object} second The second filter to compare
+ *
+ * @returns {number} Sorting order of filters
+ */
+export const sortFilters = (
+  { $state: a }: esFilters.Filter,
+  { $state: b }: esFilters.Filter
+): number => {
+  if (a!.store === b!.store) {
+    return 0;
+  } else {
+    return a!.store === esFilters.FilterStateStore.GLOBAL_STATE &&
+      b!.store !== esFilters.FilterStateStore.GLOBAL_STATE
+      ? -1
+      : 1;
+  }
+};