fix(utils): omit computedDefault of empty objects

[ranihorev]

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.

[ranihorev]

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]

@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]

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

[ranihorev]

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.

[ranihorev]

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

[epicfaace]

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

Not sure yet

[ranihorev]

👍 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 https://github.com/rjsf-team/react-jsonschema-form/pull/2849#issuecomment-1118035729, or unsubscribe https://github.com/notifications/unsubscribe-auth/AEC2ORONSAJDVY7RD7PLN4LVIMFNLANCNFSM5U7KLQQA . You are receiving this because you were mentioned.Message ID: @.***>

[ranihorev]

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]

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]

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]

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]

@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]

@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]

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

[ranihorev]

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 https://github.com/rjsf-team/react-jsonschema-form/pull/2849#issuecomment-1239628191, or unsubscribe https://github.com/notifications/unsubscribe-auth/AEC2ORLCT73EP5RHMBOTTELV5DACPANCNFSM5U7KLQQA . You are receiving this because you were mentioned.Message ID: @.***>

[heath-freenome]

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]

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]

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]

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]

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]

@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]

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]

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]

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]

I used this PR as a starting point to make my own: https://github.com/rjsf-team/react-jsonschema-form/pull/3287

[ranihorev]

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

Thanks for picking this up!

[heath-freenome]

This was fixed in #3287