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

Can we add scheme, host, port to the elli request record type? #28

Closed
CharlesOkwuagwu opened this issue Feb 25, 2017 · 13 comments
Closed

Comments

@CharlesOkwuagwu
Copy link

Please can we add scheme, host, port metadata to elli request record type (:req)?

Why is it being discarded here? https://github.com/elli-lib/elli/blob/develop/src/elli_http.erl#L688

parse_path({abs_path, FullPath}) ->
    case binary:split(FullPath, [<<"?">>]) of
        [URL]       -> {ok, {FullPath, split_path(URL), []}};
        [URL, Args] -> {ok, {FullPath, split_path(URL), split_args(Args)}}
    end;
parse_path({absoluteURI, _Scheme, _Host, _Port, Path}) -> # why  discard scheme, host and port ??
    parse_path({abs_path, Path});
parse_path(_) ->
    {error, unsupported_uri}.
@deadtrickster
Copy link
Member

deadtrickster commented Feb 25, 2017

I can do this tonight.

@yurrriq

@deadtrickster
Copy link
Member

So I looked into this and it appears that {absoluteURI branch never used.
Which means we have to do two things:

  1. Find where and why it was introduced
  2. Add asked field like this:
    • scheme - type of socket (http/https)
    • port - port of socket
    • host - from host header.

The last one is more tricky - Host field can have host AND port. We could probably add this too under host_port.

@CharlesOkwuagwu
Copy link
Author

If we are just matching on the output of erlang:decode_packet/3 i think it simplifies this considerably

@CharlesOkwuagwu
Copy link
Author

@deadtrickster I have submitted a patch to add the required changes, but I can't get it to pass your build system... please help review.

Thanks.

@yurrriq
Copy link
Member

yurrriq commented Mar 15, 2017

So #29 won't get the job done without a pretty thorough overhaul. I'm wondering, what's your need to add these fields to #req{}? On second thought, I'm fairly hesitant to make such a drastic change.

@CharlesOkwuagwu
Copy link
Author

That's fine. It just seemed that elli was needlessly discarding metadata from the request.

I felt it would be easy to capture, hence the PR i submitted.

It does require changes to #req{}, but other than that, everything else would work as it had worked prior.

@deadtrickster
Copy link
Member

Actually I think these fields are rather useful (think routing, validation, etc). I just forgot about #29 :-)

@yurrriq
Copy link
Member

yurrriq commented Mar 15, 2017

Ok. I've got a WIP stashed locally.

@yurrriq
Copy link
Member

yurrriq commented Mar 15, 2017

I can push if you want to have a look.

@deadtrickster
Copy link
Member

please do!

@yurrriq
Copy link
Member

yurrriq commented Mar 15, 2017

@deadtrickster: bfcca83

I've made FIXME notes throughout.

Edit: I'm off to bed. Feel free to take over the branch.

@tsloughter
Copy link
Member

@yurrriq what happened with this?

@yurrriq
Copy link
Member

yurrriq commented Mar 1, 2018

Missed that comment.. I'm not sure what happened with this either.

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

No branches or pull requests

4 participants