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 extensions are not supported #671

Open
julienrf opened this issue Nov 5, 2020 · 14 comments
Open

OpenAPI extensions are not supported #671

julienrf opened this issue Nov 5, 2020 · 14 comments

Comments

@julienrf
Copy link
Member

julienrf commented Nov 5, 2020

OpenAPI extensions are used by various tools such as documentation generators, API gateways, or code generators. Here is an attempt to list all of them: https://github.com/Mermade/openapi-specification-extensions

It should be possible to add extension properties to the endpoints documentation.

@julienrf
Copy link
Member Author

julienrf commented Nov 6, 2020

While thinking about this, one problem I have is that extensions can contain arbitrary JSON data. This means that an API described at the algebra level needs to be able to construct JSON values:

trait MyEndpoints extends endpoints4s.algebra.Endpoints {
  val foo = endpoint(
    get(path / "foo"),
    ok(emptyResponse),
    EndpointDocs()
      .withExtension("x-some-extension" -> Json.obj("bar" -> Json.str("baz"))) // HERE
  )
}

Currently, this is not possible because the algebra does not depend on a JSON library, and the philosophy of endpoints4s is to not introduce such a dependency.

So, I see two solutions.

  1. We abstract over the JSON AST in the algebra, and the openapi interpreter fixes it to the ujson AST (this is what is currently used by the openapi interpreter),
  2. We introduce our own (concrete) JSON AST.

Introducing our JSON AST would have the following benefits:

  • less abstract things,
  • this is an opportunity to also fix the default ujson AST which can’t represent some JSON numbers (it can only represent JavaScript numbers).

But it would have the following drawbacks:

  • yet another JSON AST,
  • we might loose some performance if we are not super careful in the design?

Thoughts?

@Fristi
Copy link
Contributor

Fristi commented Nov 6, 2020

In my playground project (similar project to this one), I also introduced a JSON AST. It allowed me to export the JsonSchema to various formats like circe, etc.

Also it would nice to be able to encode extra information about JsonSchema. Like a refined Int can have interval bounds, which is supported by JsonSchema. As well as a String can have a regex

@Fristi
Copy link
Contributor

Fristi commented Nov 6, 2020

This was the algebra I was using

trait JsonAlgebra[F[_]] extends JsonAlgebraFormatN[F] with Invariant[F] with Partial[F] with CoCartesian[F] {
  def int(bound: Range): F[Int]
  def float(bound: Range): F[Float]
  def double(bound: Range): F[Double]
  def long(bound: Range): F[Long]
  def string(description: StringDescriptor): F[String]
  val bool: F[Boolean]
  val cnil: F[CNil]
  def option[A](from: F[A]): F[Option[A]]
  def list[A](of: F[A]): F[List[A]]
  def set[A](of: F[A]): F[Set[A]]
  def vector[A](of: F[A]): F[Vector[A]]
  def seq[A](of: F[A]): F[Seq[A]]
  def nel[A](of: F[A]): F[NonEmptyList[A]]
  def enum[A](all: Set[A], show: A => String): F[A]
}

@bmeesters
Copy link
Contributor

Would this JSON AST be used outside of extensions? If not I don't think the performance would be really a problem.

I like working with Endpoints4s, but it has a bit of a learning curve. More abstract stuff would mean more traits to mixin, and this already can get a bit confusing if you are new to this sort of working.

@julienrf
Copy link
Member Author

Would this JSON AST be used outside of extensions?

We could use it to encode and decode JSON entities, but this is not required (we can continue using the default ujson AST). I have an intuition that it should be possible to not use an intermediate AST (by using an abstract algebra like @Fristi shown), this is why I’m tempted to explore this approach as well.

@bmeesters
Copy link
Contributor

I would be happy to help (I have some time starting next week), though I am not entirely sure how you guys envision the solution. Let me know if there is anything I can do to help

@julienrf
Copy link
Member Author

