On 2010/07/27 20:09:03, zhoresh wrote:
On 2010/07/27 05:36:14, gagan.goku wrote: > lgtm > > http://codereview.appspot.com/1867046/diff/1/2 > File >
java/gadgets/src/main/java/org/apache/shindig/gadgets/servlet/ProxyHandler.java
> (right): > > http://codereview.appspot.com/1867046/diff/1/2#newcode146 >
java/gadgets/src/main/java/org/apache/shindig/gadgets/servlet/ProxyHandler.java:146:
> // Skip illegal header > This is fine for now but probably the right way of fixing this issue everywhere > is to use UriUtils.isValidHeaderName and
UriUtils.isValidHeaderValue.
> Though UriUtils.isValidHeaderValue is not yet implemented, it might
make sense
> to do that. The rfc is clear on what values are acceptable for
header names
and > values. > Also, see http://codereview.appspot.com/1855044/diff/45001/46005 for
another
way > of refactoring ProxyHandler.
Since the HttpResponse already have the logic for valid header I don't
see the
real need to do it twice in this case. Just handle the exception that is rasied for bad header.
Submitted (r979836)
There is no logic in HttpResponse for checking if the header is a valid header or not. Also, there are no guarantees that this exception will be thrown by any other servlet engine (jetty's Response for example does not seem to throw an IllegalArgumentException), which is why i was saying that a better way would be to implement the rfc compliant isValidHeaderName and isValidHeaderValue and use these instead. Lets try that when we revisit http headers :) http://codereview.appspot.com/1867046/show
