Avoid to submit empty params #735

andresmgot · 2018-10-16T16:37:57Z

Fixes #411

When creating a service instance or a binding, if the schema is provided, avoid to submit empty objects. This is only made when we are sure that the parameter is not required.

migmartri · 2018-10-17T09:40:12Z

dashboard/src/actions/catalog.test.tsx

+        },
+      },
+    };
+    // It omits "f" because it's empty but not "b" because it's required


Thanks for explanation.

What's the difference between sending empty required parameters and not sending them at all? Wouldn't not sending the empty parameters regardless of being required or not have the same effect?

Nope, that't the root of the issue of #411. The GCP service broker has some kind of validation that if the key exists it's not empty. We were sending a subscription object with all the inner elements empty (but with keys). With that the broker replies 'roles/pubsub.subscriber' is a required role when 'subscription' is specified.

right, my comment was related to not to send the parameters if they are empty in any case. The difference is that you still send them if they are required params but I am suggesting not to send them even in that case. I might be missing something though.

okay, yes, it's true that it should be safe to not send anything that is empty.

migmartri · 2018-10-17T09:41:33Z

dashboard/src/actions/catalog.ts

@@ -60,16 +62,44 @@ const actions = [

 export type ServiceCatalogAction = ActionType<typeof actions[number]>;

+function isEmptyDeep(obj: any): boolean {


Please add a description of what this function does. Mainly because it seems to have a recursive behavior.

migmartri · 2018-10-17T09:43:03Z

dashboard/src/actions/catalog.ts

@@ -60,16 +62,44 @@ const actions = [

 export type ServiceCatalogAction = ActionType<typeof actions[number]>;

+function isEmptyDeep(obj: any): boolean {
+  if (typeof obj === "number") {


why is number a special case? What about string and others?

it's a lodash problem:

https://github.com/lodash/lodash/blob/4ea8c2ec249be046a0f4ae32539d652194caf74f/isEmpty.js#L33

In our case, we want to mark a number as non empty so we need this special case. It's not critical though because you cannot set a "number" in a form (AFAIK) but just in case.

migmartri · 2018-10-17T09:43:47Z

dashboard/src/actions/catalog.ts

+  return _.isEmpty(obj);
+}
+
+function removeEmptyFields(obj: object, schema?: JSONSchema6) {


maybe rename to removeEmptyNonRequiredFields?

migmartri · 2018-10-17T09:46:22Z

dashboard/src/actions/catalog.ts

@@ -60,16 +62,44 @@ const actions = [

 export type ServiceCatalogAction = ActionType<typeof actions[number]>;

+function isEmptyDeep(obj: any): boolean {


I know that we are testing the overall behavior at the action level but could we add unit tests for this function?

I would avoid that because that would require to export the function. This is supposed to be a inner function so I prefer to test this with the exposed function.

I don't know. There should be a way to unit test functions without exporting them, otherwise we are not unit testing.

Can't we make it work like this?

wrapper = shallow("foo") instance = wrapper.instance(); instance.isEmptyDeep(...) ?

The thing is that this function is complex enough too deserve its unit test. Relying on functional/integration test might not be the best option once the complexity starts growing.

moving this function to a different helper to be able to test it

migmartri · 2018-10-17T09:47:45Z

dashboard/src/actions/catalog.ts

 ): ThunkAction<Promise<boolean>, IStoreState, null, ServiceCatalogAction> {
  return async dispatch => {
    try {
-      await ServiceInstance.create(releaseName, namespace, className, planName, parameters);
+      const filteredParams = removeEmptyFields(parameters, schema);


maybe we do not need to pass schema all the time since it is actually optional and with default value to JSONSchema6? Or do we support other schemas?

I don't understand what you mean here

My bad, I was understanding schema like the schema type and thought that the default value was JSONSchema6. When what it actually does it to receive an instance of jsonschema.

migmartri · 2018-10-17T14:30:31Z

dashboard/src/actions/catalog.ts

 export function provision(
  releaseName: string,
  namespace: string,
  className: string,
  planName: string,
  parameters: {},
+  schema?: JSONSchema6,


Why is schema optional?

migmartri · 2018-10-17T14:32:34Z

dashboard/src/actions/catalog.test.tsx

+        },
+      },
+    };
+    // It omits "f" because it's empty but not "b" because it's required


right, my comment was related to not to send the parameters if they are empty in any case. The difference is that you still send them if they are required params but I am suggesting not to send them even in that case. I might be missing something though.

migmartri

This is great, thanks for the changes :)

migmartri · 2018-10-18T08:26:34Z

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 not required", async () => {


Is that description accurate?

filters submitted params if they are empty?

it's not, I missed this

migmartri · 2018-10-18T08:27:08Z

dashboard/src/actions/catalog.test.tsx

@@ -227,6 +261,41 @@ describe("addBinding", () => {
    await store.dispatch(provisionCMD);
    expect(store.getActions()).toEqual(expectedActions);
  });
+
+  it("filters the submitted parameters if they are not required", async () => {


migmartri · 2018-10-18T08:27:36Z

dashboard/src/actions/catalog.ts

@@ -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);


migmartri · 2018-10-18T08:28:03Z

dashboard/src/actions/helpers/object.test.tsx

@@ -0,0 +1,34 @@
+import * as object from "./object";
+
+describe("isEmptyDeep", () => {


migmartri · 2018-10-18T08:29:30Z

dashboard/src/actions/helpers/object.test.tsx

+    { object: { a: 1 }, isEmpty: false },
+    { object: { a: { b: "" } }, isEmpty: true },
+    { object: { a: { c: { d: "" } } }, isEmpty: true },
+    { object: { a: { c: { d: "a" } } }, isEmpty: false },


what about an example that contains both an empty and non-empty at the leaf?

{ object: { a: { c: { d: "a" }, d: "" } }, isEmpty: false },

migmartri · 2018-10-18T08:30:08Z

dashboard/src/actions/helpers/object.ts

@@ -0,0 +1,30 @@
+import * as _ from "lodash";
+
+// Check if all the keys of an object are empty. If any value of the


Thank you for the description :)

migmartri · 2018-10-18T08:30:49Z

dashboard/src/actions/helpers/object.ts

+// is empty.
+export function isEmptyDeep(obj: any): boolean {
+  if (typeof obj === "number") {
+    // isEmpty(number) is true but it's not empty


Could you mention that it is a limitation of lodash?

Avoid to submit empty params

ddc82de

migmartri reviewed Oct 17, 2018

View reviewed changes

Documentation changes

62b8055

migmartri reviewed Oct 17, 2018

View reviewed changes

Andres and others added 2 commits October 17, 2018 17:50

Delete any empty key. Move object functions to a helper

87e4612

Merge branch 'master' into emptyParams

68fff26

migmartri approved these changes Oct 18, 2018

View reviewed changes

Improve descriptions

b83ed79

andresmgot merged commit 1dc3a2b into vmware-tanzu:master Oct 18, 2018

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Avoid to submit empty params #735

Avoid to submit empty params #735

andresmgot commented Oct 16, 2018

migmartri Oct 17, 2018

andresmgot Oct 17, 2018

migmartri Oct 17, 2018

andresmgot Oct 17, 2018

migmartri Oct 17, 2018

migmartri Oct 17, 2018

andresmgot Oct 17, 2018

migmartri Oct 17, 2018

migmartri Oct 17, 2018

andresmgot Oct 17, 2018

migmartri Oct 17, 2018

andresmgot Oct 17, 2018

migmartri Oct 17, 2018

andresmgot Oct 17, 2018

migmartri Oct 17, 2018

migmartri Oct 17, 2018

migmartri Oct 17, 2018

migmartri left a comment

migmartri Oct 18, 2018

andresmgot Oct 18, 2018

migmartri Oct 18, 2018

migmartri Oct 18, 2018

migmartri Oct 18, 2018

migmartri Oct 18, 2018

migmartri Oct 18, 2018

migmartri Oct 18, 2018

		@@ -60,16 +62,44 @@ const actions = [

		export type ServiceCatalogAction = ActionType<typeof actions[number]>;

		function isEmptyDeep(obj: any): boolean {

		@@ -0,0 +1,34 @@
		import * as object from "./object";

		describe("isEmptyDeep", () => {

		@@ -0,0 +1,30 @@
		import * as _ from "lodash";

		// Check if all the keys of an object are empty. If any value of the

Avoid to submit empty params #735

Avoid to submit empty params #735

Conversation

andresmgot commented Oct 16, 2018

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

migmartri left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment