From ee4c05b94a01273d21655cda2f77e029ef7e97ae Mon Sep 17 00:00:00 2001 From: Jimmy Praet Date: Wed, 20 Nov 2024 13:50:45 +0100 Subject: [PATCH] Fix non-deterministic configuration for retrieving parameter names from Spring MVC annotations (#137) Fixes #136. * Move AnnotationParameterNameDiscoverer and ProblemValidatorConfiguration to belgif-rest-problem-spring-boot-2 module * Add `@AutoConfigureBefore(ValidationAutoConfiguration.class)` to ProblemValidatorConfiguration * Add AnnotationParameterNameProvider and ProblemValidationConfigurationCustomizer to belgif-rest-problem-spring-boot-3 module * Introduce CachedAnnotationParameterNameSupport for shared code between AnnotationParameterNameDiscoverer and AnnotationParameterNameProvider --- belgif-rest-problem-spring-boot-2/pom.xml | 5 +++ .../AnnotationParameterNameDiscoverer.java | 20 +++++++++++ .../spring/ProblemValidatorConfiguration.java | 3 ++ ...AnnotationParameterNameDiscovererTest.java | 18 ++++++++++ .../ProblemValidatorConfigurationTest.java | 0 .../AnnotationParameterNameProvider.java | 22 ++++++++++++ ...blemValidationConfigurationCustomizer.java | 21 ++++++++++++ .../AnnotationParameterNameProviderTest.java | 18 ++++++++++ ...ValidationConfigurationCustomizerTest.java | 34 +++++++++++++++++++ ...CachedAnnotationParameterNameSupport.java} | 28 ++++++++------- ...edAnnotationParameterNameSupportTest.java} | 22 ++++++++---- src/main/asciidoc/index.adoc | 5 ++- src/main/asciidoc/release-notes.adoc | 6 +++- 13 files changed, 181 insertions(+), 21 deletions(-) create mode 100644 belgif-rest-problem-spring-boot-2/src/main/java/io/github/belgif/rest/problem/spring/AnnotationParameterNameDiscoverer.java rename {belgif-rest-problem-spring => belgif-rest-problem-spring-boot-2}/src/main/java/io/github/belgif/rest/problem/spring/ProblemValidatorConfiguration.java (89%) create mode 100644 belgif-rest-problem-spring-boot-2/src/test/java/io/github/belgif/rest/problem/spring/AnnotationParameterNameDiscovererTest.java rename {belgif-rest-problem-spring => belgif-rest-problem-spring-boot-2}/src/test/java/io/github/belgif/rest/problem/spring/ProblemValidatorConfigurationTest.java (100%) create mode 100644 belgif-rest-problem-spring-boot-3/src/main/java/io/github/belgif/rest/problem/spring/AnnotationParameterNameProvider.java create mode 100644 belgif-rest-problem-spring-boot-3/src/main/java/io/github/belgif/rest/problem/spring/ProblemValidationConfigurationCustomizer.java create mode 100644 belgif-rest-problem-spring-boot-3/src/test/java/io/github/belgif/rest/problem/spring/AnnotationParameterNameProviderTest.java create mode 100644 belgif-rest-problem-spring-boot-3/src/test/java/io/github/belgif/rest/problem/spring/ProblemValidationConfigurationCustomizerTest.java rename belgif-rest-problem-spring/src/main/java/io/github/belgif/rest/problem/{spring/AnnotationParameterNameDiscoverer.java => internal/CachedAnnotationParameterNameSupport.java} (71%) rename belgif-rest-problem-spring/src/test/java/io/github/belgif/rest/problem/{spring/AnnotationParameterNameDiscovererTest.java => internal/CachedAnnotationParameterNameSupportTest.java} (65%) diff --git a/belgif-rest-problem-spring-boot-2/pom.xml b/belgif-rest-problem-spring-boot-2/pom.xml index 789df2d1..e6fd8c47 100644 --- a/belgif-rest-problem-spring-boot-2/pom.xml +++ b/belgif-rest-problem-spring-boot-2/pom.xml @@ -71,6 +71,11 @@ spring-boot-starter-test test + + org.springframework.boot + spring-boot-starter-validation + test + org.junit.jupiter junit-jupiter diff --git a/belgif-rest-problem-spring-boot-2/src/main/java/io/github/belgif/rest/problem/spring/AnnotationParameterNameDiscoverer.java b/belgif-rest-problem-spring-boot-2/src/main/java/io/github/belgif/rest/problem/spring/AnnotationParameterNameDiscoverer.java new file mode 100644 index 00000000..d9bbc04e --- /dev/null +++ b/belgif-rest-problem-spring-boot-2/src/main/java/io/github/belgif/rest/problem/spring/AnnotationParameterNameDiscoverer.java @@ -0,0 +1,20 @@ +package io.github.belgif.rest.problem.spring; + +import java.util.stream.Stream; + +import org.springframework.core.ParameterNameDiscoverer; + +import io.github.belgif.rest.problem.internal.CachedAnnotationParameterNameSupport; + +/** + * ParameterNameDiscoverer that retrieves the parameter name from Spring MVC annotations (if present). + */ +public class AnnotationParameterNameDiscoverer extends CachedAnnotationParameterNameSupport + implements ParameterNameDiscoverer { + + @Override + protected String[] toResult(Stream parameterNames) { + return parameterNames.toArray(String[]::new); + } + +} diff --git a/belgif-rest-problem-spring/src/main/java/io/github/belgif/rest/problem/spring/ProblemValidatorConfiguration.java b/belgif-rest-problem-spring-boot-2/src/main/java/io/github/belgif/rest/problem/spring/ProblemValidatorConfiguration.java similarity index 89% rename from belgif-rest-problem-spring/src/main/java/io/github/belgif/rest/problem/spring/ProblemValidatorConfiguration.java rename to belgif-rest-problem-spring-boot-2/src/main/java/io/github/belgif/rest/problem/spring/ProblemValidatorConfiguration.java index ca872c5e..5893905c 100644 --- a/belgif-rest-problem-spring/src/main/java/io/github/belgif/rest/problem/spring/ProblemValidatorConfiguration.java +++ b/belgif-rest-problem-spring-boot-2/src/main/java/io/github/belgif/rest/problem/spring/ProblemValidatorConfiguration.java @@ -3,8 +3,10 @@ import javax.validation.ConstraintViolationException; import org.springframework.beans.factory.config.BeanDefinition; +import org.springframework.boot.autoconfigure.AutoConfigureBefore; import org.springframework.boot.autoconfigure.condition.ConditionalOnClass; import org.springframework.boot.autoconfigure.condition.ConditionalOnMissingBean; +import org.springframework.boot.autoconfigure.validation.ValidationAutoConfiguration; import org.springframework.boot.validation.MessageInterpolatorFactory; import org.springframework.context.ApplicationContext; import org.springframework.context.annotation.Bean; @@ -21,6 +23,7 @@ * @see org.springframework.boot.autoconfigure.validation.ValidationAutoConfiguration */ @Configuration +@AutoConfigureBefore(ValidationAutoConfiguration.class) public class ProblemValidatorConfiguration { @Bean diff --git a/belgif-rest-problem-spring-boot-2/src/test/java/io/github/belgif/rest/problem/spring/AnnotationParameterNameDiscovererTest.java b/belgif-rest-problem-spring-boot-2/src/test/java/io/github/belgif/rest/problem/spring/AnnotationParameterNameDiscovererTest.java new file mode 100644 index 00000000..d7c995a5 --- /dev/null +++ b/belgif-rest-problem-spring-boot-2/src/test/java/io/github/belgif/rest/problem/spring/AnnotationParameterNameDiscovererTest.java @@ -0,0 +1,18 @@ +package io.github.belgif.rest.problem.spring; + +import static org.assertj.core.api.Assertions.*; + +import java.util.stream.Stream; + +import org.junit.jupiter.api.Test; + +class AnnotationParameterNameDiscovererTest { + + private final AnnotationParameterNameDiscoverer discoverer = new AnnotationParameterNameDiscoverer(); + + @Test + void toResult() { + assertThat(discoverer.toResult(Stream.of("A", "B"))).containsExactly("A", "B"); + } + +} diff --git a/belgif-rest-problem-spring/src/test/java/io/github/belgif/rest/problem/spring/ProblemValidatorConfigurationTest.java b/belgif-rest-problem-spring-boot-2/src/test/java/io/github/belgif/rest/problem/spring/ProblemValidatorConfigurationTest.java similarity index 100% rename from belgif-rest-problem-spring/src/test/java/io/github/belgif/rest/problem/spring/ProblemValidatorConfigurationTest.java rename to belgif-rest-problem-spring-boot-2/src/test/java/io/github/belgif/rest/problem/spring/ProblemValidatorConfigurationTest.java diff --git a/belgif-rest-problem-spring-boot-3/src/main/java/io/github/belgif/rest/problem/spring/AnnotationParameterNameProvider.java b/belgif-rest-problem-spring-boot-3/src/main/java/io/github/belgif/rest/problem/spring/AnnotationParameterNameProvider.java new file mode 100644 index 00000000..666aaa94 --- /dev/null +++ b/belgif-rest-problem-spring-boot-3/src/main/java/io/github/belgif/rest/problem/spring/AnnotationParameterNameProvider.java @@ -0,0 +1,22 @@ +package io.github.belgif.rest.problem.spring; + +import java.util.List; +import java.util.stream.Collectors; +import java.util.stream.Stream; + +import jakarta.validation.ParameterNameProvider; + +import io.github.belgif.rest.problem.internal.CachedAnnotationParameterNameSupport; + +/** + * ParameterNameProvider that retrieves the parameter name from Spring MVC annotations (if present). + */ +public class AnnotationParameterNameProvider extends CachedAnnotationParameterNameSupport> + implements ParameterNameProvider { + + @Override + protected List toResult(Stream parameterNames) { + return parameterNames.collect(Collectors.toList()); + } + +} diff --git a/belgif-rest-problem-spring-boot-3/src/main/java/io/github/belgif/rest/problem/spring/ProblemValidationConfigurationCustomizer.java b/belgif-rest-problem-spring-boot-3/src/main/java/io/github/belgif/rest/problem/spring/ProblemValidationConfigurationCustomizer.java new file mode 100644 index 00000000..61e09e42 --- /dev/null +++ b/belgif-rest-problem-spring-boot-3/src/main/java/io/github/belgif/rest/problem/spring/ProblemValidationConfigurationCustomizer.java @@ -0,0 +1,21 @@ +package io.github.belgif.rest.problem.spring; + +import jakarta.validation.Configuration; + +import org.springframework.boot.autoconfigure.condition.ConditionalOnClass; +import org.springframework.boot.autoconfigure.validation.ValidationConfigurationCustomizer; +import org.springframework.stereotype.Component; + +/** + * ValidationConfigurationCustomizer that registers the AnnotationParameterNameProvider. + */ +@Component +@ConditionalOnClass(Configuration.class) +public class ProblemValidationConfigurationCustomizer implements ValidationConfigurationCustomizer { + + @Override + public void customize(Configuration configuration) { + configuration.parameterNameProvider(new AnnotationParameterNameProvider()); + } + +} diff --git a/belgif-rest-problem-spring-boot-3/src/test/java/io/github/belgif/rest/problem/spring/AnnotationParameterNameProviderTest.java b/belgif-rest-problem-spring-boot-3/src/test/java/io/github/belgif/rest/problem/spring/AnnotationParameterNameProviderTest.java new file mode 100644 index 00000000..5781c398 --- /dev/null +++ b/belgif-rest-problem-spring-boot-3/src/test/java/io/github/belgif/rest/problem/spring/AnnotationParameterNameProviderTest.java @@ -0,0 +1,18 @@ +package io.github.belgif.rest.problem.spring; + +import static org.assertj.core.api.Assertions.*; + +import java.util.stream.Stream; + +import org.junit.jupiter.api.Test; + +class AnnotationParameterNameProviderTest { + + private final AnnotationParameterNameProvider provider = new AnnotationParameterNameProvider(); + + @Test + void toResult() { + assertThat(provider.toResult(Stream.of("A", "B"))).containsExactly("A", "B"); + } + +} diff --git a/belgif-rest-problem-spring-boot-3/src/test/java/io/github/belgif/rest/problem/spring/ProblemValidationConfigurationCustomizerTest.java b/belgif-rest-problem-spring-boot-3/src/test/java/io/github/belgif/rest/problem/spring/ProblemValidationConfigurationCustomizerTest.java new file mode 100644 index 00000000..61f9ad78 --- /dev/null +++ b/belgif-rest-problem-spring-boot-3/src/test/java/io/github/belgif/rest/problem/spring/ProblemValidationConfigurationCustomizerTest.java @@ -0,0 +1,34 @@ +package io.github.belgif.rest.problem.spring; + +import static org.assertj.core.api.Assertions.*; +import static org.mockito.Mockito.*; + +import jakarta.validation.Configuration; +import jakarta.validation.ParameterNameProvider; + +import org.junit.jupiter.api.Test; +import org.junit.jupiter.api.extension.ExtendWith; +import org.mockito.ArgumentCaptor; +import org.mockito.Captor; +import org.mockito.Mock; +import org.mockito.junit.jupiter.MockitoExtension; + +@ExtendWith(MockitoExtension.class) +class ProblemValidationConfigurationCustomizerTest { + + private final ProblemValidationConfigurationCustomizer customizer = new ProblemValidationConfigurationCustomizer(); + + @Mock + private Configuration configuration; + + @Captor + private ArgumentCaptor parameterNameProviderCaptor; + + @Test + void customize() { + when(configuration.parameterNameProvider(parameterNameProviderCaptor.capture())).thenReturn(configuration); + customizer.customize(configuration); + assertThat(parameterNameProviderCaptor.getValue()).isInstanceOf(AnnotationParameterNameProvider.class); + } + +} diff --git a/belgif-rest-problem-spring/src/main/java/io/github/belgif/rest/problem/spring/AnnotationParameterNameDiscoverer.java b/belgif-rest-problem-spring/src/main/java/io/github/belgif/rest/problem/internal/CachedAnnotationParameterNameSupport.java similarity index 71% rename from belgif-rest-problem-spring/src/main/java/io/github/belgif/rest/problem/spring/AnnotationParameterNameDiscoverer.java rename to belgif-rest-problem-spring/src/main/java/io/github/belgif/rest/problem/internal/CachedAnnotationParameterNameSupport.java index 8542bf06..7f1d5d79 100644 --- a/belgif-rest-problem-spring/src/main/java/io/github/belgif/rest/problem/spring/AnnotationParameterNameDiscoverer.java +++ b/belgif-rest-problem-spring/src/main/java/io/github/belgif/rest/problem/internal/CachedAnnotationParameterNameSupport.java @@ -1,4 +1,4 @@ -package io.github.belgif.rest.problem.spring; +package io.github.belgif.rest.problem.internal; import java.lang.annotation.Annotation; import java.lang.reflect.Constructor; @@ -7,8 +7,9 @@ import java.lang.reflect.Parameter; import java.util.Arrays; import java.util.concurrent.ConcurrentHashMap; +import java.util.stream.Stream; -import org.springframework.core.ParameterNameDiscoverer; +import org.springframework.lang.NonNull; import org.springframework.web.bind.annotation.CookieValue; import org.springframework.web.bind.annotation.MatrixVariable; import org.springframework.web.bind.annotation.PathVariable; @@ -16,26 +17,25 @@ import org.springframework.web.bind.annotation.RequestParam; /** - * ParameterNameDiscoverer that retrieves the parameter name from Spring MVC annotations (if present). + * Helper class for cached retrieval of parameter names from Spring MVC annotations. + * + * @param the result type */ -public class AnnotationParameterNameDiscoverer implements ParameterNameDiscoverer { +public abstract class CachedAnnotationParameterNameSupport { - private final ConcurrentHashMap parameterNameCache = new ConcurrentHashMap<>(); + private final ConcurrentHashMap parameterNameCache = new ConcurrentHashMap<>(); - @Override - public String[] getParameterNames(Constructor constructor) { + public T getParameterNames(@NonNull Constructor constructor) { return parameterNameCache.computeIfAbsent(constructor, this::getParameterNames); } - @Override - public String[] getParameterNames(Method method) { + public T getParameterNames(@NonNull Method method) { return parameterNameCache.computeIfAbsent(method, this::getParameterNames); } - private String[] getParameterNames(Executable executable) { - return Arrays.stream(executable.getParameters()) - .map(this::getParameterName) - .toArray(String[]::new); + private T getParameterNames(Executable executable) { + return toResult(Arrays.stream(executable.getParameters()) + .map(this::getParameterName)); } private String getParameterName(Parameter parameter) { @@ -64,4 +64,6 @@ private String getParameterNameFromAnnotations(Parameter parameter) { return null; } + protected abstract T toResult(Stream parameterNames); + } diff --git a/belgif-rest-problem-spring/src/test/java/io/github/belgif/rest/problem/spring/AnnotationParameterNameDiscovererTest.java b/belgif-rest-problem-spring/src/test/java/io/github/belgif/rest/problem/internal/CachedAnnotationParameterNameSupportTest.java similarity index 65% rename from belgif-rest-problem-spring/src/test/java/io/github/belgif/rest/problem/spring/AnnotationParameterNameDiscovererTest.java rename to belgif-rest-problem-spring/src/test/java/io/github/belgif/rest/problem/internal/CachedAnnotationParameterNameSupportTest.java index 3c120940..3dd730e9 100644 --- a/belgif-rest-problem-spring/src/test/java/io/github/belgif/rest/problem/spring/AnnotationParameterNameDiscovererTest.java +++ b/belgif-rest-problem-spring/src/test/java/io/github/belgif/rest/problem/internal/CachedAnnotationParameterNameSupportTest.java @@ -1,7 +1,11 @@ -package io.github.belgif.rest.problem.spring; +package io.github.belgif.rest.problem.internal; import static org.assertj.core.api.Assertions.*; +import java.util.List; +import java.util.stream.Collectors; +import java.util.stream.Stream; + import org.junit.jupiter.api.Test; import org.junit.jupiter.params.ParameterizedTest; import org.junit.jupiter.params.provider.ValueSource; @@ -11,20 +15,26 @@ import org.springframework.web.bind.annotation.RequestHeader; import org.springframework.web.bind.annotation.RequestParam; -class AnnotationParameterNameDiscovererTest { +class CachedAnnotationParameterNameSupportTest { - private final AnnotationParameterNameDiscoverer discoverer = new AnnotationParameterNameDiscoverer(); + private final CachedAnnotationParameterNameSupport> support = + new CachedAnnotationParameterNameSupport>() { + @Override + protected List toResult(Stream parameterNames) { + return parameterNames.collect(Collectors.toList()); + } + }; @Test void getParameterNameConstructor() throws Exception { - assertThat(discoverer.getParameterNames(Tested.class.getDeclaredConstructor(String.class, Long.class))) + assertThat(support.getParameterNames(Tested.class.getDeclaredConstructor(String.class, Long.class))) .containsExactly("name", "other"); } @Test void getParameterNamesMethod() throws Exception { assertThat( - discoverer.getParameterNames(Tested.class.getDeclaredMethod("plainMethod", String.class, Long.class))) + support.getParameterNames(Tested.class.getDeclaredMethod("plainMethod", String.class, Long.class))) .containsExactly("name", "other"); } @@ -32,7 +42,7 @@ void getParameterNamesMethod() throws Exception { @ValueSource( strings = { "requestParam", "pathVariable", "requestHeader", "cookieValue", "matrixVariable", "fallback" }) void getParameterNamesMethodWithJaxRsAnnotation(String method) throws Exception { - assertThat(discoverer.getParameterNames(Tested.class.getDeclaredMethod(method, String.class))) + assertThat(support.getParameterNames(Tested.class.getDeclaredMethod(method, String.class))) .containsExactly("name"); } diff --git a/src/main/asciidoc/index.adoc b/src/main/asciidoc/index.adoc index a4934d12..75077da5 100644 --- a/src/main/asciidoc/index.adoc +++ b/src/main/asciidoc/index.adoc @@ -544,7 +544,6 @@ io.github.belgif.rest.problem.scan-additional-problem-packages=com.acme.custom * *RoutingExceptionsHandler:* an exception handler for RestControllers that converts routing related validation exceptions to HTTP 400 BadRequestProblem. * *ProblemResponseErrorHandler:* a RestTemplate error handler that converts problem responses to Problem exceptions. * *ProblemRestTemplateCustomizer:* a RestTemplateCustomizer that registers the ProblemResponseErrorHandler. -* *AnnotationParameterNameDiscoverer:* a bean validation ParameterNameDiscoverer that retrieves parameter names from Spring MVC annotations In general, these components make it possible to use standard java exception handling (throw and try-catch) for dealing with problems in Spring Boot REST APIs. @@ -555,6 +554,8 @@ Rather than depending on <> directly, Spring Boot 2. * *ProblemWebClientCustomizer:* a WebClientCustomizer that registers a filter that converts problem responses to Problem exceptions. This handles integration with the https://docs.spring.io/spring-framework/reference/web/webflux-webclient.html[Reactive WebClient]. +* *AnnotationParameterNameDiscoverer:* a bean validation ParameterNameDiscoverer that retrieves parameter names from Spring MVC annotations +* *ProblemValidatorConfiguration:* registers a LocalValidatorFactoryBean with the AnnotationParameterNameDiscoverer [[belgif-rest-problem-spring-boot-3]] ==== belgif-rest-problem-spring-boot-3 @@ -565,3 +566,5 @@ Rather than depending on <> directly, Spring Boot 3. This handles integration with the https://docs.spring.io/spring-framework/reference/web/webflux-webclient.html[Reactive WebClient]. * *NoResourceFoundExceptionHandler:* an exception handler for RestControllers that converts NoResourceFoundException to HTTP 404 ResourceNotFoundProblem. * *ProblemRestClientCustomizer:* a RestClientCustomizer that registers the ProblemResponseErrorHandler. +* *AnnotationParameterNameProvider:* a bean validation ParameterNameProvider that retrieves parameter names from Spring MVC annotations +* *ProblemValidationConfigurationCustomizer:* a ValidationConfigurationCustomizer that registers the AnnotationParameterNameProvider \ No newline at end of file diff --git a/src/main/asciidoc/release-notes.adoc b/src/main/asciidoc/release-notes.adoc index 756e7fe6..bf8c499b 100644 --- a/src/main/asciidoc/release-notes.adoc +++ b/src/main/asciidoc/release-notes.adoc @@ -16,7 +16,11 @@ *belgif-rest-problem:* -* Add in(Input) in InputValidationIssue for a fluent single input setter +* Add in(Input) to InputValidationIssue for a fluent single input setter + +*belgif-rest-problem-spring:* + +* Fix non-deterministic configuration for retrieving parameter names from Spring MVC annotations == Version 0.10