fix(utils): omit computedDefault of empty objects #2849

ranihorev · 2022-05-03T15:52:46Z

Reasons for making this change

Fixes #2150 and #2708.
See this playground

If there are no defaults, computedDefault will still create an empty object here. As a result, ajv assumes that the field should exist and is failing the validation if it has any required fields.

epicfaace

Can you add tests that explicitly test for #2150 (and maybe #2708)?

ranihorev · 2022-05-03T19:57:16Z

The test I added was explicitly for #2150, I just changed the field names from uuid to a,b,c :)

EDIT: before my changes the state used to be {value: {}}

epicfaace · 2022-05-03T20:32:59Z

@ranihorev #2150 is about "if object field is not required and left empty then form can be submitted without error". The test is about default values. While the test may indeed reflect the issue, can we add a Form.js-level test that actually steps through the user-level process of this? i.e., it should leave the object field empty, the user should try to submit, there should be no error.

Can you also add a test for #2708 so we can see if it fixes that issue too?

ranihorev · 2022-05-03T21:41:30Z

Sure, I misunderstood you earlier, sorry :)
I'll give it a try

ranihorev · 2022-05-04T07:26:35Z

It was a bit more tricky than I expected, but I added the test (fixed my PR as well).
I also had to update ArrayField_test - I don't think that we should create an empty object array if nothing is selected.

epicfaace · 2022-05-04T13:43:15Z

packages/core/src/utils.js

-        if (includeUndefinedValues || computedDefault !== undefined) {
+        if (
+          includeUndefinedValues ||
+          (typeof computedDefault === "object"


This is hard to parse. Can we split it into two if statements?

if (typeof computedDefault === "object") {// if statement here}
...
else {// other if statement here }

And also comment out what each branch means

Updated the code. It's more verbose but indeed easier to read.

epicfaace · 2022-05-04T13:43:44Z

packages/core/test/utils_test.js

+        };
+        expect(getDefaultFormState(schema, {})).eql({});
+      });
+


What about a test for #2708?

Forgot to update the description. I review #2708 and the two issues are identical as far as I can tell.

So -- that might be correct, but we should still add a test that explicitly tests for the scenario in #2708 -- i.e., tries to perform form validation and then ensures that the validation is working.

ranihorev · 2022-05-04T23:45:35Z

@epicfaace when is the next release of package? trying to see if we can get this fix on time :)

epicfaace · 2022-05-04T23:59:07Z

@epicfaace when is the next release of package? trying to see if we can get this fix on time :)

Not sure yet

ranihorev · 2022-05-05T01:21:38Z

👍 And thanks for reviewing the PR!

…

On Wed, May 4, 2022 at 16:59 Ashwin Ramaswami ***@***.***> wrote: @epicfaace <https://github.com/epicfaace> when is the next release of package? trying to see if we can get this fix on time :) Not sure yet — Reply to this email directly, view it on GitHub <#2849 (comment)>, or unsubscribe <https://github.com/notifications/unsubscribe-auth/AEC2ORONSAJDVY7RD7PLN4LVIMFNLANCNFSM5U7KLQQA> . You are receiving this because you were mentioned.Message ID: ***@***.***>

ranihorev · 2022-05-06T23:21:39Z

I somehow stashed the Form level tests I wrote instead of pushing them. Sorry about that.
@epicfaace any ideas why includeUndefinedValues is set true in the validation? I'm seeing this message in the caller but it doesn't explain why
// Include form data with undefined values, which is required for validation.

ranihorev · 2022-05-10T16:11:55Z

I think that includeUndefinedValues is not the right behavior for an object. It's causing us to create an empty object and the validation to fail. I think that it was added to allow setting extraErrors but it feels wrong. What do you think?

ranihorev · 2022-07-05T05:54:27Z

I think that includeUndefinedValues is not the right behavior for an object. It's causing us to create an empty object and the validation to fail. I think that it was added to allow setting extraErrors but it feels wrong. What do you think?

@epicfaace I'd love to get your help. I just noticed that my PR could cause a breaking change - if we stop initializing every object to an empty object, we won't be able to set custom errors on nested field (for example, see this). I don't think that it's a big concern for most users, but maybe I'm wrong.

We can make it work if we switch from using only the formData in customValidate to combining it with the schema

heath-freenome · 2022-07-08T20:43:27Z

