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

Refactor middleware to use declarative encoding based on new machinery in zio.http.api #1501

Closed
jdegoes opened this issue Sep 18, 2022 · 18 comments
Labels
enhancement New feature or request

Comments

@jdegoes
Copy link
Member

jdegoes commented Sep 18, 2022

Is your feature request related to a problem? Please describe.

The middleware uses an executable encoding, which makes introspection impossible. In addition to having some performance implications, this means that it is not known in advance which headers, query parameters, or route segments a piece of middleware will inspect, or if and how a piece of middleware will modify the response headers or bodies.

This means that middleware will not form a part of the documentation generated for API endpoints created using zio.http.api. So, for example, an authentication middleware will, if used on an endpoint defined using zio.http.api, not contribute any relevant details to the documentation for this endpoint when it is generated. Similarly, the automatic client generated "for free" by APIExecutor will not be able to provide middleware with whatever it requires.

Describe the solution you'd like

In order to solve this problem, as well as create many more possibilities for optimization, the design of middleware needs to be refactored to be declarative. Previously, this would not have been possible, because a declarative description of what headers, query parameters, or route segments needed by middleware did not exist.

Now, thanks to the new machinery in zio.http.api, it is possible to declaratively describe the inputs to a middleware. This is a necessary but not sufficient step toward a declarative encoding for middleware.

The following shows an early (NOT suitable) design for a declarative encoding:

sealed trait Middleware[-R, +E, +AIn, -BIn, -AOut, +BOut]
object Middleware {
  final case class Http[A](in: In[A], incoming: (A, Request) => Request, outgoing: Response => Response) extends Middleware[Any, Nothing, Request, Response, Request, Response]
  ...
}

Describe alternatives you've considered

None.

@jdegoes jdegoes added the enhancement New feature or request label Sep 18, 2022
@afsalthaj
Copy link
Contributor

I would like to take this up

@jdegoes
Copy link
Member Author

jdegoes commented Sep 20, 2022

@afsalthaj Let's work on it together, if you don't mind! Will be done twice as fast. 😆

@afsalthaj
Copy link
Contributor

afsalthaj commented Sep 21, 2022 via email

@jdegoes
Copy link
Member Author

jdegoes commented Sep 21, 2022

In our first collaboration, we discussed making the following changes:

  1. Adding a new type parameter to In to track how the In is generated. This would allow us to impose constraints such as:
    1. This In can only be composed with that In (e.g. path1 / path2).
    2. This In has already set the body and cannot be composed with another one that has also set the body.
    3. This In may NOT consume path segments (i.e. have a Route constructor inside it).
  2. Augmenting Out with two additional constructors:
    1. One constructor to append a header to the response.
    2. Another constructor to combine two Out into a single Out, similar to In.Combine.
  3. Leveraging the new type parameter in Out to improve type safety:
    1. Do NOT allow combining two Out that have both independently set the response body.
  4. Augmenting API with two new type parameters: MiddlewareIn, and MiddlewareOut, which are used to form two new fields: middlewareIn: In[MiddlewareInput], and middlewareOut: Out[MiddlewareOutput], thus producing a declarative description of how middleware processes requests and how it augments responses.
  5. Propagating these new type parameters into Service, and preserving them during service composition.

This is only half of the design problem: we did NOT have a chance to look at how middleware should be redesigned using a declarative encoding. However, we do have some general notes on a more declarative middleware, composed of four things:

  1. What does the middleware need from the request? (In[MiddlewareInput], above)
  2. How does the middleware modify the response? (Out[MiddlewareOutput], above)
  3. How does the middleware modify the request?
  4. What does the middleware need from the response?
  5. What effect do we execute upon middleware input?
  6. What effect do we execute using the response?

A very naive model of this is something like:

trait Middleware[R, E, Input, Output] {
  def preaction(in: Input): ZIO[R, E, Unit]
  def postaction(response: Response): ZIO[R, E, Output]
}

This is not complete (by far!), because it lacks the capabilities of existing middleware (and type parameters), and it is not clear how a single middleware concept would apply to both a service as well as an http. In addition, feeding all of Response to middleware seems overkill and impossible to optimize versus a more constrained approach (but yet, API does not need to know about that).

In the next session, we will attempt to resolve these issues.

cc @afsalthaj @adamgfraser

@adamgfraser
Copy link
Contributor

One idea:

object Example {

  // executable middleware preserves existing functionality when needed
  trait Middleware[-R, +E, +AIn, -BIn, -AOut, +BOut] {
    def apply[R1 <: R, E1 >: E](http: Http[R1, E1, AIn, BIn]): Http[R1, E1, AOut, BOut]
  }

  // subset of middleware that can be declaratively described
  sealed trait APIMiddleware[-R, +E, In, Out] extends Middleware[R, E, Request, Request, Response, Response]

  object APIMiddleware {

