Skip to content
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
Merged
Show file tree
Hide file tree
Changes from 10 commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Jump to
Jump to file
Failed to load files.
Diff view
Diff view
Expand Up @@ -8,8 +8,9 @@
import com.google.common.collect.ComparisonChain;
import com.google.common.collect.Ordering;
import io.dropwizard.jersey.caching.CacheControlledResponseFeature;
import io.dropwizard.jersey.params.NonEmptyStringParamFeature;
import io.dropwizard.jersey.params.AbstractParamConverterProvider;
import io.dropwizard.jersey.sessions.SessionFactoryProvider;
import io.dropwizard.jersey.validation.ValidatingEnumParamConverterProvider;
import org.glassfish.jersey.server.ResourceConfig;
import org.glassfish.jersey.server.ServerProperties;
import org.glassfish.jersey.server.model.Resource;
Expand Down Expand Up @@ -45,6 +46,7 @@ public DropwizardResourceConfig() {
this(true, null);
}

@SuppressWarnings("unchecked")
public DropwizardResourceConfig(boolean testOnly, MetricRegistry metricRegistry) {
super();

Expand All @@ -67,7 +69,8 @@ public DropwizardResourceConfig(boolean testOnly, MetricRegistry metricRegistry)
register(io.dropwizard.jersey.optional.OptionalIntMessageBodyWriter.class);
register(io.dropwizard.jersey.optional.OptionalLongMessageBodyWriter.class);
register(io.dropwizard.jersey.optional.OptionalParamFeature.class);
register(NonEmptyStringParamFeature.class);
register(AbstractParamConverterProvider.class);
register(new ValidatingEnumParamConverterProvider(this));
register(new SessionFactoryProvider.Binder());
}

