markt-asf commented on code in PR #662: URL: https://github.com/apache/tomcat/pull/662#discussion_r1316892872
########## java/org/apache/coyote/Request.java: ########## @@ -16,19 +16,8 @@ */ package org.apache.coyote; -import java.io.IOException; -import java.io.UnsupportedEncodingException; -import java.nio.charset.Charset; -import java.util.HashMap; -import java.util.Map; -import java.util.concurrent.TimeUnit; -import java.util.concurrent.atomic.AtomicBoolean; -import java.util.concurrent.atomic.AtomicLong; -import java.util.concurrent.atomic.AtomicReference; - import jakarta.servlet.ReadListener; import jakarta.servlet.ServletConnection; - import org.apache.tomcat.util.buf.CharsetHolder; import org.apache.tomcat.util.buf.MessageBytes; import org.apache.tomcat.util.buf.UDecoder; Review Comment: This breaks the Checkstyle enforced import order rules. ########## java/org/apache/coyote/Request.java: ########## @@ -853,24 +846,22 @@ public boolean isProcessing() { * @param contentType a content type header */ private static String getCharsetFromContentType(String contentType) { - if (contentType == null) { return null; } int start = contentType.indexOf("charset="); if (start < 0) { return null; } - String encoding = contentType.substring(start + 8); - int end = encoding.indexOf(';'); + start += 8; + int end = contentType.indexOf(';', start); + String encoding; if (end >= 0) { - encoding = encoding.substring(0, end); + encoding = contentType.substring(start, end).trim(); + } else { + encoding = contentType.substring(start).trim(); } - encoding = encoding.trim(); - if ((encoding.length() > 2) && (encoding.startsWith("\"")) && (encoding.endsWith("\""))) { - encoding = encoding.substring(1, encoding.length() - 1); - } - - return encoding.trim(); + return encoding.replaceAll("\"", ""); Review Comment: This changes functionality. Granted the existing code doesn't strictly implement the requirements of RFC 9110 either but "code clean-up" PRs should not be changing functionality without explicitly drawing attention to that fact. Separately, I have concerns about the performance implications of this change. Srtring.replaceAll() is not cheap as it uses regular expressions. If we want to specification compliant here (and we probably should be) the right way to do this would be to use o.a.t.u.http.parser.MediaType. ########## java/org/apache/coyote/Request.java: ########## @@ -386,8 +385,7 @@ public void setLocalPort(int port) { * Get the character encoding used for this request. * * @return The value set via {@link #setCharset(Charset)} or if no call has been made to that method try to obtain - * if from the content type. - * + * if from the content type. Review Comment: This breaks the standard Javadoc formatting. -- 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...@tomcat.apache.org For queries about this service, please contact Infrastructure at: us...@infra.apache.org --------------------------------------------------------------------- To unsubscribe, e-mail: dev-unsubscr...@tomcat.apache.org For additional commands, e-mail: dev-h...@tomcat.apache.org