Endpoint-Specific Middleware #614
Conversation
|
@Baccata if you are happy with the contents of this PR, then I will move forward with adding tests in the project and docs for the microsite. Want to make sure the interface is solid before I move on to that though. Let me know! |
|
Ah yeah... |
modules/http4s/src/smithy4s/http4s/EndpointSpecificMiddleware.scala
Outdated
Show resolved
Hide resolved
modules/http4s/src/smithy4s/http4s/EndpointSpecificMiddleware.scala
Outdated
Show resolved
Hide resolved
|
Alright, I addressed comments, added tests, and added a guide to the docs. Give me your feedback on what needs to improve/change/etc so we can get this merged 🚀 |
| ): HttpApp[IO] => HttpApp[IO] = { inputApp => | ||
| HttpApp[IO] { request => | ||
| val hasTag: (Hints, String) => Boolean = (hints, tagName) => | ||
| hints.get[smithy.api.Tags].exists(_.value.contains(tagName)) |
There was a problem hiding this comment.
first time I'm seeing anyone use this hint for anything in Scala
There was a problem hiding this comment.
Hah yeah it was just a really easy way I could think to test that the hints were being sent through properly. I don't have any real use cases for it atm. Though I guess it could come in handy for some arbitrary middleware thing 🤷
|
amazing stuff :) |
| )(implicit effect: EffectCompat[F]) extends SmithyHttp4sClientEndpoint[F, Op, I, E, O, SI, SO] { | ||
| // format: on | ||
|
|
||
| private val transformedClient: Client[F] = | ||
| Client.fromHttpApp[F](middleware(client.toHttpApp)) |
There was a problem hiding this comment.
Right, so unfortunately, I don't think we should rely on fromHttpApp in non-test code, because it's a non-trivial transformation and I'm somewhat scared that we'd be giving a footgun to the users, as it's easy to mess up the request lifecycle in client-side middleware.
Essentially the main problem is that the HttpApp abstraction should work against Resource[F, *] instead of F. I believe there's an issue somewhere in http4s, but the impact will be quite heavy on the ecosystem so I think they're waiting for further stabilisation before pulling the trigger on that.
So, we need to require the middleware to be provided as a Client[F] => Client[F], which unfortunately implies creating another construct to echo the EndpointSpecificMiddleware, which may mean that we'd have to rename that middleware to `EndpointSpecificServerMiddleware which is a bit of a mouthful.
How about ServerEndpointMiddleware and ClientEndpointMiddleware ?
There was a problem hiding this comment.
@Baccata I think fromHttpApp is not even as bad, the real evil is toHttpApp. But I agree, the main usecase is tests and we should probably stick to that
Adds the ability to provide a middleware that can provide HttpApp[F] => HttpApp[F] conversions based on the data contained in the service and endpoint (such as Hints). This will allow more idiomatic implementations of things such as authentication/authorization and other mechanisms that rely on inspecting traits on a per-endpoint basis.
More docs to come as part of the PR.