Sounds OK to me, though we should test this against an actual servlet to make
sure that the request properties are coming through correctly.
G
On Sunday, May 24, 2009, at 06:45PM, "Christopher Brind" <[email protected]>
wrote:
>Hi,
>
>Comments in line ...
>
>2009/5/24 Greg Brown <[email protected]>
>
>> Chris,
>>
>> This looks great - thanks for jumping on it so quickly. I made a few minor
>> changes:
>
>
>
>>
>> Couple of questions:
>>
>> - Should we be using addRequestProperty() here when index > 0?
>>
>> for (String key : requestHeaders) {
>> for (int index = 0; index < requestHeaders.getLength(key); index++)
>> {
>> connection.setRequestProperty(key, requestHeaders.get(key,
>> index));
>> }
>> }
>
>
>That's updating the actual connection, not the underlying properties
>classes. However, you might have spotted something else. The Javadoc for
>that set method is:
>
>"NOTE: HTTP requires all request properties which can legally have multiple
>instances with the same key to use a comma-seperated list syntax which
>enables multiple properties to be appended into a single property."
>
>Which probably explains why it was previously called RequestProperties, i.e.
>in line with the HttpURLConnection class ... doh! Personally, I still
>prefer "RequestHeaders" as it seems more explicit than RequestProperties.
>
>So, it should probably look like this:
>
> for (String key : requestHeaders) {
> StringBuffer buffer = new StringBuffer();
> for (int index = 0; index < requestHeaders.getLength(key);
>index++) {
> if (index > 0) {
> buffer.append(",");
> }
> buffer.append(requestHeaders.get(key, index));
> }
> connection.setRequestProperty(key, buffer.toString());
> }
>
>(will commit that just now)
>
>
>
>>
>>
>> - It looks like you used JUnit 3 to write the tests - any reason we
>> wouldn't want to use JUnit 4?
>
>
>I just selected the wrong library when updating the project classpath - I
>did mean to select Junit 4, though I'm still not keen on the annotation
>version of JUnit which is why I've gone for extending TestCase and having a
>test method. Can change that if you want... ?
>
>Btw, I also just removed an uncessary 'unchecked' exception warning
>supression from QueryDictionaryTest, probably introduced as a result of not
>having to have an instance of Query to create a QueryDictionary.
>
>Cheers,
>Chris
>
