[IMPROVEMENT] AkkaHttpServerInterpreter should be extendable

[quincyjo]

AkkaHttpServerInterpreter is implemented as a trait, and it seems very logical that a consumer might want to implement a custom instance of this in order to meet their needs. For example, in our use case, we want to wrap all EP routes with a directive for logging and a custom directive for handling open tracing support. Instead of having to wrap this in a custom class, it would seem that the proper way to do this would be to provide a custom AkkaHttpServerInterpreter that would generate the Route appropriately. While the implementation of AkkaHttpServerInterpreter is pretty light, it makes use of several private classes that I think should be usable: AkkaServerRequest, AkkaRequestBody, and AkkaToResponseBody.

I would maybe like to make see something like this:

val myServerInterpreter: AkkaHttpServerInterpreter = AkkaHttpServerInterpreter.instance {
  case (interpreter, severRequest, serverEndpoints) => // Existing implementation  
    onSuccess(interpreter(serverRequest, ses)) {
      case None           => reject
      case Some(response) => serverResponseToAkka(response)
    }
}

or even

val myServerInterpreter: AkkaHttpServerInterpreter = AkkaHttpServerInterpreter.instance {
  responseMonad: F[Option[ServerResponse[B]]] =>
    onSuccess(responseMonad) {
      case None           => reject
      case Some(response) => serverResponseToAkka(response)
    }
}

Which would then be used as expected:

val route: Route = myServerInterpreter.toRoute(myServerEndpoints)

This would allow standard route directives to be implemented and interpreted as expected instead of having to wrap Tapir's models in redundant custom ones. Being able to provide custom converters and interpreter construction logic might also be useful.

[adamw]

I'm not sure I understand the problem. Can't you do the following:

trait MyInterpreter extends AkkaHttpServerInterpreter {
  override def toRoute(ses: List[ServerEndpoint[AkkaStreams with WebSockets, Future]]): Route =
    myDirective1 { myDirective2 { super.toRoute(ses) } }
}

Or maybe even just defining a custom List[ServerEndpoint] => Route function which would call the interpreter internally? That's what the interpreter really is :)

[quincyjo]

I have noticed some weird things with route result future mapping when placing directives around the toRoute call as you did there (which is equivalent to the initial approach). But wrapping the List[ServerEndpoint] => Route assumes directives are agnostic of the resource. For instance, in tracing each EP has a traced operation based on its method and URI (eg traced ("GET /status"), and it may instead be required to wrap each ServerEndpoint`.
In your example trait you could of course do:

trait MyInterpreter extends AkkaHttpServerInterpreter {
  override def toRoute(ses: List[ServerEndpoint[AkkaStreams with WebSockets, Future]]): Route =
    Directives.concat(ses.map { se => myDirective1 { myDirective2 { super.toRoute(Seq(se)) } }}: _*)
}

This is effectively what was used at first just extending AkkaHttpServerInterpreter and is a good point (I usually avoid super usage I guess so didn't think about that). What actually made me dig into this was that the open tracing directive we wrote makes use of mapRouteResultFuture to scope the MDC across the ec. It worked before switching to Tapir, but now it seems that the future returned from mapRouteResultFuture is the future from the server interpreter and can map before the future contained in the server logic of a given ServerEndpoint has been composed. I was digging into Akka Http to try and figure out maybe a better way to do that but might give more context into the issue being encountered.

[adamw]

Hmm ok I think there's several things here :)

First:

I have noticed some weird things with route result future mapping when placing directives around the toRoute

Can you maybe demonstrate what these weird things are? Maybe that's a manifestation of some other issue which would be good to fix :)

Second, you can use composition instead of inheritance quite easily I think:

class MyInterpreter(delegate: AkkaHttpServerInterpreter) {
  def toRoute(ses: List[ServerEndpoint[AkkaStreams with WebSockets, Future]]): Route =
    Directives.concat(ses.map { se => myDirective1 { myDirective2 { delegate.toRoute(Seq(se)) } }}: _*)
}

Finally, the problems with the MDC may indicate that we are using the wrong EC somewhere. Again, an example reproducing the problem would be great to see what's really going on :)