endpoints4s / endpoints4s

Describe HTTP endpoints in Scala and derive clients, servers, and documentation
https://endpoints4s.github.io
MIT License
405 stars 97 forks source link

Be able to compose endpoints at the algebra level #794

Closed agboom closed 2 years ago

agboom commented 3 years ago

Previously discussed in https://gitter.im/endpoints4s/endpoints4s?at=60548b4aa7dfe1372ee3921a

Problem description

With the current algebras, it is not possible to compose endpoints. For example, the following can be declared in an abstract algebra, but its implementation can only exist in a concrete interpreter (Akka, http4s etc.):

def authenticatedEndpoint[A, B, AA, BB](endpoint: Endpoint[A, B]): Endpoint[AA, BB] // tuplers left out for brevity

In a sense, this method adds functionality to an existing endpoint and returns a new endpoint. This can then be passed to another similar function, hence composability. The current way to add functionality to an existing endpoint is to define a new method that accepts all the constituents (method, url, response etc.) and proxy these to the existing endpoint method. This is done for example in the BasicAuthentication algebra. For a single layer this is fine, but it poses a problem if you want to add multiple types of functionalities to an endpoint. For example, consider an endpoint that you want to have some custom errors, an authentication layer and some versioning in the url. At least in our use cases we have encountered this a few times.

It would be great to be able to compose it like authenticatedEndpoint(endpointWithError(versionedEndpoint(endpoint(request, response)))). However, this is not possible because the algebra types are abstract and/or do not have the necessary operations to achieve this.

Possible design directions

As discussed in Gitter, there are several possible design directions to consider. We have come up with two general directions, both with plusses and minuses. They are general directions in the sense that they're not fully working yet. We felt that sharing these early on would help to get a discussion going and have the highest chance to be accepted. Both designs are pretty disruptive for the fundamental algebras, so that's one of the things we could discuss and see how to improve on that.

Concrete datatype in algebra

One way to deal with this is to define a separate concrete datatype for Request[A], Response[A] and/or Endpoint[A, B], such as RequestPayload[UrlP, BodyP, HeadersP]. This allows you to access its constituents at the algebra level and thus implement methods such as the one above. We've experimented with this in a separate branch.

A drawback is that it requires an extra datatype and it needs to be materialized at some point to the interpreter specific type (see def materialize in the example). Another drawback as you can see in the example is that existing operations such as def get and def orElse do not work with RequestPayload unless we rewrite them. But rewriting them makes them incompatible for the Request datatype.

Structural types

Another way, which promises to be more idiomatic to endpoints4s is to expand the existing abstract types Request[A] etc. to a structural type with the necessary type interface. The interpreters then can adhere to this new type while the algebra operations can access its constituents via the structural type interface. We've experimented with this as well in another branch.

An upside of this approach is that it's based on the existing abstract type and it can be made source compatible, maybe even binary compatible? However, the linked example does not type check and we weren't able to figure out yet why. The meat of the change is in these lines. The other changes are attempts to make it compile.

Follow ups

Our main goal with this issue is to get the word out and gather input from the community. What do you think of the design directions? Which one works best for the future of endpoints4s? Is there a direction we missed that should be explored? Any suggestions on making the code compile are also welcome :)

If we agree on the direction we can continue working on that, i.e. make it compile, properly test it and create a PR.

bmeesters commented 3 years ago

I also tried out an approach similar to 'Concrete data type' but slightly different: https://github.com/endpoints4s/endpoints4s/compare/master...bmeesters:composable-endpoints (you should read it from the last file to the first since the interpreters are above the algebra).

It is still crude and incomplete, but I do think it shows promise. It works as follows:

The upside is there are no algebra changes necessary, only interpreters might need to change (for example akka-http-server). Another upside is that it is relevatively a small amount of new code. The downside is that the code contains duplication, since the logic of the interpreters now need to live in two places. Another issue is that the lazy variants have a lot of type parameters, making type signatures harder to read. We might be able to solve both though with some refactoring, but I didn't go that far yet since a lack of time.

@julienrf I am curious what you think of the three provided solutions here.

edit: a small side-note is that the my approach currently prioritizes being as least disruptive as possible, but if we do not care (too much) and are willing to make a 2.0 version, then we could change a lot of the original type signatures on the methods and avoid any duplication of logic.

julienrf commented 3 years ago

Thanks for the detailed and thoughtful posts!

From what I understand, what we need is to have extractors for retrieving the underlying Request and Response from an Endpoint, and so on for every entity?

So, maybe we should add the following methods to the algebra?

// (Using Scala 3 syntax with extension methods, for brevity)
trait Endpoints {

  type Request[A]
  type Response[B]
  type Endpoint[A, B]
  // Constructor
  def endpoint[A, B](request: Request[A], response: Response[B]): Endpoint[A, B]
  // Extractors
  extension [A, B](endpoint: Endpoint[A, B]) {
    def request: Request[A]
    def response: Response[B]
  }
  // ... and so on for the other entities: Request, Response, etc.
}

