-
Notifications
You must be signed in to change notification settings - Fork 1.9k
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
Issue #6026 - Cleaning up warning for paths with uncovered HTTP methods #6029
Issue #6026 - Cleaning up warning for paths with uncovered HTTP methods #6029
Conversation
Signed-off-by: Joakim Erdfelt <joakim.erdfelt@gmail.com>
} | ||
if (LOG.isDebugEnabled()) | ||
LOG.debug("{} has uncovered http methods", currentContext, new Throwable()); | ||
LOG.warn("{} has paths with uncovered http methods: [{}]", |
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.
The log warn message is the wrong way around, it is the uncovered paths not methods, so it should be:
LOG.warn("{} has paths with uncovered http methods: [{}]", | |
LOG.warn("{} has uncovered http methods for paths: [{}]", |
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.
Yet the method name is checkPathsWithUncoveredHttpMethods
??
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.
Correct, we are reporting the uncovered paths not the uncovered methods.
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.
Confusing.
- "has paths with uncovered http methods" - identifies paths, that has uncovered http methods
- "has uncovered http methods for paths" - identifies methods, that are not covered by the paths
Perhaps there's a 3rd choice that's less confusing. What does "uncovered http method" mean?
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.
Agree with @joakime here, the method name suggests the other way around.
Also, do we really want to use WARN? I don't think nothing happens if we hit this case, and we don't want to clog log files if repeated requests for this case happens?
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.
The name of the method is "checkPathsWithUncoveredMethods" - ie we are checking the paths for which there are uncovered methods. The logging is reporting the paths, not the methods - it would be incorrect to imply that we are reporting methods!
Furthermore, this warning is required by the servlet specification: your choice if you want to make it INFO instead of WARN, but the spec requires that we produce this warning at deployment time.
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.
Is this the section of the spec this covers?
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.
Yes, this is the section. Please see comment by @gregw - we can enumerate the paths that are uncovered but not the methods.
} | ||
if (LOG.isDebugEnabled()) | ||
LOG.debug("{} has uncovered http methods", currentContext, new Throwable()); | ||
LOG.warn("{} has paths with uncovered http methods: [{}]", |
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.
Agree with @joakime here, the method name suggests the other way around.
Also, do we really want to use WARN? I don't think nothing happens if we hit this case, and we don't want to clog log files if repeated requests for this case happens?
A bit of background.... the servlet spec is beyond brain dead here! If an app has a security constraints:
Then a request to So @janbartel is correct: there should be a warning that enumerates the paths. I kind of think we should also by default refuse to deploy.... but that's also contrary to the spec and going beyond the scope of this PR to just remove a noisy stack trace... which I agree gave no needed information. |
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.
The boolean
return value from checkPathsWithUncoveredHttpMethods()
is never used, can it be changed without breaking?
LOG.debug("{} has uncovered http methods", currentContext, new Throwable()); | ||
LOG.warn("{} has paths with uncovered http methods: [{}]", | ||
ContextHandler.getCurrentContext(), | ||
String.join(", ", paths)); |
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.
There is no need for String.join()
as Set.toString()
already formats like you want (external brackets and commas to separate items).
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.
Done.
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.
if we backport this to 9.4.x then removing the return value is a bad idea.
But for 10+ i'm fine with removing the return value.
Signed-off-by: Joakim Erdfelt <joakim.erdfelt@gmail.com>
Leaving it at WARN level. |
Signed-off-by: Joakim Erdfelt <joakim.erdfelt@gmail.com>
Closes #6026
Signed-off-by: Joakim Erdfelt joakim.erdfelt@gmail.com