Join collection matrix variable values with a comma

spring-cloud-openfeign-core/src/main/java/org/springframework/cloud/openfeign/annotation/MatrixVariableParameterProcessor.java

@@ -18,12 +18,16 @@
 
 import java.lang.annotation.Annotation;
 import java.lang.reflect.Method;
+import java.util.Collection;
 import java.util.Map;
+import java.util.Objects;
 import java.util.stream.Collectors;
 
 import feign.MethodMetadata;
 
 import org.springframework.cloud.openfeign.AnnotatedParameterProcessor;
+import org.springframework.util.CollectionUtils;
+import org.springframework.util.StringUtils;
 import org.springframework.web.bind.annotation.MatrixVariable;
 
 import static feign.Util.checkState;
@@ -32,8 +36,11 @@
 /**
  * {@link MatrixVariable} annotation processor.
  *
- * Can expand maps or single objects. Values are assigned from the objects
- * {@code toString()} method.
+ * Can expand maps or single objects. For a {@link Map} typed variable, values are
+ * assigned from the objects {@code toString()} method. For any other type, a value that
+ * is a {@link Collection} or an array is joined with {@code ,}, which is the separator a
+ * matrix variable uses for repeated values, and nested collections and arrays are
+ * flattened the same way; any other value is assigned from its {@code toString()} method.
  *
  * @author Matt King
  * @see AnnotatedParameterProcessor
@@ -63,12 +70,42 @@ public boolean processArgument(AnnotatedParameterContext context, Annotation ann
 			data.indexToExpander().put(parameterIndex, this::expandMap);
 		}
 		else {
-			data.indexToExpander().put(parameterIndex, object -> ";" + name + "=" + object.toString());
+			data.indexToExpander().put(parameterIndex, this::expandValue);
+			prefixTemplateVariable(data, name);
 		}
 
 		return true;
 	}
 
+	/**
+	 * Moves the {@code ;name=} prefix of the matrix variable out of the expanded value
+	 * and into the URI template, so that it stays a literal. Feign always pct-encodes the
+	 * values it substitutes into a URI template, which would turn the separators into
+	 * {@code %3B} and {@code %3D} and stop the server from reading the segment as matrix
+	 * variables.
+	 */
+	private void prefixTemplateVariable(MethodMetadata data, String name) {
+		String uri = data.template().url();
+		String variable = "{" + name + "}";
+
+		if (uri.contains(variable)) {
+			data.template().uri(uri.replace(variable, ";" + name + "=" + variable));
+		}
+	}
+
+	private String expandValue(Object value) {
+		if (value.getClass().isArray()) {
+			return expandValue(CollectionUtils.arrayToList(value));
+		}
+
+		if (value instanceof Collection<?> values) {
+			return StringUtils.collectionToCommaDelimitedString(
+					values.stream().filter(Objects::nonNull).map(this::expandValue).toList());
+		}
+
+		return value.toString();
+	}
+
 	@SuppressWarnings("unchecked")
 	private String expandMap(Object object) {
 		Map<String, Object> paramMap = (Map) object;

spring-cloud-openfeign-core/src/test/java/org/springframework/cloud/openfeign/support/SpringMvcContractTests.java

@@ -34,10 +34,15 @@
 import java.util.Locale;
 import java.util.Map;
 import java.util.Objects;
+import java.util.concurrent.atomic.AtomicReference;
+import java.util.function.Consumer;
 
 import com.fasterxml.jackson.annotation.JsonAutoDetect;
+import feign.Client;
+import feign.Feign;
 import feign.MethodMetadata;
 import feign.Param;
+import feign.Response;
 import org.junit.jupiter.api.BeforeEach;
 import org.junit.jupiter.api.Test;
 import org.junit.jupiter.api.extension.ExtendWith;
@@ -802,8 +807,8 @@ void testMatrixVariable_ObjectParam() throws Exception {
 		MethodMetadata data = contract.parseAndValidateMetadata(method.getDeclaringClass(), method);
 
 		assertThat(data.template().method()).isEqualTo("GET");
-		assertThat(data.template().url()).isEqualTo("/matrixVariableObject/{param}");
-		assertThat(";param=value").isEqualTo(data.indexToExpander().get(0).expand("value"));
+		assertThat(data.template().url()).isEqualTo("/matrixVariableObject/;param={param}");
+		assertThat("value").isEqualTo(data.indexToExpander().get(0).expand("value"));
 	}
 
 	@Test
@@ -819,6 +824,58 @@ void testMatrixVariableWithNoName() throws NoSuchMethodException {
 		assertThat(";param=value").isEqualTo(data.indexToExpander().get(0).expand(testMap));
 	}
 
+	@Test
+	void testMatrixVariable_CollectionParam() throws Exception {
+		Method method = TestTemplate_MatrixVariable.class.getDeclaredMethod("matrixVariableCollection", List.class);
+		MethodMetadata data = contract.parseAndValidateMetadata(method.getDeclaringClass(), method);
+
+		assertThat(data.template().url()).isEqualTo("/matrixVariable/;colours={colours}");
+		assertThat(data.indexToExpander().get(0).expand(List.of("red", "blue"))).isEqualTo("red,blue");
+	}
+
+	@Test
+	void testMatrixVariable_ArrayParam() throws Exception {
+		Method method = TestTemplate_MatrixVariable.class.getDeclaredMethod("matrixVariableArray", String[].class);
+		MethodMetadata data = contract.parseAndValidateMetadata(method.getDeclaringClass(), method);
+
+		assertThat(data.template().url()).isEqualTo("/matrixVariable/;colours={colours}");
+		assertThat(data.indexToExpander().get(0).expand(new String[] { "red", "blue" })).isEqualTo("red,blue");
+	}
+
+	@Test
+	void testMatrixVariable_CollectionParamWithNestedCollectionAndArrayValues() throws Exception {
+		Method method = TestTemplate_MatrixVariable.class.getDeclaredMethod("matrixVariableCollection", List.class);
+		MethodMetadata data = contract.parseAndValidateMetadata(method.getDeclaringClass(), method);
+
+		assertThat(data.indexToExpander().get(0).expand(List.of(List.of("red", "blue"), new String[] { "green" })))
+			.isEqualTo("red,blue,green");
+	}
+
+	@Test
+	void testMatrixVariable_SingleParamKeepsSeparatorsInTheRequestUrl() {
+		assertThat(captureRequestUrl(api -> api.matrixVariableObject("value")))
+			.isEqualTo("http://localhost/matrixVariableObject/;param=value");
+	}
+
+	@Test
+	void testMatrixVariable_CollectionParamKeepsSeparatorsInTheRequestUrl() {
+		assertThat(captureRequestUrl(api -> api.matrixVariableCollection(List.of("red", "blue"))))
+			.isEqualTo("http://localhost/matrixVariable/;colours=red,blue");
+	}
+
+	private String captureRequestUrl(Consumer<TestTemplate_MatrixVariable> call) {
+		AtomicReference<String> url = new AtomicReference<>();
+		Client client = (request, options) -> {
+			url.set(request.url());
+			return Response.builder().status(200).request(request).body(new byte[0]).build();
+		};
+		call.accept(Feign.builder()
+			.contract(contract)
+			.client(client)
+			.target(TestTemplate_MatrixVariable.class, "http://localhost"));
+		return url.get();
+	}
+
 	@Test
 	void testAddingTemplatedParameterWithTheSameKey() throws NoSuchMethodException {
 		Method method = TestTemplate_Advanced.class.getDeclaredMethod("testAddingTemplatedParamForExistingKey",
@@ -1184,6 +1241,12 @@ public interface TestTemplate_MatrixVariable {
 		@GetMapping("/matrixVariable/{params}")
 		String matrixVariableNotNamed(@MatrixVariable Map<String, Object> params);
 
+		@GetMapping("/matrixVariable/{colours}")
+		String matrixVariableCollection(@MatrixVariable("colours") List<String> colours);
+
+		@GetMapping("/matrixVariable/{colours}")
+		String matrixVariableArray(@MatrixVariable("colours") String[] colours);
+
 	}
 
 	@JsonAutoDetect