-
Notifications
You must be signed in to change notification settings - Fork 0
Add DestinationPathPatternMessageMatcher human reviewer #21
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
base: main
Are you sure you want to change the base?
Conversation
Closes spring-projectsgh-16500 Signed-off-by: Pat McCusker <[email protected]>
Reviewer's Guide by SourceryThis pull request introduces Class diagram for SimpDestinationMessageMatcherclassDiagram
class SimpDestinationMessageMatcher {
-PathMatcher pathMatcher
-String pattern
-SimpMessageType type
+SimpDestinationMessageMatcher(String pattern, PathMatcher pathMatcher)
+SimpDestinationMessageMatcher(String pattern, PathMatcher pathMatcher, SimpMessageType type)
+matches(Message<?> message)
+extractPathVariables(Message<?> message)
}
File-Level Changes
Tips and commandsInteracting with Sourcery
Customizing Your ExperienceAccess your dashboard to:
Getting Help
|
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.
Hey @GuusArts - I've reviewed your changes - here's some feedback:
Overall Comments:
- Consider removing the
LazySimpDestinationMessageMatcher
andLazySimpDestinationPatternMessageMatcher
classes and inlining their logic. - It might be helpful to provide a usage example in the javadoc for
destinationPathPatterns
.
Here's what I looked at during the review
- 🟢 General issues: all looks good
- 🟢 Security: all looks good
- 🟢 Testing: all looks good
- 🟡 Complexity: 1 issue 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.
* @since 6.5 | ||
*/ | ||
public Builder.Constraint destinationPathPatterns(String... patterns) { | ||
return destinationPathPatterns(null, 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.
issue (complexity): Consider creating a helper method to encapsulate the common matcher-creation logic, reducing code duplication and improving maintainability by avoiding nearly identical code paths in different methods, such as in destinationPathPatterns.
Consider extracting the common matcher‐creation logic into a helper method rather than repeating nearly identical code paths in different methods. For example, you could create a private method that builds a matcher based on the provided pattern and type, and then have your builder methods call that helper. This would reduce duplication and ease maintenance.
Example Suggestion:
private MessageMatcher<Object> createDestinationMatcher(String pattern, SimpMessageType type, boolean useHttpPathSeparator) {
if (MessageMatcherFactory.usesPathPatterns()) {
// For new behavior using path patterns
return new LazySimpDestinationPatternMessageMatcher(pattern, type, useHttpPathSeparator);
}
// Fallback to legacy matcher
return new LazySimpDestinationMessageMatcher(pattern, type);
}
Then, in your builder methods (like destinationPathPatterns
), replace the loop with:
private Builder.Constraint destinationPathPatterns(SimpMessageType type, String... patterns) {
List<MessageMatcher<?>> matchers = new ArrayList<>(patterns.length);
for (String pattern : patterns) {
matchers.add(createDestinationMatcher(pattern, type, this.useHttpPathSeparator));
}
return new Builder.Constraint(matchers);
}
This consolidation preserves existing behavior while reducing conditional branching and duplicated code paths.
|
Closes spring-projectsgh-16500
Summary by Sourcery
Add a new DestinationPathPatternMessageMatcher to improve WebSocket message destination matching with more flexible path pattern support
New Features:
Enhancements:
Tests: