Add `RequestParams` to handle repeated query parameters by felixbarny · Pull Request #144506 · elastic/elasticsearch

felixbarny · 2026-03-18T16:38:27Z

Introduces RequestParams, a Map<String, String> view over HTTP request parameters that natively preserves multiple values per key (e.g. match[]=foo&match[]=bar).

Motivation

The existing decodeQueryString fills a Map<String, String> so repeated parameters silently drop all but the last value. This is required by #144494 which implements the Prometheus series API endpoint that relies on multiple occurrences of match[].

What changed

RequestParams (new class, replaces ad-hoc Map<String, String> usage):

Extends AbstractMap<String, String> — the standard Map interface operates on the last value for each key, preserving backwards compatibility with all existing single-value callsites
getAll(String) returns all values for a repeated parameter as a non-empty, immutable List<String> (so getFirst()/getLast() are always safe on a non-null result)
requireSingle(String) asserts that a key has exactly one value, throwing BadParameterException otherwise
put(String, String) is mutable and stores the value as a one-element immutable list, preserving the non-empty list invariant
Factory methods fromQueryString, fromUri(String), and from(URI) replace the previous RestUtils.decodeQueryString overloads and wrap parse errors as RestRequest.BadParameterException

RestRequest:

params() returns RequestParams instead of Map<String, String>
repeatedParamAsList delegates to RequestParams.getAll()

Stack:

The existing decodeQueryString fills a Map<String,String> so repeated parameters (e.g. match[]=foo&match[]=bar) silently drop all but the last value. Add decodeQueryStringMulti that returns Map<String,List<String>> and preserves every occurrence. Internally the parsing loop is extracted into a private parseQueryStringPairs helper that accepts a BiConsumer<String,String>, allowing both the single-value and multi-value variants to share the same decoding logic without duplication.

elasticsearchmachine · 2026-03-18T16:39:03Z

Pinging @elastic/es-storage-engine (Team:StorageEngine)

kkrik-es

Let's also get a review from Distributed.

elasticsearchmachine · 2026-03-23T13:27:33Z

Pinging @elastic/es-distributed (Team:Distributed)

elasticsearchmachine · 2026-03-23T15:24:29Z

Pinging @elastic/es-core-infra (Team:Core/Infra)

coderabbitai · 2026-03-23T15:28:58Z

Note

Reviews paused

It looks like this branch is under active development. To avoid overwhelming you with review comments due to an influx of new commits, CodeRabbit has automatically paused this review. You can configure this behavior by changing the reviews.auto_review.auto_pause_after_reviewed_commits setting.

Use the following commands to manage reviews:

@coderabbitai resume to resume automatic reviews.
@coderabbitai review to trigger a single review.

Use the checkboxes below for quick actions:

▶️ Resume reviews
🔍 Trigger review

📝 Walkthrough

Walkthrough

Query string parameter handling has been refactored to support multi-valued parameters (repeated parameter names). A new RequestParams class was introduced to manage query parameters, implementing Map<String, String> with an underlying LinkedHashMap<String, List<String>>. The get(key) method returns the last value for a parameter, while getAll(key) returns all values. Two RestUtils.decodeQueryString() methods were removed and replaced with a package-private variant returning the multi-value map. RestRequest now uses RequestParams instead of Map<String, String> for storing parameters, with centralized decoding via factory methods. Test fixtures and test classes were updated to use the new RequestParams API instead of manual RestUtils.decodeQueryString() calls with explicit map construction.

✨ Finishing Touches

🧪 Generate unit tests (beta)

Create PR with unit tests

🛠️ Update Documentation: Commit on current branch
🛠️ Update Documentation: Create PR

_{Comment @coderabbitai help to get the list of available commands and usage tips.}

Adds ParameterMap, a Map<String,String> that preserves multiple values per key via getAll()/getSingle(), and migrates all call sites that created a local HashMap just to pass to decodeQueryString over to the new decodeQueryStringMulti() which returns a ParameterMap directly. Also removes the now-dead decodeQueryString(URI, Map) overload.

Replaces the decodeQueryString + local HashMap pattern in both HttpRequest.Builder and HttpRequestTemplate.Builder fromUrl() with decodeQueryStringMulti, keeping all Map<String, String> signatures unchanged.

…from factory methods

Mutation methods (put, remove, clear) now throw UnsupportedOperationException. Value lists are defensively copied via List.copyOf in the constructor, and keySet/entrySet no longer expose mutable views. Tests updated accordingly.

server/src/main/java/org/elasticsearch/rest/RestUtils.java

DaveCTurner

Neat. A few interface nits but overall this seems like a good approach.