Would that be enough?

bmeesters commented 3 years ago

That could be enough, if we could also get the underlying information of those requests and responses. For example so that we can add auth to the request headers. Ultimately, the interpreters would also need to have some payload information to be able to implement those extension methods right? Is that possible if a Request does not have the underlying type information anymore?

edit the reason for these complex type signatures is as I could not think of a way to get the header/url/body information back. I suppose only Requests are a bit problematic now, since a Request[A] loses that information. But maybe you have some ideas for more extension methods on Request to make that possible? Also these extractors are also breaking right? Since it adds more constraints to the interpreters?

julienrf commented 3 years ago

I realize now that the Endpoint[A, B] example is not enough to see the whole picture because it is different from, say, Request[A] in the sense that a Request[A] is made of a verb, a URL, and possibly an entity and headers, whose specific type is not fully known.

So, if we try to sketch what the extractors would look like in the case of Request[A], it would be the following:

trait Endpoints {

  type Request[A] <: {
    type Url
    type Headers
    type Entity
  }

  extension [A](request: Request[A]) {
    def url: Url[request.Url]
    def headers: RequestHeaders[request.Headers]
    def entity: RequestEntity[request.Entity]
  }
}

So, we end up with something along the lines of what you both proposed (@agboom was using type members, and @bmeesters was using type parameters).

We would need to play a little bit more with such a design to see if it is usable in practice…

bmeesters commented 3 years ago

Yes indeed (though I think type members would be nicer than type parameters). I wonder whether the new type Request you propose has any run-time overhead, since this is a structural type right, or is that not true for type only members? Otherwise this could also work:

type Request[A] <: RequestLike

trait RequestLike {
    type Url
    type Headers
    type Entity
  }

Which kind of similar to the approaches we sketched. I think your solution uses extension methods instead of inheritance but it otherwise the same in spirit.

Note though that this is a breaking change in the algebra, since it requires new members. Is this okay for you? If it is @agboom and me would both be willing to work it out in more detail and make a full PR

On another note, loving the new extension syntax in Scala 3 :smile:

Another edit: not only does the algebra change, the interpreters would basically also all become case classes to be able to provide the extension methods. A function (like in akka-http) would not be enough

julienrf commented 3 years ago

I wonder whether the new type Request you propose has any run-time overhead, since this is a structural type right, or is that not true for type only members?

The type Request[A] <: { type Foo; type Bar } has no run-time overhead compared to type Request[A]. The version you proposed with a dedicated trait RequestLike may have an overhead in some cases (e.g., we could not write type Request[A] = Unit like we can today), although I don’t think this could become critical…

Note though that this is a breaking change in the algebra, since it requires new members. Is this okay for you?

Good point. From a compatibility perspective, what is forbidden is to add abstract methods to a trait. What we have been doing so far, in that case, was to provide a default implementation that throws an exception. This forces someone who depends both on a module that uses algebra 1.x, and on a module that uses algebra 1.y to bump its interpreters to support the highest version.

bmeesters commented 3 years ago

I see, thanks for the explanation.

What do you propose? Take your idea and create a full PR? We can do the heavy lifting if you are available for the review if you like?

julienrf commented 3 years ago

Sure, I would be happy to help/review if you want to do the heavy lifting :slightly_smiling_face: It might be a good idea to experiment outside of endpoints4s first (with a very limited subset of the algebra), just to check that the approach is usable in practice, and that it solves the issue.

agboom commented 3 years ago

Thanks for looking at the proposal and the suggested ideas @julienrf! I did not think of the <: notation in structural types, that could prove to be quite useful!

agboom commented 3 years ago

@julienrf I've created another branch where I experimented with your suggestions (i.e. type Request[A] <: { ... }).

https://github.com/endpoints4s/endpoints4s/compare/master...simacan:structural-types-2

I've managed to make the tests pass for Akka HTTP server (at least sbt ~akka-http-server/testOnly *ServerInterpreterTest works). There are some issues here and there:

BTW, do you know what the semantic difference is between type A <: { ... } and type A = { ... } in Scala? Both are valid syntax, but I couldn't find any documentation regarding their semantics.

harpocrates commented 3 years ago

I'm personally a little bit against switching to concrete types or even to adding generalized combinators like authenticatedEndpoint, although I greatly appreciate the discussion and design process of this thread an elsewhere! I think that it is a feature that endpoints and their components can't be modified or post-processed too much (at least not in the abstract interpreter) after they've been initially defined.

Instead of rewriting the core algebra traits, have you considered instead making a new DefunctionalizedAlgebra interpreter whose sole purpose is to materialize endpoints into the concrete ADTs? You could define combinators that manipulate these data types, and then add methods on the ADTs to re-interpret using any other interpreter. With such a design, we wouldn't need to sacrifice the existing minimal design (no allocation overhead, no code bloat in Scala.js), but we could have a new interpreter package that could be opt-ed into for folks who want to be able to write more combinators like this. With such an approach, you could even hypothetically build up your endpoints entirely using the ADTs (as opposed to the methods inside various algebras) and then only interpret them at the very end. I see some type parameter issues lurking on the horizon, but I think those are the same that will have to be anyways confronted with the other proposed approach...

