OpenAPI 3 - Githubissues

maksbotan commented 4 years ago

Hi,

I've started porting swagger2 to openapi3, I will track my progress here.

I am following the spec at https://swagger.io/specification/, version 3.0.3.

Currently it works for simple cases:

λ> (decs, resp) = runDeclare (declareResponse "application/json" (Proxy @Day)) mempty
λ> resps = mempty & responses.at 200 ?~ Inline resp :: Responses
λ> op = mempty & responses .~ resps :: Operation
λ> swag = mempty & components.schemas <>~ decs & paths.at "/day" ?~ pathItem :: Swagger
λ> BSL.putStrLn $ encode swag

{"openapi":"3.0.3","info":{"version":"","title":""},"paths":{"/day":{"get":{"responses":{"200":{"content":{"application/json":{"schema":{"$ref":"#/components/schemas/Day"}}},"description":""}}}}},"components":{"schemas":{"Day":{"example":"2016-07-22","format":"date","type":"string"}}}}

And it renders as expected on Swagger Editor:

Of course, servant-swagger will have to be updated as well... See https://github.com/haskell-servant/servant-swagger/issues/99

Fixes #105.

maksbotan commented 4 years ago

So, test suite passes, doctests not yet, I will fix them later.

OpenAPI 3 pet store example roundtrips without losses, so that's something.

Of course, Generic generators should be updated to use new features like oneOf.

Any preliminary thoughts?

sir4ur0n commented 4 years ago

I am not qualified to review this PR, but I just wanted to let you know this work on OpenAPI is valuable (I understand this is a first step before adding support for sum types oneOf; a good step nonetheless) and I thank you/appreciate your work on this PR :bow:

maksbotan commented 4 years ago

Update: I've implemented schema generation for (all) sum types, using SumEncoding configuration from aeson.

Example using Light type from tests:

data Light
  = NoLight
  | LightFreq Double
  | LightColor Color
  | LightWaveLength { waveLength :: Double }
  deriving (Generic)

Light:
  type: object
  oneOf:
    - required:
        - tag
        - contents
      type: object
      properties:
        tag:
          type: string
          enum:
            - NoLight
        contents:
          example: []
          items: {}
          maxItems: 0
          type: array
    - required:
        - tag
        - contents
      type: object
      properties:
        tag:
          type: string
          enum:
            - LightFreq
        contents:
          format: double
          type: number
    - required:
        - tag
        - contents
      type: object
      properties:
        tag:
          type: string
          enum:
            - LightColor
        contents:
          $ref: '#/components/schemas/Color'
    - required:
        - waveLength
        - tag
      type: object
      properties:
        tag:
          type: string
          enum:
            - LightWaveLength
        waveLength:
          format: double
          type: number

Validation tests pass.

There are some TODOs remaining thoigh.

maksbotan commented 4 years ago

Yay, CI passes!

sir4ur0n commented 4 years ago

Hey, just wanted to let you know: as soon as you feel like it works (even if not polished/documented/etc. yet), feel free to ping me, and we will give it a try on my project. We have several sum types, which we currently can't display in Swagger, so we would love to modestly contribute by beta-testing this and provide feedback (or contribute on what we understand, e.g. the documentation).

Cheers and good luck!

maksbotan commented 4 years ago

Hi @Sir4ur0n.

This mostly works now, I'm going to add more tests for new functionality (sum types) and recheck everything by the spec.

However, keep in mind that OpenAPI 3 changed a lot of things, so a migration is certainly needed. And if you are using servant-swagger: it won't work with this PR because of those changes.

But anyway, you are more than welcome to try this code :)

sir4ur0n commented 4 years ago

However, keep in mind that OpenAPI 3 changed a lot of things, so a migration is certainly needed. And if you are using servant-swagger: it won't work with this PR because of those changes.

This is exactly the piece I didn't have in mind :sweat_smile:

I'm not familiar with the boundaries between this library and servant-swagger: will compilation break? Or runtime? Maybe adapting servant-swagger wouldn't be such a big deal?

It would be much smoother if we can test with servant-swagger, but I guess we could give it a manual shot by displaying the JSON in another UI :thinking:

