Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Validation is not applied for ConfigurationProperties that implement Validator and use @ConstructorBinding #33669

Closed
fstaudt opened this issue Jan 3, 2023 · 3 comments
Labels
type: bug A general bug
Milestone

Comments

@fstaudt
Copy link

fstaudt commented Jan 3, 2023

In this article (https://reflectoring.io/validate-spring-boot-configuration-parameters-at-startup/ ), it is advised to implement Validator interface in class annotated with @ConfigurationProperties for complex checks.

This mechanism works well when class is not annotated with @ConstructorBinding.
It doesn't work if class is annotated with @ConstructorBinding or if class is implemented as an immutable class in Spring Boot 3.0.0.

I've reproduced the issue in a Java project on this github repository for Spring Boot 2.7.7 (branch sb27) and for Spring Boot 3.0.0 (branch master).

In test SimpleDocumentationPropertiesTest, validate method of class SimpleDocumentationProperties is well called.
This class is a simple POJO with @ConfigurationProperties annotation.

In test ConstructorBindingDocumentationPropertiesTest, test fails because validate method of class ConstructorBindingDocumentationProperties is not called.
This class is implemented as an immutable class with same @ConfigurationProperties annotation and additional @ConstructorBinding annotation (for Spring Boot 2).

Issue can also be reproduced in Kotlin projects when ConfigurationProperties classes are implemented with data classes.

I'd like to use immutable classes for all my configuration properties classes and still be able to use Validator interface for complex checks.

@spring-projects-issues spring-projects-issues added the status: waiting-for-triage An issue we've not yet triaged label Jan 3, 2023
@fstaudt
Copy link
Author

fstaudt commented Jan 3, 2023

First analysis points out to these lines:
https://github.com/spring-projects/spring-boot/blob/main/spring-boot-project/spring-boot/src/main/java/org/springframework/boot/context/properties/ConfigurationPropertiesBinder.java#L146-L148

I believe following change would fix the issue (to be tested):

  if (Validator.class.isAssignableFrom(target.type.getRawClass())) {
    if (target.getValue() != null) {
      validators.add((Validator) target.getValue().get());
    } else {
      validators.add((Validator) getBinder().bindOrCreate("validator", target));
    }
  }

@philwebb philwebb changed the title validate not called when ConfigurationProperties with @ConstructorBinding implements Validator Validation is not applied for ConfigurationProperties that implement Validator and use @ConstructorBinding Jan 18, 2023
@philwebb philwebb added type: bug A general bug and removed status: waiting-for-triage An issue we've not yet triaged labels Jan 18, 2023
@philwebb philwebb added this to the 2.7.x milestone Jan 18, 2023
@philwebb
Copy link
Member

I think this is a bug, but for what it's worth I don't think it makes as much sense to use Validator with constructor binding.

For setter based binding, it's useful to have the validate callback that is called once all the setters have been called. For constructor binding, you should already have all the data required for validation in the constructor parameters. I would do validation in the constructor and throw an exception there to prevent the object from being created in an invalid state.

@fstaudt
Copy link
Author

fstaudt commented Jan 23, 2023

Validate callback also eases aggregation of multiple validation errors in one response for a given properties class.
The same benefit can be achieved with BeanPropertyBindingResult and BindException with some additional boiler plate.

Validate callback also provides out-of-the-box a clear error message in application logs:

***************************
APPLICATION FAILED TO START
***************************

Description:

Binding to target org.springframework.boot.context.properties.bind.BindException: Failed to bind properties under 'documentation' to com.example.cbcpvalidator.SimpleDocumentationProperties failed:

    Property: documentation.url
    Value: "http://docs.spring.io/spring-boot/docs/current/reference/htmlsingle/"
    Origin: class path resource [application.yml] - 2:8
    Reason: Documentation URL must start with https://.


Action:

Update your application's configuration

I was not able to get this clear error message when throwing explicitly a BindException.

In this context, it would still make sense to use Validator with constructor binding.
Would it be OK if I provide a PR to fix this bug ?

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
type: bug A general bug
Projects
None yet
Development

No branches or pull requests

4 participants