-
Notifications
You must be signed in to change notification settings - Fork 38.6k
HTTP method support in MappedInterceptor #35273
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
HTTP method support in MappedInterceptor #35273
Conversation
845a609
to
7161bdc
Compare
@bclozel Thanks for the feedback on #35272. Understood that this feature is not currently planned. I'll consider maintaining it as an external utility or extension module, since I still believe this helps reduce boilerplate and improves clarity in interceptor setups. Appreciate your time and the review! |
We have not declined nor reviewed this yet. We don't need both an issue and a PR so we closed the other one. |
Thanks for the clarification, @bclozel. |
Until then (the review by those grannies) : |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Aside from the comments below, generally the request seems reasonable with the following implementation related points:
- The existing
MappedInterceptor
could be enhanced instead to support mapping by HTTP method. - The terminology
allowedMethods
could be justhttpMethods
, orinclude/excludeHttpMethods
for consistency withinclude/excludePatterns
.
spring-aop/.factorypath
Outdated
<factorypathentry kind="EXTJAR" id="/home/sriram/.gradle/caches/modules-2/files-2.1/org.pcollections/pcollections/4.0.1/59f3bf5fb28c5f5386804dcf129267416b75d7c/pcollections-4.0.1.jar" enabled="true" runInBatchMode="false"/> | ||
<factorypathentry kind="EXTJAR" id="/home/sriram/.gradle/caches/modules-2/files-2.1/com.uber.nullaway/nullaway/0.12.7/1a813b7d156768204b0c8d52ba0632a8db6fbe12/nullaway-0.12.7.jar" enabled="true" runInBatchMode="false"/> | ||
<factorypathentry kind="EXTJAR" id="/home/sriram/.gradle/caches/modules-2/files-2.1/com.google.guava/failureaccess/1.0.3/aeaffd00d57023a2c947393ed251f0354f0985fc/failureaccess-1.0.3.jar" enabled="true" runInBatchMode="false"/> | ||
</factorypath> |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
These seem to have been committed by mistake. Could you please remove them from the pull request?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I've removed the unintended .factorypath entries from the pull request.
/** | ||
* Add patterns for URLs the interceptor should be included in. | ||
* <p>For pattern syntax see {@link PathPattern} when parsed patterns | ||
* <p> | ||
* For pattern syntax see {@link PathPattern} when parsed patterns |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
There are a number of Javadoc style changes, probably applied by the IDE. Could you please undo those? They are not related and in any case inconsistent with the rest of the codebase.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I've reverted the unintended Javadoc formatting changes in InterceptorRegistration.java to keep the style consistent with the rest of the codebase.
* @since 5.0.3 | ||
*/ | ||
public InterceptorRegistration addPathPatterns(List<String> patterns) { | ||
this.includePatterns = (this.includePatterns != null ? | ||
this.includePatterns : new ArrayList<>(patterns.size())); | ||
this.includePatterns = (this.includePatterns != null ? this.includePatterns : new ArrayList<>(patterns.size())); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Likewise, a number of code style changes. Please, undo those.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I've reverted the unintended code style changes in InterceptorRegistration.java to match the existing formatting in the codebase.
@rstoyanchev Thanks for the detailed feedback. I understand the requested changes — I’ll work on enhancing MappedInterceptor with HTTP method support, update the naming to includeHttpMethods/excludeHttpMethods, and remove the unintended commits. I should have an updated PR ready in about 2 days. Thanks again for your feedback. |
…rceptorRegistration - Extended MappedInterceptor to support include/exclude HTTP methods for request matching - Added corresponding constructors for HandlerInterceptor and WebRequestInterceptor - Updated InterceptorRegistration with fluent methods to configure HTTP method filters - Ensured backward compatibility by keeping existing constructors and methods intact - Improved matches() logic to combine URL pattern and HTTP method conditions This enhancement enables finer control over interceptor application based on HTTP methods, aligning with modern Spring 7.x practices. Signed-off-by: SRIRAM9487 <[email protected]>
bd7617f
to
3b695ca
Compare
- Verify inclusion of single and multiple HTTP methods - Verify exclusion of single and multiple HTTP methods - Test combined include and exclude HTTP method filters - Confirm correct matching behavior with URL path patterns - Validate interceptor lifecycle method delegation (preHandle, postHandle, afterCompletion) - All tests pass successfully, ensuring robust coverage of method filtering logic Signed-off-by: SRIRAM9487 <[email protected]>
e6f5020
to
173e11a
Compare
This enhancement enables finer control over interceptor application based on HTTP methods, aligning with modern Spring 7.x practices. - Extend MappedInterceptor with include/exclude HTTP methods - Add constructors for interceptor implementations - Update InterceptorRegistration with fluent methods - Keep existing constructors and methods for compatibility - Update matches() to check HTTP method conditions See gh-35273 Signed-off-by: SRIRAM9487 <[email protected]>
Support HTTP method-restricted interceptors in
InterceptorRegistration
This PR implements #35272, adding support for restricting
HandlerInterceptor
registration to specific HTTP methods.Motivation
Spring MVC currently allows interceptors to be applied based on path patterns, but not HTTP methods. When method-based filtering is needed, it must be manually implemented within the interceptor itself:
This approach clutters interceptor logic with method checks and duplicates similar logic across interceptors when needed. Centralizing HTTP method filtering in the registration API provides a cleaner and more declarative configuration style.
Solution
This PR introduces a new method to
InterceptorRegistration
:This allows restricting the interceptor to specific HTTP methods like
GET
,POST
, etc. Under the hood, the framework wraps the givenHandlerInterceptor
in a newMethodInterceptor
that delegates calls only when the request's HTTP method matches one of the allowed methods.The interceptor will now be invoked only for POST and PUT requests matching
/api/**
.Implementation Details
InterceptorRegistration
stores allowed HTTP methods as aSet<String>
.MethodInterceptor
.MethodInterceptor
checks the request's method before delegating to the original interceptor.Tests
A dedicated test class
MethodInterceptorTests
is included to validate:preHandle
,postHandle
,afterCompletion
) are covered.Example
In this example,
AuditInterceptor
will apply only forGET
requests under/admin/**
.Compatibility
MappedInterceptor
,HandlerInterceptor
) without exposing new public types.