Pr 16635 last version

[GuusArts]

User description

---

PR Type

Enhancement, Tests

---

Description

• Introduced PathPatternMessageMatcher for enhanced message matching.

  • Supports path patterns and variable extraction.
  • Provides a builder for flexible configuration.

• Added MessageMatcher.MatchResult for detailed match results.

  • Includes match status and extracted variables.

• Deprecated SimpDestinationMessageMatcher in favor of PathPatternMessageMatcher.

• Updated tests to validate new functionality and ensure backward compatibility.

---

Changes walkthrough 📝

	Relevant files
Enhancement	7 files MessageMatcherAuthorizationManagerConfiguration.java Integrated `MessageMatcherFactory` for path pattern support +3/-1      PathPatternMessageMatcherBuilderFactoryBean.java Added factory bean for `PathPatternMessageMatcher.Builder` +44/-0    MessageMatcherDelegatingAuthorizationManager.java Enhanced authorization manager with path pattern support  +46/-36  MessageMatcher.java Added `MatchResult` class for detailed match results          +77/-1    MessageMatcherFactory.java Introduced utility for path pattern-based matchers              +52/-0    PathPatternMessageMatcher.java Implemented `PathPatternMessageMatcher` for flexible message matching +202/-0  SimpDestinationMessageMatcher.java Deprecated `SimpDestinationMessageMatcher` in favor of new matcher +9/-1
Tests	3 files MessageMatcherDelegatingAuthorizationManagerTests.java Added tests for path pattern-based authorization                  +59/-1    PathPatternMessageMatcherTests.java Added unit tests for `PathPatternMessageMatcher`                  +155/-0  SimpDestinationMessageMatcherTests.java Updated tests for deprecated `SimpDestinationMessageMatcher` +10/-1

---

Need help?

Type /help how to ... in the comments thread for any questions about Qodo Merge usage.

Check out the documentation for more information.

[sourcery-ai]

Reviewer's Guide by Sourcery

This pull request introduces PathPatternMessageMatcher to replace the deprecated SimpDestinationMessageMatcher, enhancing message authorization with improved pattern matching capabilities. It also updates the MessageMatcher interface to provide more detailed match results and removes usages of deprecated methods and classes.

Class diagram for PathPatternMessageMatcher

classDiagram
  class PathPatternMessageMatcher {
    -PathPattern pattern
    -PathContainer.Options options
    -MessageMatcher messageTypeMatcher
    +PathPatternMessageMatcher(PathPattern pattern, PathContainer.Options options)
    +boolean matches(Message message)
    +MatchResult matcher(Message message)
    +static Builder withDefaults()
    +static Builder withPathPatternParser(PathPatternParser parser)
  }
  class Builder {
    -PathPatternParser parser
    +Builder(PathPatternParser parser)
    +PathPatternMessageMatcher matcher(String pattern)
    +PathPatternMessageMatcher matcher(SimpMessageType type, String pattern)
  }
  PathPatternMessageMatcher -- MessageMatcher : implements
  PathPatternMessageMatcher -- MatchResult : returns
  PathPatternMessageMatcher ..> Builder : uses

Loading

Deprecated class diagram for SimpDestinationMessageMatcher

classDiagram
  class SimpDestinationMessageMatcher {
    -PathMatcher matcher
    -String pattern
    -SimpMessageType messageType
    +SimpDestinationMessageMatcher(String pattern)
    +SimpDestinationMessageMatcher(String pattern, SimpMessageType messageType)
    +boolean matches(Message message)
    +Map~String, String~ extractPathVariables(Message message)
  }
  note for SimpDestinationMessageMatcher "Deprecated: Use PathPatternMessageMatcher instead"

Loading

File-Level Changes

