Gi60s / openapi-enforcer

Apache License 2.0
94 stars 22 forks source link

Provide a switch to report examples with non-explicitly defined properties #132

Closed phillim1 closed 2 years ago

phillim1 commented 2 years ago

This is a request to provide a switch to allow the reporting of non-explicitly defined properties in an example in a Swagger definition when additionalProperties: false is not present.

For example, in the attached sample API, lines 79-81 contain the fields documentType, documentStatus and documentDetails in an example - these fields are not explicitly defined in the API. When this file is validated with openapi-enforcer then the fields are correctly reported as invalid because the additionalProperties: false is defined on line 107.

However, most of our APIs do not define the additionalProperties keyword and we would like to be able to report when the examples become out of synch with the explicitly defined properties. Hence, if additionalProperties: false is removed from line 107 then it would be great if openapi-enforcer could provide a warning that the example fields might be out of synch with the schema. Is this possible?

Note that this request is similar to this recent comment on additionalProperties behaviour.

Gi60s commented 2 years ago

Hello @phillim1 and thanks for the issue. I think you're right that it is important to add this as a warning.

Looking at the current code I could make it so that a warning is always shown for examples that have additional properties, but there are a few potential hurdles:

  1. The only way to allow you to turn this on or off without it being a significant amount of work updating the library is to create a global configuration option. I might do that.
  2. There will be cases when using a discriminator or allOf will cause the warning to appear even when all properties are defined via the additional schemas.

As an example, take these components:

components:
  schemas:
    Cat:
      allOf:
      - "$ref": "#/components/schemas/Pet"
      - type: object
        properties:
          birthDate:
            type: string
            format: date
          huntingSkill:
            type: string
    Pet:
      type: object
      required:
      - petType
      discriminator:
        propertyName: petType
        mapping:
          dog: Dog
          cat: "#/components/schemas/Cat"
      properties:
        petType:
          type: string
      example:
        petType: cat
        birthDate: '2000-01-01'
        huntingSkill: stealth

The example is on the Pet schema, but the warning would produce something like this:

One or more warnings exist in the OpenApi definition
  at: components > schemas > Cat > allOf > 0 > example
    Invalid value
      at: birthDate
        Property is an additional property
      at: huntingSkill
        Property is an additional property
      Did not validate against all schemas
        at: 1 > petType
          Property is an additional property

Also, I'd like to mention that version 2 of this library is making a lot of progress. I can add this functionality to that library without too much work and there is also significantly more control over managing errors and warnings global and on a case-by-case basis. I'm looking forward to having that complete as it will also bring browser support, TypeScript types, and more. 🎉

After having explained the two hurdles, what thoughts do you have?

Gi60s commented 2 years ago

I wanted to provide a second example where I put the example in the Cat instead of the Pet.

components:
  schemas:
    Cat:
      allOf:
      - "$ref": "#/components/schemas/Pet"
      - type: object
        properties:
          birthDate:
            type: string
            format: date
          huntingSkill:
            type: string
        example:
          petType: cat
          birthDate: '2000-01-01'
          huntingSkill: stealth
    Pet:
      type: object
      required:
      - petType
      discriminator:
        propertyName: petType
        mapping:
          dog: Dog
          cat: "#/components/schemas/Cat"
      properties:
        petType:
          type: string

Warning:

One or more warnings exist in the OpenApi definition
  at: components > schemas > Cat > allOf > 1 > example
    Invalid value
      at: petType
        Property is an additional property
phillim1 commented 2 years ago

Thanks for the speedy response and analysis.

Despite the allOf/discriminator issue, it would still be useful to us to have the global option (maybe flagged as an 'experimental' option) to find examples that are out of sync with the explicitly defined properties regardless of additionalProperties settings.

Typically our APIs use example at field level, however, we are beginning to define more examples in newer APIs for use with mock servers, so reporting out of sync examples is valuable to us even if there are false positives.

If v2 allows specific message overrides based on message context then that would be great since we could set any messages issued as a result of discriminator/allOf from WARN to INFO.

In v2 the line numbers and location relating to an issue would also be useful to us.

Another (probably hopeful!) possibility for v2 would be to provide a diff tool for 2 APIs - at the moment we use this tool - https://github.com/Azure/openapi-diff - but we would like to switch partly because it doesn't support OAS3.

Gi60s commented 2 years ago

I will add the global option to toggle the warning for additionalProperties.

As for location, line numbers, and position, those are implemented and working in v2, so that is good news.

I'll take a look at the diff tool, but is there anything specifically that you're looking for it to do? Do you care about comparing structures or the order of defined items? Also, it's possible that the diff tool may be outside the scope of the enforcer, but it wouldn't be hard to build a package on the side that did that.

phillim1 commented 2 years ago

We use the diff tool in our API build pipelines to compare the modified version of the API with the currently deployed version of the API and if the diff tool issues Errors then the build fails.

The diff tool produces a report with Errors, e.g. adding a required parameter, and Warnings, e.g. adding an optional field. The list of possible issues are on this page. Note that the message severity depends on the context of the issue e.g. whether versions have changed, whether the field is in a request or response etc.

When we believe the tool has not functioned correctly, we allow Errors to be overridden using a file specific to an API that specifies which messages (with context) can be overridden.

Hence our diff requirements derive from the capabilities offered by the diff tool.

Gi60s commented 2 years ago

This feature has been published to NPM as version 1.17.0. There isn't really anything you need to change to make this work, it will simply show up as a warning now.

To turn it off refer to this page in the docs: https://openapi-enforcer.com/api/#enforcer-config