kristianmandrup / schema-to-yup

Schema to Yup validation
Other
283 stars 50 forks source link

Object type not respecting nullable property #128

Closed jakeloudenslager-champ closed 1 year ago

jakeloudenslager-champ commented 1 year ago

In my json schema I have defined

"state": {
      "label": "Residing State",
      "type": "object",
      "properties": {
        "label": {
          "type": "string"
        },
        "value": {
          "type": "string"
        }
      },
      "defaultValue": null,
      "nullable": true
    }

However, in the form I receive this error.

image

Inspecting the generated Yup schema I can see

image

I'm also seeing this field being marked as optional instead of required which might be related.

You can see the error I described above by selecting a state and then clicking the x in the box to clear it.

Code Sandbox Recreation:

https://codesandbox.io/s/practical-tu-i2ym8f?file=/src/jsonSchema.json

kristianmandrup commented 1 year ago

Looking at the code for nullable in mixed.js

  isType() {
    const value = this.constraints.isType;
    this.addConstraint("isType", { value, errName: "notOneOf" });
    return this;
  }

  nullable() {
    const { nullable, isNullable } = this.constraints;
    const value = nullable || isNullable;
    this.addConstraint("nullable", { value, errName: "notOneOf" });
    return this;
  }

Copy-pasta error on errName "notOneOf" multiple times. Very interesting. Will fix that.

Yup nullable vs required https://github.com/jquense/yup/issues/1350

Also have a look at mode https://github.com/kristianmandrup/schema-to-yup#Mode

We should also add tests for these cases with nullable and required combinations on an object.

kristianmandrup commented 1 year ago

For now I've fixed the errName for those and pushed. Would welcome a PR with a couple of test cases added for your scenario to work from.

jakeloudenslager-champ commented 1 year ago

So essentially the reason we want to have a field that is both nullable and required is because its a dropdown. Initially, it is in a null state so that no value is selected, but we do want to require that the user chooses a value before submitting the form.

with regards to required not applying

The pure yup definition produces this

yup.object().shape({
  name: yup
    .string()
    .label("Name")
    .required()
    .matches(
      /^[a-z0-9]+$/i,
      "Only alphanumeric characters allowed for this field"
    )
    .min(11)
    .max(17),
  state: yup
    .object()
    .shape({
      label: yup.string(),
      value: yup.string()
    })
    .label("Residing State")
    .required()
    .nullable()
});

image

However, the json schema

{
  "title": "Draft Order Form",
  "type": "object",
  "properties": {
    "name": {
      "label": "Name",
      "type": "string",
      "pattern": "^[a-zA-Z0-9]+$",
      "minLength": 11,
      "maxLength": 17,
      "defaultValue": "",
      "nullable": true
    },
    "state": {
      "label": "Residing State",
      "type": "object",
      "properties": {
        "label": {
          "type": "string"
        },
        "value": {
          "type": "string"
        }
      },
      "defaultValue": null,
      "nullable": true
    }
  },
  "required": ["name", "state"]
}

produces this

image

It also looks like label is not being applied either

I have also tried to mark state as required directly on the entry

kristianmandrup commented 1 year ago

Thanks. I'll look into this ASAP.

With regards to label this is the current impl

  label() {
    const value = this.value;
    const label = value.title || value.label;
    this.base = (label && this.base.label(label)) || this.base;
    return this;
  }

nullable should be made to look similar, ie.

  nullable() {
    const nullable = this.value.nullable === true;
    this.base = (nullable && this.base.nullable()) || this.base;
    return this;
  }

where this.base is the current Yup schema instance (such as an Object schema) that you are building/chaining on.

kristianmandrup commented 1 year ago

I made the change for nullable in latest commit. Pls try it out. Unfortunately very busy with work and family life ATM

jakeloudenslager-champ commented 1 year ago

no worries,

I'm fairly sure I was able to figure out how to test it (npm wasn't updated). I cloned the repo, built the project and then swapped out my project's schema-to-yup dist folder with the newly built one and I was able to see the updated nullable function in Chrome's sources tab.

However I still got the same error. I did look at the debug for a little bit breakpointing on the return in nullable() and saw that the constraints appear to be defined correctly when I step through it for each of the fields, but then the overall schema that is returned is incorrect.

kristianmandrup commented 1 year ago

Cool :) I would advise to start simple, by reducing your schema to one with just nullable, then adding the required constraint to a field, then one with both nullable and required to see when and how it breaks.

{
  "title": "Draft Order Form",
  "type": "object",
  "properties": {
    "state": {
      "label": "Residing State",
      "type": "object",
      "nullable": true
    }
  },
}

Then

{
  "title": "Draft Order Form",
  "type": "object",
  "properties": {
    "state": {
      "label": "Residing State",
      "type": "object",
      "defaultValue": null,
      "nullable": true
    }
  },
}

And so on... ideally add each as a test case to the test suites so others can benefit as well :)

kristianmandrup commented 1 year ago

I've since added a test case for nullable and it seems to work as I would expect. Feel free to verify.

ankitWOW commented 1 year ago

Hi @kristianmandrup ,

I think still "Object" type not respecting nullable property.

"isNullable" is not working for the following JSON.

"properties": {
                    "breed": {
                        "type": "object",
                        "defaultValue": null,
                        "nullable": true
                    },
             },

Can you please check it and let me know if I am missing anything else for the same?