    // possible to "embed" some executable middleware into API though without docs
    final case class Executable[-R, +E](executable: Middleware[R, E, Request, Request, Response, Response])
        extends APIMiddleware[R, E, Unit, Unit] {
      def apply[R1 <: R, E1 >: E](http: Http[R1, E1, Request, Request]): Http[R1, E1, Response, Response] =
        executable(http)
    }

    // fixed hierarchy of fully declarative middleware
    sealed trait Declarative[-R, +E, In, Out] extends APIMiddleware[R, E, In, Out] {

      // can "interpret" any declarative descripton to update an appropriate app
      def apply[R1 <: R, E1 >: E](http: Http[R1, E1, Request, Response]): Http[R1, E1, Request, Response] =
        ???
    }

    object Declarative {

      sealed trait Input[-R, +E, In] extends Declarative[R, E, In, Unit]
      object Input {
        final case class AddHeader(header: Header) extends Input[Any, Nothing, Unit]
      }

      sealed trait Output[-R, +E, Out] extends Declarative[R, E, Unit, Out]

      sealed trait Execution[-R, +E] extends Declarative[R, E, Unit, Unit]
    }
  }
}

I'm worried about declarative middleware having an R and an E type parameter since API doesn't so it seems like we may need to get rid of that. Also need to think about how codecs would work with this because it seems like they can transform the In and Out types rather than just adding constraints to it.

@afsalthaj
Copy link
Contributor

afsalthaj commented Sep 27, 2022

This might be super close to what Adam said.
That we could add two types of Middleware, one is Executable Middleware, and the other an IntrospectableMiddleware.

Conceptually

type HttpMiddleware[R, E] = HttpApp[R, E] => HttpApp[R, E]
sealed trait  IntrospectableMiddleware[A, B], 
which is a ADT with terms representing transformation from API[A, B] to API[A, B]

Do we need any other middleware other than HttpMiddleware ?

Hopefully the answer is yes. In fact the very notion of HttpMiddleware (executable) can be simplified for developers.
Example:

Instead of

val middlewares: HttpMiddleware[Any, IOException] =
    // print debug info about request and response
    Middleware.debug ++
      // close connection if request takes more than 3 seconds
      Middleware.timeout(3 seconds) ++
      // add static header
      Middleware.addHeader("X-Environment", "Dev") ++
      // add dynamic header
      serverTime

  // Run it like any simple app
  val run = Server.serve(app @@ middlewares).provide(Server.default)

We could simply do

val app: HttpApp = ???

app
  .withTimeOut(3.seconds)
  .withDebug
  .addResponseHeader(response => Clock.now.map(t => response.addHeader("time" -> t.toString))

@afsalthaj
Copy link
Contributor

I had to edit the above snippet a couple of times :)

@jdegoes
Copy link
Member Author

jdegoes commented Sep 27, 2022

Here's the changes I was envisioning being made to API:

final case class API[MiddlewareIn, MiddlewareOut, HandlerIn, HandlerOut](
  middlewareIn: In[Query & Headers, MiddlewareIn],
  middlewareOut: Out[Headers, Unit],
  handlerIn: In[Route & Query & Headers & Body, HandlerIn],
  handlerOut: Out[Headers & Body, HandlerOut],
  doc: Doc
)

(In addition to the changes described above to In and Out.)

Now we have a precise description of what middleware requires.

Then we can further divide middleware into: MiddlewareSpec[In, Out], which is a bundle of middlewareIn / middlewareOut (in fact, we should store MiddlewareSpec in API as a single logical entity, probably), and Middleware, which is the "handler" or "implementation" of a MiddlewareSpec. I don't know what that would look like, but I know the goal: the middleware can only do what the spec says.

I think it's necessary to decrease the power of Middleware: it cannot alter the input or output type anymore.

I think that's fine as for performance reasons we'll be pushing people to API which already handles decoding / encoding.

@jdegoes
Copy link
Member Author

jdegoes commented Sep 27, 2022

cc @adamgfraser @afsalthaj

@jdegoes
Copy link
Member Author

jdegoes commented Sep 27, 2022

@adamgfraser I like that direction. To use my names, MiddlewareSpec cannot use E or R, and may only describe:

  1. What the middleware requires from the request
  2. What the middleware adds to the request (does this make sense? Maybe for middleware chaining???)
  3. What the middleware requires from the response (does this make sense? Maybe for middelware chaining??)
  4. What the middleware adds to the response

In the above code sketch, API can do both (1) and (4). We have to decide on (2) and (3).

I think we delete A/B in/out from Middleware (deleting its ability to do transcoding) so it becomes Middleware[R, E], or maybe: Middleware[R, E, In, Out] (???).

Perhaps, to convert a API to a Service, you need to specify TWO things:

  1. The handler function, which handles the input to produce the output.
  2. The middleware, which handles the middleware input to produce the middleware output.

