Corrected defaulting & non-required empty property handling by dropdevcoding · Pull Request #542 · rjsf-team/react-jsonschema-form

dropdevcoding · 2017-04-10T14:25:08Z

Reasons for making this change

In the current version imho the defaulting is broken:

When having a non required array property minItems will be set though. Behavior should be: When required, minItems will be applied, when not required and no defaults are set, minItems should not be applied

The value handling imho also is broken for non required fields:

When having defined an object which is not required but has required properties, the following should apply:
- When one of the object's property is set the invariant rules shall apply
- When no property of the object is set the whole object should be undefined since it is not required because otherwise an empty object is left in formData and the invariant rules will be applied which causes an incorrect validation error
When deleting the last item of a non required array, the following should apply:
- When the last element of a non required array is removed, the whole array should be set to undefined, otherwise the array is left empty in formData and eventual invariants like minItems would be applied and cause validation errors although they should not in this case

If this is related to existing tickets, include links to them as well.

#465

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

glasserc

Hi! Thanks for your contribution! Sorry it took me so long to get to this. In thinking about it, I do agree that there are some issues with defaults and non-requried property handling, but I'm not completely sure about the fixes you propose. I'm concerned about the increased complexity here, where adding required now doubles the possible code paths through all these functions. More test cases would make me feel better, but I only see one.

Here's an example of the kind of edge case I'm concerned about. With this patch applied, I tried this schema:

{
  "type": "object",
  "definitions": {
    "Thing": {
      "type": "object",
      "properties": {
        "name": {
          "type": "string"
        }
      }
    }
  },
  "properties": {
    "foo": {
      "type": "array",
      "minItems": 2,
      "items": {
        "$ref": "#/definitions/Thing"
      }
    }
  }
}

(based on the test case you added). Then, I clicked the "Add" button twice, and then I clicked one "X" button once. I'd expect to have one element left in the array afterwards, but both of them will be removed.

I think this PR should mostly focus on validation/filling in defaults without trying to get too fancy in the UX. But I'd be willing to be convinced if you find that we can't do validation without updating formData or something like that.

glasserc · 2017-04-24T20:15:55Z

src/components/fields/ArrayField.js

      // refs #195: revalidate to ensure properly reindexing errors
-      onChange(formData.filter((_, i) => i !== index), { validate: true });
+
+      formData = formData ? formData.filter((_, i) => i !== index) : formData;


Shouldn't we be doing this filter before the if (!required) check?

glasserc · 2017-04-24T20:31:18Z

