Fix optional object with required fields by davidMcneil · Pull Request #682 · rjsf-team/react-jsonschema-form

davidMcneil · 2017-08-26T20:54:21Z

Reasons for making this change

The library would require an optional object if it had required fields. (#675)

Note, this change does not take into account the HTML5 required property. As such I added a "HTML5 Validation" checkbox to the example app to allow for turning off HTML5 validation.

Checklist

I'm updating documentation
- I've checked the rendering of the Markdown text I've added
- If I'm adding a new section, I've updated the Table of Content
I'm adding or updating code
- I've added and/or updated tests
- I've updated docs if needed
- I've run npm run cs-format on my branch to conform my code to prettier coding style
I'm adding a new feature
- I've updated the playground with an example use of the feature

n1k0

Looks good though I suspect a few optimizations could be made here and there. I'd like another pass of review from @glasserc if he doesn't mind :)

n1k0 · 2017-08-28T14:27:59Z

playground/app.js

  };

-  setLiveValidate = ({ formData }) => this.setState({ liveValidate: formData });
+  setValidate = ({ formData }) => this.setState({ ...formData });


Just call setState(formData) here. You may even want to try using onChange={this.setState} later.

n1k0 · 2017-08-28T14:29:57Z

src/utils.js

+    }
+  }
+  if (objectNeedsTrimmed(result)) {
+    result = undefined;


Nit: just return;

n1k0 · 2017-08-28T14:31:20Z

test/utils_test.js

    });

