Skip to content

Commit

Permalink
Reject multiple @⁠HttpExchange declarations in HTTP interface clients
Browse files Browse the repository at this point in the history
This commit updates HttpServiceMethod so that multiple @⁠HttpExchange
declarations on the same element (class or method) are rejected.

Closes spring-projectsgh-32049
  • Loading branch information
sbrannen committed Jan 19, 2024
1 parent 9c6e559 commit 6691ff2
Show file tree
Hide file tree
Showing 2 changed files with 118 additions and 5 deletions.
Original file line number Diff line number Diff line change
Expand Up @@ -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;
Expand All @@ -32,7 +33,11 @@
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;
Expand All @@ -54,6 +59,7 @@
* @author Rossen Stoyanchev
* @author Sebastien Deleuze
* @author Olga Maciaszek-Sharma
* @author Sam Brannen
* @since 6.0
*/
final class HttpServiceMethod {
Expand Down Expand Up @@ -145,7 +151,7 @@ private void applyArguments(HttpRequestValues.Builder requestValues, Object[] ar

/**
* 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,
Expand Down Expand Up @@ -177,10 +183,20 @@ public static HttpRequestValuesInitializer create(
Method method, Class<?> containingClass, @Nullable StringValueResolver embeddedValueResolver,
Supplier<HttpRequestValues.Builder> requestValuesSupplier) {

HttpExchange typeAnnotation = AnnotatedElementUtils.findMergedAnnotation(containingClass, HttpExchange.class);
HttpExchange methodAnnotation = AnnotatedElementUtils.findMergedAnnotation(method, HttpExchange.class);
List<AnnotationDescriptor> 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<AnnotationDescriptor> 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);
Expand Down Expand Up @@ -262,6 +278,43 @@ private static List<MediaType> initAccept(@Nullable HttpExchange typeAnnotation,

return null;
}

private static List<AnnotationDescriptor> 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<HttpExchange> 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();
}
}

}


Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -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;

Expand All @@ -37,8 +42,10 @@
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;

Expand All @@ -52,6 +59,7 @@
*
* @author Rossen Stoyanchev
* @author Olga Maciaszek-Sharma
* @author Sam Brannen
*/
class HttpServiceMethodTests {

Expand Down Expand Up @@ -204,6 +212,33 @@ void typeAndMethodAnnotatedService() {
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);
Expand Down Expand Up @@ -305,9 +340,34 @@ private interface MethodLevelAnnotatedService {

}


@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 {
}

}

0 comments on commit 6691ff2

Please sign in to comment.