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

Author: wilkinsona

spring-boot-actuator/src/main/java/org/springframework/boot/actuate/endpoint/ConfigurationPropertiesReportEndpoint.java

@@ -23,10 +23,12 @@
 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.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 @@
 public class ConfigurationPropertiesReportEndpoint
 		extends AbstractEndpoint<Map<String, Object>> 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 @@ private Map<String, Object> safeSerialize(ObjectMapper mapper, Object bean,
 	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 @@ private void applySerializationModifier(ObjectMapper mapper) {
 		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,25 +275,35 @@ else if (item instanceof List) {
 	 * 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;
 		}
 
 	}
 
 	/**
-	 * {@link SimpleBeanPropertyFilter} to filter out all bean properties whose names
-	 * start with '$$'.
+	 * {@link SimpleBeanPropertyFilter} for serialization of
+	 * {@link ConfigurationProperties} beans. The filter hides:
+	 *
+	 * <ul>
+	 * <li>Properties that have a name starting with '$$'.
+	 * <li>Properties that are self-referential.
+	 * <li>Properties that throw an exception when retrieving their value.
+	 * </ul>
 	 */
-	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 @@ private boolean include(String name) {
 			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);
+		}
 	}
 
 	/**

spring-boot-actuator/src/test/java/org/springframework/boot/actuate/endpoint/ConfigurationPropertiesReportEndpointSerializationTests.java

@@ -22,6 +22,7 @@
 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 void testNestedNaming() throws Exception {
 
 	@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 void testCycle() throws Exception {
 		Map<String, Object> map = (Map<String, Object>) 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<String, Object> bar = (Map<String, Object>) 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<String, Object> properties = report.invoke();
+		Map<String, Object> nestedProperties = (Map<String, Object>) properties
+				.get("cycle");
+		assertThat(nestedProperties).isNotNull();
+		assertThat(nestedProperties.get("prefix")).isEqualTo("cycle");
+		Map<String, Object> map = (Map<String, Object>) nestedProperties
+				.get("properties");
+		assertThat(map).isNotNull();
+		assertThat(map).containsOnlyKeys("error");
+		assertThat(map).containsEntry("error", "Cannot serialize 'cycle'");
 	}
 
 	@Test
@@ -149,7 +172,6 @@ public void testEmptyMapIsNotAdded() throws Exception {
 		Map<String, Object> nestedProperties = (Map<String, Object>) properties
 				.get("foo");
 		assertThat(nestedProperties).isNotNull();
-		System.err.println(nestedProperties);
 		assertThat(nestedProperties.get("prefix")).isEqualTo("foo");
 		Map<String, Object> map = (Map<String, Object>) nestedProperties
 				.get("properties");
@@ -190,7 +212,6 @@ public void testInetAddress() throws Exception {
 		Map<String, Object> nestedProperties = (Map<String, Object>) properties
 				.get("foo");
 		assertThat(nestedProperties).isNotNull();
-		System.err.println(nestedProperties);
 		assertThat(nestedProperties.get("prefix")).isEqualTo("foo");
 		Map<String, Object> map = (Map<String, Object>) nestedProperties
 				.get("properties");
@@ -223,6 +244,20 @@ public void testInitializedMapAndList() throws Exception {
 		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<String, Object> properties = endpoint.invoke();
+		Map<String, Object> nestedProperties = (Map<String, Object>) ((Map<String, Object>) properties
+				.get("hikariDataSource")).get("properties");
+		assertThat(nestedProperties).doesNotContainKey("error");
+	}
+
 	@Configuration
 	@EnableConfigurationProperties
 	public static class Base {
@@ -248,24 +283,12 @@ public Foo foo() {
 
 	@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 void setName(String name) {
 
 	}
 
-	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 List<String> getList() {
 
 	}
 
+	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();
+		}
+
+	}
+
 }