maksbotan commented 4 years ago

Sadly, compilation most likely will break. You can try though :)

Adapting servant-swagger is on my list, I'll go to it after this PR is in ready state.

maksbotan commented 4 years ago

Hi @fizruk, @phadej, @fisx and everyone interested!

I've marked this PR as ready for review. I tried to clean up the history with a rebase.

Here are some thoughts.

This is a massive change, just as the change in the spec itself. I suppose that it would make sense to split it into a separate openapi3 package.
servant-swagger is migrated in https://github.com/haskell-servant/servant-swagger/pull/121, perhaps it should be called servant-openapi3 as well.
OpenAPI 3 got simplified with respect to parameters, so SwaggerKind and stuff are no longer required. Maybe there are some relics of it left though.
I'm ready to hear your suggestions on the code and work together to make this change happen.

maksbotan commented 4 years ago

Thanks for your review, @roosemberth! I will look at this :)

fizruk commented 4 years ago

Hi @fizruk, @phadej, @fisx and everyone interested!

I've marked this PR as ready for review. I tried to clean up the history with a rebase.

@maksbotan Thank you for this amazing work!

Here are some thoughts.

This is a massive change, just as the change in the spec itself. I suppose that it would make sense to split it into a separate openapi3 package.

Yes, I think this is the most sensible thing to do, since it should be way easier to maintain that way! Feel free to just create a copy of this repo named openapi3.

servant-swagger is migrated in haskell-servant/servant-swagger#121, perhaps it should be called servant-openapi3 as well.

Yes, same goes for servant-openapi3.

OpenAPI 3 got simplified with respect to parameters, so SwaggerKind and stuff are no longer required. Maybe there are some relics of it left though.

That sounds great! Less code is better 👍

I'm ready to hear your suggestions on the code and work together to make this change happen.

One suggestion is to also adapt servant-swagger-ui, since it is the trio of packages I find extremely useful (and I think others do as well) 😊

Another suggestion might be to update doctests to use YAML, as I see that it is what people use often, and also it is easier to read in documentation, I think.

At lastly, perhaps a list of key changes since Swagger 2.0 (or a link to such a list) somewhere in the obvious place might be great for users to switch over :) Especially, I believe we should emphasize that sum types are now encoded (more) faithfully in OpenAPI 3.0 than in Swagger 2.0, which should be a great motivator to move to the new package. We could also add references openapi3, servant-openapi3 and servant-openapi3-ui to the front pages of swagger2 packages, for people to find new packages faster :)

maksbotan commented 4 years ago

Thanks for the response!

I will make new packages in the biocad/ org.

Are you going to review this PR first or I can just proceed with copying?

I will prepare a list of changes, as you suggested, once the repo is a new place.

roosemberth commented 4 years ago

IMHO: I think it would be a good idea to finish reviewing the PR here (maybe change the target branch?) and then fork-off the merged branch.

fizruk commented 4 years ago

Thanks for the response!

I will make new packages in the biocad/ org.

Perfect!

Are you going to review this PR first or I can just proceed with copying?

I glanced over it, it looks good to me, so I think you can proceed. You can take the suggestion of @roosemberth to complete merge here (in a separate branch), or proceed in any other way you want :)

I will prepare a list of changes, as you suggested, once the repo is a new place.

Great, thank you! 😊

maksbotan commented 4 years ago

Thanks @roosemberth for your review!

maksbotan commented 4 years ago

So, the new home is at https://github.com/biocad/openapi3/ 🎉

I hope I did not mess with copyrights and acknowledgments. I've also added @fizruk as a maintainer to that repo.

By all means, check it out and file issues :)

sir4ur0n commented 4 years ago

Hi, I'm asking here pretty much the same questions I asked there:

Hi, so what's the status of this PR? Will this development land in swagger2 or remain in a dedicated openapi3 package?

If new package:

Shouldn't swagger2 readme/documentation state that it's a deprecated package, and users should migrate to openapi3? I don't see the point of maintaining both packages (except for a few months for retro-compatibility reasons)

If it lands in this package:

Are there remaining steps to merge it?

Thank you!

GetShopTV / swagger2

OpenAPI 3 #219