If you did not define a middleware spec, then maybe you don't need to provide middleware.

Or maybe it should work a bit different: you can implement a MiddlewareSpec to produce a Middleware, which can independently be applied to the Http you get back from Service#toHttp.

@jdegoes
Copy link
Member Author

jdegoes commented Sep 27, 2022

Actually, In is already sufficient for Middleware by itself because it's invertible. So we can simplify to:

final case class API[MiddlewareIn, MiddlewareOut, HandlerIn, HandlerOut](
  middlewareIn: In[Query & Headers, MiddlewareIn],
  middlewareOut: In[Headers, MiddlewareOut],
  handlerIn: In[Route & Query & Headers & Body, HandlerIn],
  handlerOut: Out[HandlerOut]
  doc: Doc
)

In fact maybe we don't need Out if we have constrained In:

final case class API[MiddlewareIn, MiddlewareOut, HandlerIn, HandlerOut](
  middlewareIn: In[Query & Headers, MiddlewareIn],
  middlewareOut: In[Headers, MiddlewareOut],
  handlerIn: In[Route & Query & Headers & Body, HandlerIn],
  handlerOut: In[Body, HandlerOut]
  doc: Doc
)

Now a handler for middleware must accept MiddlewareIn (which we can construct from query and headers) and produce MiddlewareOut, which we can translate to appropriate headers.

Factoring out MiddlewareSpec:

final case class MiddlewareSpec[MiddlewareIn, MiddlewareOut] {
  middlewareIn: In[Query & Headers, MiddlewareIn],
  middlewareOut: In[Headers, MiddlewareOut]
)

final case class API[MiddlewareIn, MiddlewareOut, HandlerIn, HandlerOut](
  middlewareSpec: MiddlewareSpec[MiddlewareIn, MiddlewareOut],
  handlerIn: In[Route & Query & Headers & Body, HandlerIn],
  handlerOut: In[Body, HandlerOut]
  doc: Doc
)

@afsalthaj
Copy link
Contributor

Working with these models already

@guersam
Copy link
Contributor

guersam commented Sep 28, 2022

Question: will this usecase be described using the new encoding?

An authentication middleware

  1. Reads the auth token from the header
  2. Attempts to load a login session from the session storage with the token
  3. If the session is invalid, reject the request with a 401 response
  4. Otherwise produce the retrieved Session (or User,) so that the handler can use the valid session information in a type safe and ergonomic way

The existing executable encoding does 1,2,3 successfully, but not 4. There are a workaround using (Fiber)Ref with R type, but it doesn't guarantee the user is authenticated in type level.

@jdegoes
Copy link
Member Author

jdegoes commented Sep 28, 2022

@guersam To accomodate this use case cleanly (i.e. without fiber ref), a middleware would have to be able to produce some value that can be consumed by the handler.

I am not sure what that would look like, but we can think about it.

@afsalthaj
Copy link
Contributor

A very early stage trying to validate just the Out realm
#1598

@afsalthaj
Copy link
Contributor

afsalthaj commented Oct 2, 2022

Here is an example in the draft PR on how this looks like in terms of usage

https://github.com/zio/zio-http/blob/fb9fe69b5ff61e99798b0de574607b67bb5acd4e/zio-http-example/src/main/scala/example/APIExamples.scala

  val getUser =
    API.get(literal("users") / int).out[Int] @@ MiddlewareSpec.addHeader("key", "value")

It would be even ideal to have @@ at service level that inspects every API and add the middleware.

i.e

  val addHeader = 
    MiddlewareSpec.addHeader("key", "value")
    
  val getUser =
    API.get(literal("users") / int).out[Int]

  val getUsersService =
    getUser.handle[Any, Nothing] { case (id: Int) =>
      ZIO.succeedNow(1)
    }

  val getUserPosts =
    API
      .get(literal("users") / int / literal("posts") / query("name") / int)

  val getUserPostsService =
    getUserPosts.handle[Any, Nothing] { case (id1, query, id2) => ??? }

  val services = (getUsersService ++ getUserPostsService) @@ addHeader // which delegates to `@@` in `API` case class

However this implies the documentation need to rely on the API that is accessible from the service, and not the raw APIs. I hope that's the case anyway

cc @jdegoes @adamgfraser

@987Nabil
Copy link
Contributor

987Nabil commented Oct 2, 2022

@afsalthaj I think it makes sense, to offer docs based on the combination of endpoints and middleware. That said, I think that it should be possible to combine docs on a higher level. But I don't see this conflicting.

@vigoo
Copy link
Contributor

vigoo commented Jan 21, 2023

Can we close this now that we have the Middleware/ MiddlewareSpec in zio.http.api?

@jdegoes jdegoes closed this as completed Apr 18, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement New feature or request
Projects
None yet
Development

No branches or pull requests

6 participants