maksbotan commented 4 years ago

Hi @fisx, @fizruk, @phadej, @Sir4ur0n, @roosemberth.

I've finished this PR. I've added openapi3 dependency under a flag, disabled by default, to make servant-swagger-ui support both swagger2 and openapi3 packages, until the former is deprecated.

Do you have objections or other proposals?

I also hope for some preliminary testing before I make Hackage releases.

sir4ur0n commented 4 years ago

I've finished this PR. I've added openapi3 dependency under a flag, disabled by default, to make servant-swagger-ui support both swagger2 and openapi3 packages, until the former is deprecated.

Isn't it inconsistent with the duplication of servant-swagger and swagger2? I don't mind per se, but I find it weird for developers to have a clear separation between Open API 2 and Open API 3 for the Swagger + Servant integration but not for the UI 😅

That being said, I will try to test this tomorrow on our (private, sorry) application and give you feedback!

Again, thank you for this work 🙇‍♂️

maksbotan commented 4 years ago

@Sir4ur0n hi! how is it going with the testing? :)

roosemberth commented 4 years ago

Do you have objections or other proposals?

LGTM.

I don't have the time to thoroughly review this MR, but it looks good and this is great! Thanks!

sir4ur0n commented 4 years ago

Hey, sorry for the delay, we had some hot stuff that came up. Now I'm testing the full combo!

Here are my thoughts/remarks

Required code changes on our side

Maybe this section is of no interest, but I track here what changes I had to do to make everything compile in our project. Maybe if somebody else wants to give it a try, this would save them some time :smile: I thought these may also be useful to help write the changelog/migration guide

Adapt `stack.yaml`

extra-deps:
  # Open API 3 support
  # https://github.com/biocad/openapi3/
  - git: git@github.com:biocad/openapi3.git
    commit: bd9df532f2381c4b22fe86ef722715088f5cfa68
  # https://github.com/biocad/servant-openapi3/
  - git: git@github.com:biocad/servant-openapi3.git
    commit: 022875ea189c0b4b49ad6e3ae95298a5ba977a57
  # See https://github.com/haskell-servant/servant-swagger-ui/pull/77
  - git: git@github.com:maksbotan/servant-swagger-ui.git
    commit: 421f01455da9e04b8d7e19a6f91860350593a299
    subdirs: 
      - servant-swagger-ui
      - servant-swagger-ui-core
flags:
  servant-swagger-ui:
    openapi3: true
  servant-swagger-ui-core:
    openapi3: true

Adapt dependencies in `package.yaml`

- - servant-swagger
- - swagger2
+ - servant-openapi3
+ - openapi3

Some modules have moved

- import Data.Swagger
+ import Data.OpenApi

- Swagger
+ OpenApi

- import Servant.Swagger
+ import Servant.OpenApi

- HasSwagger
- toSwagger
+ HasOpenApi
+ toOpenApi

- import Servant.Swagger.UI (SwaggerSchemaUI)
+ import Servant.Swagger.UI (OpenApiSchemaUI)

Definitions was changed to Components -> Schemas

- & definitions %~ foobar
+ & components . schemas %~ foobar

Note: apparently Data.OpenApi exposes a value function, which provokes many shadowing warnings in our code. No big deal, but as warnings break our CI, I thought it was worth mentioning

Add `ToSchema` instances to our sum types

This is one of the key features we were looking for :smile: E.g.

-- | In a common JSON customization module
sumUnaryOptions :: Options
sumUnaryOptions = customDefaultOptions {sumEncoding = UntaggedValue}

sumUnarySchema :: SchemaOptions
sumUnarySchema = fromAesonOptions sumUnaryOptions

-- | For every sum type add this instance
instance ToSchema Foobar where
  declareNamedSchema = genericDeclareNamedSchema sumUnarySchema

Conclusion

The tedious part was finding the replacement modules/types/functions and finding the right dependencies/tags to use in stack.yaml, but otherwise this was a smooth migration. As usual, Haskell's compiler is the big brother taking care of checking everything, so as soon as it compiled it Just Worked™.

This is AWESOME work, thank you so much, and congratulations :tada:

Besides Hackage/Stackage publishing, are there any big steps remaining? I think on our side we will switch to these Open API 3 libraries today or next week :smile:

Cheers!

maksbotan commented 4 years ago

Thanks very much, @Sir4ur0n!

I'll make Hackage releases soon.

fizruk commented 4 years ago

I've finished this PR. I've added openapi3 dependency under a flag, disabled by default, to make servant-swagger-ui support both swagger2 and openapi3 packages, until the former is deprecated.

Do you have objections or other proposals?

@maksbotan Great work! I would advise to create a separate package for discoverability (openapi3, servant-openapi3 and servant-openapi3-ui is a nice lineup) and ease of use (no flag tweaking for end users).

maksbotan commented 4 years ago

@fizruk I've actually found a way to make separate servant-openapi-ui without duplicating the actual UI files :) Please take another look.

maksbotan commented 4 years ago

I've updated swagger-ui to 3.35.1 and removed git dependency on openapi3 since that is now on Hackage.

I guess it's time to merge and release this package as well.

Profpatsch commented 4 years ago

servant-swagger-ui-core still depends on swagger2, is this intended?

Profpatsch commented 4 years ago

And

"servant-openapi-ui" -> "servant-swagger-ui";
"servant-openapi-ui" -> "servant-swagger-ui-core";

meaning servant-openapi-ui has a transitive dependency on swagger2.

Profpatsch commented 4 years ago

Okay lol, stack building these two modules at the same time just made my machine OOM:

