Bugfix: Fix TS to avoid assigning filters to undefined (#40249)

* Define getAppState TS correctly, to avoid filter assignment into an undefined state * Added tests for the case where appState is undefined
elastic · Jul 3, 2019 · aef6223 · aef6223
1 parent 1eebc40
commit aef6223
Show file tree

Hide file tree

Showing 2 changed files with 103 additions and 58 deletions.
diff --git a/src/legacy/core_plugins/data/public/filter/filter_manager/filter_state_manager.test.ts b/src/legacy/core_plugins/data/public/filter/filter_manager/filter_state_manager.test.ts
@@ -58,16 +58,6 @@ describe('filter_state_manager', () => {
     globalStateStub = new StubState();
     const indexPatterns = new StubIndexPatterns();
     filterManager = new FilterManager(indexPatterns);
-
-    // FilterStateManager is tested indirectly.
-    // Therefore, we don't need it's instance.
-    new FilterStateManager(
-      globalStateStub,
-      () => {
-        return appStateStub;
-      },
-      filterManager
-    );
   });
 
   afterEach(() => {
@@ -76,51 +66,102 @@ describe('filter_state_manager', () => {
     }
   });
 
-  test('should update filter manager global filters', done => {
-    const f1 = getFilter(FilterStateStore.GLOBAL_STATE, false, false, 'age', 34);
-    globalStateStub.filters.push(f1);
-
-    setTimeout(() => {
-      expect(filterManager.getGlobalFilters()).toHaveLength(1);
-      done();
-    }, 100);
-  });
-
-  test('should update filter manager app filters', done => {
-    expect(filterManager.getAppFilters()).toHaveLength(0);
-
-    const f1 = getFilter(FilterStateStore.APP_STATE, false, false, 'age', 34);
-    appStateStub.filters.push(f1);
-
-    setTimeout(() => {
-      expect(filterManager.getAppFilters()).toHaveLength(1);
-      done();
-    }, 100);
+  describe('app_state_undefined', () => {
+    beforeEach(() => {
+      // FilterStateManager is tested indirectly.
+      // Therefore, we don't need it's instance.
+      new FilterStateManager(
+        globalStateStub,
+        () => {
+          return undefined;
+        },
+        filterManager
+      );
+    });
+
+    test('should NOT watch state until both app and global state are defined', done => {
+      const f1 = getFilter(FilterStateStore.GLOBAL_STATE, false, false, 'age', 34);
+      globalStateStub.filters.push(f1);
+
+      setTimeout(() => {
+        expect(filterManager.getGlobalFilters()).toHaveLength(0);
+        done();
+      }, 100);
+    });
+
+    test('should NOT update app URL when filter manager filters are set', async () => {
+      appStateStub.save = sinon.stub();
+      globalStateStub.save = sinon.stub();
+
+      const f1 = getFilter(FilterStateStore.APP_STATE, false, false, 'age', 34);
+      const f2 = getFilter(FilterStateStore.GLOBAL_STATE, false, false, 'age', 34);
+
+      await filterManager.setFilters([f1, f2]);
+
+      sinon.assert.notCalled(appStateStub.save);
+      sinon.assert.calledOnce(globalStateStub.save);
+    });
   });
 
-  test('should update URL when filter manager filters are set', async () => {
-    appStateStub.save = sinon.stub();
-    globalStateStub.save = sinon.stub();
-
-    const f1 = getFilter(FilterStateStore.APP_STATE, false, false, 'age', 34);
-    const f2 = getFilter(FilterStateStore.GLOBAL_STATE, false, false, 'age', 34);
-
-    await filterManager.setFilters([f1, f2]);
-
-    sinon.assert.calledOnce(appStateStub.save);
-    sinon.assert.calledOnce(globalStateStub.save);
-  });
-
-  test('should update URL when filter manager filters are added', async () => {
-    appStateStub.save = sinon.stub();
-    globalStateStub.save = sinon.stub();
-
-    const f1 = getFilter(FilterStateStore.APP_STATE, false, false, 'age', 34);
-    const f2 = getFilter(FilterStateStore.GLOBAL_STATE, false, false, 'age', 34);
-
-    await filterManager.addFilters([f1, f2]);
-
-    sinon.assert.calledOnce(appStateStub.save);
-    sinon.assert.calledOnce(globalStateStub.save);
+  describe('app_state_defined', () => {
+    beforeEach(() => {
+      // FilterStateManager is tested indirectly.
+      // Therefore, we don't need it's instance.
+      new FilterStateManager(
+        globalStateStub,
+        () => {
+          return appStateStub;
+        },
+        filterManager
+      );
+    });
+
+    test('should update filter manager global filters', done => {
+      const f1 = getFilter(FilterStateStore.GLOBAL_STATE, false, false, 'age', 34);
+      globalStateStub.filters.push(f1);
+
+      setTimeout(() => {
+        expect(filterManager.getGlobalFilters()).toHaveLength(1);
+        done();
+      }, 100);
+    });
+
+    test('should update filter manager app filters', done => {
+      expect(filterManager.getAppFilters()).toHaveLength(0);
+
+      const f1 = getFilter(FilterStateStore.APP_STATE, false, false, 'age', 34);
+      appStateStub.filters.push(f1);
+
+      setTimeout(() => {
+        expect(filterManager.getAppFilters()).toHaveLength(1);
+        done();
+      }, 100);
+    });
+
+    test('should update URL when filter manager filters are set', async () => {
+      appStateStub.save = sinon.stub();
+      globalStateStub.save = sinon.stub();
+
+      const f1 = getFilter(FilterStateStore.APP_STATE, false, false, 'age', 34);
+      const f2 = getFilter(FilterStateStore.GLOBAL_STATE, false, false, 'age', 34);
+
+      await filterManager.setFilters([f1, f2]);
+
+      sinon.assert.calledOnce(appStateStub.save);
+      sinon.assert.calledOnce(globalStateStub.save);
+    });
+
+    test('should update URL when filter manager filters are added', async () => {
+      appStateStub.save = sinon.stub();
+      globalStateStub.save = sinon.stub();
+
+      const f1 = getFilter(FilterStateStore.APP_STATE, false, false, 'age', 34);
+      const f2 = getFilter(FilterStateStore.GLOBAL_STATE, false, false, 'age', 34);
+
+      await filterManager.addFilters([f1, f2]);
+
+      sinon.assert.calledOnce(appStateStub.save);
+      sinon.assert.calledOnce(globalStateStub.save);
+    });
   });
 });
diff --git a/src/legacy/core_plugins/data/public/filter/filter_manager/filter_state_manager.ts b/src/legacy/core_plugins/data/public/filter/filter_manager/filter_state_manager.ts
@@ -23,6 +23,8 @@ import _ from 'lodash';
 import { State } from 'ui/state_management/state';
 import { FilterManager } from './filter_manager';
 
+type GetAppStateFunc = () => State | undefined | null;
+
 /**
  * FilterStateManager is responsible for watching for filter changes
  * and syncing with FilterManager, as well as syncing FilterManager changes
@@ -31,12 +33,12 @@ import { FilterManager } from './filter_manager';
 export class FilterStateManager {
   filterManager: FilterManager;
   globalState: State;
-  getAppState: () => State;
+  getAppState: GetAppStateFunc;
   prevGlobalFilters: Filter[] | undefined;
   prevAppFilters: Filter[] | undefined;
   interval: NodeJS.Timeout | undefined;
 
-  constructor(globalState: State, getAppState: () => State, filterManager: FilterManager) {
+  constructor(globalState: State, getAppState: GetAppStateFunc, filterManager: FilterManager) {
     this.getAppState = getAppState;
     this.globalState = globalState;
     this.filterManager = filterManager;
@@ -63,7 +65,7 @@ export class FilterStateManager {
       if (stateUndefined) return;
 
       const globalFilters = this.globalState.filters || [];
-      const appFilters = appState.filters || [];
+      const appFilters = (appState && appState.filters) || [];
 
       const globalFilterChanged = !(
         this.prevGlobalFilters && _.isEqual(this.prevGlobalFilters, globalFilters)
@@ -96,7 +98,9 @@ export class FilterStateManager {
     // Update Angular state before saving State objects (which save it to URL)
     const partitionedFilters = this.filterManager.getPartitionedFilters();
     const appState = this.getAppState();
-    appState.filters = partitionedFilters.appFilters;
+    if (appState) {
+      appState.filters = partitionedFilters.appFilters;
+    }
     this.globalState.filters = partitionedFilters.globalFilters;
     this.saveState();
   }