fix parsing / printing of IPv6 addresses in URIs

mirage / ocaml-uri

RFC3986 URI parsing library for OCaml

Other

97 stars 57 forks source link

fix parsing / printing of IPv6 addresses in URIs #169

Closed anmonteiro closed 11 months ago

anmonteiro commented 1 year ago

fixes #163

bikallem commented 1 year ago

I recently came across the same issue referenced by this PR.

Are the CI failures related to this PR? From the tests, the implementation looks correct.

tmcgilchrist commented 1 year ago

@anmonteiro if you rebase this the build should pass. see https://github.com/tmcgilchrist/ocaml-uri/pull/2/checks?check_run_id=12145037435

avsm commented 1 year ago

This looks good to me after a read through the code, but I'd like to just run it through crowbar a bit more. It should be ok for a uri.5.0 release soon.

lindig commented 1 year ago

Could you consider merging this? We just hit this bug when trying to move to IPv6.

bernhardkaindl commented 1 year ago

@avsm We likewise had to take this change as patch into XenServer as we need it working too!

BTW:

Pau also manually tested it to work as expected using an utop session:
```
# #require "uri" ;;
# let uri = Uri.of_string "http://[::1]";;
# Uri.path uri ;;
- : string = ""
# Uri.host uri ;;
```
I hope this helps you to review it for merging.

benjamreis commented 1 year ago

FYI I backported this fix in a previous version of the lib also presenting the IPv6 URI bug and it has indeed solved it. Eager to see this PR merged and to get the fix from upstream! :+1:

dinosaure commented 11 months ago

It seems that this PR is highly required but it breaks the uri behavior. From the RFC point-of-view, the fix seems right as explained in #169. I need to convince myself more to merge & cut a release due to the implication that this PR can have on the OCaml ecosystem. However, I consider to merge it as soon as I can.