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

AuthMiddleware / Apply: simplify code #2551

Merged
merged 1 commit into from May 20, 2019

Conversation

@diesalbla
Copy link
Contributor

@diesalbla diesalbla commented May 4, 2019

The apply method from the AuthMiddleware object bulds a service out of three elements: a function to get the auth info from a request, the authenticated service to run if it is authenticated, and the fallback service to run if authentication fails.

We simplify the code of this apply method by doing the following:

  • We avoid the methods local and mapF of Kleisli.
  • We avoid using the Choice instance for Kleisli[OptionT]. Instead we pattern-match right after the call to the authUser function.
  • We avoid calling the AuthedRequest.apply(Kleisli) method, a combinator. Instead, we build the AuthedRequest in each branch of the pattern-match.

What is left is the code of the function in a direct style.

This change is BINARY-INCOMPATIBLE, since it removes one parameter (the Choice).

@diesalbla diesalbla force-pushed the AuthMiddleware_apply_simplify branch from edd49dc to bc6cd60 May 4, 2019
@@ -105,17 +104,16 @@ package object server {
def apply[F[_], Err, T](
authUser: Kleisli[F, Request[F], Either[Err, T]],
Copy link
Contributor Author

@diesalbla diesalbla May 4, 2019

Choose a reason for hiding this comment

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

@rossabaker Since this may change source/binary compatibility, perhaps we can reduce the type of authUser to a function Request[F] => F[Either[Err, T]]

Loading

Copy link
Member

@rossabaker rossabaker May 15, 2019

Choose a reason for hiding this comment

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

I'm kind of surprised we didn't make it Kleisli[EitherT[F, Err, ?], Request[F], T], which I'm sure goes in the opposite direction that you prefer. I also haven't used our auth and don't have a great feel for the ergonomics.

Loading

Copy link
Contributor Author

@diesalbla diesalbla May 15, 2019

Choose a reason for hiding this comment

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

I suppose not, mostly because it is shorter and I do find it easier to read:

Request[F] => F[Either[Err, T]]

Kleisli[F, Request[F], Either[Err, T]]

Kleisli[EitherT[F, Err, ?], Request[F], T]

Anyway, that is a story for another time.

Loading

@alexknvl
Copy link

@alexknvl alexknvl commented May 4, 2019

Perhaps this change can be made binary compatible by doing val _ = C. The unnecessary parameter could later be removed.

Loading

@diesalbla diesalbla force-pushed the AuthMiddleware_apply_simplify branch from d251d01 to 198b111 May 4, 2019
@diesalbla
Copy link
Contributor Author

@diesalbla diesalbla commented May 5, 2019

@alexknvl Good idea. Done.

Loading

Copy link
Member

@rossabaker rossabaker left a comment

I don't see a big win here for 0.20.x, though eliminating the Choice constraint is nice. I'd probably just eliminate it and keep targeting master.

Loading

@@ -105,17 +104,16 @@ package object server {
def apply[F[_], Err, T](
authUser: Kleisli[F, Request[F], Either[Err, T]],
Copy link
Member

@rossabaker rossabaker May 15, 2019

Choose a reason for hiding this comment

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

I'm kind of surprised we didn't make it Kleisli[EitherT[F, Err, ?], Request[F], T], which I'm sure goes in the opposite direction that you prefer. I also haven't used our auth and don't have a great feel for the ergonomics.

Loading

@diesalbla diesalbla force-pushed the AuthMiddleware_apply_simplify branch 2 times, most recently from 21bfd19 to ca2ef8e May 15, 2019
@diesalbla
Copy link
Contributor Author

@diesalbla diesalbla commented May 15, 2019

@rossabaker I have reverted to the original incompatible change, without Alex suggestion, that removes the Choice implicit.

Loading

Copy link
Member

@rossabaker rossabaker left a comment

👍 That was a great suggestion to preserve bincompat. I just don't think the change is significant enough to have to do on two branches.

Loading

@diesalbla diesalbla force-pushed the AuthMiddleware_apply_simplify branch from ca2ef8e to 52665d7 May 15, 2019
The `apply` method from the AuthMiddleware object bulds a service out
of three elements: a function to get the auth info from a request,
the authenticated service to run if it is authenticated, and the
fallback service to run if authentication fails.

We simplify the code of this apply method by doing the following:
- We avoid the methods `local` and `mapF` of Kleisli.
- We avoid using the `Choice` instance for `Kleisli[OptionT]`.
- We avoid calling the `AuthedRequest(Kleisli)` method.

This leaves the code of the function in a direct style.
@diesalbla diesalbla force-pushed the AuthMiddleware_apply_simplify branch from 52665d7 to 9ea658b May 15, 2019
@aeons aeons merged commit 16048bc into http4s:master May 20, 2019
1 check passed
Loading
@diesalbla diesalbla deleted the AuthMiddleware_apply_simplify branch May 21, 2019
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Linked issues

Successfully merging this pull request may close these issues.

None yet

5 participants