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

Generalize AuthedRequest to ContextRequest #2612

Merged
merged 4 commits into from Nov 5, 2019

Conversation

@hamnis
Copy link
Contributor

hamnis commented May 31, 2019

A ContextRequest is defined as a request with some data.

AuthedRequest can be defined as a ContextRequest with auth data
as the context.

AuthedRequest signals that it should only be used for auth stuff, but
ContextRequest opens up possibility to reuse it to other things.

Copy link
Member

rossabaker left a comment

I don't use our auth, so I'd like to hear from others. A few gut reactions:

  • I agree that making "auth" special when it's just a context doesn't feel quite right.

  • This reminds me a bit of @ChristopherDavenport's idea of requests (and responses) being a typeclass.

  • I'm a bit uncomfortable how this leaves ContextRequest as being magically an AuthRequest through aliasing and implicits and extractors. It doesn't feel quite right. Half-baked thought that i don't necessarily endorse myself yet: should this "context" be part of the effect, taking advantage of the idea that Tuple2 is a monad?

Kleisli(request => getContext(request).map(ctx => ContextRequest(ctx, request)))


implicit class AuthInfo[F[_], A](req: ContextRequest[F, A]) {

This comment has been minimized.

Copy link
@rossabaker

rossabaker Jun 3, 2019

Member

This feels awkward to me. It's always going to be in scope for a ContextRequest, so why not just a member of ContextRequest?

}
}

object AuthedRequest {

This comment has been minimized.

Copy link
@rossabaker

rossabaker Jun 3, 2019

Member

I think I'd keep this companion in its own file.

import cats.data.{Kleisli, OptionT}
import cats.Applicative

object ContextService {

This comment has been minimized.

Copy link
@rossabaker

rossabaker Jun 3, 2019

Member

We're moving away from the name "service". We should consider how this interacts with #2567.

This comment has been minimized.

Copy link
@hamnis

hamnis Jun 25, 2019

Author Contributor

I renamed that to ContextRoutes

/**
* An HTTP middleware that adds a context.
*/
type ContextMiddleware[F[_], T] =

This comment has been minimized.

Copy link
@rossabaker

rossabaker Jun 3, 2019

Member

Does anybody actually use these middleware aliases? I've come to regard them as obfuscation more than anything. I wonder what others think.

This comment has been minimized.

Copy link
@hamnis

hamnis Jun 25, 2019

Author Contributor

We are using them on our project. I find them quite useful.

This comment has been minimized.

Copy link
@RafalSumislawski

RafalSumislawski Jul 30, 2019

Contributor

We too use them.

@hamnis hamnis force-pushed the hamnis:context-request branch 2 times, most recently from 0915844 to 81c01f8 Jun 25, 2019
@hamnis hamnis force-pushed the hamnis:context-request branch from 81c01f8 to 913fac4 Sep 17, 2019
@hamnis hamnis force-pushed the hamnis:context-request branch from 913fac4 to cc52f52 Sep 26, 2019
@hamnis

This comment has been minimized.

Copy link
Contributor Author

hamnis commented Oct 11, 2019

@rossabaker @ChristopherDavenport is this something that is interesting before 0.21 final? or delay for the next bit?

@hamnis hamnis force-pushed the hamnis:context-request branch from cc52f52 to 4f3dee2 Oct 30, 2019
@hamnis

This comment has been minimized.

Copy link
Contributor Author

hamnis commented Oct 30, 2019

Added a deprecation warning for use of authInfo and used context everywhere in the codebase.
Is this something we want to do for 0.21 ? or postpone until later?

@hamnis hamnis force-pushed the hamnis:context-request branch from 4f3dee2 to 96758df Oct 30, 2019
@hamnis hamnis force-pushed the hamnis:context-request branch from 96758df to 620c6f1 Oct 31, 2019
hamnis added 2 commits May 31, 2019
A ContextRequest is defined as a request with some data.

AuthedRequest can be defined as a ContextRequest with auth data
as the context.

AuthedRequest signals that it should only be used for auth stuff, but
ContextRequest opens up possibility to reuse it to other things.
@hamnis hamnis force-pushed the hamnis:context-request branch from 620c6f1 to dab8008 Nov 1, 2019
Copy link
Member

rossabaker left a comment

This looks good to me, and should be shipped with 0.21 if we're going to do it. It just needs a scalafmt.

ContextRequest(context, req.mapK(fk))

@deprecated("Use context instead", "0.21.0")
def authInfo: A = context

This comment has been minimized.

Copy link
@rossabaker

rossabaker Nov 2, 2019

Member

Seeing how many places this changed just in this repo, it would be a great candidate for a scalafix, but that's not essential to this PR.

@rossabaker rossabaker added this to the 0.21.0 milestone Nov 2, 2019
Copy link
Member

rossabaker left a comment

Thanks!

@rossabaker rossabaker merged commit ae6892c into http4s:master Nov 5, 2019
2 checks passed
2 checks passed
Summary no rules match, no planned actions
Details
continuous-integration/travis-ci/pr The Travis CI build passed
Details
@hamnis hamnis deleted the hamnis:context-request branch Dec 2, 2019
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
5 participants
You can’t perform that action at this time.