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

[azertyfun]

Reasons for making this change

This is a rebase and fix of #1228, which is stale. See also #675, #682

I've kept the change exactly the same (trusting @numical's work there), beyond rebasing I merely added b9f462e which fixes the tests. I don't know if it makes sense to also edit the documentation beyond the changelog, or if this should be implicit behavior?

Checklist

• I'm updating documentation
  • I've checked the rendering of the Markdown text I've added
• I'm adding or updating code
  • I've added and/or updated tests. I've run npm run test:update to update snapshots, if needed.
  • I've updated docs if needed
  • I've updated the changelog with a description of the PR
• I'm adding a new feature
  • I've updated the playground with an example use of the feature

[azertyfun]

I just realized that I accidentally brought back a test file. Currently fixing that.

[azertyfun]

I just realized that I accidentally brought back a test file. Currently fixing that.

[azertyfun]

Actually the more I look at this the less I understand.. The original PR only trims extra data on validation, but doesn't do anything for the submitted data.

With this schema:

{
    "type": "object",
    "properties": {
        "test": {
            "type": "object",
            "properties": {"nested": {"type": "string", "required": "true"}}
        }
    }
}

If no field is filled then the form will validate successfully but the submitted data will be {test: { nested: undefined }} when I would expect {}. Fine, whatever, this looks like intended behavior (at the time?) and may be desirable and is easily dealt with by the user in the onSubmit handler.

---

My second issue is that I don't know what the intended behavior is when dealing with the tests for oneOf/allOf. The original implementation didn't consider the schema at all, but the trimmed data gets rid of an empty string which is actually meaningful in those cases (and I assume everywhere else that isn't a text field!). Essentially we're just guessing that "" is the user not filling in a field, but the test suite uses "" as a meaningful "other" choice instead (which isn't semantically wrong IMO).

{
  "type": "object",
  "properties": {
    "street_address": {
      "type": "string"
    },
    "country": {
      "enum": [
        "United States of America",
        "Canada",
        "United Kingdom",
        "France"
      ]
    }
  },
  "allOf": [
    {
      "if": {
        "properties": {
          "country": {
            "const": "United States of America"
          }
        }
      },
      "then": {
        "properties": {
          "zipcode": {
            "type": "string"
          }
        }
      }
    },
    {
      "if": {
        "properties": {
          "country": {
            "const": "United Kingdom"
          }
        }
      },
      "then": {
        "properties": {
          "postcode": {
            "type": "string"
          }
        }
      }
    },
    {
      "if": {
        "properties": {
          "country": {
            "const": "France"
          }
        }
      },
      "then": {
        "properties": {
          "telephone": {
            "type": "string"
          }
        }
      }
    }
  ]
}

This leads me to believe that maybe the right approach is to detect empty filed in the UI and set the to undefined when empty? Then we can change trimEmptyValues to trimUndefinedValues which is more meaningful.

[heath-freenome]

@azertyfun As part of the pending v5 release we are moving utils and validation into separate packages. It also sounds like you aren't confident that this fix is doing what is intended?

[azertyfun]

Well I'm not sure what it is that is intended. The semantics of getting rid of fields in complex cases is really unclear/undefined, and would require a lot more thought than I could give it in the relatively short timespan I could spend on this PR.

I was hoping to revive the old discussion on trimming non-required forms so we can iron out the edge cases, but if you are refactoring all this then I guess we should try again after that's done.

[heath-freenome]

@azertyfun I am done with all the refactoring. The code you originally changed is now located in @rjsf/utils and @rjsf/validator-ajv6 and @rjsf/validator-ajv8

[PrzemyslawMikos]

Hi @heath-freenome, have issue described by @azertyfun been resolved? If not I would like to offer my help with resolving that. I'm facing same problem.

[azertyfun]

Sorry for the lack of feedback. The internal tool we have that relies on rjsf has been put in an indefinite "works well enough for me" state and I haven't had time (nor will I anytime soon) to progress on this further.

@PrzemyslawMikos feel free to carry on. I'm not sure if there's anything on the work I did that's particularly useful; the approach I based my work on was quite naive, mostly because it conflates undefined, null, and '' as meaning the same thing ("the user didn't mean to fill in this field"). Internally RJSF fields are initially undefined, but if you type something in a nested field then erase that, the field becomes '' and the JSON hierarchy stays filled in forever.

The idea was to iterate over everything to remove all parts of the hierarchy that only lead to undefined and/or empty fields, but as described above this quickly gets complicated with advanced behaviors like oneOf; where we just assume that the frontend having an empty field is the user wishing to get rid of that field, if the schema contains an empty field as a possible value then it doesn't mean we want to get rid of it. But comparing the form data to the schema to guess if a field is empty because of user input or because of the schema feels hacky and wrong to me.
Also this approach has the downside of preventing the user from sending empty strings on submit. The current behavior isn't any "better" (type something into a field then clear it to send an empty string), but people might be relying on it.

---

I still stand by the idea that a much better solution is that the RJSF frontend should be the one that sets fields to undefined when they are explicitly cleared by the user. Ideally that would be made clear in the UI; for a text field, that may mean adding a clear icon and a different style when the field is undefined. A similar mechanism would have to be added for each data type.

However this is a much bigger change than I had anticipated; the frontend needs to support this, across all rendering engines (react, antd, etc.). IDK how that plays out with existing user-defined field templates; that may end up being a breaking change. Also there will probably be many edge cases to figure out and tests to add and fix.

Regardless I'm closing this PR as I'm not likely to work on it any time soon, but feel free to create your own and tag me if you have questions! Though of course I'm neither an owner nor an expert on RJSF.