redoc icon indicating copy to clipboard operation
redoc copied to clipboard

oneOfs inside allOf produces incorrect "Incompatible types" warning, and lacks location

Open mgabeler-lee-6rs opened this issue 2 years ago • 2 comments

Describe the bug An allOf that contains overlapping oneOfs 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 $refs, 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.

mgabeler-lee-6rs avatar Jun 07 '22 16:06 mgabeler-lee-6rs

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.

RomanHotsiy avatar Jun 08 '22 06:06 RomanHotsiy

FWIW, despite the warnings, the "visualization" redoc provides for this sample schema seems completely reasonable.

mgabeler-lee-6rs avatar Jun 09 '22 14:06 mgabeler-lee-6rs