From 85dc89e1b448fa5f9ed2ac9786147d6a29fe1a5f Mon Sep 17 00:00:00 2001 From: Andy Wilkinson Date: Wed, 15 Nov 2017 18:58:39 +0000 Subject: [PATCH] Make serialization of @ConfigurationProperties beans more defensive Previously, serialization of a @ConfigurationProperties bean to JSON would fail if: - A property on the bean returned the bean (the bean was self-referential) - An exception was thrown when attempting to retrieve a property's value. This commit makes the serialization more defensive by skipping any property that is affected by either of the problems described above. Debug logging has been added to aid diagnosis of missing properties. Closes gh-10846 --- ...ConfigurationPropertiesReportEndpoint.java | 67 +++++++--- ...rtiesReportEndpointSerializationTests.java | 122 ++++++++++++++---- 2 files changed, 150 insertions(+), 39 deletions(-) diff --git a/spring-boot-actuator/src/main/java/org/springframework/boot/actuate/endpoint/ConfigurationPropertiesReportEndpoint.java b/spring-boot-actuator/src/main/java/org/springframework/boot/actuate/endpoint/ConfigurationPropertiesReportEndpoint.java index 2b89e4e901..31d148bf4c 100644 --- a/spring-boot-actuator/src/main/java/org/springframework/boot/actuate/endpoint/ConfigurationPropertiesReportEndpoint.java +++ b/spring-boot-actuator/src/main/java/org/springframework/boot/actuate/endpoint/ConfigurationPropertiesReportEndpoint.java @@ -23,10 +23,12 @@ import java.util.HashMap; import java.util.List; import java.util.Map; +import com.fasterxml.jackson.core.JsonGenerator; import com.fasterxml.jackson.databind.BeanDescription; import com.fasterxml.jackson.databind.ObjectMapper; import com.fasterxml.jackson.databind.SerializationConfig; import com.fasterxml.jackson.databind.SerializationFeature; +import com.fasterxml.jackson.databind.SerializerProvider; import com.fasterxml.jackson.databind.introspect.Annotated; import com.fasterxml.jackson.databind.introspect.AnnotatedMethod; import com.fasterxml.jackson.databind.introspect.JacksonAnnotationIntrospector; @@ -37,6 +39,8 @@ import com.fasterxml.jackson.databind.ser.PropertyWriter; import com.fasterxml.jackson.databind.ser.SerializerFactory; import com.fasterxml.jackson.databind.ser.impl.SimpleBeanPropertyFilter; import com.fasterxml.jackson.databind.ser.impl.SimpleFilterProvider; +import org.apache.commons.logging.Log; +import org.apache.commons.logging.LogFactory; import org.springframework.beans.BeansException; import org.springframework.boot.context.properties.ConfigurationBeanFactoryMetaData; @@ -64,7 +68,7 @@ import org.springframework.util.StringUtils; public class ConfigurationPropertiesReportEndpoint extends AbstractEndpoint> implements ApplicationContextAware { - private static final String CGLIB_FILTER_ID = "cglibFilter"; + private static final String CONFIGURATION_PROPERTIES_FILTER_ID = "configurationPropertiesFilter"; private final Sanitizer sanitizer = new Sanitizer(); @@ -174,7 +178,7 @@ public class ConfigurationPropertiesReportEndpoint protected void configureObjectMapper(ObjectMapper mapper) { mapper.configure(SerializationFeature.FAIL_ON_EMPTY_BEANS, false); mapper.configure(SerializationFeature.WRITE_NULL_MAP_VALUES, false); - applyCglibFilters(mapper); + applyConfigurationPropertiesFilter(mapper); applySerializationModifier(mapper); } @@ -188,15 +192,11 @@ public class ConfigurationPropertiesReportEndpoint mapper.setSerializerFactory(factory); } - /** - * Configure PropertyFilter to make sure Jackson doesn't process CGLIB generated bean - * properties. - * @param mapper the object mapper - */ - private void applyCglibFilters(ObjectMapper mapper) { - mapper.setAnnotationIntrospector(new CglibAnnotationIntrospector()); - mapper.setFilterProvider(new SimpleFilterProvider().addFilter(CGLIB_FILTER_ID, - new CglibBeanPropertyFilter())); + private void applyConfigurationPropertiesFilter(ObjectMapper mapper) { + mapper.setAnnotationIntrospector( + new ConfigurationPropertiesAnnotationIntrospector()); + mapper.setFilterProvider(new SimpleFilterProvider() + .setDefaultFilter(new ConfigurationPropertiesPropertyFilter())); } /** @@ -275,14 +275,14 @@ public class ConfigurationPropertiesReportEndpoint * properties. */ @SuppressWarnings("serial") - private static class CglibAnnotationIntrospector + private static class ConfigurationPropertiesAnnotationIntrospector extends JacksonAnnotationIntrospector { @Override public Object findFilterId(Annotated a) { Object id = super.findFilterId(a); if (id == null) { - id = CGLIB_FILTER_ID; + id = CONFIGURATION_PROPERTIES_FILTER_ID; } return id; } @@ -290,10 +290,20 @@ public class ConfigurationPropertiesReportEndpoint } /** - * {@link SimpleBeanPropertyFilter} to filter out all bean properties whose names - * start with '$$'. + * {@link SimpleBeanPropertyFilter} for serialization of + * {@link ConfigurationProperties} beans. The filter hides: + * + * */ - private static class CglibBeanPropertyFilter extends SimpleBeanPropertyFilter { + private static class ConfigurationPropertiesPropertyFilter + extends SimpleBeanPropertyFilter { + + private static final Log logger = LogFactory + .getLog(ConfigurationPropertiesPropertyFilter.class); @Override protected boolean include(BeanPropertyWriter writer) { @@ -309,6 +319,31 @@ public class ConfigurationPropertiesReportEndpoint return !name.startsWith("$$"); } + @Override + public void serializeAsField(Object pojo, JsonGenerator jgen, + SerializerProvider provider, PropertyWriter writer) throws Exception { + if (writer instanceof BeanPropertyWriter) { + try { + if (pojo == ((BeanPropertyWriter) writer).get(pojo)) { + if (logger.isDebugEnabled()) { + logger.debug("Skipping '" + writer.getFullName() + "' on '" + + pojo.getClass().getName() + + "' as it is self-referential"); + } + return; + } + } + catch (Exception ex) { + if (logger.isDebugEnabled()) { + logger.debug("Skipping '" + writer.getFullName() + "' on '" + + pojo.getClass().getName() + "' as an exception " + + "was thrown when retrieving its value", ex); + } + return; + } + } + super.serializeAsField(pojo, jgen, provider, writer); + } } /** diff --git a/spring-boot-actuator/src/test/java/org/springframework/boot/actuate/endpoint/ConfigurationPropertiesReportEndpointSerializationTests.java b/spring-boot-actuator/src/test/java/org/springframework/boot/actuate/endpoint/ConfigurationPropertiesReportEndpointSerializationTests.java index 8042634ecf..d79f52ad72 100644 --- a/spring-boot-actuator/src/test/java/org/springframework/boot/actuate/endpoint/ConfigurationPropertiesReportEndpointSerializationTests.java +++ b/spring-boot-actuator/src/test/java/org/springframework/boot/actuate/endpoint/ConfigurationPropertiesReportEndpointSerializationTests.java @@ -22,6 +22,7 @@ import java.util.HashMap; import java.util.List; import java.util.Map; +import com.zaxxer.hikari.HikariDataSource; import org.junit.After; import org.junit.Before; import org.junit.Test; @@ -100,8 +101,8 @@ public class ConfigurationPropertiesReportEndpointSerializationTests { @Test @SuppressWarnings("unchecked") - public void testCycle() throws Exception { - this.context.register(CycleConfig.class); + public void testSelfReferentialProperty() throws Exception { + this.context.register(SelfReferentialConfig.class); EnvironmentTestUtils.addEnvironment(this.context, "foo.name:foo"); this.context.refresh(); ConfigurationPropertiesReportEndpoint report = this.context @@ -114,8 +115,30 @@ public class ConfigurationPropertiesReportEndpointSerializationTests { Map map = (Map) nestedProperties .get("properties"); assertThat(map).isNotNull(); - assertThat(map).hasSize(1); - assertThat(map.get("error")).isEqualTo("Cannot serialize 'foo'"); + assertThat(map).containsOnlyKeys("bar", "name"); + assertThat(map).containsEntry("name", "foo"); + Map bar = (Map) map.get("bar"); + assertThat(bar).containsOnlyKeys("name"); + assertThat(bar).containsEntry("name", "123456"); + } + + @Test + @SuppressWarnings("unchecked") + public void testCycle() { + this.context.register(CycleConfig.class); + this.context.refresh(); + ConfigurationPropertiesReportEndpoint report = this.context + .getBean(ConfigurationPropertiesReportEndpoint.class); + Map properties = report.invoke(); + Map nestedProperties = (Map) properties + .get("cycle"); + assertThat(nestedProperties).isNotNull(); + assertThat(nestedProperties.get("prefix")).isEqualTo("cycle"); + Map map = (Map) nestedProperties + .get("properties"); + assertThat(map).isNotNull(); + assertThat(map).containsOnlyKeys("error"); + assertThat(map).containsEntry("error", "Cannot serialize 'cycle'"); } @Test @@ -149,7 +172,6 @@ public class ConfigurationPropertiesReportEndpointSerializationTests { Map nestedProperties = (Map) properties .get("foo"); assertThat(nestedProperties).isNotNull(); - System.err.println(nestedProperties); assertThat(nestedProperties.get("prefix")).isEqualTo("foo"); Map map = (Map) nestedProperties .get("properties"); @@ -190,7 +212,6 @@ public class ConfigurationPropertiesReportEndpointSerializationTests { Map nestedProperties = (Map) properties .get("foo"); assertThat(nestedProperties).isNotNull(); - System.err.println(nestedProperties); assertThat(nestedProperties.get("prefix")).isEqualTo("foo"); Map map = (Map) nestedProperties .get("properties"); @@ -223,6 +244,20 @@ public class ConfigurationPropertiesReportEndpointSerializationTests { assertThat(list).containsExactly("abc"); } + @Test + @SuppressWarnings("unchecked") + public void hikariDataSourceConfigurationPropertiesBeanCanBeSerialized() { + this.context = new AnnotationConfigApplicationContext(); + this.context.register(HikariDataSourceConfig.class); + this.context.refresh(); + ConfigurationPropertiesReportEndpoint endpoint = this.context + .getBean(ConfigurationPropertiesReportEndpoint.class); + Map properties = endpoint.invoke(); + Map nestedProperties = (Map) ((Map) properties + .get("hikariDataSource")).get("properties"); + assertThat(nestedProperties).doesNotContainKey("error"); + } + @Configuration @EnableConfigurationProperties public static class Base { @@ -248,24 +283,12 @@ public class ConfigurationPropertiesReportEndpointSerializationTests { @Configuration @Import(Base.class) - public static class CycleConfig { + public static class SelfReferentialConfig { @Bean @ConfigurationProperties(prefix = "foo") - public Cycle foo() { - return new Cycle(); - } - - } - - @Configuration - @Import(Base.class) - public static class MetadataCycleConfig { - - @Bean - @ConfigurationProperties(prefix = "bar") - public Cycle foo() { - return new Cycle(); + public SelfReferential foo() { + return new SelfReferential(); } } @@ -373,11 +396,11 @@ public class ConfigurationPropertiesReportEndpointSerializationTests { } - public static class Cycle extends Foo { + public static class SelfReferential extends Foo { private Foo self; - public Cycle() { + public SelfReferential() { this.self = this; } @@ -449,4 +472,57 @@ public class ConfigurationPropertiesReportEndpointSerializationTests { } + static class Cycle { + + private final Alpha alpha = new Alpha(this); + + public Alpha getAlpha() { + return this.alpha; + } + + static class Alpha { + + private final Cycle cycle; + + Alpha(Cycle cycle) { + this.cycle = cycle; + } + + public Cycle getCycle() { + return this.cycle; + } + + } + + } + + @Configuration + @Import(Base.class) + static class CycleConfig { + + @Bean + @ConfigurationProperties(prefix = "cycle") + public Cycle cycle() { + return new Cycle(); + } + + } + + @Configuration + @EnableConfigurationProperties + static class HikariDataSourceConfig { + + @Bean + public ConfigurationPropertiesReportEndpoint endpoint() { + return new ConfigurationPropertiesReportEndpoint(); + } + + @Bean + @ConfigurationProperties(prefix = "test.datasource") + public HikariDataSource hikariDataSource() { + return new HikariDataSource(); + } + + } + }