From a8c5885aff57c2e51d38c4e6cb1475d6f3ac1991 Mon Sep 17 00:00:00 2001 From: Yanming Zhou Date: Fri, 10 Jan 2025 17:30:27 +0800 Subject: [PATCH 1/3] Fix wrong document about RequestHeaderArgumentResolver Also test is added to verify that. Signed-off-by: Yanming Zhou See gh-34230 --- .../ROOT/pages/integration/rest-clients.adoc | 2 +- .../invoker/RequestHeaderArgumentResolverTests.java | 13 ++++++++++++- 2 files changed, 13 insertions(+), 2 deletions(-) diff --git a/framework-docs/modules/ROOT/pages/integration/rest-clients.adoc b/framework-docs/modules/ROOT/pages/integration/rest-clients.adoc index 482217e6d6..a1f4519032 100644 --- a/framework-docs/modules/ROOT/pages/integration/rest-clients.adoc +++ b/framework-docs/modules/ROOT/pages/integration/rest-clients.adoc @@ -940,7 +940,7 @@ method parameters: | `@RequestHeader` | Add a request header or multiple headers. The argument may be a `Map` or `MultiValueMap` with multiple headers, a `Collection` of values, or an - individual value. Type conversion is supported for non-String values. This overrides + individual value. Type conversion is supported for non-String values. This does not override the annotation's `headers` attribute. | `@PathVariable` diff --git a/spring-web/src/test/java/org/springframework/web/service/invoker/RequestHeaderArgumentResolverTests.java b/spring-web/src/test/java/org/springframework/web/service/invoker/RequestHeaderArgumentResolverTests.java index 537c2a816c..5eaa3f9af1 100644 --- a/spring-web/src/test/java/org/springframework/web/service/invoker/RequestHeaderArgumentResolverTests.java +++ b/spring-web/src/test/java/org/springframework/web/service/invoker/RequestHeaderArgumentResolverTests.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. @@ -23,6 +23,7 @@ import org.junit.jupiter.api.Test; import org.springframework.util.ObjectUtils; import org.springframework.web.bind.annotation.RequestHeader; import org.springframework.web.service.annotation.GetExchange; +import org.springframework.web.service.annotation.HttpExchange; import static org.assertj.core.api.Assertions.assertThat; @@ -32,6 +33,7 @@ import static org.assertj.core.api.Assertions.assertThat; * * @author Olga Maciaszek-Sharma * @author Rossen Stoyanchev + * @author Yanming Zhou */ class RequestHeaderArgumentResolverTests { @@ -49,6 +51,12 @@ class RequestHeaderArgumentResolverTests { assertRequestHeaders("id", "test"); } + @Test + void doesNotOverrideAnnotationHeaders() { + this.service.executeWithAnnotationHeaders("test"); + assertRequestHeaders("id", "default", "test"); + } + private void assertRequestHeaders(String key, String... values) { List actualValues = this.client.getRequestValues().getHeaders().get(key); if (ObjectUtils.isEmpty(values)) { @@ -65,6 +73,9 @@ class RequestHeaderArgumentResolverTests { @GetExchange void execute(@RequestHeader String id); + @HttpExchange(method = "GET", headers = "id=default") + void executeWithAnnotationHeaders(@RequestHeader String id); + } } From 5150a9a6ad9bc58fcc3fd721859e29b0663b5a34 Mon Sep 17 00:00:00 2001 From: rstoyanchev Date: Wed, 15 Jan 2025 18:31:25 +0000 Subject: [PATCH 2/3] Polishing contribution Closes gh-34230 --- .../modules/ROOT/pages/integration/rest-clients.adoc | 8 ++++---- .../invoker/RequestHeaderArgumentResolverTests.java | 8 ++++---- 2 files changed, 8 insertions(+), 8 deletions(-) diff --git a/framework-docs/modules/ROOT/pages/integration/rest-clients.adoc b/framework-docs/modules/ROOT/pages/integration/rest-clients.adoc index a1f4519032..ec4fd12a01 100644 --- a/framework-docs/modules/ROOT/pages/integration/rest-clients.adoc +++ b/framework-docs/modules/ROOT/pages/integration/rest-clients.adoc @@ -938,10 +938,10 @@ method parameters: | Dynamically set the HTTP method for the request, overriding the annotation's `method` attribute | `@RequestHeader` -| Add a request header or multiple headers. The argument may be a `Map` or - `MultiValueMap` with multiple headers, a `Collection` of values, or an - individual value. Type conversion is supported for non-String values. This does not override - the annotation's `headers` attribute. +| Add a request header or multiple headers. The argument may be a single value, + a `Collection` of values, `Map`,`MultiValueMap`. + Type conversion is supported for non-String values. Header values are added and + do not override already added header values. | `@PathVariable` | Add a variable for expand a placeholder in the request URL. The argument may be a diff --git a/spring-web/src/test/java/org/springframework/web/service/invoker/RequestHeaderArgumentResolverTests.java b/spring-web/src/test/java/org/springframework/web/service/invoker/RequestHeaderArgumentResolverTests.java index 5eaa3f9af1..4c19292e59 100644 --- a/spring-web/src/test/java/org/springframework/web/service/invoker/RequestHeaderArgumentResolverTests.java +++ b/spring-web/src/test/java/org/springframework/web/service/invoker/RequestHeaderArgumentResolverTests.java @@ -53,8 +53,8 @@ class RequestHeaderArgumentResolverTests { @Test void doesNotOverrideAnnotationHeaders() { - this.service.executeWithAnnotationHeaders("test"); - assertRequestHeaders("id", "default", "test"); + this.service.executeWithAnnotationHeaders("2"); + assertRequestHeaders("myHeader", "1", "2"); } private void assertRequestHeaders(String key, String... values) { @@ -73,8 +73,8 @@ class RequestHeaderArgumentResolverTests { @GetExchange void execute(@RequestHeader String id); - @HttpExchange(method = "GET", headers = "id=default") - void executeWithAnnotationHeaders(@RequestHeader String id); + @HttpExchange(method = "GET", headers = "myHeader=1") + void executeWithAnnotationHeaders(@RequestHeader String myHeader); } From 384d2749c6d06915c3da88a62c6183e7fd735af6 Mon Sep 17 00:00:00 2001 From: rstoyanchev Date: Wed, 15 Jan 2025 19:03:00 +0000 Subject: [PATCH 3/3] Polishing in HttpServiceMethod --- .../controller/ann-requestmapping.adoc | 7 +- .../service/invoker/HttpServiceMethod.java | 85 ++++++++----------- .../invoker/HttpServiceMethodTests.java | 8 +- .../RequestMappingHandlerMappingTests.java | 3 +- 4 files changed, 43 insertions(+), 60 deletions(-) diff --git a/framework-docs/modules/ROOT/pages/web/webflux/controller/ann-requestmapping.adoc b/framework-docs/modules/ROOT/pages/web/webflux/controller/ann-requestmapping.adoc index 4f24f60527..cbfcad7a7c 100644 --- a/framework-docs/modules/ROOT/pages/web/webflux/controller/ann-requestmapping.adoc +++ b/framework-docs/modules/ROOT/pages/web/webflux/controller/ann-requestmapping.adoc @@ -605,9 +605,4 @@ For method parameters and returns values, generally, `@HttpExchange` supports a subset of the method parameters that `@RequestMapping` does. Notably, it excludes any server-side specific parameter types. For details, see the list for xref:integration/rest-clients.adoc#rest-http-interface-method-parameters[@HttpExchange] and -xref:web/webflux/controller/ann-methods/arguments.adoc[@RequestMapping]. - -`@HttpExchange` also supports a `headers()` parameter which accepts `"name=value"`-like -pairs like in `@RequestMapping(headers={})` on the client side. On the server side, -this extends to the full syntax that -xref:#webflux-ann-requestmapping-params-and-headers[`@RequestMapping`] supports. +xref:web/webflux/controller/ann-methods/arguments.adoc[@RequestMapping]. \ No newline at end of file diff --git a/spring-web/src/main/java/org/springframework/web/service/invoker/HttpServiceMethod.java b/spring-web/src/main/java/org/springframework/web/service/invoker/HttpServiceMethod.java index 819667c744..eaed346dd6 100644 --- a/spring-web/src/main/java/org/springframework/web/service/invoker/HttpServiceMethod.java +++ b/spring-web/src/main/java/org/springframework/web/service/invoker/HttpServiceMethod.java @@ -22,7 +22,6 @@ import java.time.Duration; import java.util.ArrayList; import java.util.List; import java.util.Optional; -import java.util.Set; import java.util.function.Function; import java.util.function.Supplier; @@ -160,7 +159,7 @@ final class HttpServiceMethod { private record HttpRequestValuesInitializer( @Nullable HttpMethod httpMethod, @Nullable String url, @Nullable MediaType contentType, @Nullable List acceptMediaTypes, - @Nullable MultiValueMap otherHeaders, + MultiValueMap headers, Supplier requestValuesSupplier) { public HttpRequestValues.Builder initializeRequestValuesBuilder() { @@ -177,16 +176,8 @@ final class HttpServiceMethod { if (this.acceptMediaTypes != null) { requestValues.setAccept(this.acceptMediaTypes); } - if (this.otherHeaders != null) { - this.otherHeaders.forEach((name, values) -> { - if (values.size() == 1) { - requestValues.addHeader(name, values.get(0)); - } - else { - requestValues.addHeader(name, values.toArray(new String[0])); - } - }); - } + this.headers.forEach((name, values) -> + values.forEach(value -> requestValues.addHeader(name, value))); return requestValues; } @@ -217,10 +208,10 @@ final class HttpServiceMethod { String url = initUrl(typeAnnotation, methodAnnotation, embeddedValueResolver); MediaType contentType = initContentType(typeAnnotation, methodAnnotation); List acceptableMediaTypes = initAccept(typeAnnotation, methodAnnotation); - MultiValueMap headers = initHeaders(typeAnnotation, methodAnnotation, - embeddedValueResolver); - return new HttpRequestValuesInitializer(httpMethod, url, contentType, - acceptableMediaTypes, headers, requestValuesSupplier); + MultiValueMap headers = initHeaders(typeAnnotation, methodAnnotation, embeddedValueResolver); + + return new HttpRequestValuesInitializer( + httpMethod, url, contentType, acceptableMediaTypes, headers, requestValuesSupplier); } @Nullable @@ -296,48 +287,42 @@ final class HttpServiceMethod { return null; } - private static MultiValueMap parseHeaders(String[] headersArray, + private static MultiValueMap initHeaders( + @Nullable HttpExchange typeAnnotation, HttpExchange methodAnnotation, @Nullable StringValueResolver embeddedValueResolver) { + MultiValueMap headers = new LinkedMultiValueMap<>(); - for (String h: headersArray) { - String[] headerPair = StringUtils.split(h, "="); - if (headerPair != null) { - String headerName = headerPair[0].trim(); - List headerValues = new ArrayList<>(); - Set parsedValues = StringUtils.commaDelimitedListToSet(headerPair[1]); - for (String headerValue : parsedValues) { - if (embeddedValueResolver != null) { - headerValue = embeddedValueResolver.resolveStringValue(headerValue); - } - if (headerValue != null) { - headerValue = headerValue.trim(); - headerValues.add(headerValue); - } - } - if (!headerValues.isEmpty()) { - headers.addAll(headerName, headerValues); - } - } + if (typeAnnotation != null) { + addHeaders(typeAnnotation.headers(), embeddedValueResolver, headers); } + addHeaders(methodAnnotation.headers(), embeddedValueResolver, headers); return headers; } - @Nullable - private static MultiValueMap initHeaders(@Nullable HttpExchange typeAnnotation, HttpExchange methodAnnotation, - @Nullable StringValueResolver embeddedValueResolver) { - MultiValueMap methodLevelHeaders = parseHeaders(methodAnnotation.headers(), - embeddedValueResolver); - if (!ObjectUtils.isEmpty(methodLevelHeaders)) { - return methodLevelHeaders; - } + private static void addHeaders( + String[] rawValues, @Nullable StringValueResolver embeddedValueResolver, + MultiValueMap outputHeaders) { - MultiValueMap typeLevelHeaders = (typeAnnotation != null ? - parseHeaders(typeAnnotation.headers(), embeddedValueResolver) : null); - if (!ObjectUtils.isEmpty(typeLevelHeaders)) { - return typeLevelHeaders; + for (String rawValue: rawValues) { + String[] pair = StringUtils.split(rawValue, "="); + if (pair == null) { + continue; + } + String name = pair[0].trim(); + List values = new ArrayList<>(); + for (String value : StringUtils.commaDelimitedListToSet(pair[1])) { + if (embeddedValueResolver != null) { + value = embeddedValueResolver.resolveStringValue(value); + } + if (value != null) { + value = value.trim(); + values.add(value); + } + } + if (!values.isEmpty()) { + outputHeaders.addAll(name, values); + } } - - return null; } private static List getAnnotationDescriptors(AnnotatedElement element) { diff --git a/spring-web/src/test/java/org/springframework/web/service/invoker/HttpServiceMethodTests.java b/spring-web/src/test/java/org/springframework/web/service/invoker/HttpServiceMethodTests.java index 90932f856b..012d70f74c 100644 --- a/spring-web/src/test/java/org/springframework/web/service/invoker/HttpServiceMethodTests.java +++ b/spring-web/src/test/java/org/springframework/web/service/invoker/HttpServiceMethodTests.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. @@ -348,8 +348,10 @@ class HttpServiceMethodTests { @PostExchange(url = "/url", contentType = APPLICATION_JSON_VALUE, accept = APPLICATION_JSON_VALUE) void performPost(); - @HttpExchange(contentType = APPLICATION_JSON_VALUE, headers = {"CustomHeader=a,b, c", - "Content-Type=" + APPLICATION_NDJSON_VALUE}, method = "GET") + @HttpExchange( + method = "GET", + contentType = APPLICATION_JSON_VALUE, + headers = {"CustomHeader=a,b, c", "Content-Type=" + APPLICATION_NDJSON_VALUE}) void performGetWithHeaders(); } diff --git a/spring-webflux/src/test/java/org/springframework/web/reactive/result/method/annotation/RequestMappingHandlerMappingTests.java b/spring-webflux/src/test/java/org/springframework/web/reactive/result/method/annotation/RequestMappingHandlerMappingTests.java index 8e438acd3f..fc305815db 100644 --- a/spring-webflux/src/test/java/org/springframework/web/reactive/result/method/annotation/RequestMappingHandlerMappingTests.java +++ b/spring-webflux/src/test/java/org/springframework/web/reactive/result/method/annotation/RequestMappingHandlerMappingTests.java @@ -430,7 +430,8 @@ class RequestMappingHandlerMappingTests { @PostExchange(url = "/custom", contentType = "application/json", accept = "text/plain;charset=UTF-8") public void customValuesExchange(){} - @HttpExchange(method="GET", url = "/headers", + @HttpExchange( + method="GET", url = "/headers", headers = {"h1=hv1", "!h2", "Accept=application/ignored"}) public String customHeadersExchange() { return "info";