Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

[BUG] oneOf and similar tools unexpectedly require 2 or more items #492

Closed
Z5eyhS0uubejR0SVmX2O opened this issue Feb 1, 2021 · 13 comments · Fixed by #493 or asyncapi/spec-json-schemas#41
Labels

Comments

@Z5eyhS0uubejR0SVmX2O
Copy link
Contributor

Z5eyhS0uubejR0SVmX2O commented Feb 1, 2021

Describe the bug
The oneOf schema attribute requires 2 or more items, but should work with a single value in the array.

To Reproduce
Steps to reproduce the behavior:

  1. Create a document with a single definition listed in oneOf
  2. And attempt to generate HTML with the playground or generator
  3. See error that oneOf should NOT have fewer than 2 items

Expected behavior
By JSON Schema specification, there is no restriction that there be 2 or more items in a oneOf or siblings. In practice, tools including this JSON Schema Validator and Swagger Editor will accept a single definition for oneOf.
JSON schema example :

{
  "$schema": "https://json-schema.org/draft/2019-09/schema",
  "oneOf": [
    { 
      "type": "object",
      "properties": {
        "property": {
          "type": "string" 
        }
      },
      "required": [
        "property"
      ]
    } 
  ]
}

JSON value example:

{
  "property": "value"
}

OpenAPI 3 example:

openapi: "3.0.0"
info:
  title: Account Service
  version: 1.0.0
  description: This service is in charge of processing user signups
paths:
  /resource:
    get:
      operationId: get
      responses:
        '200':
          description: Some stuff
          content:
            application/json:    
              schema:
                type: object
                oneOf:
                  - type: object
                    properties:
                      property: 
                        type: string
                    required:
                    - property

Note: I'm not necessarily trying to say that "just because OpenAPI does it, you should", but I think the implication is that those groups of tools share an interpretation of the JSON schema spec which asyncapi does not. Sure would be nice if there was alignment though :-).

Sample document

asyncapi: 2.0.0
info:
  title: Account Service
  version: 1.0.0
  description: This service is in charge of processing user signups
channels:
  user/signedup:
    subscribe:
      message:
        $ref: '#/components/messages/UserSignedUp'
components:
  messages:
    UserSignedUp:
      payload:
        type: object
        oneOf:
          - type: object
            properties:
              property:
                type: string
            required:
              - property

Additional context
I might just work around this, but it's an adoption hurdle that I have to go in and modify my existing base of JSON schemas to adopt asyncapi. Otherwise, super psyched to have this tool added to the arsenal next to OpenAPI 3!

@github-actions
Copy link

github-actions bot commented Feb 1, 2021

Welcome to AsyncAPI. Thanks a lot for reporting your first issue. Please check out our contributors guide and the instructions about a basic recommended setup useful for opening a pull request.

Keep in mind there are also other channels you can use to interact with AsyncAPI community. For more details check out this issue.

@derberg
Copy link
Member

derberg commented Feb 2, 2021

@Z5eyhS0uubejR0SVmX2O this part is guilty here https://github.com/asyncapi/asyncapi/blob/master/versions/2.0.0/schema.json#L402. I checked the spec https://github.com/asyncapi/asyncapi/blob/master/versions/2.0.0/asyncapi.md and do not see anything specific saying that AsyncAPI Schema is restricting oneOf or anyOf to have at least 2 elements.

I'm of course curious why you need oneOf if you have just one item, but also as you noticed, json schema is not restrictive there.

@fmvilas is it a bug or is there a valid reason to have "minItems": 2 for oneOf or anyOf

@fmvilas
Copy link
Member

fmvilas commented Feb 2, 2021

I think it's a bug. There will be cases where the oneOf array is generated and might contain just a single element. If done by hand, I'd simply remove the oneOf keyword but that's not always possible/easy so this needs to be fixed. Thanks, @Z5eyhS0uubejR0SVmX2O for opening the issue!

@Z5eyhS0uubejR0SVmX2O
Copy link
Contributor Author

Z5eyhS0uubejR0SVmX2O commented Feb 2, 2021

@derberg I'm using it where I basically want inheritance. Using a $ref completely overrides the definition, but sometimes I want to simply extend a base schema with additional properties:

openapi: "3.0.0"
info:
  title: Account Service
  version: 1.0.0
  description: This service is in charge of processing user signups