openapi3               > [ 5 of 17] Compiling Data.OpenApi.Internal
swagger2               > [ 2 of 17] Compiling Data.Swagger.Internal.AesonUtils
swagger2               > [ 3 of 17] Compiling Data.Swagger.Internal.TypeShape
swagger2               > [ 4 of 17] Compiling Data.Swagger.Internal.Utils
swagger2               > [ 5 of 17] Compiling Data.Swagger.Internal

maksbotan commented 4 years ago

I've updated this with my recent changes to master. Sadly I don't yet see a way to fix the "depends on both swager2 and openapi3" problem. servant-swagger-ui and servant-openapi-ui share the same swagger-ui (js) code and I don't want to duplicate that code in the both packages. Perhaps this can be split even further?

Profpatsch commented 4 years ago

servant-swagger-ui and servant-openapi-ui share the same swagger-ui (js) code and I don't want to duplicate that code in the both packages.

I have not looked at the code in detail, but I don’t see a reason why a bit of duplication would be unreasonable if it gets rid of quite serious double dependencies.

Gau-thier commented 3 years ago

Hello, Thanks for the job done! Do you have any news about this PR? I mean, will it be merged to master, and when?

Thanks!

maksbotan commented 3 years ago

Hi @gsebil08! Thanks for the interest :) I couldn't come up with a nice way to avoid duplication and was overwhelmed by other stuff lately. I will try to do in the non-creative way soon.

Anyway, I've been using the version from this PR for my work and it works well.

Gau-thier commented 3 years ago

Hi @maksbotan! Thanks for your quick reply! I used to work with @Sir4ur0n (😘), so we are also using the version from this PR and it works like a charm!

I was simply upgrading some of our dependencies in our project, and wondering about the status of this one 😄

maksbotan commented 3 years ago

Hi all! @Sir4ur0n @gsebil08 and especially @phadej, please review this once again.

I've found a nice (I think) way to generalize this to support both swagger2 and openapi3 without actually pulling both packages into deps. Namely, I've made all serve* functions accept arbitrary ToJSON a => a as input and serve plain Value in APIs. This way the package no longer depends even on swagger2.

Please tell me what you think.

I think I will go ahead with merging and Hackage release if there are no responses in a week.

phadej commented 3 years ago

@maksbotan I'm not participating in servant development, don't expect me to review.

Gau-thier commented 3 years ago

Hello @maksbotan, thanks for the suggestion! I don't feel legitimate to review the changes, but still I tried to used this on my project. And I am facing a weird issue and I don't know how to bypass it. On my first attempt, I tried to link to the latest commit of this PR in my stack.yaml file:

resolver: lts-17.2
packages:
  - .
extra-deps:
  - git: git@github.com:maksbotan/servant-swagger-ui.git
    commit: 6576953c39bf1407813ba1880525ed08ea8af9eb

And got:

Cloning 6576953c39bf1407813ba1880525ed08ea8af9eb from git@github.com:maksbotan/servant-swagger-ui.git
No cabal file found for Repo from git@github.com:maksbotan/servant-swagger-ui.git, commit 6576953c39bf1407813ba1880525ed08ea8af9eb

So I tried to add the subdirs section, as following:

  - git: git@github.com:maksbotan/servant-swagger-ui.git
    commit: 6576953c39bf1407813ba1880525ed08ea8af9eb
    subdirs:
      - servant-swagger-ui
      - servant-openapi-ui

and finally face:

Cloning 6576953c39bf1407813ba1880525ed08ea8af9eb from git@github.com:maksbotan/servant-swagger-ui.git
No cabal file found for Repo from git@github.com:maksbotan/servant-swagger-ui.git, commit 6576953c39bf1407813ba1880525ed08ea8af9eb in subdir servant-openapi-ui

I must have miss something which I can't find on my own... Do you have any idea?

maksbotan commented 3 years ago

Hi @gsebil08,

Use subdirs servant-swagger-ui-core and servant-swagger-ui. servant-openapi-ui is gone, no longer needed :)

Gau-thier commented 3 years ago

Thanks for your answer. I was lost between the changes I must do and the changes @Sir4ur0n did in the first place. Here are the changes I did on our project:

package.yaml

- servant-openapi-ui
+ servant-swagger
+ servant-swagger-ui

OpenApiServer

- import Servant.OpenAPI.UI (openapiSchemaUIServer)
+ import Servant.Swagger.UI (swaggerSchemaUIServer)

stack.yaml

- commit: 09ee7a2325d37b0c733edd89b3264403c4d82ff9
-    subdirs:
-      - servant-openapi-ui
+ commit: 6576953c39bf1407813ba1880525ed08ea8af9eb
+    subdirs:
+      - servant-swagger-ui

And in our swagger-ui the oneOf feature is still working great!!

If you really want, I can have a look on the changes you made on your pull request, but as I am a "simple user" I am not sure it will be useful...

maksbotan commented 3 years ago

Great it worked for you!

Profpatsch commented 3 years ago

Namely, I've made all serve* functions accept arbitrary ToJSON a => a as input and serve plain Value in APIs. This way the package no longer depends even on swagger2.

Please tell me what you think.

Sound good!

haskell-servant / servant-swagger-ui

OpenAPI 3 support #77

Required code changes on our side

Adapt `stack.yaml`

Adapt dependencies in `package.yaml`

Some modules have moved

Definitions was changed to Components -> Schemas

Add `ToSchema` instances to our sum types

Conclusion

package.yaml

OpenApiServer

stack.yaml

haskell-servant / servant-swagger-ui

OpenAPI 3 support #77

Required code changes on our side

Adapt stack.yaml

Adapt dependencies in package.yaml

Some modules have moved

Definitions was changed to Components -> Schemas

Add ToSchema instances to our sum types

Conclusion

package.yaml

OpenApiServer

stack.yaml

Adapt `stack.yaml`

Adapt dependencies in `package.yaml`

Add `ToSchema` instances to our sum types