Hi, Sorry Greg, I didn't understand what you were saying initially about the addRequestHeader. I've done some investigation and ran the code using the web-test-war.
If we use connection.addRequestHeader then you get output like this: 10:21:34,656 INFO [STDOUT] [Request Headers] 10:21:34,657 INFO [STDOUT] test-header: Value1 10:21:34,657 INFO [STDOUT] test-header: Value2 i.e. multiple headers, but if we use the other method of comma separating then we get output like this: 10:23:02,173 INFO [STDOUT] test-header: Value1,Value2 i.e. a single header, not that surprising, though I think the Javadocs are not as clear as they could be. So what I think the Javadocs really mean by "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." is that *pre-defined HTTP headers* e.g. * accept* ( http://www.w3.org/Protocols/rfc2616/rfc2616-sec14.html ) must use comma separated values to handle multiple instances. Custom headers can do what they want, I suppose, since they are not going to be handled as part of the protocol by a web-server, for instance and will be interpreted appropriately at the developer's discretion. So what is the use-case that we're catering for? My feeling is that we should use the addRequestHeaders method as you initially suggested and generate multiple headers for the same key rather than build a comma separated list. Additionally, we need to document clearly which one of the QueryDictionary methods should be used depending on the scenario. For instance, if the developer really does want to override the accept header with multiple values they should know that they have to build a comma separated list and then call getRequestHeaders().set("accept", csv); By defaulting to the multiple headers scenario it means developers have the choice, rather than us assuming they'd want a comma separated list of values in a single header. If you agree, let me know and I'll commit the change. Cheers, Chris 2009/5/25 Christopher Brind <[email protected]> > > OK will do. I'm pretty busy today but I'll try and do it today or > tomorrow subject to time. It should be something that is testable as a unit > test, if not I'll do it the long way. :) > > Cheers, > Chris > > > 2009/5/25 Greg Brown <[email protected]> > > 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 >> > >> > >
