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

Openapi 3.1 #17

Merged
merged 28 commits into from
Oct 11, 2022
Merged

Openapi 3.1 #17

merged 28 commits into from
Oct 11, 2022

Conversation

hamnis
Copy link
Contributor

@hamnis hamnis commented Aug 19, 2022

Based on the PR in softwaremill/tapir#1271

Added some missing properties.
Need to work through the spec to make sure we have everything

Fixes #16
Adds parsing of openapi files for #2


sealed trait AnySchema extends SchemaLike

object AnySchema {
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

suggestions for names here?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

not entirely sure if its useful to have the encodings in the model, or if this should possibly be a configuration of encoder.

prefixItems, contains, patternProperties

Introduce new Pattern wrapper to make it possible to write a validator for regex
@hamnis
Copy link
Contributor Author

hamnis commented Aug 29, 2022

Need to figure out how to deal with AsyncAPI since it does not really support all of json-schema. Should we assume that users that care about async-api will limit themselves to the subset that it supports?

@hamnis hamnis marked this pull request as ready for review August 29, 2022 10:28
@hamnis
Copy link
Contributor Author

hamnis commented Sep 8, 2022

@adamw is this a direction that is interesting for the library?

@adamw
Copy link
Member

adamw commented Sep 14, 2022

@hamnis Sure, sorry I somehow thought that this is WIP. I think most of these changes are source-backwards-compatible, so we should be able to use this in tapir, replacing the OpenAPI version?

@hamnis
Copy link
Contributor Author

hamnis commented Sep 14, 2022

It is mostly source compatible yes, but it is not binary compatible.
A new major version of this library is probably warranted.

I just noticed the build failed. I will have a look.

@adamw
Copy link
Member

adamw commented Sep 14, 2022

Yes, though in case of sttp-apispec we're still in 0.x versions, and as for tapir - this doesn't affect core, where bin compat rules apply, so should be good :)

@rafalambrozewicz
Copy link
Contributor

H @hamnis

Thanks for your input. I'm trying to figure out if we could utilize these changes in the current tapir documentation creation logic by using locally build sttp-apispec from this branch as tapir dependency and running the tests. After hardcoding "3.0.3" as schema version in OpenAPI object and not supporting "ArraySchemaType" when converting from tapir schema to openapi/asyncapi schema, I see two minor and two major problems:

The small ones are:

  1. "patternProperties": it looks like for the given message component payload the "patternProperties" field is present even though it is an empty ListMap in a model. For example when running "VerifyAsyncAPIYamlTest" the result is as follows:
components:
  messages:
    string:
      payload:
        type: string
        patternProperties: {}
      contentType: text/plain

but the expected form is:

components:
  messages:
    string:
      payload:
        type: string
      contentType: text/plain
  1. "example" / "default" / "value": instead of one element value, an one-element array is used. For example when running "should use default for a query parameter" in "VerifyYamlTest" the result is as follows:
parameters:
  - name: name
    ...
    schema:
      type: string
      default: 
        - tom
    example: 
      - alan

but the expected value is:

parameters:
  - name: name
    ...
    schema:
      type: string
      default: tom
    example: alan

If I'm not mistaken both of these problems should be solved as a part of this PR as encoders are here.

The big ones i.e. breaking backward compatibility are:

  1. exclusive minimum / maximum handling:
    In 3.0.3:
schema:
  type: integer
  format: int32
  minimum: 0
  exclusiveMinimum: true
  maximum: 42
  exclusiveMaximum: true

In 3.1:

schema:
  type: integer
  format: int32
  exclusiveMinimum: 0
  exclusiveMaximum: 42

"exclusive" is no longer a boolean (failing test: "exclusive bounds" / "VerifyYamlTest")

  1. null values handling
    In 3.0.3:
properties:
  optionalIntField:
    type: integer
    format: int32
    nullable: true

In 3.1:

properties:
  optionalIntField:
    type:
      -integer
      -'null'
    format: int32

Old handling is not allowed as "In order to achieve the goal of letting JSON Schema tools understand OpenAPI, it was decided to remove nullable entirely instead of deprecate it." (failing test: "should mark optional fields as nullable when configured to do so" in "VerifyYamlTest")

I'm not sure how to gracefully handle these... ( FYI @adamw )

@hamnis
Copy link
Contributor Author

hamnis commented Oct 4, 2022

the two small ones I will fix. Adds these as tests here as well.

for the big ones, I suppose we can do something like I did for encoding of AnySchema, meaning a configuration of the Encoders.

@adamw
Copy link
Member

adamw commented Oct 4, 2022

Yeah I guess having a single model, and configurable 3.0 and 3.1 encoders will do the trick. Thanks :)

@hamnis
Copy link
Contributor Author

hamnis commented Oct 4, 2022

I suppose we could also add a jsonschema-circe module here and let the openapi depend on that.

@hamnis
Copy link
Contributor Author

hamnis commented Oct 4, 2022

@rafalambrozewicz I think all issues should be resolved now.

Copy link
Contributor

@rafalambrozewicz rafalambrozewicz left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks, @hamnis - it did the trick, still, one new issue popped up, and two minor ones arose.

  1. unknown "extensions:" keyword gets added to yaml, when we want to add an extension. For example in "should add openapi extensions to the schema" / "VerifyYamlTest" test, the expected result is:
components:
  schemas:
    FruitAmount:
        ...
      type: object
      properties:
        ...
      x-schema:
        string: a
        int: 1

but the actual is:

components:
  schemas:
    FruitAmount:
        ...
      type: object
      properties:
        ...
      extensions:
        x-schema:
          string: a
          int: 1     

Two other issues are minor as produced yml is valid, but the order of properties seems odd.

  1. null handling, null information is the very first element, usually it is added as last (test: "should mark optional fields as nullable when configured to do so" / "VerifyYamlTest"):
    expected:
optionalIntField:
  type:integer
  format:int32
  nullable:true

actual:

optionalIntField:
  nullable:true
  type:integer
  format:int32    
  1. exclusiveMinimum / exclusiveMaximum information not next to "minimum" "maximum" (test: "exclusive bounds" / "VerifyYamlTest").
    expected:
          schema:
            type: integer
            format: int32
            minimum: 0
            exclusiveMinimum: true
            maximum: 42
            exclusiveMaximum: true

actual:

        schema:
          type: integer
          format: int32
          minimum: 0
          maximum: 42
          exclusiveMinimum: true
          exclusiveMaximum: true

- Extension expansion
- grouping of min/max with exclusive min/max
- nullable at end
@hamnis
Copy link
Contributor Author

hamnis commented Oct 5, 2022

@rafalambrozewicz I tested with running the tests on tapir/openapi docs and it passed. So I think this should be good now.

https://gist.github.com/hamnis/3c1339e98627fe51d40018d431f95ca8

Copy link
Contributor

@rafalambrozewicz rafalambrozewicz left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks, @hamnis I confirm that we are good now since all tapir docs generation tests are passing. In general, it looks good to me 👍 Still - I've added some questions and suggestions.

build.sbt Show resolved Hide resolved
build.sbt Outdated Show resolved Hide resolved
build.sbt Outdated Show resolved Hide resolved
Added to make sure we need to check as little as possible for validation purposes.
@rafalambrozewicz rafalambrozewicz merged commit 2c63ef6 into softwaremill:master Oct 11, 2022
@hamnis hamnis deleted the openapi-3.1 branch October 11, 2022 10:25
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Openapi 3.1 support in the model
3 participants