SetQueryParams with collection of KV pairs, duplicate keys should append, not overwrite

[tmenier]

Flurl allows setting multiple query parameters via object notation, with special handling for things that look like collections of key/value pairs. It's possible that these collections could contain duplicate keys, and in this case every value should be appended. Instead they get overwritten each time, so this test fails:

var url = "http://foo.com".SetQueryParams(new[] {
    new { key = "x", value = 1 },
    new { key = "x", value = 2 },
});

Assert.AreEqual("http://foo.com?x=1&x=2", url.ToString());
// fail! actual value is http://foo.com?x=2

In a nutshell, that should pass.

[Marusyk]

It seems to be a breaking changes. Could you please advise if you wanna replace that logic or add some parameter for that? Then I'll try to prepare PR. Thx

[tmenier]

@Marusyk Sometimes it's a fine line between a breaking change and fixing some unintended behavior seems "obviously" quirky/buggy. I logged this because I bumped into it in a real-world scenario and it seemed "obviously" wrong, but I'm now wondering if the likelihood of breaking someone is higher than I thought. Tough call.

Keep in mind this DOES overwrite, which is intentional and won't change:

url
    .SetQueryParam("x", 1)
    .SetQueryParam("x", 2);

I intentionally used the word "Set" instead of "Add" or "Append" here to help make it clear that you're providing the entire value so go ahead and squash whatever might already be there. If you want x=1&x=2, you can always do SetQueryParam("x", new[] { 1, 2 }).

The scenario in this issue is more unusual. If you use SetQueryParams with object notation or a dictionary, you can't have duplicate keys, so it doesn't apply there either. This is only if you pass in some collection of key/value pairs that does have duplicate keys. In my mind, you're saying you want all of them, and overwriting is unintuitive. But, maybe it's unusual enough that the risk of breaking someone is not worth changing it? I'm not sure.

[rcollette]

In the case where all the values are passed in the single set method and squashed, it seems like an obvious bug to be patched.

[tmenier]

I was hoping I could piggyback off existing code to fix this but it's a little trickier than I thought. I'm considering adding AppendQueryParam(s) that, unlike SetQueryParam(s), don't overwrite. With that in place this will be much easier.

[tmenier]

Although I still think this seems like an obvious quirk, now that I'm pushing it out to 4.0 I'll play it safe and call it breaking. #688 will be a prerequisite.

[tmenier]

Now available in prerelease 4 https://www.nuget.org/packages/Flurl/4.0.0-pre4