Skip to content

Support multi-value query parameters #6118

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

Jump to bottom
Merged
merged 2 commits into from
May 12, 2025
Merged

Support multi-value query parameters #6118

merged 2 commits into from
May 12, 2025

Conversation

kwondh5217
Copy link
Contributor

Motivation:

This PR is a follow-up to #6072, which added support for mapping all query parameters into a Map<String, String>. However, it does not handle cases where multiple values exist for the same query parameter key.

For example:

GET /foo?a=1&a=2&b=3

The current implementation only retains the last value a=2, losing a=1.
To better support multi-value query parameters, we introduce support for Map<String, List<?>>.

Modifications:

  • Enhanced ofQueryParamMap in AnnotatedValueResolver to support:
    • Map<String, String> → Single-value mapping (existing behavior).
    • Map<String, List> → Multi-value mapping (new behavior).
  • If the value type of the map is List<?>, query parameters with the same key are collected into a list.
  • Added validation to ensure that the value type is List<?>. If an unsupported type is used, an IllegalArgumentException is thrown.

Result:

  • Users can now use @param Map<String, List<?>> to collect multi-value query parameters.

@github-actions GitHub Actions
Copy link
Contributor

github-actions bot commented Feb 23, 2025 •
edited
Loading

🔍 Build Scan® (commit: 794ea24)

Job name Status Build Scan®
build-ubicloud-standard-16-jdk-8 ✅ https://ge.armeria.dev/s/5wlvgtnzl6qrk
build-ubicloud-standard-16-jdk-21-snapshot-blockhound ✅ https://ge.armeria.dev/s/lvgkyonytdpru
build-ubicloud-standard-16-jdk-17-min-java-17-coverage ❌ (failure) https://ge.armeria.dev/s/te4sdy3eqghmu
build-ubicloud-standard-16-jdk-17-min-java-11 ✅ https://ge.armeria.dev/s/byhm7j5pibkko
build-ubicloud-standard-16-jdk-17-leak ✅ https://ge.armeria.dev/s/vqggwd22d76ka
build-ubicloud-standard-16-jdk-11 ✅ https://ge.armeria.dev/s/a77gq5isou3ba
build-macos-latest-jdk-21 ✅ https://ge.armeria.dev/s/ub6onqxru6umy

@ikhoon ikhoon added this to the 1.33.0 milestone Feb 24, 2025
@kwondh5217 kwondh5217 requested a review from ikhoon February 24, 2025 12:19
@kwondh5217 kwondh5217 changed the title Support multi-value query parameters with @Param Map<String, List<?>> Support multi-value query parameters Feb 24, 2025
minwoox
minwoox approved these changes Apr 9, 2025
View reviewed changes
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.

Left small suggestions. Thanks!

@kwondh5217
Copy link
Contributor Author

Thanks for the suggestions, @minwoox!
I’ve pushed the updates now.

@kwondh5217 kwondh5217 force-pushed the main branch 3 times, most recently from 236cb7f to d7daa0d Compare April 9, 2025 11:43
@kwondh5217
Copy link
Contributor Author

Thanks for the suggestions, @jrhee17 !
I’ve pushed the updates now.

@kwondh5217 kwondh5217 force-pushed the main branch 3 times, most recently from 95ad3d0 to 0dff99c Compare April 16, 2025 16:24
@codecov Codecov
Copy link

codecov bot commented Apr 16, 2025 •
edited
Loading

Codecov Report

Attention: Patch coverage is 86.20690% with 4 lines in your changes missing coverage. Please review.

Project coverage is 74.64%. Comparing base (8150425) to head (690480e).
Report is 62 commits behind head on main.

Files with missing lines Patch % Lines
...rnal/server/annotation/AnnotatedValueResolver.java 86.20% 1 Missing and 3 partials ⚠️
Additional details and impacted files 
@@             Coverage Diff              @@
##               main    #6118      +/-   ##
============================================
+ Coverage     74.46%   74.64%   +0.17%     
- Complexity    22234    22480     +246     
============================================
  Files          1963     1972       +9     
  Lines         82437    83018     +581     
  Branches      10764    10805      +41     
============================================
+ Hits          61385    61967     +582     
+ Misses        15918    15901      -17     
- Partials       5134     5150      +16

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

🚀 New features to boost your workflow:
  • ❄️ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.
  • 📦 JS Bundle Analysis: Save yourself from yourself by tracking and limiting bundle sizes in JS merges.

ikhoon
ikhoon approved these changes Apr 17, 2025
View reviewed changes
Copy link
Contributor

@ikhoon ikhoon left a comment

Choose a reason for hiding this comment

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

Thanks, @kwondh5217!

@kwondh5217
Add support for multi-value query parameters
690480e 
Signed-off-by: Daeho Kwon <trewq231@naver.com>
@jrhee17 jrhee17 merged commit d4f8a23 into line:main May 12, 2025
13 of 14 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@ikhoon ikhoon ikhoon approved these changes

@jrhee17 jrhee17 jrhee17 approved these changes

@minwoox minwoox minwoox approved these changes

@trustin trustin Awaiting requested review from trustin trustin is a code owner

Assignees
No one assigned
Labels
new feature
Projects
None yet
Milestone
1.33.0
Development

Successfully merging this pull request may close these issues.
4 participants
@kwondh5217 @ikhoon @jrhee17 @minwoox