Remove trailing question mark on empty query

[aaneto]

Hello everyone!

I have been tinkering with the implementation of query_pairs_mut after looking at this issue on the reqwest project. The core of the issue is that a serialization of an empty query insert a '?' at the end of the url.

I went on to try and make the serialization of query_pairs_mut to only insert a trailing question mark after the first serialization. Making url.query_pairs_mut().clear(); equivalent to url.set_query(None).

I don't know if this behaviour is desired on the rust-url side, but would like to know if this change is feasible.

---

This change is 

[SimonSapin]

URLSearchParams is what’s most similar to query_pairs_mut on the web, and it does set a precedent of removing the query string component when it would be the empty string: https://url.spec.whatwg.org/#concept-urlsearchparams-update

There’s a question whether there are current users of the url crate who rely on the current behavior. It feels low-risk but it’s still a behavior change. It’s not obviously fixing a bug to me: an empty query string is different from no query string, and it’s not absurd that removing each key/value pair yields the former.

However, looking at the diff, including ? in the expected test results looks wrong. The ? character is not part of the application/x-www-form-urlencoded syntax, it is the separator that marks the start of a query string in an URL. But form_urlencoded::Serializer can be used without an URL.

[dekellum]

I had assumed the current behavior was consistent with avoiding all non-specified normalizations. To add this normalization I'm using something like this, also usable as a workaround:

        let mut remove_query = false;
        if let Some(q) = url.query() {
            if q.is_empty() {
                remove_query = true;
            }
        }
        if remove_query {
            url.set_query(None);
        }

FWIW: I think dropping an empty query is form of normalization, albeit a commonly desired and less controversial one.

Edit: NLL (2018 edition) can make above much simpler:

        if let Some(q) = url.query() {
            if q.is_empty() {
                url.set_query(None);
            }
        }

[aaneto]

Adding the '?' to the serialization is really wrong, with those points in mind.

The reason behind this modification was avoiding having to remove the query after every empty serialization, maybe that's not a relevant concern, since the query removal is not that expensive.

My first idea was doing this empty checking on reqwest side, but maybe this modification would be useful here also. I can remake this branch to use @dekellum version which looks cleaner and already pass tests without changes if this change is desired, but I feel that this desirability is undecided.