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
Handle an empty query parameter with @Default #5534
Conversation
- if the type of the parameter is primitive, throw exception - if the type of the parameter is collection, empty collection - add test cases
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.
The overall changes look nice.
@@ -86,7 +86,11 @@ class AnnotatedValueResolverTest { | |||
static final Set<String> existingHttpParameters = ImmutableSet.of("param1", | |||
"enum1", | |||
"sensitive"); | |||
|
|||
// These parameters will be present without the values. e.g., ?emptyParam1=&emptyParam2=&... | |||
static final Set<String> existingWithoutValueParameters = ImmutableSet.of("emptyParam1", |
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.
Should we also add an e2e test in which a client sends a query parameter with a value or an empty value? EmptyParameterWithDefaultAnnotationTest
could be added for that.
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.
Sure. Why not.
@@ -1026,6 +1031,17 @@ private Object convert(@Nullable String value) { | |||
if (value == null) { | |||
return defaultOrException(); | |||
} | |||
if (value.isEmpty()) { | |||
logger.warn("Parameter is present but the value is missing: " + httpElementName); |
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.
As it is an expected behavior, we can remove the warning.
Codecov ReportAll modified and coverable lines are covered by tests ✅
Additional details and impacted files@@ Coverage Diff @@
## main #5534 +/- ##
===========================================
+ Coverage 0 74.08% +74.08%
- Complexity 0 20982 +20982
===========================================
Files 0 1819 +1819
Lines 0 77315 +77315
Branches 0 9881 +9881
===========================================
+ Hits 0 57282 +57282
- Misses 0 15378 +15378
- Partials 0 4655 +4655 ☔ View full report in Codecov by Sentry. |
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.
Nice work! 🚀 I can't believe that this is your first contribution. 😄
...com/linecorp/armeria/internal/server/annotation/EmptyParameterWithDefaultAnnotationTest.java
Show resolved
Hide resolved
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.
Changes look straightforward 👍 Thanks @facewise ! 🙇 👍 🙇
...com/linecorp/armeria/internal/server/annotation/EmptyParameterWithDefaultAnnotationTest.java
Show resolved
Hide resolved
core/src/main/java/com/linecorp/armeria/server/annotation/Default.java
Outdated
Show resolved
Hide resolved
Co-authored-by: jrhee17 <guins_j@guins.org>
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.
Great! Thanks!
Motivation:
Say we have an annotated service that uses
@Default
annotation to optionally bind query parameters.It does not inject
null
if there is a parameter but no value. A NumberFormatException is raised when/list?page=
is sent as the request path.It would be better to solve this problem as follows the value of the parameter is empty.
Modifications:
""
if the type of the parameter isString
null
if the type of the parameter is notString
such asInteger
,Boolean
and so onIllegalArgumentException
if the type of the parameter is primitive type[]
if the type of the parameter isList
orSet
Result:
@Default
#5515.