Skip to content

Join collection matrix variable values with a comma - #1421

Merged
ryanjbaxter merged 5 commits into
spring-cloud:mainfrom
kdelay:fix/matrix-variable-collection-values-50x
Sep 24, 2026
Merged

ryanjbaxter merged 5 commits into
spring-cloud:mainfrom
kdelay:fix/matrix-variable-collection-values-50x

Conversation

@kdelay

@kdelay kdelay commented Sep 18, 2026 •

Copy link
Copy Markdown
Contributor

MatrixVariableParameterProcessor builds the path segment from each value's toString(), so a collection comes out bracketed: @MatrixVariable("colours") List<String> of red, blue gives ;colours=[red, blue]. A matrix variable separates repeated values with a comma, so WebUtils.parseMatrixVariables reads that as {colours=[[red, blue]]} and ;colours=red,blue as {colours=[red, blue]}. Values are now joined with a comma; arrays and nested values take the same path.

Feign also pct-encodes values substituted into a URI template. The ;name= prefix now sits in the template and stays literal: through a stub Client a String param was %3Bparam%3Dvalue, now ;param=value.

Map variables are unchanged; a collection-valued entry needs a fix in Feign's path-style expansion.

verify: 448 tests, 0 failures.

@ryanjbaxter

Copy link
Copy Markdown
Contributor

Since this changes the return format, I do not want to put this in 5.0.x but it is OK to put in main

@kdelay
kdelay force-pushed the fix/matrix-variable-collection-values-50x branch from f1b2108 to 0e58e3a Compare September 18, 2026 13:20
@kdelay
kdelay changed the base branch from 5.0.x to main September 18, 2026 13:20
@kdelay

kdelay commented Sep 18, 2026

Copy link
Copy Markdown
Contributor Author

Retargeted to main and rebased onto d4d8148 (0e58e3a). Re-ran on that base: reverting only MatrixVariableParameterProcessor leaves the two added tests failing (SpringMvcContractTests:832 and :840) with the rest of the 71 passing; with the change, ./mvnw -P spring -pl spring-cloud-openfeign-core verify gives 445 tests, 0 failures, 0 Checkstyle violations.

@ryanjbaxter

Copy link
Copy Markdown
Contributor

Please sign your commit so the DCO passes

MatrixVariableParameterProcessor built the path segment from the raw
toString() of each value, so a collection came out bracketed:
;colours=[red, blue] instead of ;colours=red,blue. A matrix variable
separates repeated values with a comma, so the receiving side reads
"[red" and " blue]" back out of the bracketed form.

Both branches of the processor were affected, including the
Map<String, List<String>> signature used as the @MatrixVariable example
in the reference documentation.

Signed-off-by: kdelay <kdelay20@gmail.com>
@kdelay
kdelay force-pushed the fix/matrix-variable-collection-values-50x branch from 0e58e3a to bd2b41f Compare September 18, 2026 13:54
@kdelay

kdelay commented Sep 18, 2026

Copy link
Copy Markdown
Contributor Author

Re-pushed the commit (bd2b41f, same tree as 0e58e3a) with the sign-off. DCO is green now.

assertThat(";param=value").isEqualTo(data.indexToExpander().get(0).expand(testMap));
}

@Test

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Can we add a test through a real Feign client and/or RestTemplate? I believe RequestTemplateFactoryResolver.expandElements() will add a , in the result as well


