joerghoh commented on code in PR #53:
URL:
https://github.com/apache/sling-org-apache-sling-engine/pull/53#discussion_r1834439699
##########
src/main/java/org/apache/sling/engine/impl/SlingHttpServletResponseImpl.java:
##########
@@ -339,28 +350,91 @@ private Optional<String>
checkContentTypeOverride(@Nullable String contentType)
}
/**
- * Retrieves the message to log when the 'Content-Type' header is changed
via an include.
+ * Finds unmatched TIMER_START messages in a log of messages.
+ *
+ * @return a string containing the unmatched TIMER_START messages
+ */
+ private String findUnmatchedTimerStarts() {
+ Iterator<String> messages =
requestData.getRequestProgressTracker().getMessages();
+ List<String> unmatchedStarts = new ArrayList<>();
+ Stack<String> timerStack = new Stack<>();
+
+ // this regex matches TIMER_START{ followed by any characters except
}, and then
+ // a closing }. The part inside the braces is captured for later use.
+ Pattern startPattern = Pattern.compile("TIMER_START\\{([^}]+)\\}");
+
+ // this regex matches TIMER_END{ followed by one or more digits, a
comma, any
+ // characters except }, and then a closing }. The part after the comma
and
+ // before the closing brace is captured for later use.
+ Pattern endPattern = Pattern.compile("TIMER_END\\{\\d+,([^}]+)\\}");
+
+ while (messages.hasNext()) {
+ String message = messages.next();
+ Matcher startMatcher = startPattern.matcher(message);
+ Matcher endMatcher = endPattern.matcher(message);
+
+ // use a simple stack to keep track of the timers that have been
started. When
+ // an end timer is found, it is compared to the top of the stack.
If they match,
+ // the timer is popped from the stack. If they don't match, the
timer is added
+ // to the list of unmatched starts. As the stack is a LIFO data
structure, the
+ // last timer that was started will be the first one to be ended.
There is no
+ // Start1, Start2, End1 scenario, without an End2 in between.
+ if (startMatcher.find()) {
+ timerStack.push(startMatcher.group(1));
+ } else if (endMatcher.find()) {
+ String endTimer = endMatcher.group(1);
+ if (!timerStack.isEmpty() &&
timerStack.peek().equals(endTimer)) {
+ timerStack.pop();
+ } else {
+ unmatchedStarts.add(endTimer);
+ }
+ }
+ }
+
+ while (!timerStack.isEmpty()) {
+ unmatchedStarts.add(timerStack.pop());
+ }
+
+ return unmatchedStarts.toString().trim();
+ }
+
+ /**
+ * Retrieves the message to log when the 'Content-Type' header is changed
via an
+ * include.
+ *
* @param currentContentType the current 'Content-Type' header
- * @param setContentType the 'Content-Type' header that is being set
+ * @param setContentType the 'Content-Type' header that is being set
Review Comment:
Can you please remove this whitespace change?
##########
src/main/java/org/apache/sling/engine/impl/SlingHttpServletResponseImpl.java:
##########
@@ -339,28 +350,91 @@ private Optional<String>
checkContentTypeOverride(@Nullable String contentType)
}
/**
- * Retrieves the message to log when the 'Content-Type' header is changed
via an include.
+ * Finds unmatched TIMER_START messages in a log of messages.
+ *
+ * @return a string containing the unmatched TIMER_START messages
+ */
+ private String findUnmatchedTimerStarts() {
+ Iterator<String> messages =
requestData.getRequestProgressTracker().getMessages();
+ List<String> unmatchedStarts = new ArrayList<>();
+ Stack<String> timerStack = new Stack<>();
+
+ // this regex matches TIMER_START{ followed by any characters except
}, and then
+ // a closing }. The part inside the braces is captured for later use.
+ Pattern startPattern = Pattern.compile("TIMER_START\\{([^}]+)\\}");
Review Comment:
Wouldn't it make sense to convert these into constants?
##########
src/test/java/org/apache/sling/engine/impl/SlingHttpServletResponseImplTest.java:
##########
@@ -90,12 +134,14 @@ public void testContentMethods() {
Mockito.verify(orig, never()).setLocale(null);
Mockito.verify(orig, never()).setBufferSize(4500);
- verify(requestProgressTracker, times(1))
- .log(String.format(
- "ERROR: Servlet %s tried to override the
'Content-Type' header from 'null'"
- + " to 'text/plain'. This is a violation of
the RequestDispatcher.include() contract -"
- + "
https://jakarta.ee/specifications/servlet/4.0/apidocs/javax/servlet/requestdispatcher#include-javax.servlet.ServletRequest-javax.servlet.ServletResponse-.";,
- ACTIVE_SERVLET_NAME));
+ ArgumentCaptor<String> logCaptor =
ArgumentCaptor.forClass(String.class);
+ verify(requestProgressTracker, times(1)).log(logCaptor.capture());
+ String logMessage = logCaptor.getValue();
+ assertEquals(
+ String.format(
Review Comment:
I fail to see how this large log message helps me to understand where and
why the change the content-type was changed. I would appreciate if you could
provide an example how to interpret this lengthy log message (preferably in the
ticket SLING-12478).
--
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: [email protected]
For queries about this service, please contact Infrastructure at:
[email protected]
