This is an automated email from the ASF dual-hosted git repository.
remm pushed a commit to branch main
in repository https://gitbox.apache.org/repos/asf/tomcat.git
The following commit(s) were added to refs/heads/main by this push:
new 947802bb12 Fix processing of the time-taken token
947802bb12 is described below
commit 947802bb12e42b6f98ef1552cfb4c46490bf76d4
Author: remm <r...@apache.org>
AuthorDate: Fri Mar 7 17:02:32 2025 +0100
Fix processing of the time-taken token
Simplify.
Add test case.
---
.../catalina/valves/ExtendedAccessLogValve.java | 41 ++---
.../valves/TestExtendedAccessLogValve.java | 171 +++++++++++++++++----
...ve.java => TestExtendedAccessLogValveWrap.java} | 2 +-
webapps/docs/changelog.xml | 4 +
4 files changed, 162 insertions(+), 56 deletions(-)
diff --git a/java/org/apache/catalina/valves/ExtendedAccessLogValve.java
b/java/org/apache/catalina/valves/ExtendedAccessLogValve.java
index 819096df0e..ce4de03805 100644
--- a/java/org/apache/catalina/valves/ExtendedAccessLogValve.java
+++ b/java/org/apache/catalina/valves/ExtendedAccessLogValve.java
@@ -135,23 +135,7 @@ public class ExtendedAccessLogValve extends AccessLogValve
{
}
/* Wrap all values in double quotes. */
- StringBuilder buffer = new StringBuilder(svalue.length() + 2);
- buffer.append('\"');
- int i = 0;
- while (i < svalue.length()) {
- int j = svalue.indexOf('\"', i);
- if (j == -1) {
- buffer.append(svalue.substring(i));
- i = svalue.length();
- } else {
- buffer.append(svalue.substring(i, j + 1));
- buffer.append('"');
- i = j + 1;
- }
- }
-
- buffer.append('\"');
- return buffer.toString();
+ return "\"" + svalue.replace("\"", "\"\"") + "\"";
}
@Override
@@ -539,16 +523,19 @@ public class ExtendedAccessLogValve extends
AccessLogValve {
if (tokenizer.hasSubToken()) {
String nextToken = tokenizer.getToken();
if ("taken".equals(nextToken)) {
- nextToken = tokenizer.getToken();
-
- if ("ns".equals(nextToken)) {
- return new
ElapsedTimeElement(ElapsedTimeElement.Style.NANOSECONDS);
- } else if ("us".equals(nextToken)) {
- return new
ElapsedTimeElement(ElapsedTimeElement.Style.MICROSECONDS);
- } else if ("ms".equals(nextToken)) {
- return new
ElapsedTimeElement(ElapsedTimeElement.Style.MILLISECONDS);
- } else if ("fracsec".equals(nextToken)) {
- return new
ElapsedTimeElement(ElapsedTimeElement.Style.SECONDS_FRACTIONAL);
+ if (tokenizer.hasSubToken()) {
+ nextToken = tokenizer.getToken();
+ if ("ns".equals(nextToken)) {
+ return new
ElapsedTimeElement(ElapsedTimeElement.Style.NANOSECONDS);
+ } else if ("us".equals(nextToken)) {
+ return new
ElapsedTimeElement(ElapsedTimeElement.Style.MICROSECONDS);
+ } else if ("ms".equals(nextToken)) {
+ return new
ElapsedTimeElement(ElapsedTimeElement.Style.MILLISECONDS);
+ } else if ("fracsec".equals(nextToken)) {
+ return new
ElapsedTimeElement(ElapsedTimeElement.Style.SECONDS_FRACTIONAL);
+ } else {
+ return new
ElapsedTimeElement(ElapsedTimeElement.Style.SECONDS);
+ }
} else {
return new
ElapsedTimeElement(ElapsedTimeElement.Style.SECONDS);
}
diff --git a/test/org/apache/catalina/valves/TestExtendedAccessLogValve.java
b/test/org/apache/catalina/valves/TestExtendedAccessLogValve.java
index fb3377ef3b..58955fb73e 100644
--- a/test/org/apache/catalina/valves/TestExtendedAccessLogValve.java
+++ b/test/org/apache/catalina/valves/TestExtendedAccessLogValve.java
@@ -16,48 +16,163 @@
*/
package org.apache.catalina.valves;
+import java.io.File;
+import java.io.IOException;
+import java.nio.charset.StandardCharsets;
+import java.nio.file.Files;
+import java.util.ArrayList;
+import java.util.Collection;
+import java.util.HashMap;
+import java.util.List;
+import java.util.Map;
+import java.util.regex.Pattern;
+
+import jakarta.servlet.http.HttpServlet;
+import jakarta.servlet.http.HttpServletRequest;
+import jakarta.servlet.http.HttpServletResponse;
+
import org.junit.Assert;
import org.junit.Test;
+import org.junit.runner.RunWith;
+import org.junit.runners.Parameterized;
+import org.junit.runners.Parameterized.Parameter;
-public class TestExtendedAccessLogValve {
+import org.apache.catalina.Context;
+import org.apache.catalina.Host;
+import org.apache.catalina.startup.Tomcat;
+import org.apache.catalina.startup.TomcatBaseTest;
+import org.apache.tomcat.util.buf.ByteChunk;
- @Test
- public void alpha() {
- Assert.assertEquals("\"foo\"", ExtendedAccessLogValve.wrap("foo"));
- }
+@RunWith(Parameterized.class)
+public class TestExtendedAccessLogValve extends TomcatBaseTest {
- @Test
- public void testNull() {
- Assert.assertEquals("-", ExtendedAccessLogValve.wrap(null));
+ @Parameterized.Parameters(name = "{index}: pattern=[{0}]")
+ public static Collection<Object[]> data() {
+ List<Object[]> patterns = new ArrayList<>();
+ patterns.add(new Object[]{"basic", "time cs-method cs-uri-stem
cs-uri-query"});
+ patterns.add(new Object[]{"ip", "time cs-method sc-status c-ip s-ip
s-dns c-dns"});
+ patterns.add(new Object[]{"headers", "time cs-method cs(Referer)
cs(Cookie) sc(Content-Type)"});
+ patterns.add(new Object[]{"bytes", "date time cs-method cs-uri-stem
bytes time-taken cached"});
+ return patterns;
}
- @Test
- public void empty() {
- Assert.assertEquals("\"\"", ExtendedAccessLogValve.wrap(""));
- }
+ @Parameter(0)
+ public String name;
- @Test
- public void singleQuoteMiddle() {
- Assert.assertEquals("\"foo'bar\"",
ExtendedAccessLogValve.wrap("foo'bar"));
- }
+ @Parameter(1)
+ public String logPattern;
@Test
- public void doubleQuoteMiddle() {
- Assert.assertEquals("\"foo\"\"bar\"",
ExtendedAccessLogValve.wrap("foo\"bar"));
+ public void testLogFormat() throws Exception {
+
+ Tomcat tomcat = getTomcatInstance();
+ Host host = tomcat.getHost();
+
+ // Create temporary directory for logs
+ File logDir = getTemporaryDirectory();
+
+ ExtendedAccessLogValve valve = new ExtendedAccessLogValve();
+ valve.setPattern(logPattern);
+ valve.setDirectory(logDir.getAbsolutePath());
+ valve.setPrefix("access_log_" + name);
+
+ host.getPipeline().addValve(valve);
+
+ // Add test servlet
+ Context ctx = getProgrammaticRootContext();
+ Tomcat.addServlet(ctx, "testServlet", new HttpServlet() {
+ private static final long serialVersionUID = 1L;
+ @Override
+ protected void doGet(HttpServletRequest req, HttpServletResponse
resp) throws IOException {
+ resp.getWriter().write("Test response");
+ }
+ });
+ ctx.addServletMappingDecoded("/test", "testServlet");
+
+ tomcat.start();
+
+ String url = "http://localhost:"; + getPort() + "/test";
+ ByteChunk out = new ByteChunk();
+ Map<String, List<String>> reqHead = new HashMap<>();
+ List<String> cookieHeaders = new ArrayList<>();
+ cookieHeaders.add("COOKIE-1_1=1_1;COOKIE-1_2=1_2;COOKIE-1_3=1_3");
+ reqHead.put("Cookie", cookieHeaders);
+ List<String> refererHeader = new ArrayList<>();
+ refererHeader.add("/some/path");
+ reqHead.put("Referer", refererHeader);
+ List<String> contentTypeHeader = new ArrayList<>();
+ contentTypeHeader.add("text/plain");
+ reqHead.put("Content-Type", contentTypeHeader);
+ Map<String, List<String>> resHead = new HashMap<>();
+ int status = getUrl(url, out, reqHead, resHead);
+ Assert.assertEquals(HttpServletResponse.SC_OK, status);
+
+ tomcat.stop();
+
+ // Read log file
+ File[] logFiles = logDir.listFiles((dir, fname) ->
fname.startsWith("access_log_" + name));
+ Assert.assertNotNull(logFiles);
+ Assert.assertTrue(logFiles.length > 0);
+
+ File logFile = logFiles[0];
+ String content = new String(Files.readAllBytes(logFile.toPath()),
StandardCharsets.UTF_8);
+
+ processLogContent(content);
+
}
- @Test
- public void doubleQuoteStart() {
- Assert.assertEquals("\"\"\"foobar\"",
ExtendedAccessLogValve.wrap("\"foobar"));
+ private void processLogContent(String content) {
+ String[] lines = content.split("\\r?\\n");
+
+ List<String> dataLines = new ArrayList<>();
+ for (String line : lines) {
+ if (!line.startsWith("#")) { // Skip directives
+ dataLines.add(line.trim());
+ } else {
+ if (line.startsWith("#Fields: ")) {
+ Assert.assertEquals(line.substring("#Fields: ".length()),
logPattern);
+ }
+ }
+ }
+
+ Assert.assertTrue("No data entries found", !dataLines.isEmpty());
+
+ String entryLine = dataLines.get(0);
+ String[] parts = entryLine.split("\\s+");
+
+ String[] expectedFields = logPattern.split("\\s+");
+
+ Assert.assertEquals(expectedFields.length, parts.length);
+
+ for (int i=0; i < expectedFields.length; i++) {
+ checkField(expectedFields[i], parts[i]);
+ }
}
- @Test
- public void doubleQuoteEnd() {
- Assert.assertEquals("\"foobar\"\"\"",
ExtendedAccessLogValve.wrap("foobar\""));
+ private void checkField(String fieldId, String value) {
+ if ("time".equals(fieldId)) {
+ Assert.assertTrue("Invalid time format", isTimeFormat(value));
+ } else if ("cs-method".equals(fieldId)) {
+ Assert.assertEquals("GET", value);
+ } else if (fieldId.startsWith("c-ip")) {
+ // IPv4 with optional port
+
Assert.assertTrue(value.matches("^\\d{1,3}(\\.\\d{1,3}){3}(:\\d+)?$"));
+ } else if ("cs-uri-stem".equals(fieldId)) {
+ Assert.assertEquals("/test", value);
+ } else if (fieldId.equals("sc-status")) {
+ Assert.assertEquals("200", value);
+ } else if (fieldId.contains("Referer")) {
+ Assert.assertTrue(value.equals("\"/some/path\""));
+ } else if ("bytes".equals(fieldId)) {
+ // Non-negative integer or '-'
+ Assert.assertTrue(value.equals("-") || value.matches("\\d+"));
+ } else if ("time-taken".equals(fieldId)) {
+ // Fixed format (e.g., 0.015)
+ Assert.assertTrue(value.matches("^\\d+(\\.\\d+)?$"));
+ }
}
- @Test
- public void doubleQuote() {
- Assert.assertEquals("\"\"\"\"", ExtendedAccessLogValve.wrap("\""));
+ private boolean isTimeFormat(String s) {
+ return Pattern.matches("^\\d{2}:\\d{2}(:\\d{2}(\\.\\d+)?)?$", s);
}
-}
+}
\ No newline at end of file
diff --git a/test/org/apache/catalina/valves/TestExtendedAccessLogValve.java
b/test/org/apache/catalina/valves/TestExtendedAccessLogValveWrap.java
similarity index 97%
copy from test/org/apache/catalina/valves/TestExtendedAccessLogValve.java
copy to test/org/apache/catalina/valves/TestExtendedAccessLogValveWrap.java
index fb3377ef3b..129f6a371e 100644
--- a/test/org/apache/catalina/valves/TestExtendedAccessLogValve.java
+++ b/test/org/apache/catalina/valves/TestExtendedAccessLogValveWrap.java
@@ -19,7 +19,7 @@ package org.apache.catalina.valves;
import org.junit.Assert;
import org.junit.Test;
-public class TestExtendedAccessLogValve {
+public class TestExtendedAccessLogValveWrap {
@Test
public void alpha() {
diff --git a/webapps/docs/changelog.xml b/webapps/docs/changelog.xml
index b1cfac4946..864dac4eab 100644
--- a/webapps/docs/changelog.xml
+++ b/webapps/docs/changelog.xml
@@ -161,6 +161,10 @@
rate limit based on the exact values configured. Based on pull request
<pr>794</pr> by Chenjp. (markt)
</add>
+ <fix>
+ Fix parsing of the <code>time-taken</code> token in the
+ <code>ExtendedAccessLogValve</code>. (remm)
+ </fix>
</changelog>
</subsection>
<subsection name="Coyote">
---------------------------------------------------------------------
To unsubscribe, e-mail: dev-unsubscr...@tomcat.apache.org
For additional commands, e-mail: dev-h...@tomcat.apache.org
