Default value ambiguity in `oneOf` / Support for discriminators?
Opened this issue ยท 24 comments
I can't work out if discriminators are supposed to be supported, or not. I certainly can't get them to be honored. If I use express-ajv-swagger-validation then they are honored, but default values are not. Kind of hoping express-openapi-validator might be a one-stop-shop as everything else works a treat.
Actually, it could be that oneOf isn't working. I have schema types with different properties and they're not being distinguished from one another.
Can you give an example? allOf is working well for me.
So my schema is pretty big, but way down in it I have something like this:
When I POST an instance of SimilarTypeOne, we're all set. When I POST an instance of SimilarTypeTwo the validator complains uniqueOne is missing.
If I switch the order in the oneOf, instances of SimilarTypeOne now get rejected for not having uniqueTwo.
SimilarTypeOne:
allOf:
- $ref: '#/components/schemas/SimilarSuperType'
type: object
properties:
type:
type: string
enum: [Some Value One]
uniqueOne:
type: string
required:
- type
- uniqueOne
SimilarTypeTwo:
allOf:
- $ref: '#/components/schemas/SimilarSuperType'
type: object
properties:
type:
type: string
enum: [Some Value Tow]
uniqueTwo:
type: string
required:
- type
- uniqueTwo
SomeType:
allOf:
- $ref: '#/components/schemas/SomeSuperTYpe'
type: object
properties:
something:
type: array
minItems: 1
maxItems: 2
items:
oneOf:
- $ref: "#/components/schemas/SimilarTypeOne"
- $ref: "#/components/schemas/SimilarTypeTwo"
discriminator:
propertyName: type
@cdimascio can't send you the whole schema for copyright / ip reasons and in simplifying / anonymizing for a reproducible test case, the problem goes away : (
If I figure out what the problem is, I'll let you know.
@catadch i created a few tests to exercise the schema you sent. it appears to work in that i'm able to use the two oneOf instances
see here -> https://github.com/cdimascio/express-openapi-validator/pull/105/files
there may be some other element that is contributing to the issue.
it would be great if you can provide a snippet that can repro the issue.
feel free to modify my tests as well
@cdimascio - I found the problem.
In the schema below, it's the enums / defaults for the two "value" number type properties that causes the problem.
Comment either one out and the problem goes away.
I have a stand-alone project to demonstrate this is you want it.
openapi: '3.0.0'
info:
description: express-openapi-validator-test
version: 1.0.0
title: express-openapi-validator-test
license:
name: MIT License
url: https://opensource.org/licenses/MIT
servers:
- url: http://localhost:3010/v0
paths:
/typethrees:
post:
requestBody:
required: true
content:
application/json:
schema:
oneOf:
- $ref: '#/components/schemas/TypeThree'
responses:
'201':
description: Application response
content:
application/json:
schema:
$ref: '#/components/schemas/TypeThree'
default:
description: unexpected error
content:
application/json:
schema:
$ref: '#/components/schemas/Error'
components:
schemas:
Error:
required:
- code
- message
properties:
code:
type: integer
format: int32
message:
type: string
SuperTypeOne:
type: object
properties:
type:
type: string
value:
type: number
format: int32
discriminator:
propertyName: type
required:
- value
- type
TypeOne:
allOf:
- $ref: '#/components/schemas/SuperTypeOne'
type: object
properties:
type:
type: string
enum: [TypeOne]
value:
type: number
format: int32
enum: [ 1 ]
default: 1
uniqueOne:
type: string
default: Unique One
required:
- type
- uniqueOne
TypeTwo:
allOf:
- $ref: '#/components/schemas/SuperTypeOne'
type: object
properties:
type:
type: string
enum: [TypeTwo]
value:
type: number
format: int32
enum: [ 2 ]
default: 2
uniqueTwo:
type: string
default: Unique Two
required:
- uniqueTwo
SuperTypeTwo:
type: object
properties:
id:
type: string
format: uuid
date:
type: string
format: date-time
someStrings:
type: array
items:
type: string
TypeThree:
allOf:
- $ref: '#/components/schemas/SuperTypeTwo'
type: object
properties:
type:
type: string
enum: [TypeThree]
default: TypeThree
somethings:
type: array
minItems: 1
maxItems: 2
items:
oneOf:
- $ref: "#/components/schemas/TypeOne"
- $ref: "#/components/schemas/TypeTwo"
discriminator:
propertyName: type
required:
- type
- somethings
hmm. i added a new test that adds the value properties in a manner similar to what you show above
same PR here -> https://github.com/cdimascio/express-openapi-validator/pull/105/files
I'm still not able to reproduce
please send along your sample project that is able to trigger the issue.
also, feel free to review my test to make sure i'm exercising your case properly
@cdimascio try this:
https://github.com/catadch/express-openapi-validator-test
Let me know when you'e cloned it and I'll make it private again.
i cloned it, ran npm start
, but appears index.js
is missing
$ npm test
As in the README : )
gotcha. just picked up the readme change. i can run the test. thanks!
(fyi, i'm heading out shortly, but will definitely have a look tomorrow)
thanks for your help on this
No worries.
It's 01:00 where I am and I've been working on this for 48 hours straight, so i'm out of here too.
get some sleep :) hopefully we'll have some answers soon.
This is odd. I added your spec and tests into the test suite and they pass O.o
See this PR: #107
the api spec: https://github.com/cdimascio/express-openapi-validator/pull/107/files#diff-ed3d36f532fcf4bdd12dfce177b18308R1
There must be something else in the project that's playing a role. will poke around further in your example.
nevermind. it's now reproducible.
This looks like it may be an issue with Ajv and how defaults are handled. When in the context of oneOf
, allOf
, there is some ambiguity. This ambiguity may explain why we see a proliferation of errors in this particular case, that is, we see mix of errors, some related to each oneOf
type.
The details of the Ajv feature and ambiguity is described here https://github.com/epoberezkin/ajv/issues/42
Getting back to your example, I observed a couple of behaviors:
-
By removing the
default
keyword on theenum
valued property,value
, the validation errors no longer occur. Of course, you clearly want to provide a default. But seems to indicate the problem is with the default that is used. -
Note that in your example, you have two fields where you provide a default:
uniqueOne
anduniqueTwo
. These defaults are always properly applied. I believe that this is because the property is not used in bothoneOf
types, thus there is no confusion surrounding the default value. In the case ofvalue
, the property exists in both types, but has a different default value. I believe that this is a cause of ambiguity and that Ajv is unsure which default to use.
Ultimately, the best course of action may be to try to reproduce this using Ajv only.
If we can show Ajv is choosing the incorrect default it may be appropriate to file an issue there.
Other options might be to:
- modify the api to ensure the default values are the same for overlapping properties of
oneOf
types - avoid
oneOf
in cases where the default values are different for sharedoneOf
properties (this will cause some duplication across models) - keep the behavior you want, but provide a hacky workaround using middleware to remove the ambiguity. (Note that adding this middleware func to your test project will fix cause your tests to pass)
// Install middleware prior to the validator to handle these ambiguous defaults and inject their values prior to validation
app.use((req, res, next) => {
if (Array.isArray(req.body.somethings)) {
req.body.somethings.forEach(s => {
if (s.type === 'TypeOne' && !s.value) {
s.value = 1;
}
if (s.type === 'TypeTwo' && !s.value) {
s.value = 2;
}
})
}
next();
});
// install validator
new OpenApiValidator(opts).install(app);
// actual routes below here
// ....
All in all, discriminators should work for most cases, however, as we see here, when ambiguity is present in the schema and the discriminator is necessary to inform a choice, the validator may fail.
Ideally, Ajv would support this. However, in the case that it will not, a performant solution is non trivial. I'm open to ideas and suggestions.
On the bright side, this issue is limited to cases where oneOf or anyOf are used AND conflicting defaults exist in that oneOf or anyOf branch.
Keeping this open. Will investigate possible solutions for this case
@catadch curious to know your thoughts on my comments above and how you are currently planning to proceed
@catadch we now have support for top level discriminators. See the discriminator example here
Hey @cdimascio, did you have any thoughts on the direction you'd see being taken to support discriminators within the request body?
We're currently using the validator (and loving it so far) where we're supporting a request body which contains a top level attribute where the value is a map of elements where each could inherit from a base schema. Each one of those elements has their own set of required fields (though they share common fields which are required across each). To use the validation provided by this middleware, we're using anyOf
to match against any one of those elements. Something like below:
anyOf:
- $ref: '#/components/schemas/Question'
- $ref: '#/components/schemas/SubclassQuestionA'
- $ref: '#/components/schemas/SubclassQuestionB'
While the validation works as we'd expect, we do have an amount of duplication as we're not using polymorphism - not a biggie. As we're using the anyOf approach and not using discriminators, we do receive a verbose error output which we'd like to reduce, when sending an invalid request body as the validator is validating the invalid question against each schema. Each schema has a type
attribute which we could use if it were supported.
I'm happy to take a stab at this, just wondering had you any thoughts on how to go about this? Is there any gotchas to be aware of?