paths:
  /resource1:
    get:
      operationId: get1
      responses:
        '200':
          description: Some stuff
          content:
            application/json:    
              schema:
                type: object
                oneOf:
                  - $ref: '#/components/schemas/SimpleObject'
                properties:
                  other_property:
                    type: string
  /resource2:
    get:
      operationId: get2
      responses:
        '200':
          description: Some other stuff
          content:
            application/json:    
              schema:
                $ref: '#/components/schemas/SimpleObject'
                properties:
                  other_property:
                    type: string
components:
  schemas:
    SimpleObject:
      type: object
      properties:
        property: 
          type: string
      required:
      - property

The other_propety in the response to get2 will not be rendered in generated HTML, beause that schema object will be completely overwritten by the $ref.

Because I sometimes do this, I actually overuse the oneOf approach more broadly because it's more versatile.

I mix and match these things regularly:

openapi: "3.0.0"
info:
  title: Account Service
  version: 1.0.0
  description: This service is in charge of processing user signups
paths:
  /simple_resource:
    get:
      operationId: get1
      responses:
        '200':
          description: Some stuff
          content:
            application/json:    
              schema:
                type: object
                oneOf:
                  - $ref: '#/components/schemas/BaseObject'
  /more_complex_resource:
    get:
      operationId: get2
      responses:
        '200':
          description: Some stuff
          content:
            application/json:    
              schema:
                type: object
                oneOf:
                  - $ref: '#/components/schemas/BaseObject'
                anyOf:
                  - $ref: '#/components/schemas/AdditionalDetailObject'
components:
  schemas:
    BaseObject:
      type: object
      properties:
        property: 
          type: string
      required:
      - property
    AdditionalDetailObject:
      type: object
      properties:
        other_propety: 
          type: string
      required:
      - property

@Z5eyhS0uubejR0SVmX2O
Copy link
Contributor Author

Z5eyhS0uubejR0SVmX2O commented Feb 2, 2021

@derberg @fmvilas
Humorously, I just noticed this line in asyncapi's schema.json: https://github.com/asyncapi/asyncapi/blob/master/versions/2.0.0/schema.json#L379-L382
Which would not be allowed in the schema being described: https://github.com/asyncapi/asyncapi/blob/master/versions/2.0.0/schema.json#L407-L413

@derberg
Copy link
Member

derberg commented Feb 2, 2021

@Z5eyhS0uubejR0SVmX2O thanks for describing the use case for me 🙇🏼

would you like to open up a PR to fix those bugs? It would have to get to asyncapi repo and asyncapi-node -> https://github.com/asyncapi/asyncapi-node/blob/master/schemas/2.0.0.json

yes, we need to duplicate atm 😞 but we are at least aware 😄

@Z5eyhS0uubejR0SVmX2O
Copy link
Contributor Author

@derberg PRs created, but I'm concerned the testing failures manifesting aren't something I directly caused with my commits.

Linting broken on node repo:
https://github.com/asyncapi/asyncapi-node/pull/41/checks?check_run_id=1817283654

Test broken specifically on windows-latest when looking for some version files on this repo:
https://github.com/asyncapi/asyncapi/pull/493/checks?check_run_id=1817232881

@derberg
Copy link
Member

derberg commented Feb 3, 2021

@Z5eyhS0uubejR0SVmX2O that was fast! thanks. I provided instructions on how to fix CI for asyncapi-node. For the failing test on windows in asyncapi repo, just do not worry, we will merge what you have and fix it with a followup PR

@derberg
Copy link
Member

derberg commented Feb 3, 2021

@fmvilas from my perspective I see those PRs as correct, so once @Z5eyhS0uubejR0SVmX2O updates one of them we can merge imho, objections?

@Z5eyhS0uubejR0SVmX2O
Copy link
Contributor Author

@derberg I would like to begin using the tool and it's a lot easier to do that if I don't have to go in and revise my existing JSON schemas :-)

@fmvilas
Copy link
Member

fmvilas commented Feb 3, 2021

@derberg Nope! Let's keep moving forward with bug fixes :)

derberg pushed a commit to asyncapi/spec-json-schemas that referenced this issue Feb 4, 2021
* Change minItems of oneOf and anyOf to 1
asyncapi/spec#492
* Add explicit lack of linter to fix PR Action
@derberg
Copy link
Member

derberg commented Feb 5, 2021

@Z5eyhS0uubejR0SVmX2O your fixes are already available in all asyncapi tools

@Z5eyhS0uubejR0SVmX2O
Copy link
Contributor Author

@Z5eyhS0uubejR0SVmX2O your fixes are already available in all asyncapi tools

Thank @derberg, I pulled the latest Docker image yesterday evening and started playing!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
3 participants