Fb/fix validation for null value (#63)

* wip 1

* wip 2

* wip 3

* wip 4

* test for new behavior

Author: rlindner81

example-cap-server/http/feature-service.http

@@ -105,6 +105,13 @@ Content-Type: application/json
     "options": {
       "clearSubScopes": true
     }
+  },
+  {
+    "key": "/fts/check-service-extension",
+    "value": null,
+    "options": {
+      "clearSubScopes": true
+    }
   }
 ]
 

src/featureToggles.js

@@ -349,8 +349,12 @@ class FeatureToggles {
     return scopeKey === undefined || typeof scopeKey === "string";
   }
 
+  static _isValidScopeMapValue(value) {
+    return typeof value === "string";
+  }
+
   // NOTE: this function is used during initialization, so we cannot check this.__isInitialized
-  async _validateFeatureValue(featureKey, value, scopeMap, scopeKey) {
+  async _validateFeatureValue(featureKey, value, { scopeMap, scopeKey, isChange = false } = {}) {
     if (!this.__isConfigProcessed) {
       return [{ errorMessage: "not initialized" }];
     }
@@ -395,9 +399,14 @@ class FeatureToggles {
       return [{ featureKey, scopeKey, errorMessage: "scopeKey is not valid" }];
     }
 
-    // NOTE: value === null is our way of encoding featureKey resetting changes, so it is always allowed
+    // NOTE: value === null is our way of encoding featureKey resetting changes, so it is allowed for changes but not
+    //   for actual values
     if (value === null) {
-      return [];
+      if (isChange) {
+        return [];
+      } else {
+        return [{ featureKey, ...(scopeKey && { scopeKey }), errorMessage: "value null is not allowed" }];
+      }
     }
 
     // NOTE: skip validating active properties during initialization
@@ -533,7 +542,10 @@ class FeatureToggles {
   async validateFeatureValue(featureKey, value, scopeMap = undefined) {
     return scopeMap === undefined
       ? await this._validateFeatureValue(featureKey, value)
-      : await this._validateFeatureValue(featureKey, value, scopeMap, FeatureToggles.getScopeKey(scopeMap));
+      : await this._validateFeatureValue(featureKey, value, {
+          scopeMap,
+          scopeKey: FeatureToggles.getScopeKey(scopeMap),
+        });
   }
 
   /**
@@ -560,12 +572,10 @@ class FeatureToggles {
     let validatedStateScopedValues = {};
 
     for (const [scopeKey, value] of Object.entries(scopedValues)) {
-      const entryValidationErrors = await this._validateFeatureValue(
-        featureKey,
-        value,
-        FeatureToggles.getScopeMap(scopeKey),
-        scopeKey
-      );
+      const entryValidationErrors = await this._validateFeatureValue(featureKey, value, {
+        scopeMap: FeatureToggles.getScopeMap(scopeKey),
+        scopeKey,
+      });
       let updateValue = value;
       if (Array.isArray(entryValidationErrors) && entryValidationErrors.length > 0) {
         validationErrors = validationErrors.concat(entryValidationErrors);
@@ -902,10 +912,6 @@ class FeatureToggles {
   // START OF GET_FEATURE_VALUE SECTION
   // ========================================
 
-  static _isValidScopeMapValue(value) {
-    return typeof value === "string";
-  }
-
   /**
    * This is used to make sure scopeMap is either undefined or a shallow map with string entries. This happens for all
    * public interfaces with a scopeMap parameter, except {@link validateFeatureValue} and {@link changeFeatureValue}.
@@ -1242,7 +1248,11 @@ class FeatureToggles {
             scopeMap
           );
 
-          const validationErrors = await this._validateFeatureValue(featureKey, newValue, scopeMap, scopeKey);
+          const validationErrors = await this._validateFeatureValue(featureKey, newValue, {
+            scopeMap,
+            scopeKey,
+            isChange: true,
+          });
           if (Array.isArray(validationErrors) && validationErrors.length > 0) {
             logger.warning(
               new VError(
@@ -1289,7 +1299,11 @@ class FeatureToggles {
 
   async _changeRemoteFeatureValue(featureKey, newValue, scopeMap, options) {
     const scopeKey = FeatureToggles.getScopeKey(scopeMap);
-    const validationErrors = await this._validateFeatureValue(featureKey, newValue, scopeMap, scopeKey);
+    const validationErrors = await this._validateFeatureValue(featureKey, newValue, {
+      scopeMap,
+      scopeKey,
+      isChange: true,
+    });
     if (Array.isArray(validationErrors) && validationErrors.length > 0) {
       return validationErrors;
     }

test/__snapshots__/featureToggles.test.js.snap

@@ -201,3 +201,26 @@ exports[`feature toggles test basic apis initializeFeatureToggles 1`] = `
   },
 }
 `;
+
+exports[`feature toggles test basic apis initializeFeatureToggles warns for invalid values 1`] = `
+{
+  "test/feature_a": {
+    "SOURCE": "RUNTIME",
+    "TYPE": "boolean",
+  },
+  "test/feature_b": {
+    "SOURCE": "RUNTIME",
+    "TYPE": "string",
+  },
+  "test/feature_c": {
+    "SOURCE": "RUNTIME",
+    "TYPE": "number",
+  },
+}
+`;
+
+exports[`feature toggles test basic apis initializeFeatureToggles warns for invalid values 3`] = `
+{
+  "validationErrors": "[{"featureKey":"test/feature_b","errorMessage":"value null is not allowed"},{"featureKey":"test/feature_c","errorMessage":"value \\"{0}\\" has invalid type {1}, must be {2}","errorMessageValues":["1","string","number"]}]",
+}
+`;

test/featureToggles.test.js

@@ -25,12 +25,15 @@ jest.mock("../src/shared/env", () => require("./__mocks__/env"));
 const redisWrapperMock = require("../src/redisWrapper");
 jest.mock("../src/redisWrapper", () => require("./__mocks__/redisWrapper"));
 
-const mockFallbackValues = Object.fromEntries(
-  Object.entries(mockConfig).map(([key, { fallbackValue }]) => [key, fallbackValue])
-);
-const mockActiveKeys = Object.entries(mockConfig)
-  .filter(([, value]) => value.active !== false)
-  .map(([key]) => key);
+const configToFallbackValues = (config) =>
+  Object.fromEntries(Object.entries(config).map(([key, { fallbackValue }]) => [key, fallbackValue]));
+const configToActiveKeys = (config) =>
+  Object.entries(config)
+    .filter(([, value]) => value.active !== false)
+    .map(([key]) => key);
+
+const mockFallbackValues = configToFallbackValues(mockConfig);
+const mockActiveKeys = configToActiveKeys(mockConfig);
 
 let featureToggles = null;
 const loggerSpy = {
@@ -243,6 +246,56 @@ describe("feature toggles test", () => {
       expect(loggerSpy.error).not.toHaveBeenCalled();
     });
 
+    it("initializeFeatureToggles warns for invalid values", async () => {
+      const badMockConfig = {
+        [FEATURE.A]: {
+          fallbackValue: false, // valid
+          type: "boolean",
+        },
+        [FEATURE.B]: {
+          fallbackValue: null, // null not allowed
+          type: "string",
+        },
+        [FEATURE.C]: {
+          fallbackValue: "1", // type mismatch
+          type: "number",
+        },
+      };
+      const fallbackValues = configToFallbackValues(badMockConfig);
+      const activeKeys = configToActiveKeys(badMockConfig);
+      await featureToggles.initializeFeatures({ config: badMockConfig });
+
+      expect(featureToggles.__isInitialized).toBe(true);
+      expect(featureToggles.__fallbackValues).toStrictEqual(fallbackValues);
+      expect(featureToggles.__stateScopedValues).toStrictEqual({});
+      expect(featureToggles.__config).toMatchSnapshot();
+      expect(redisWrapperMock.watchedHashGetSetObject).toHaveBeenCalledTimes(activeKeys.length);
+      for (let fieldIndex = 0; fieldIndex < activeKeys.length; fieldIndex++) {
+        expect(redisWrapperMock.watchedHashGetSetObject).toHaveBeenNthCalledWith(
+          fieldIndex + 1,
+          redisKey,
+          activeKeys[fieldIndex],
+          expect.any(Function)
+        );
+      }
+      expect(redisWrapperMock.registerMessageHandler).toHaveBeenCalledTimes(1);
+      expect(redisWrapperMock.registerMessageHandler).toHaveBeenCalledWith(
+        redisChannel,
+        featureToggles.__messageHandler
+      );
+      expect(redisWrapperMock.subscribe).toHaveBeenCalledTimes(1);
+      expect(redisWrapperMock.subscribe).toHaveBeenCalledWith(redisChannel);
+
+      expect(loggerSpy.warning).toHaveBeenCalledTimes(1);
+      expect(loggerSpy.warning).toHaveBeenCalledWith(expect.any(VError));
+      const [loggedErr] = loggerSpy.warning.mock.calls[0];
+      expect(loggedErr).toMatchInlineSnapshot(
+        `[FeatureTogglesError: found invalid fallback values during initialization]`
+      );
+      expect(VError.info(loggedErr)).toMatchSnapshot();
+      expect(loggerSpy.error).not.toHaveBeenCalled();
+    });
+
     it("_changeRemoteFeatureValues", async () => {
       await featureToggles.initializeFeatures({ config: mockConfig });
       redisWrapperMock.watchedHashGetSetObject.mockClear();