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

Support for HTTP GET map parameters #6072

Open
wants to merge 6 commits into
base: main
Choose a base branch
from
Open

Conversation

kwondh5217
Copy link

@kwondh5217 kwondh5217 commented Jan 19, 2025

Motivation:

This pull request addresses #6058

Currently, @Param cannot be mapped to a Map, but this change enables that functionality, allowing query parameters to be handled as a Map.

Modifications:

  • Introduced logic in AnnotatedValueResolver to detect when a parameter is of type Map and the @Param annotation has an unspecified value.
    • Future enhancements may include supporting the mapping of specific values to a Map when the @Param value is explicitly specified.
  • Added a new method ofQueryParamMap to handle the creation of an AnnotatedValueResolver for mapping all query parameters into a Map.
    • Query parameters are collected into a Map using the stream-based collector.
  • Updated the existing logic to ensure this behavior only applies when the @Param value is unspecified and the parameter type is Map.

Result:

Copy link
Contributor

@minwoox minwoox left a comment

Choose a reason for hiding this comment

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

HI, @kwondh5217, thanks for the PR. 😉
Would you add an e2e test that verifies if this change works?

GET http://127.0.0.1:xxx/map?a=b&c=d

@Get("/map")
public HttpResponse map(@Params Map<String, Object> map) {...}

@kwondh5217
Copy link
Author

@minwoox Thanks for your comment!
I've added a test in AnnotatedServiceTest to verify that this change works.
Please take a look when you have time! 🙇

@minwoox minwoox added this to the 1.32.0 milestone Jan 21, 2025
Copy link
Contributor

@minwoox minwoox left a comment

Choose a reason for hiding this comment

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

Basically looks good. Left just nits. 👍

@@ -358,19 +359,30 @@ private static void testResolver(AnnotatedValueResolver resolver) {
} else {
if (String.class.isAssignableFrom(resolver.elementType())) {
assertThat(value).isEqualTo("");
} else if (QUERY_PARAM_MAP.equals(resolver.httpElementName())) {
assertThat(value).isNotNull();
Copy link
Contributor

Choose a reason for hiding this comment

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

It seems like this line of code is not invoked. Would you explain why you've added this?

Copy link
Author

Choose a reason for hiding this comment

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

@minwoox Thank you for pointing this out! 🙇
This line was added by mistake and isn't necessary. I've removed it now. Appreciate your attention to detail!

@@ -458,6 +460,11 @@ private static AnnotatedValueResolver of(AnnotatedElement annotatedElement,
final Param param = annotatedElement.getAnnotation(Param.class);
if (param != null) {
final String name = findName(typeElement, param.value());
// If the parameter is of type Map and the @Param annotation does not specify a value,
// map all query parameters into the Map.
if (Map.class.isAssignableFrom(type) && DefaultValues.isUnspecified(param.value())) {
Copy link
Contributor

Choose a reason for hiding this comment

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

How about raising an exception if the value is specified?

if (Map.class.isAssignableFrom(type)) {
    if (DefaultValues.isSpecified(param.value())) {
        // throw an exception...
    }
    return ofQueryParamMap(name, annotatedElement, typeElement, type, description);
}

Copy link
Author

Choose a reason for hiding this comment

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

@minwoox Thank you for the suggestion! 🙇
I've updated the code to raise an exception when a value is specified for a Map parameter and added the test cases. Please take a look when you have time.

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

Successfully merging this pull request may close these issues.

Support for HTTP GET map parameters?
2 participants