Better logging support by id in RequestMatcher #6308
Conversation
1. Added default method getId in RequestMatcher and marked it as FunctionalInterface 2. Created Class AbstractRequestMatcher with default support for getId logging things 3. Implemented build in matchers using AbstractRequestMatcher 4. Changed toString methods of build in matchers to reflect id. 5. Modified Regex and El RequestMatcher toString tests to reflect new toString. Fixes: spring-projectsgh-6274
1c6a4dd to
9ee6281
Compare
|
Thanks for the PR @ankurpathak! After some more thought I think we need to take a more holistic approach to logging improvements. I created gh-6311 for us to improve logging within Spring Security as a whole. We will keep this ticket open and potentially merge these changes depending on the findings of that ticket |
There was a problem hiding this comment.
some basic review, there are many instances where new changes have been introduced to change in formatter.
Here is the guidelines on managing whitespace/formatting issues https://github.com/spring-projects/spring-security/blob/master/CONTRIBUTING.md#mind-the-whitespace
| @@ -89,11 +90,11 @@ public boolean matches(HttpServletRequest request) { | |||
| @Override | |||
| public String toString() { | |||
| StringBuilder sb = new StringBuilder(); | |||
| sb.append("ExactUrl [processUrl='").append(processUrl).append("']"); | |||
| sb.append("ExactUrl [processUrl='").append(processUrl).append("', id=").append(getId()).append("]"); | |||
There was a problem hiding this comment.
I think we can live away with String builder now, by-default string concatenations internally invoke string builder only. Plus it will result in cleaner code
| } |
There was a problem hiding this comment.
let's use the existing formatter so this does not occur
| /** | ||
| * Standard implementation of {@code SecurityFilterChain}. | ||
| * | ||
| * @author Luke Taylor | ||
| * | ||
| * @since 3.1 | ||
| */ | ||
| public final class DefaultSecurityFilterChain implements SecurityFilterChain { | ||
| public final class DefaultSecurityFilterChain implements SecurityFilterChain{ |
There was a problem hiding this comment.
let's use the existing formatter so this does not occur
| @@ -61,4 +63,5 @@ public boolean matches(HttpServletRequest request) { | |||
| public String toString() { | |||
| return "[ " + requestMatcher + ", " + filters + "]"; | |||
| } | |||
|
|
|||
There was a problem hiding this comment.
let's use the existing formatter so this does not occur
There was a problem hiding this comment.
What do you mean by existing formatter??
There was a problem hiding this comment.
I meant, these whitespaces means that the code formatter we have used for this PR does not match the one used originally. Formatter can also mean any settings in your IDE that indents/formats/structures code. Please see the section on whitespaces in contributing guidelines. Hope this helps
| @@ -76,6 +75,6 @@ public boolean matches(HttpServletRequest request) { | |||
|
|
|||
| @Override | |||
| public String toString() { | |||
| return "AndRequestMatcher [requestMatchers=" + requestMatchers + "]"; | |||
| return "AndRequestMatcher [requestMatchers=" + requestMatchers + ", id=" + getId()+"]"; | |||
|
@ankurpathak I'm going to close this PR as it's quite old and would need to be rebased off main. As well, there are a few logging improvement commits you'll notice linked in gh-6311. If you feel the updates provided in this PR are still required, please submit a new PR rebased off main and align with the logging improvements already merged and referenced in gh-6311. |
marked it as FunctionalInterface
support for getId logging things
id.
Fixes: gh-6274