server/src/main/java/org/elasticsearch/rest/RestRequest.java

server/src/main/java/org/elasticsearch/rest/RestUtils.java

…decodeQueryString overload

…tating immutable RequestParams Add markerParams Set as an overlay for internal marker parameters (serverlessRequest, operatorRequest) so that setParamTrueOnceAndConsume no longer calls params.put() which throws UnsupportedOperationException on the immutable RequestParams. The overlay is propagated via the copy constructor and is visible through hasParam(), param(), and paramAsBoolean() (the ToXContent.Params interface).

… overlay Instead of keeping RequestParams immutable and adding overlay maps to work around callsites that legitimately mutate params (e.g. RestIndexAction, RestClusterRerouteAction, SettingsFilter, test code), restore the pre-PR mutable put() semantics on RequestParams directly. The value lists stored per key remain immutable (List.of()), so a non-null result from getAll() always guarantees getFirst()/getLast() are safe to call. The shared empty() singleton is replaced with a fresh instance per call to avoid shared-state mutation bugs. This also reverts the markerParams overlay commit: setParamTrueOnceAndConsume goes back to calling params.put() directly, and the markerParams field, copy-constructor propagation, and hasParam()/param() checks are all removed.

Catch IllegalArgumentException in RequestParams.fromQueryString and rethrow as RestRequest.BadParameterException, matching the existing pattern in requireSingle(). This makes the private static RestRequest.params(String) wrapper redundant, so remove it and call RequestParams.fromUri() directly from RestRequest.request().

…ception wrapping fromUri called decodeQueryString directly, bypassing the try/catch added in fromQueryString. Delegate to fromQueryString instead so all three parsing entry points (fromQueryString, fromUri, from(URI)) consistently wrap IllegalArgumentException as BadParameterException.

…thods

…Params AbstractMap's default implementations of these methods iterate the entry set, which failed with UnsupportedOperationException before this change. Now all three delegate directly to the backing LinkedHashMap, fixing the crash observed via RestController.clear() in integration tests.

fromSingleValues now maps null values to "" to match the previous Map<String,String> behaviour where null meant a valueless parameter. RestUtilsTests.testReservedParameters updated to expect BadParameterException (wrapping IllegalArgumentException) since fromUri now wraps all parse errors in BadParameterException.

- decodeQueryString now returns RequestParams directly, built via the new package-private addValue() method, eliminating the intermediate Map<String, List<String>> - addParam takes RequestParams instead of Map<String, List<String>> - fromQueryString no longer needs the of() wrapping step - Backing field widened to Map<String, List<String>>; copy logic moved into of() - getAll() returns Collections.unmodifiableList to prevent callers mutating the backing list - put() uses Collections.singletonList (no ArrayList needed for replacements)

DaveCTurner

Neat 😁 just a few nits about test coverage but otherwise this looks good.

server/src/test/java/org/elasticsearch/rest/RequestParamsTests.java

server/src/main/java/org/elasticsearch/rest/RequestParams.java

server/src/test/java/org/elasticsearch/rest/RestUtilsTests.java

test/framework/src/main/java/org/elasticsearch/test/rest/FakeRestRequest.java

- Cover hasNext() in entrySet iterator test - Add testEntrySetClear() for entrySet().clear() - Add testAddValue() for the package-private addValue() method - Add testFromUri() and testFrom() for the URI factory methods - Add testDecodeQueryStringMultipleValuesUnadorned() for bare repeated params

DaveCTurner

LGTM thanks Felix

The existing decodeQueryString fills a Map<String,String> so repeated parameters (e.g. match[]=foo&match[]=bar) silently drop all but the last value. Adds a RequestParams that supports multiple values for a parameter.

felixbarny added >non-issue :StorageEngine/TSDB You know, for Metrics labels Mar 18, 2026

felixbarny self-assigned this Mar 18, 2026

This was referenced Mar 18, 2026

Add PrometheusSeriesResponseListener for series endpoint #144492

Merged

Add PrometheusSeriesPlanBuilder for series endpoint #144493

Merged

Add PrometheusSeriesRestAction for /_prometheus/api/v1/series endpoint #144494

Merged

felixbarny added the v9.4.0 label Mar 18, 2026

elasticsearchmachine added Team:StorageEngine external-contributor Pull request authored by a developer outside the Elasticsearch team labels Mar 18, 2026

elasticsearchmachine and others added 2 commits March 18, 2026 16:48

[CI] Auto commit changes from spotless

09c97c8

Merge branch 'main' into prometheus-series-rest-utils

23044f7

felixbarny requested a review from kkrik-es March 23, 2026 08:01

