From 6691ff207244ef46cdf18e16df17f3cd19c9ce2a Mon Sep 17 00:00:00 2001 From: Sam Brannen <104798+sbrannen@users.noreply.github.com> Date: Thu, 18 Jan 2024 18:25:12 +0100 Subject: [PATCH] =?UTF-8?q?Reject=20multiple=20@=E2=81=A0HttpExchange=20de?= =?UTF-8?q?clarations=20in=20HTTP=20interface=20clients?= MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit This commit updates HttpServiceMethod so that multiple @⁠HttpExchange declarations on the same element (class or method) are rejected. Closes gh-32049 --- .../service/invoker/HttpServiceMethod.java | 63 +++++++++++++++++-- .../invoker/HttpServiceMethodTests.java | 60 ++++++++++++++++++ 2 files changed, 118 insertions(+), 5 deletions(-) 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 d31bd387b9a..f715ba50fea 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 @@ -16,6 +16,7 @@ package org.springframework.web.service.invoker; +import java.lang.reflect.AnnotatedElement; import java.lang.reflect.Method; import java.time.Duration; import java.util.List; @@ -32,7 +33,11 @@ import org.springframework.core.KotlinDetector; import org.springframework.core.MethodParameter; import org.springframework.core.ParameterizedTypeReference; import org.springframework.core.ReactiveAdapter; -import org.springframework.core.annotation.AnnotatedElementUtils; +import org.springframework.core.annotation.MergedAnnotation; +import org.springframework.core.annotation.MergedAnnotationPredicates; +import org.springframework.core.annotation.MergedAnnotations; +import org.springframework.core.annotation.MergedAnnotations.SearchStrategy; +import org.springframework.core.annotation.RepeatableContainers; import org.springframework.core.annotation.SynthesizingMethodParameter; import org.springframework.http.HttpHeaders; import org.springframework.http.HttpMethod; @@ -54,6 +59,7 @@ import org.springframework.web.service.annotation.HttpExchange; * @author Rossen Stoyanchev * @author Sebastien Deleuze * @author Olga Maciaszek-Sharma + * @author Sam Brannen * @since 6.0 */ final class HttpServiceMethod { @@ -145,7 +151,7 @@ final class HttpServiceMethod { /** * Factory for {@link HttpRequestValues} with values extracted from the type - * and method-level {@link HttpExchange @HttpRequest} annotations. + * and method-level {@link HttpExchange @HttpExchange} annotations. */ private record HttpRequestValuesInitializer( @Nullable HttpMethod httpMethod, @Nullable String url, @@ -177,10 +183,20 @@ final class HttpServiceMethod { Method method, Class containingClass, @Nullable StringValueResolver embeddedValueResolver, Supplier requestValuesSupplier) { - HttpExchange typeAnnotation = AnnotatedElementUtils.findMergedAnnotation(containingClass, HttpExchange.class); - HttpExchange methodAnnotation = AnnotatedElementUtils.findMergedAnnotation(method, HttpExchange.class); + List methodHttpExchanges = getAnnotationDescriptors(method); + Assert.state(!methodHttpExchanges.isEmpty(), + () -> "Expected @HttpExchange annotation on method " + method); + Assert.state(methodHttpExchanges.size() == 1, + () -> "Multiple @HttpExchange annotations found on method %s, but only one is allowed: %s" + .formatted(method, methodHttpExchanges)); - Assert.notNull(methodAnnotation, () -> "Expected @HttpRequest annotation on method " + method.toGenericString()); + List typeHttpExchanges = getAnnotationDescriptors(containingClass); + Assert.state(typeHttpExchanges.size() <= 1, + () -> "Multiple @HttpExchange annotations found on %s, but only one is allowed: %s" + .formatted(containingClass, typeHttpExchanges)); + + HttpExchange methodAnnotation = methodHttpExchanges.get(0).httpExchange; + HttpExchange typeAnnotation = (!typeHttpExchanges.isEmpty() ? typeHttpExchanges.get(0).httpExchange : null); HttpMethod httpMethod = initHttpMethod(typeAnnotation, methodAnnotation); String url = initUrl(typeAnnotation, methodAnnotation, embeddedValueResolver); @@ -262,6 +278,43 @@ final class HttpServiceMethod { return null; } + + private static List getAnnotationDescriptors(AnnotatedElement element) { + return MergedAnnotations.from(element, SearchStrategy.TYPE_HIERARCHY, RepeatableContainers.none()) + .stream(HttpExchange.class) + .filter(MergedAnnotationPredicates.firstRunOf(MergedAnnotation::getAggregateIndex)) + .map(AnnotationDescriptor::new) + .distinct() + .toList(); + } + + + private static class AnnotationDescriptor { + + private final HttpExchange httpExchange; + private final MergedAnnotation root; + + AnnotationDescriptor(MergedAnnotation mergedAnnotation) { + this.httpExchange = mergedAnnotation.synthesize(); + this.root = mergedAnnotation.getRoot(); + } + + @Override + public boolean equals(Object obj) { + return (obj instanceof AnnotationDescriptor that && this.httpExchange.equals(that.httpExchange)); + } + + @Override + public int hashCode() { + return this.httpExchange.hashCode(); + } + + @Override + public String toString() { + return this.root.synthesize().toString(); + } + } + } 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 dc9851027bf..05f0e84e889 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 @@ -16,6 +16,11 @@ package org.springframework.web.service.invoker; +import java.lang.annotation.ElementType; +import java.lang.annotation.Retention; +import java.lang.annotation.RetentionPolicy; +import java.lang.annotation.Target; +import java.lang.reflect.Method; import java.util.List; import java.util.Optional; @@ -37,8 +42,10 @@ import org.springframework.lang.Nullable; import org.springframework.web.service.annotation.GetExchange; import org.springframework.web.service.annotation.HttpExchange; import org.springframework.web.service.annotation.PostExchange; +import org.springframework.web.service.annotation.PutExchange; import static org.assertj.core.api.Assertions.assertThat; +import static org.assertj.core.api.Assertions.assertThatIllegalStateException; import static org.springframework.http.MediaType.APPLICATION_CBOR_VALUE; import static org.springframework.http.MediaType.APPLICATION_JSON_VALUE; @@ -52,6 +59,7 @@ import static org.springframework.http.MediaType.APPLICATION_JSON_VALUE; * * @author Rossen Stoyanchev * @author Olga Maciaszek-Sharma + * @author Sam Brannen */ class HttpServiceMethodTests { @@ -204,6 +212,33 @@ class HttpServiceMethodTests { assertThat(requestValues.getHeaders().getAccept()).containsExactly(MediaType.APPLICATION_JSON); } + @Test // gh-32049 + void multipleAnnotationsAtClassLevel() { + Class serviceInterface = MultipleClassLevelAnnotationsService.class; + + assertThatIllegalStateException() + .isThrownBy(() -> this.proxyFactory.createClient(serviceInterface)) + .withMessageContainingAll( + "Multiple @HttpExchange annotations found on " + serviceInterface, + "@" + HttpExchange.class.getName(), + "@" + ExtraHttpExchange.class.getName() + ); + } + + @Test // gh-32049 + void multipleAnnotationsAtMethodLevel() throws NoSuchMethodException { + Class serviceInterface = MultipleMethodLevelAnnotationsService.class; + Method method = serviceInterface.getMethod("post"); + + assertThatIllegalStateException() + .isThrownBy(() -> this.proxyFactory.createClient(serviceInterface)) + .withMessageContainingAll( + "Multiple @HttpExchange annotations found on method " + method, + "@" + PostExchange.class.getName(), + "@" + PutExchange.class.getName() + ); + } + protected void verifyReactorClientInvocation(String methodName, @Nullable ParameterizedTypeReference expectedBodyType) { assertThat(this.reactorClient.getInvokedMethodName()).isEqualTo(methodName); assertThat(this.reactorClient.getBodyType()).isEqualTo(expectedBodyType); @@ -305,9 +340,34 @@ class HttpServiceMethodTests { } + @SuppressWarnings("unused") @HttpExchange(url = "${baseUrl}", contentType = APPLICATION_CBOR_VALUE, accept = APPLICATION_CBOR_VALUE) private interface TypeAndMethodLevelAnnotatedService extends MethodLevelAnnotatedService { } + + @HttpExchange("/exchange") + @ExtraHttpExchange + private interface MultipleClassLevelAnnotationsService { + + @PostExchange("/post") + void post(); + } + + + private interface MultipleMethodLevelAnnotationsService { + + @PostExchange("/post") + @PutExchange("/post") + void post(); + } + + + @HttpExchange + @Target(ElementType.TYPE) + @Retention(RetentionPolicy.RUNTIME) + private @interface ExtraHttpExchange { + } + }