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: [email protected]
For queries about this service, please contact Infrastructure at:
[email protected]
---------------------------------------------------------------------
To unsubscribe, e-mail: [email protected]
For additional commands, e-mail: [email protected]
