Hi David,
David Daney wrote:
> David Daney wrote:
>
>> PR libgcj/26487 shows problems with our existing header handling.
>>
>> I hacked up the attached patch, which needs more testing (please test
>> it!)
>>
>> The basic problem is that the headers were being held in a map which
>> scrambled them up if there were more than one header of the same name.
>>
>> My change was to just hold the headers in a list. This makes some
>> searching operations less efficient, but seems the best way to keep
>> the headers from being combined.
>>
> New version of the patch. Just cleaned up and javadocs added.
>
> I guess I will commit it tomorrow if no one objects.
>
> 2006-03-02 David Daney <[EMAIL PROTECTED]>
>
> * gnu/java/net/protocol/http/HTTPURLConnection.java
> (getRequestProperties): Rewrote.
> (addRequestProperty): Rewrote.
> (getHeaderFields): Rewrote.
> (getHeaderField): Rewrote.
> (getHeaderFieldKey): Rewrote.
> (getHeaderField): Removed useless cast.
> * gnu/java/net/protocol/http/Headers.java: Entire class rewritten.
> * gnu/java/net/protocol/http/Request.java (dispatch): Use new
> Headers
> interface.
> (notifyHeaderHandlers): Use new Headers interface.
>
Nice to see you have removed that now useless inner Header class. This was one
of the comments I wanted to make. I worked this day on mauve testcases for this
rewrite.
These exposed two small bugs in Headers.java:
> Index: gnu/java/net/protocol/http/Headers.java
> ===================================================================
> RCS file:
> /sources/classpath/classpath/gnu/java/net/protocol/http/Headers.java,v
> retrieving revision 1.6
> diff -c -p -r1.6 Headers.java
> *** gnu/java/net/protocol/http/Headers.java 2 Mar 2006 00:26:57 -0000
> 1.6
> --- gnu/java/net/protocol/http/Headers.java 2 Mar 2006 20:40:32 -0000
[...]
> !
> ! /**
> ! * Get a new Map containing all the headers. The keys of the Map
> ! * are Strings (the header names). The values of the Map are
> ! * unmodifiable Lists containing Strings (the header values).
> ! *
> ! * <p>
> ! *
> ! * The returned map is modifiable. Changing it will not effect this
> ! * collection of Headers in any way.
> ! *
> ! * @return a Map containing all the headers.
> ! */
> ! public Map getAsMap()
> {
> ! LinkedHashMap m = new LinkedHashMap();
> ! for (Iterator it = headers.iterator(); it.hasNext(); )
> {
> ! HeaderElement e = (HeaderElement)it.next();
> ! String k = e.name.toLowerCase();
No lowercase here. Otherwise keys with uppercase names won't be found.
String k = e.name;
> ! ArrayList l = (ArrayList)m.get(k);
> ! if (l == null)
> ! {
> ! l = new ArrayList(1);
> ! l.add(e.value);
> ! m.put(k, l);
> ! }
> ! else
> ! l.add(e.value);
The second value must be added before. The test show
that SUN always adds the last received value for a
key first.
l.add(0, e.value);
> }
> ! for (Iterator it = m.entrySet().iterator(); it.hasNext(); )
> {
> ! Map.Entry me = (Map.Entry)it.next();
> ! ArrayList l = (ArrayList)me.getValue();
> ! me.setValue(Collections.unmodifiableList(l));
> }
> + return m;
> + }
> +
With these two tests all testcases here pass.
Updated diff -u patch attached.
Wolfgang
Index: gnu/java/net/protocol/http/Headers.java
===================================================================
RCS file: /cvsroot/classpath/classpath/gnu/java/net/protocol/http/Headers.java,v
retrieving revision 1.6
diff -u -r1.6 Headers.java
--- gnu/java/net/protocol/http/Headers.java 2 Mar 2006 00:26:57 -0000 1.6
+++ gnu/java/net/protocol/http/Headers.java 2 Mar 2006 21:18:28 -0000
@@ -1,5 +1,5 @@
/* Headers.java --
- Copyright (C) 2004 Free Software Foundation, Inc.
+ Copyright (C) 2004, 2006 Free Software Foundation, Inc.
This file is part of GNU Classpath.
@@ -44,124 +44,74 @@
import java.io.InputStream;
import java.text.DateFormat;
import java.text.ParseException;
+import java.util.ArrayList;
+import java.util.Collections;
import java.util.Date;
import java.util.Iterator;
import java.util.LinkedHashMap;
-import java.util.LinkedHashSet;
import java.util.Map;
-import java.util.Set;
/**
- * A collection of HTTP header names and associated values.
- * Retrieval of values is case insensitive. An iteration over the keys
+ * A collection of HTTP header names and associated values. The
+ * values are [EMAIL PROTECTED] ArrayList ArrayLists} of Strings. Retrieval of
+ * values is case insensitive. An iteration over the collection
* returns the header names in the order they were received.
*
* @author Chris Burdess ([EMAIL PROTECTED])
+ * @author David Daney ([EMAIL PROTECTED])
*/
-public class Headers
- extends LinkedHashMap
+class Headers
{
-
+ /**
+ * A list of HeaderElements
+ *
+ */
+ private final ArrayList headers = new ArrayList();
+
static final DateFormat dateFormat = new HTTPDateFormat();
- static class Header
+ static class HeaderElement
{
+ String name;
+ String value;
- final String name;
-
- Header(String name)
+ HeaderElement(String name, String value)
{
- if (name == null || name.length() == 0)
- {
- throw new IllegalArgumentException(name);
- }
this.name = name;
+ this.value = value;
}
-
- public int hashCode()
- {
- return name.toLowerCase().hashCode();
- }
-
- public boolean equals(Object other)
- {
- if (other instanceof Header)
- {
- return ((Header) other).name.equalsIgnoreCase(name);
- }
- return false;
- }
-
- public String toString()
- {
- return name;
- }
-
- }
-
- static class HeaderEntry
- implements Map.Entry
- {
-
- final Map.Entry entry;
-
- HeaderEntry(Map.Entry entry)
- {
- this.entry = entry;
- }
-
- public Object getKey()
- {
- return ((Header) entry.getKey()).name;
- }
-
- public Object getValue()
- {
- return entry.getValue();
- }
-
- public Object setValue(Object value)
- {
- return entry.setValue(value);
- }
-
- public int hashCode()
- {
- return entry.hashCode();
- }
-
- public boolean equals(Object other)
- {
- return entry.equals(other);
- }
-
- public String toString()
- {
- return getKey().toString() + "=" + getValue();
- }
-
}
public Headers()
{
}
- public boolean containsKey(Object key)
- {
- return super.containsKey(new Header((String) key));
- }
-
- public Object get(Object key)
+ /**
+ * Return an Iterator over this collection of headers.
+ * Iterator.getNext() returns objects of type [EMAIL PROTECTED] HeaderElement}.
+ *
+ * @return the Iterator.
+ */
+ Iterator iterator()
{
- return super.get(new Header((String) key));
+ return headers.iterator();
}
-
+
/**
- * Returns the value of the specified header as a string.
+ * Returns the value of the specified header as a string. If
+ * multiple values are present, the last one is returned.
*/
public String getValue(String header)
{
- return (String) super.get(new Header(header));
+ for (int i = headers.size() - 1; i >= 0; i--)
+ {
+ HeaderElement e = (HeaderElement)headers.get(i);
+ if (e.name.equalsIgnoreCase(header))
+ {
+ return e.value;
+ }
+ }
+ return null;
}
/**
@@ -227,51 +177,62 @@
}
}
- public Object put(Object key, Object value)
- {
- return super.put(new Header((String) key), value);
- }
-
- public Object remove(Object key)
+ /**
+ * Add a header to this set of headers. If there is an existing
+ * header with the same name, it is discarded.
+ *
+ * @param name the header name
+ * @param value the header value
+ *
+ * @see #addValue
+ */
+ public void put(String name, String value)
{
- return super.remove(new Header((String) key));
+ remove(name);
+ headers.add(headers.size(), new HeaderElement(name, value));
}
- public void putAll(Map t)
+ /**
+ * Add all headers from a set of headers to this set. If any of the
+ * headers to be added have the same name as existing headers, the
+ * existing headers will be discarded.
+ *
+ * @param o the headers to be added
+ */
+ public void putAll(Headers o)
{
- for (Iterator i = t.keySet().iterator(); i.hasNext(); )
+ for (Iterator it = o.iterator(); it.hasNext(); )
{
- String key = (String) i.next();
- String value = (String) t.get(key);
- put(key, value);
+ HeaderElement e = (HeaderElement)it.next();
+ remove(e.name);
}
- }
-
- public Set keySet()
- {
- Set keys = super.keySet();
- Set ret = new LinkedHashSet();
- for (Iterator i = keys.iterator(); i.hasNext(); )
+ for (Iterator it = o.iterator(); it.hasNext(); )
{
- ret.add(((Header) i.next()).name);
+ HeaderElement e = (HeaderElement)it.next();
+ addValue(e.name, e.value);
}
- return ret;
}
- public Set entrySet()
+ /**
+ * Remove a header from this set of headers. If there is more than
+ * one instance of a header of the given name, they are all removed.
+ *
+ * @param name the header name
+ */
+ public void remove(String name)
{
- Set entries = super.entrySet();
- Set ret = new LinkedHashSet();
- for (Iterator i = entries.iterator(); i.hasNext(); )
+ for (Iterator it = headers.iterator(); it.hasNext(); )
{
- Map.Entry entry = (Map.Entry) i.next();
- ret.add(new HeaderEntry(entry));
+ HeaderElement e = (HeaderElement)it.next();
+ if (e.name.equalsIgnoreCase(name))
+ it.remove();
}
- return ret;
}
/**
- * Parse the specified input stream, adding headers to this collection.
+ * Parse the specified InputStream, adding headers to this collection.
+ *
+ * @param in the InputStream.
*/
public void parse(InputStream in)
throws IOException
@@ -333,18 +294,91 @@
}
}
- private void addValue(String name, String value)
+
+ /**
+ * Add a header to this set of headers. If there is an existing
+ * header with the same name, it is not effected.
+ *
+ * @param name the header name
+ * @param value the header value
+ *
+ * @see #put
+ */
+ public void addValue(String name, String value)
+ {
+ headers.add(headers.size(), new HeaderElement(name, value));
+ }
+
+ /**
+ * Get a new Map containing all the headers. The keys of the Map
+ * are Strings (the header names). The values of the Map are
+ * unmodifiable Lists containing Strings (the header values).
+ *
+ * <p>
+ *
+ * The returned map is modifiable. Changing it will not effect this
+ * collection of Headers in any way.
+ *
+ * @return a Map containing all the headers.
+ */
+ public Map getAsMap()
{
- Header key = new Header(name);
- String old = (String) super.get(key);
- if (old == null)
+ LinkedHashMap m = new LinkedHashMap();
+ for (Iterator it = headers.iterator(); it.hasNext(); )
{
- super.put(key, value);
+ HeaderElement e = (HeaderElement)it.next();
+ String k = e.name;
+ ArrayList l = (ArrayList)m.get(k);
+ if (l == null)
+ {
+ l = new ArrayList(1);
+ l.add(e.value);
+ m.put(k, l);
+ }
+ else
+ l.add(0, e.value);
}
- else
+ for (Iterator it = m.entrySet().iterator(); it.hasNext(); )
{
- super.put(key, old + ", " + value);
+ Map.Entry me = (Map.Entry)it.next();
+ ArrayList l = (ArrayList)me.getValue();
+ me.setValue(Collections.unmodifiableList(l));
}
+ return m;
+ }
+
+ /**
+ * Get the name of the Nth header.
+ *
+ * @param i the header index.
+ *
+ * @return the header name.
+ *
+ * @see #getHeaderValue
+ */
+ public String getHeaderName(int i)
+ {
+ if (i >= headers.size() || i < 0)
+ return null;
+
+ return ((HeaderElement)headers.get(i)).name;
+ }
+
+ /**
+ * Get the value of the Nth header.
+ *
+ * @param i the header index.
+ *
+ * @return the header value.
+ *
+ * @see #getHeaderName
+ */
+ public String getHeaderValue(int i)
+ {
+ if (i >= headers.size() || i < 0)
+ return null;
+
+ return ((HeaderElement)headers.get(i)).value;
}
}
Index: gnu/java/net/protocol/http/Request.java
===================================================================
RCS file: /cvsroot/classpath/classpath/gnu/java/net/protocol/http/Request.java,v
retrieving revision 1.8
diff -u -r1.8 Request.java
--- gnu/java/net/protocol/http/Request.java 9 Feb 2006 09:23:38 -0000 1.8
+++ gnu/java/net/protocol/http/Request.java 2 Mar 2006 21:18:29 -0000
@@ -302,12 +302,10 @@
String line = method + ' ' + requestUri + ' ' + version + CRLF;
out.write(line.getBytes(US_ASCII));
// Request headers
- for (Iterator i = requestHeaders.keySet().iterator();
- i.hasNext(); )
+ for (Iterator i = requestHeaders.iterator(); i.hasNext(); )
{
- String name =(String) i.next();
- String value =(String) requestHeaders.get(name);
- line = name + HEADER_SEP + value + CRLF;
+ Headers.HeaderElement elt = (Headers.HeaderElement)i.next();
+ line = elt.name + HEADER_SEP + elt.value + CRLF;
out.write(line.getBytes(US_ASCII));
}
out.write(CRLF.getBytes(US_ASCII));
@@ -438,23 +436,17 @@
void notifyHeaderHandlers(Headers headers)
{
- for (Iterator i = headers.entrySet().iterator(); i.hasNext(); )
+ for (Iterator i = headers.iterator(); i.hasNext(); )
{
- Map.Entry entry = (Map.Entry) i.next();
- String name =(String) entry.getKey();
+ Headers.HeaderElement entry = (Headers.HeaderElement) i.next();
// Handle Set-Cookie
- if ("Set-Cookie".equalsIgnoreCase(name))
- {
- String value = (String) entry.getValue();
- handleSetCookie(value);
- }
+ if ("Set-Cookie".equalsIgnoreCase(entry.name))
+ handleSetCookie(entry.value);
+
ResponseHeaderHandler handler =
- (ResponseHeaderHandler) responseHeaderHandlers.get(name);
+ (ResponseHeaderHandler) responseHeaderHandlers.get(entry.name);
if (handler != null)
- {
- String value = (String) entry.getValue();
- handler.setValue(value);
- }
+ handler.setValue(entry.value);
}
}
Index: gnu/java/net/protocol/http/HTTPURLConnection.java
===================================================================
RCS file: /cvsroot/classpath/classpath/gnu/java/net/protocol/http/HTTPURLConnection.java,v
retrieving revision 1.22
diff -u -r1.22 HTTPURLConnection.java
--- gnu/java/net/protocol/http/HTTPURLConnection.java 27 Feb 2006 23:37:20 -0000 1.22
+++ gnu/java/net/protocol/http/HTTPURLConnection.java 2 Mar 2006 21:18:29 -0000
@@ -422,20 +422,7 @@
if (connected)
throw new IllegalStateException("Already connected");
- HashMap m = new HashMap(requestHeaders);
- Iterator it = m.entrySet().iterator();
- while (it.hasNext())
- {
- Map.Entry e = (Map.Entry)it.next();
- String s = (String)e.getValue();
- String sa[] = s.split(",");
- ArrayList l = new ArrayList(sa.length);
- for (int i = 0; i < sa.length; i++)
- {
- l.add(sa[i].trim());
- }
- e.setValue(Collections.unmodifiableList(l));
- }
+ Map m = requestHeaders.getAsMap();
return Collections.unmodifiableMap(m);
}
@@ -449,16 +436,7 @@
public void addRequestProperty(String key, String value)
{
super.addRequestProperty(key, value);
-
- String old = requestHeaders.getValue(key);
- if (old == null)
- {
- requestHeaders.put(key, value);
- }
- else
- {
- requestHeaders.put(key, old + "," + value);
- }
+ requestHeaders.addValue(key, value);
}
public OutputStream getOutputStream()
@@ -533,17 +511,9 @@
return null;
}
}
- Headers headers = response.getHeaders();
- LinkedHashMap ret = new LinkedHashMap();
- ret.put(null, Collections.singletonList(getStatusLine(response)));
- for (Iterator i = headers.entrySet().iterator(); i.hasNext(); )
- {
- Map.Entry entry = (Map.Entry) i.next();
- String key = (String) entry.getKey();
- String value = (String) entry.getValue();
- ret.put(key, Collections.singletonList(value));
- }
- return Collections.unmodifiableMap(ret);
+ Map m = response.getHeaders().getAsMap();
+ m.put(null, Collections.singletonList(getStatusLine(response)));
+ return Collections.unmodifiableMap(m);
}
String getStatusLine(Response response)
@@ -571,20 +541,7 @@
{
return getStatusLine(response);
}
- Iterator i = response.getHeaders().entrySet().iterator();
- Map.Entry entry;
- int count = 1;
- do
- {
- if (!i.hasNext())
- {
- return null;
- }
- entry = (Map.Entry) i.next();
- count++;
- }
- while (count <= index);
- return (String) entry.getValue();
+ return response.getHeaders().getHeaderValue(index - 1);
}
public String getHeaderFieldKey(int index)
@@ -600,24 +557,8 @@
return null;
}
}
- if (index == 0)
- {
- return null;
- }
- Iterator i = response.getHeaders().entrySet().iterator();
- Map.Entry entry;
- int count = 1;
- do
- {
- if (!i.hasNext())
- {
- return null;
- }
- entry = (Map.Entry) i.next();
- count++;
- }
- while (count <= index);
- return (String) entry.getKey();
+ // index of zero is the status line.
+ return response.getHeaders().getHeaderName(index - 1);
}
public String getHeaderField(String name)
@@ -633,7 +574,7 @@
return null;
}
}
- return (String) response.getHeader(name);
+ return response.getHeader(name);
}
public long getHeaderFieldDate(String name, long def)