bmeesters commented 3 years ago

@harpocrates I understand your concerns. However I also think that embracing combinators is one of Scala's core strenghts, and currently further manipulating endpoints like first class entities and enrich them along the way is simply not possible.

I still think the current proposal is a valid choice, since the added code would likely be fairly small and provides a lot of benefit. It also does not constraint us as a closed ADT would do (like in alternatives such as Tapir). I am also open to other alternatives though, like the one you suggested. But I am not sure how that would work exactly. I understand the idea of having a separate interpreter with the added combinators. However how would that work if we want to lean on the current interpreters for client/server/open-api? Somehow we need to go back from concrete data structure to the abstract methods that can then be interpreted again differently for different purposes. I suppose you can make an eval method on the new ADT and provide some interpreter as argument? Or did you have something else in mind? Still, we also somehow need to change the algebras for this to work, since we currently lose information about the Url, Body and Headers since it is not part of the current Request type.

julienrf commented 3 years ago

@agboom Thank you for your investigation!

  • I was not able to implement xmapPartial for Request[A], because the request now carries UrlP, EntityP etc., but there is no mapping or tupler in scope for these constituents, only for A itself. It might be solvable by including the tuplers, but that would affect the signature for PartialInvariantFunctor I think. I may be getting ahead of things, but with the availability of url, entity etc. in the Request type, could it be possible that Request.xmapPartial becomes obsolete?

Would the following work?

  implicit def requestPartialInvariantFunctor: PartialInvariantFunctor[Request] =
    new PartialInvariantFunctor[Request] {
      def xmapPartial[A, B](fa: Request[A], f: A => Validated[B], g: B => A): Request[B] =
        new Request[B] {
          type UrlP = fa.UrlP
          type EntityP = fa.EntityP
          type HeadersP = fa.HeadersP
          override def method: Method = fa.method
          override def url: Url[UrlP] = fa.url
          override def entity: RequestEntity[EntityP] = fa.entity
          override def headers: RequestHeaders[HeadersP] = fa.headers
          override def documentation: Documentation = fa.documentation
          val directive: Directive1[B] = directive1InvFunctor.xmapPartial(fa.directive, f, g)
          def uri(b: B): Uri = fa.uri(g(b))
        }
    }

That being said, the fact that xmapPartial returns a Request[B] (without exposing the underlying types UrlP, EntityP, and HeadersP might be an issue at use-site).

BTW, do you know what the semantic difference is between type A <: { ... } and type A = { ... } in Scala? Both are valid syntax, but I couldn't find any documentation regarding their semantics.

AFAICT, with a bound <:, you can easily refine the type member in subtraits.

julienrf commented 3 years ago

@harpocrates

Instead of rewriting the core algebra traits, have you considered instead making a new DefunctionalizedAlgebra interpreter whose sole purpose is to materialize endpoints into the concrete ADTs? You could define combinators that manipulate these data types, and then add methods on the ADTs to re-interpret using any other interpreter. With such a design, we wouldn't need to sacrifice the existing minimal design (no allocation overhead, no code bloat in Scala.js), but we could have a new interpreter package that could be opt-ed into for folks who want to be able to write more combinators like this. With such an approach, you could even hypothetically build up your endpoints entirely using the ADTs (as opposed to the methods inside various algebras) and then only interpret them at the very end

This is also proposed in #68, and this is what Tapir does. I guess we could have an interpreter that would reify endpoint descriptions into an ADT, but one issue would be that it would be hard to make that ADT extensible/modular, like we have in the algebra.

bmeesters commented 3 years ago

Would the following work?

  implicit def requestPartialInvariantFunctor: PartialInvariantFunctor[Request] =
    new PartialInvariantFunctor[Request] {
      def xmapPartial[A, B](fa: Request[A], f: A => Validated[B], g: B => A): Request[B] =
        new Request[B] {
          type UrlP = fa.UrlP
          type EntityP = fa.EntityP
          type HeadersP = fa.HeadersP
          override def method: Method = fa.method
          override def url: Url[UrlP] = fa.url
          override def entity: RequestEntity[EntityP] = fa.entity
          override def headers: RequestHeaders[HeadersP] = fa.headers
          override def documentation: Documentation = fa.documentation
          val directive: Directive1[B] = directive1InvFunctor.xmapPartial(fa.directive, f, g)
          def uri(b: B): Uri = fa.uri(g(b))
        }
    }

If not, we could also see if the Aux pattern is possible (using Shapeless as an inspiration). This is of course getting quite complicated, but I also think that xmapPartial might not be really necessary anymore if you can access the individual pieces.

agboom commented 3 years ago

