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.

Sign up for GitHub

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

Support for HTTP GET map parameters #6072

Show file tree
Hide file tree
Changes from 2 commits
Commits
Show all changes
6 commits
Select commit Hold shift + click to select a range
169c2fc
Add ofQueryParamMap method to mapping all query parameters to a Map
kwondh5217 Jan 19, 2025
e152414
Add ofQueryParamMap method to mapping all query parameters to a Map
kwondh5217 Jan 19, 2025
caee7ef
Add tests for `@Param Map<String, Object>`
kwondh5217 Jan 21, 2025
5f6abad
Add tests for `@Param Map<String, Object>`
kwondh5217 Jan 21, 2025
ffe3bb9
Add tests for `@Param Map<String, Object>`
kwondh5217 Jan 21, 2025
f3ba5a2
Fix `@Param` handling to raise exception for Map with specified value
kwondh5217 Jan 21, 2025
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
26 changes: 26 additions & 0 deletions ...src/main/java/com/linecorp/armeria/internal/server/annotation/AnnotatedValueResolver.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -48,13 +48,15 @@
import java.util.LinkedHashSet;
import java.util.List;
import java.util.Map;
import java.util.Map.Entry;
import java.util.Objects;
import java.util.Optional;
import java.util.ServiceLoader;
import java.util.Set;
import java.util.function.BiFunction;
import java.util.function.Function;
import java.util.function.Supplier;
import java.util.stream.Collectors;

import org.slf4j.Logger;
import org.slf4j.LoggerFactory;
Expand Down Expand Up @@ -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.

return ofQueryParamMap(name, annotatedElement, typeElement, type, description);
}
if (type == File.class || type == Path.class || type == MultipartFile.class) {
return ofFileParam(name, annotatedElement, typeElement, type, description);
}
Expand Down Expand Up @@ -585,6 +592,25 @@ private static AnnotatedValueResolver ofQueryParam(String name,
.build();
}

private static AnnotatedValueResolver ofQueryParamMap(String name,
AnnotatedElement annotatedElement,
AnnotatedElement typeElement, Class<?> type,
DescriptionInfo description) {

return new Builder(annotatedElement, type, name)
.annotationType(Param.class)
.typeElement(typeElement)
.description(description)
.aggregation(AggregationStrategy.FOR_FORM_DATA)
.resolver((resolver, ctx) -> ctx.queryParams().stream()
.collect(Collectors.toMap(
Entry::getKey,
Entry::getValue,
(existing, replacement) -> replacement
)))
.build();
}

private static AnnotatedValueResolver ofFileParam(String name,
AnnotatedElement annotatedElement,
AnnotatedElement typeElement, Class<?> type,
Expand Down
15 changes: 14 additions & 1 deletion ...test/java/com/linecorp/armeria/internal/server/annotation/AnnotatedValueResolverTest.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -112,6 +112,7 @@ class AnnotatedValueResolverTest {
static final ServiceRequestContext context;
static final HttpRequest request;
static final RequestHeaders originalHeaders;
static final String QUERY_PARAM_MAP = "queryParamMap";
static Map<String, AttributeKey<?>> successExpectAttrKeys;
static Map<String, AttributeKey<?>> failExpectAttrKeys;

Expand Down Expand Up @@ -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!

} else {
assertThat(value).isNull();
}
}
} else {
assertThat(resolver.defaultValue()).isNotNull();
if (QUERY_PARAM_MAP.equals(resolver.httpElementName())) {
assertThat(resolver.defaultValue()).isNull();
} else {
assertThat(resolver.defaultValue()).isNotNull();
}
if (resolver.hasContainer() && List.class.isAssignableFrom(resolver.containerType())) {
assertThat((List<Object>) value).hasSize(1)
.containsOnly(resolver.defaultValue());
assertThat(((List<Object>) value).get(0).getClass())
.isEqualTo(resolver.elementType());
} else if (resolver.shouldWrapValueAsOptional()) {
assertThat(value).isEqualTo(Optional.of(resolver.defaultValue()));
} else if (QUERY_PARAM_MAP.equals(resolver.httpElementName())) {
assertThat(value).isNotNull();
assertThat(value).isInstanceOf(Map.class);
assertThat((Map<?, ?>) value).size()
.isEqualTo(existingHttpParameters.size() + existingWithoutValueParameters.size());
} else {
assertThat(value).isEqualTo(resolver.defaultValue());
}
Expand Down Expand Up @@ -447,6 +459,7 @@ void method1(@Param String var1,
@Param @Default Integer emptyParam2,
@Param @Default List<String> emptyParam3,
@Param @Default List<Integer> emptyParam4,
@Param Map<String, Object> queryParamMap,
@Header List<String> header1,
@Header("header1") Optional<List<ValueEnum>> optionalHeader1,
@Header String header2,
Expand Down
Loading