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 > > >