private String expandValue(Object value) {
if (value instanceof Collection<?> values) {
return values.stream().filter(Objects::nonNull).map(Object::toString).collect(Collectors.joining(","));

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

I think you can use org.springframework.util.StringUtils.collectionToCommaDelimitedString

Use StringUtils.collectionToCommaDelimitedString for the join and flatten arrays and nested collections through the same path.

Signed-off-by: kdelay <kdelay20@gmail.com>
@kdelay

kdelay commented Sep 18, 2026

Copy link
Copy Markdown
Contributor Author

Done in 4bb1784: join via StringUtils.collectionToCommaDelimitedString, arrays through the same path (CollectionUtils.arrayToList), so nested collections/arrays flatten too. Reverting only the processor leaves the 4 added tests failing and the other 69 passing; verify is 447 tests, 0 failures, 0 Checkstyle violations.

You are right about expandElements. With a stub Client a List param gives /matrixVariable/%3Bcolours%3Dred,%3Bcolours%3Dblue, a Map param %3Bcolours%3Dred%2Cblue%3Bsize%3DL, on main too. Should I open a separate issue for that?

@ryanjbaxter

Copy link
Copy Markdown
Contributor

You are right about expandElements. With a stub Client a List param gives /matrixVariable/%3Bcolours%3Dred,%3Bcolours%3Dblue, a Map param %3Bcolours%3Dred%2Cblue%3Bsize%3DL, on main too. Should I open a separate issue for that?

No, IMO we should deal with it as part of this change

Feign pct-encodes every value it substitutes into a URI template, so the
';' and '=' produced by the expander left the segment as %3Bname%3Dvalue and
the server could not read it as matrix variables. Move the ';name=' prefix
into the URI template, where it survives as a literal, and let the expander
produce only the value. A collection parameter is expanded element by element
by Feign and joined with ',', which now yields ';colours=red,blue'.

Signed-off-by: kdelay <kdelay20@gmail.com>
@kdelay

kdelay commented Sep 18, 2026

Copy link
Copy Markdown
Contributor Author

Done in 02fbad7. Feign pct-encodes every value substituted into a URI template, so the separators came out as %3B/%3D. The ;name= prefix now lives in the template, where it stays literal.

Measured through a stub Client (2 tests added): String was %3Bparam%3Dvalue, now ;param=value; List was %3Bcolours%3Dred,%3Bcolours%3Dblue, now ;colours=red,blue.

Arrays (not Iterable, so joined inside the value) and Map (dynamic keys) still encode; both need reserved characters inside a value, which UriTemplate forbids.

verify: 449 tests, 0 failures.

}

private String expandValue(Object value) {
if (value.getClass().isArray()) {

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

I don't think the Map-typed @MatrixVariable case case is handled properly

Let Feign expand a Map typed matrix variable through a path-style URI template expression, so the ; and = separators stay literal in the request URL and only the keys and the values are encoded.

Signed-off-by: kdelay <kdelay20@gmail.com>
@kdelay

kdelay commented Sep 19, 2026

Copy link
Copy Markdown
Contributor Author

Done in 94a9ad3: the Map variable is now a path-style expression ({;params}) expanded by Feign, so ; and = stay literal. {"colours":"red","size":"L"} was %3Bcolours%3Dred%3Bsize%3DL, now ;colours=red;size=L. verify: 449 tests, 0 failures.

Collection-valued entries are still not covered: expandMap encodes each value without recursing, so ["red","blue"] gives ;colours=%5Bred%2C%20blue%5D, and joining them myself yields red%2Cblue, a single value. The reserved-safe form is a repeated name, which Feign emits for a top-level Iterable only. I can raise that with Feign.

@ryanjbaxter

Copy link
Copy Markdown
Contributor

Shouldn't colours=[red,blue], size=L result in ;colours=red,blue;size=L? This code still seems to return ;colours=[red, blue];size=L. Could you add a test for this?

@kdelay

kdelay commented Sep 22, 2026

Copy link
Copy Markdown
Contributor Author

Measured on feign-core 13.14 through a stub Client: a literal , between repeated values only comes from Feign joining an Iterable argument itself. A Map reaches the expander as one object and Feign pct-encodes the result, so ;colours=red,blue;size=L comes out as %3Bcolours%3Dred%2Cblue%3Bsize%3DL. Path-style keeps ; and = literal but encodes each value: "red,blue" becomes red%2Cblue. indexToEncoded is never read in 13.14.

That output needs a Feign-side change to path-style expansion. Open it upstream and keep this PR to the cases it covers, or drop the map change?

@ryanjbaxter

Copy link
Copy Markdown
Contributor

Thanks for the explaination.

Lets drop the Map coverage for now and file the bug upstream in Feign.

Feign encodes each value of a path-style expression, so a collection valued
map entry cannot keep its separators. Leave map expansion as it was and keep
this change to the cases it covers.

Signed-off-by: kdelay <kdelay20@gmail.com>
@kdelay

kdelay commented Sep 22, 2026

Copy link
Copy Markdown
Contributor Author

Done in f404d40: map expansion and its tests are back to what they are on main. The change now covers single values, collections and arrays.

./mvnw -P spring -pl spring-cloud-openfeign-core verify: 448 tests, 0 failures, 0 Checkstyle violations. Reverting only the processor leaves the 6 matrix variable tests failing and the other 68 passing.

I will open the path-style expansion bug with Feign and link it here.

@ryanjbaxter ryanjbaxter added bug Something isn't working and removed waiting-for-triage labels Sep 24, 2026
@ryanjbaxter ryanjbaxter added this to the 5.1.0-RC1 milestone Sep 24, 2026
@ryanjbaxter
ryanjbaxter merged commit 5dcc821 into spring-cloud:main Sep 24, 2026
2 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

bug Something isn't working

Projects

Status: Done

Development

Successfully merging this pull request may close these issues.

3 participants