Merge branch '6.2.x'

spring-projects · Feb 27, 2025 · 33fef8d · 33fef8d
2 parents 8a7d20f + f62251a
commit 33fef8d
Show file tree

Hide file tree

Showing 4 changed files with 105 additions and 35 deletions.
diff --git a/spring-web/src/main/java/org/springframework/web/service/invoker/HttpRequestValues.java b/spring-web/src/main/java/org/springframework/web/service/invoker/HttpRequestValues.java
@@ -465,7 +465,7 @@ private String appendQueryParams(
 
 			UriComponentsBuilder uriComponentsBuilder = UriComponentsBuilder.fromUriString(uriTemplate);
 			for (Map.Entry<String, List<String>> entry : requestParams.entrySet()) {
-				String nameVar = entry.getKey().replace(":", "%3A"); // suppress treatment as regex
+				String nameVar = "queryParam-" + entry.getKey().replace(":", "%3A"); // suppress treatment as regex
 				uriVars.put(nameVar, entry.getKey());
 				for (int j = 0; j < entry.getValue().size(); j++) {
 					String valueVar = nameVar + "[" + j + "]";

diff --git a/spring-web/src/test/java/org/springframework/web/service/invoker/HttpRequestValuesTests.java b/spring-web/src/test/java/org/springframework/web/service/invoker/HttpRequestValuesTests.java
@@ -79,17 +79,19 @@ void queryParamsWithUriTemplate() {
 
 		assertThat(uriTemplate)
 				.isEqualTo("/path?" +
-						"{param1}={param1[0]}&" +
-						"{param2}={param2[0]}&" +
-						"{param2}={param2[1]}");
+						"{queryParam-param1}={queryParam-param1[0]}&" +
+						"{queryParam-param2}={queryParam-param2[0]}&" +
+						"{queryParam-param2}={queryParam-param2[1]}");
 
 		assertThat(requestValues.getUriVariables())
-				.containsOnlyKeys("param1", "param2", "param1[0]", "param2[0]", "param2[1]")
-				.containsEntry("param1", "param1")
-				.containsEntry("param2", "param2")
-				.containsEntry("param1[0]", "1st value")
-				.containsEntry("param2[0]", "2nd value A")
-				.containsEntry("param2[1]", "2nd value B");
+				.containsOnlyKeys(
+						"queryParam-param1", "queryParam-param2", "queryParam-param1[0]",
+						"queryParam-param2[0]", "queryParam-param2[1]")
+				.containsEntry("queryParam-param1", "param1")
+				.containsEntry("queryParam-param2", "param2")
+				.containsEntry("queryParam-param1[0]", "1st value")
+				.containsEntry("queryParam-param2[0]", "2nd value A")
+				.containsEntry("queryParam-param2[1]", "2nd value B");
 
 		URI uri = UriComponentsBuilder.fromUriString(uriTemplate)
 				.encode()
@@ -107,7 +109,7 @@ void queryParamWithSemicolon() {
 				.build();
 
 		String uriTemplate = requestValues.getUriTemplate();
-		assertThat(uriTemplate).isEqualTo("/path?{userId%3Aeq}={userId%3Aeq[0]}");
+		assertThat(uriTemplate).isEqualTo("/path?{queryParam-userId%3Aeq}={queryParam-userId%3Aeq[0]}");
 
 		URI uri = UriComponentsBuilder.fromUriString(uriTemplate)
 				.encode()
@@ -162,7 +164,7 @@ void requestPartAndRequestParam() {
 		String uriTemplate = requestValues.getUriTemplate();
 		assertThat(uriTemplate).isNotNull();
 
-		assertThat(uriTemplate).isEqualTo("/path?{query param}={query param[0]}");
+		assertThat(uriTemplate).isEqualTo("/path?{queryParam-query param}={queryParam-query param[0]}");
 
 		URI uri = UriComponentsBuilder.fromUriString(uriTemplate)
 				.encode()

diff --git a/.../test/java/org/springframework/web/service/invoker/PathVariableArgumentResolverTests.java b/.../test/java/org/springframework/web/service/invoker/PathVariableArgumentResolverTests.java
@@ -1,5 +1,5 @@
 /*
- * Copyright 2002-2023 the original author or authors.
+ * Copyright 2002-2025 the original author or authors.
  *
  * Licensed under the Apache License, Version 2.0 (the "License");
  * you may not use this file except in compliance with the License.
@@ -16,11 +16,15 @@
 
 package org.springframework.web.service.invoker;
 
+import java.net.URI;
+
 import org.jspecify.annotations.Nullable;
 import org.junit.jupiter.api.Test;
 
 import org.springframework.web.bind.annotation.PathVariable;
+import org.springframework.web.bind.annotation.RequestParam;
 import org.springframework.web.service.annotation.GetExchange;
+import org.springframework.web.util.DefaultUriBuilderFactory;
 
 import static org.assertj.core.api.Assertions.assertThat;
 
@@ -46,6 +50,17 @@ void pathVariable() {
 		assertPathVariable("id", "test");
 	}
 
+	@Test // gh-34499
+	void pathVariableAndRequestParamWithSameName() {
+		this.service.executeWithPathVarAndRequestParam("{transfer-id}", "aValue");
+
+		assertPathVariable("transfer-id", "{transfer-id}");
+
+		HttpRequestValues values = this.client.getRequestValues();
+		URI uri = (new DefaultUriBuilderFactory()).expand(values.getUriTemplate(), values.getUriVariables());
+		assertThat(uri.toString()).isEqualTo("/transfers/%7Btransfer-id%7D?transfer-id=aValue");
+	}
+
 	@SuppressWarnings("SameParameterValue")
 	private void assertPathVariable(String name, @Nullable String expectedValue) {
 		assertThat(this.client.getRequestValues().getUriVariables().get(name)).isEqualTo(expectedValue);
@@ -57,6 +72,11 @@ private interface Service {
 		@GetExchange
 		void execute(@PathVariable String id);
 
+		@GetExchange("/transfers/{transfer-id}")
+		void executeWithPathVarAndRequestParam(
+				@PathVariable("transfer-id") String transferId,
+				@RequestParam("transfer-id") String transferIdParam);
+
 	}
 
 }
diff --git a/.../main/java/org/springframework/web/servlet/mvc/method/annotation/ResponseBodyEmitter.java b/.../main/java/org/springframework/web/servlet/mvc/method/annotation/ResponseBodyEmitter.java
@@ -1,5 +1,5 @@
 /*
- * Copyright 2002-2024 the original author or authors.
+ * Copyright 2002-2025 the original author or authors.
  *
  * Licensed under the Apache License, Version 2.0 (the "License");
  * you may not use this file except in compliance with the License.
@@ -21,7 +21,7 @@
 import java.util.LinkedHashSet;
 import java.util.List;
 import java.util.Set;
-import java.util.concurrent.atomic.AtomicBoolean;
+import java.util.concurrent.atomic.AtomicReference;
 import java.util.function.Consumer;
 
 import org.jspecify.annotations.Nullable;
@@ -72,20 +72,19 @@ public class ResponseBodyEmitter {
 
 	private @Nullable Handler handler;
 
+	private final AtomicReference<State> state = new AtomicReference<>(State.START);
+
 	/** Store send data before handler is initialized. */
 	private final Set<DataWithMediaType> earlySendAttempts = new LinkedHashSet<>(8);
 
-	/** Store successful completion before the handler is initialized. */
-	private final AtomicBoolean complete = new AtomicBoolean();
-
 	/** Store an error before the handler is initialized. */
 	private @Nullable Throwable failure;
 
-	private final DefaultCallback timeoutCallback = new DefaultCallback();
+	private final TimeoutCallback timeoutCallback = new TimeoutCallback();
 
 	private final ErrorCallback errorCallback = new ErrorCallback();
 
-	private final DefaultCallback completionCallback = new DefaultCallback();
+	private final CompletionCallback completionCallback = new CompletionCallback();
 
 
 	/**
@@ -125,7 +124,7 @@ synchronized void initialize(Handler handler) throws IOException {
 			this.earlySendAttempts.clear();
 		}
 
-		if (this.complete.get()) {
+		if (this.state.get() == State.COMPLETE) {
 			if (this.failure != null) {
 				this.handler.completeWithError(this.failure);
 			}
@@ -141,7 +140,7 @@ synchronized void initialize(Handler handler) throws IOException {
 	}
 
 	void initializeWithError(Throwable ex) {
-		if (this.complete.compareAndSet(false, true)) {
+		if (this.state.compareAndSet(State.START, State.COMPLETE)) {
 			this.failure = ex;
 			this.earlySendAttempts.clear();
 			this.errorCallback.accept(ex);
@@ -183,8 +182,7 @@ public void send(Object object) throws IOException {
 	 * @throws java.lang.IllegalStateException wraps any other errors
 	 */
 	public synchronized void send(Object object, @Nullable MediaType mediaType) throws IOException {
-		Assert.state(!this.complete.get(), () -> "ResponseBodyEmitter has already completed" +
-				(this.failure != null ? " with error: " + this.failure : ""));
+		assertNotComplete();
 		if (this.handler != null) {
 			try {
 				this.handler.send(object, mediaType);
@@ -211,11 +209,15 @@ public synchronized void send(Object object, @Nullable MediaType mediaType) thro
 	 * @since 6.0.12
 	 */
 	public synchronized void send(Set<DataWithMediaType> items) throws IOException {
-		Assert.state(!this.complete.get(), () -> "ResponseBodyEmitter has already completed" +
-				(this.failure != null ? " with error: " + this.failure : ""));
+		assertNotComplete();
 		sendInternal(items);
 	}
 
+	private void assertNotComplete() {
+		Assert.state(this.state.get() == State.START, () -> "ResponseBodyEmitter has already completed" +
+				(this.failure != null ? " with error: " + this.failure : ""));
+	}
+
 	private void sendInternal(Set<DataWithMediaType> items) throws IOException {
 		if (items.isEmpty()) {
 			return;
@@ -245,7 +247,7 @@ private void sendInternal(Set<DataWithMediaType> items) throws IOException {
 	 * related events such as an error while {@link #send(Object) sending}.
 	 */
 	public void complete() {
-		if (this.complete.compareAndSet(false, true) && this.handler != null) {
+		if (trySetComplete() && this.handler != null) {
 			this.handler.complete();
 		}
 	}
@@ -262,14 +264,19 @@ public void complete() {
 	 * {@link #send(Object) sending}.
 	 */
 	public void completeWithError(Throwable ex) {
-		if (this.complete.compareAndSet(false, true)) {
+		if (trySetComplete()) {
 			this.failure = ex;
 			if (this.handler != null) {
 				this.handler.completeWithError(ex);
 			}
 		}
 	}
 
+	private boolean trySetComplete() {
+		return (this.state.compareAndSet(State.START, State.COMPLETE) ||
+				(this.state.compareAndSet(State.TIMEOUT, State.COMPLETE)));
+	}
+
 	/**
 	 * Register code to invoke when the async request times out. This method is
 	 * called from a container thread when an async request times out.
@@ -364,7 +371,7 @@ public Object getData() {
 	}
 
 
-	private class DefaultCallback implements Runnable {
+	private class TimeoutCallback implements Runnable {
 
 		private final List<Runnable> delegates = new ArrayList<>(1);
 
@@ -374,9 +381,10 @@ public synchronized void addDelegate(Runnable delegate) {
 
 		@Override
 		public void run() {
-			ResponseBodyEmitter.this.complete.compareAndSet(false, true);
-			for (Runnable delegate : this.delegates) {
-				delegate.run();
+			if (ResponseBodyEmitter.this.state.compareAndSet(State.START, State.TIMEOUT)) {
+				for (Runnable delegate : this.delegates) {
+					delegate.run();
+				}
 			}
 		}
 	}
@@ -392,11 +400,51 @@ public synchronized void addDelegate(Consumer<Throwable> callback) {
 
 		@Override
 		public void accept(Throwable t) {
-			ResponseBodyEmitter.this.complete.compareAndSet(false, true);
-			for(Consumer<Throwable> delegate : this.delegates) {
-				delegate.accept(t);
+			if (ResponseBodyEmitter.this.state.compareAndSet(State.START, State.COMPLETE)) {
+				for (Consumer<Throwable> delegate : this.delegates) {
+					delegate.accept(t);
+				}
+			}
+		}
+	}
+
+
+	private class CompletionCallback implements Runnable {
+
+		private final List<Runnable> delegates = new ArrayList<>(1);
+
+		public synchronized void addDelegate(Runnable delegate) {
+			this.delegates.add(delegate);
+		}
+
+		@Override
+		public void run() {
+			if (ResponseBodyEmitter.this.state.compareAndSet(State.START, State.COMPLETE)) {
+				for (Runnable delegate : this.delegates) {
+					delegate.run();
+				}
 			}
 		}
 	}
 
+
+	/**
+	 * Represents a state for {@link ResponseBodyEmitter}.
+	 * <p><pre>
+	 *     START ----+
+	 *       |       |
+	 *       v       |
+	 *    TIMEOUT    |
+	 *       |       |
+	 *       v       |
+	 *   COMPLETE <--+
+	 * </pre>
+	 * @since 6.2.4
+	 */
+	private enum State {
+		START,
+		TIMEOUT, // handling a timeout
+		COMPLETE
+	}
+
 }