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鈥檒l occasionally send you account related emails.

Already on GitHub? Sign in to your account

Add HttpMethodOverrider middleware #2355

Closed

Conversation

Projects
None yet
3 participants
@nebtrx
Copy link
Contributor

nebtrx commented Jan 13, 2019

This one adds an Http Method Overrider middleware(to contribute with #2014) in order to allow using HTTP verbs such as PUT or DELETE in places where the client doesn't support it.

It takes some ideas from django-method-override and from ExpressJS method override.

Let me know what you guys think of it and if it requires some changes, specially in the test description since I'm new with the predominant testing style.

PS: Also, I just realized of the existence of the headers package and its particular way of representing/encoding Headers.
Do you think that should be necessary for Vary taking into consideration how I'm using it?

Cheers!! 馃槾

nebtrx added some commits Jan 13, 2019

@rossabaker
Copy link
Member

rossabaker left a comment

Thanks. This is a good start. Some thoughts below.

Show resolved Hide resolved ...er/src/main/scala/org/http4s/server/middleware/HttpMethodOverrider.scala Outdated
Show resolved Hide resolved ...er/src/main/scala/org/http4s/server/middleware/HttpMethodOverrider.scala Outdated
Show resolved Hide resolved ...er/src/main/scala/org/http4s/server/middleware/HttpMethodOverrider.scala Outdated
def processRequestWithMethod(
req: Request[F],
parseResult: ParseResult[Method]): F[Response[F]] = parseResult match {
case Left(_) => F.pure(Response[F](Status.MethodNotAllowed))

This comment has been minimized.

Copy link
@rossabaker

rossabaker Jan 14, 2019

Member

I think this would be a BadRequest. MethodNotAllowed means we recognize the method.

Show resolved Hide resolved ...er/src/main/scala/org/http4s/server/middleware/HttpMethodOverrider.scala Outdated
Show resolved Hide resolved ...er/src/main/scala/org/http4s/server/middleware/HttpMethodOverrider.scala Outdated
if (config.overridableMethods.contains(req.method)) Some(()) else None

def getUnsafeOverrideMethod(req: Request[F]): Option[String] =
config.overrideStrategy match {

This comment has been minimized.

Copy link
@rossabaker

rossabaker Jan 14, 2019

Member

Are there other plausible strategies, such that this might be a function on the strategy and unseal the strategy? People don't embed this in a UrlForm, do they?

This comment has been minimized.

Copy link
@nebtrx

nebtrx Jan 15, 2019

Author Contributor

Ohh, I wasn't aware of UrlForm. I have to take a look at it since I was only thinking about QueryParamDecoder. The Query Param Strategy might get in the middle. If so, We should find a way to update the request and remove the param once it has been overridden.

I'm also considering a strategy like what you say. Perhaps something that receives a Request[G] => Request[G] or Request[G] => Method. The first one seems to lose some control over the request pipeline and the second one...mmm....perhaps it exposes too much information to just override the Request method.

I'll think about this.

Show resolved Hide resolved ...er/src/main/scala/org/http4s/server/middleware/HttpMethodOverrider.scala
@nebtrx

This comment has been minimized.

Copy link
Contributor Author

nebtrx commented Jan 15, 2019

@rossabaker thanks for your feedback!
I addressed most of the suggestions, except for a couple still requiring some time to check a few things.

nebtrx added some commits Jan 15, 2019

@nebtrx

This comment has been minimized.

Copy link
Contributor Author

nebtrx commented Jan 19, 2019

Hey @rossabaker, this one is ready for the second round.

In the last commit, I added a third strategy for allowing method override in request receiving an UrlForm. I think we 're good with three strategies since that's more than the usual amount of features provided by other method override middlewares. We could probably have another one receiving aRequest[G] => Method function. About that, perhaps I'm biased but, I'm not fully convinced about the value it delivers if someone can already override the method using headers, request params or forms. Think about it and let me know if you'd like to see that strategy implemented.

Also, since I had to generalize for F[_] and G[_], that required to naturally transform G to F to be able to decode the UrlForm. Therefore, a natural transformation has to be provided at some point. So, I modeled the strategy as follows:

case class FormOverrideStrategy[G[_], F[_]](fieldName: String, naturalTransformation: G ~> F)

I got to say it, it kinda looks odd to me -- I'm not an expert whatsoever -- but, the alternative would be to provide the natural transformation when constructing the midleware. I think that'd be unfair for those just want to user Header strategy. Please take a look at let me know your thoughts, especially about the use of TypeTag for pattern matching on the parameterized case class(Thanks Type Erasure!).

PS: After refactoring a little bit the suggestion of using Alternative was clear as water 馃憤.

@rossabaker
Copy link
Member

rossabaker left a comment

Sorry about the delayed review. This is shaping up nicely. Still a couple pending questions.

Show resolved Hide resolved ...er/src/main/scala/org/http4s/server/middleware/HttpMethodOverrider.scala Outdated
Show resolved Hide resolved ...er/src/main/scala/org/http4s/server/middleware/HttpMethodOverrider.scala
case QueryOverrideStrategy(parameter) => req.params.get(parameter)
case HeaderOverrideStrategy(headerName) => F.pure(req.headers.get(headerName).map(_.value))
case QueryOverrideStrategy(parameter) => F.pure(req.params.get(parameter))
case FormOverrideStrategy(field, f) if runtimeTypeNT == typeOf[G ~> F] =>

This comment has been minimized.

Copy link
@rossabaker

rossabaker Jan 24, 2019

Member

This scares me. I'm too sleepy to think of how to do this without reflection. But I'm not sure reflection makes it much safer anyway: aren't you trading a ClassCastException for a MatchError?

This comment has been minimized.

Copy link
@nebtrx

nebtrx Feb 1, 2019

Author Contributor

Yeah, it scared me too :S and I hate reflection. I spend ~ two hours looking for the proper/elegant way of overcoming type erasure in this kind of scenarios and there seems to a general consensus about using the following approaches:

  1. Using ClassTag[_], TypeTag[_] or WeakTypeTag[_]
  2. Using Manifest[_]

Both are pretty similar. I had a slightly bigger preference for the first one and since Headerkey is already using that approach with ClassTag, I went for it.

Let me know if have a better approach. I keep looking for better ways to handle this

This comment has been minimized.

Copy link
@nebtrx

nebtrx Feb 1, 2019

Author Contributor

BTW, yes, I'd be trading ClassCastException for MatchError. It kind of feels more logical to me since we trying to match strategies. WDYT?

This comment has been minimized.

Copy link
@rossabaker

rossabaker Feb 3, 2019

Member

I can't think of a better solution, and I think it would also be difficult to use it incorrectly. I'm inclined to roll with it. I'd rather have the feature, and if someone can think of something more clever later, so much the better.

This comment has been minimized.

Copy link
@ChristopherDavenport

ChristopherDavenport Feb 4, 2019

Member

Hmm, I'm not sure if introducing blockers to the ScalaJS plan is fantastic.

@nebtrx

This comment has been minimized.

Copy link
Contributor Author

nebtrx commented Feb 1, 2019

Hey @rossabaker

Sorry for taking too much time to address your concerns, I was off for a few days. I pushed a commit with your package suggestion and I left some comments about the reflection stuff.

@nebtrx

This comment has been minimized.

Copy link
Contributor Author

nebtrx commented Feb 3, 2019

Cool @rossabaker! Thx for walking me through this.

@ChristopherDavenport
Copy link
Member

ChristopherDavenport left a comment

I think we'll need to address this in the future, but this gets us started.

@nebtrx

This comment has been minimized.

Copy link
Contributor Author

nebtrx commented Feb 7, 2019

Thanks for your review @ChristopherDavenport.

Regarding this:

I think we'll need to address this in the future, but this gets us started.

Were you referring about introducing blockers to the ScalaJS plan? or something else?

@ChristopherDavenport

This comment has been minimized.

Copy link
Member

ChristopherDavenport commented Feb 7, 2019

Yeah, I have a possible approach for how to improve this, but I don't want to shove this off onto you if you don't want to take it on.

My first idea is that we can put the type parameters all the way up that coproduct hierarchy such that. OverrideStrategy[+G[_], +F[_]] where we can then instantiate them as Nothing for the other two. Or if that doesn't work, we can just have to apply the type parameter. However that would work fully in types and not needing reflection.

@nebtrx

This comment has been minimized.

Copy link
Contributor Author

nebtrx commented Feb 7, 2019

Yeah, I have a possible approach for how to improve this, but I don't want to shove this off onto you if you don't want to take it on.

My first idea is that we can put the type parameters all the way up that coproduct hierarchy such that. OverrideStrategy[+G[_], +F[_]] where we can then instantiate them as Nothing for the other two. Or if that doesn't work, we can just have to apply the type parameter. However that would work fully in types and not needing reflection.

@ChristopherDavenport No worries, I can explore that path, especially because that issue was really annoying for me. However, at first glance, I don't see how we can get rid of the type erasure warning when pattern matching on something like OverrideStrategy[+G[_], +F[_]]. I'll give it a try tho, I might be missing the elephant in the room.

@rossabaker

This comment has been minimized.

Copy link
Member

rossabaker commented Feb 14, 2019

Extended in #2407.

@rossabaker rossabaker closed this Feb 14, 2019

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
You can鈥檛 perform that action at this time.