Improve swagger docs support #287

daddykotex · 2022-06-30T19:30:10Z

Address: #150

Also address a security issue with the url query param that was previously required.

The result looks like:

I'm still unsure about the user experience around smithy4s.http4s.swagger.multipleDocs[IO](HelloWorldService, PizzaAdminService)(path = "docs")

Let me know what you think

daddykotex · 2022-06-30T19:51:49Z

I'll mark as draft, just because I would need to update doc but will only do so when we agree on the UI to build the swagger http4s route

Baccata · 2022-07-01T07:26:28Z

This looks great ! To improve the UX, we can use the partially-applied pattern :

def docs : PartiallyAppliedDocs = new PartiallyAppliedDocs("docs") 

class PartiallyAppliedDocs(path: Path) {
   def atPath(path: Path) = new PartiallyAppliedDocs(path)
   def apply(first: HasId, rest: HasId*)= ...
}

That wouldn't be breaking the sources of most current usage I think.

Baccata · 2022-07-04T14:17:15Z

modules/http4s-swagger/test/src-ce3/TestCompat.scala

@@ -28,5 +28,5 @@ trait TestCompat { self: BaseIOSuite =>
  }

  def docs(path: String) =
-    Docs[IO](service, path, swaggerUiPath = "swaggerui")
+    Docs.multiple[IO](path, swaggerUiPath = "swaggerui")(service)


might as well remove the multiple method altogether and just document the new way of wiring things up 😄

Also we might want to make the swaggerUiPath configurable in the PartiallyApplied construct

Also we might want to make the swaggerUiPath configurable in the PartiallyApplied construct

I started doing it, but I found that cumbersome. I figured, if someone needs to go all the way down and customize the swagger ui, they can use Docs.build[IO](path, swaggerUiPath = "swaggerui")(service) like in this TestCompat