I think that includeUndefinedValues is not the right behavior for an object. It's causing us to create an empty object and the validation to fail. I think that it was added to allow setting extraErrors but it feels wrong. What do you think?

@ranihorev So we are actively working on v5 (see the rjsf-v5 branch) which has a bunch of breaking changes... including refactoring utils.js into a new @rjsf/utils package and the validator.js into @rjsf/validator-ajv6. So these changes will likely not get merged into v4. You are welcome to create a change list against that branch.

And yes, in order to properly build the schema for custom validation, the includeUndefinedValues is provided to build the FormValidation objects passed to the customValidator function. This functionality is essential... And yes we could defer creating empty objects in the formData only for calling custom validation. But we would need that behavior and it cannot be removed from computeDefaults() so if you do provide a fix to the rjsf-v5 branch, it would have to maintain that capability.

ranihorev · 2022-07-08T21:38:50Z

@heath-freenome thanks for letting me know. I forked the repo so I'm not in a rush. I'll try moving this PR over to rjsf-v5.
btw, will the be any further work on v4? asking cause I have another PR

heath-freenome · 2022-08-20T17:59:22Z

@heath-freenome thanks for letting me know. I forked the repo so I'm not in a rush. I'll try moving this PR over to rjsf-v5. btw, will the be any further work on v4? asking cause I have another PR

@ranihorev Sorry for the late reply to your question... no, V4 is locked. You can port your other PR to the v5 branch as well

heath-freenome · 2022-09-07T16:36:44Z

@ranihorev Can you port this change, the tests to the @rjsf/utils package and also update the CHANGELOG.md?

ranihorev · 2022-09-08T01:54:44Z

I can but I'm still not sure how we want to treat custom validations in this case. What is the expected behavior?

…

On Wed, Sep 7, 2022 at 19:36 Heath C ***@***.***> wrote: @ranihorev <https://github.com/ranihorev> Can you port this change, the tests to the @rjsf/utils package and also update the CHANGELOG.md? — Reply to this email directly, view it on GitHub <#2849 (comment)>, or unsubscribe <https://github.com/notifications/unsubscribe-auth/AEC2ORLCT73EP5RHMBOTTELV5DACPANCNFSM5U7KLQQA> . You are receiving this because you were mentioned.Message ID: ***@***.***>

heath-freenome · 2022-09-08T19:01:13Z

I can but I'm still not sure how we want to treat custom validations in this case. What is the expected behavior?
…
On Wed, Sep 7, 2022 at 19:36 Heath C @.> wrote: @ranihorev https://github.com/ranihorev Can you port this change, the tests to the @rjsf/utils package and also update the CHANGELOG.md? — Reply to this email directly, view it on GitHub <#2849 (comment)>, or unsubscribe https://github.com/notifications/unsubscribe-auth/AEC2ORLCT73EP5RHMBOTTELV5DACPANCNFSM5U7KLQQA . You are receiving this because you were mentioned.Message ID: @.>

@epicfaace Do you have an opinion?

heath-freenome · 2022-09-14T06:41:09Z

I can but I'm still not sure how we want to treat custom validations in this case. What is the expected behavior?

@ranihorev Here is what I propose... In both of the validators, let's try moving

   const newFormData = getDefaultFormState<T>(
      this,
      schema,
      formData,
      rootSchema,
      true
    ) as T;

to right before the customValidate(...) using the formData in the this.ajv.validate() call.

I've sanity checked the tests with that change and it seems to work. Then make your change in the utils so that it continues to respect the includeUndefinedValues flag.

epicfaace · 2022-09-14T13:38:18Z

I can but I'm still not sure how we want to treat custom validations in this case. What is the expected behavior?
…
On Wed, Sep 7, 2022 at 19:36 Heath C @.> wrote: @ranihorev https://github.com/ranihorev Can you port this change, the tests to the @rjsf/utils package and also update the CHANGELOG.md? — Reply to this email directly, view it on GitHub <#2849 (comment)>, or unsubscribe https://github.com/notifications/unsubscribe-auth/AEC2ORLCT73EP5RHMBOTTELV5DACPANCNFSM5U7KLQQA . You are receiving this because you were mentioned.Message ID: @.>

Sorry, I'm not clear what you're asking?

heath-freenome · 2022-09-23T17:11:00Z

