Fb/resilient scope map handling (#19)

rlindner81 · web-flow · commit b4e2526c3e24 · 2023-07-18T17:28:26.000+02:00
* new todo

* wip 1

* remove bad approach

* better approach

* fix tests and numerous bugs

* remove TODOs

* more tests and fixes

* one more test

* empty line

* rollback some unnecessary stuff
diff --git a/src/featureToggles.js b/src/featureToggles.js
@@ -24,7 +24,7 @@ const { REDIS_INTEGRATION_MODE } = redis;
 const { Logger } = require("./logger");
 const { isOnCF, cfEnv } = require("./env");
 const { HandlerCollection } = require("./shared/handlerCollection");
-const { ENV, isNull } = require("./shared/static");
+const { ENV, isObject } = require("./shared/static");
 const { promiseAllDone } = require("./shared/promiseAllDone");
 const { LimitedLazyCache } = require("./shared/cache");
 
@@ -278,27 +278,32 @@ class FeatureToggles {
       return [{ featureKey, errorMessage: "feature key is not valid" }];
     }
 
-    if (scopeMap) {
+    if (scopeMap !== undefined) {
+      if (!isObject(scopeMap)) {
+        return [
+          {
+            featureKey,
+            errorMessage: "scopeMap must be undefined or an object",
+          },
+        ];
+      }
       const allowedScopesCheckMap = this.__config[featureKey][CONFIG_KEY.ALLOWED_SCOPES_CHECK_MAP];
       for (const [scope, value] of Object.entries(scopeMap)) {
-        if (allowedScopesCheckMap && !allowedScopesCheckMap[scope]) {
+        if (!FeatureToggles._isValidScopeMapValue(value)) {
           return [
             {
               featureKey,
-              scopeKey,
-              errorMessage: 'scope "{0}" is not allowed',
-              errorMessageValues: [scope],
+              errorMessage: 'scope "{0}" has invalid type {1}, must be string',
+              errorMessageValues: [scope, typeof value],
             },
           ];
         }
-        const scopeType = typeof value;
-        if (scopeType !== "string") {
+        if (allowedScopesCheckMap && !allowedScopesCheckMap[scope]) {
           return [
             {
               featureKey,
-              scopeKey,
-              errorMessage: 'scope "{0}" has invalid type {1}, must be string',
-              errorMessageValues: [scope, scopeType],
+              errorMessage: 'scope "{0}" is not allowed',
+              errorMessageValues: [scope],
             },
           ];
         }
@@ -453,7 +458,7 @@ class FeatureToggles {
    */
   async _validateFallbackValues(fallbackValues) {
     let validationErrors = [];
-    if (isNull(fallbackValues) || typeof fallbackValues !== "object") {
+    if (!isObject(fallbackValues)) {
       return validationErrors;
     }
 
@@ -518,7 +523,7 @@ class FeatureToggles {
           this.__redisKey,
           featureKey,
           async (scopedValues) => {
-            if (typeof scopedValues !== "object" || scopedValues === null) {
+            if (!isObject(scopedValues)) {
               return null;
             }
             const [validatedScopedValues, scopedValidationErrors] = await this._validateScopedValues(
@@ -761,11 +766,34 @@ 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}.
+   * For these two interfaces, we want the "bad" scopeMaps to cause validation errors.
+   * Also not for {@link getScopeKey}, where the sanitization must not happen in place.
+   */
+  static _sanitizeScopeMap(scopeMap) {
+    if (!isObject(scopeMap)) {
+      return undefined;
+    }
+    for (const [scope, value] of Object.entries(scopeMap)) {
+      if (!FeatureToggles._isValidScopeMapValue(value)) {
+        Reflect.deleteProperty(scopeMap, scope);
+      }
+    }
+    return scopeMap;
+  }
+
+  // NOTE: getScopeMap does the scopeMap sanitization on the fly, because it must not modify scopeMap in place.
   static getScopeKey(scopeMap) {
-    if (typeof scopeMap !== "object" || scopeMap === null) {
+    if (!isObject(scopeMap)) {
       return SCOPE_ROOT_KEY;
     }
-    const scopeMapKeys = Object.keys(scopeMap);
+    const scopeMapKeys = Object.keys(scopeMap).filter((scope) => FeatureToggles._isValidScopeMapValue(scopeMap[scope]));
     if (scopeMapKeys.length === 0) {
       return SCOPE_ROOT_KEY;
     }
@@ -781,8 +809,8 @@ class FeatureToggles {
   // NOTE: there are multiple scopeMaps for every scopeKey with more than one inner entry. This will return the unique
   // scopeMap whose keys are sorted, i.e., matching the keys in the scopeKey.
   static getScopeMap(scopeKey) {
-    return typeof scopeKey !== "string" || scopeKey === SCOPE_ROOT_KEY
-      ? {}
+    return !this._isValidScopeKey(scopeKey) || scopeKey === undefined || scopeKey === SCOPE_ROOT_KEY
+      ? undefined
       : scopeKey.split(SCOPE_KEY_OUTER_SEPARATOR).reduce((acc, scopeInnerEntry) => {
           const [scopeInnerKey, value] = scopeInnerEntry.split(SCOPE_KEY_INNER_SEPARATOR);
           acc[scopeInnerKey] = value;
@@ -877,6 +905,7 @@ class FeatureToggles {
    */
   getFeatureValue(featureKey, scopeMap = undefined) {
     this._ensureInitialized();
+    scopeMap = FeatureToggles._sanitizeScopeMap(scopeMap);
     return FeatureToggles._getFeatureValueForScopeAndStateAndFallback(
       this.__superScopeCache,
       this.__stateScopedValues,
diff --git a/src/shared/static.js b/src/shared/static.js
@@ -11,7 +11,10 @@ const ENV = Object.freeze({
 
 const isNull = (...args) => args.reduce((result, arg) => result || arg === undefined || arg === null, false);
 
+const isObject = (input) => typeof input === "object" && input !== null;
+
 module.exports = {
   ENV,
   isNull,
+  isObject,
 };
diff --git a/test/featureToggles.test.js b/test/featureToggles.test.js
@@ -172,9 +172,9 @@ describe("feature toggles test", () => {
       ];
 
       let i = 0;
-      expect(FeatureToggles.getScopeMap(scopeKeys[i++])).toMatchInlineSnapshot(`{}`);
-      expect(FeatureToggles.getScopeMap(scopeKeys[i++])).toMatchInlineSnapshot(`{}`);
-      expect(FeatureToggles.getScopeMap(scopeKeys[i++])).toMatchInlineSnapshot(`{}`);
+      expect(FeatureToggles.getScopeMap(scopeKeys[i++])).toMatchInlineSnapshot(`undefined`);
+      expect(FeatureToggles.getScopeMap(scopeKeys[i++])).toMatchInlineSnapshot(`undefined`);
+      expect(FeatureToggles.getScopeMap(scopeKeys[i++])).toMatchInlineSnapshot(`undefined`);
       expect(FeatureToggles.getScopeMap(scopeKeys[i++])).toMatchInlineSnapshot(`
         {
           "tenantId": "t1",
@@ -380,14 +380,8 @@ describe("feature toggles test", () => {
       expect(await featureToggles.validateFeatureValue(...inputArgsList[i++])).toMatchInlineSnapshot(`
         [
           {
-            "errorMessage": "value "{0}" has invalid type {1}, must be {2}",
-            "errorMessageValues": [
-              1,
-              "number",
-              "boolean",
-            ],
+            "errorMessage": "scopeMap must be undefined or an object",
             "featureKey": "test/feature_a",
-            "scopeKey": "//",
           },
         ]
       `);
@@ -403,7 +397,6 @@ describe("feature toggles test", () => {
               "usr",
             ],
             "featureKey": "test/feature_aa",
-            "scopeKey": "usr::u1",
           },
         ]
       `);
@@ -415,7 +408,6 @@ describe("feature toggles test", () => {
               "Tenant",
             ],
             "featureKey": "test/feature_aa",
-            "scopeKey": "Tenant::t1",
           },
         ]
       `);
diff --git a/test/integration-local/__snapshots__/featureToggles.test.js.snap b/test/integration-local/__snapshots__/featureToggles.test.js.snap
@@ -93,6 +93,11 @@ exports[`local integration test after init getFeatureValues, getFeaturesInfos 1`
   },
   "test/feature_e": {
     "config": {
+      "ALLOWED_SCOPES": [
+        "component",
+        "layer",
+        "tenant",
+      ],
       "TYPE": "number",
       "VALIDATION": "^\\d{1}$",
     },
diff --git a/test/integration-local/featureToggles.test.js b/test/integration-local/featureToggles.test.js
@@ -48,6 +48,7 @@ const config = {
     fallbackValue: 5,
     type: "number",
     validation: "^\\d{1}$",
+    allowedScopes: ["component", "layer", "tenant"],
   },
   [FEATURE.F]: {
     fallbackValue: "best",
@@ -193,6 +194,51 @@ describe("local integration test", () => {
       expect(redisWrapperLoggerSpy.error).toHaveBeenCalledTimes(0);
     });
 
+    it("getFeatureValue with bad scopes", async () => {
+      const oldRootValue = 5;
+      const trapValue = 9;
+      const badScopeMap1 = null; // not an object
+      const badScopeMap2 = { tanent: "undefined" }; // scope that is not allowed
+      const badScopeMap3 = { tenant: undefined, layer: undefined };
+      const trapScopeMap = { tenant: "undefined", layer: "undefined" };
+
+      expect(await changeFeatureValue(FEATURE.E, trapValue, badScopeMap1)).toMatchInlineSnapshot(`
+        [
+          {
+            "errorMessage": "scopeMap must be undefined or an object",
+            "featureKey": "test/feature_e",
+          },
+        ]
+      `);
+      expect(await changeFeatureValue(FEATURE.E, trapValue, badScopeMap2)).toMatchInlineSnapshot(`
+        [
+          {
+            "errorMessage": "scope "{0}" is not allowed",
+            "errorMessageValues": [
+              "tanent",
+            ],
+            "featureKey": "test/feature_e",
+          },
+        ]
+      `);
+      expect(await changeFeatureValue(FEATURE.E, trapValue, badScopeMap3)).toMatchInlineSnapshot(`
+        [
+          {
+            "errorMessage": "scope "{0}" has invalid type {1}, must be string",
+            "errorMessageValues": [
+              "tenant",
+              "undefined",
+            ],
+            "featureKey": "test/feature_e",
+          },
+        ]
+      `);
+      expect(await changeFeatureValue(FEATURE.E, trapValue, trapScopeMap)).toMatchInlineSnapshot(`undefined`);
+      expect(getFeatureValue(FEATURE.E, badScopeMap1)).toEqual(oldRootValue);
+      expect(getFeatureValue(FEATURE.E, badScopeMap2)).toEqual(oldRootValue);
+      expect(getFeatureValue(FEATURE.E, badScopeMap3)).toEqual(oldRootValue);
+    });
+
     it("getFeatureValue, changeFeatureValue with scopes", async () => {
       const rootOldValue = getFeatureValue(FEATURE.E);
 
@@ -362,6 +408,15 @@ describe("local integration test", () => {
       expect(await validateFeatureValue(FEATURE.C, "", { tenant: "t1" })).toMatchInlineSnapshot(`[]`);
 
       // invalid
+      expect(await validateFeatureValue(FEATURE.C, "", null)).toMatchInlineSnapshot(`
+        [
+          {
+            "errorMessage": "scopeMap must be undefined or an object",
+            "featureKey": "test/feature_c",
+          },
+        ]
+      `);
+
       expect(await validateFeatureValue(FEATURE.C, "", { tenant: { subTenant: "bla" } })).toMatchInlineSnapshot(`
         [
           {
@@ -371,7 +426,6 @@ describe("local integration test", () => {
               "object",
             ],
             "featureKey": "test/feature_c",
-            "scopeKey": "tenant::[object Object]",
           },
         ]
       `);
@@ -384,7 +438,6 @@ describe("local integration test", () => {
               "object",
             ],
             "featureKey": "test/feature_c",
-            "scopeKey": "tenant::a,b,c",
           },
         ]
       `);
@@ -397,7 +450,6 @@ describe("local integration test", () => {
               "function",
             ],
             "featureKey": "test/feature_c",
-            "scopeKey": "tenant::() => "1"",
           },
         ]
       `);
