Address query sort issues

zmackie commented 5 years ago

A recent change to HTTP mock added some code that allowed Registering a responder with a query where query order does not matter. This was a great change, but the implementation had an issue: it is perfectly valid for multiple query params to have the same key. Languages and HTTP libraries handle this situation in various ways, but in Go, the standard library handles it by combining the values into a slice. Thus:

func main() {
    u, err := url.Parse("http://bing.com/search?q=dotnet&q=aaa")
    if err != nil {
        log.Fatal(err)
    }
    fmt.Println(u.Query())

}

$ map[q:[dotnet aaa]]

https://play.golang.org/p/x0P-dhI85sh

In addressing this problem, I discovered another issue. Existing code was sorting Querys and considering urls with query params in different order to be different. Reasonable people could disagree about this, but there seems to be general consensus that this second part (considering order of query params as significant) is incorrect. If you think of a query as a map of keys to values (which is the type used internally in Go), order does not enter into it. Image a server dealing with multiple params. It wouldn't care about their order in general. It would treat the keys and values as individual query and handle them in a transparent order that callers would never know or care about.

Thus, I submit this PR with the following intent:

Instead of have callers of RegisterResponder care about query order and adding new methods to get around this confusion, RoundTrip and RegisterResponder should canonicalize querys, using the Go standard libraries url.Query().Encode() to avoid confusion around query param order. Thus all urls containing the same query params, whatever the order, will be considered equivalent for matching to responders.

zmackie commented 5 years ago

Looks like ./transport.go:390:10: undefined: strings.Builder is failing on older version of go, which makes sense as its a newer api. If the maintainers are generally in favor of this PR, I'll dig into making it compatible with older versions of go.

zmackie commented 5 years ago

We could also let #54 shake out, and I can address my issue after that's merged. Up to ya'll.

dlebech commented 5 years ago

Thanks @Zanadar and sorry for the wait. I don't think I'm the best person to review your PR, since it's non-trivial and I don't actually use this library anymore (but it is my fault that the regression exists :disappointed: ) Perhaps I can convince other recent contributors (@snigle, @konalegi) to review, and then I'll be happy to merge (I'm apparently the only one with merge privileges (but not admin privileges) actively looking at this repo, and I'm struggling to keep up, sorry! :grimacing: )

zmackie commented 5 years ago

@dlebech Sounds good. No rush! Being a maintainer is hard! Thanks for everything you do!

dlebech commented 5 years ago

Still no-one has stepped in here and I'm sorry about that @Zanadar . I hope someone (perhaps @kardolus) could provide some eyes to look this over and I'll be happy to merge it. I wish I could hand over maintainer privileges, but I don't have admin rights to the repo :disappointed_relieved:

I would probably recommend using a fork at this point, because I can't provide the quality needed to keep this one going. :beers:

jarcoal / httpmock

Address query sort issues #55