Merge branch 'main' into prometheus-series-rest-utils

9310802

kkrik-es approved these changes Mar 23, 2026

View reviewed changes

felixbarny added the :Distributed/Network Http and internode communication implementations label Mar 23, 2026

elasticsearchmachine added the Team:Distributed Meta label for distributed team. label Mar 23, 2026

felixbarny added the :Core/Infra/REST API REST infrastructure and utilities label Mar 23, 2026

elasticsearchmachine added the Team:Core/Infra Meta label for core/infra team label Mar 23, 2026

felixbarny added 6 commits March 24, 2026 10:30

Use decodeQueryStringMulti in Watcher HttpRequest/Template fromUrl

2e10f20

Replaces the decodeQueryString + local HashMap pattern in both HttpRequest.Builder and HttpRequestTemplate.Builder fromUrl() with decodeQueryStringMulti, keeping all Map<String, String> signatures unchanged.

Remove decodeQueryString(Map) and drop Multi suffix from RestUtils

36e3c24

Move query-string parsing to ParameterMap as fromQueryString/fromUrl/…

8804194

…from factory methods

Rename ParameterMap to RequestParams

285febe

Make RequestParams immutable

0b5404b

Mutation methods (put, remove, clear) now throw UnsupportedOperationException. Value lists are defensively copied via List.copyOf in the constructor, and keySet/entrySet no longer expose mutable views. Tests updated accordingly.

sidosera reviewed Mar 24, 2026

View reviewed changes

server/src/main/java/org/elasticsearch/rest/RestUtils.java Outdated Show resolved Hide resolved

DaveCTurner reviewed Mar 24, 2026

View reviewed changes

server/src/main/java/org/elasticsearch/rest/RestRequest.java Outdated Show resolved Hide resolved

server/src/main/java/org/elasticsearch/rest/RestUtils.java Outdated Show resolved Hide resolved

server/src/main/java/org/elasticsearch/rest/RestUtils.java Outdated Show resolved Hide resolved

felixbarny added 3 commits March 24, 2026 15:03

Rename RestRequest paramAsList to repeatedParamAsList

8dd9c25

Update GCS fixture to use RequestParams.from(URI) instead of removed …

fcfefd2

…decodeQueryString overload

felixbarny requested a review from DaveCTurner March 24, 2026 17:08

felixbarny added 4 commits March 24, 2026 18:47

Add @throws BadParameterException to RequestParams parsing factory me…

aded238

…thods

felixbarny enabled auto-merge (squash) March 24, 2026 17:57

felixbarny disabled auto-merge March 24, 2026 18:00

felixbarny and others added 5 commits March 25, 2026 07:48

Use Maps.newLinkedHashMapWithExpectedSize

144a1f9

Merge branch 'main' into prometheus-series-rest-utils

2e5a3a6

felixbarny changed the title ~~Add RestUtils.decodeQueryStringMulti for repeated query parameters~~ Add RequestParams to handle repeated query parameters Mar 25, 2026

DaveCTurner reviewed Mar 27, 2026

View reviewed changes

This comment was marked as resolved.

Sign in to view

Merge branch 'main' into prometheus-series-rest-utils

0a0a7e8

felixbarny requested a review from DaveCTurner March 30, 2026 07:14

Merge branch 'main' into prometheus-series-rest-utils

18390cd

DaveCTurner approved these changes Mar 30, 2026

View reviewed changes

felixbarny merged commit 246ecb6 into elastic:main Mar 30, 2026
36 checks passed

felixbarny deleted the prometheus-series-rest-utils branch March 30, 2026 10:35

felixbarny mentioned this pull request Mar 30, 2026

Restore exception semantics for RequestParams #145212

Merged

felixbarny mentioned this pull request Mar 30, 2026

Preserve multi-value request params through PathTrie iteration #145223

Merged

Conversation

felixbarny commented Mar 18, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Motivation

What changed

Uh oh!

elasticsearchmachine commented Mar 18, 2026

Uh oh!

kkrik-es left a comment

Choose a reason for hiding this comment

Uh oh!

elasticsearchmachine commented Mar 23, 2026

Uh oh!

elasticsearchmachine commented Mar 23, 2026

Uh oh!

coderabbitai bot commented Mar 23, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Reviews paused

Walkthrough

Uh oh!

Uh oh!

DaveCTurner left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

DaveCTurner left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

This comment was marked as resolved.

Uh oh!

DaveCTurner left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

5 participants

felixbarny commented Mar 18, 2026 •

edited

Loading

coderabbitai bot commented Mar 23, 2026 •

edited

Loading