oneOf schemas do not validate as expected
brenden-ef opened this issue · 16 comments
Not 100% sure if this is related to #109 or not.
We have a use case where we need to use oneOf
to conditionally validate one field based on the value of another.
Given the following schema
{
"title": "Schema title",
"description": "Schema description",
"type": "object",
"properties": {
"firstName": {
"type": "string",
},
"lastName": {
"type": "string",
},
"requireAddress": {
"type": "string",
"enum": [
"yes",
"no"
]
},
"addressLine1": {
"type": "string",
},
"addressLine2": {
"type": "string",
}
},
"required": [
"firstName",
"lastName",
"requireAddress"
],
"oneOf": [
{
"properties": {
"requireAddress": {
"const": "yes"
},
"addressLine1": {
"minLength": 2
},
"addressLine2": {
"minLength": 2
}
},
"required": [
"addressLine1",
"addressLine2"
]
},
{
"properties": {
"requireAddress": {
"const": "no"
}
}
}
]
}
I would expect this object to fail validation because requireAddress
is yes
, but addressLine1
is not present:
{
"firstName": "han",
"lastName": "yolo",
"requireAddress": "yes",
"addressLine2": "houseNumber"
}
However, the object is marked as valid by the schema returned from buildYup
.
You can see the validation behaving the way we would expect on https://www.jsonschemavalidator.net if you enter the above objects.
Reproduction: https://github.com/brenden-ef/schema-to-yup-one-of-repro
Thanks. I will add it as a test case and try to resolve it shortly.
Ah yes, the const
issue again. This is indeed related to #109
It is currently implemented this way:
- If
const
is set to nothing (null
orundefined
) it ignores it and returns. - If const is a data reference, ie. there is a
$data
property, then the value is set toyup.ref(dataRef)
, which is the normalized data ref path. - Finally a yup constraint is added for
const
with thevalue
(dataref or "as-is").
const() {
let value = this.constraints.const;
if (this.isNothing(value)) return this;
// TODO: resolve const data ref if valid format
if (this.isDataRef(value)) {
const dataRefPath = this.normalizeDataRefPath(value);
value = yup.ref(dataRefPath);
}
return this.addConstraint("const", { value });
}
Looking closer at the yup schema documentation, I can see there is no yup constraint const
.
The closest alternative is oneOf
that is passed a list containing a single value to test on.
So I think the following fix should solve it.
const() {
let value = this.constraints.const;
if (this.isNothing(value)) return this;
// TODO: resolve const data ref if valid format
if (this.isDataRef(value)) {
const dataRefPath = this.normalizeDataRefPath(value);
value = yup.ref(dataRefPath);
}
return this.addConstraint("oneOf", { values: [value] });
}
Try it in the new const-oneOf
branch.
It passed the test
const schema = {
// schema from the repo https://github.com/brenden-ef/schema-to-yup-one-of-repro
}
const object = {
firstName: "han",
lastName: "yolo",
requireAddress: "yes",
addressLine2: "houseNumber",
};
test("oneOf const", async () => {
const yupSchema = buildYup(schema);
// console.dir(schema)
try {
const valid = await yupSchema.isValid(object);
expect(valid).toBe(false);
} catch (e) {
console.log(e);
}
});
Test results
PASS test/one-of-const.test.js
✓ oneOf const (23 ms)
Test Suites: 1 passed, 1 total
Tests: 1 passed, 1 total
Please confirm, then I will merge to master and close the issue ;)
I think the try/catch in your test might be swallowing the failure? When I run it locally I see this.
Ran all test suites matching /one-of-const.test.js/i.
❯ npx jest one-of-const.test.js
console.log
JestAssertionError: expect(received).toBe(expected) // Object.is equality
Expected: false
Received: true
at Object.toBe (/Users/brenden/code/schema-to-yup/test/one-of-const.test.js:63:19)
at processTicksAndRejections (node:internal/process/task_queues:96:5) {
matcherResult: {
actual: true,
expected: false,
message: '\x1B[2mexpect(\x1B[22m\x1B[31mreceived\x1B[39m\x1B[2m).\x1B[22mtoBe\x1B[2m(\x1B[22m\x1B[32mexpected\x1B[39m\x1B[2m) // Object.is equality\x1B[22m\n' +
'\n' +
'Expected: \x1B[32mfalse\x1B[39m\n' +
'Received: \x1B[31mtrue\x1B[39m',
name: 'toBe',
pass: false
}
}
at Object.log (test/one-of-const.test.js:65:13)
And when I comment out the try/catch I see the test failure.
❯ npx jest one-of-const.test.js
FAIL ./one-of-const.test.js
✕ oneOf const (5 ms)
● oneOf const
expect(received).toBe(expected) // Object.is equality
Expected: false
Received: true
61 | // try {
62 | const valid = await yupSchema.isValid(object);
> 63 | expect(valid).toBe(false);
| ^
64 | // } catch (e) {
65 | // console.log(e);
66 | // }
at Object.toBe (test/one-of-const.test.js:63:19)
Test Suites: 1 failed, 1 total
Tests: 1 failed, 1 total
Could you write the equivalent Yup schema that would work for your minimal case? then we can work to target that. Cheers.
Sure thing. My schedule is a bit wild today, but will work to get it to you ASAP.
The only way I've been able to get it to work without using js to conditionally compose multiple schemas is by using when
:
object({
firstName: string().required(),
lastName: string().required(),
requireAddress: string().required().oneOf(["yes", "no"]),
addressLine1: string().when("requireAddress", {
is: "yes",
then: (schema) => schema.required()
})
});
Normally I'd try to go the route using with
, but we don't have control over the schemas we're converting, and other systems that don't use yup have to validate against same schemas. According to json schema spec though oneOf
can be used to handle the same kind of conditional logic.
I see. Currently it doesn't understand specific const values such as yes
or no
to indicate on/off switching of logic.
Should not be too hard to customize it to implement such logic using the customization options included (see Readme docs).
It turns out this is due to a limitation of oneOf
in yup
- https://stackoverflow.com/questions/72925269/how-to-validate-using-oneof-with-yup
- jquense/yup#1393
- jquense/yup#1393 (comment)
Yup doesn't treat oneOf
in any way like a conditional. The library currently attempts a 1-1 mapping to the extent possible.
I'm not sure what the best approach for your case would be. Perhaps override the built-on oneOf so that if passed one or more objects, see for which property they all overlap and if so use that for the when-is
condition and in the then
return whichever object passes the is
test.
https://stackoverflow.com/questions/34392741/best-way-to-get-intersection-of-keys-of-two-objects
function intersectKeys(first, ...rest) {
const restKeys = rest.map(o => Object.keys(o));
return Object.fromEntries(Object.entries(first).filter(entry => restKeys.every(rk => rk.includes(entry[0]))));
}
or
function intersectingKeys(...objects) {
return objects
.map((object) => Object.keys(object))
.sort((a, b) => a.length - b.length)
.reduce((a, b) => a.filter((key) => b.includes(key)));
}
Likely the only option is to leverage https://github.com/jquense/yup#schematestname-string-message-string--function--any-test-function-schema
I think something like this should do the trick
oneOfConditional() {
let { config, parentNode, isObject, value, key } = this;
// optionally set custom errMsg
const oneOfConstraints = value;
this.base = this.base.test(
key,
// errMsg,
(value, context) => {
for (let constraint in oneOfConstraints) {
if (!isObject(constraint)) {
return value === constraint;
}
const yupSchema = config.buildYup(constraint, config, parentNode);
const result = yupSchema.validate();
if (result) return true;
}
return false;
}
);
}
notOneOfConditional() {
let { config, parentNode, isObject, value, key } = this;
// optionally set custom errMsg
const oneOfConstraints = value;
this.base = this.base.test(
key,
// errMsg,
(value, context) => {
for (let constraint in oneOfConstraints) {
if (!isObject(constraint)) {
return value !== constraint;
}
const yupSchema = config.buildYup(constraint, config, parentNode);
const result = yupSchema.validate();
if (result) return false;
}
return true;
}
);
}
There should now be better support for overriding mixed
constraint handler methods such as oneOf
. Also added advanced customization example that handle your case.
https://github.com/kristianmandrup/schema-to-yup#Advancedcustomconstraintexample
const typeConfig = this.config[this.type] || {};
const mixedConfig = this.config.mixed || {};
this.mixedConfig = mixedConfig;
this.typeConfig = {
...mixedConfig,
...typeConfig,
};
configureTypeConfig() {
if (this.typeConfig.enabled || this.typeConfig.extends) return;
if (!this.typeConfig.convert) return;
this.typeConfig.extends = Object.keys(this.typeConfig.convert);
}
get configuredTypeEnabled() {
return Array.isArray(this.typeConfig.enabled)
? this.typeConfig.enabled
: this.typeEnabled;
}
get $typeEnabled() {
return this.$typeExtends || this.configuredTypeEnabled;
}
get enabled() {
return [...this.mixedEnabled, ...this.$typeEnabled];
}
convertEnabled() {
this.enabled.map((name) => {
const convertFn = this.convertFnFor(name);
if (convertFn) {
convertFn(this);
}
});
}
convertFnFor(name) {
return (
this.customConvertFnFor(name, this) || this.builtInConvertFnFor(name)
);
}
customConvertFnFor(name) {
const typeConvertMap = this.typeConfig.convert || {};
return typeConvertMap[name];
}
Might be a little hard to follow, but essentially if the custom constraint function is set in typeConfig and all part of enabled array, then it will be triggered here and passed this
as the single argument (the type handler) and override the built in type handler method if one exists for the same name.
convertEnabled() {
this.enabled.map((name) => {
const convertFn = this.convertFnFor(name);
if (convertFn) {
convertFn(this);
}
});
}
Ah great! I will try this out later on today and let you know how it goes.
@kristianmandrup any news? the "oneOf" below dont work:
{
type: 'object',
properties: { ... },
oneOf: [
{
required: ['questionId'],
},
{
required: ['categoryId', 'questionsCount'],
},
]
}
Sorry, this is currently a known limitation. Not sure how to translate this to a Yup schema. Feel free to have a go at it and if you find a good approach I can integrate it shortly. Sorry for the late response. Not really maintaining this repo much.
It was meant as a starting point for the community to continue. In dire need of re-engineering from the ground up!