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

Intercepting requests, more flexible server interpreter callbacks #1065

Closed
adamw opened this issue Mar 6, 2021 · 4 comments
Closed

Intercepting requests, more flexible server interpreter callbacks #1065

adamw opened this issue Mar 6, 2021 · 4 comments

Comments

@adamw
Copy link
Member

adamw commented Mar 6, 2021

Currently, there are two types of callbacks which are invoked when interpreting an endpoint (or a list of endpoints) as a server:

  • logging - when there's a failure when decoding an endpoint's inputs; when an endpoint successfully handles a request; and when there's an exception when executing the server logic (LogRequestHandling)
  • handling decode failures - which allows to determine if a decode failure should result in a response (e.g. when the body can't be parsed), or if the next endpoint should be tried (e.g. path mismatch) (DecodeFailureHandler)

This works to some degree, but especially the decode failure handler is a source of confusion. I think we can do a better (and more general) job.

The idea isn't new - intercepting and manipulating the request before it is handled is present in each web framework/library (be it servlet filters, akka directives or http4s middleware). So far tapir relied on these interpreter-specific constructs, but even for tapir, an abstraction might be useful. And not even one, but two.

Endpoint interceptors

First, I propose adding EndpointInterceptors. These would implement the following:

trait EndpointInterceptor[F[_]] {
  def onDecodeFailure(request: ServerRequest, e: Endpoint[_, _, _, _], 
    df: DecodeFailure, next: DecodeFailure => F[Option[ServerResponse]]): F[Option[ServerResponse]]

  def onDecodeSuccess[I](request: ServerRequest, e: Endpoint[I, _, _, _], 
    i: I, next: I => F[ServerResponse]): F[ServerResponse]
}

The ServerRequest class is an abstraction on top of an underlying request representation. When a request arrives, it is decoded as specified by the given endpoint's inputs. This can fail - for whatever reason, ranging from path mismatch, to an exception when parsing the body - and then the onDecodeFailure method is called. The result is an optional response. Returning F[None] here means that the next endpoint should be tried.

If the inputs match, then the onDecodeSuccess method is called. In this case, we must return a response (or a failed effect F, if the server logic or encoding the result to a response fails).

An interceptor should perform its logic, which can involve calling the next interceptor in the stack (using next), or omitting that step and returning a response right away.

By default, we would have a stack of two endpoint interceptors, which would

  1. handle decode failures, and in some cases return responses (what today is DecodeFailureHandler)
  2. handle logging (what today is LogRequestHandling'; by the way, this should allow fixing Add server option for logging the Left part in F[Either[E, O]]? #719 and Effectful handling of decoding failures #578)

The lowest level (the "last" next) would return F[None] for onDecodeFailure, and run the server logic & encode outputs for onDecodeSuccess.

This would give us much greater flexibility, and allow implementing functionalities such as:

  • handling exceptions (in an effectful or not way)
  • adding fine-grained, endpoint-aware metrics (as we can plug into the whole lifecycle)

The default stack would of course be fully interchangeable, so both decode failure handling and log handling could be customised.

Request interceptors

Second, we could add interceptors which would be called even earlier in the process, before any endpoint<->request matching is done:

trait RequestInterceptor[F[_]] {
  def onRequest(request: ServerRequest, 
    next: ServerRequest => F[Option[ServerResponse]]): F[Option[ServerResponse]]
}

This would allow changing the server request, and potentially implementing features such as global metrics (which are not endpoint-aware, but can capture e.g. the timing more accurately - before most of tapir's logic is run), CORS, correlation ids, timeouts or compression (this would still be interpreter-specific, basing on the representation of the request's body, of course).

In comparison, endpoint interceptors have much more context (the specific endpoint, and decoding result), but they are called multiple times for a request - once for each endpoint. On the other hand, request interceptors would be called once per request.

Unifying server interpreters

To implement the above, the implementation of server interpreters will have to be unified, so that the general signature of an interpreter is similar to a function ServerRequest => F[Option[ServerResponse]]. This would then have to be converted into to a library-specific representation (akka Routes, http4s Routes etc.)

This means refactoring the server interpreters, capturing some common abstractions and extracting common code (which in general is a good thing). Also, we'll have to introduce an abstraction on top of responses (ServerResponse).

Naming & feedback

What do you think? Any suggestions on functionalities / signatures?

I'm also considering various names: possible suffixes include XyzListener, XyzFilter, XyzDecorator and possibly others. I think XyzInterceptor captures the intention, but maybe a better naming scheme is possible. The same applies to the Endpoint/Request prefixes.

Some ongoing work is available on the https://github.com/softwaremill/tapir/tree/interceptors branch.

@adamw
Copy link
Member Author

adamw commented Mar 19, 2021

PR: #1089

@ghostbuster91
Copy link
Contributor

Hi Adam,

I haven't yet find time to dig into the implementation but I doubt that's necessary ;) The whole idea is really great. Abstracting over decodingFailureHandler and logginHandler seems so logical and natural that it is hard to believe that no one thought about it before.

One small thing. Would it make sense to introduce a new type for expressing the control flow of interceptors? So instead of returning F[None] to indicate that the next endpoint should be tried we would have a specific type e.g. FlowControl.Next ? Obviously there would need to exist another subtype of it to return the value. I know that it doesn't bring much to the table as it will have (at the moment) exactly two inhabitants just as Option has. I just though that it would reveal more intentions to the developers what is going to happen rather than relaying on implicit conventions.

@adamw
Copy link
Member Author

adamw commented Mar 21, 2021

One small thing. Would it make sense to introduce a new type for expressing the control flow of interceptors? So instead of returning F[None] to indicate that the next endpoint should be tried we would have a specific type e.g. FlowControl.Next ? Obviously there would need to exist another subtype of it to return the value. I know that it doesn't bring much to the table as it will have (at the moment) exactly two inhabitants just as Option has. I just though that it would reveal more intentions to the developers what is going to happen rather than relaying on implicit conventions.

I though about that (similarly also in DecodeFaillureHandler - should it return an Option, or some custom type?), but in the end reached the conclusion that a custom one wouldn't give much more information. After all, the interceptor has to either return a response, or return "no response". So the response is optional.

FlowControl.Next or similar would make sense in something tightly bound with a flow control processor. However, we don't really know, if the server interpreter will even attempt to match the next endpoint, and if there is a "next endpoint" at all.

The interceptor's task is to return an optional response - not decide, how the server interpreter should behave.

adamw added a commit that referenced this issue Mar 22, 2021
@adamw
Copy link
Member Author

adamw commented Mar 31, 2021

Implemented in 0.18.0-M1

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

No branches or pull requests

2 participants