getDefaultFormState populates default data to whole formData #579

olzraiti · 2017-05-16T09:13:21Z

Reasons for making this change

getDefaultFormState didn't populate formData with existing array items inside object properties with default data, and therefore wasn't working correctly with deeply nested data. I'm sorry computeDefaults had to be rewritten - I had to add formData to the parameters.

Since the logic is pretty complex and I had to rewrite it, feel free to suggest tests to add.

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

… descriptions & titles.

…onschema-form

non-defined properties should probably be out of scope) This reverts commit 742c679.

glasserc

Thanks for your contribution! Sorry it took me so long to look at it -- I was intimidated by the number of commits 😊

I'm glad you tackled this. I think you're exposing some very complicated and even strange behavior regarding the use of default. For example, we have an existing test "should keep parent defaults if they don't have a node level default" that I sort of disagree with. I think we need a good, clear, crisp definition of when we fill in defaults. To my mind, if a schema has a default, we use that default only when the field is completely missing. However, the test I mentioned fills in a schema's children even though the field is present!

Similarly, I am a little confused about when parentDefaults enter into play and when they override the child's defaults. If I have {default: {"a": 1}, properties: {"a": {default: 4}}}, my intuition is that the computed default should be {"a": 1} -- you fill in the default for the object, and then check its children. But that's at odds with your implementation, and I could be swayed to an alternate viewpoint/strategy, especially if it has a clear summary.

How about tests for:

An array with both a default value, and items which have a default value.
Arrays with fewer items than minItems successfully get filled from the defaults
Arrays with fixed fields (items is an array) as well as default
Arrays with a default value and a $ref that points to something with a default value
Some of the above for objects too :)
Objects with a default value that contains nested children as well as a $ref that points to something with a default value that also has nested children

glasserc · 2017-06-09T16:55:15Z

src/utils.js

+            schema.items[i] &&
+            "type" in schema.items[i]
+            ? schema.items[i]
+            : schema.items;


If schema.items is an array, but it isn't long enough, or its "type" is missing from one element, you will use the whole array as the child schema.

olzraiti · 2017-06-13T22:24:04Z

Thanks for your contribution! Sorry it took me so long to look at it -- I was intimidated by the number of commits 😊

Hehe yeah, for some reason all my pull requests show all the commits I've made combined in my previous pull requests.

My hands hands pretty full at the moment, so this will have to wait. I'll dig deeper into your feedback later - hopefully sometime next month. Thanks for the input :-)

…est cases for array default data filling.

…sts cases

olzraiti · 2017-08-02T10:33:39Z

I found some issues when writing the tests. Local schema defaults weren't used if there was a $ref schema. Also defaults weren't filled properly when there was array formData with less items than minItems. additionalItems defaults weren't used.

The fixes broke one test - the logic is indeed inconsistent at the moment. Now ArrayField List of inputs should honor given formData, even when it does not meet the minItems-requirement is failing, and it is conflicting with should fill defaults to an array with less items than minItems which is succeeding. I didn't update the former yet, since this needs some discussion.

I'd remove the ArrayField List of inputs should honor given formData, even when it does not meet the minItems-requirement, since I think its also conflicting with #434.

I'm glad you tackled this. I think you're exposing some very complicated and even strange behavior regarding the use of default. For example, we have an existing test "should keep parent defaults if they don't have a node level default" that I sort of disagree with. I think we need a good, clear, crisp definition of when we fill in defaults. To my mind, if a schema has a default, we use that default only when the field is completely missing. However, the test I mentioned fills in a schema's children even though the field is present!

Similarly, I am a little confused about when parentDefaults enter into play and when they override the child's defaults. If I have {default: {"a": 1}, properties: {"a": {default: 4}}}, my intuition is that the computed default should be {"a": 1} -- you fill in the default for the object, and then check its children. But that's at odds with your implementation, and I could be swayed to an alternate viewpoint/strategy, especially if it has a clear summary.

The logic I'm using is that we favor always the most nested default. I think your logic that the parent default should be filled first is logical, but somehow extending the parent default with child defaults seems intuitive to me. The situation with nested defaults is nonsensical, since no matter which way you compute the defaults, one of those defaults wouldn't be used. It's conflicting any way you look at it.

From a practical point of view, the only reason I can think of why someone would want to use nested defaults is when someone wants to extend an existing schema object, which has defaults and wants to override some child default by providing a default to the child. By using the most nested defaults, like in the logic I'm using here, that would be possible. It works the same way how we favor local schemas instead of a refered schema, without using a defined schema (I wrote a test case should use local default in favor of ref default for object which shows this behaviour).

