Add Test for ServletRequestPathUtils.parseAndCache(method=null)#18166
Conversation
539fa3e to
c8bd4d4
Compare
| String requestMethod = request.getMethod(); | ||
| return requestMethod != null && this.method.name().equals(requestMethod); |
There was a problem hiding this comment.
We don’t need to check for null because this.method.name() will return a String type and String#equals(null) will return false, so the test passes without any changes.
There was a problem hiding this comment.
but this fixes the NPE case - ig that is our expectation of getting false for invalid
There was a problem hiding this comment.
this.method.name().equals(request.getMethod()) will return false if request.getMethod() == null and won’t throw an NPE unless request is null or method is null. The test passes without any changes.
There was a problem hiding this comment.
oh got it - thanks for the explanation
|
Thank you for your submission @ronodhirSoumik As pointed out, the test will pass without changes to the code. Additionally, the issue you are linking to is caused by ConcurrentHashMap not supporting a null key (which is of type ServletContext) and is unrelated to the method being null. I think that the test would still provide value by ensuring that we do not get a NullPointerException later. If you would modify the PR to just have the test and update the PR subject, I can merge it. |
c8bd4d4 to
e292d4c
Compare
Signed-off-by: Soumik Sarker <ronodhirsoumik@gmail.com>
d8a7001 to
d3e138c
Compare
|
@rwinch I have updated the PR with only test file (also unlinked the previous issue number) - plz review |
|
Thanks for the Pull Request! This is now merged into main 😄 |
No description provided.