@bmeesters I think this blog post could be a good source of inspiration: https://www.lihaoyi.com/post/ZeroOverheadTreeProcessingwiththeVisitorPattern.html

So, most probably, we want to fix the type JsonSchema[A] in our interpreters to something that provides a Visitor[A]:

trait JsonSchema[A] {
  def visitor: Visitor[A] // Or maybe a `Visitor[Validated[A]]` so that we can produce nice error messages?
}

At the moment, it is not clear to me why the ujson visitor takes two type parameters, instead of just one: https://www.javadoc.io/doc/com.lihaoyi/upickle-core_2.13/latest/upickle/core/Visitor.html
(FTR, it was forked from Jawn, which uses only one type parameter: https://www.javadoc.io/doc/org.typelevel/jawn-parser_2.13/latest/org/typelevel/jawn/Facade.html)

Also, I must say that I’m still not sure how would this (parsing JSON entities to case classes without using an intermediate AST) interact with the current issue (defining an abstract algebra for constructing JSON values vs defining a refied AST).

@bmeesters
Copy link
Contributor

Alright, I'll look into it (next week). I'll see if I can figure something out and get back to you.

@bmeesters
Copy link
Contributor

So I looked a bit more into it, but I don't have that much time, and I think there are too many open questions now to be able to solve it this week. Instead I am going to take a smaller issue. So anybody feel free to start on this.

@agboom
Copy link
Contributor

agboom commented Feb 24, 2021

Just thinking out loud here (I'm still learning about both Endpoints4s and OpenAPI extensions), but could a design without requiring a Json interpreter at this level also work? For example, instead of defining the extension as Json object, would it be possible to define the extension as a data type and requiring a JsonSchema in the interface? Or would this overcomplicate things?

I imagine you could create something like this:

trait MyEndpoints extends endpoints4s.algebra.Endpoints with endpoints4s.algebra.JsonSchemas {
  case class Bar(bar: String)

  val barSchema: JsonSchema[Bar] = (
    field[String]("bar")
  ).xmap(Bar.apply)(unlift(Bar.unapply))
 
  val foo = endpoint(
    get(path / "foo"),
    ok(emptyResponse),
    EndpointDocs()
      .withExtension("x-some-extension" -> Bar(bar = "baz"))(barSchema) // HERE
  )
}

@julienrf
Copy link
Member Author

That’s an interesting idea! I think it would work, although it’s a bit convoluted.

@agboom
Copy link
Contributor

agboom commented Feb 26, 2021

I agree that it would be quite involved. The JSON AST approach sounds more elegant and has more benefits as you pointed out.

@mleclercq
Copy link
Contributor

I think @agboom idea is interesting because is allows to decouple the question of defining a JSON AST from the question of supporting OpenAPI extensions. If I understand correctly, the JSON AST would have its own JsonSchema so if @agboom proposal was implemented as is, then JSON AST could be passed to withExtension instead of custom types like Bar in the example. In other words, both approaches would be possible.

@mleclercq
Copy link
Contributor

I gave it a try using JsonSchemas (see mleclercq@ad6317c).

Here is what it looks like in practice:

trait MyOpenApiExtensions extends openapi.ExtensionsFromSchemas {
  val `x-foo` = extensionType[String]("x-foo")
  val `x-bar` = extensionType[Int]("x-bar")
}

trait MyEndpoints extends algebra.Endpoints {
  val e = endpoint(
    get(path / "e"),
    ok(emptyResponse)
  )
}
object MyEndpointsDocumentation
    extends MyEndpoints
    with openapi.Endpoints
    with openapi.JsonSchemas
    with MyOpenApiExtensions {

  val api =
    openApi(Info("MyEndpoints", "1.0"))(
      e.withExtensions(`x-foo`("helloworld"))
    )
      .withExtensions(`x-bar`(42))
}

With this approach, extensions are not specified on the EndpointDocs but instead are specified on DocumentedEndpoints and OpenApi objects.

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

No branches or pull requests

5 participants