Here's an example, since that was probably hard to read. We have a schema A {default: {a: 1, b: 2}}. Now, if I wanted to use that schema as a backbone (without creating a defition from it for some reason), but override the default for a, I would end up with schema B {default: {a: 1, b: 2}, a: { default: 4}} which would end up with {a: 4, b: 2} as default.

This is just one point of view - I'm not using nested defaults so I'm not sure what the practical point really is. :)

…ad of children

olzraiti · 2017-08-14T15:13:25Z

I decided to revert the default population order to favor parents instead of children, because the newly merged pull requests #663 (see tests "should render enough input to match minItems, populating the first with default values, and the rest empty" and "should render enough input to match minItems, populating the first with default values, and the rest with the item default") was incompatible with the idea. So you can ignore my previous comment, where I was gathering my thoughts about why the children-first logic could be valid :)

For example, we have an existing test "should keep parent defaults if they don't have a node level default" that I sort of disagree with. I think we need a good, clear, crisp definition of when we fill in defaults. To my mind, if a schema has a default, we use that default only when the field is completely missing. However, the test I mentioned fills in a schema's children even though the field is present!

So this is the logic now. Hence, I made these changes to the existing tests:

Renamed the test "should keep parent defaults if they don't have a node level default" to "should keep parent defaults even if they have a node level default" and updated the test to check that parent defaults are used.
Removed the test "ArrayField List of inputs should honor given formData, even when it does not meet the minItems-requirement", since it is conflicting with "should fill defaults to an array with less items than minItems"

olzraiti · 2017-11-10T08:40:33Z

Bumping, since this is ~3rd time I've got to handle a conflict with this PR.

This is a complicated issue and I've failed to express clearly why this solution is better than the current default populating. Partly that's because, as we have discussed, we currently don't have a clear reasoning with populating the defaults. My previous comment summarises the current conflicting approaches and the solution that this PR offers.

n7best · 2017-11-24T21:38:49Z

does this solve #768 also?

olzraiti · 2017-11-27T12:20:22Z

@n7best I made some changes and added a test case for defaults in dependencies, and I believe this PR would solve #768 now.

glasserc

Hi, just to reiterate, thank you very much for your patience, your thoroughness, and your forbearance with this. I think it's really important to make sure that the fundamental "axioms" of a system are coherent and make sense, and I see that you're doing that here. I'm sorry for taking so long to review it in favor of tiny little PRs that don't really contribute much. I'll try to follow up more aggressively on this PR in particular. I've "starred" this PR in my email so hopefully that will help.

I'd like to consider renaming this PR to something like Clean up edge cases with getDefaultFormState. What do you think?

I still feel somewhat confused by the nature of the axiom being set forth in this PR. The axiom I would have expected is "If an object is completely absent (or undefined), fill it in using its schema's default value, and then recur on its properties/items." However, it seems like this PR's effect is something more like "If an object is missing, fill it in using its schema's default value, or alternately from the default of any of its ancestors' defaults." I tried to express what I thought made sense in a previous comment, but I realize now that I spoke incompletely. I would expect {default: {"a": 1}, properties: {"a": {default: 4}} to have two different behaviors depending on formData. With formData undefined, I would expect to get {"a": 1}. With formData {} (or {"b": 5}), I would expect to get {"a": 4} (or {"b": 5, "a": 4}).

