redoc
redoc copied to clipboard
oneOfs inside allOf produces incorrect "Incompatible types" warning, and lacks location
Describe the bug
An allOf
that contains overlapping oneOf
s produces a confusing and incorrect warning about incompatible types:
Incompatible types in allOf at "undefined": "string" and "object"
Expected behavior Should not generate warnings for valid schemas
Minimal reproducible OpenAPI snippet
openapi: 3.1.0
info:
title: bug demo
paths:
/:
get:
responses:
'200':
content:
application/json:
schema:
allOf:
- oneOf:
- type: string
- type: object
- oneOf:
- type: string
- type: object
Screenshots N/A
Additional context
After a brutal debugging session (I was unable to find a way to test this using a non-minified version of redoc
, and the minified version is worse than most, because most of the code seems to be inside eval
strings or something?) I traced this down from a much more complex real schema to this minimal test case.
It seems like the way hoistOneOfs
and mergeAllOf
work together is ... not correct. It ends up checking that every possible combination of each oneOf
in the allOf
is valid, and that is not correct. What I think it should be checking is that some possible combination is valid. The schema is only invalid if no combination is valid, not if any combination is invalid.
Note that, if the allOf
branches are replaced with $ref
s, this sometimes goes away, I think due to short circuiting? It only seems to be happening with "dereferenced" schemas like the example above.
Also, the lack of a proper pointer to where in the schema this came from (at "undefined"
) drastically increased the time it took to trace this down. While the reformatted schemas it's generating may not have an exact pointer, there's at least a "greatest common ancestor" type value that could be given here I think.
This is indeed working not 100% correctly.
The issue is we are not checking if anything is valid this is not validator. We try to somehow visualize the schema in a way that someone without json schema knowledge would understand.
The issue is in mergeAllOf
code which we are going to rewrite from scratch at some point. It does not eliminate non-existing cases (like conflicting types, etc).
Thanks for simplified case. It will help us to implement better allOf merging.
FWIW, despite the warnings, the "visualization" redoc provides for this sample schema seems completely reasonable.