I can but I'm still not sure how we want to treat custom validations in this case. What is the expected behavior?
…
On Wed, Sep 7, 2022 at 19:36 Heath C @.> wrote: @ranihorev https://github.com/ranihorev Can you port this change, the tests to the @rjsf/utils package and also update the CHANGELOG.md? — Reply to this email directly, view it on GitHub <#2849 (comment)>, or unsubscribe https://github.com/notifications/unsubscribe-auth/AEC2ORLCT73EP5RHMBOTTELV5DACPANCNFSM5U7KLQQA . You are receiving this because you were mentioned.Message ID: _@**.**_>

Sorry, I'm not clear what you're asking?

@epicfaace Check out my proposal in comments above

ranihorev · 2022-10-05T13:57:05Z

I can but I'm still not sure how we want to treat custom validations in this case. What is the expected behavior?

@ranihorev Here is what I propose... In both of the validators, let's try moving
   const newFormData = getDefaultFormState<T>(
      this,
      schema,
      formData,
      rootSchema,
      true
    ) as T;
to right before the customValidate(...) using the formData in the this.ajv.validate() call.

I've sanity checked the tests with that change and it seems to work. Then make your change in the utils so that it continues to respect the includeUndefinedValues flag.

@heath-freenome to make sure we're on the same page: you are suggesting to run this.ajv.validate(schema, formData); on formData instead of newFormData and only use the newFormData for the custom validation? Am I right?

I wonder if we should call getDefaultFormState twice - once with includeUndefinedValues = false for the regular validation and once with true for the custom validation

heath-freenome · 2022-10-05T16:44:31Z

@heath-freenome to make sure we're on the same page: you are suggesting to run this.ajv.validate(schema, formData); on formData instead of newFormData and only use the newFormData for the custom validation? Am I right?

I wonder if we should call getDefaultFormState twice - once with includeUndefinedValues = false for the regular validation and once with true for the custom validation

There is some sense to that as well... Go with that.

ranihorev · 2022-10-07T18:09:45Z

I started without computing it twice. It works great in the UI, but it breaks a lot of tests in utils as they all assume (incorrectly IMO) that those nested default objects will be generated. My plan is to fix them by manually setting includeUndefinedValues to true

ranihorev · 2022-10-10T00:48:27Z

