2012/8/17 <ma...@apache.org>:
> Author: markt
> Date: Thu Aug 16 21:50:44 2012
> New Revision: 1374073
>
> URL: http://svn.apache.org/viewvc?rev=1374073&view=rev
> Log:
> FindBugs: Fix unused code by implementing the TODO associated with it
>
> Added:
> tomcat/trunk/java/org/apache/tomcat/util/http/ResponseUtil.java (with
> props)
> Modified:
> tomcat/trunk/java/org/apache/catalina/connector/Response.java
> tomcat/trunk/java/org/apache/coyote/Response.java
> tomcat/trunk/webapps/docs/changelog.xml
>
> Modified: tomcat/trunk/java/org/apache/catalina/connector/Response.java
> URL:
> http://svn.apache.org/viewvc/tomcat/trunk/java/org/apache/catalina/connector/Response.java?rev=1374073&r1=1374072&r2=1374073&view=diff
> ==============================================================================
> --- tomcat/trunk/java/org/apache/catalina/connector/Response.java (original)
> +++ tomcat/trunk/java/org/apache/catalina/connector/Response.java Thu Aug 16
> 21:50:44 2012
> @@ -51,6 +51,7 @@ import org.apache.tomcat.util.buf.CharCh
> import org.apache.tomcat.util.buf.UEncoder;
> import org.apache.tomcat.util.http.FastHttpDateFormat;
> import org.apache.tomcat.util.http.MimeHeaders;
> +import org.apache.tomcat.util.http.ResponseUtil;
> import org.apache.tomcat.util.http.ServerCookie;
> import org.apache.tomcat.util.http.parser.AstMediaType;
> import org.apache.tomcat.util.http.parser.HttpParser;
> @@ -1023,8 +1024,8 @@ public class Response
> /**
> * An extended version of this exists in {@link
> org.apache.coyote.Response}.
> * This check is required here to ensure that the usingWriter checks in
> - * {@link #setContentType(String)} are applied since usingWriter is not
> - * visible to {@link org.apache.coyote.Response}
> + * {@link #setContentType(String)} and {@link #setLocale(Locale) are
> applied
> + * since usingWriter is not visible to {@link org.apache.coyote.Response}
> *
> * Called from set/addHeader.
> * Return true if the header is special, no need to set the header.
> @@ -1034,6 +1035,15 @@ public class Response
> setContentType(value);
> return true;
> }
> + if (name.equalsIgnoreCase("Content-Language")) {
> + Locale locale = ResponseUtil.getLocaleFromLanguageHeader(value);
> + if (locale == null) {
> + return false;
> + } else {
> + setLocale(locale);
> + return true;
> + }
> + }
> return false;
> }
>
>
> Modified: tomcat/trunk/java/org/apache/coyote/Response.java
> URL:
> http://svn.apache.org/viewvc/tomcat/trunk/java/org/apache/coyote/Response.java?rev=1374073&r1=1374072&r2=1374073&view=diff
> ==============================================================================
> --- tomcat/trunk/java/org/apache/coyote/Response.java (original)
> +++ tomcat/trunk/java/org/apache/coyote/Response.java Thu Aug 16 21:50:44 2012
> @@ -26,6 +26,7 @@ import javax.servlet.WriteListener;
> import org.apache.coyote.http11.AbstractOutputBuffer;
> import org.apache.tomcat.util.buf.ByteChunk;
> import org.apache.tomcat.util.http.MimeHeaders;
> +import org.apache.tomcat.util.http.ResponseUtil;
> import org.apache.tomcat.util.http.parser.AstMediaType;
> import org.apache.tomcat.util.http.parser.HttpParser;
> import org.apache.tomcat.util.http.parser.ParseException;
> @@ -342,8 +343,14 @@ public final class Response {
> return false;
> }
> }
> - if( name.equalsIgnoreCase( "Content-Language" ) ) {
> - // XXX XXX Need to construct Locale or something else
> + if (name.equalsIgnoreCase("Content-Language")) {
> + Locale locale = ResponseUtil.getLocaleFromLanguageHeader(value);
> + if (locale == null) {
> + return false;
> + } else {
> + setLocale(locale);
> + return true;
> + }
> }
> return false;
> }
>
> Added: tomcat/trunk/java/org/apache/tomcat/util/http/ResponseUtil.java
> URL:
> http://svn.apache.org/viewvc/tomcat/trunk/java/org/apache/tomcat/util/http/ResponseUtil.java?rev=1374073&view=auto
> ==============================================================================
> --- tomcat/trunk/java/org/apache/tomcat/util/http/ResponseUtil.java (added)
> +++ tomcat/trunk/java/org/apache/tomcat/util/http/ResponseUtil.java Thu Aug
> 16 21:50:44 2012
> @@ -0,0 +1,60 @@
> +/*
> + * Licensed to the Apache Software Foundation (ASF) under one or more
> + * contributor license agreements. See the NOTICE file distributed with
> + * this work for additional information regarding copyright ownership.
> + * The ASF licenses this file to You under the Apache License, Version 2.0
> + * (the "License"); you may not use this file except in compliance with
> + * the License. You may obtain a copy of the License at
> + *
> + * http://www.apache.org/licenses/LICENSE-2.0
> + *
> + * Unless required by applicable law or agreed to in writing, software
> + * distributed under the License is distributed on an "AS IS" BASIS,
> + * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
> + * See the License for the specific language governing permissions and
> + * limitations under the License.
> + */
> +package org.apache.tomcat.util.http;
> +
> +import java.util.Locale;
> +
> +public class ResponseUtil {
> +
> + private ResponseUtil() {
> + // Hide default constructor as this is a utility class
> + }
> +
> + public static Locale getLocaleFromLanguageHeader(String header) {
> + if (header == null) {
> + return null;
> + }
> +
> + if (header.indexOf(',') > -1) {
> + // Multiple values. RFC 2616 does not define a priority.
> + // No way to select a Locale
> + return null;
> + }
> +
> + String tags[] = header.split("-");
> + String primaryTag = tags[0];
> +
> + if (primaryTag.length() != 2) {
> + // Not an ISO-639 language abbreviation. No way to determine
> Locale
> + return null;
> + }
> +
> + String firstSubTag = null;
> + if (tags.length > 1) {
> + if (tags[1].length() == 2) {
> + // Hopefully an ISO-3166 country code
> + firstSubTag = tags[1];
> + }
> + }
> +
> + if (firstSubTag == null) {
> + return new Locale(primaryTag);
> + } else {
> + return new Locale(primaryTag, firstSubTag);
> + }
> + }
> +}
>
> Propchange: tomcat/trunk/java/org/apache/tomcat/util/http/ResponseUtil.java
> ------------------------------------------------------------------------------
> svn:eol-style = native
>
> Modified: tomcat/trunk/webapps/docs/changelog.xml
> URL:
> http://svn.apache.org/viewvc/tomcat/trunk/webapps/docs/changelog.xml?rev=1374073&r1=1374072&r2=1374073&view=diff
> ==============================================================================
> --- tomcat/trunk/webapps/docs/changelog.xml (original)
> +++ tomcat/trunk/webapps/docs/changelog.xml Thu Aug 16 21:50:44 2012
> @@ -92,6 +92,12 @@
> Service. This removes the need to maintain two copies of the mappings
> for Servlets and Filters. (markt)
> </scode>
> + <add>
> + If the <code>Content-Language</code> HTTP header is set directly,
> + attempt to determine the Locale from the header value and call
> + <code>ServletResponse.setLocale()</code> with the derived Locale.
> + (markt)
> + </add>
> </changelog>
> </subsection>
> <subsection name="Coyote">
I do not like it.
My concerns are that
================
1) It contradicts the specification of ServletResponse.getLocale().
The Javadoc for ServletResponse.getLocale() in Servlet 3.0 spec is the
same as in Tomcat source code and says that the method returns the
value set by setLocale() and is the container's default locale
otherwise.
Effectively this commit changes the value returned by getLocale() so
that the default locale is no longer returned.
It seems reasonable to expect an explicit call to setLocale(), so not
to bother with creation of Locale object.
2) This implementation may change the value of Content-Language header
It relies on roundtrip of header -> Locale -> header, but this
roundtrip may change the value of the header (explained below). I
think that if the header has been set explicitly, it would be better
to preserve it as is.
Regarding the roundtrip:
===================
- When any of those "checkSpecialHeader()" methods return true, it
means that the header will not be set.
- Later in AbstractHttpProcessor#prepareResponse() Tomcat will create
the header from Locale.
- The Locale class constructor converts language to lowercase and also
maps some "old" codes to other ones.
- There is a problem with ResponseUtil.getLocaleFromLanguageHeader():
It checks "if (tags.length > 1)". If the purpose of the method is to
reconstruct a locale, it is OK. But if the purpose is to get exact
match then it should check "if (tags.length == 2)", because only 2
components are used to create a Locale. According to RFC2616 ch 3.10
there may be an arbitrary count of subtags.
language-tag = primary-tag *( "-" subtag )
One more bug in ResponseUtil.getLocaleFromLanguageHeader():
===================================
"if (primaryTag.length() != 2)"
should be "if (primaryTag.length() != 2 && primaryTag.length() != 3)"
RFC2616 ch 3.10 says that language codes are 2 characters, but this
information is outdated. The language codes can be 3 characters as
well.
The IANA registry is defined by RFC 5646 and can be seen here: Look for "aaa".
http://www.iana.org/protocols/
http://www.iana.org/assignments/language-subtag-registry/
The Locale(String,String) constructor in JDK 7u05 says "An ISO 639
alpha-2 or alpha-3 language code".
Best regards,
Konstantin Kolinko
---------------------------------------------------------------------
To unsubscribe, e-mail: dev-unsubscr...@tomcat.apache.org
For additional commands, e-mail: dev-h...@tomcat.apache.org
