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

Support method validation for an interface only proxy like an HTTP interface client #29782

Closed
DanielLiu1123 opened this issue Jan 8, 2023 · 7 comments
Assignees
Labels
in: web Issues in web modules (web, webmvc, webflux, websocket) type: enhancement A general enhancement
Milestone

Comments

@DanielLiu1123
Copy link

DanielLiu1123 commented Jan 8, 2023

I want to validate the params on client side, it works fine when using spring-cloud-openfeign, but it doesn't work when using HttpExchange.

@Validated
public interface EchoClient {
    @GetExchange("/echo")
    String echo(@RequestParam("message") @NotNull String message);
}
@spring-projects-issues spring-projects-issues added the status: waiting-for-triage An issue we've not yet triaged or decided on label Jan 8, 2023
@sbrannen
Copy link
Member

sbrannen commented Jan 8, 2023

Where does that @Validate annotation come from?

Do you perhaps mean Spring's @org.springframework.validation.annotation.Validated?

If so, have you registered an org.springframework.validation.beanvalidation.MethodValidationPostProcessor bean?

@sbrannen sbrannen added the status: waiting-for-feedback We need additional information before we can continue label Jan 8, 2023
@DanielLiu1123
Copy link
Author

Do you perhaps mean Spring's @org.springframework.validation.annotation.Validated?

yep~

If so, have you registered an org.springframework.validation.beanvalidation.MethodValidationPostProcessor bean?

I have spring-boot-starter-validation on classpath, so there is a MethodValidationPostProcessor bean.

I've looked into the proxy bean, there are two advisors, I think the second one is used to method validation, but it doesn't work.
image

@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 Jan 8, 2023
@sbrannen
Copy link
Member

sbrannen commented Jan 8, 2023

Thanks for the feedback. We'll look into it.

@sbrannen sbrannen added in: web Issues in web modules (web, webmvc, webflux, websocket) and removed status: feedback-provided Feedback has been provided labels Jan 8, 2023
@sbrannen sbrannen added this to the Triage Queue milestone Jan 8, 2023
@rstoyanchev rstoyanchev self-assigned this Jan 10, 2023
@rstoyanchev
Copy link
Contributor

rstoyanchev commented Jan 19, 2023

The proxy advisors are not in the right order indeed. The one for @HttpExchange doesn't delegate and is meant to be last in such a chain, but is always added first when the proxy is created. I was able to use an AdvisedSupportListener that updates the order whenever there is an advice change, although I'm not sure if there is a better way.

Now MethodValidationInterceptor is getting called but raises an ISE with "Target must not be null". From what I can see it needs the target in order 1) get the @Validated from the class to check for validation groups, and 2) invoke bean validation.

So it looks like method validation via @Validated doesn't work for interface-only proxies, unless I'm missing something, but I'm wondering if it can be made to work? Perhaps by passing the type-level @Validated into the the interceptor so it doesn't have to look it up. For Hibernate Validator, I see it asserts the target is not null, but I'm not sure if it needs much else from it, or whether passing the proxy itself would work. /cc @jhoeller

It's also interesting that it works with spring-cloud-openfeign. @DanielLiu1123, do you get an actual ConstraintViolation with spring-cloud-openfeign since with the above snippet you would also get an IAE from the HttpServiceProxyFactory code for such a required @RequestParameter.

@DanielLiu1123
Copy link
Author

Here's my test example(Spring Boot 3.0.1, Spring Cloud 2022.0.0):

dependencies {
    implementation 'org.springframework.boot:spring-boot-starter-web'
    implementation 'org.springframework.boot:spring-boot-starter-validation'
    implementation 'org.springframework.cloud:spring-cloud-starter-openfeign'
    compileOnly 'org.projectlombok:lombok'
    annotationProcessor 'org.projectlombok:lombok'
}
@SpringBootApplication
@EnableFeignClients
public class App implements ApplicationRunner {
    public static void main(String[] args) {
        SpringApplication.run(App.class, args);
    }

    @FeignClient(name = "post", url = "https://my-json-server.typicode.com")
    @Validated
    interface FeignPostApi {
        @GetMapping("/typicode/demo/posts/{id}")
        Post get(@PathVariable("id") @Min(2) int id);
    }

    @HttpExchange("https://my-json-server.typicode.com")
    @Validated
    interface HttpExchangePostApi {
        @GetExchange("/typicode/demo/posts/{id}")
        Post get(@PathVariable("id") @Min(2) int id);
    }

    @Data
    static class Post {
        public String id;
        public String title;
    }

    @Autowired
    HttpExchangePostApi httpExchangePostApi;
    @Autowired
    FeignPostApi feignPostApi;

    @Override
    public void run(ApplicationArguments args) throws Exception {

        // @Validated not work, can get the response
        System.out.println(httpExchangePostApi.get(1));

        // @Validated works, throw ConstraintViolationException
        System.out.println(feignPostApi.get(1));
    }

    @Bean
    public HttpExchangePostApi postApi(WebClient.Builder builder) {
        return HttpServiceProxyFactory.builder(WebClientAdapter.forClient(builder.build()))
                .build()
                .createClient(HttpExchangePostApi.class);
    }
}

feignPostApi.get(1) throws ConstraintViolationException:

jakarta.validation.ConstraintViolationException: get.id: must be greater than or equal to 2

@DanielLiu1123
Copy link
Author

I personally think the ability to support @Validated on interface (feign, httpExchange) is a very useful feature. It can unify client-side and server-side constraints (server-side implements the interface).

@rstoyanchev rstoyanchev modified the milestones: Triage Queue, 6.0.5 Jan 20, 2023
@rstoyanchev rstoyanchev added type: enhancement A general enhancement and removed status: waiting-for-triage An issue we've not yet triaged or decided on labels Jan 26, 2023
@rstoyanchev rstoyanchev changed the title HttpExchange interface method validation support Support method validation for an interface only proxy like an HTTP interface client Jan 26, 2023
@rstoyanchev
Copy link
Contributor

This has been addressed by improving Spring AOP support for interface-only proxies where there is no target class, and in which case the last AOP advisor does the actual work and should always remain last in the order. So this change should help not only method validation, but also any other AOP advice applied to an HTTP interface client.

mdeinum pushed a commit to mdeinum/spring-framework that referenced this issue Jun 29, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
in: web Issues in web modules (web, webmvc, webflux, websocket) type: enhancement A general enhancement
Projects
None yet
Development

No branches or pull requests

4 participants