Change	Details	Files
Introduces PathPatternMessageMatcher to replace SimpDestinationMessageMatcher for improved pattern matching in message authorization.	Added PathPatternMessageMatcher class. Modified MessageMatcherDelegatingAuthorizationManager to use PathPatternMessageMatcher when available. Updated MessageMatcherFactory to facilitate the creation of PathPatternMessageMatcher instances. Deprecated SimpDestinationMessageMatcher. Added PathPatternMessageMatcherBuilderFactoryBean to configure the PathPatternMessageMatcher.Builder bean. Updated tests to reflect the changes in message matching.	messaging/src/main/java/org/springframework/security/messaging/access/intercept/MessageMatcherDelegatingAuthorizationManager.java messaging/src/main/java/org/springframework/security/messaging/util/matcher/MessageMatcher.java messaging/src/test/java/org/springframework/security/messaging/access/intercept/MessageMatcherDelegatingAuthorizationManagerTests.java messaging/src/test/java/org/springframework/security/messaging/util/matcher/SimpDestinationMessageMatcherTests.java messaging/src/main/java/org/springframework/security/messaging/util/matcher/SimpDestinationMessageMatcher.java config/src/main/java/org/springframework/security/config/annotation/web/socket/MessageMatcherAuthorizationManagerConfiguration.java messaging/src/main/java/org/springframework/security/messaging/util/matcher/PathPatternMessageMatcher.java messaging/src/test/java/org/springframework/security/messaging/util/matcher/PathPatternMessageMatcherTests.java messaging/src/main/java/org/springframework/security/messaging/util/matcher/MessageMatcherFactory.java config/src/main/java/org/springframework/security/config/web/messaging/PathPatternMessageMatcherBuilderFactoryBean.java
Updates MessageMatcher interface to include a matcher method that returns a MatchResult object, providing more detailed information about the match, including extracted variables.	Added a default matcher method to the MessageMatcher interface that returns a MatchResult. Introduced the MatchResult class to encapsulate the result of a match, including whether the message matched and any extracted variables.	messaging/src/main/java/org/springframework/security/messaging/util/matcher/MessageMatcher.java
Removes usages of deprecated methods and classes.	Replaces usages of SimpDestinationMessageMatcher with PathPatternMessageMatcher. Removes usages of deprecated methods in SimpDestinationMessageMatcher.	messaging/src/main/java/org/springframework/security/messaging/access/intercept/MessageMatcherDelegatingAuthorizationManager.java messaging/src/main/java/org/springframework/security/messaging/util/matcher/SimpDestinationMessageMatcher.java

---

Tips and commands

Interacting with Sourcery

• Trigger a new review: Comment @sourcery-ai review on the pull request.
• Continue discussions: Reply directly to Sourcery's review comments.
• Generate a GitHub issue from a review comment: Ask Sourcery to create an
  issue from a review comment by replying to it. You can also reply to a
  review comment with @sourcery-ai issue to create an issue from it.
• Generate a pull request title: Write @sourcery-ai anywhere in the pull
  request title to generate a title at any time. You can also comment
  @sourcery-ai title on the pull request to (re-)generate the title at any time.
• Generate a pull request summary: Write @sourcery-ai summary anywhere in
  the pull request body to generate a PR summary at any time exactly where you
  want it. You can also comment @sourcery-ai summary on the pull request to
  (re-)generate the summary at any time.
• Generate reviewer's guide: Comment @sourcery-ai guide on the pull
  request to (re-)generate the reviewer's guide at any time.
• Resolve all Sourcery comments: Comment @sourcery-ai resolve on the
  pull request to resolve all Sourcery comments. Useful if you've already
  addressed all the comments and don't want to see them anymore.
• Dismiss all Sourcery reviews: Comment @sourcery-ai dismiss on the pull
  request to dismiss all existing Sourcery reviews. Especially useful if you
  want to start fresh with a new review - don't forget to comment
  @sourcery-ai review to trigger a new review!
• Generate a plan of action for an issue: Comment @sourcery-ai plan on
  an issue to generate a plan of action for it.

Customizing Your Experience

Access your dashboard to:

• Enable or disable review features such as the Sourcery-generated pull request
  summary, the reviewer's guide, and others.
• Change the review language.
• Add, remove or edit custom review instructions.
• Adjust other review settings.

Getting Help

• Contact our support team for questions or feedback.
• Visit our documentation for detailed guides and information.
• Keep in touch with the Sourcery team by following us on X/Twitter, LinkedIn or GitHub.

[qodo-merge-pro]

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:

⏱️ Estimated effort to review: 4 🔵🔵🔵🔵⚪

🧪 PR contains tests

🔒 No security concerns identified

