dropwizard / dropwizard

A damn simple library for building production-ready RESTful web services.
https://www.dropwizard.io
Apache License 2.0
8.51k stars 3.44k forks source link

Putting @Valid validation on List doesn't validate individual elements #470

Closed pimlottc closed 10 years ago

pimlottc commented 10 years ago

I'm trying to use the @Valid annotation on a resource request parameter to validate a list of inputs. While this works fine for a single model object, when I try it on a List of model objects, it doesn't validate each individual object like I expected. For example, adding a batch POST method to dropwizard-example's HelloWorldResource:

@POST
@Path("/s")
public void receiveHellos(@Valid List<Saying> sayings) {
    LOGGER.info("Received some sayings: {}", sayings);
}

Testing:

$ curl -X POST --include  http://localhost:8080/hello-world --data '{"content" : "hello"}' --header "Content-Type: application/json"; echo
HTTP/1.1 422 Unprocessable Entity
Date: Wed, 12 Feb 2014 23:28:25 GMT
Content-Type: application/json
Transfer-Encoding: chunked

{"errors":["content length must be between 0 and 3 (was hello)"]}
$ curl -X POST --include  http://localhost:8080/hello-world/s --data '[{"content" : "hello"}]' --header "Content-Type: application/json"; echo
HTTP/1.1 204 No Content
Date: Wed, 12 Feb 2014 23:31:13 GMT
Content-Type: application/json
pimlottc commented 10 years ago

I believe this should be supported to be consistent with JSR-303 (§ 3.1.3. Graph Validation):

Collection-valued, array-valued and generally Iterable fields and properties may also be decorated with the @Valid annotation. This causes the contents of the iterator to be validated. Any object implementing java.lang.Iterable is supported. This includes specifically:

• arrays of objects • java.util.Collection • java.util.Set • java.util.List • java.util.Map (special treatment see below)

Each object provided by the iterator is validated. For Map, the value of each Map.Entry is validated (the key is not validated).

nicktelford commented 10 years ago

Validation annotations aren't (currently) usable on Jersey resource methods. To do this, we need to upgrade to Jersey 2.x, which is planned, but a major undertaking.

mveitas commented 10 years ago

@nicktelford Are you saying that you can't use something like this? Isn't this implemented by the JacksonMessageBodyProvider?

public Response create(@Auth String authId, @Valid Glucose glucose) { .... }

nicktelford commented 10 years ago

I've just realised my mistake. Validation constraints themselves (e.g. @NotEmpty, @Min, etc.) cannot be used on a Jersey resource method parameter, but objects annotated with @Valid are validated only if they're deserialized by Jackson.

This issue is legitimate, as the JacksonMessageBodyProvider implements this validation, and is failing to cascade validations on collections.

mveitas commented 10 years ago

@nicktelford I probably won't have any connectivity to my office VPN due to the snowstorm, so I'll probably have some time this afternoon to put together a PR for this unless you wanted to tackle this.

nicktelford commented 10 years ago

Go for it. I have some ideas for handling Iterable instances, but I'm stumped on validating Map instances correctly.

mveitas commented 10 years ago

As @pimlottc mentioned it's not different than any other collection: Map, the value of each Map.Entry is validated (the key is not validated).

nicktelford commented 10 years ago

I totally missed that, I was looking at the Hibernate implementation and noticed that it retains the key for some reason. I guess we can just ignore it then.

pimlottc commented 10 years ago

Thanks for the quick response guys! One additional thought -

            assertThat(ConstraintViolations.formatUntyped(e.getConstraintViolations()))
                    .contains("id must be greater than or equal to 0 (was -1)",
                            "id must be greater than or equal to 0 (was -2)");

Ideally there'd be some way to determine which violation corresponded to which element within the collection. Not sure if there is a clean way to do this though, and what the message syntax might look like (including for maps).

mveitas commented 10 years ago

The message syntax for the map would be tough as you most likely would want to base it on the key. They value of the key might be a complex value and have a meaningful way to display a string.

Adding messaging for things such as a List or an array make sense, but how would you deal with a Set? There really isn't a way to return the index.

@pimlottc If you would like to research this further and figure out what it would take to get intelligent messaging for the list element validations, go for it!

nicktelford commented 10 years ago

Well, in the case of a Set, the value is unique, and unordered, so that's all you could ever provide.

nicktelford commented 10 years ago

471 resolves this issue. Any outstanding points will need to be raised elsewhere (just make a reference to this issue).

thegamblerrises commented 6 years ago

Hi, is it fixed only for Dropwizard or for jersey project as well?

joschi commented 6 years ago

@thegamblerrises This issue only covers Dropwizard.