@bmeesters and I experimented a bit more with structural types and noticed that invoking methods in a structural type leads to reflective calls (for example on this line). Assuming that reflective calls are not wanted because of performance overhead[1], we stopped pursuing that direction and applied @julienrf's suggestion of extension methods. We've pushed the results in another branch that combines a minimal structural type with extension methods. For response it looks like:

  type Response[A] <: {
    type EntityP
    type HeadersP
  }

In any case, adding extractors to the algebra and interpreters is pretty easy. The difficult part is the modifiers, such as xmapPartial and choiceResponse. Modifying A in Response[A] must result in modifying either EntityP or HeadersP as well (since A is actually the tupled type (EntityP, HeadersP)). An implementation of choiceResponse is found on this line. As with the xmapPartial implementation above, the code compiles, but it's not semantically correct, because the constituents of response A are returned and nothing of response B.

We'll keep trying things, but any ideas on how to solve this particular problem are welcome!

[1]: I also briefly looked at structural types in Scala 3 to see if performance would be improved, but I don't think that's the case. Since extension methods are fine for extractors I don't think it will be a problem. http://dotty.epfl.ch/docs/reference/changed-features/structural-types.html

mleclercq commented 3 years ago

I've experimented with a different approach that doesn't try to "resurrect the types inside the request or response" but instead provides some operations to modify them "after the fact". See https://github.com/endpoints4s/endpoints4s/compare/master...mleclercq:middleware

The idea is that a Middleware can be applied when the endpoint is created:

def endpointWithMiddleware[A, B, FA[_], FB[_]](
    request: Request[A],
    response: Response[B],
    middleware: Middleware[FA, FB],
    docs: EndpointDocs = EndpointDocs()
): Endpoint[FA[A], FB[B]] = {
  val (requestFA, responseFB, docsF) = middleware(request, response, docs)
  endpoint(requestFA, responseFB, docsF)
}

A middleware is defined as a polymorphic functions where the generic types FA and FB captures how the middleware modifies the types of the request and response

trait Middleware[FA[_], FB[_]] {
  def apply[A, B](
      request: Request[A],
      response: Response[B],
      docs: EndpointDocs
  ): (Request[FA[A]], Response[FB[B]], EndpointDocs)
}

Middlewares can be composed together, we simply apply them in order. (Note with this approach we end up pretty quickly with complicated type lambdas so I added the kind-projector scalac plugin)

To be able to do something useful in a middleware apply method, I added a couple of operations to the Requests and Responses algebra:

trait RequestMiddlewares extends Requests {

  def addRequestHeaders[A, H](
      request: Request[A],
      headers: RequestHeaders[H]
  )(implicit tupler: Tupler[A, H]): Request[tupler.Out]

  def addRequestQueryString[A, Q, Out](
      request: Request[A],
      qs: QueryString[Q]
  )(implicit tupler: Tupler[A, Q]): Request[tupler.Out]
}

trait ResponseMiddlewares extends Responses { this: Errors =>

  def addResponseHeaders[A, H](
      response: Response[A],
      headers: ResponseHeaders[H]
  )(implicit tupler: Tupler[A, H]): Response[tupler.Out]
}

