openapi-examples-validator icon indicating copy to clipboard operation
openapi-examples-validator copied to clipboard

New feature: validate examples based on discriminator feature

Open gigaga opened this issue 4 years ago • 6 comments

Closes #155

From this PR, we are able to validate examples that contain "discriminator". However to can validate a specification, inheritance have to be in the following form :

---
openapi: 3.0.2
info:
  title: petstore Inheritance
  version: 1.0.7
paths:
  "/pet":
    post:
      tags:
      - pet
      summary: Add a new pet to the store
      description: Add a new pet to the store
      operationId: addPet
      requestBody:
        description: Create a new pet in the store
        content:
          application/json:
            schema:
              $ref: "#/components/schemas/Pet"
            examples:
              dog:
                value:
                  name: "12"
                  catName: "123"
                  type: "CAT"
        required: true 
      responses:
        '200':
          description: Successful operation
          content:
            application/json:
              schema:
                "$ref": "#/components/schemas/Pet"
        '405':
          description: Invalid input
components:
  schemas:
    Dog: 
      type: object
      properties:
        dogName:
          type: string
        type:
          type: string
          enum:
            - DOG
    Cat:
      type: object
      properties:
        catName:
          type: string
        type:
          type: string
          enum:
            - CAT
    Pet:
      required:
      - name
      - type
      type: object
      properties:  
        id:
          type: integer
          format: int64
          example: 10
        name:
          type: string
          example: doggie
        type:
          type: string
      discriminator:
        propertyName: type
      oneOf:
        - $ref: "#/components/schemas/Cat"
        - $ref: "#/components/schemas/Dog"

In other terms :

  • mapping keyword hasn't be present
  • main schema has reference sub-schemas by using oneOf keyword, and not by using anyOf from sub-schemas
  • each sub-schema has to specify the discriminator property with its related value

gigaga avatar Dec 01 '21 11:12 gigaga

Just for reference.. this is the continuation of #156

codekie avatar Dec 01 '21 14:12 codekie

Hi @codekie ,

Can you tell me when you think that this PR could be merged? I'm waiting for your next release on my project ;)

Regards,

gigaga avatar Dec 08 '21 10:12 gigaga

Hi @gigaga . I will need some time to thoroughly review and test this before I merge it into the main-branch, as this feature contains breaking changes. Due to my current workload, it's unlikely to happen this month.

But you gotta be aware that even when I have merged this, there won't be a new major release very soon, as there are other features that I'd like to include (in the next major release) which take some time to develop.

But what I can tell at a quick glance (I still have to do a thorough review yet, though) is that the included tests for the new feature are rather rudimentary. I'd like to see additional test-cases that cover (for example):

  • Unsupported inheritance-type (eg. MOUSE)
  • With noAdditionalProperties set (to also make sure that the child-classes are not merged)

I also suggest to make dogName and catName required fields (that's the only field that distinguishes the types Cat and `Dog from each other and right now, it's optional and thus could be omitted, or simply disabled by typos in the property-name).

I know, the code-change for this feature may appear minor on the first glance, but it opens a whole new set of potential issues. And the more we can cover with automated tests, the better. The tests are not only there to check the current implementation, but also make it more robust to future changes (like refactoring, dependency upgrades, ...). That's why I mentioned cases like before. So, the more test-cases you can come up with, that are related to inheritance, the better.

codekie avatar Dec 09 '21 13:12 codekie

Hi @codekie Can you tell me when this feature could be merged ? ;)

Regards,

gigaga avatar Nov 17 '22 13:11 gigaga

Hi @gigaga . I can't make any promises, but maybe I have some time in the upcoming two weeks to look into this.

codekie avatar Nov 17 '22 13:11 codekie

Great news :) Thanks for all :D

gigaga avatar Nov 17 '22 15:11 gigaga

Hi @codekie ,

Can you tell me when this PR could be merged ? ;)

Regards,

gigaga avatar Jan 13 '23 09:01 gigaga

Sorry, no. I still have to address the before-mentioned issues.

codekie avatar Jan 13 '23 10:01 codekie

Hey @codekie, do you have any updates on this? We are working on a customer project and this is the only issue that stops us from having this library go to production code. Do you have any estimate on when you could have a look and merge this? Would be really helpful for us. Thank you in advance!

crosa7 avatar May 18 '23 14:05 crosa7

@crosa7 I had a look into it again. I have fixed the failed tests after the update-merge, but I have identified a couple of breaking changes that I need to have a closer look at first.

I don't want to make any estimates at this point as I have made them before and I couldn't hold them, but I'm on it and I'd like to have this feature there. I just want to make sure that everything else still works properly afterwards.

codekie avatar May 21 '23 16:05 codekie

Thanks a lot for your effort @codekie. We will be looking forward to it

crosa7 avatar May 21 '23 18:05 crosa7