16635 initial commit #18
Conversation
Closes spring-projectsgh-16500 Signed-off-by: Pat McCusker <[email protected]>
Reviewer's Guide by SourceryThis pull request introduces No diagrams generated as the changes look simple and do not need a visual representation. File-Level Changes
Tips and commandsInteracting with Sourcery
Customizing Your ExperienceAccess your dashboard to:
Getting Help
|
|
Qodo Merge was enabled for this repository. To continue using it, please link your Git account with your Qodo account here. PR Reviewer Guide 🔍Here are some key observations to aid the review process:
|
GuusArts
changed the title
|
Qodo Merge was enabled for this repository. To continue using it, please link your Git account with your Qodo account here. PR Code Suggestions ✨Explore these optional code suggestions:
|
|||||||||
![sourcery-ai[bot]](https://avatars.githubusercontent.com/in/48477?s=60&v=4){kind=small}
There was a problem hiding this comment.
Hey @GuusArts - I've reviewed your changes - here's some feedback:
Overall Comments:
- Consider removing the
LazySimpDestinationMessageMatcherandLazySimpDestinationPatternMessageMatcherclasses and instead creating the matchers directly. - It might be helpful to provide a usage example in the javadoc for the new
destinationPathPatternsmethod.
Here's what I looked at during the review
- 🟢 General issues: all looks good
- 🟢 Security: all looks good
- 🟢 Testing: all looks good
- 🟡 Complexity: 2 issues found
- 🟢 Documentation: all looks good
Help me be more useful! Please click 👍 or 👎 on each comment and I'll use the feedback to improve your reviews.
| @@ -87,12 +89,11 @@ private MessageAuthorizationContext<?> authorizationContext(MessageMatcher<?> ma | |||
| if (!matcher.matches((Message) message)) { | |||
| return null; | |||
| } | |||
| if (matcher instanceof SimpDestinationMessageMatcher simp) { | |||
There was a problem hiding this comment.
issue (complexity): Consider introducing a common interface for matchers that extract path variables to reduce duplicated instance-of checks in the authorizationContext method and simplify code navigation by checking the interface instead of concrete types..
Consider introducing a common abstraction for matchers that extract path variables. Currently, you have duplicated instance‑of checks in the authorizationContext method for both lazy matcher types. You can reduce branching by defining an interface (e.g. HasPathVariables) and letting both lazy matcher classes implement it. Then update authorizationContext to check this interface rather than multiple concrete types.
For example:
// 1. Define the common interface
public interface HasPathVariables {
Map<String, String> extractPathVariables(Message<?> message);
}// 2. Have your lazy matcher classes implement it
private static final class LazySimpDestinationMessageMatcher implements MessageMatcher<Object>, HasPathVariables {
// existing code
@Override
public Map<String, String> extractPathVariables(Message<?> message) {
return this.delegate.get().extractPathVariables(message);
}
// ...
}
private static final class LazySimpDestinationPatternMessageMatcher implements MessageMatcher<Object>, HasPathVariables {
// existing code
@Override
public Map<String, String> extractPathVariables(Message<?> message) {
return this.delegate.get().extractPathVariables(message);
}
// ...
}// 3. Simplify the authorizationContext method with a single check:
private MessageAuthorizationContext<?> authorizationContext(MessageMatcher<?> matcher, Message<?> message) {
if (!matcher.matches(message)) {
return null;
}
if (matcher instanceof HasPathVariables hv) {
return new MessageAuthorizationContext<>(message, hv.extractPathVariables(message));
}
return new MessageAuthorizationContext<>(message);
}By applying these changes you reduce duplicated logic, simplify navigation in the code, and maintain all current functionality.
| * Initialize this builder with a {@link PathPatternRouteMatcher} configured with the | ||
| * {@link org.springframework.http.server.PathContainer.Options#HTTP_PATH} separator | ||
| */ | ||
| public static Builder withDefaults() { |
There was a problem hiding this comment.
issue (complexity): Consider consolidating the two static builder methods into a single parameterized builder method using an enum to specify the route matcher type, simplifying the API and reducing duplication while maintaining functionality
You could simplify the API by reducing the dual static builder methods and abstracting the route matcher choice into a single parameterized builder. For example, define an enum to clarify the matcher type and consolidate builder creation:
public enum RouteMatcherType {
SLASH, DOT
}Then replace the two static methods with one unified builder method:
public static Builder builder(RouteMatcherType type) {
PathPatternRouteMatcher matcher = (type == RouteMatcherType.DOT)
? DOT_SEPARATED_ROUTE_MATCHER
: SLASH_SEPARATED_ROUTE_MATCHER;
return new Builder(matcher);
}This change removes duplicative entry points while keeping all functionality intact. The rest of your Builder and matching logic remains the same.
|
There was a problem hiding this comment.
Pull Request Overview
This PR introduces a new DestinationPathPatternMessageMatcher to enhance message destination matching while deprecating the old SimpDestinationMessageMatcher. Key changes include new matcher implementation with support for path variable extraction, updated tests to verify the new behaviors, and minor documentation updates (e.g. copyright year).
Reviewed Changes
Copilot reviewed 8 out of 8 changed files in this pull request and generated no comments.
Show a summary per file
| File | Description |
|---|---|
| SimpDestinationMessageMatcherTests.java | Added tests to ensure extraction method throws an exception when no match is found. |
| DestinationPathPatternMessageMatcherTests.java | Added comprehensive tests for the new matcher covering various route patterns and message types. |
| MessageMatcherDelegatingAuthorizationManagerTests.java | Updated tests to use destinationPathPatterns and messageRouteSeparator, replacing deprecated methods. |
| SimpDestinationMessageMatcher.java | Marked the class as deprecated and updated the Javadoc to refer to the new matcher. |
| DestinationPathPatternMessageMatcher.java | Introduced the new implementation leveraging PathPatternRouteMatcher for both slash- and dot-separated routes. |
| MessageMatcherDelegatingAuthorizationManager.java | Integrated the new matcher into the authorization context and replaced deprecated lazy matcher usage. |
| WebSocketMessageBrokerSecurityConfigurationTests.java, WebSocketMessageBrokerSecurityConfigurationDocTests.java | Updated copyright statements. |
Comments suppressed due to low confidence (1)
messaging/src/main/java/org/springframework/security/messaging/access/intercept/MessageMatcherDelegatingAuthorizationManager.java:516
- [nitpick] Consider renaming 'LazySimpDestinationPatternMessageMatcher' to 'LazyDestinationPathPatternMessageMatcher' to maintain clear consistency with the new 'DestinationPathPatternMessageMatcher' naming in the codebase.
private static final class LazySimpDestinationPatternMessageMatcher implements MessageMatcher<Object> {
🎉 Snyk checks have passed. No issues have been found so far.✅ security/snyk check is complete. No issues have been found. (View Details) ✅ code/snyk check is complete. No issues have been found. (View Details) |
User description
Closes spring-projectsgh-16500
PR Type
Enhancement, Tests
Description
Introduced
DestinationPathPatternMessageMatcherfor enhanced message matching.SimpDestinationMessageMatcher.Deprecated
SimpDestinationMessageMatcherand related methods in favor of the new matcher.Added new test cases for
DestinationPathPatternMessageMatcherto validate functionality.Updated existing tests to use the new
DestinationPathPatternMessageMatcher.Changes walkthrough 📝
2 files
Updated copyright year to 2025Updated copyright year to 20253 files
IntegratedDestinationPathPatternMessageMatcherand deprecated oldmatchers
Added new `DestinationPathPatternMessageMatcher` implementationDeprecated `SimpDestinationMessageMatcher` in favor of new matcher3 files
Updated tests to use `DestinationPathPatternMessageMatcher`Added tests for `DestinationPathPatternMessageMatcher`Added edge case tests for deprecated `SimpDestinationMessageMatcher`