search

metadevpro / openapi3-ts

TS Model & utils for creating and exposing OpenAPI 3.x contracts.
MIT License
485 stars 64 forks source link

v3.0: `exclusiveMaximum` must be a boolean #83

Closed RobinTail closed 2 years ago

RobinTail commented 2 years ago

Lines 306 and 308 have been changed in this commit: https://github.com/metadevpro/openapi3-ts/commit/e7a08516a86620a66a16f4552d5b9affd6c77987

These changes have been released in version 3.0.0.

However, according to this spec, exclusiveMaximum must be boolean: https://datatracker.ietf.org/doc/html/draft-wright-json-schema-validation-00#section-5.3

More than that, the Swagger validator complains about numeric value as well: image

I think that the mentioned commit should be reverted. Otherwise, could you please clarify what was the reason for that change?

RobinTail commented 2 years ago

CC @OrIOg

RobinTail commented 2 years ago

Ok, I found the PR that describes the reason: https://github.com/metadevpro/openapi3-ts/issues/78

And this looks a bit strange to me, since my link to the spec is taken from the swagger.io website: https://swagger.io/specification/

Schema Object

The Schema Object allows the definition of input and output data types. These types can be objects, but also primitives and arrays. This object is an extended subset of the JSON Schema Specification Wright Draft 00 1. For more information about the properties, see JSON Schema Core 2 and JSON Schema Validation 3. Unless stated otherwise, the property definitions follow the JSON Schema.

The document (3) clearly says that the property MUST be boolean: https://datatracker.ietf.org/doc/html/draft-wright-json-schema-validation-00

However the one referred in the PR says it must be a number: https://json-schema.org/draft/2020-12/json-schema-validation.html#rfc.section.6.2.3

Presuming that the one in the PR is newer, it's still questionable how to deal with the complains in Swagger validator (see the screenshot)

RobinTail commented 2 years ago

Here is another link to the documentation that describes exclusiveMaximum as boolean: https://swagger.io/docs/specification/data-models/data-types/?sbsearch=exclusiveMaximum

It's based on OpenAPI 3.0 and as per my understanding it should not be breaking changes in 3.1 as well. So, it's still confusing.

RobinTail commented 2 years ago

Holy Moly, they really changed it without any backward compatibility:

https://www.ietf.org/rfcdiff?url1=draft-wright-json-schema-validation-00&url2=draft-wright-json-schema-validation-01&difftype=--html

image

pjmolina commented 2 years ago

Looks like the change is coming from different versions of JSON Schema. OpenAPI is not redefining them, but using them directly from JSON Schema. See: https://json-schema.org/understanding-json-schema/reference/numeric.html

Latest spec OpenAPI 3.1.0 uses JSON Schema Specification Draft 2020-12 (the number version)

So, my best guess is we should stick to numbers.

Take in mind (last time I checked) Swagger Editor (from swagger.io) was still supporting spec. v2. and not yet 3.x

RobinTail commented 2 years ago

Yeah, I see. So, if the library is currently only 3.1 compatible, should we reflect it on openapi version here, for example: https://github.com/metadevpro/openapi3-ts/blob/master/src/dsl/OpenApiBuilder.ts#L16 ?

And there also a lot of comments/jsdoc in the code that refer to 3.0, while the versions are so different and backward incompatible.

What do you think?

Also, which tool would you recommend to validate 3.1 specification? Swagger.io validates 3.0.x only.

@pjmolina

pjmolina commented 2 years ago

Indeed, we can enforce 3.1.0 but also thinking about relaxing the type for exclusiveMaximum and exclusiveMinimum to be number | boolean to help people to transition from OpenAPI 3.0 (boolean) to 3.1. (number)

What do you think it would be more useful? What do you need in your use cases @RobinTail 3.0.0 or 3.1.0?

pjmolina commented 2 years ago

Also, which tool would you recommend to validate 3.1 specification?

A good list of validators and compatibility is maintained here: https://openapi.tools/#data-validators Support for 3.1 has a "slow adoption" so I am more inclined to still support 3.0 and 3.1 in this lib if we can handle the complexity.

Take a look also to this one: https://github.com/seriousme/openapi-schema-validator

RobinTail commented 2 years ago

Indeed, we can enforce 3.1.0 but also thinking about relaxing the type for exclusiveMaximum and exclusiveMinimum to be number | boolean to help people to transition from OpenAPI 3.0 (boolean) to 3.1. (number) What do you think it would be more useful?

Well, I believe that ideally, in a case like this, the type should depend on the version literal and be generic.

For example, consider this:

type OpenApiVersion = `3.${number}.${number}`;

export interface OpenAPIObject<V extends OpenApiVersion> {
  openapi: V;
  // ...
}

export interface SchemaObject<V extends OpenApiVersion> {
  // ...
  exclusiveMaximum?: V extends `3.1.${number}` ? number : boolean;
}

It requires at least Typescript 4.1 to support type literals. And perhaps more entities, and most likely the OpenApiBuilder class, would have to become generic too. I'm not sure about this yet, since it increases the code complexity. But the benefit of it — keeping the single but generic type declaration.

I will think more on other ways too. @pjmolina

RobinTail commented 2 years ago

What do you need in your use cases @RobinTail 3.0.0 or 3.1.0?

For my case, I need to provide users of my library with a tool that generates the specification of their API, that could be used for the following cases:

I'm not sure at the moment which OpenAPI versions are currently supported by Webstorm and Postman, but Swagger still only supports 3.0. I'm going to find out and let you know soon. Thank you @pjmolina

OrIOg commented 2 years ago

Holy Moly, they really changed it without any backward compatibility:

https://www.ietf.org/rfcdiff?url1=draft-wright-json-schema-validation-00&url2=draft-wright-json-schema-validation-01&difftype=--html

image

I should have checked it in depth before reporting the issue & the PR. It is my bad, sorry. It does seems that Swagger revisions isn't really support 3.1 even though they talk about the openapi 3.1 in the version But it would be great to be able to support both versions.

I'll try testing @RobinTail 's PR on my side, even with the breaking changes it might be the way to do it correctly. There might be a way by extending said schema and changing definition, but pros & cons are to be established

RobinTail commented 2 years ago

It's ok, @OrIOg . I prepared another one: #85 , according to @pjmolina vision. It's much simpler and might fulfill all needs.