From 10dbf3c571258bcc788d4fd091aeb42b3c011da6 Mon Sep 17 00:00:00 2001 From: Phillip Webb Date: Wed, 18 Jan 2017 20:29:31 -0800 Subject: [PATCH] Use `@Validated` as trigger for JSR-330 validation Update `ConfigurationPropertiesBindingPostProcessor` so that `@Validated` is expected to be used to trigger JSR-330 validation. Any existing configuration classes that use JSR-330 annotations but don't have `@Validated` will currently still be validated, but will now log a warning. This should give users a chance to add the requested annotations before the next Spring Boot release where we will use them as the exclusive signal that validation is required. Closes gh-7579 --- .../main/asciidoc/spring-boot-features.adoc | 10 ++-- .../spring-boot-sample-simple/pom.xml | 4 ++ .../simple/SampleConfigurationProperties.java | 39 +++++++++++++++ .../src/main/resources/application.properties | 4 +- .../simple/SampleSimpleApplicationTests.java | 4 ++ .../src/test/resources/application.properties | 1 - .../bind/PropertiesConfigurationFactory.java | 7 +-- .../properties/ConfigurationProperties.java | 8 ++-- ...urationPropertiesBindingPostProcessor.java | 48 ++++++++++++++----- ...onPropertiesBindingPostProcessorTests.java | 2 + .../EnableConfigurationPropertiesTests.java | 37 ++++++++++++++ .../analyzer/BindFailureAnalyzerTests.java | 2 + 12 files changed, 142 insertions(+), 24 deletions(-) create mode 100644 spring-boot-samples/spring-boot-sample-simple/src/main/java/sample/simple/SampleConfigurationProperties.java delete mode 100644 spring-boot-samples/spring-boot-sample-simple/src/test/resources/application.properties diff --git a/spring-boot-docs/src/main/asciidoc/spring-boot-features.adoc b/spring-boot-docs/src/main/asciidoc/spring-boot-features.adoc index 9684b2870c..c773a776b2 100644 --- a/spring-boot-docs/src/main/asciidoc/spring-boot-features.adoc +++ b/spring-boot-docs/src/main/asciidoc/spring-boot-features.adoc @@ -1090,13 +1090,16 @@ only rely on custom converters qualified with `@ConfigurationPropertiesBinding`. [[boot-features-external-config-validation]] ==== @ConfigurationProperties Validation -Spring Boot will attempt to validate external configuration, by default using JSR-303 -(if it is on the classpath). You can simply add JSR-303 `javax.validation` constraint -annotations to your `@ConfigurationProperties` class: +Spring Boot will attempt to validate `@ConfigurationProperties` classes whenever they +annotated with Spring's `@Validated` annotation. You can use JSR-303 `javax.validation` +constraint annotations directly on your configuration class. Simply ensure that a +compliant JSR-303 implementation is on your classpath, then add constraint annotations to +your fields: [source,java,indent=0] ---- @ConfigurationProperties(prefix="foo") + @Validated public class FooProperties { @NotNull @@ -1114,6 +1117,7 @@ as `@Valid` to trigger its validation. For example, building upon the above [source,java,indent=0] ---- @ConfigurationProperties(prefix="connection") + @Validated public class FooProperties { @NotNull diff --git a/spring-boot-samples/spring-boot-sample-simple/pom.xml b/spring-boot-samples/spring-boot-sample-simple/pom.xml index 7fccbaab2d..9387a641ee 100644 --- a/spring-boot-samples/spring-boot-sample-simple/pom.xml +++ b/spring-boot-samples/spring-boot-sample-simple/pom.xml @@ -24,6 +24,10 @@ org.springframework.boot spring-boot-starter + + org.hibernate + hibernate-validator + org.springframework.boot diff --git a/spring-boot-samples/spring-boot-sample-simple/src/main/java/sample/simple/SampleConfigurationProperties.java b/spring-boot-samples/spring-boot-sample-simple/src/main/java/sample/simple/SampleConfigurationProperties.java new file mode 100644 index 0000000000..566e9010c5 --- /dev/null +++ b/spring-boot-samples/spring-boot-sample-simple/src/main/java/sample/simple/SampleConfigurationProperties.java @@ -0,0 +1,39 @@ +/* + * Copyright 2012-2017 the original author or authors. + * + * Licensed under the Apache License, Version 2.0 (the "License"); + * you may not use this file except in compliance with the License. + * You may obtain a copy of the License at + * + * http://www.apache.org/licenses/LICENSE-2.0 + * + * Unless required by applicable law or agreed to in writing, software + * distributed under the License is distributed on an "AS IS" BASIS, + * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. + * See the License for the specific language governing permissions and + * limitations under the License. + */ + +package sample.simple; + +import javax.validation.constraints.NotNull; + +import org.springframework.boot.context.properties.ConfigurationProperties; +import org.springframework.stereotype.Component; + +@Component +@ConfigurationProperties(prefix = "sample") +public class SampleConfigurationProperties { + + @NotNull + private String name; + + public String getName() { + return this.name; + } + + public void setName(String name) { + this.name = name; + } + +} diff --git a/spring-boot-samples/spring-boot-sample-simple/src/main/resources/application.properties b/spring-boot-samples/spring-boot-sample-simple/src/main/resources/application.properties index b04cdc39b5..9964fe3399 100644 --- a/spring-boot-samples/spring-boot-sample-simple/src/main/resources/application.properties +++ b/spring-boot-samples/spring-boot-sample-simple/src/main/resources/application.properties @@ -1 +1,3 @@ -name: Phil +name=Phil +sample.name=Andy + diff --git a/spring-boot-samples/spring-boot-sample-simple/src/test/java/sample/simple/SampleSimpleApplicationTests.java b/spring-boot-samples/spring-boot-sample-simple/src/test/java/sample/simple/SampleSimpleApplicationTests.java index 057e0f761d..3591b21031 100644 --- a/spring-boot-samples/spring-boot-sample-simple/src/test/java/sample/simple/SampleSimpleApplicationTests.java +++ b/spring-boot-samples/spring-boot-sample-simple/src/test/java/sample/simple/SampleSimpleApplicationTests.java @@ -58,6 +58,10 @@ public class SampleSimpleApplicationTests { SampleSimpleApplication.main(new String[0]); String output = this.outputCapture.toString(); assertThat(output).contains("Hello Phil"); + assertThat(output).contains("The @ConfigurationProperties bean class " + + "sample.simple.SampleConfigurationProperties contains " + + "validation constraints but had not been annotated " + + "with @Validated"); } @Test diff --git a/spring-boot-samples/spring-boot-sample-simple/src/test/resources/application.properties b/spring-boot-samples/spring-boot-sample-simple/src/test/resources/application.properties deleted file mode 100644 index 4dfe84cedc..0000000000 --- a/spring-boot-samples/spring-boot-sample-simple/src/test/resources/application.properties +++ /dev/null @@ -1 +0,0 @@ -name: Phil \ No newline at end of file diff --git a/spring-boot/src/main/java/org/springframework/boot/bind/PropertiesConfigurationFactory.java b/spring-boot/src/main/java/org/springframework/boot/bind/PropertiesConfigurationFactory.java index 13f305da05..eca9522251 100644 --- a/spring-boot/src/main/java/org/springframework/boot/bind/PropertiesConfigurationFactory.java +++ b/spring-boot/src/main/java/org/springframework/boot/bind/PropertiesConfigurationFactory.java @@ -267,8 +267,9 @@ public class PropertiesConfigurationFactory relaxedTargetNames); dataBinder.bind(propertyValues); if (this.validator != null) { - validate(dataBinder); + dataBinder.validate(); } + checkForBindingErrors(dataBinder); } private Iterable getRelaxedTargetNames() { @@ -338,8 +339,8 @@ public class PropertiesConfigurationFactory return this.target != null && Map.class.isAssignableFrom(this.target.getClass()); } - private void validate(RelaxedDataBinder dataBinder) throws BindException { - dataBinder.validate(); + private void checkForBindingErrors(RelaxedDataBinder dataBinder) + throws BindException { BindingResult errors = dataBinder.getBindingResult(); if (errors.hasErrors()) { logger.error("Properties configuration failed validation"); diff --git a/spring-boot/src/main/java/org/springframework/boot/context/properties/ConfigurationProperties.java b/spring-boot/src/main/java/org/springframework/boot/context/properties/ConfigurationProperties.java index f6734e8c54..b7a7447bde 100644 --- a/spring-boot/src/main/java/org/springframework/boot/context/properties/ConfigurationProperties.java +++ b/spring-boot/src/main/java/org/springframework/boot/context/properties/ConfigurationProperties.java @@ -23,6 +23,7 @@ import java.lang.annotation.RetentionPolicy; import java.lang.annotation.Target; import org.springframework.core.annotation.AliasFor; +import org.springframework.validation.annotation.Validated; /** * Annotation for externalized configuration. Add this to a class definition or a @@ -80,9 +81,10 @@ public @interface ConfigurationProperties { boolean ignoreUnknownFields() default true; /** - * Flag to indicate that an exception should be raised if a Validator is available and - * validation fails. If it is set to false, validation errors will be swallowed. They - * will be logged, but not propagated to the caller. + * Flag to indicate that an exception should be raised if a Validator is available, + * the class is annotated with {@link Validated @Validated} and validation fails. If + * it is set to false, validation errors will be swallowed. They will be logged, but + * not propagated to the caller. * @return the flag value (default true) */ boolean exceptionIfInvalid() default true; diff --git a/spring-boot/src/main/java/org/springframework/boot/context/properties/ConfigurationPropertiesBindingPostProcessor.java b/spring-boot/src/main/java/org/springframework/boot/context/properties/ConfigurationPropertiesBindingPostProcessor.java index 3bf2c3d01e..d704eec7c7 100644 --- a/spring-boot/src/main/java/org/springframework/boot/context/properties/ConfigurationPropertiesBindingPostProcessor.java +++ b/spring-boot/src/main/java/org/springframework/boot/context/properties/ConfigurationPropertiesBindingPostProcessor.java @@ -45,6 +45,7 @@ import org.springframework.context.event.ContextRefreshedEvent; import org.springframework.context.support.PropertySourcesPlaceholderConfigurer; import org.springframework.core.Ordered; import org.springframework.core.PriorityOrdered; +import org.springframework.core.annotation.AnnotatedElementUtils; import org.springframework.core.annotation.AnnotationUtils; import org.springframework.core.convert.ConversionService; import org.springframework.core.convert.converter.Converter; @@ -61,6 +62,7 @@ import org.springframework.util.ClassUtils; import org.springframework.util.StringUtils; import org.springframework.validation.Errors; import org.springframework.validation.Validator; +import org.springframework.validation.annotation.Validated; import org.springframework.validation.beanvalidation.LocalValidatorFactoryBean; /** @@ -362,8 +364,8 @@ public class ConfigurationPropertiesBindingPostProcessor implements BeanPostProc return this.validator; } if (this.localValidator == null && isJsr303Present()) { - this.localValidator = new LocalValidatorFactory() - .run(this.applicationContext); + this.localValidator = new ValidatedLocalValidatorFactoryBean( + this.applicationContext); } return this.localValidator; } @@ -394,18 +396,38 @@ public class ConfigurationPropertiesBindingPostProcessor implements BeanPostProc } /** - * Factory to create JSR 303 LocalValidatorFactoryBean. Inner class to prevent class - * loader issues. + * {@link LocalValidatorFactoryBean} supports classes annotated with + * {@link Validated @Validated}. */ - private static class LocalValidatorFactory { - - public Validator run(ApplicationContext applicationContext) { - LocalValidatorFactoryBean validator = new LocalValidatorFactoryBean(); - MessageInterpolatorFactory interpolatorFactory = new MessageInterpolatorFactory(); - validator.setApplicationContext(applicationContext); - validator.setMessageInterpolator(interpolatorFactory.getObject()); - validator.afterPropertiesSet(); - return validator; + private static class ValidatedLocalValidatorFactoryBean + extends LocalValidatorFactoryBean { + + private static final Log logger = LogFactory + .getLog(ConfigurationPropertiesBindingPostProcessor.class); + + ValidatedLocalValidatorFactoryBean(ApplicationContext applicationContext) { + setApplicationContext(applicationContext); + setMessageInterpolator(new MessageInterpolatorFactory().getObject()); + afterPropertiesSet(); + } + + @Override + public boolean supports(Class type) { + if (!super.supports(type)) { + return false; + } + if (AnnotatedElementUtils.isAnnotated(type, Validated.class)) { + return true; + } + if (type.getPackage().getName().startsWith("org.springframework.boot")) { + return false; + } + if (getConstraintsForClass(type).isBeanConstrained()) { + logger.warn("The @ConfigurationProperties bean " + type + + " contains validation constraints but had not been annotated " + + "with @Validated."); + } + return true; } } diff --git a/spring-boot/src/test/java/org/springframework/boot/context/properties/ConfigurationPropertiesBindingPostProcessorTests.java b/spring-boot/src/test/java/org/springframework/boot/context/properties/ConfigurationPropertiesBindingPostProcessorTests.java index 48c98d4509..b653723429 100644 --- a/spring-boot/src/test/java/org/springframework/boot/context/properties/ConfigurationPropertiesBindingPostProcessorTests.java +++ b/spring-boot/src/test/java/org/springframework/boot/context/properties/ConfigurationPropertiesBindingPostProcessorTests.java @@ -46,6 +46,7 @@ import org.springframework.validation.BindException; import org.springframework.validation.Errors; import org.springframework.validation.ValidationUtils; import org.springframework.validation.Validator; +import org.springframework.validation.annotation.Validated; import static org.assertj.core.api.Assertions.assertThat; import static org.junit.Assert.fail; @@ -456,6 +457,7 @@ public class ConfigurationPropertiesBindingPostProcessorTests { } @ConfigurationProperties(prefix = "test") + @Validated public static class PropertyWithJSR303 extends PropertyWithoutJSR303 { @NotNull diff --git a/spring-boot/src/test/java/org/springframework/boot/context/properties/EnableConfigurationPropertiesTests.java b/spring-boot/src/test/java/org/springframework/boot/context/properties/EnableConfigurationPropertiesTests.java index 492e2f81a1..93c253c708 100644 --- a/spring-boot/src/test/java/org/springframework/boot/context/properties/EnableConfigurationPropertiesTests.java +++ b/spring-boot/src/test/java/org/springframework/boot/context/properties/EnableConfigurationPropertiesTests.java @@ -38,6 +38,7 @@ import org.springframework.core.env.MutablePropertySources; import org.springframework.stereotype.Component; import org.springframework.test.context.support.TestPropertySourceUtils; import org.springframework.validation.BindException; +import org.springframework.validation.annotation.Validated; import static org.assertj.core.api.Assertions.assertThat; @@ -172,6 +173,17 @@ public class EnableConfigurationPropertiesTests { this.context.refresh(); } + @Test + public void testNoExceptionOnValidationWithoutValidated() { + this.context.register(IgnoredIfInvalidButNotValidatedTestConfiguration.class); + TestPropertySourceUtils.addInlinedPropertiesToEnvironment(this.context, + "name:foo"); + this.context.refresh(); + IgnoredIfInvalidButNotValidatedTestProperties bean = this.context + .getBean(IgnoredIfInvalidButNotValidatedTestProperties.class); + assertThat(bean.getDescription()).isNull(); + } + @Test public void testNoExceptionOnValidation() { this.context.register(NoExceptionIfInvalidTestConfiguration.class); @@ -432,6 +444,12 @@ public class EnableConfigurationPropertiesTests { } + @Configuration + @EnableConfigurationProperties(IgnoredIfInvalidButNotValidatedTestProperties.class) + protected static class IgnoredIfInvalidButNotValidatedTestConfiguration { + + } + @Configuration @EnableConfigurationProperties(NoExceptionIfInvalidTestProperties.class) protected static class NoExceptionIfInvalidTestConfiguration { @@ -658,6 +676,7 @@ public class EnableConfigurationPropertiesTests { } @ConfigurationProperties + @Validated protected static class ExceptionIfInvalidTestProperties extends TestProperties { @NotNull @@ -673,7 +692,25 @@ public class EnableConfigurationPropertiesTests { } + @ConfigurationProperties + protected static class IgnoredIfInvalidButNotValidatedTestProperties + extends TestProperties { + + @NotNull + private String description; + + public String getDescription() { + return this.description; + } + + public void setDescription(String description) { + this.description = description; + } + + } + @ConfigurationProperties(exceptionIfInvalid = false) + @Validated protected static class NoExceptionIfInvalidTestProperties extends TestProperties { @NotNull diff --git a/spring-boot/src/test/java/org/springframework/boot/diagnostics/analyzer/BindFailureAnalyzerTests.java b/spring-boot/src/test/java/org/springframework/boot/diagnostics/analyzer/BindFailureAnalyzerTests.java index 93a34e0cc8..66f0f0bca2 100644 --- a/spring-boot/src/test/java/org/springframework/boot/diagnostics/analyzer/BindFailureAnalyzerTests.java +++ b/spring-boot/src/test/java/org/springframework/boot/diagnostics/analyzer/BindFailureAnalyzerTests.java @@ -32,6 +32,7 @@ import org.springframework.boot.context.properties.EnableConfigurationProperties import org.springframework.boot.diagnostics.FailureAnalysis; import org.springframework.context.annotation.AnnotationConfigApplicationContext; import org.springframework.context.i18n.LocaleContextHolder; +import org.springframework.validation.annotation.Validated; import static org.assertj.core.api.Assertions.assertThat; @@ -90,6 +91,7 @@ public class BindFailureAnalyzerTests { } @ConfigurationProperties("test.foo") + @Validated static class ValidationFailureProperties { @NotNull