joerghoh commented on code in PR #56:
URL:
https://github.com/apache/sling-org-apache-sling-engine/pull/56#discussion_r1983453353
##########
src/main/java/org/apache/sling/engine/impl/SlingHttpServletResponseImpl.java:
##########
@@ -312,6 +312,15 @@ public void setIntHeader(final String name, final int
value) {
}
}
+ private String getCurrentStackTrace() {
+ StackTraceElement[] stackTraceElements =
Thread.currentThread().getStackTrace();
+ StringBuilder stackTraceBuilder = new StringBuilder("Stack trace of
the current content type header change:\n");
+ for (StackTraceElement element : stackTraceElements) {
+ stackTraceBuilder.append(element.toString()).append("\n");
Review Comment:
```suggestion
stackTraceBuilder.append(element.toString()).append(System.lineSeparator());
```
##########
src/main/java/org/apache/sling/engine/impl/SlingHttpServletResponseImpl.java:
##########
@@ -321,14 +330,17 @@ public void setContentType(final String type) {
if (message.isPresent()) {
if (isCheckContentTypeOnInclude()) {
requestData.getRequestProgressTracker().log("ERROR: " +
message.get());
+ LOG.error(getCurrentStackTrace());
throw new ContentTypeChangeException(message.get());
}
if (isProtectHeadersOnInclude()) {
LOG.error(message.get());
+ LOG.error(getCurrentStackTrace());
Review Comment:
Please do only 1 log statement here, as splitting it up makes log analysis
and correlation hard to almost impossible. So please combine this statement
with the previous one, also to add more context.
##########
src/main/java/org/apache/sling/engine/impl/SlingHttpServletResponseImpl.java:
##########
@@ -321,14 +330,17 @@ public void setContentType(final String type) {
if (message.isPresent()) {
if (isCheckContentTypeOnInclude()) {
requestData.getRequestProgressTracker().log("ERROR: " +
message.get());
+ LOG.error(getCurrentStackTrace());
Review Comment:
This results in a stacktrace being logged (to the system logs) without any
additional context; I don't think that this is helpful at all.
(I think that the logs are more durable than the content of the
RequestProgressTracker, which is by default never logged to disk at all. For
that the logging should also contain all relevant information to debug and
understand this situation after the fact.)
##########
src/main/java/org/apache/sling/engine/impl/SlingHttpServletResponseImpl.java:
##########
@@ -321,14 +330,17 @@ public void setContentType(final String type) {
if (message.isPresent()) {
if (isCheckContentTypeOnInclude()) {
requestData.getRequestProgressTracker().log("ERROR: " +
message.get());
+ LOG.error(getCurrentStackTrace());
throw new ContentTypeChangeException(message.get());
}
if (isProtectHeadersOnInclude()) {
LOG.error(message.get());
+ LOG.error(getCurrentStackTrace());
requestData.getRequestProgressTracker().log("ERROR: " +
message.get());
return;
}
LOG.warn(message.get());
+ LOG.warn(getCurrentStackTrace());
Review Comment:
see above
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: dev-unsubscr...@sling.apache.org
For queries about this service, please contact Infrastructure at:
us...@infra.apache.org
