opticdev / optic

OpenAPI linting, diffing and testing. Optic helps prevent breaking changes, publish accurate documentation and improve the design of your APIs.
https://useoptic.com
MIT License
1.36k stars 83 forks source link

warning: invalid allOf variant warning if allOf includes oneOf #2827

Open corneliu-petrescu opened 7 months ago

corneliu-petrescu commented 7 months ago

Hi,

We're seeing a parse warning when trying to lint the following API spec:

Example: -- invalid allOf variant

  ---
openapi: 3.0.3
info:
  title: Test
  version: 3.0.0
servers:
  - url: /test
    description: Test
tags:
  - name: Test
    description: test
paths:
  /test:
    post:
      description: test
      operationId: test
      tags:
        - Test
      requestBody:
        content:
          application/vnd.api+json:
            schema:
              allOf:
                - type: object
                  properties:
                    prop1:
                       type: string
                  required: [prop1]
                - oneOf:
                    - type: object
                      properties:
                        prop2:
                          type: string
                    - type: object
                      properties:
                        prop3:
                          type: string
      responses:
        '201':
          description: success
          content:
            application/vnd.api+json:
              schema:
                type: object
            location:
              schema:
                type: string
niclim commented 7 months ago

Ah interesting, it looks like our mergeAllOf handler doesn't handle this case.

This might be a little trickier to implement, given each oneOf child would result in a top level polymorphic item, and you could have multiple oneOf children which would result in all combinations of polymorphic options.

corneliu-petrescu commented 7 months ago

We have 2 or 3 APIs that use the above approach. What would be the impact of it at this moment? Is it just a warning?

niclim commented 6 months ago

I think right now it'll exclude values in oneOf blocks that are nested inside of an allOf block. This would affect the diff that's generated and rules run against it.

corneliu-petrescu commented 6 months ago

For now I'm investigating rewriting some of the schemas using

oneOf
 - allOf
     prop1
     prop2
 - allOf
     prop1
     prop3

This does not give any warnings

checkin247 commented 5 months ago

I think I just came across the same ...

optic diff reference\Configuration.yaml --last-change 
  1. invalid allOf variant at reference\Configuration.yaml:103:3535 this is a model with a allOf of two models, where the second model has a oneOf (many)
  2. Operations: No operations changed where I have remove the required attribute of a property in a Model listed in the oneOf