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
Changes from 12 commits
3a39767
de3a6d7
ff0a660
63e3f4e
4013644
e7c5c65
9f8c52f
bfbb6fa
b64ef9f
a492f0a
435447d
7fa61dc
0012759
9818fad
1b179e4
8b30c5a
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,80 @@ | ||
package io.dropwizard.jersey.params; | ||
|
||
import com.google.common.base.Strings; | ||
import io.dropwizard.jersey.validation.JerseyParameterNameProvider; | ||
import org.glassfish.jersey.internal.inject.ExtractorException; | ||
import org.glassfish.jersey.server.internal.LocalizationMessages; | ||
|
||
import javax.ws.rs.ProcessingException; | ||
import javax.ws.rs.WebApplicationException; | ||
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 { | ||
|
||
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>() { | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Should this param converter be more like Jersey's default There was a problem hiding this comment. Choose a reason for hiding this commentThe 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 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? There was a problem hiding this comment. Choose a reason for hiding this commentThe 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 There was a problem hiding this comment. Choose a reason for hiding this commentThe 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 |
||
@Override | ||
@SuppressWarnings("unchecked") | ||
public T fromString(String value) { | ||
if (rawType != NonEmptyStringParam.class && Strings.isNullOrEmpty(value)) { | ||
return null; | ||
} | ||
try { | ||
return _fromString(value); | ||
} catch (InvocationTargetException ex) { | ||
final Throwable cause = ex.getCause(); | ||
if (cause instanceof WebApplicationException) { | ||
throw (WebApplicationException) cause; | ||
} else { | ||
throw new ExtractorException(cause); | ||
} | ||
} catch (final Exception ex) { | ||
throw new ProcessingException(ex); | ||
} | ||
} | ||
|
||
protected T _fromString(String value) throws Exception { | ||
return constructor.newInstance(value, parameterName); | ||
} | ||
|
||
@Override | ||
public String toString(T value) throws IllegalArgumentException { | ||
if (value == null) { | ||
throw new IllegalArgumentException(LocalizationMessages.METHOD_PARAMETER_CANNOT_BE_NULL("value")); | ||
} | ||
return value.toString(); | ||
} | ||
|
||
}; | ||
} | ||
return null; | ||
} | ||
|
||
} |
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -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> { | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe 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. There was a problem hiding this comment. Choose a reason for hiding this commentThe 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 There was a problem hiding this comment. Choose a reason for hiding this commentThe 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); | ||
} | ||
|
||
} |
This file was deleted.
There was a problem hiding this comment.
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?