Avoid to submit empty params (#735)

* Avoid to submit empty params * Documentation changes * Delete any empty key. Move object functions to a helper * Improve descriptions
vmware-tanzu · Oct 18, 2018 · 1dc3a2b · 1dc3a2b
1 parent a29ec5c
commit 1dc3a2b
Show file tree

Hide file tree

Showing 7 changed files with 152 additions and 3 deletions.
diff --git a/dashboard/package.json b/dashboard/package.json
@@ -5,6 +5,7 @@
   "dependencies": {
     "@types/js-yaml": "^3.10.1",
     "@types/json-schema": "^6.0.1",
+    "@types/lodash": "^4.14.117",
     "@types/moxios": "^0.4.8",
     "@types/pako": "^1.0.0",
     "@types/qs": "^6.5.1",
@@ -15,6 +16,7 @@
     "connected-react-router": "^4.5.0",
     "enzyme-adapter-react-16": "^1.1.1",
     "js-yaml": "^3.10.0",
+    "lodash": "^4.17.11",
     "protobufjs": "^6.8.4",
     "qs": "^6.5.2",
     "raf": "^3.4.0",

diff --git a/dashboard/src/actions/catalog.test.tsx b/dashboard/src/actions/catalog.test.tsx
@@ -165,6 +165,40 @@ describe("provision", () => {
     await store.dispatch(provisionCMD);
     expect(store.getActions()).toEqual(expectedActions);
   });
+
+  it("filters the submitted parameters if they are empty", async () => {
+    const params = {
+      a: 1,
+      f: {
+        g: [],
+        h: {
+          i: "",
+        },
+      },
+      j: {
+        k: {
+          l: "m",
+        },
+      },
+    };
+    // It omits "f" because it's empty but not "a" or "j"
+    const expectedParams = { a: 1, j: { k: { l: "m" } } };
+    const cmd = catalogActions.provision(
+      testArgs.releaseName,
+      testArgs.namespace,
+      testArgs.className,
+      testArgs.planName,
+      params,
+    );
+    await store.dispatch(cmd);
+    expect(ServiceInstance.create).toHaveBeenCalledWith(
+      testArgs.releaseName,
+      testArgs.namespace,
+      testArgs.className,
+      testArgs.planName,
+      expectedParams,
+    );
+  });
 });
 
 describe("deprovision", () => {
@@ -227,6 +261,41 @@ describe("addBinding", () => {
     await store.dispatch(provisionCMD);
     expect(store.getActions()).toEqual(expectedActions);
   });
+
+  it("filters the submitted parameters if they are empty", async () => {
+    const params = {
+      a: 1,
+      b: {
+        c: [],
+      },
+      f: {
+        g: [],
+        h: {
+          i: "",
+        },
+      },
+      j: {
+        k: {
+          l: "m",
+        },
+      },
+    };
+    // It omits "f" because it's empty but not "a" or "j"
+    const expectedParams = { a: 1, j: { k: { l: "m" } } };
+    const cmd = catalogActions.addBinding(
+      testArgs.bindingName,
+      testArgs.instanceName,
+      testArgs.namespace,
+      params,
+    );
+    await store.dispatch(cmd);
+    expect(ServiceBinding.create).toHaveBeenCalledWith(
+      testArgs.bindingName,
+      testArgs.instanceName,
+      testArgs.namespace,
+      expectedParams,
+    );
+  });
 });
 
 describe("removeBinding", () => {

diff --git a/dashboard/src/actions/catalog.ts b/dashboard/src/actions/catalog.ts
@@ -7,6 +7,7 @@ import { IServiceBindingWithSecret, ServiceBinding } from "../shared/ServiceBind
 import { IServiceBroker, IServicePlan, ServiceCatalog } from "../shared/ServiceCatalog";
 import { IServiceInstance, ServiceInstance } from "../shared/ServiceInstance";
 import { IStoreState } from "../shared/types";
+import helpers from "./helpers";
 
 export const checkCatalogInstall = createAction("CHECK_INSTALL");
 export const installed = createAction("INSTALLED");
@@ -69,7 +70,8 @@ export function provision(
 ): ThunkAction<Promise<boolean>, IStoreState, null, ServiceCatalogAction> {
   return async dispatch => {
     try {
-      await ServiceInstance.create(releaseName, namespace, className, planName, parameters);
+      const filteredParams = helpers.object.removeEmptyFields(parameters);
+      await ServiceInstance.create(releaseName, namespace, className, planName, filteredParams);
       return true;
     } catch (e) {
       dispatch(errorCatalog(e, "create"));
@@ -86,7 +88,8 @@ export function addBinding(
 ): ThunkAction<Promise<boolean>, IStoreState, null, ServiceCatalogAction> {
   return async dispatch => {
     try {
-      await ServiceBinding.create(bindingName, instanceName, namespace, parameters);
+      const filteredParams = helpers.object.removeEmptyFields(parameters);
+      await ServiceBinding.create(bindingName, instanceName, namespace, filteredParams);
       return true;
     } catch (e) {
       dispatch(errorCatalog(e, "create"));

diff --git a/dashboard/src/actions/helpers/index.ts b/dashboard/src/actions/helpers/index.ts
@@ -0,0 +1,5 @@
+import * as object from "./object";
+
+export default {
+  object,
+};
diff --git a/dashboard/src/actions/helpers/object.test.tsx b/dashboard/src/actions/helpers/object.test.tsx
@@ -0,0 +1,35 @@
+import * as object from "./object";
+
+describe("isEmptyDeep", () => {
+  [
+    { object: {}, isEmpty: true },
+    { object: { a: 1 }, isEmpty: false },
+    { object: { a: { b: "" } }, isEmpty: true },
+    { object: { a: { c: { d: "" } } }, isEmpty: true },
+    { object: { a: { c: { d: "a" } } }, isEmpty: false },
+    { object: { a: { c: { d: [] } } }, isEmpty: true },
+    { object: { a: { c: { d: {} } } }, isEmpty: true },
+    { object: { a: { c: { d: "a" }, d: "" } }, isEmpty: false },
+  ].forEach(t => {
+    it(`${JSON.stringify(t.object)} should be ${t.isEmpty ? "empty" : "not empty"}`, () => {
+      expect(object.isEmptyDeep(t.object)).toBe(t.isEmpty);
+    });
+  });
+});
+
+describe("removeEmptyField", () => {
+  [
+    { in: {}, out: {} },
+    { in: { a: 1 }, out: { a: 1 } },
+    { in: { a: { b: "" } }, out: {} },
+    { in: { a: { b: "" }, c: 1 }, out: { c: 1 } },
+    { in: { a: { c: { d: "" } } }, out: {} },
+    { in: { a: { c: { d: "a" } } }, out: { a: { c: { d: "a" } } } },
+    { in: { a: { c: { d: [] } } }, out: {} },
+    { in: { a: { c: { d: {} } } }, out: {} },
+  ].forEach(t => {
+    it(`${JSON.stringify(t.in)} should be simplified to ${JSON.stringify(t.out)}`, () => {
+      expect(object.removeEmptyFields(t.in)).toEqual(t.out);
+    });
+  });
+});
diff --git a/dashboard/src/actions/helpers/object.ts b/dashboard/src/actions/helpers/object.ts
@@ -0,0 +1,31 @@
+import * as _ from "lodash";
+
+// Check if all the keys of an object are empty. If any value of the
+// object is a nested object it recursively checks if the inner object
+// is empty.
+export function isEmptyDeep(obj: any): boolean {
+  if (typeof obj === "number") {
+    // lodash function isEmpty(number) return true
+    // but we should not consider it empty
+    return false;
+  }
+  if (typeof obj === "object" && !_.isEmpty(obj)) {
+    // Check if nested objects are empty
+    // If some of the keys are not empty the result is not empty
+    return !Object.keys(obj).some(k => {
+      return !isEmptyDeep(obj[k]);
+    });
+  }
+  return _.isEmpty(obj);
+}
+
+// Remove empty keys from an object (recursively)
+export function removeEmptyFields(obj: object) {
+  const res = { ...obj };
+  Object.keys(res).forEach(k => {
+    if (isEmptyDeep(res[k])) {
+      delete res[k];
+    }
+  });
+  return res;
+}
diff --git a/dashboard/yarn.lock b/dashboard/yarn.lock
@@ -202,6 +202,10 @@
   version "6.0.1"
   resolved "https://registry.yarnpkg.com/@types/json-schema/-/json-schema-6.0.1.tgz#a761975746f1c1b2579c62e3a4b5e88f986f7e2e"
 
+"@types/lodash@^4.14.117":
+  version "4.14.117"
+  resolved "https://registry.yarnpkg.com/@types/lodash/-/lodash-4.14.117.tgz#695a7f514182771a1e0f4345d189052ee33c8778"
+
 "@types/long@^4.0.0":
   version "4.0.0"
   resolved "https://registry.yarnpkg.com/@types/long/-/long-4.0.0.tgz#719551d2352d301ac8b81db732acb6bdc28dbdef"
@@ -5473,7 +5477,7 @@ lodash@^4.0.0, lodash@^4.17.10, lodash@~4.17.10:
   version "4.17.10"
   resolved "https://registry.yarnpkg.com/lodash/-/lodash-4.17.10.tgz#1b7793cf7259ea38fb3661d4d38b3260af8ae4e7"
 
-lodash@^4.13.1, lodash@^4.15.0, lodash@^4.17.4, lodash@^4.17.5:
+lodash@^4.13.1, lodash@^4.15.0, lodash@^4.17.11, lodash@^4.17.4, lodash@^4.17.5:
   version "4.17.11"
   resolved "https://registry.yarnpkg.com/lodash/-/lodash-4.17.11.tgz#b39ea6229ef607ecd89e2c8df12536891cac9b8d"