These operations allow to add headers or query strings parameters to a Request and add headers to a Response. They are fairly easy to implement in the interpreters (I'v tried with akka, http4s, play and xlr), but I had to slightly change the Request types for the client interpreters for akka, play and xlr. This is because the current concrete type for Request in these interpreters represents a "request that is already in flight" so as is, they do not allow to change the Request content before it is sent.

I've tried that approach with an AuthenticationMiddlewares (endpoints4s/algebra/BasicAuthentication.scala).

The middleware takes care of adding the Authorization header using existing algebra operations so there is no need to have dedicated interpreters for it for each HTTP framework.

This approach is probably less powerful than the approaches presented above. I think it covers several use cases like authentication, distributed tracing, API keys, or any operations that do not fundamentally change the types A and B of an endpoint, but only "add stuff around them".

edit Fixed broken links

bmeesters commented 3 years ago

@mleclercq I think it is a great idea to look at alternatives! We have tried several variations on the 'keep the types approach' and from what I have seen it is only possible if you keep a lot of type parameters, since as type members at some point you loose the information again unless you use the Aux pattern. Though I still believe that might be possible, maybe other approaches like yours can achieve the same.

Though I have to admit, I cannot say I fully understand yet what is going on. Just to make sure we are on the same page, here is some example detailing how (ideally) both approaches should look.

Preferably, I would like to do something like this using the 'keep the types' approach:

val myEndpoint: Endpoint[SomeInput, SomeOutput]  = ???

val upgradedEndpoint: Endpoint[WithBearer[SomeInput], Either[PossibleErrors, SomeOutput]] =
  myEndpoint
     .withBearerAuth
     .withDefaultErrorHandling

However this seems difficult since we once we define myEndpoint we lose how the request and response are built up. And as a result to get it all back you need a lot of types (e.g. Endpoint.Aux[H1, Url, Body, H2, Entity, StatusCode, A, B]). I haven't found a way to get rid of those types yet. Though if Endpoint[A, B] <: Endpoint.Aux[H1, Url, Body, H2, Entity, StatusCode, A, B] then you can choose to hide it whenever you do not need it. That said I am not that content yet.

If I understand your approach correctly it should be able to work something like this right?:

type WithErrors[A] = Either[PossibleErrors, A]
val bearerAuthentication: Middleware[WithBearerAuth, Id]  = ??? // I made it an Id instead of an Option just to match above
val errorHandling: Middleware[Id, WithErrors]  = ???

val myRequest: Request[SomeInput]
val myResponse: Response[SomeOutput]
val endpoint = endpointWithMiddleware(myRequest, myResponse, bearerAuthentication.andThen(errorHandling))

Is that correct? The upside is that the current types work as is, making it simpler for those that do not need Middlewares. On the other hand, you still cannot transform an existing endpoint right? I do think it would be workable though for our use cases. Interested what @julienrf and @agboom think.

mleclercq commented 3 years ago

Is that correct? The upside is that the current types work as is, making it simpler for those that do not need Middlewares. On the other hand, you still cannot transform an existing endpoint right?

Indeed, the middlewares has to be specified at the time the endpoint is created like in:

val myRequest: Request[SomeInput]
val myResponse: Response[SomeOutput]
val endpoint = endpointWithMiddleware(myRequest, myResponse, bearerAuthentication.andThen(errorHandling))

Unless we add an operation like the following in the algebra:

def applyMiddleware[A, B, FA[_], FB[_]](
  endpoint: Endpoint[A, B],
  middleware: Middleware[FA, FB]
): Endpoint[FA[A], FB[B]]

But that operation would require more changes in the interpreters because in many cases the concrete type for Endpoint do not allow to recover and transform the Request and Response in it (for instance in type Endpoint[A, B] = A => Future[B]).

With some "curyfication" we can make a Middleware looks like the endpoint method:

  class WithMiddlewarePartiallyApplied[FA[_], FB[_]](middleware: Middleware[FA, FB]) {

    def apply[A, B](  // Same prototype as endpoint()
        request: Request[A],
        response: Response[B],
        docs: EndpointDocs = EndpointDocs()
    ): Endpoint[FA[A], FB[B]] =
      endpointWithMiddleware(request, response, middleware, docs)

    def alsoUse[GA[_], GB[_]](other: Middleware[GA, GB]): WithMiddlewarePartiallyApplied[
      λ[X => GA[FA[X]]],
      λ[X => GB[FB[X]]]
    ] =
      new WithMiddlewarePartiallyApplied(middleware.andThen(other))
  }

  def useMiddleware[FA[_], FB[_]](
      middleware: Middleware[FA, FB]
  ): WithMiddlewarePartiallyApplied[FA, FB] =
    new WithMiddlewarePartiallyApplied(middleware)

Which can then be used as follow:

val authEndpoint = useMiddleware(bearerAuthentication.andThen(errorHandling))

val myRequest: Request[SomeInput]
val myResponse: Response[SomeOutput]
val endpoint = authEndpoint(myRequest, myResponse)

// Middlewares can be composed at the endpoint creation point
val otherEndpoint = authEndpoint.alsoUse(anotherMiddleware)(???, ???)

So this approach does not allow to add middlewares to endpoints that are defined in a third-party library. But I'm not sure I see concrete use cases for such "after the fact" change of the endpoints definition.

bmeesters commented 3 years ago

I see, thanks for the extra explanation.

So this approach does not allow to add middlewares to endpoints that are defined in a third-party library. But I'm not sure I see concrete use cases for such "after the fact" change of the endpoints definition.

I agree, the most important thing is that we can deal with concerns separately (e.g. auth and error handling) and easily create an endpoint that can take all these concerns together. In our codebase we can now easily create an endpoint with auth, or an endpoint with error handling, but have to hardcode a new endpoint creation method to provide both.

For me this solution would solve the problems we have. However I find the code a bit complex (might also be because of a lack of experience with this kind of code). The plus side though seems that this complexity does not leak to the end user of the library.

I am also still willing to improve the PoC I have with the other approach (being able to deconstruct a Request/Response from a created endpoint and use type members/variables to make it work). If that is still desirable.

mleclercq commented 3 years ago

TBH, I'm not sure I understand why we would need to keep track of the "inner" types inside a Endpoint like in Endpoint.Aux[H1, Url, Body, H2, Entity, StatusCode, A, B]. The problem is that these type parameters are not independent from each others.

In a sens, the A and B are derived from the other one. For instance, we cannot change H1 without changing A (I assume that H1 is the type of the request headers).

In you example above:

val myEndpoint: Endpoint[SomeInput, SomeOutput]  = ???

val upgradedEndpoint: Endpoint[WithBearer[SomeInput], Either[PossibleErrors, SomeOutput]] =
  myEndpoint
     .withBearerAuth
     .withDefaultErrorHandling

I do not see why we need to keep track of the type of the request headers since it does not appear in the returned Endpoint type.

The type of the withBearerAuth method just need to be something like:

def withBearerAuth[A, B](endpoint: Endpoint[A, B]): Endpoint[WithBearer[A]]
bmeesters commented 3 years ago

TBH, I'm not sure I understand why we would need to keep track of the "inner" types inside a Endpoint like in Endpoint.Aux[H1, Url, Body, H2, Entity, StatusCode, A, B]. The problem is that these type parameters are not independent from each others

Yes that's true and also why the tuplers are necessary in the current codebase. To 'prove' the relationship. Though that is not something new in the PoC.

I do not see why we need to keep track of the type of the request headers since it does not appear in the returned Endpoint type.

In a sense you are right. Though somehow you need to provide that information to the interpreters. Since the client, OpenAPI docs, and server all need to know somehow that the bearer is in the header and not in the body or the url. Currently we use a dedicated method that has different interpretations to ensure that each interpreter does it correctly.

It seems the Middleware solution has it covered with the withHeaders. I would have to look more into the details to be sure though.

edit

def withBearerAuth[A, B](endpoint: Endpoint[A, B]): Endpoint[WithBearer[A]]

would also work of course, though you basically hide that it is in the headers and silently make that assumption in the interpreters. Though you would still need to be able to deconstruct an endpoint into its parts and reconstruct it with a new input variable somehow I suppose.

edit 2

But I suppose this could be a good compromise. It would make the implementation much simpler and already solves some of the problems. It is also less powerful, but it could be useful enough.

mleclercq commented 3 years ago

Though you would still need to be able to deconstruct an endpoint into its parts and reconstruct it with a new input variable somehow I suppose.

Yes to be able to apply middleware on endpoints, you need to be able to implement an operation like:

def applyMiddleware[A, B, FA[_], FB[_]](
  endpoint: Endpoint[A, B],
  middleware: Middleware[FA, FB]
): Endpoint[FA[A], FB[B]]

For that, we need to change the concrete type of Endpoint in the interpreters, especially the client ones. For instance in akka-http-client we currently have:

type Endpoint[A, B] = A => Future[B]

We could change that to:

case class Endpoint[A, B](request: Request[A], response: Response[B], docs: EndpointDoc) extends A => Future[B] {
  def apply(a: A): Future[B] = ???
}

// Now we can apply a middleware on an Endpoint
def applyMiddleware[A, B, FA[_], FB[_]](
  endpoint: Endpoint[A, B],
  middleware: Middleware[FA, FB]
): Endpoint[FA[A], FB[B]] = {
  val (requestFA, responseFB, docsF) = middleware(endpoint.request, endpoint.response, endpoint.docs)
  Endpoint(requestFA, responseFB, docsF)
}

So if we are OK with these larger changes in the interpreters, that is something that is totally doable.

bmeesters commented 3 years ago

I think we would need to make these changes to the interpreters anyway. Also with the other approach.

That said, your remark brought to my attention that we can also greatly simplify that approach by simply discarding the 'inner' types (Headers, Url, Body, Entity, etc.) and just use the parameters that are present (In, Out). By using abstract methods you can still individually update headers, bodies, etc.

In that case the only needed change in the algebras is the extension methods we talked about before:

def getRequestFromEndpoint[In, Out](endpoint: Endpoint[In, Out]): Request[In]
def getResponseEndpoint[In, Out](endpoint: Endpoint[In, Out]): Response[In]

implicit class EndpointOps[In, Out](endpoint: Endpoint[In, Out]) {
  def request: Request[In] = getRequestFromEndpoint(endpoint)
  def response: Response[Out] = getResponseEndpoint(endpoint)
}

The interpreters would need to change the same as with the Middleware approach. This does not provide the same power as the original approach but it is significantly simpler.

The question then remains, do we want this, or the middleware solution, or both? I think the original approach is in hindsight not worth the complexity IMO.

mleclercq commented 3 years ago

I tried this approach and it simplifies things a lot (no need for all the FunctionK complexity). See https://github.com/endpoints4s/endpoints4s/compare/master...mleclercq:middleware-take-two

Instead of adding "getter", I added the following operations:

def mapEndpointRequest[A, B, C](endpoint: Endpoint[A, B], f: Request[A] => Request[C]): Endpoint[C, B]
def mapEndpointResponse[A, B, C](endpoint: Endpoint[A, B], f: Response[B] => Response[C]): Endpoint[A, C]
def mapEndpointDocs[A, B](endpoint: Endpoint[A, B], f: EndpointDocs => EndpointDocs): Endpoint[A, B]

With the "getters" we would need to also have a getter for EndpointDocs to allow to transform an endpoint while preserving its docs. Which imply that the Endpoint concrete types in the interpreters would need to keep the EndpointDocs only for that purpose.

With the "map" operations, the interpreters do not need to keep the EndpointDocs (except the openapi one, of course) and the mapEndpointDocs operation just returns the given endpoint unchanged.

The AuthenticationMiddleware example now looks as follow:

trait AuthenticationMiddlewares extends Middlewares {
  import BasicAuthentication._

  lazy val basicAuthHeader: RequestHeaders[Option[Credentials]] = ???
  lazy val bearerAuthHeader: RequestHeaders[Bearer] = ???
  def unauthorizedResponse(realm: Option[String]) = ???

  implicit class AuthenticationEndpointOps[A, B](endpoint: Endpoint[A, B]) {

    def withBasicAuth(realm: Option[String]): Endpoint[(A, Option[Credentials]), Option[B]] =
      endpoint
        .mapRequest(_.withHeaders(basicAuthHeader))
        .mapResponse(_.orElse(unauthorizedResponse(realm)).xmap(_.left.toOption)(_.toLeft(())))

    def withBearerAuth: Endpoint[(A, Bearer), Option[B]] =
      endpoint
        .mapRequest(_.withHeaders(bearerAuthHeader))
        .mapResponse(_.orElse(unauthorizedResponse(None)).xmap(_.left.toOption)(_.toLeft(())))
  }
}
julienrf commented 3 years ago

First, thank you all for the continuous investigation of this complex problem!

The question then remains, do we want this, or the middleware solution, or both? I think the original approach is in hindsight not worth the complexity IMO.

I’m a bit lost, could you put a link of what you mean exactly by “this”? So far I like the “middleware” approach (it reminds me Play framework’s ActionFunction), but I need to dig a bit deeper in both approaches before having a strong opinion.

Instead of adding "getter", I added the following operations:

def mapEndpointRequest[A, B, C](endpoint: Endpoint[A, B], f: Request[A] => Request[C]): Endpoint[C, B]
def mapEndpointResponse[A, B, C](endpoint: Endpoint[A, B], f: Response[B] => Response[C]): Endpoint[A, C]
def mapEndpointDocs[A, B](endpoint: Endpoint[A, B], f: EndpointDocs => EndpointDocs): Endpoint[A, B]

I didn’t have a look at the code, but I’m surprised that you could implement both client and server interpreters? So far we always have to provide bidirectional mappings when we transform requests or responses (with xmap), but here you have only one function direction.

mleclercq commented 3 years ago

I didn’t have a look at the code, but I’m surprised that you could implement both client and server interpreters? So far we always have to provide bidirectional mappings when we transform requests or responses (with xmap), but here you have only one function direction.

You do not need to provide "reverse function" for mapping the constituants of the endpoint, but you may use xmap inside the given function.

For instance:

val myEndpoint: Endpoint[Int, String] = ???
case class WithMyHeader[A](s: String, value: A)

// Add a header to the request and "xmap" the pair `(A, String)` to a custom type
val myEndpoint2: Endpoint[WithMyHeader[Int], String] =
  mapEndpointRequest[Int, String, WithMyHeader[Int]](
    myEndpoint,
    // Note `withHeader` is a new operation which allows to add a header to an already constructed request
    _.withHeaders(requestHeader("My header")).xmap { case (a, header) =>
      WithMyHeader(header, a)
    } { withMyHeader =>
      withMyHeader.value -> withMyHeader.s
    }
  )

// Add another possible response. No need to `xmap` (or more precisely `orElse` takes care of it)
val myEndpoint3 = mapEndpointResponse[WithMyHeader[Int], String, Either[String, Unit]](
  myEndpoint2,
  _.orElse(response(Conflict, emptyResponse))
)
mleclercq commented 3 years ago

There is one thing that bothers me with the changes to the Endpoint concrete types in the client interpreters...

For instance in akka-http-client I changed the Endpoint type from

type Endpoint[A, B] = A => Future[B]

To

case class Endpoint[A, B](request: Request[A], response: Response[B]) extends (A => Future[B]) {
  def apply(a: A): Future[B] =
    // The following code was initially in the `endpoint()` method implementation
    settings
      .requestExecutor(request(a))
      .flatMap { httpResponse =>
        decodeResponse(response, httpResponse) match {
          case Some(entityB) =>
            entityB(httpResponse.entity).flatMap(futureFromEither)
          case None =>
            httpResponse.entity
              .discardBytes() // See https://github.com/akka/akka-http/issues/1495
            Future.failed(
              new Throwable(
                s"Unexpected response status: ${httpResponse.status.intValue()}"
              )
            )
        }
      }
}

So while an Endpoint[A, B] is still a A => Future[B], it makes it hard to implement an operation like the following that would "wraps" the A => Future[B] with some retry mechanism:

def retryWithBackoff[A, [B](
  endpoint: Endpoint[A, Either[Unit, B]]
): Endpoint[A, Either[Unit, B]]

Of course, the method above could be changed to return A => Future[B] instead of Endpoint[A, B] which is fine as long as it is only defined in the interpreter and not in the algebra. But I can see the following use-case:

In https://github.com/endpoints4s/endpoints4s/issues/844, I'm trying to implement a custom algebra for Google Endpoints. One of the operation of that algebra is used to specify that an endpoint has some quotas:

type WithQuota[A]

def endpointWithQuota[A, B](
    endpoint: Endpoint[A, B],
    costs: (XGoogleManagement.Metric, Int)*
): Endpoint[A, WithQuota[B]]

So on the client-side we may receive a 429 Too Many Request response. So this can be implemented as:

type WithQuota[A] = Either[A, Unit] // Left:OK, Right: quota exceeded

def endpointWithQuota[A, B](
    endpoint: Endpoint[A, B],
    costs: (XGoogleManagement.Metric, Int)*
): Endpoint[A, WithQuota[B]] =
  mapEndpointResponse(endpoint, _.orElse(response(TooManyRequest, emptyResponse)))

It would be very nice if it was possible to also add an automated retry with backoff in here.

One option would be to make the actual A => Future[B] a parameter of the Endpoint[A, B] case class:

  case class Endpoint[A, B](request: Request[A], response: Response[B], call: A => Future[B])
      extends (A => Future[B]) {
    def apply(a: A): Future[B] = call(a)
  }

But I'm not sure why, it doesn't feel right either...

julienrf commented 3 years ago

@mleclercq Don’t we just need to also tune the carrier type for Response[B]?

bmeesters commented 3 years ago

I’m a bit lost, could you put a link of what you mean exactly by “this”

@julienrf, sorry, in such a long conversation it is better to be explicit. I meant with the original approach:

i.e.

// type parameters
type Endpoint[A, B]
type EndpointAux[A, B, H1, U, B, S, E, H2] = Endpoint[A, B]

extension[A, B, H1, U, B] (endpoint: Endpoint[A, B]) request: Request.Aux[H1, U, B]

// type members
type Endpoint[A, B] {
  type Url
  type Body
  // etc.
}

extension[A, B] (endpoint: Endpoint[A, B]) request: Request[A] { type Url = endpoint.Url; /* etc. */ }

However, from what I have seen with @agboom if we go for type parameters it gets very complex very fast. And if we go for type members the compiler loses the information unless we use the Aux pattern (getting back to problem one).

I think the second approach outlined by @mleclercq is great and actually what I meant with "this" and a good compromise in ergonomics and power. It is simpler than the original middleware approach but already pretty useful. You still have to put some information in the interpreters instead of in the algebra, but the result is IMO much simpler. I hope we can get https://github.com/endpoints4s/endpoints4s/compare/master...mleclercq:middleware-take-two in soon! If I can do anything to help please let me know. And thanks @mleclercq for getting this further! :pray:

julienrf commented 3 years ago

However, from what I have seen with @agboom if we go for type parameters it gets very complex very fast. And if we go for type members the compiler loses the information unless we use the Aux pattern (getting back to problem one).

Have you tried using path-dependent types instead of the Aux pattern?

bmeesters commented 3 years ago

Have you tried using path-dependent types instead of the Aux pattern?

Yes we did, but you would need to use the Aux pattern anyway as otherwise the compiler seems to forget that information. It might be possible, but the PoCs we tried were not successful. Ultimately it might also not be worth it, since just being able to map requests and responses already is a great addition and solves 90% of the problems we have now.

mleclercq commented 3 years ago

@mleclercq Don’t we just need to also tune the carrier type for Response[B]?

I guess you mean that if we introduce an automated retry on the client side, we would go from a response that may contains "quota exceeded" to a response that doesn't.

I think that in this particular use case, it shouldn't change because it is still possible that all retry tentatives fail which still need to be handled by the client but as some fatal errors. But at least the client code does not need to implement the retry mechanism since it would be provided by the interpreter.

julienrf commented 3 years ago

What I wanted to say is that I think it should be easy to implement the retry mechanism in the interpreter as you intend, with the class Endpoint. If this is not the case, maybe we need to change the way the interpreter fixes the type Response into something that makes it easier to implement the retry mechanism?

agboom commented 3 years ago

@mleclercq Thanks for pursuing this and sharing your approach and implementation in detail! I have to say I'm new to kind projectors, but I think I understand the gist of it. Both approaches seem promising to me, but the second one appears to the most promising of the two, because of its simplicity. I'll try to experiment a little with the second approach to see if I can understand it better and gather any feedback.

julienrf commented 2 years ago

Hey @agboom and @bmeesters, what is the current status of this issue, with the added mapRequest and mapResponse methods? Is the issue fully solved or not yet?

bmeesters commented 2 years ago

Hey @julienrf, I think mapRequest and mapResponse already improve the situation a lot! That said, as discussed earlier, there are some limitations. We would preferably also need some 'effectful' map of both versions for authorization using a micro-service.

In my opinion though this ticket can be closed and we should make more fine-grained tickets for tackling the different limitations one by one.

agboom commented 2 years ago

I agree with @bmeesters: we have seen that these added methods greatly increase the composability of endpoints. Any future enhancements can be (or already are) described in different places, so I'll close this one.