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

Provide HandlerAspect context via env instead of function param (#2488) #2778

Merged
merged 1 commit into from
Apr 21, 2024

Conversation

987Nabil
Copy link
Contributor

This PR is a different way of solving #2488 that seems to me to be superior to #2553

With this PR, the context provided by a HandlerAspec is no longer injected via the function param if a handler/Handler.fromFunction but instead via the environment. So users would just require context via ZIO.serviceX.
The implementation is such, that h: Handler[Env with Ctx, _, _, _] @@ aspect: HandlerAspect[_, Ctx] would eliminate the Ctx from the env of the handler. The aspect may add new Env requirements as well. Only for partial Env providing in Scala 2, an explicit param needs to be handed over to @@. For all other cases inference works fine.

This change also enables auth context to be handed over to handlers of endpoint API implementations, since it does not require the context to be known while creating the endpoint. There is an example in the added tests.

This seems to be the most consistent way to provide context via an aspect that works for all use cases

  • single handler
  • Routes
  • Endpoint API

fixes #2488
/claim #2488

@987Nabil 987Nabil requested review from jdegoes and vigoo as code owners April 17, 2024 19:19
@987Nabil 987Nabil force-pushed the provide-context-via-env branch 3 times, most recently from 71165b0 to aa853e8 Compare April 17, 2024 19:47
Method.GET / "profile" / "me" -> handler { (name: String, _: Request) =>
ZIO.succeed(Response.text(s"Welcome $name!"))
Method.GET / "profile" / "me" -> handler { (_: Request) =>
ZIO.serviceWith[String](name => Response.text(s"Welcome $name!"))
} @@ bearerAuthWithContext,
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

By the changes in this PR, we can move bearerAuthWithContext to apply it to the whole routes on line 72 💯

@987Nabil 987Nabil force-pushed the provide-context-via-env branch 3 times, most recently from aa152ee to c5f3858 Compare April 19, 2024 09:12
@guersam
Copy link
Contributor

guersam commented Apr 19, 2024

It's just what I have been wanted, thanks for the work, @987Nabil :D

Providing context via environment has a great DX benefit that attaching a middleware in the future doesn't break existing handlers' interface.

Also, as one of the primary use case for this is authentication, there are many endpoints that don't care of the exact user identity but as long as the user is authenticated. This implementation enables mixing two types of endpoints seamlessly .

@987Nabil 987Nabil force-pushed the provide-context-via-env branch from c5f3858 to 713f321 Compare April 19, 2024 18:20
@987Nabil 987Nabil force-pushed the provide-context-via-env branch from 713f321 to 3a3c4a6 Compare April 19, 2024 19:12
Copy link
Member

@jdegoes jdegoes left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Nice work! You're going to make a lot of people happy!

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

Successfully merging this pull request may close these issues.

There's no way to use a "providing middleware" on a Routes nor an HttpApp
4 participants