jarcoal / httpmock

HTTP mocking for Golang
http://godoc.org/github.com/jarcoal/httpmock
MIT License
1.93k stars 103 forks source link

Query paramter with no value #66

Closed itchyny closed 5 years ago

itchyny commented 5 years ago

Recent change #50 in the v1 branch breaks the behavior of query parameter without values. Query parameter with no value is valid due to RFC 3986. Here's a patch to test the case.

--- a/transport_test.go
+++ b/transport_test.go
@@ -217,6 +217,9 @@ func TestMockTransportPathOnlyFallback(t *testing.T) {
        "/hello/world?query=string&query=string2": {
            testUrl + "hello/world?query=string&query=string2",
        },
+       "/hello/world?query": {
+           testUrl + "hello/world?query",
+       },
        "/hello/world#fragment": {
            testUrl + "hello/world#fragment",
        },

Ping to the related patch authors: @konalegi (for #50), @Zanadar (for #55) and @maxatome (for #61)

maxatome commented 5 years ago

@itchyny could you check with last commit of #61 ?

itchyny commented 5 years ago

It looks like it passes the test case in my comment, thanks. But I'm afraid looking up ?q against mock ?q= succeeds but ?q= against ?q fails and users are confused by this asymmetric behavior. I'm not sure how useful the strict order of RegisterResponder is. Normalizing in RegisterResponder is the another solution (in #55) (and honestly, I do prefer this way) and, this will avoid the asymmetry. This looks like a matter of library design.

maxatome commented 5 years ago

Normalizing in RegisterResponder does not allow to register for a specific order of query parameters. That is why I prefer to first try to match with untouched query params given to RegisterResponder, then to fall back to sorted query params. I do not have an easy solution to handle any-ordered query params given as a string with the current logic of storing responders, except ordering by yourself the url given to RegisterResponder or using RegisterResponderWithQuery or RegisterResponderWithQueryValues of course. For information, I prepare an other PR introducing regexps matches, in which I will try to address this case too.

maxatome commented 5 years ago

FYI #61 just merged.

itchyny commented 5 years ago

Thank you! I understand the difference of RegisterResponder and RegisterResponderWithQuery.