[ML] Fixes sync issue where a manipulated AppState wouldn't contain the original attribute anymore. (#18653)

Inconcistencies between AppState and derived custom states for UI components like mlSelectLimit and mlSelectInterval could trigger errors because the parent of a nested object attribute could become undefined. This PR adds additional checks to state_factory.js's get/set/reset methods to reinitialize itself with AppState and fall back to its default state should it not be present within AppState anymore. The original issues were hard to reproduce consistently using the UI only. However the error could be triggered when creating a malformed URL, like replacing the mlSelectInterval:(interval:(display:Auto,val:auto)) part of the URL to something like mlSelectInterval:(undefined). This PR includes additional unit tests which simulate this behaviour and fail without the additional checks provided by this PR.

x-pack/plugins/ml/public/factories/__tests__/state_factory.js

@@ -13,9 +13,11 @@ import { stateFactoryProvider } from '../state_factory';
 
 describe('ML - mlStateFactory', () => {
   let stateFactory;
+  let AppState;
 
   beforeEach(ngMock.module('kibana'));
   beforeEach(ngMock.inject(($injector) => {
+    AppState = $injector.get('AppState');
     const Private = $injector.get('Private');
     stateFactory = Private(stateFactoryProvider);
   }));
@@ -53,4 +55,49 @@ describe('ML - mlStateFactory', () => {
     state.set('testValue', 10);
     state.changed();
   });
+
+  it(`Malformed AppState, state falls back to undefined, doesn't throw an error`, () => {
+    const state = stateFactory('testName');
+
+    // First update the state without interfering with AppState
+    state.set('testValue', 1);
+    expect(state.get('testValue')).to.be(1);
+
+    // Manipulate AppState first, then set and get the custom state.
+    const appState = new AppState();
+    appState.fetch();
+    appState.testName = undefined;
+    appState.save();
+    state.set('testValue', 2);
+    expect(state.get('testValue')).to.be(2);
+
+    // Now set the custom state, then manipulate AppState, then get the custom state.
+    // Because AppState was malformed between set and get, the custom state will fallback
+    // to the default state, in this case undefined
+    state.set('testValue', 3);
+    appState.fetch();
+    appState.testName = undefined;
+    appState.save();
+    expect(state.get('testValue')).to.be(undefined);
+  });
+
+  it(`Malformed AppState, state falls back to default, doesn't throw an error`, () => {
+    const state = stateFactory('testName', {
+      testValue: 1
+    });
+
+    // First update the state without interfering with AppState
+    state.set('testValue', 2);
+    expect(state.get('testValue')).to.be(2);
+
+    // Now set the custom state, then manipulate AppState, then get the custom state.
+    // Because AppState was malformed between set and get, the custom state will fallback
+    // to the default state, in this case 1
+    state.set('testValue', 3);
+    const appState = new AppState();
+    appState.fetch();
+    appState.testName = undefined;
+    appState.save();
+    expect(state.get('testValue')).to.be(1);
+  });
 });

x-pack/plugins/ml/public/factories/state_factory.js

@@ -19,11 +19,7 @@ import { listenerFactoryProvider } from './listener_factory';
 // Have a look at the unit tests which demonstrate basic usage.
 
 export function stateFactoryProvider(AppState) {
-  return function (stateName, defaultState) {
-    if (typeof stateName !== 'string') {
-      throw 'stateName needs to be of type `string`';
-    }
-
+  function initializeAppState(stateName, defaultState) {
     const appState = new AppState();
     appState.fetch();
 
@@ -49,6 +45,16 @@ export function stateFactoryProvider(AppState) {
       }
     }
 
+    return appState;
+  }
+
+  return function (stateName, defaultState) {
+    if (typeof stateName !== 'string') {
+      throw 'stateName needs to be of type `string`';
+    }
+
+    let appState = initializeAppState(stateName, defaultState);
+
     // () two times here, because the Provider first returns
     // the Factory, which then returns the actual listener
     const listener = listenerFactoryProvider()();
@@ -60,11 +66,13 @@ export function stateFactoryProvider(AppState) {
     // on the state.
     const state = {
       get(name) {
-        return appState[stateName] && appState[stateName][name];
+        updateAppState();
+        return appState[stateName][name];
       },
       // only if value doesn't match the existing one, the state gets updated
       // and saved.
       set(name, value) {
+        updateAppState();
         if (!_.isEqual(appState[stateName][name], value)) {
           appState[stateName][name] = value;
           appState.save();
@@ -73,6 +81,7 @@ export function stateFactoryProvider(AppState) {
         return state;
       },
       reset() {
+        updateAppState();
         if (!_.isEqual(appState[stateName], defaultState)) {
           appState[stateName] = _.cloneDeep(defaultState);
           appState.save();
@@ -92,6 +101,16 @@ export function stateFactoryProvider(AppState) {
       }
     };
 
+    // gets the current state of AppState, if for whatever reason this custom
+    // state isn't part of AppState anymore, reinitialize it
+    function updateAppState() {
+      appState.fetch();
+      if (typeof appState[stateName] === 'undefined') {
+        appState = initializeAppState(stateName, defaultState);
+        changed = true;
+      }
+    }
+
     return state;
   };
 }