Skip to content

HTTPS clone URL

Subversion checkout URL

You can clone with HTTPS or Subversion.

Download ZIP

Loading…

onrequest protocol handler #291

Closed
saleyn opened this Issue · 12 comments

2 participants

@saleyn

This is a feature request. The onrequest handler is defined in the protocol globally, such that request rewrites can go through that function before any dispatching rules are applied. However, when there are multiple host-specific rules defined for different hosts, it would be much more convenient to be able to specify onrequest handler for each host separately, so that request rewrites can be customized per host dispatching rule-set. In the existing implementation if a url rewrite needs to be applied only to one host definition, the onrequest handler has to do pretty much the same redundant work as the dispatch function does in the protocol to match the appropriate host name before applying a url rewrite.

@saleyn

Ideally the dispatch rules could optionally contain Options such as:

{dispatch, [{Host, PathRules} | {Host, PathRules, Options}]}

in the later case the Options list could have:

[{onhostrequest, {fun(Req, Opaque), Opaque}]
@saleyn

One more issue with the onrequest logic. I believe it is a bug. The current implementation of the protocol module is:

onrequest(Req, State=#state{onrequest=undefined}, Host, Path) ->
    dispatch(Req, State, Host, Path);
onrequest(Req, State=#state{onrequest=OnRequest}, Host, Path) ->
    Req2 = OnRequest(Req),
    case cowboy_req:get(resp_state, Req2) of
        waiting -> dispatch(Req2, State, Host, Path);
        _ -> next_request(Req2, State, ok)
    end.

However, if the OnRequest function rewrites the path in the request, it won't be effective, since Path is passed as separate argument that makes it impossible to rewrite the path in the onrequest handler.

@essen
Owner

Yes this looks like a bug.

@essen
Owner

You can match on hosts or paths directly in the onrequest handler, isn't that enough?

@saleyn

My point was that you CAN do it in the onrequest handler, which means that you would need to exactly replicate the matching logic implemented in the cowboy_protocol, which makes little sense, since not only it'll be redundant from the implementation point of view, but also from the invocation, since it would be first done in onrequest handler, and later in the dispatching part of the protocol handler.

@essen
Owner

Yeah I see. I'll think about it, but don't expect a change anytime soon. :) I do want to improve the onrequest/dispatch phase of requests but I'm not sure how the two should be tied.

@saleyn

Right, this is not a priority, but rather a valid implementation concern that I ran into when I needed to do custom URL rewriting on multiple subdomains.

@essen
Owner

Yeah. I'll let this open for a while until we can find a great solution to this.

@essen
Owner

Hey, I still don't think this should be done in routing. However the soon to be added middleware mechanism should allow to have a rewrite middleware for performing these tasks independently of routing. Do you think this would fit your uses?

@saleyn

It is fine, as long as it avoids the redundancy of parsing.

@essen
Owner

Middlewares are in.

@essen
Owner

I suppose I can close this. :) Thanks!

@essen essen closed this
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Something went wrong with that request. Please try again.