matchers: query now ANDs multiple keys

[armadi1809]

Closes #6053

@francislavoie please let me know if you would rather solve the issue some other way. Thanks!

[mholt]

Thanks for helping out with this! I wonder if it'd be a little more robust / readable if it instead checked for any that don't match?

[francislavoie]

Probably need a couple more test cases to cover when there's query keys with a value that doesn't match (not only keys that don't exist in the request)

[armadi1809]

@mholt I am not sure I totally get what you mean by "if it instead checked for any that don't match?". I tried to tweak the logic a little bit to make it more readable / less nested. Happy to rework it again If you still don't like it.

@francislavoie I added a couple more tests around the case where the query key values do not match.

Also, I am not sure why the runner failed to download the action in the setup job of the failing test 😅

[francislavoie]

I think I see a possible bug. What if there's two values configured for a key and both match? It would increment twice. Need a test for that. I think the fix is to add a continue after incrementing (i.e. OR operation by short circuiting on the first positive match)

I re-ran the failing mac build. Probably just some flaky server.

[armadi1809]

I thought about this but since we're only checking against paramVal[0], it won't increment the counter twice. However, now that I am thinking about it, there is actually another potential bug. Let's say the matcher is MatchQuery{"debug": []string{"1"}} and the input is "/?debug=1&debug=2". This would work and we would get true back from the matcher. However, if the input is reversed (i.e. "/?debug=2&debug=1") this would give us false because we are only checking against paramVal[0]. I don't exactly get why we're only checking agains the first element but I believe a better approach would be to change the check to slices.Contains(paramVal, v) and continue on the first match as you suggested above. Does this sound reasonable?

[francislavoie]

Yeah, I think so. 👍

[armadi1809]

@francislavoie Any idea why some of the builds are failing to get the "slices" package?

[francislavoie]

Slices isn't available in Go 1.20, it's new in 1.21

[armadi1809]

Should I then be using "golang.org/x/exp/slices" similar to the caddyconfig/httpcaddyfile/httptype.go file or manually check if the slice contains the value I am looking for?

[francislavoie]

Yeah, you can use "golang.org/x/exp/slices".

[armadi1809]

Should be good to go now. Thanks for the help!

[francislavoie]

Only thing is I'm not quite sure what the behaviour should be when there's multiple values provided for a query. There's an argument to be said that it should only match if there's a single matching value. But we didn't really envision this being used with multi-value query keys.

Whatever we decide (what you have now is probably fine), we'll need to document clearly the way it behaves. We should adjust the MatchQuery godoc comment for the JSON docs https://caddyserver.com/docs/json/apps/http/servers/routes/match/query/.

Edit: Oh we already document it as such:

NOTE: Notice that query string values are arrays, not singular values. This is because repeated keys are valid in query strings, and each one may have a different value. This matcher will match for a key if any one of its configured values is assigned in the query string. Backend applications relying on query strings MUST take into consideration that query string values are arrays and can have multiple values.

So yeah, should be fine.

[armadi1809]

That sounds good. Let me know if we decide to treat that case differently then and I can adjust the code accordingly! Thanks again for your review/feedback.

[francislavoie]

LGTM, thanks for working on this!