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

Unwrap a validator instance of specified type contained in SpringValidatorAdapter #37119

Conversation

zpavloudis
Copy link
Contributor

@zpavloudis zpavloudis commented Aug 28, 2023

Hi Spring boot team!

This PR attempts to close gh-37081 by implementing the unwrap method in boot/autoconfigure/validation/ValidatorAdapter provided by Spring's SmartValidator interface which was added in consequence to Spring's gh-31082.

It has been tested against the sample project provided in gh-31082 and returns a Bad Request as long as the @Validated annotation is removed from class-level.

2023-08-28T19:55:01.610+03:00  INFO 14176 --- [           main] com.example.demo.DemoApplication         : Started DemoApplication in 3.059 seconds (process running for 3.702)
2023-08-28T19:55:10.613+03:00 DEBUG 14176 --- [ctor-http-nio-3] o.s.w.s.adapter.HttpWebHandlerAdapter    : [01873b0d-1] HTTP GET "/class/-1"
2023-08-28T19:55:10.642+03:00 DEBUG 14176 --- [ctor-http-nio-3] s.w.r.r.m.a.RequestMappingHandlerMapping : [01873b0d-1] Mapped to com.example.demo.ClassLevelController#getEntity(Long)
2023-08-28T19:55:10.849+03:00 DEBUG 14176 --- [ctor-http-nio-3] org.springframework.web.HttpLogging      : [01873b0d-1] Resolved [HandlerMethodValidationException: 400 BAD_REQUEST "Validation failure"] for HTTP GET /class/-1
2023-08-28T19:55:10.892+03:00 DEBUG 14176 --- [ctor-http-nio-3] org.springframework.web.HttpLogging      : [01873b0d-1] Encoding [{timestamp=Mon Aug 28 19:55:10 EEST 2023, path=/class/-1, status=400, error=Bad Request, requestId=0 (truncated)...]
2023-08-28T19:55:10.938+03:00 DEBUG 14176 --- [ctor-http-nio-3] o.s.w.s.adapter.HttpWebHandlerAdapter    : [01873b0d-1] Completed 400 BAD_REQUEST
2023-08-28T19:55:28.995+03:00 DEBUG 14176 --- [ctor-http-nio-3] o.s.w.s.adapter.HttpWebHandlerAdapter    : [01873b0d-2] HTTP GET "/method/-1"
2023-08-28T19:55:28.996+03:00 DEBUG 14176 --- [ctor-http-nio-3] s.w.r.r.m.a.RequestMappingHandlerMapping : [01873b0d-2] Mapped to com.example.demo.MethodLevelController#getEntity(Long)
2023-08-28T19:55:29.003+03:00 DEBUG 14176 --- [ctor-http-nio-3] org.springframework.web.HttpLogging      : [01873b0d-2] Resolved [HandlerMethodValidationException: 400 BAD_REQUEST "Validation failure"] for HTTP GET /method/-1
2023-08-28T19:55:29.004+03:00 DEBUG 14176 --- [ctor-http-nio-3] org.springframework.web.HttpLogging      : [01873b0d-2] Encoding [{timestamp=Mon Aug 28 19:55:29 EEST 2023, path=/method/-1, status=400, error=Bad Request, requestId= (truncated)...]
2023-08-28T19:55:29.005+03:00 DEBUG 14176 --- [ctor-http-nio-3] o.s.w.s.adapter.HttpWebHandlerAdapter    : [01873b0d-2] Completed 400 BAD_REQUEST

However it appears that boot.autoconfigure.validation.ValidationAutoConfigurationTests#methodValidationPostProcessorValidatorDependencyDoesNotTriggerEarlyInitialization fails when building against latest Spring 6.1.0-SNAPSHOT logging multiple warnings of

Bean 'org.springframework.boot.autoconfigure.validation.ValidationAutoConfigurationTests$CustomValidatorConfiguration$SomeServiceConfiguration' of type [org.springframework.boot.autoconfigure.validation.ValidationAutoConfigurationTests$CustomValidatorConfiguration$SomeServiceConfiguration] is not eligible for getting processed by all BeanPostProcessors (for example: not eligible for auto-proxying). Is this bean getting eagerly injected into a currently created BeanPostProcessor [methodValidationPostProcessor]

I've taken the liberty in submitting this PR as the above test also fails when not having my changes incorporated, I am not sure whether this is deemed an issue in Spring or in Spring boot but I think it shows some resemblence to Spring's gh-24553.

@spring-projects-issues spring-projects-issues added the status: waiting-for-triage An issue we've not yet triaged label Aug 28, 2023
@wilkinsona wilkinsona self-assigned this Aug 30, 2023
@wilkinsona
Copy link
Member

Thanks, @zpavloudis. I've opened spring-projects/spring-framework#31137 for the test failure caused by the Framework upgrade.

@wilkinsona
Copy link
Member

@zpavloudis there are several commits here that are unrelated to the validator unwrapping. Could you please try rebasing your changes on the HEAD of the main branch and force pushing?

@wilkinsona wilkinsona added the status: waiting-for-feedback We need additional information before we can continue label Sep 4, 2023
@zpavloudis
Copy link
Contributor Author

@wilkinsona Now I do understand this is not the place to ask the why's and how's of git rebase but I have git rebase upstream/main and git came back with

First, rewinding head to replay your work on top of it... Applying: Unwrap a validator instance of specified type contained in SpringValidatorAdapter

and when attempting to push to origin - not via the terminal but through intellij's push dialog the list still reports several unrelated commits, as if it wants to update the forked branch with all the latest commits adding my work on top of it.

I can certainly try a force push but would this not lead to the same situation as before?

@spring-projects-issues spring-projects-issues added status: feedback-provided Feedback has been provided and removed status: waiting-for-feedback We need additional information before we can continue labels Sep 4, 2023
@wilkinsona
Copy link
Member

You might need to do a git rebase --interactive and then delete all of the commits that aren't related to your change.

@zpavloudis zpavloudis force-pushed the expose-inner-validator-gh-37081 branch from 2399ff6 to fd16ae7 Compare September 4, 2023 12:27
@zpavloudis
Copy link
Contributor Author

@wilkinsona Thank you for your feedback. looks like that's done it!

Copy link
Member

@wilkinsona wilkinsona left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks again for the PR, @zpavloudis. I've left a few comments for your consideration. It would also be good to test the unwrapping. Would you like to do that by adding a few tests to ValidatorAdapterTests?

@zpavloudis
Copy link
Contributor Author

Thanks again for the PR, @zpavloudis. I've left a few comments for your consideration. It would also be good to test the unwrapping. Would you like to do that by adding a few tests to ValidatorAdapterTests?

Thank your for your feedback.

I've opted for 1 test that runs through the unwrap method asserting that target is unwrapped to a Jakarta Validator as well as asserting an IllegalArgumentException is thrown when an unsupported type is passed.

I could be missing something so I remain open to suggestions.

@wilkinsona wilkinsona added type: enhancement A general enhancement and removed status: waiting-for-triage An issue we've not yet triaged status: feedback-provided Feedback has been provided labels Sep 11, 2023
@wilkinsona wilkinsona modified the milestones: 3.2.x, 3.2.0-M3 Sep 11, 2023
wilkinsona pushed a commit that referenced this pull request Sep 11, 2023
wilkinsona added a commit that referenced this pull request Sep 11, 2023
@wilkinsona
Copy link
Member

Thanks very much for making your first contribution to Spring Boot, @zpavloudis.

I polished your changes a little bit:

  • split the two tests into separate methods
  • add a test for a SmartValidator that's wrapped multiple times
  • fix the unwrap implementation to handle multiple wrapping

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

Successfully merging this pull request may close these issues.

Expose SpringValidatorAdapter contained in ValidatorAdapter via new SmartValidator method
3 participants