+    describe("trimObject()", () => {
+      it("should't mutate the provided objects", () => {


Nit: shouldn't

n1k0 · 2017-08-28T14:32:08Z

test/utils_test.js

+      it("should trim empty object", () => {
+        const obj = {};
+        const res = trimObject(obj);
+        expect(res).eql(undefined);


Please ensure using expect(res).to.be.a("undefined") for better accuracy.

n1k0 · 2017-08-28T14:33:30Z

src/utils.js

  }, acc);
 }

+function objectNeedsTrimmed(obj) {


How about objectNeedsTrimming?

n1k0 · 2017-08-28T14:34:21Z

src/utils.js


+function objectNeedsTrimmed(obj) {
+  // Recursively determine if an object needs to be simplified to `undefined`.
+  return Object.keys(obj).every(key => {


Shouldn't we use some instead of every here?

I probably needed to do a better job explaining this in the PR, but we need to convert objects with all of their fields set to undefined to simply undefined and do this recursively. For example, convert

{ "a": true, "b": { "c": undefined, "d": { "e": undefined} } } to {"a": true, "b": undefined}

The reason is, assuming we have a schema where b is optional but its fields are required, if b is set to an object the validator requires all of its required fields to be set. So we must "trim" down its unspecified fields and b itself to simply undefined. The every is requiring all of an objects fields are undefined to do a "trim". I do not believe setting it to some would work because the user would be unable to set the fields of an object (the entire object would keep being set to undefined) unless they were somehow able to simultaneously set all fields at once. Thanks for the feedback!

n1k0 · 2017-08-28T14:35:49Z

test/utils_test.js

+        const res = trimObject(obj);
+        expect(res).eql({ a: [] });
+      });
+    });


I'd like to see more functional tests added at the component level for this behavior.

n1k0 · 2017-08-28T14:44:04Z

src/utils.js

+    if (result.hasOwnProperty(key) && isObject(result[key])) {
+      result[key] = trimObject(result[key]);
+    }
+  }


We may want to benchmark this, but I wonder how much slower would be to just use JSON.parse(JSON.stringify(obj)). I've read that these fns were heavily optimized in modern js engines. That would give us:

Deep cloning for free,

Getting rid of the objectNeedsTrimmed function call.

I am not sure simply calling JSON.parse(JSON.stringify(obj)) would work I tried it and it converts {"a": true, "b": {"c": undefined, "d": {"e": undefined}}} to { "a": true, "b": {"d": {}}} and I believe we would need it to convert it to { "a": true, "b":undefined}. That being said we could now simply remove empty objects and get the same behavior, but we still have to walk through the fields of the object recursively.

glasserc · 2017-08-28T15:34:54Z

I'll take a look in a second, but before I forget, I want to note that this is related to #542 (and through it, #465 and #331).

glasserc

This looks OK to me. The main concern I have is what happens when we trim a required object, since we do so without any regard to whether the object is required or not. In particular, I think it's impossible in the current iteration to have an empty required object (i.e. a required object full of optional fields) as part of a valid input. In other words, I agree with @n1k0 's suggestion to add more functional tests at the component level.

glasserc · 2017-08-28T16:00:21Z

src/utils.js

+    result = formData || defaults;
  }
-  return formData || defaults;
+  return trimFormData(result);


This means that we trim defaults, which feels like it could be a bit surprising?

glasserc · 2017-08-28T16:06:26Z

test/utils_test.js

+      it("should trim field object", () => {
+        const obj = { a: true, o: { b: undefined } };
+        const res = trimObject(obj);
+        expect(res).eql({ a: true, o: undefined });


This might be a bit pedantic, but why return an object with undefined s in it, and not e.g. {a: true}? (Of course, this would require changes to your implementation.

davidMcneil · 2017-08-30T18:19:14Z

@glasserc You are absolutely right this does not handle the case where we have a required object with all optional fields. With the schema below, assuming neither field1 or field2 are set, I believe all the relevant cases we need to handle are listed in the table.

{
  "type": "object",
  "properties": {
    "object": {
      "type": "object",
      "properties": {
        "field1": {
          "type": "string"
        },
        "field2": {
          "type": "string"
        }
      }
    }
  }
}

`object`	`field1` and `field2`	submit
required	none required	`{object: {}}`
optional	none required	`{object: {}}` or `{}` ?
required	some required	invalid
optional	some required	`{}`

I would appreciate any feedback on the "correct/best" way to handle the second state, optional object and optional fields, as well as thoughts on how best to implement this fix. I do not believe it is possible to simply "trim" the object by solely looking at the current formData the way the current state of this PR does. Somehow we need to know whether or not a field is required. Is it acceptable to simply pass in the schema to trimFormData and walk the schema as I walk the formData and check if a field is required or not before trimming it? Or is there a better way to do this?

glasserc · 2017-09-06T16:43:05Z

My only real concern with trimming entire objects is that the error messages make sense. If you have a required object with required fields, I think the error messages belong on the required fields ("phone number is a required field") and not the containing object ("user is a required field"). So for me, I'm most concerned about the 3rd case. As for the second case, I think a user might expect either one depending on the application. I think what we should do here is maintain backwards compatibility (so {object: {}}). If someone complains then maybe we can add an option. But I don't want to add too much complexity at once, especially if we aren't sure we need it.

I don't have a strong opinion on how to implement it. I guess you have to examine both the schema (to determine whether the field is required) and the formData (to see what data is already there). Doing that in trimFormData seems fine to me architecturally, but that's without looking at any resulting code.

gdbassett · 2017-10-23T21:20:33Z

I would suggest {} for the 3rd case as the existence of object in the formData may imply information.

For case 3, it is the lack of object that appears to be the error, rather than the lack of field1 or field2. As such I'd think the error message would reflect "user is a required field" rather than "phone number".

mirajp · 2019-01-08T22:41:52Z

Any updates on this PR?
@davidMcneil @glasserc

epicfaace · 2019-01-21T00:09:53Z

@davidMcneil , it looks like you deleted the repository / made it private from which this PR was started from, so I can't review your changes.

epicfaace · 2019-03-16T21:19:58Z

I'm closing this PR due to the above issue, but @davidMcneil please feel free to make another PR with your actual changes.

n1k0 reviewed Aug 28, 2017

View reviewed changes

glasserc reviewed Aug 28, 2017

View reviewed changes

glasserc mentioned this pull request Sep 22, 2017

Easier Bootstrap Grid usage and basic BS4 compatibility #623

Closed

9 tasks

davidMcneil mentioned this pull request Oct 23, 2017

Optional object with required fields in react-jsonschema-form #675

Closed

2 tasks

Check if object in mergeObjects

9c6b104

glasserc mentioned this pull request Nov 9, 2017

Problem with nested object having a required property #740

Closed

epicfaace added the awaiting changes label Jan 21, 2019

epicfaace closed this Mar 16, 2019

numical mentioned this pull request Mar 19, 2019

trim empty objects to allow correct validation of optional objects with mandatory fields #1228

Closed

5 tasks

azertyfun mentioned this pull request Jun 10, 2022

[updated] trim empty objects to allow correct validation of optional objects with mandatory fields #2879

Closed

8 tasks

Uh oh!

Conversation

davidMcneil commented Aug 26, 2017 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Reasons for making this change

Checklist

Uh oh!

n1k0 left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

glasserc commented Aug 28, 2017

Uh oh!

glasserc left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

davidMcneil commented Aug 30, 2017

Uh oh!

glasserc commented Sep 6, 2017 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

gdbassett commented Oct 23, 2017

Uh oh!

mirajp commented Jan 8, 2019 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

epicfaace commented Jan 21, 2019

Uh oh!

epicfaace commented Mar 16, 2019

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

6 participants

davidMcneil commented Aug 26, 2017 •

edited

Loading

glasserc commented Sep 6, 2017 •

edited

Loading

mirajp commented Jan 8, 2019 •

edited

Loading