kristianmandrup / schema-to-yup

Schema to Yup validation
Other
281 stars 51 forks source link

oneOf with deep type does not work #109

Closed nujabness closed 2 years ago

nujabness commented 2 years ago

Hi there,

I'm currently facing a problem with schema-to-yup. My schema looks like this:

{
  "$schema": "https://json-schema.org/draft/2020-12/schema",
  "type": "object",
  "properties": {
    "myField": {
      "type": "object",
      "oneOf": [
        {
          "type": "object",
          "required": ["question"],
          "properties": {
            "question": {
              "type": "string",
              "enum": [
                "value1",
                "value2"
              ]
            }
          }
        },
        {
          "type": "object",
          "required": ["question", "questionOther"],
          "properties": {
            "question": {
              "type": "string",
              "enum": ["other"]
            },
            "questionOther": {
              "type": "string"
            }
          }
        }
      ]
    }
  }
}

I'm trying to make a field required depending on a value of another field but with this schema the field does not trigger any validation.

I also tried other schema more simple like :

{
  "$schema": "https://json-schema.org/draft/2020-12/schema",
  "type": "object",
  "properties": {
    "myField": {
      "type": "string",
      "oneOf": [
        {"const": "value1"},
        {"const": "value2"}
      ]
    }
  }
}

I’m getting the following error: TypeError: s is not iterable in my browser's console.

Also this schema works but this not a valid json schema:

{
  "$schema": "https://json-schema.org/draft/2020-12/schema",
  "type": "object",
  "properties": {
    "myField": {
      "type": "string",
      "oneOf": ["value1", "value2"]
    }
  }
} 

It seems like oneOf is just an alias of enum. Is any workaround available to achieve my goal?

Here a link of a vue project with different use cases: yup-sandbox

kristianmandrup commented 2 years ago

Yes, when I initially developed this library I did not know all the intricacies of JSON schema (I don't even to this day). So I like had a limited understanding of oneOf. It should essentially work almost the same as the support for multi value, taking an array of possibly complex definitions as in multi-property-value-resolver which incidentally has a oneOf method:

  oneOf() {
    const schemaValues = this.value
    const createEntry = this.createEntry.bind(this)
    const resolvedValidatorSchemas = schemaValues.map(createEntry)
    return this.mixed().oneOf(resolvedValidatorSchemas)
  }

So I would recommend first trying to use the customization options described in the Readme

to add a custom oneOf entry handler which calls createMultiPropertyValueResolver.

When you make this work I can integrate the solution directly in the library.

kristianmandrup commented 2 years ago

So the default oneOf implementation in mixed.js looks like this:

  oneOf() {
    let values =
      this.constraints.enum || this.constraints.oneOf || this.constraints.anyOf;
    if (this.isNothing(values)) return this;
    values = Array.isArray(values) ? values : [values];
    const resolvedValues = this.resolveValues(values)
    // using alias
    const alias = ["oneOf", "enum", "anyOf"].find(key => {
      return this.constraints[key] !== undefined;
    });
    return this.addConstraint(alias, { values: resolvedValues });
  }

I would think it needs to be reconfigured to sth like the following

  oneOf() {
    let values =
      this.constraints.enum || this.constraints.oneOf || this.constraints.anyOf;
    const opts = {
      ...this.opts,
      value: values,
    }
    const resolver = createMultiPropertyValueResolver(opts, this.config, this)
    this.base = resolver.resolve()
    return this
}
kristianmandrup commented 2 years ago

On further examination the above suggested should not be needed. I should already handle the recursive case (albeit possible incorrectly)

I refactored the internal code slightly to this:

  get oneOfAliases() {
    return ["oneOf", "enum", "anyOf"]
  }

  get oneOfAlias() {
    return this.oneOfAliases.find(key => {
      return this.constraints[key] !== undefined;
    });
  }

  oneOf() {
    let values = this.oneOfValues
    if (this.isNothing(values)) return this;
    values = this.normalizeValues(values)
    const resolvedValues = this.resolveValues(values)
    // using alias
    const alias = this.oneOfAlias
    return this.addConstraint(alias, { values: resolvedValues });
  }

resolveValues is key here, it is supposed to handle the recursive case

  resolveValues(values) {
    const schemaValues = values
    return schemaValues.map(value => {
      return this.isObjectType(value) ? this.resolveValue(value) : value
    })
  }  

So that if any of the values is an object type, it will call resolveValue to resolve that value recursively. If also adding more detailed logging capabilities for this case. Please try your sandbox with detailed logging:

const config = {
  logDetailed: [{    
    method: 'oneOf'
  }]
kristianmandrup commented 2 years ago

Recursive const like in your case might well require special case treatment, like you point to, so that:

"myField": {
      "type": "string",
      "oneOf": [
        {"const": "value1"},
        {"const": "value2"}
      ]
    }

Would internally be transformed to .oneOf(["value1", "value2")

kristianmandrup commented 2 years ago

I think this might do the trick

  isConst(value) {
    return this.hasKey(value, 'const')
  }

  resolveValue(value) {
    // special case for const
    if (this.isConst(value)) return value['const']

    const createYupSchemaEntry = this.config.createYupSchemaEntry // || this.entryHandler.createNew
    const opts = { schema: this.schema, key: this.key, value, config: this.config }
    return createYupSchemaEntry(opts)  
  }

I've pushed this fix to master so you can try it out