note build is the new name I used instead of multiples`

will update the docs, but not much has changed from a user point of view

will update the docs, but not much has changed from a user point of view

A screenshot of the drop-down menu added to the docs would be nice.

I can do that

daddykotex · 2022-07-04T14:51:04Z

Updated docs:

updated: see below

daddykotex · 2022-07-04T14:52:25Z

wow, chrome screenshot does not like that

v2:

Baccata · 2022-07-04T15:49:48Z

Right, so great work 👍 , but I'm gonna nitpick at the fact that there are too many ways to create doc routes. Comments incoming

Baccata · 2022-07-04T17:04:39Z

modules/http4s-swagger/src-jvm-ce2/Compat.scala

@@ -50,21 +55,19 @@ private[smithy4s] object Compat {
  }

  trait DocsCompanion extends SwaggerUiInit {
-    def apply[F[_]](
-        hasId: HasId,
+    def build[F[_]](


This method should disappear plainly and simply in favour of "smart-setters" in the PartiallyApplied construct.

Baccata · 2022-07-04T17:05:06Z

modules/http4s-swagger/src-jvm-ce3/Compat.scala

-      val docs = Docs[F](hasId, path)
-      docs.routes
+    def docs: PartiallyAppliedDocs = new PartiallyAppliedDocs("docs")
+    def atPath(path: String): PartiallyAppliedDocs =


This method should disappear plainly and simply in favour of "smart-setters" in the PartiallyApplied construct.

Baccata · 2022-07-04T17:06:06Z

modules/http4s-swagger/src-jvm-ce3/Compat.scala

+    def atPath(path: String): PartiallyAppliedDocs =
+      new PartiallyAppliedDocs(path)
+
+    class PartiallyAppliedDocs(path: String) {


This should receive smart setters to virtually "mutate" (read, immutable copy) of the path value and the swaggerUiPath value. The apply method should be the last thing the user call and should create the HttpRoutes

Additionally, it'd be great if some tests could be added to exercise the smart setters, in order to ensure that the API is the same across CE versions. The only thing that needs specialisation (in tests) is the creation of the PartiallyApplied, which should require a Blocker in CE2. The rest can be CE-version-agnostic

Baccata · 2022-07-04T17:12:26Z

modules/http4s-swagger/src-jvm/smithy4s/http4s/swagger/Docs.scala

-      PermanentRedirect(
-        Location(Uri.unsafeFromString(s"/$path/index.html?url=/$jsonSpec"))
-      )
+      Found(Location(Uri.unsafeFromString(s"/$path/index.html")))


I'm curious (and also somewhat illiterate when it comes to 3xx statuses) : can you tl;dr the "why" of changing the status ?

I changed it because 308 is cached very aggressively (forever) and thus you can't change a redirect afterwards. But I can revert that, I don't have an opinion on the matter. On quick look at MDN suggest that it's mostly for SEO purposes.

In my case, it was annoying because I had run a smithy project locally before, accessed the docs (for say PizzaAdmin) and now my browser was always redirecting me to this old url with pizza spec in it

Makes total sense, thanks !

Baccata · 2022-07-05T08:03:02Z

@daddykotex I made some changes to improve the UX a little, but it's great work 👍

daddykotex · 2022-07-05T12:59:06Z

build.sbt

@@ -520,32 +520,6 @@ lazy val `http4s-swagger` = projectMatrix
  )
  .http4sJvmPlatform(allJvmScalaVersions, jvmDimSettings)

-lazy val `doc-check` = projectMatrix


Ahh crap, I forgot to remove my test module loll

no worries :)

daddykotex · 2022-07-05T13:06:43Z

Ahhh, two interesting things:

it was my first time using the partially applied pattern, seen before, but never implemented it. I found it weird to have F[_] on apply rather that as a class type parameter. I see that you've replaced the former with the later, that's great
did not think about moving the blocker as case class member instead of a parameter to the apply method. again, good to see you did that

thanks

daddykotex added 5 commits June 30, 2022 15:28

Update & bump swagger dep

31402dc

Generate swagger init on the fly

67e3c95

Support multiple specs

f4674fb

Fix tests

a3b81c0

Fix copyright headers

83de21e

daddykotex marked this pull request as draft June 30, 2022 19:50

Use Partially applied pattern for docs ux

a372893

daddykotex force-pushed the dfrancoeur/swagger2 branch from ce53475 to a372893 Compare July 4, 2022 14:03

Baccata reviewed Jul 4, 2022

View reviewed changes

daddykotex added 2 commits July 4, 2022 10:25

Rename multiple to build

f98086c

Improve documentation for swagger setup

f2229c3

daddykotex marked this pull request as ready for review July 4, 2022 14:36

Tweak Open api support documentation

509b1d6

Baccata marked this pull request as draft July 4, 2022 15:58

Baccata reviewed Jul 4, 2022

View reviewed changes

daddykotex and others added 4 commits July 4, 2022 16:35

doc-check module for test

83cbfe0

Improve partially applied docs

74351b8

Clean-up, red tests

8e3c1f3

Rename withSwaggerUiPath to withSwaggerUiResources

607ea0d

Baccata marked this pull request as ready for review July 5, 2022 07:41

Remove doc-check module

3fa3ab1

Baccata approved these changes Jul 5, 2022

View reviewed changes

Baccata merged commit 1ba3a4f into main Jul 5, 2022

Baccata deleted the dfrancoeur/swagger2 branch July 5, 2022 12:38

daddykotex commented Jul 5, 2022

View reviewed changes

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Improve swagger docs support #287

Improve swagger docs support #287

daddykotex commented Jun 30, 2022

daddykotex commented Jun 30, 2022

Baccata commented Jul 1, 2022

Baccata Jul 4, 2022

daddykotex Jul 4, 2022

Baccata Jul 4, 2022

daddykotex Jul 4, 2022

daddykotex commented Jul 4, 2022 •

edited

Loading

daddykotex commented Jul 4, 2022

Baccata commented Jul 4, 2022 •

edited

Loading

Baccata Jul 4, 2022

Baccata Jul 4, 2022

Baccata Jul 4, 2022 •

edited

Loading

Baccata Jul 4, 2022

daddykotex Jul 4, 2022

Baccata Jul 5, 2022

Baccata commented Jul 5, 2022

daddykotex Jul 5, 2022 •

edited

Loading

Baccata Jul 5, 2022

daddykotex commented Jul 5, 2022

Improve swagger docs support #287

Improve swagger docs support #287

Conversation

daddykotex commented Jun 30, 2022

daddykotex commented Jun 30, 2022

Baccata commented Jul 1, 2022

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

daddykotex commented Jul 4, 2022 • edited Loading

daddykotex commented Jul 4, 2022

Baccata commented Jul 4, 2022 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Baccata Jul 4, 2022 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Baccata commented Jul 5, 2022

daddykotex Jul 5, 2022 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

daddykotex commented Jul 5, 2022

daddykotex commented Jul 4, 2022 •

edited

Loading

Baccata commented Jul 4, 2022 •

edited

Loading

Baccata Jul 4, 2022 •

edited

Loading

daddykotex Jul 5, 2022 •

edited

Loading