Wolfgang Baer wrote:
Hi David,
David Daney wrote:
Wolfgang Baer wrote:
Hi,
I wrote mauve tests for the various HttpURLConnection request properties
methods and notices some minor bugs in the new Headers implementation.
The patch fixes these and adds documentation to this class in more
detail.
The tests show that put method used in setRequestProperty should not
remove
all occurences of the property but only the last on in the list. Also as
its internal case sensitive the key must not be replaced (only the
value),
otherwise getAsMap() can produce wrong maps. I reimplemented putAll()
with
the same semantics.
I wrote both put and putAll to mimic their old bahavior. I wanted to
change the behavior as little as possible, while still fixing it.
putAll() IIRC is only used internally, to propagate the headers into the
Request object. I question whether it should be changed.
OK
If you have test cases that show that the current putAll() is broken,
then the change should be made. But change just for change's sake I am
not so sure.
No, I have no testcases for putAll() - only for put(). As you said its only
used internally to override settings. The change won't affect the current
usage so far. The reason for reimplementation was more that people (later if
they use that class) may assume that putAll() behaves as put() on a whole
Headers collection which is not true.
If we don't change the method we need to document clearly that its not the
behaviour of put() on a Headers collection. Also I think we can at least save
one of the iterations.
The thing I worry about is headers like 'Content-Length'. In order for
HTTP to function correctly we must supply the correct value. I think by
allowing putAll() to append, we might have a situation where there could
be multiple headers for some critical protocol headers that could break
things.
