Bugfix: Make util/request-url handle empty query string

[rutchkiwi]

Hello. I didn't make a separate issue, as I think the code itself most easily explains this problem.

The bug

Empty query strings would make request-url insert a trailing ? in the url for no reason. While not strictly invalid, it's a bit ugly. Empty query strings are added by ring.mock.request/request when you pass in an empty map for parameters.

Fix

This fixes that by checking that the query string is not empty before adding the ?.

Thanks in advance for your attention!

[weavejester]

Thanks for the pull request, and apologies for taking a little while to get round to reviewing this.

The code looks fine, but we'll need to update the commit message to adhere to the contributing guidelines. Can you change the commit message to:

Fix request-url when :query-string is empty

This avoids generating URLs with a trailing '?'.

[rutchkiwi]

Thanks, no worries! Apologies for not checking those guidelines. Updated now.

[weavejester]

Thanks! Let me think about this fix a little, as on reflection I'm a little less certain about it.

Aside from ring-mock, do you know of any other libraries that generate a blank query string?

[rutchkiwi]

Aside from ring-mock, do you know of any other libraries that generate a blank query string?

No, both http-kit and undertow-adapter give :query-string = nil for no query string.
When given just ?, like www.example.com/? http-kit gives you an empty query string.

IMO, logically, nil and empty string should be equivalent in this case. Unless you for some reason wanted to preserve a trailing ? - which to me seems a bit crazy.

But for my use case, if ring-mock stopped putting in the empty query string the problem would also go away.

[weavejester]

My reservation is that an absent query string and a blank query string are different things when parsing a URL. For instance, I'd expect "https://example.com/?" to have a query string of "" and "https://example.com/" to have an absent or nil query string when parsed.

Given that, producing a URL from a request map should maybe do the reverse; that is, parsing a URL into a request map, then generating a URL from the request map should end up with the original URL. I'm wondering in this case if it would be better to fix ring-mock.

[rutchkiwi]

I can see the logic. Shall I make the change to ring mock instead then?

[weavejester]

Oh, yes please. I think that would be a good change to make regardless. I'll keep this PR open a little while longer while I consider it, though.

[rutchkiwi]

Looking into it - ring mock only adds the query-string: "" when you pass it an empty map of parameters. For nil, it doesn't include the query-string at all. Are you still up for me to change it to make nil and {} params equivalent, i.e not include the query-string? Considering it then breaks the "symmetry" if you will.

IMO, while I see the theoretical reasons for wanting to distinguish no query string and an empty one - in practice it's just confusing to have them be considered separate. I can't imagine any case where you'd want anyone to see the www.example.com/? type URL.

[weavejester]

Yes, there's certainly a tension between behavior that's more consistent vs. behavior that's more practically useful.

Typically more basic functions will lean toward consistency while more advanced functions aim for practicality. We want the core building blocks to be as predicable as possible, while the functions built from those blocks can be more sophisticated.

So my thought in this case is that an passing an empty map of parameters {} to a mock request produces a nil query string, because the mock request is a tool that we want optimized for practical usage. While the request-url function is instead kept how it is in order to be more consistent, because it's one of the basic building block that more advanced functions are built upon.

Does that reasoning seem like it makes sense?

[rutchkiwi]

I've made a new PR for ring-mock now: ring-clojure/ring-mock#29
I hadn't thought of that core/peripheral differentiation. While I think I still lean towards thinking changing request-url would be better, I can totally appreciate the logic. And it is your library after all :)