search

elli-lib / elli

Simple, robust and performant Erlang web server
https://github.com/elli-lib/elli/blob/develop/doc/README.md
MIT License
324 stars 38 forks source link

on otp-21+ use uri_string to parse uri #93

Closed tsloughter closed 3 years ago

tsloughter commented 3 years ago

This also fixes shit because currently it seems to always be returning undefined for scheme and other properties.

I'd also like to extend the request to include the rest of the information uri_string:parse can give -- like userinfo.

We should re-evaluate what OTP versions to retain support for.

coveralls commented 3 years ago

Coverage Status

Coverage increased (+0.07%) to 75.62% when pulling d920130e104736f2aa6b118527e1c9e39e753c1c on tsloughter:use-uri-string into 581ca7ad81b198ab5dcc30a81ac075ba30baced5 on elli-lib:main.

coveralls commented 3 years ago

Coverage Status

Coverage decreased (-0.4%) to 75.135% when pulling b99032db8a234e25f78c49891d8e09443c77f9bd on tsloughter:use-uri-string into 581ca7ad81b198ab5dcc30a81ac075ba30baced5 on elli-lib:main.

yurrriq commented 3 years ago

lgtm except for https://travis-ci.org/github/elli-lib/elli/jobs/743673725. will plan to check on this tomorrow too

yurrriq commented 3 years ago

Won't it always be the case that {undefined, undefined, undefined} = {Scheme, Host, Port} for {abs_path, FullPath}? :+1: to this PR still, and for adding stuff like userinfo and query to the request.

yurrriq commented 3 years ago

Looks like uri_string:dissect_query/1 is failing on name=knut%3D&foo with {error,missing_value,<<>>}

yurrriq commented 3 years ago

Seems like RFC 3986 doesn't specify, but that uri_string:dissect_query/1 doesn't support keys without values, at least on 21.3.

yurrriq commented 3 years ago

aaand. you already patched it :smile: https://github.com/erlang/otp/pull/1840

tsloughter commented 3 years ago

Hah, I don't remember patching that :+1:

Did you notice what is the earliest OTP with that patch? We should probably only use uri_string from that version and above.

yurrriq commented 3 years ago

Did you notice what is the earliest OTP with that patch?

Looks like 22.0

We should probably only use uri_string from that version and above.

:+1: