New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jersey Validation Improvements #1734

Merged
merged 16 commits into from Sep 29, 2016

Conversation

Projects
None yet
5 participants
@cmicali
Contributor

cmicali commented Sep 21, 2016

This PR is a few changes we have been using for a while to improve the built-in jersey parameter validation error messages provided by Dropwizard.

Better Enum Validation Messages
Currently dropwizard resources return a 404 if a request is made with an invalid value for a parameter of an Enum type. This PR includes ValidatingEnumParamConverterProvider which provides converters for all enums used in method parameters. The converter then provides better error messages like "query param choice must be one of [OptionA, OptionB, OptionC]" when invalid values are sent.

Parameter Names in Parse Failure Messages
Currently when an invalid value is passed into a *Param parameter dropwizard returns an error message like "Parameter is not a number.". This PR includes AbstractParamConverterProvider
along with a slight enhancement to the *Param classes that returns messages that include the name of the parameter that failed validation, like "query param user_id is not a number."

Hibernate ParameterNameProvider
Small change to move most of parameter naming logic from ConstraintMessage to JerseyParameterNameProvider. This implements the standard ParameterNameProvider interface that Hibernate uses to resolve parameter names during validation.

New DurationParam
Include a new DurationParam that can parse String input in the format supported by dropwizard's existing Duration class.

@@ -0,0 +1,24 @@
/*
* Copyright (c) 2016. Sense Labs, Inc. All Rights Reserved

This comment has been minimized.

@nickbabcock

nickbabcock Sep 21, 2016

Contributor

Please remove copyright

This comment has been minimized.

@cmicali

cmicali Sep 21, 2016

Contributor

D'oh, sorry - auto-added by intellij.

@nickbabcock

This comment has been minimized.

Contributor

nickbabcock commented Sep 21, 2016

Awesome PR, did a quick glance and everything looks top notch.

I'm going to let this PR sit for a little bit as I mull over some of the implementation (like better enum validation messages).

.queryParam("length", 50)
.request()
.get();
assertThat(response.getStatus()).isEqualTo(400);

This comment has been minimized.

@nickbabcock

nickbabcock Sep 21, 2016

Contributor

Do you mind splitting up each of these assertions into a separate test case with a descriptive name like the other tests found in the file?

This comment has been minimized.

@cmicali

cmicali Sep 21, 2016

Contributor

Yeah no problem - will do now.

fix handling of NonEmptyStringParam, remove invalid copyright header,…
… split composite test methods into individual test methods
@cmicali

This comment has been minimized.

Contributor

cmicali commented Sep 21, 2016

Addressed comments on comments & tests and also fixed test failures related to NonEmptyStringParam. The new converter for *Param types was not handling NonEmptyStringParam properly. I added support for it and removed NonEmptyStringParamFeature which was no longer needed.

@coveralls

This comment has been minimized.

coveralls commented Sep 21, 2016

Coverage Status

Changes Unknown when pulling 4013644 on cmicali:jersey-validation into * on dropwizard:master*.

import io.dropwizard.util.Duration;
public class DurationParam extends AbstractParam<Duration> {

This comment has been minimized.

@cmicali

cmicali Sep 22, 2016

Contributor

This isn't directly related to the PR, but we use this often and seems like something Dropwizard should include by default.

This comment has been minimized.

@nickbabcock

nickbabcock Sep 22, 2016

Contributor

Ideally, this may have been split into another PR, but that's ok here.

Can you write a test for this?

EDIT: and also update the PR description to mention this feature so when updating the release notes we won't miss anything

This comment has been minimized.

@cmicali

cmicali Sep 24, 2016

Contributor

Done - updated description and added test.

// leaving Jersey to handle these parameters as it normally would.
return null;
}
return new ParamConverter<T>() {

This comment has been minimized.

@nickbabcock

nickbabcock Sep 22, 2016

Contributor

Should this param converter be more like Jersey's default AbstractStringReader?

This comment has been minimized.

@cmicali

cmicali Sep 24, 2016

Contributor

Possibly - the one thing we might not want to change is the null handling. Jerey's provider will throw an IllegalArgumentException if the value is null. My change does not, as this would be a breaking change with previous versions of dropwizard.

Otherwise, we could change the exception handling to match abstract string provider or we could inherit from AbstractStringReader and just modify the behavior we don't like. Thoughts?

This comment has been minimized.

@nickbabcock

nickbabcock Sep 25, 2016

Contributor

Do you know how it will break backwards compatibility because doesn't the current classes get instantiated through StringConstructor, which calls AbstractStringReader

This comment has been minimized.

@cmicali

cmicali Sep 26, 2016

Contributor

I think you're right, I'm getting confused. Just committed an update that makes it basically match AbstractStringReader and added a test for not provide vs empty parameter. All tests pass so I think this is OK.

public class JerseyParameterNameProvider extends ReflectionParameterNameProvider {
@Override
public List<String> getParameterNames(Constructor<?> constructor) {

This comment has been minimized.

@nickbabcock

nickbabcock Sep 22, 2016

Contributor

Does this method need to be included? Looks like you're just delegating to super.

This comment has been minimized.

@cmicali

cmicali Sep 24, 2016

Contributor

No, will remove.

@@ -0,0 +1,146 @@
/*
* Copyright (c) 2016. Sense Labs, Inc. All Rights Reserved

This comment has been minimized.

@nickbabcock

nickbabcock Sep 22, 2016

Contributor

Oh no, another copyright 😛

This comment has been minimized.

@cmicali

cmicali Sep 24, 2016

Contributor

Removed!

This comment has been minimized.

@nickbabcock

nickbabcock Sep 27, 2016

Contributor

Looks like the copyright came back 😉

This comment has been minimized.

@cmicali

cmicali Sep 27, 2016

Contributor

Removed, again. Intellij: amazing 99.999% of the time.

@coveralls

This comment has been minimized.

coveralls commented Sep 24, 2016

Coverage Status

Changes Unknown when pulling b64ef9f on cmicali:jersey-validation into * on dropwizard:master*.

@evnm

This is great! Thanks for contributing.

Just have a couple nits and requests for documentation.

import java.lang.reflect.InvocationTargetException;
import java.lang.reflect.Type;
public class AbstractParamConverterProvider implements ParamConverterProvider {

This comment has been minimized.

@evnm

evnm Sep 24, 2016

Member

Could you add a brief Javadoc to this class explaining what it does, please?

import java.util.Optional;
public class JerseyParameterNameProvider extends ReflectionParameterNameProvider {

This comment has been minimized.

@evnm

evnm Sep 24, 2016

Member

Rm extra newline

import java.util.List;
import java.util.Optional;
public class JerseyParameterNameProvider extends ReflectionParameterNameProvider {

This comment has been minimized.

@evnm

evnm Sep 24, 2016

Member

Ditto brief Javadoc

import java.util.Set;
@Provider
public class ValidatingEnumParamConverterProvider<C extends Enum<C>> implements ParamConverterProvider {

This comment has been minimized.

@evnm

evnm Sep 24, 2016

Member

Ditto brief Javadoc

This comment has been minimized.

@cmicali

cmicali Sep 25, 2016

Contributor

Thanks for the feedback - newline removed and javadoc added to those 3 classes.

@coveralls

This comment has been minimized.

coveralls commented Sep 25, 2016

Coverage Status

Changes Unknown when pulling a492f0a on cmicali:jersey-validation into * on dropwizard:master*.

@evnm

evnm approved these changes Sep 25, 2016

jersey: updated AbstractParamConverterProvider to look/behave more li…
…ke AbstractStringReader from Jersey and added a test for @NotNull empty *Param values
@coveralls

This comment has been minimized.

coveralls commented Sep 26, 2016

Coverage Status

Changes Unknown when pulling 435447d on cmicali:jersey-validation into * on dropwizard:master*.

@coveralls

This comment has been minimized.

coveralls commented Sep 27, 2016

Coverage Status

Changes Unknown when pulling 7fa61dc on cmicali:jersey-validation into * on dropwizard:master*.

* include the parameter name and a list of valid values.</p>
*/
@Provider
public class ValidatingEnumParamConverterProvider<C extends Enum<C>> implements ParamConverterProvider {

This comment has been minimized.

@nickbabcock

nickbabcock Sep 28, 2016

Contributor

I was able to cut the implementation in half and make it somewhat compatible with the FuzzyEnumModule that we use for Jackson enum serialization. I think it could be a good idea if params and body enums were handled the same way:

If there isn't anything fundamentally wrong with my suggested foundation, I'd like to see it integrated and improved upon 😄

This comment has been minimized.

@cmicali

cmicali Sep 28, 2016

Contributor

😮 this is much better. Facepalm for the scanning work when all you need is

 if (!rawType.isEnum()) {
    return null;
}

I will update this PR with this - should I include

 final String text = CharMatcher.WHITESPACE
                    .removeFrom(jp.getText())
                    .replace('-', '_')
                    .replace('.', '_');

from FuzzyEnumModule to make it behave exactly the same way?

This comment has been minimized.

@cmicali

cmicali Sep 28, 2016

Contributor

Two other questions:

  1. Should I rename the class FuzzyEnumParamConverterProvider to make it match FuzzyEnumModule ?
  2. Should we move the 'fuzzy' part out into dropwizard-util, maybe to an Enums class?

This comment has been minimized.

@nickbabcock

nickbabcock Sep 28, 2016

Contributor

No problem -- the power of open source 🎉

To answer your questions:

  • Yes it is probably prudent to make the implementations exactly the same.
  • So extracting the shared code into an Enums class in dropwizard-util would be a fine idea
  • And yes a rename to FuzzyEnumParamConverterProvider, I'm assuming, will help discover-ability

This comment has been minimized.

@cmicali

cmicali Sep 28, 2016

Contributor

Changes made and committed.

This comment has been minimized.

@nickbabcock

nickbabcock Sep 29, 2016

Contributor

Looks like there was a change that made readsEnumsUsingToStringWithDeserializationFeatureOff test case fail. Also looks like there is a style nitpick remaining.

This comment has been minimized.

@cmicali

cmicali Sep 29, 2016

Contributor

Forgot to add that change to the previous commit - commited/pushed.

cmicali added some commits Sep 28, 2016

- Better enum param converter from nick babcock
- Renamed ValidatingEnumParamConverterProvider to FuzzyEnumParamConverterProvider
- Moved the fuzzy string to enum code to an Enums class and updated FuzzyEnumModule and FuzzyEnumParamConverterProvider to use it
@coveralls

This comment has been minimized.

coveralls commented Sep 29, 2016

Coverage Status

Changes Unknown when pulling 9818fad on cmicali:jersey-validation into * on dropwizard:master*.

@coveralls

This comment has been minimized.

coveralls commented Sep 29, 2016

Coverage Status

Changes Unknown when pulling 1b179e4 on cmicali:jersey-validation into * on dropwizard:master*.

@evnm

Just a couple more style nits. This came together nicely!

}
}
//In some cases there are certain enums that don't follow the same patter across an enterprise. So this

This comment has been minimized.

@evnm

evnm Sep 29, 2016

Member
  • Single space after slashes
  • s/patter/pattern/

This comment has been minimized.

@cmicali

cmicali Sep 29, 2016

Contributor

👍

return (T) constant;
}
throw new WebApplicationException(getErrorResponse(String.format("%s must be one of [%s]", parameterName, Joiner.on(", ").join(constants))));

This comment has been minimized.

@evnm

evnm Sep 29, 2016

Member

This line's pretty long. Let's break it up and move the Joiner to a static field:

final String errorMsg = String.format(
    "%s must be one of [%s]", parameterName, joiner.join(constants));
throw new WebApplicationException(getErrorResponse(errorMsg));

This comment has been minimized.

@cmicali

cmicali Sep 29, 2016

Contributor

Good idea - done 👍

@coveralls

This comment has been minimized.

coveralls commented Sep 29, 2016

Coverage Status

Changes Unknown when pulling 8b30c5a on cmicali:jersey-validation into * on dropwizard:master*.

@evnm

evnm approved these changes Sep 29, 2016

@evnm evnm merged commit c45ea5f into dropwizard:master Sep 29, 2016

@evnm

This comment has been minimized.

Member

evnm commented Sep 29, 2016

Thanks, @cmicali!

@nickbabcock

This comment has been minimized.

Contributor

nickbabcock commented Sep 29, 2016

Updated release notes in b1c563e

Thanks for the huge effort, @cmicali 😄

@cmicali

This comment has been minimized.

Contributor

cmicali commented Sep 29, 2016

Happy to contribute back, not huge at all. Thanks for all the great detailed review, the result is significantly better for it.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment