Fixing ServletUtil.fromHttpServletRequest to take multiple header values for same header name

Currently ServletUtil.fromHttpServletRequest() iterates over all the header names and add the header value returned by getHeader(headerName).
But this is buggy because getHeader() method returns only 1 header value, which is almost always the first header value for that headerName.

This change refactors fromHttpServletRequest() to take multiple header values for the same header name.

[anupama.dutta, 2010-08-13 13:23:51]

http://codereview.appspot.com/1944044/diff/2001/3002
File
java/gadgets/src/main/java/org/apache/shindig/gadgets/servlet/ServletUtil.java
(right):

http://codereview.appspot.com/1944044/diff/2001/3002#newcode62
java/gadgets/src/main/java/org/apache/shindig/gadgets/servlet/ServletUtil.java:62:
Enumeration<?> headerNames = servletReq.getHeaderNames();
Wondering why we can't use UriUtils.copyRequestHeaders directly here, instead of
copying over the code!

[gagan.goku, 2010-08-13 18:40:31]

http://codereview.appspot.com/1944044/diff/2001/3002
File
java/gadgets/src/main/java/org/apache/shindig/gadgets/servlet/ServletUtil.java
(right):

http://codereview.appspot.com/1944044/diff/2001/3002#newcode62
java/gadgets/src/main/java/org/apache/shindig/gadgets/servlet/ServletUtil.java:62:
Enumeration<?> headerNames = servletReq.getHeaderNames();
On 2010/08/13 13:23:51, anupama.dutta wrote:
> Wondering why we can't use UriUtils.copyRequestHeaders directly here, instead
of
> copying over the code!

The problem is that copyRequestHeaders has been refactored to copy request
headers from 1 HttpRequest to another HttpRequest.

[anupama.dutta, 2010-08-14 03:12:27]

http://codereview.appspot.com/1944044/diff/2001/3002
File
java/gadgets/src/main/java/org/apache/shindig/gadgets/servlet/ServletUtil.java
(right):

http://codereview.appspot.com/1944044/diff/2001/3002#newcode62
java/gadgets/src/main/java/org/apache/shindig/gadgets/servlet/ServletUtil.java:62:
Enumeration<?> headerNames = servletReq.getHeaderNames();
On 2010/08/13 18:40:31, gagan.goku wrote:
> On 2010/08/13 13:23:51, anupama.dutta wrote:
> > Wondering why we can't use UriUtils.copyRequestHeaders directly here,
instead
> of
> > copying over the code!
> 
> The problem is that copyRequestHeaders has been refactored to copy request
> headers from 1 HttpRequest to another HttpRequest.

Is it possible to do some kind of refactoring of the copyRequestHeaders method
which will allow you to have the iteration logic in just one place. For example,
suppose we have getHeaderEntries method which does the iteration and returns a
multi-map that is guaranteed to have only valid header names and values and then
we insert them into the HttpServletRequest or HttpRequest object as appropriate
in the original functions (fromServletRequest or copyRequestHeaders
respectively)?

[anupama.dutta, 2010-08-16 06:30:21]

On 2010/08/14 03:12:27, anupama.dutta wrote:
> http://codereview.appspot.com/1944044/diff/2001/3002
> File
> java/gadgets/src/main/java/org/apache/shindig/gadgets/servlet/ServletUtil.java
> (right):
> 
> http://codereview.appspot.com/1944044/diff/2001/3002#newcode62
>
java/gadgets/src/main/java/org/apache/shindig/gadgets/servlet/ServletUtil.java:62:
> Enumeration<?> headerNames = servletReq.getHeaderNames();
> On 2010/08/13 18:40:31, gagan.goku wrote:
> > On 2010/08/13 13:23:51, anupama.dutta wrote:
> > > Wondering why we can't use UriUtils.copyRequestHeaders directly here,
> instead
> > of
> > > copying over the code!
> > 
> > The problem is that copyRequestHeaders has been refactored to copy request
> > headers from 1 HttpRequest to another HttpRequest.
> 
> Is it possible to do some kind of refactoring of the copyRequestHeaders method
> which will allow you to have the iteration logic in just one place. For
example,
> suppose we have getHeaderEntries method which does the iteration and returns a
> multi-map that is guaranteed to have only valid header names and values and
then
> we insert them into the HttpServletRequest or HttpRequest object as
appropriate
> in the original functions (fromServletRequest or copyRequestHeaders
> respectively)?

As mentioned offline, the current code change looks fine.
LGTM. Please send to dev@ for review and commit.

[Paul Lindner, 2010-08-16 07:12:13]

lgtm, committing

[gagan.goku, 2010-08-16 07:23:24]

Thanks Paul

-- 
The only thing missing in life is background music.
-- Gagandeep Singh


On Mon, Aug 16, 2010 at 12:42 PM, <lindner@inuus.com> wrote:

> lgtm, committing
>
>
> http://codereview.appspot.com/1944044/show
>