Gi60s / openapi-enforcer

Apache License 2.0
94 stars 22 forks source link

EnforcerException on default query parameter #145

Closed thomasheartman closed 1 year ago

thomasheartman commented 1 year ago

Hey! 👋🏼 Great work on this package 🙌🏼 It's great at catching errors and I love that it also gives you warnings 😄

Description

Using Enforcer on our own spec, I ran into an issue: it doesn't seem to allow default values for query parameters when the query parameter can have different types. In other words, the following query param definition seems to be invalid:

 "parameters": [
          {
            "name": "download",
            "schema": {
              "default": false,
              "anyOf": [
                {
                  "type": "boolean"
                },
                {
                  "type": "string",
                  "minLength": 1
                },
                {
                  "type": "number"
                }
              ]
            },
            "in": "query"
          }
]

From what I can tell (according to the swagger docs), it should be valid. And as an important note: the enforcer does accept default query parameters if the schema has a root type. In other words, both of these pass the enforcer validation (but are incompatible with our generated types):

Simple type:

 "parameters": [
          {
            "name": "download",
            "schema": {
              "default": false,
              "type": "boolean"
            },
            "in": "query"
          }
]

Complex type and simple type (should probably be invalid)

 "parameters": [
          {
            "name": "download",
            "schema": {
              "default": false,
              "type": "boolean", // <- note this addition
              "anyOf": [
                {
                  "type": "boolean"
                },
                {
                  "type": "string",
                  "minLength": 1
                },
                {
                  "type": "number"
                }
              ]
            },
            "in": "query"
          }
]

Expected outcome / request

To be honest, I'm not sure whether this is a bug or whether we've just not annotated the types correctly. But I haven't been able to find anything to say there is a different way of annotating it at the moment, so I'm leaning towards this being a bug.

I would expect this to be accepted (or at least be easily ignorable with an error code).

If, on the other hand, this is me messing up, I'd love it if you could tell me how. Judging from the docs I linked previously, the default property should go on the schema property, but I may have made other mistakes.

Reproducible example

I used the following JSON spec to encounter this issue. It's quite large, but it doesn't have any other errors than the one this issue is about. It does have a few warnings we should address, however, but that is for us to fix.

OpenAPI spec pastebin link

(Sorry, the spec is too large for GitHub to allow me to post it 🙈 )

Background

The query parameter can be any string or number for backwards compatibility reason. Hence, we cannot change the type directly in the API without breaking it.

Gi60s commented 1 year ago

Hello, @thomasheartman. I'd like to start by apologizing for the long wait on my response. I was out of town (no internet) last week and I'm still catching up. I'll try and take a good look at this today so hopefully you'll hear back from me either late today or tomorrow.

Thank you for your patience and I'm glad to hear that you've enjoyed the library so much.

Gi60s commented 1 year ago

You are correct that I should be handling this differently. You can now specify a default value at the top level of an anyOf or oneOf schema. I tested the schema that you shared and didn't see any errors.

This has been published to npm as version 1.21.1.

Thanks for the issue. I'm glad I could make an improvement on this.

thomasheartman commented 1 year ago

Hey, @Gi60s 😄 No worries at all about the response time! We all deserve a bit of time off and away, so it's all good.

Thanks for looking into it and fixing it 🙌🏼 I'll take it for a spin later today or next week 🔥

Cheers 🙏🏼