src/components/fields/ArrayField.js

    return value => {
      const { formData, onChange } = this.props;
-      const newFormData = formData.map((item, i) => {
+      let newFormData = formData.map((item, i) => {


Nit: Does this still need to be let?

glasserc · 2017-04-24T20:50:51Z

src/utils.js

+      if (required && schema.minItems) {
        return new Array(schema.minItems).fill(
-          computeDefaults(schema.items, defaults, definitions)
+          computeDefaults(schema.items, defaults, definitions, required)


Should schema.required be relevant here?

glasserc · 2017-04-24T20:51:32Z

src/utils.js

+
+export function cleanUpNonRequiredArray(values) {
+  values = values.filter(item => {
+    return item !== null && item !== undefined;


Why do we need to filter out undefined values here?

glasserc · 2017-04-24T20:52:33Z

src/utils.js

 export function mergeObjects(obj1, obj2, concatArrays = false) {
  // Recursively merge deeply nested objects.
+  obj1 = obj1 || {};
+  obj2 = obj2 || {};


What is this change about?

dropdevcoding · 2017-04-25T08:05:52Z

Okay, I'll sum up the reasons for the changes:
Having given a schema like

{
  "type": "object"
  "properties": {
   "nonRequiredOuterProp": {
     "type": "object",
     "properties": {
       "requiredInnerProp1": {
         "type": "string"
       },
       "requiredInnerProp2": {
         "type":"array",
         "items": {
           "type": "string"
         }
       }
     },
     "required": [
      "requiredInnerProp1",
      "requiredInnerProp2"
     ]
   }
  }
}

Which means nonRequiredOuterProp may be undefined or if set must match the given criteria,
the form doesn't allow the user to restore the undefined state.

As soon as I fill in requiredInnerProp1 and requiredInnerProp2 the object turns from undefined to

{
   "nonRequiredOuterProp": {
     "requiredInnerProp1": "myInputForRequiredInnerProp1",
     "requiredInnerProp2": "myInputForRequiredInnerProp2"
}

which is correct, but when I remove the whole input the objects falls back to {} which is not valid, since requiredInnerProp1and requiredInnerProp2are not defined but required for nonRequiredPropwhere nonRequiredProp is not required at all.

So the expected behaviour is:
When an object in NOT required, it collapses to undefined (initial state) if none of its properties is defined. Otherwise validation would fail (possibly jsonschemadoes not handle this case right. I also introduced ajvsince I use it for backend validation).

As soon as the user fills out a property the empty object remains and the user hasn't got any chance to transfer it to initial state.

Same behaviour applies to non required arrays.

I hope this will help to clarify. If you got further questions, please don't hesitate to ask :-)

eggyal · 2017-04-28T05:09:40Z

If I've understood this issue correctly, then I think it's also fixed by #557? In particular 9d0b741 and 132bc60?

glasserc · 2017-06-20T20:32:35Z

Hi @dropdevcoding, thanks for explaining the reasoning behind the PR. What do you think about my objections to the behavior of the changes, and my comments on the code review?

epicfaace · 2019-12-22T06:02:29Z

Closing this PR due to inactivity. If you'd still like to get this PR in, please make a new PR rebased to the latest code. Thanks!

dropdevcoding added 3 commits April 10, 2017 11:21

corrected defaulting and removal behavior for non required fields

6163404

moved cleanup functions for non required structures to utils

5fa5af8

readded temporarily removed formData proptype in arrayfield

0e4c984

glasserc requested changes Apr 24, 2017

View reviewed changes

gdbassett mentioned this pull request Jul 14, 2017

Question: hide/show functionality for large forms? #598

Closed

2 tasks

glasserc mentioned this pull request Aug 28, 2017

Fix optional object with required fields #682

Closed

9 tasks

epicfaace closed this Dec 22, 2019

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Uh oh!

Corrected defaulting & non-required empty property handling#542

Corrected defaulting & non-required empty property handling#542
dropdevcoding wants to merge 3 commits intorjsf-team:masterfrom
dropdevcoding:master

dropdevcoding commented Apr 10, 2017

Uh oh!

glasserc left a comment

Uh oh!

glasserc Apr 24, 2017

Uh oh!

glasserc Apr 24, 2017

Uh oh!

glasserc Apr 24, 2017

Uh oh!

glasserc Apr 24, 2017

Uh oh!

glasserc Apr 24, 2017

Uh oh!

dropdevcoding commented Apr 25, 2017

Uh oh!

eggyal commented Apr 28, 2017

Uh oh!

glasserc commented Jun 20, 2017

Uh oh!

epicfaace commented Dec 22, 2019

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

4 participants

Uh oh!

Conversation

dropdevcoding commented Apr 10, 2017

Reasons for making this change

Checklist

Uh oh!

glasserc left a comment

Choose a reason for hiding this comment

Uh oh!

glasserc Apr 24, 2017

Choose a reason for hiding this comment

Uh oh!

glasserc Apr 24, 2017

Choose a reason for hiding this comment

Uh oh!

glasserc Apr 24, 2017

Choose a reason for hiding this comment

Uh oh!

glasserc Apr 24, 2017

Choose a reason for hiding this comment

Uh oh!

glasserc Apr 24, 2017

Choose a reason for hiding this comment

Uh oh!

dropdevcoding commented Apr 25, 2017

Uh oh!

eggyal commented Apr 28, 2017

Uh oh!

glasserc commented Jun 20, 2017

Uh oh!

epicfaace commented Dec 22, 2019

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

4 participants