From 86b96254a1f28693a1ee7d2412a536d325a74896 Mon Sep 17 00:00:00 2001 From: Stephane Nicoll Date: Tue, 10 Apr 2018 15:03:28 +0200 Subject: [PATCH 1/2] Migrate to ApplicationContextRunner --- .../ControllerEndpointDiscovererTests.java | 83 ++++++++-------- .../ServletEndpointDiscovererTests.java | 98 ++++++++++--------- 2 files changed, 97 insertions(+), 84 deletions(-) diff --git a/spring-boot-project/spring-boot-actuator/src/test/java/org/springframework/boot/actuate/endpoint/web/annotation/ControllerEndpointDiscovererTests.java b/spring-boot-project/spring-boot-actuator/src/test/java/org/springframework/boot/actuate/endpoint/web/annotation/ControllerEndpointDiscovererTests.java index b29ad955ad..af79cdcfb5 100644 --- a/spring-boot-project/spring-boot-actuator/src/test/java/org/springframework/boot/actuate/endpoint/web/annotation/ControllerEndpointDiscovererTests.java +++ b/spring-boot-project/spring-boot-actuator/src/test/java/org/springframework/boot/actuate/endpoint/web/annotation/ControllerEndpointDiscovererTests.java @@ -31,7 +31,9 @@ import org.springframework.boot.actuate.endpoint.annotation.DiscoveredEndpoint; import org.springframework.boot.actuate.endpoint.annotation.Endpoint; import org.springframework.boot.actuate.endpoint.annotation.ReadOperation; import org.springframework.boot.actuate.endpoint.web.PathMapper; -import org.springframework.context.annotation.AnnotationConfigApplicationContext; +import org.springframework.boot.test.context.assertj.AssertableApplicationContext; +import org.springframework.boot.test.context.runner.ApplicationContextRunner; +import org.springframework.boot.test.context.runner.ContextConsumer; import org.springframework.context.annotation.Configuration; import org.springframework.context.annotation.Import; @@ -41,74 +43,77 @@ import static org.assertj.core.api.Assertions.assertThat; * Tests for {@link ControllerEndpointDiscoverer}. * * @author Phillip Webb + * @author Stephane Nicoll */ public class ControllerEndpointDiscovererTests { @Rule public final ExpectedException thrown = ExpectedException.none(); + private final ApplicationContextRunner contextRunner = new ApplicationContextRunner(); + @Test public void getEndpointsWhenNoEndpointBeansShouldReturnEmptyCollection() { - load(EmptyConfiguration.class, - (discoverer) -> assertThat(discoverer.getEndpoints()).isEmpty()); + this.contextRunner.withUserConfiguration(EmptyConfiguration.class).run( + assertDiscoverer((discoverer) -> + assertThat(discoverer.getEndpoints()).isEmpty())); } @Test public void getEndpointsShouldIncludeControllerEndpoints() { - load(TestControllerEndpoint.class, (discoverer) -> { - Collection endpoints = discoverer.getEndpoints(); - assertThat(endpoints).hasSize(1); - ExposableControllerEndpoint endpoint = endpoints.iterator().next(); - assertThat(endpoint.getId()).isEqualTo("testcontroller"); - assertThat(endpoint.getController()) - .isInstanceOf(TestControllerEndpoint.class); - assertThat(endpoint).isInstanceOf(DiscoveredEndpoint.class); - }); + this.contextRunner.withUserConfiguration(TestControllerEndpoint.class) + .run(assertDiscoverer((discoverer) -> { + Collection endpoints = discoverer.getEndpoints(); + assertThat(endpoints).hasSize(1); + ExposableControllerEndpoint endpoint = endpoints.iterator().next(); + assertThat(endpoint.getId()).isEqualTo("testcontroller"); + assertThat(endpoint.getController()) + .isInstanceOf(TestControllerEndpoint.class); + assertThat(endpoint).isInstanceOf(DiscoveredEndpoint.class); + })); } @Test public void getEndpointsShouldIncludeRestControllerEndpoints() { - load(TestRestControllerEndpoint.class, (discoverer) -> { - Collection endpoints = discoverer.getEndpoints(); - assertThat(endpoints).hasSize(1); - ExposableControllerEndpoint endpoint = endpoints.iterator().next(); - assertThat(endpoint.getId()).isEqualTo("testrestcontroller"); - assertThat(endpoint.getController()) - .isInstanceOf(TestRestControllerEndpoint.class); - }); + this.contextRunner.withUserConfiguration(TestRestControllerEndpoint.class) + .run(assertDiscoverer((discoverer) -> { + Collection endpoints = discoverer.getEndpoints(); + assertThat(endpoints).hasSize(1); + ExposableControllerEndpoint endpoint = endpoints.iterator().next(); + assertThat(endpoint.getId()).isEqualTo("testrestcontroller"); + assertThat(endpoint.getController()) + .isInstanceOf(TestRestControllerEndpoint.class); + })); } @Test public void getEndpointsShouldNotDiscoverRegularEndpoints() { - load(WithRegularEndpointConfiguration.class, (discoverer) -> { - Collection endpoints = discoverer.getEndpoints(); - List ids = endpoints.stream().map(ExposableEndpoint::getId) - .collect(Collectors.toList()); - assertThat(ids).containsOnly("testcontroller", "testrestcontroller"); - }); + this.contextRunner.withUserConfiguration(WithRegularEndpointConfiguration.class) + .run(assertDiscoverer((discoverer) -> { + Collection endpoints = discoverer.getEndpoints(); + List ids = endpoints.stream().map(ExposableEndpoint::getId) + .collect(Collectors.toList()); + assertThat(ids).containsOnly("testcontroller", "testrestcontroller"); + })); } @Test public void getEndpointWhenEndpointHasOperationsShouldThrowException() { - load(TestControllerWithOperation.class, (discoverer) -> { - this.thrown.expect(IllegalStateException.class); - this.thrown.expectMessage("ControllerEndpoints must not declare operations"); - discoverer.getEndpoints(); - }); + this.contextRunner.withUserConfiguration(TestControllerWithOperation.class) + .run(assertDiscoverer((discoverer) -> { + this.thrown.expect(IllegalStateException.class); + this.thrown.expectMessage("ControllerEndpoints must not declare operations"); + discoverer.getEndpoints(); + })); } - private void load(Class configuration, + private ContextConsumer assertDiscoverer( Consumer consumer) { - AnnotationConfigApplicationContext context = new AnnotationConfigApplicationContext( - configuration); - try { + return (context) -> { ControllerEndpointDiscoverer discoverer = new ControllerEndpointDiscoverer( context, PathMapper.useEndpointId(), Collections.emptyList()); consumer.accept(discoverer); - } - finally { - context.close(); - } + }; } @Configuration diff --git a/spring-boot-project/spring-boot-actuator/src/test/java/org/springframework/boot/actuate/endpoint/web/annotation/ServletEndpointDiscovererTests.java b/spring-boot-project/spring-boot-actuator/src/test/java/org/springframework/boot/actuate/endpoint/web/annotation/ServletEndpointDiscovererTests.java index 61ef143c86..2a17c5070a 100644 --- a/spring-boot-project/spring-boot-actuator/src/test/java/org/springframework/boot/actuate/endpoint/web/annotation/ServletEndpointDiscovererTests.java +++ b/spring-boot-project/spring-boot-actuator/src/test/java/org/springframework/boot/actuate/endpoint/web/annotation/ServletEndpointDiscovererTests.java @@ -40,7 +40,9 @@ import org.springframework.boot.actuate.endpoint.annotation.ReadOperation; import org.springframework.boot.actuate.endpoint.web.EndpointServlet; import org.springframework.boot.actuate.endpoint.web.ExposableServletEndpoint; import org.springframework.boot.actuate.endpoint.web.PathMapper; -import org.springframework.context.annotation.AnnotationConfigApplicationContext; +import org.springframework.boot.test.context.assertj.AssertableApplicationContext; +import org.springframework.boot.test.context.runner.ApplicationContextRunner; +import org.springframework.boot.test.context.runner.ContextConsumer; import org.springframework.context.annotation.Configuration; import org.springframework.context.annotation.Import; @@ -50,88 +52,94 @@ import static org.assertj.core.api.Assertions.assertThat; * Tests for {@link ServletEndpointDiscoverer}. * * @author Phillip Webb + * @author Stephane Nicoll */ public class ServletEndpointDiscovererTests { @Rule public final ExpectedException thrown = ExpectedException.none(); + private final ApplicationContextRunner contextRunner = new ApplicationContextRunner(); + @Test public void getEndpointsWhenNoEndpointBeansShouldReturnEmptyCollection() { - load(EmptyConfiguration.class, - (discoverer) -> assertThat(discoverer.getEndpoints()).isEmpty()); + this.contextRunner.withUserConfiguration(EmptyConfiguration.class) + .run(assertDiscoverer((discoverer) + -> assertThat(discoverer.getEndpoints()).isEmpty())); } @Test public void getEndpointsShouldIncludeServletEndpoints() { - load(TestServletEndpoint.class, (discoverer) -> { - Collection endpoints = discoverer.getEndpoints(); - assertThat(endpoints).hasSize(1); - ExposableServletEndpoint endpoint = endpoints.iterator().next(); - assertThat(endpoint.getId()).isEqualTo("testservlet"); - assertThat(endpoint.getEndpointServlet()).isNotNull(); - assertThat(endpoint).isInstanceOf(DiscoveredEndpoint.class); - }); + this.contextRunner.withUserConfiguration(TestServletEndpoint.class) + .run(assertDiscoverer((discoverer) -> { + Collection endpoints = discoverer.getEndpoints(); + assertThat(endpoints).hasSize(1); + ExposableServletEndpoint endpoint = endpoints.iterator().next(); + assertThat(endpoint.getId()).isEqualTo("testservlet"); + assertThat(endpoint.getEndpointServlet()).isNotNull(); + assertThat(endpoint).isInstanceOf(DiscoveredEndpoint.class); + })); } @Test public void getEndpointsShouldNotDiscoverRegularEndpoints() { - load(WithRegularEndpointConfiguration.class, (discoverer) -> { - Collection endpoints = discoverer.getEndpoints(); - List ids = endpoints.stream().map(ExposableEndpoint::getId) - .collect(Collectors.toList()); - assertThat(ids).containsOnly("testservlet"); - }); + this.contextRunner.withUserConfiguration(WithRegularEndpointConfiguration.class) + .run(assertDiscoverer((discoverer) -> { + Collection endpoints = discoverer.getEndpoints(); + List ids = endpoints.stream().map(ExposableEndpoint::getId) + .collect(Collectors.toList()); + assertThat(ids).containsOnly("testservlet"); + })); } @Test public void getEndpointWhenEndpointHasOperationsShouldThrowException() { - load(TestServletEndpointWithOperation.class, (discoverer) -> { - this.thrown.expect(IllegalStateException.class); - this.thrown.expectMessage("ServletEndpoints must not declare operations"); - discoverer.getEndpoints(); - }); + this.contextRunner.withUserConfiguration(TestServletEndpointWithOperation.class) + .run(assertDiscoverer((discoverer) -> { + this.thrown.expect(IllegalStateException.class); + this.thrown.expectMessage("ServletEndpoints must not declare operations"); + discoverer.getEndpoints(); + })); } @Test public void getEndpointWhenEndpointNotASupplierShouldThrowException() { - load(TestServletEndpointNotASupplier.class, (discoverer) -> { - this.thrown.expect(IllegalStateException.class); - this.thrown.expectMessage("must be a supplier"); - discoverer.getEndpoints(); - }); + this.contextRunner.withUserConfiguration(TestServletEndpointNotASupplier.class) + .run(assertDiscoverer((discoverer) -> { + this.thrown.expect(IllegalStateException.class); + this.thrown.expectMessage("must be a supplier"); + discoverer.getEndpoints(); + })); } @Test public void getEndpointWhenEndpointSuppliesWrongTypeShouldThrowException() { - load(TestServletEndpointSupplierOfWrongType.class, (discoverer) -> { - this.thrown.expect(IllegalStateException.class); - this.thrown.expectMessage("must supply an EndpointServlet"); - discoverer.getEndpoints(); - }); + this.contextRunner + .withUserConfiguration(TestServletEndpointSupplierOfWrongType.class) + .run(assertDiscoverer((discoverer) -> { + this.thrown.expect(IllegalStateException.class); + this.thrown.expectMessage("must supply an EndpointServlet"); + discoverer.getEndpoints(); + })); } @Test public void getEndpointWhenEndpointSuppliesNullShouldThrowException() { - load(TestServletEndpointSupplierOfNull.class, (discoverer) -> { - this.thrown.expect(IllegalStateException.class); - this.thrown.expectMessage("must not supply null"); - discoverer.getEndpoints(); - }); + this.contextRunner.withUserConfiguration(TestServletEndpointSupplierOfNull.class) + .run(assertDiscoverer((discoverer) -> { + this.thrown.expect(IllegalStateException.class); + this.thrown.expectMessage("must not supply null"); + discoverer.getEndpoints(); + })); } - private void load(Class configuration, + private ContextConsumer assertDiscoverer( Consumer consumer) { - AnnotationConfigApplicationContext context = new AnnotationConfigApplicationContext( - configuration); - try { + return (context) -> { ServletEndpointDiscoverer discoverer = new ServletEndpointDiscoverer(context, PathMapper.useEndpointId(), Collections.emptyList()); consumer.accept(discoverer); - } - finally { - context.close(); - } + }; } @Configuration From e8fac7d9c4ae4461bd34fa34527318bdd13feb63 Mon Sep 17 00:00:00 2001 From: Stephane Nicoll Date: Tue, 10 Apr 2018 20:19:28 +0200 Subject: [PATCH 2/2] Properly discover endpoints that are proxied Previously to this commit, if a `@ControllerEndpoint`, `@RestControllerEndpoint` or `@ServletEndpoint` annotated bean was proxied, the endpoint wasn't properly detected. This commit makes sure that annotation retrieval works on the user class while preserving the get (vs. find) retrieval semantic Closes gh-12441 --- .../ControllerEndpointDiscoverer.java | 3 +- .../annotation/ServletEndpointDiscoverer.java | 3 +- .../ControllerEndpointDiscovererTests.java | 47 +++++++++++++++++++ .../ServletEndpointDiscovererTests.java | 28 +++++++++++ 4 files changed, 79 insertions(+), 2 deletions(-) diff --git a/spring-boot-project/spring-boot-actuator/src/main/java/org/springframework/boot/actuate/endpoint/web/annotation/ControllerEndpointDiscoverer.java b/spring-boot-project/spring-boot-actuator/src/main/java/org/springframework/boot/actuate/endpoint/web/annotation/ControllerEndpointDiscoverer.java index 112a509224..7df0d22e42 100644 --- a/spring-boot-project/spring-boot-actuator/src/main/java/org/springframework/boot/actuate/endpoint/web/annotation/ControllerEndpointDiscoverer.java +++ b/spring-boot-project/spring-boot-actuator/src/main/java/org/springframework/boot/actuate/endpoint/web/annotation/ControllerEndpointDiscoverer.java @@ -29,6 +29,7 @@ import org.springframework.boot.actuate.endpoint.web.PathMapper; import org.springframework.context.ApplicationContext; import org.springframework.core.annotation.AnnotatedElementUtils; import org.springframework.util.Assert; +import org.springframework.util.ClassUtils; /** * {@link EndpointDiscoverer} for {@link ExposableControllerEndpoint controller @@ -60,7 +61,7 @@ public class ControllerEndpointDiscoverer @Override protected boolean isEndpointExposed(Object endpointBean) { - Class type = endpointBean.getClass(); + Class type = ClassUtils.getUserClass(endpointBean.getClass()); return AnnotatedElementUtils.isAnnotated(type, ControllerEndpoint.class) || AnnotatedElementUtils.isAnnotated(type, RestControllerEndpoint.class); } diff --git a/spring-boot-project/spring-boot-actuator/src/main/java/org/springframework/boot/actuate/endpoint/web/annotation/ServletEndpointDiscoverer.java b/spring-boot-project/spring-boot-actuator/src/main/java/org/springframework/boot/actuate/endpoint/web/annotation/ServletEndpointDiscoverer.java index 0b9b5ccf96..f00857c2db 100644 --- a/spring-boot-project/spring-boot-actuator/src/main/java/org/springframework/boot/actuate/endpoint/web/annotation/ServletEndpointDiscoverer.java +++ b/spring-boot-project/spring-boot-actuator/src/main/java/org/springframework/boot/actuate/endpoint/web/annotation/ServletEndpointDiscoverer.java @@ -30,6 +30,7 @@ import org.springframework.boot.actuate.endpoint.web.PathMapper; import org.springframework.context.ApplicationContext; import org.springframework.core.annotation.AnnotatedElementUtils; import org.springframework.util.Assert; +import org.springframework.util.ClassUtils; /** * {@link EndpointDiscoverer} for {@link ExposableServletEndpoint servlet endpoints}. @@ -60,7 +61,7 @@ public class ServletEndpointDiscoverer @Override protected boolean isEndpointExposed(Object endpointBean) { - Class type = endpointBean.getClass(); + Class type = ClassUtils.getUserClass(endpointBean.getClass()); return AnnotatedElementUtils.isAnnotated(type, ServletEndpoint.class); } diff --git a/spring-boot-project/spring-boot-actuator/src/test/java/org/springframework/boot/actuate/endpoint/web/annotation/ControllerEndpointDiscovererTests.java b/spring-boot-project/spring-boot-actuator/src/test/java/org/springframework/boot/actuate/endpoint/web/annotation/ControllerEndpointDiscovererTests.java index af79cdcfb5..b1ea4acc06 100644 --- a/spring-boot-project/spring-boot-actuator/src/test/java/org/springframework/boot/actuate/endpoint/web/annotation/ControllerEndpointDiscovererTests.java +++ b/spring-boot-project/spring-boot-actuator/src/test/java/org/springframework/boot/actuate/endpoint/web/annotation/ControllerEndpointDiscovererTests.java @@ -31,11 +31,14 @@ import org.springframework.boot.actuate.endpoint.annotation.DiscoveredEndpoint; import org.springframework.boot.actuate.endpoint.annotation.Endpoint; import org.springframework.boot.actuate.endpoint.annotation.ReadOperation; import org.springframework.boot.actuate.endpoint.web.PathMapper; +import org.springframework.boot.autoconfigure.AutoConfigurations; +import org.springframework.boot.autoconfigure.validation.ValidationAutoConfiguration; import org.springframework.boot.test.context.assertj.AssertableApplicationContext; import org.springframework.boot.test.context.runner.ApplicationContextRunner; import org.springframework.boot.test.context.runner.ContextConsumer; import org.springframework.context.annotation.Configuration; import org.springframework.context.annotation.Import; +import org.springframework.validation.annotation.Validated; import static org.assertj.core.api.Assertions.assertThat; @@ -73,6 +76,22 @@ public class ControllerEndpointDiscovererTests { })); } + @Test + public void getEndpointsShouldDiscoverProxyControllerEndpoints() { + this.contextRunner.withUserConfiguration(TestProxyControllerEndpoint.class) + .withConfiguration(AutoConfigurations.of( + ValidationAutoConfiguration.class)) + .run(assertDiscoverer((discoverer) -> { + Collection endpoints = discoverer.getEndpoints(); + assertThat(endpoints).hasSize(1); + ExposableControllerEndpoint endpoint = endpoints.iterator().next(); + assertThat(endpoint.getId()).isEqualTo("testcontroller"); + assertThat(endpoint.getController()) + .isInstanceOf(TestProxyControllerEndpoint.class); + assertThat(endpoint).isInstanceOf(DiscoveredEndpoint.class); + })); + } + @Test public void getEndpointsShouldIncludeRestControllerEndpoints() { this.contextRunner.withUserConfiguration(TestRestControllerEndpoint.class) @@ -86,6 +105,22 @@ public class ControllerEndpointDiscovererTests { })); } + @Test + public void getEndpointsShouldDiscoverProxyRestControllerEndpoints() { + this.contextRunner.withUserConfiguration(TestProxyRestControllerEndpoint.class) + .withConfiguration(AutoConfigurations.of( + ValidationAutoConfiguration.class)) + .run(assertDiscoverer((discoverer) -> { + Collection endpoints = discoverer.getEndpoints(); + assertThat(endpoints).hasSize(1); + ExposableControllerEndpoint endpoint = endpoints.iterator().next(); + assertThat(endpoint.getId()).isEqualTo("testrestcontroller"); + assertThat(endpoint.getController()) + .isInstanceOf(TestProxyRestControllerEndpoint.class); + assertThat(endpoint).isInstanceOf(DiscoveredEndpoint.class); + })); + } + @Test public void getEndpointsShouldNotDiscoverRegularEndpoints() { this.contextRunner.withUserConfiguration(WithRegularEndpointConfiguration.class) @@ -133,11 +168,23 @@ public class ControllerEndpointDiscovererTests { } + @ControllerEndpoint(id = "testcontroller") + @Validated + static class TestProxyControllerEndpoint { + + } + @RestControllerEndpoint(id = "testrestcontroller") static class TestRestControllerEndpoint { } + @RestControllerEndpoint(id = "testrestcontroller") + @Validated + static class TestProxyRestControllerEndpoint { + + } + @Endpoint(id = "test") static class TestEndpoint { diff --git a/spring-boot-project/spring-boot-actuator/src/test/java/org/springframework/boot/actuate/endpoint/web/annotation/ServletEndpointDiscovererTests.java b/spring-boot-project/spring-boot-actuator/src/test/java/org/springframework/boot/actuate/endpoint/web/annotation/ServletEndpointDiscovererTests.java index 2a17c5070a..f2b0a48c8b 100644 --- a/spring-boot-project/spring-boot-actuator/src/test/java/org/springframework/boot/actuate/endpoint/web/annotation/ServletEndpointDiscovererTests.java +++ b/spring-boot-project/spring-boot-actuator/src/test/java/org/springframework/boot/actuate/endpoint/web/annotation/ServletEndpointDiscovererTests.java @@ -40,11 +40,14 @@ import org.springframework.boot.actuate.endpoint.annotation.ReadOperation; import org.springframework.boot.actuate.endpoint.web.EndpointServlet; import org.springframework.boot.actuate.endpoint.web.ExposableServletEndpoint; import org.springframework.boot.actuate.endpoint.web.PathMapper; +import org.springframework.boot.autoconfigure.AutoConfigurations; +import org.springframework.boot.autoconfigure.validation.ValidationAutoConfiguration; import org.springframework.boot.test.context.assertj.AssertableApplicationContext; import org.springframework.boot.test.context.runner.ApplicationContextRunner; import org.springframework.boot.test.context.runner.ContextConsumer; import org.springframework.context.annotation.Configuration; import org.springframework.context.annotation.Import; +import org.springframework.validation.annotation.Validated; import static org.assertj.core.api.Assertions.assertThat; @@ -81,6 +84,20 @@ public class ServletEndpointDiscovererTests { })); } + @Test + public void getEndpointsShouldDiscoverProxyServletEndpoints() { + this.contextRunner.withUserConfiguration(TestProxyServletEndpoint.class) + .withConfiguration(AutoConfigurations.of(ValidationAutoConfiguration.class)) + .run(assertDiscoverer((discoverer) -> { + Collection endpoints = discoverer.getEndpoints(); + assertThat(endpoints).hasSize(1); + ExposableServletEndpoint endpoint = endpoints.iterator().next(); + assertThat(endpoint.getId()).isEqualTo("testservlet"); + assertThat(endpoint.getEndpointServlet()).isNotNull(); + assertThat(endpoint).isInstanceOf(DiscoveredEndpoint.class); + })); + } + @Test public void getEndpointsShouldNotDiscoverRegularEndpoints() { this.contextRunner.withUserConfiguration(WithRegularEndpointConfiguration.class) @@ -163,6 +180,17 @@ public class ServletEndpointDiscovererTests { } + @ServletEndpoint(id = "testservlet") + @Validated + static class TestProxyServletEndpoint implements Supplier { + + @Override + public EndpointServlet get() { + return new EndpointServlet(TestServlet.class); + } + + } + @Endpoint(id = "test") static class TestEndpoint {