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 index 0a7e4975ce8..3a9615fdec8 100644 --- 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 @@ public class HttpRequestValues { UriComponentsBuilder uriComponentsBuilder = UriComponentsBuilder.fromUriString(uriTemplate); for (Map.Entry> 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 index 5f026afe654..99420e82227 100644 --- 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 @@ class HttpRequestValuesTests { 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 @@ class HttpRequestValuesTests { .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 @@ class HttpRequestValuesTests { 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/spring-web/src/test/java/org/springframework/web/service/invoker/PathVariableArgumentResolverTests.java b/spring-web/src/test/java/org/springframework/web/service/invoker/PathVariableArgumentResolverTests.java index 3d5149ed629..8d7d8c01f36 100644 --- a/spring-web/src/test/java/org/springframework/web/service/invoker/PathVariableArgumentResolverTests.java +++ b/spring-web/src/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 @@ class PathVariableArgumentResolverTests { 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 @@ class PathVariableArgumentResolverTests { @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/spring-webmvc/src/main/java/org/springframework/web/servlet/mvc/method/annotation/ResponseBodyEmitter.java b/spring-webmvc/src/main/java/org/springframework/web/servlet/mvc/method/annotation/ResponseBodyEmitter.java index 845194e0b2b..f3821c133ce 100644 --- a/spring-webmvc/src/main/java/org/springframework/web/servlet/mvc/method/annotation/ResponseBodyEmitter.java +++ b/spring-webmvc/src/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.ArrayList; 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 = new AtomicReference<>(State.START); + /** Store send data before handler is initialized. */ private final Set 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 @@ public class ResponseBodyEmitter { 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 @@ public class ResponseBodyEmitter { } 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 class ResponseBodyEmitter { * @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 class ResponseBodyEmitter { * @since 6.0.12 */ public synchronized void send(Set 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 items) throws IOException { if (items.isEmpty()) { return; @@ -245,7 +247,7 @@ public class ResponseBodyEmitter { * 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,7 +264,7 @@ public class ResponseBodyEmitter { * {@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); @@ -270,6 +272,11 @@ public class ResponseBodyEmitter { } } + 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 class ResponseBodyEmitter { } - private class DefaultCallback implements Runnable { + private class TimeoutCallback implements Runnable { private final List delegates = new ArrayList<>(1); @@ -374,9 +381,10 @@ public class ResponseBodyEmitter { @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 class ResponseBodyEmitter { @Override public void accept(Throwable t) { - ResponseBodyEmitter.this.complete.compareAndSet(false, true); - for(Consumer delegate : this.delegates) { - delegate.accept(t); + if (ResponseBodyEmitter.this.state.compareAndSet(State.START, State.COMPLETE)) { + for (Consumer delegate : this.delegates) { + delegate.accept(t); + } + } + } + } + + + private class CompletionCallback implements Runnable { + + private final List 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}. + *

+	 *     START ----+
+	 *       |       |
+	 *       v       |
+	 *    TIMEOUT    |
+	 *       |       |
+	 *       v       |
+	 *   COMPLETE <--+
+	 * 
+ * @since 6.2.4 + */ + private enum State { + START, + TIMEOUT, // handling a timeout + COMPLETE + } + }