Now, I'm not strongly wedded to this behavior and there could be reasons why it doesn't really make sense (maybe because in an HTML form, there's no clear distinction between a missing object and an empty one?). So I'm happy to be convinced why the behavior you've chosen is consistent/logical/makes sense, but I still don't even think I understand this behavior. Could you try to distill it down to a one-to-three sentence summary for me?

I know this is really frustrating because you've obviously spent a lot of time on this PR and it doesn't feel like it's making progress, but I actually feel like it's pretty good and would like to see something like it land, so please don't give up hope! If you don't have a large amount of time to carry it forward, just say so and maybe I can find the time to take over.

glasserc · 2018-01-12T18:43:24Z

src/utils.js

+          let childSchema = undefined;
+
+          if (Array.isArray(schema.items)) {
+            if (schema.items.length > i && schema.items[i]) {


Is this schema.items[i] condition correct? My reading of http://json-schema.org/latest/json-schema-validation.html#rfc.section.6.4.2 is that we only use additionalItems "at a position greater than the size of 'items'".

glasserc · 2018-01-12T18:44:47Z

src/utils.js

+                  (parentDefaults || []).length,
+                  minItemsLength,
+                  Array.isArray(schema.items) ? schema.items.length : 0
+                );


This is quite a hairy condition. Could we break this into a few if statements, possibly with comments explaining why e.g. we want 0 elements if it's a multi-select with minItems (but not in multiselects without minItems)? (I know to some extent you inherited this code, but feel free to change anything that you can't come up with an explanation for.)

glasserc · 2018-01-12T18:54:36Z

src/utils.js

+          if (parentDefaults && parentDefaults.length > i) {
+            defaultItem = parentDefaults[i];
+          } else if ("default" in schema && schema.default.length > i) {
+            defaultItem = schema.default[i];


I find this a bit surprising. I guess I would have assumed that we take either parentDefaults in their entirety, or schema.default in its entirety, but not both.

glasserc · 2018-01-12T18:56:42Z

src/utils.js

+            if (parentDefaults && key in parentDefaults) {
+              propParentDefault = parentDefaults[key];
+            } else if (schema.default && key in schema.default) {
+              propParentDefault = schema.default[key];


Same concern here about taking both parentDefaults and schema.default.

glasserc · 2018-01-12T19:07:07Z

test/utils_test.js

@@ -168,7 +168,7 @@ describe("utils", () => {
        expect(
          getDefaultFormState(schema, { level1: { level2: { leaf4: 4 } } })
        ).eql({
-          level1: { level2: { leaf1: 1, leaf2: 2, leaf3: 3, leaf4: 4 } },
+          level1: { level2: { leaf1: 1, leaf2: 1, leaf3: 1, leaf4: 4 } },


See, this doesn't make any sense to me. I would have expected { level1: { level2: { leaf3: 3, leaf4: 4 } } }. I would have expected level1.default not to apply because there is already a level1 field, and level1.level2.default to not apply because there is already a level1.level2 field, and level1.level2.leaf3.default to apply because there is no level1.level2.leaf3. Could you summarize under what circumstances a default is filled in?

I agree @glasserc

glasserc · 2018-01-12T19:08:29Z

test/utils_test.js

+      expect(getDefaultFormState(schema, undefined, schema.definitions)).eql({
+        foo: 42,
+      });
+    });


glasserc · 2018-01-12T19:11:31Z

test/utils_test.js

+        },
+      };
+
+      expect(getDefaultFormState(schema, [0, undefined])).eql([0, 2]);


Same concern here about filling in elements of default when the field itself already exists.

glasserc · 2018-01-12T19:12:15Z

test/utils_test.js

+      expect(getDefaultFormState(schema, [1])).eql([1, 0]);
+    });
+
+    it("should use array  array default in favor or array item default even if it is a ref", () => {


array array default?

glasserc · 2018-01-12T19:14:54Z

test/utils_test.js

+      expect(getDefaultFormState(schema, [undefined], schema.definitions)).eql([
+        0,
+      ]);
+    });


Same concern about filling in the toplevel default when the element exists. I think this test would make sense to me if instead of [undefined], you had undefined. However, as written, I would have expected it to be [1].

glasserc · 2018-01-12T19:16:51Z

test/utils_test.js

+        "Do you have a pet?": "Yes",
+        "Does it bite?": true,
+      });
+    });


olzraiti · 2018-02-14T10:56:27Z

Hey,

Currently I don't have the time for this one. I read quickly your feedback (thanks!), and the axiom you are suggesting seems intuitive. Feel free to take the wheel :-)

martijnthe · 2018-09-03T08:43:32Z

@olzraiti it's a bummer this hasn't moved along.
I'd like to see #768 get fixed and it looks like it does?
What's left here to do?

MatinF · 2019-02-05T13:40:23Z

Any update on this?

epicfaace · 2019-02-10T16:31:12Z

@glasserc has left some comments, so if anyone is interested in continuing from this PR and resolving the issues (or fixing this problem another way), feel free to do so.

olzraiti · 2019-11-18T14:40:02Z

I'm closing this since the original issue is fixed by #1499 and I don't remember anymore if this PR had something else relevant in it, and there are so many conflicts that I don't find it likely that anyone would dig into this anymore.

olzraiti added 30 commits July 12, 2016 15:28

fix HiddenWidget error when value is undefined

251974c

add context prop

8e69465

Replace tabs with spaces and add context PropType to Form.

7ae018c

Pass context to FieldTemplate

1217215

Fix passing context to SchemaField

4136956

Replace tabs with spaces

f8158b9

replace context with formContext

9eeaf75

Don't pass formContext to fields that don't use it

39fb8c3

Remove duplicate default export from Form

7ada187

Add tests for formContext

6ab8b12

Fix formContext tests lint issues

933f808

FormContext is passed internally inside registry. Pass formContext to…

b7670f4

… descriptions & titles.

update README

770f761

fix lint and package.json

295e572

fix tests. FormContext must be and object.

7bc5952

Fix minor style issues

90c0bd8

Update README

2861c92

Merge branch 'master' of https://github.com/mozilla-services/react-js…

9e0caac

…onschema-form

Use empty title instead of name

9c158ed

Use empty label instead of name

4974362

Add tests for passing titles/labels

8d120e4

fix lint issues

a919ae1

Merge branch 'master' of https://github.com/mozilla-services/react-js…

2dc4169

…onschema-form

Merge branch 'master' of https://github.com/mozilla-services/react-js…

a76d9a6

…onschema-form

Merge branch 'master' of https://github.com/mozilla-services/react-js…

cfc3afa

…onschema-form

Merge branch 'master' of https://github.com/mozilla-services/react-js…

9f6298e

…onschema-form

Merge branch 'master' of https://github.com/mozilla-services/react-js…

2773fb5

…onschema-form

rewrite computeDefaults to support nested arrays default data population

e69559c

rm unnecessary ref checks from computeDefaults

a7b9c40

allow non schema defined fields for object in computeDefaults

2f38f31

olzraiti added 4 commits May 19, 2017 10:24

fix array length checks

a706347

fix case of object default

742c679

Revert "fix case of object default" (Supporting default object with

f7f89bc

non-defined properties should probably be out of scope) This reverts commit 742c679.

Merge master

91a087e

glasserc reviewed Jun 9, 2017

View reviewed changes

glasserc mentioned this pull request Jun 9, 2017

Minimum Items validation in multiple choice inputs is not working #534

Closed

2 tasks

olzraiti added 4 commits August 1, 2017 16:53

fix additional items default computing

3aac5c2

fix default filling when formData has less items than minitems. Add t…

7f3448e

…est cases for array default data filling.

extend ref schema with local schema when filling default data. Add te…

a01ff1e

…sts cases

typofix

c83464c

olzraiti mentioned this pull request Aug 3, 2017

Arrays with both minItems and default misbehaves #631

Closed

merge master & revert default population order to favor parents inste…

4e6db9c

…ad of children

fix merge conflict

f4685b9

olzraiti added 2 commits November 27, 2017 14:12

Compute defaults correctly for dependencies

72daf58

Fix computing refs

64f55ed

glasserc reviewed Jan 12, 2018

View reviewed changes

hatstand mentioned this pull request Nov 19, 2018

Resolve default properties for nested dynamic dependencies #1078

Closed

9 tasks

MoshiMoshi0 mentioned this pull request Mar 19, 2019

Widgets with custom formData json output #1225

Closed

3 tasks

olzraiti closed this Nov 18, 2019

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

getDefaultFormState populates default data to whole formData #579

getDefaultFormState populates default data to whole formData #579

olzraiti commented May 16, 2017 •

edited

Loading

glasserc left a comment

glasserc Jun 9, 2017

olzraiti commented Jun 13, 2017 •

edited

Loading

olzraiti commented Aug 2, 2017

olzraiti commented Aug 14, 2017

olzraiti commented Nov 10, 2017

n7best commented Nov 24, 2017

olzraiti commented Nov 27, 2017

glasserc left a comment

glasserc Jan 12, 2018

glasserc Jan 12, 2018

glasserc Jan 12, 2018

glasserc Jan 12, 2018

glasserc Jan 12, 2018

epicfaace Jan 21, 2019

glasserc Jan 12, 2018

glasserc Jan 12, 2018

glasserc Jan 12, 2018

glasserc Jan 12, 2018

glasserc Jan 12, 2018

olzraiti commented Feb 14, 2018

martijnthe commented Sep 3, 2018

MatinF commented Feb 5, 2019

epicfaace commented Feb 10, 2019

olzraiti commented Nov 18, 2019

getDefaultFormState populates default data to whole formData #579

getDefaultFormState populates default data to whole formData #579

Conversation

olzraiti commented May 16, 2017 • edited Loading

Reasons for making this change

Checklist

glasserc left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

olzraiti commented Jun 13, 2017 • edited Loading

olzraiti commented Aug 2, 2017

olzraiti commented Aug 14, 2017

olzraiti commented Nov 10, 2017

n7best commented Nov 24, 2017

olzraiti commented Nov 27, 2017

glasserc 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

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

olzraiti commented Feb 14, 2018

martijnthe commented Sep 3, 2018

MatinF commented Feb 5, 2019

epicfaace commented Feb 10, 2019

olzraiti commented Nov 18, 2019

olzraiti commented May 16, 2017 •

edited

Loading

olzraiti commented Jun 13, 2017 •

edited

Loading