I found a bug in my PR :(
oneOf properties with default values are being discarded. For example:

{
          type: "array",
          items: {
            properties: {
              foo: {
                type: "object",
                properties: {
                  name: {
                    type: "string",
                  },
                },
                dependencies: {
                  name: {
                    oneOf: [
                      {
                        properties: {
                          name: {
                            enum: ["first"],
                          },
                          grade: {
                            type: "string",
                            default: "A",
                          },
                        },
                      },
                      {
                        properties: {
                          name: {
                            enum: ["second"],
                          },
                          grade: {
                            type: "string",
                            default: "B",
                          },
                        },
                      },
                    ],
                  },
                },
              },
            },
          },
        }

I'll look for a solution, but if anyone has an idea, please let me know :)

heath-freenome · 2022-10-10T16:40:11Z

I found a bug in my PR :( oneOf properties with default values are being discarded. For example:

...

I'll look for a solution, but if anyone has an idea, please let me know :)

Nothing comes to mind at the moment :(

zmagauina-fn · 2022-12-07T20:58:35Z

I used this PR as a starting point to make my own: #3287

ranihorev · 2022-12-09T00:06:43Z

I used this PR as a starting point to make my own: #3287

Thanks for picking this up!

heath-freenome · 2022-12-12T17:35:04Z

This was fixed in #3287

epicfaace reviewed May 3, 2022

View reviewed changes

ranihorev force-pushed the fix-required-empty-obj branch from 907945e to 828ba9e Compare May 3, 2022 17:00

ranihorev force-pushed the fix-required-empty-obj branch from f2ea45a to 691f08c Compare May 4, 2022 07:23

ranihorev force-pushed the fix-required-empty-obj branch from 691f08c to a05bad3 Compare May 4, 2022 07:37

epicfaace reviewed May 4, 2022

View reviewed changes

ranihorev force-pushed the fix-required-empty-obj branch from 484cb47 to a6753b5 Compare July 5, 2022 03:21

heath-freenome added the v5 refactor Needs refactor due to v5 breaking changes label Aug 29, 2022

heath-freenome added the awaiting changes label Sep 7, 2022

ranihorev force-pushed the fix-required-empty-obj branch from a6753b5 to e516a7f Compare October 7, 2022 18:06

fix(utils): omit computedDefault of empty objects

38044a6

ranihorev force-pushed the fix-required-empty-obj branch from e516a7f to 38044a6 Compare October 8, 2022 23:20

zmagauina-fn mentioned this pull request Dec 7, 2022

fix(utils): omit computedDefault of empty required properties nested in optional parent property #3287

Merged

4 tasks

heath-freenome closed this Dec 12, 2022

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

fix(utils): omit computedDefault of empty objects #2849

fix(utils): omit computedDefault of empty objects #2849

ranihorev commented May 3, 2022 •

edited

epicfaace left a comment

ranihorev commented May 3, 2022 •

edited

epicfaace commented May 3, 2022

ranihorev commented May 3, 2022

ranihorev commented May 4, 2022

epicfaace May 4, 2022

ranihorev May 4, 2022 •

edited

epicfaace May 4, 2022

ranihorev May 4, 2022

epicfaace May 6, 2022

ranihorev commented May 4, 2022

epicfaace commented May 4, 2022

ranihorev commented May 5, 2022 via email

ranihorev commented May 6, 2022

ranihorev commented May 10, 2022

ranihorev commented Jul 5, 2022 •

edited

heath-freenome commented Jul 8, 2022 •

edited

ranihorev commented Jul 8, 2022

heath-freenome commented Aug 20, 2022

heath-freenome commented Sep 7, 2022

ranihorev commented Sep 8, 2022 via email

heath-freenome commented Sep 8, 2022

heath-freenome commented Sep 14, 2022 •

edited

epicfaace commented Sep 14, 2022

heath-freenome commented Sep 23, 2022

ranihorev commented Oct 5, 2022

heath-freenome commented Oct 5, 2022

ranihorev commented Oct 7, 2022

ranihorev commented Oct 10, 2022

heath-freenome commented Oct 10, 2022

zmagauina-fn commented Dec 7, 2022

ranihorev commented Dec 9, 2022

heath-freenome commented Dec 12, 2022

fix(utils): omit computedDefault of empty objects #2849

fix(utils): omit computedDefault of empty objects #2849

Conversation

ranihorev commented May 3, 2022 • edited

Reasons for making this change

epicfaace left a comment

Choose a reason for hiding this comment

ranihorev commented May 3, 2022 • edited

epicfaace commented May 3, 2022

ranihorev commented May 3, 2022

ranihorev commented May 4, 2022

epicfaace May 4, 2022

Choose a reason for hiding this comment

ranihorev May 4, 2022 • edited

Choose a reason for hiding this comment

epicfaace May 4, 2022

Choose a reason for hiding this comment

ranihorev May 4, 2022

Choose a reason for hiding this comment

epicfaace May 6, 2022

Choose a reason for hiding this comment

ranihorev commented May 4, 2022

epicfaace commented May 4, 2022

ranihorev commented May 5, 2022 via email

ranihorev commented May 6, 2022

ranihorev commented May 10, 2022

ranihorev commented Jul 5, 2022 • edited

heath-freenome commented Jul 8, 2022 • edited

ranihorev commented Jul 8, 2022

heath-freenome commented Aug 20, 2022

heath-freenome commented Sep 7, 2022

ranihorev commented Sep 8, 2022 via email

heath-freenome commented Sep 8, 2022

heath-freenome commented Sep 14, 2022 • edited

epicfaace commented Sep 14, 2022

heath-freenome commented Sep 23, 2022

ranihorev commented Oct 5, 2022

heath-freenome commented Oct 5, 2022

ranihorev commented Oct 7, 2022

ranihorev commented Oct 10, 2022

heath-freenome commented Oct 10, 2022

zmagauina-fn commented Dec 7, 2022

ranihorev commented Dec 9, 2022

heath-freenome commented Dec 12, 2022

ranihorev commented May 3, 2022 •

edited

ranihorev commented May 3, 2022 •

edited

ranihorev May 4, 2022 •

edited

ranihorev commented Jul 5, 2022 •

edited

heath-freenome commented Jul 8, 2022 •

edited

heath-freenome commented Sep 14, 2022 •

edited