⚡ Recommended focus areas for review Potential Bug The authorizationContext method returns null early without checking matchResult.isMatch(), which could lead to unexpected behavior when processing messages. MessageMatcher.MatchResult matchResult = matcher.matcher((Message) message); if (!matchResult.isMatch()) { return null; } Singleton Initialization The MessageMatcherFactory uses static state without thread safety considerations, which could lead to race conditions in concurrent environments. private static PathPatternMessageMatcher.Builder builder; public static void setApplicationContext(ApplicationContext context) { builder = context.getBeanProvider(PathPatternMessageMatcher.Builder.class).getIfUnique(); } Inconsistent Behavior The extractPathVariables method doesn't check if matches() returns true before extracting variables, but matcher() implementation does, creating inconsistent behavior. public Map<String, String> extractPathVariables(Message<?> message) { final String destination = SimpMessageHeaderAccessor.getDestination(message.getHeaders()); return (destination != null) ? this.matcher.extractUriTemplateVariables(this.pattern, destination) : Collections.emptyMap();

[qodo-merge-pro]

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:

Category	Suggestion	Impact
Possible issue	Check pattern match before extraction The extractPathVariables method doesn't check if the pattern actually matches the destination before extracting variables. This can lead to unexpected behavior or exceptions if the destination doesn't match the pattern. Add a check to ensure the pattern matches before extracting variables. messaging/src/main/java/org/springframework/security/messaging/util/matcher/SimpDestinationMessageMatcher.java [134-137] public Map<String, String> extractPathVariables(Message<?> message) { final String destination = SimpMessageHeaderAccessor.getDestination(message.getHeaders()); - return (destination != null) ? this.matcher.extractUriTemplateVariables(this.pattern, destination) - : Collections.emptyMap(); + if (destination == null || !this.matcher.match(this.pattern, destination)) { + return Collections.emptyMap(); + } + return this.matcher.extractUriTemplateVariables(this.pattern, destination); +} [To ensure code accuracy, apply this suggestion manually] Suggestion importance[1-10]: 8 __ Why: This suggestion addresses a potential bug where variables could be extracted from non-matching patterns, which might lead to unexpected behavior or exceptions. The improved code properly checks if the pattern matches before attempting to extract variables, making the code more robust.	Medium
General	Remove unnecessary cast The cast to (Message) is unnecessary and potentially unsafe as it removes generic type information. The method signature already accepts Message<?>, so the cast can be removed. messaging/src/main/java/org/springframework/security/messaging/access/intercept/MessageMatcherDelegatingAuthorizationManager.java [89-100] private MessageAuthorizationContext<?> authorizationContext(MessageMatcher<?> matcher, Message<?> message) { - MessageMatcher.MatchResult matchResult = matcher.matcher((Message) message); + MessageMatcher.MatchResult matchResult = matcher.matcher(message); if (!matchResult.isMatch()) { return null; } if (!CollectionUtils.isEmpty(matchResult.getVariables())) { return new MessageAuthorizationContext<>(message, matchResult.getVariables()); } return new MessageAuthorizationContext<>(message); } Apply this suggestion Suggestion importance[1-10]: 5 __ Why: The suggestion correctly identifies an unnecessary cast to (Message) that can be safely removed. While this is a valid improvement for code cleanliness, it's a minor issue that doesn't affect functionality or performance significantly.	Low
More

[sourcery-ai]

Hey @GuusArts - I've reviewed your changes - here's some feedback:

Overall Comments:

• Consider removing the deprecated SimpDestinationMessageMatcher and LazySimpDestinationMessageMatcher classes.
• It would be helpful to provide a sample configuration that uses PathPatternMessageMatcherBuilderFactoryBean.

Here's what I looked at during the review

• 🟢 General issues: all looks good
• 🟢 Security: all looks good
• 🟢 Testing: all looks good
• 🟢 Complexity: all looks good
• 🟢 Documentation: all looks good

---

Sourcery is free for open source - if you like our reviews please consider sharing them ✨

• X
• Mastodon
• LinkedIn
• Facebook

_{Help me be more useful! Please click 👍 or 👎 on each comment and I'll use the feedback to improve your reviews.}

[sonarqubecloud]

Quality Gate failed

Failed conditions
8.9% Duplication on New Code (required ≤ 3%)

See analysis details on SonarQube Cloud