Expand Down Expand Up @@ -95,7 +98,7 @@ public void setUrlPattern(String urlPattern) {
* @return all registered types
*/
@VisibleForTesting
Set<Class<?>> allClasses() {
public Set<Class<?>> allClasses() {
final Set<Class<?>> allClasses = new HashSet<>(getClasses());
for (Object singleton : getSingletons()) {
allClasses.add(singleton.getClass());
Expand Down
Expand Up @@ -16,15 +16,21 @@
*/
public abstract class AbstractParam<T> {
private static final Logger LOGGER = LoggerFactory.getLogger(AbstractParam.class);
private final String parameterName;
private final T value;

protected AbstractParam(String input) {
this(input, "Parameter");
}

/**
* Given an input value from a client, creates a parameter wrapping its parsed value.
*
* @param input an input value from a client request
*/
@SuppressWarnings({"AbstractMethodCallInConstructor", "OverriddenMethodCallDuringObjectConstruction"})
protected AbstractParam(String input) {
protected AbstractParam(String input, String parameterName) {
this.parameterName = parameterName;
try {
this.value = parse(input);
} catch (Exception e) {
Expand All @@ -45,9 +51,13 @@ protected AbstractParam(String input) {
*/
protected Response error(String input, Exception e) {
LOGGER.debug("Invalid input received: {}", input);
String errorMessage = errorMessage(e);
if (errorMessage.contains("%s")) {
errorMessage = String.format(errorMessage, parameterName);
}
return Response.status(getErrorStatus())
.entity(new ErrorMessage(getErrorStatus().getStatusCode(),
errorMessage(e)))
errorMessage))
.type(mediaType())
.build();
}
Expand All @@ -69,7 +79,7 @@ protected MediaType mediaType() {
* @return the error message to be sent the client
*/
protected String errorMessage(Exception e) {
return String.format("Invalid parameter: %s", e.getMessage());
return String.format("%s is invalid: %s", parameterName, e.getMessage());
}

/**
Expand Down
@@ -0,0 +1,68 @@
package io.dropwizard.jersey.params;

import com.google.common.base.Strings;
import io.dropwizard.jersey.validation.JerseyParameterNameProvider;

import javax.ws.rs.InternalServerErrorException;
import javax.ws.rs.ext.ParamConverter;
import javax.ws.rs.ext.ParamConverterProvider;
import java.lang.annotation.Annotation;
import java.lang.reflect.Constructor;
import java.lang.reflect.InvocationTargetException;
import java.lang.reflect.Type;

/**
* Provides converters to jersey for dropwizard's *Param classes.
*
* <p>When a param class is used as a resource parameter this converter will instantiate the parameter class with the
* value provided and the name of the parameter, so if value parsing fails the parameter name can be used in the error
* message. If the param class does not have a two-string constructor this provider will return null, causing jersey
* to use the single-string constructor for the parameter type as it normally would.</p>
*/
public class AbstractParamConverterProvider implements ParamConverterProvider {
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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


public AbstractParamConverterProvider() {
}

@Override
public <T> ParamConverter<T> getConverter(Class<T> rawType, Type genericType, Annotation[] annotations) {
if (AbstractParam.class.isAssignableFrom(rawType)) {
final String parameterName = JerseyParameterNameProvider.getParameterNameFromAnnotations(annotations).orElse("Parameter");
final Constructor<T> constructor;
try {
constructor = rawType.getConstructor(String.class, String.class);
} catch (NoSuchMethodException ignored) {
// The Param class did not have a (String, String) constructor. We return null,
// leaving Jersey to handle these parameters as it normally would.
return null;
}
return new ParamConverter<T>() {
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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?

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

@Override
@SuppressWarnings("unchecked")
public T fromString(String s) {
if (rawType != NonEmptyStringParam.class && Strings.isNullOrEmpty(s)) {
return null;
}
try {
return constructor.newInstance(s, parameterName);
} catch (InstantiationException | IllegalAccessException e) {
throw new InternalServerErrorException(String.format("Unable to convert parameter %s: %s", parameterName, e.getMessage()));
} catch (InvocationTargetException e) {
Throwable t = e.getTargetException();
if (t instanceof RuntimeException) {
throw (RuntimeException) t;
}
return null;
}
}

@Override
public String toString(T t) {
return t == null ? null : t.toString();
}
};
}
return null;
}

}
Expand Up @@ -11,9 +11,13 @@ public BooleanParam(String input) {
super(input);
}

public BooleanParam(String input, String parameterName) {
super(input, parameterName);
}

@Override
protected String errorMessage(Exception e) {
return "Parameter must be \"true\" or \"false\".";
return "%s must be \"true\" or \"false\".";
}

@Override
Expand Down
Expand Up @@ -12,6 +12,10 @@ public DateTimeParam(String input) {
super(input);
}

public DateTimeParam(String input, String parameterName) {
super(input, parameterName);
}

@Override
protected DateTime parse(String input) throws Exception {
return new DateTime(input, DateTimeZone.UTC);
Expand Down
@@ -0,0 +1,29 @@
package io.dropwizard.jersey.params;

import io.dropwizard.util.Duration;

/**
* A parameter encapsulating duration values. All non-parsable values will return a {@code 400 Bad
* Request} response. Supports all input formats the {@link Duration} class supports.
*/
public class DurationParam extends AbstractParam<Duration> {
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Done - updated description and added test.


public DurationParam(String input) {
super(input);
}

public DurationParam(String input, String parameterName) {
super(input, parameterName);
}

@Override
protected String errorMessage(Exception e) {
return "%s is not a valid duration.";
}

@Override
protected Duration parse(String input) throws Exception {
return Duration.parse(input);
}

}
Expand Up @@ -9,9 +9,13 @@ public IntParam(String input) {
super(input);
}

public IntParam(String input, String parameterName) {
super(input, parameterName);
}

@Override
protected String errorMessage(Exception e) {
return "Parameter is not a number.";
return "%s is not a number.";
}

@Override
Expand Down
Expand Up @@ -11,6 +11,10 @@ public LocalDateParam(String input) {
super(input);
}

public LocalDateParam(String input, String parameterName) {
super(input, parameterName);
}

@Override
protected LocalDate parse(String input) throws Exception {
return new LocalDate(input);
Expand Down
Expand Up @@ -9,9 +9,13 @@ public LongParam(String input) {
super(input);
}

public LongParam(String input, String parameterName) {
super(input, parameterName);
}

@Override
protected String errorMessage(Exception e) {
return "Parameter is not a number.";
return "%s is not a number.";
}

@Override
Expand Down
Expand Up @@ -15,6 +15,10 @@ public NonEmptyStringParam(String input) {
super(input);
}

public NonEmptyStringParam(String input, String parameterName) {
super(input, parameterName);
}

@Override
protected Optional<String> parse(String input) throws Exception {
return Optional.ofNullable(Strings.emptyToNull(input));
Expand Down

This file was deleted.

Expand Up @@ -12,9 +12,13 @@ public UUIDParam(String input) {
super(input);
}

public UUIDParam(String input, String parameterName) {
super(input, parameterName);
}

@Override
protected String errorMessage(Exception e) {
return "Parameter is not a UUID.";
return "%s is not a UUID.";
}

@Override
Expand Down
Expand Up @@ -16,16 +16,7 @@
import javax.validation.ElementKind;
import javax.validation.Path;
import javax.validation.metadata.ConstraintDescriptor;
import javax.ws.rs.CookieParam;
import javax.ws.rs.FormParam;
import javax.ws.rs.HeaderParam;
import javax.ws.rs.MatrixParam;
import javax.ws.rs.PathParam;
import javax.ws.rs.QueryParam;
import javax.ws.rs.core.Context;
import java.lang.annotation.Annotation;
import java.lang.reflect.Field;
import java.lang.reflect.Method;
import java.util.List;
import java.util.Optional;
import java.util.Set;
Expand Down Expand Up @@ -133,19 +124,13 @@ private static Optional<String> getMemberName(ConstraintViolation<?> violation,
// Extract the failing *Param annotation inside the Bean Param
if (param.getSource().equals(Parameter.Source.BEAN_PARAM)) {
final Field field = FieldUtils.getField(param.getRawType(), member.getName(), true);
return getMemberName(field.getDeclaredAnnotations());
return JerseyParameterNameProvider.getParameterNameFromAnnotations(field.getDeclaredAnnotations());
}

return Optional.empty();
break;
case METHOD:
// Constraint violation occurred directly on a function
// parameter annotated with *Param
final Method method = invocable.getHandlingMethod();
final int paramIndex = member.as(Path.ParameterNode.class).getParameterIndex();
return getMemberName(method.getParameterAnnotations()[paramIndex]);
default:
return Optional.empty();
return Optional.of(member.getName());
}
return Optional.empty();
}

/**
Expand All @@ -166,31 +151,6 @@ private static Optional<String> getMethodReturnValueName(ConstraintViolation<?>
return returnValueNames >= 0 ? Optional.of(result.toString()) : Optional.empty();
}

/**
* Derives member's name and type from it's annotations
*/
private static Optional<String> getMemberName(Annotation[] memberAnnotations) {
for (Annotation a : memberAnnotations) {
if (a instanceof QueryParam) {
return Optional.of("query param " + ((QueryParam) a).value());
} else if (a instanceof PathParam) {
return Optional.of("path param " + ((PathParam) a).value());
} else if (a instanceof HeaderParam) {
return Optional.of("header " + ((HeaderParam) a).value());
} else if (a instanceof CookieParam) {
return Optional.of("cookie " + ((CookieParam) a).value());
} else if (a instanceof FormParam) {
return Optional.of("form field " + ((FormParam) a).value());
} else if (a instanceof Context) {
return Optional.of("context");
} else if (a instanceof MatrixParam) {
return Optional.of("matrix param " + ((MatrixParam) a).value());
}
}

return Optional.empty();
}

private static boolean isValidationMethod(ConstraintViolation<?> v) {
return v.getConstraintDescriptor().getAnnotation() instanceof ValidationMethod;
}
Expand Down