cowboy_req:match_qs with nonempty constraint fails ungracefully on redefined parameters

[achan1989]

I'm using Cowboy 2.7.0.

Perform a HTTP get on http://host/path?username=x&username=y.
In the handler, calling cowboy_req:match_qs([{username, nonempty}], Req) causes the following crash:

initial call: cowboy_stream_h:request_process/3
pid: <0.522.0>
registered_name: []
exception error: no function clause matching 
                 cowboy_constraints:nonempty(forward,[<<"y">>,<<"x">>]) 
  in function  cowboy_constraints:apply_list/3 (/home/myproject/_build/default/lib/cowboy/src/cowboy_constraints.erl, line 48)

I believe that the same will happen for the int constraint.

Given that cowboy_req:parse_qs can handle a redefined parameter, I found this a little surprising (but understandable). I suppose you could modify cowboy_constraints:apply_constraint to handle this in a few different ways:

1. Succeed and return all parameters if they all pass the constraint.
2. Succeed and return any parameters that pass the constraint, as long as one does.
3. Always fail if a parameter is redefined (raise a request_error like normal).
4. Allow the caller to choose one of those behaviours.

But maybe that's more complicated than you want?

[essen]

So that's not really a bug, the constraints were made to work only with binaries on purpose. They can of course be extended, although I do not think the int constraint should be (or perhaps there's a good reason for it to also accept int but I'm not sure there is.

For nonempty I could definitely see a case for accepting lists and looking whether the list is empty. It sounds like this would cover your case.

But otherwise constraints crashing on invalid input data is the correct behavior.

[achan1989]

So that's not really a bug, the constraints were made to work only with binaries on purpose.

For nonempty I could definitely see a case for accepting lists and looking whether the list is empty. It sounds like this would cover your case.

For my case I want to assert that each member of the list isn't empty. But fair enough. On reflection I'm not too desperate to put this change into the library, as it's an unusual situation that people might not want to accept by default. I'll define a custom constraint function.

But otherwise constraints crashing on invalid input data is the correct behavior.

The main issue I had was not the crashing itself, but that it doesn't give a friendly failure reason. Normally when a nonempty fails you'd get:

{{request_error, {match_qs,#{username => {nonempty,empty,<<>>}}},
  'Query string validation constraints failed for the reasons provided.'},
  blah...

If redefined parameters aren't valid then personally I would prefer it to crash with something like:

{{request_error, {match_qs,#{username => {nonempty,redefinition_forbidden,[<<"y">>,<<"x">>]}}},
  'Query string validation constraints failed for the reasons provided.'},
  blah...

That way it's more obvious why it's failing, and I don't have to read the library code to figure out what's going on. It's a minor point though. Thanks for maintaining a cool library :)

[essen]

Yeah I don't think nonempty should look at members, but maybe there should be a nonempty_values or something.

I can also see the value in not crashing. To be honest this is one of the cases where I wasn't sure what was the best and therefore did not make a decision yet (you'll notice there's no tests for these cases). I think it comes from constraints being added for routing only, initially, where the crash does help. But then they were added to match functions and there it's not exactly the same.

I'll soon be adding some good improvements to constraints via the work I'm doing on Farwest. So let's keep this open and I'll consider it when that happens.