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

Validate afterThrowing method signature in ThrowsAdviceInterceptor #1896

Closed

Conversation

plx927
Copy link

@plx927 plx927 commented Jul 25, 2018

  1. I found MethodAfterAdviceInterceptor implements AfterAdvice, so i think MethodBeforeAdviceInterceptor also should implements BeforeAdvice to indicate that is a BeforeAdvice.

  2. Recently when i search spring source code, i found when ThrowsAdviceInterceptoris constructed, it just found methods which name is afterThrowing and last param is ThrowableSubclass; but sometimes developer may be disorder the 4 param due to careless(eg:
    afterThrowing( Object[] args, Method method, Object target, ServletException ex)
    ), now the error only can be found when exception occur, i think framework should be reminder developer in time, when ThrowsAdviceInterceptor is constructed, if we find he/she write error, we should tell he/she that error occurs as soon as possible.

@jhoeller
Copy link
Contributor

I've opened SPR-17088 for the MethodBeforeAdviceInterceptor part since we can easily roll this into 5.1 RC1 still. As for validating reflective signatures, good point but we have other such cases across the framework, so might rather aim for a wider-spread solution.

@plx927
Copy link
Author

plx927 commented Jul 25, 2018

ok, I will continue to find such case to improve framework and try to find a wider-spread solution, thank you for your encouragement!

@jhoeller
Copy link
Contributor

It's well spotted indeed, thanks for raising it!

As for a wider-spread solution, this is in particular about a common exception type that we would raise in case of an unexpected method signature for a reflectively invoked callback in other parts of the framework. I'm not keen on introducing an exception type just for AOP throws advices there.

@plx927
Copy link
Author

plx927 commented Jul 25, 2018

I got it. Just now, i found there is an AopConfigException in aop module, can i exchange AopConfigException to my custom exception?

@jhoeller
Copy link
Contributor

AopConfigException sounds fine for this purpose, indeed! Could you rebase your PR against master (picking up the latest round of refinements there) and squash it into a single commit please?

@plx927
Copy link
Author

plx927 commented Jul 25, 2018

ok, i am doing it!

@plx927
Copy link
Author

plx927 commented Aug 6, 2018

@jhoeller this pr need add more commit?

@spring-projects-issues spring-projects-issues added the status: waiting-for-triage An issue we've not yet triaged or decided on label Jan 24, 2019
@rstoyanchev rstoyanchev added the in: core Issues in core modules (aop, beans, core, context, expression) label Nov 10, 2021
@rstoyanchev rstoyanchev added this to the Triage Queue milestone Dec 7, 2021
@jhoeller jhoeller added type: enhancement A general enhancement and removed status: waiting-for-triage An issue we've not yet triaged or decided on labels Jan 18, 2022
@jhoeller jhoeller changed the title Optimize throws advice interceptor Validate afterThrowing method signature in ThrowsAdviceInterceptor Aug 4, 2023
@jhoeller jhoeller modified the milestones: General Backlog, 6.1.0-M4 Aug 4, 2023
@jhoeller jhoeller closed this in cc90a95 Aug 6, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
in: core Issues in core modules (aop, beans, core, context, expression) type: enhancement A general enhancement
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

4 participants