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

Custom percent-encoders: don't mutate base encoder #156

Merged
merged 1 commit into from
Apr 26, 2021

Conversation

aantron
Copy link
Contributor

@aantron aantron commented Apr 18, 2021

At the moment, `Custom mutates the safe character array of its underlying component kind:

ocaml-uri/lib/uri.ml

Lines 166 to 176 in b4a8375

| `Custom ((component : component), safe, unsafe) ->
let safe_chars = safe_chars_for_component component in
for i = 0 to String.length safe - 1 do
let c = Char.code safe.[i] in
safe_chars.(c) <- true
done;
for i = 0 to String.length unsafe - 1 do
let c = Char.code unsafe.[i] in
safe_chars.(c) <- false
done;
safe_chars

This was added in #147. I'm not sure if this is intentional or not (cc @orbitz). I am currently abusing `Custom to build IRI-like percent encoders from existing component kinds (as a workaround for lack of #34). I don't want to modify the underlying encoders, though — I'd like both IRI and ASCII-only support for my own code, and, in addition, I fear about violating the assumptions of other code linked into the program.

@orbitz
Copy link

orbitz commented Apr 18, 2021

Whoops! No, this is unintentional!

@aantron
Copy link
Contributor Author

aantron commented Apr 26, 2021

Any word on a merge of this PR and release? This is needed for Dream (see aantron/dream#50). Of course, it can be worked around by not using ocaml-uri, or using it less, but I would prefer not to add code that exists in ocaml-uri to Dream as well.

@avsm
Copy link
Member

avsm commented Apr 26, 2021

This is obviously safe and very important to release. I'll merge and cut a release now.

@avsm avsm merged commit 482a034 into mirage:master Apr 26, 2021
@aantron
Copy link
Contributor Author

aantron commented Apr 26, 2021

Thanks @orbitz and @avsm!

avsm added a commit to avsm/opam-repository that referenced this pull request Apr 26, 2021
CHANGES:

* Do not mutate the base encoder when using custom percent encoders.
  This was a bug introduced in mirage/ocaml-uri#147. (mirage/ocaml-uri#156 @aantron)
* Disable Travis CI tests and switch Win/Mac tests to GitHub Actions and
  Linux ones to ocaml-ci (@avsm).
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

Successfully merging this pull request may close these issues.

3 participants