Re: boost.url review

From: Vinnie Falco (vinnie.falco_at_[hidden])
Date: 2022-08-16 18:31:07

On Tue, Aug 16, 2022 at 11:03 AM Ruben Perez <rubenperez038_at_[hidden]> wrote:
> Here is my review of Boost.Url.

Thank you for graciously contributing your time to review the library!

> I've found the naming for view containers confusing (e.g.
> params and params_view).

Well...yeah. I am not entirely happy about having 8 different
containers between the segments and params ranges. However the
decision was made to offer the user an interface that resembles
standard containers, so that standard algorithms could be composed on
them (more or less). I think that the ForwardRange and
BidirectionalRange interfaces were the right choice, so we followed
the design where it led us. The need for having read-only views,
modifiable views, over separate encoded versus unencoded strings left
us with the 8 containers.

> I would either emphasize it in the docs, or rename the containers to
> something that reflects this fact (e.g. const_params and mutable_params,
> as asio does).

I would much rather emphasize it in the docs, because writing "const"
and "mutable" over and over is bound to be a chore. Asio gets away
with it because the mutability of buffers is central to the role of
asynchronous operations but I'm not so sure the same applies here.

We are very happy to hear proposals for alternate container names,
especially if they are complete proposals (give an alternative for
each existing name) and if necessary what the url_view and url member
functions would be renamed to.

> I'd consider exposing higher-level functions here, maybe taking a lvalue
> reference to a std::string, allowing the user to percent-encode strings easier
> (so they resemble JavaScript encodeURIComponent).

Yes, I am not joyous about our current percent-encoding APIs. There's
a lot of them, with small variations, and some simple operations are
missing (appending or assigning to a MutableString out-param as you
pointed out). They are still great but I think they could be greater.
We will work on them some more, and if you or anyone else would like
to provide input into that process, it is welcomed.

> I don't know if this is a common use case, but I'd consider adding a function
> that simplifies it (i.e. params::set_value(string_view key, string_view value).

Yep, we need that! There are open questions (like what do you do if
more than one key exists). Here is an open issue for it:

<https://github.com/CPPAlliance/url/issues/412>

> I'm not very keen on exposing the parsing infrastructure and helper
> functions (other than percent encoding/decoding) as public API,
> as I think it violates the principle of API surface minimization.

I feel you there but they have to go somewhere because the
alternatives are worse. It isn't perfect, I know.

> The documentation is good and the reference is complete, but I miss
> some examples.

Yep. Its not easy to come up with a complete application that just
does stuff with URLs.

> I'd add a page and an example on percent encoding functions, as they
> are only explained in the reference and the explanation assumes
> familiarity with the CharSet concept.

Yes I think these routines need some tidying and docs.

> I'm also curious about the security review that is announced
> in the main page - when is that going to take place?

Once the code settles down a little, if the library is accepted then
the version which first appears in Boost would be reviewed. Possibly
the second release if the company doing the review is very busy.

Regards