Forwarding all http request headers when fetching a resource via ProxyHandler

Currently ProxyHandler just takes in the proxyUri to fetch and totally ignores the http request headers like "User-Agent".

This change passes on the HttpRequest object which has all the headers present in original request - Host and Content-Length header.

[gagan.goku, 2010-08-22 14:54:32]

Please take a look. Will add appropriate if the idea looks okay and bug is
important to fix.

[Kuntal Loya, 2010-09-02 13:34:59]

http://codereview.appspot.com/1987047/diff/4001/5001
File
java/gadgets/src/main/java/org/apache/shindig/gadgets/servlet/ProxyHandler.java
(right):

http://codereview.appspot.com/1987047/diff/4001/5001#newcode92
java/gadgets/src/main/java/org/apache/shindig/gadgets/servlet/ProxyHandler.java:92:
ProxyUriManager.ProxyUri proxyUri = proxyUriManager.process(
Can the ProxyUri not store information about the original request - something
like a ProxyRequest?

[gagan.goku, 2010-09-07 01:20:50]

http://codereview.appspot.com/1987047/diff/4001/5001
File
java/gadgets/src/main/java/org/apache/shindig/gadgets/servlet/ProxyHandler.java
(right):

http://codereview.appspot.com/1987047/diff/4001/5001#newcode92
java/gadgets/src/main/java/org/apache/shindig/gadgets/servlet/ProxyHandler.java:92:
ProxyUriManager.ProxyUri proxyUri = proxyUriManager.process(
On 2010/09/02 13:34:59, Kuntal Loya wrote:
> Can the ProxyUri not store information about the original request - something
> like a ProxyRequest?

like we discussed, ProxyUri is a wrapper over the url that is getting proxied
through shindig. It is mostly independent of the request like http headers.
If we want to use http header information, HttpRequest object is more suited as
it has the whole http request and the container information as well.

[Kuntal Loya, 2010-09-17 09:36:04]

http://codereview.appspot.com/1987047/diff/4001/java/gadgets/src/main/java/or...
File
java/gadgets/src/main/java/org/apache/shindig/gadgets/servlet/ProxyHandler.java
(right):

http://codereview.appspot.com/1987047/diff/4001/java/gadgets/src/main/java/or...
java/gadgets/src/main/java/org/apache/shindig/gadgets/servlet/ProxyHandler.java:85:
UriUtils.DisallowedHeaders.HOST);
Why should the host be not copied?

http://codereview.appspot.com/1987047/diff/4001/java/gadgets/src/main/java/or...
java/gadgets/src/main/java/org/apache/shindig/gadgets/servlet/ProxyHandler.java:95:
HttpRequest rcr = buildHttpRequest(httpRequest, proxyUri,
proxyUri.getResource());
Have httpRequest and tgt uri as the params.

http://codereview.appspot.com/1987047/diff/4001/java/gadgets/src/main/java/or...
java/gadgets/src/main/java/org/apache/shindig/gadgets/servlet/ProxyHandler.java:160:
// response.setHeader("Content-Disposition", "attachment;filename=p.txt");
unrelated change

http://codereview.appspot.com/1987047/diff/4001/java/gadgets/src/main/java/or...
File
java/gadgets/src/main/java/org/apache/shindig/gadgets/servlet/ProxyServlet.java
(right):

http://codereview.appspot.com/1987047/diff/4001/java/gadgets/src/main/java/or...
java/gadgets/src/main/java/org/apache/shindig/gadgets/servlet/ProxyServlet.java:89:
ProxyUriManager.ProxyUri proxyUri = proxyUriManager.process(reqUri);
multiple proxyUri objects created for a single request - once here, and once to
build the request.
We should try to reuse them, or put the related logic together.

[Kuntal Loya, 2010-09-21 07:22:07]

http://codereview.appspot.com/1987047/diff/4001/java/gadgets/src/main/java/or...
File
java/gadgets/src/main/java/org/apache/shindig/gadgets/servlet/ProxyHandler.java
(right):

http://codereview.appspot.com/1987047/diff/4001/java/gadgets/src/main/java/or...
java/gadgets/src/main/java/org/apache/shindig/gadgets/servlet/ProxyHandler.java:85:
UriUtils.DisallowedHeaders.HOST);
On 2010/09/17 09:36:04, Kuntal Loya wrote:
> Why should the host be not copied?

Ah, got this one.
Just add a few comments here as to what are we copying/not copying and why.

[gagan.goku, 2010-09-28 09:32:52]

http://codereview.appspot.com/1987047/diff/4001/java/gadgets/src/main/java/or...
File
java/gadgets/src/main/java/org/apache/shindig/gadgets/servlet/ProxyHandler.java
(right):

http://codereview.appspot.com/1987047/diff/4001/java/gadgets/src/main/java/or...
java/gadgets/src/main/java/org/apache/shindig/gadgets/servlet/ProxyHandler.java:85:
UriUtils.DisallowedHeaders.HOST);
On 2010/09/17 09:36:04, Kuntal Loya wrote:
> Why should the host be not copied?

because if proxy request is example.org/gadgets/proxy?url=http://test.com/
then host header is "Host: example.org"
whereas we want to fetch http://test.com

http://codereview.appspot.com/1987047/diff/4001/java/gadgets/src/main/java/or...
java/gadgets/src/main/java/org/apache/shindig/gadgets/servlet/ProxyHandler.java:85:
UriUtils.DisallowedHeaders.HOST);
On 2010/09/21 07:22:07, Kuntal Loya wrote:
> On 2010/09/17 09:36:04, Kuntal Loya wrote:
> > Why should the host be not copied?
> 
> Ah, got this one.
> Just add a few comments here as to what are we copying/not copying and why.

Done.

http://codereview.appspot.com/1987047/diff/4001/java/gadgets/src/main/java/or...
java/gadgets/src/main/java/org/apache/shindig/gadgets/servlet/ProxyHandler.java:95:
HttpRequest rcr = buildHttpRequest(httpRequest, proxyUri,
proxyUri.getResource());
On 2010/09/17 09:36:04, Kuntal Loya wrote:
> Have httpRequest and tgt uri as the params.

Done.

http://codereview.appspot.com/1987047/diff/4001/java/gadgets/src/main/java/or...
java/gadgets/src/main/java/org/apache/shindig/gadgets/servlet/ProxyHandler.java:160:
// response.setHeader("Content-Disposition", "attachment;filename=p.txt");
On 2010/09/17 09:36:04, Kuntal Loya wrote:
> unrelated change

Done.

[Kuntal Loya, 2010-09-30 20:46:10]

Code looks better now, some minor comments though -
Will look at the tests later.

http://codereview.appspot.com/1987047/diff/18001/java/gadgets/src/main/java/o...
File
java/gadgets/src/main/java/org/apache/shindig/gadgets/servlet/ProxyHandler.java
(right):

http://codereview.appspot.com/1987047/diff/18001/java/gadgets/src/main/java/o...
java/gadgets/src/main/java/org/apache/shindig/gadgets/servlet/ProxyHandler.java:28:
import org.apache.shindig.gadgets.LockedDomainService;
Not required.

http://codereview.appspot.com/1987047/diff/18001/java/gadgets/src/main/java/o...
java/gadgets/src/main/java/org/apache/shindig/gadgets/servlet/ProxyHandler.java:41:
import java.util.logging.Logger;
Not required.

http://codereview.appspot.com/1987047/diff/18001/java/gadgets/src/main/java/o...
java/gadgets/src/main/java/org/apache/shindig/gadgets/servlet/ProxyHandler.java:84:

Remove empty line.

http://codereview.appspot.com/1987047/diff/18001/java/gadgets/src/main/java/o...
java/gadgets/src/main/java/org/apache/shindig/gadgets/servlet/ProxyHandler.java:88:
private void copyHeaders(HttpRequest source, HttpRequest target) throws
GadgetException {
Maybe move to UriUtils or ServerUtils which has copyResponseHeaders method as
well.

http://codereview.appspot.com/1987047/diff/18001/java/gadgets/src/main/java/o...
java/gadgets/src/main/java/org/apache/shindig/gadgets/servlet/ProxyHandler.java:110:
if (rcr == null) {
This rcr null check looks redundant. NPE will be thrown in buildHttpRequest
before we reach here if rcr is null.

http://codereview.appspot.com/1987047/diff/18001/java/gadgets/src/main/java/o...
File
java/gadgets/src/main/java/org/apache/shindig/gadgets/servlet/ProxyServlet.java
(left):

http://codereview.appspot.com/1987047/diff/18001/java/gadgets/src/main/java/o...
java/gadgets/src/main/java/org/apache/shindig/gadgets/servlet/ProxyServlet.java:63:
public void setLockedDomainService(LockedDomainService lockedDomainService) {
Why this reordering?

[gagan.goku, 2010-10-01 06:14:18]

http://codereview.appspot.com/1987047/diff/18001/java/gadgets/src/main/java/o...
File
java/gadgets/src/main/java/org/apache/shindig/gadgets/servlet/ProxyHandler.java
(right):

http://codereview.appspot.com/1987047/diff/18001/java/gadgets/src/main/java/o...
java/gadgets/src/main/java/org/apache/shindig/gadgets/servlet/ProxyHandler.java:28:
import org.apache.shindig.gadgets.LockedDomainService;
On 2010/09/30 20:46:11, Kuntal Loya wrote:
> Not required.

Done.

http://codereview.appspot.com/1987047/diff/18001/java/gadgets/src/main/java/o...
java/gadgets/src/main/java/org/apache/shindig/gadgets/servlet/ProxyHandler.java:41:
import java.util.logging.Logger;
On 2010/09/30 20:46:11, Kuntal Loya wrote:
> Not required.

Done.

http://codereview.appspot.com/1987047/diff/18001/java/gadgets/src/main/java/o...
java/gadgets/src/main/java/org/apache/shindig/gadgets/servlet/ProxyHandler.java:84:

On 2010/09/30 20:46:11, Kuntal Loya wrote:
> Remove empty line.

Done.

http://codereview.appspot.com/1987047/diff/18001/java/gadgets/src/main/java/o...
java/gadgets/src/main/java/org/apache/shindig/gadgets/servlet/ProxyHandler.java:88:
private void copyHeaders(HttpRequest source, HttpRequest target) throws
GadgetException {
On 2010/09/30 20:46:11, Kuntal Loya wrote:
> Maybe move to UriUtils or ServerUtils which has copyResponseHeaders method as
> well.

This method actually uses functions defined in UriUtils and is specific to
current proxy use case.
I think its better to keep it here.

http://codereview.appspot.com/1987047/diff/18001/java/gadgets/src/main/java/o...
java/gadgets/src/main/java/org/apache/shindig/gadgets/servlet/ProxyHandler.java:110:
if (rcr == null) {
On 2010/09/30 20:46:11, Kuntal Loya wrote:
> This rcr null check looks redundant. NPE will be thrown in buildHttpRequest
> before we reach here if rcr is null.

Done.

http://codereview.appspot.com/1987047/diff/18001/java/gadgets/src/main/java/o...
File
java/gadgets/src/main/java/org/apache/shindig/gadgets/servlet/ProxyServlet.java
(left):

http://codereview.appspot.com/1987047/diff/18001/java/gadgets/src/main/java/o...
java/gadgets/src/main/java/org/apache/shindig/gadgets/servlet/ProxyServlet.java:63:
public void setLockedDomainService(LockedDomainService lockedDomainService) {
On 2010/09/30 20:46:11, Kuntal Loya wrote:
> Why this reordering?

reverted

[Kuntal Loya, 2010-10-01 08:56:28]

http://codereview.appspot.com/1987047/diff/25001/java/gadgets/src/main/java/o...
File
java/gadgets/src/main/java/org/apache/shindig/gadgets/servlet/ProxyServlet.java
(right):

http://codereview.appspot.com/1987047/diff/25001/java/gadgets/src/main/java/o...
java/gadgets/src/main/java/org/apache/shindig/gadgets/servlet/ProxyServlet.java:89:
+ host;
Keep the same indentation as previous line.

http://codereview.appspot.com/1987047/diff/25001/java/gadgets/src/test/java/o...
File
java/gadgets/src/test/java/org/apache/shindig/gadgets/servlet/ProxyHandlerTest.java
(right):

http://codereview.appspot.com/1987047/diff/25001/java/gadgets/src/test/java/o...
java/gadgets/src/test/java/org/apache/shindig/gadgets/servlet/ProxyHandlerTest.java:95:
refresh, false, noCache, ContainerConfig.DEFAULT_CONTAINER, url,
Uri.parse(url));
Why is url the gadget?

http://codereview.appspot.com/1987047/diff/25001/java/gadgets/src/test/java/o...
java/gadgets/src/test/java/org/apache/shindig/gadgets/servlet/ProxyHandlerTest.java:151:
assertEquals(recorder.getHeader("Content-Type"), contentType);
Can we verify if a header in proxyUriHttpRequest also exists in req?

http://codereview.appspot.com/1987047/diff/25001/java/gadgets/src/test/java/o...
java/gadgets/src/test/java/org/apache/shindig/gadgets/servlet/ProxyHandlerTest.java:422:
expect(pipeline.execute((HttpRequest) EasyMock.anyObject())).andReturn(resp);
Why would this not be req?

http://codereview.appspot.com/1987047/diff/25001/java/gadgets/src/test/java/o...
File
java/gadgets/src/test/java/org/apache/shindig/gadgets/servlet/ProxyServletTest.java
(right):

http://codereview.appspot.com/1987047/diff/25001/java/gadgets/src/test/java/o...
java/gadgets/src/test/java/org/apache/shindig/gadgets/servlet/ProxyServletTest.java:55:
private final ProxyUriManager.ProxyUri proxyUri =
mock(ProxyUriManager.ProxyUri.class);
have a HttpRequest proxyHttpRequest as well and use it for proxyHandler.fetch

http://codereview.appspot.com/1987047/diff/25001/java/gadgets/src/test/java/o...
java/gadgets/src/test/java/org/apache/shindig/gadgets/servlet/ProxyServletTest.java:82:
expect(request.getHeaders("Host")).andReturn(makeEnumeration("example.org"));
why would request.getHeader("Host") return uri.getAuthority() while
request.getHeaders("Host") return "example.org".

[gagan.goku, 2010-10-04 10:11:08]

http://codereview.appspot.com/1987047/diff/25001/java/gadgets/src/main/java/o...
File
java/gadgets/src/main/java/org/apache/shindig/gadgets/servlet/ProxyServlet.java
(right):

http://codereview.appspot.com/1987047/diff/25001/java/gadgets/src/main/java/o...
java/gadgets/src/main/java/org/apache/shindig/gadgets/servlet/ProxyServlet.java:89:
+ host;
On 2010/10/01 08:56:28, Kuntal Loya wrote:
> Keep the same indentation as previous line.

Done.

http://codereview.appspot.com/1987047/diff/25001/java/gadgets/src/test/java/o...
File
java/gadgets/src/test/java/org/apache/shindig/gadgets/servlet/ProxyHandlerTest.java
(right):

http://codereview.appspot.com/1987047/diff/25001/java/gadgets/src/test/java/o...
java/gadgets/src/test/java/org/apache/shindig/gadgets/servlet/ProxyHandlerTest.java:95:
refresh, false, noCache, ContainerConfig.DEFAULT_CONTAINER, url,
Uri.parse(url));
On 2010/10/01 08:56:28, Kuntal Loya wrote:
> Why is url the gadget?

passing the gadget url as null throws NPE when converting proxyUri to Uri (or
doing toString) because the gadget url parameter of query params is set to null.
Since these tests set the gadget url as null and i had to set it to some non
null value, i though setting gadget url as the url itself would be okay.
The tests are not dependent on the gadget url anyways

http://codereview.appspot.com/1987047/diff/25001/java/gadgets/src/test/java/o...
java/gadgets/src/test/java/org/apache/shindig/gadgets/servlet/ProxyHandlerTest.java:151:
assertEquals(recorder.getHeader("Content-Type"), contentType);
On 2010/10/01 08:56:28, Kuntal Loya wrote:
> Can we verify if a header in proxyUriHttpRequest also exists in req?

Done.

http://codereview.appspot.com/1987047/diff/25001/java/gadgets/src/test/java/o...
java/gadgets/src/test/java/org/apache/shindig/gadgets/servlet/ProxyHandlerTest.java:422:
expect(pipeline.execute((HttpRequest) EasyMock.anyObject())).andReturn(resp);
On 2010/10/01 08:56:28, Kuntal Loya wrote:
> Why would this not be req?

Done.

http://codereview.appspot.com/1987047/diff/25001/java/gadgets/src/test/java/o...
java/gadgets/src/test/java/org/apache/shindig/gadgets/servlet/ProxyHandlerTest.java:422:
expect(pipeline.execute((HttpRequest) EasyMock.anyObject())).andReturn(resp);
On 2010/10/01 08:56:28, Kuntal Loya wrote:
> Why would this not be req?

Done.

http://codereview.appspot.com/1987047/diff/25001/java/gadgets/src/test/java/o...
File
java/gadgets/src/test/java/org/apache/shindig/gadgets/servlet/ProxyServletTest.java
(right):

http://codereview.appspot.com/1987047/diff/25001/java/gadgets/src/test/java/o...
java/gadgets/src/test/java/org/apache/shindig/gadgets/servlet/ProxyServletTest.java:55:
private final ProxyUriManager.ProxyUri proxyUri =
mock(ProxyUriManager.ProxyUri.class);
On 2010/10/01 08:56:28, Kuntal Loya wrote:
> have a HttpRequest proxyHttpRequest as well and use it for proxyHandler.fetch

Not needed anymore. Added a makeHttpRequestForProxyUrl method which does the
same thing.

http://codereview.appspot.com/1987047/diff/25001/java/gadgets/src/test/java/o...
java/gadgets/src/test/java/org/apache/shindig/gadgets/servlet/ProxyServletTest.java:82:
expect(request.getHeaders("Host")).andReturn(makeEnumeration("example.org"));
On 2010/10/01 08:56:28, Kuntal Loya wrote:
> why would request.getHeader("Host") return uri.getAuthority() while
> request.getHeaders("Host") return "example.org".

Nice catch. it was not getting tested anywhere, but made it consistent now.

[Kuntal Loya, 2010-10-04 10:35:36]

lgtm.

http://codereview.appspot.com/1987047/diff/31001/java/gadgets/src/main/java/o...
File
java/gadgets/src/main/java/org/apache/shindig/gadgets/servlet/ProxyServlet.java
(right):

http://codereview.appspot.com/1987047/diff/31001/java/gadgets/src/main/java/o...
java/gadgets/src/main/java/org/apache/shindig/gadgets/servlet/ProxyServlet.java:88:
(resourceUri != null ? resourceUri.toString() : "n/a")
style comment - keep both the breaks either after or before the +.

[gagan.goku, 2010-10-04 11:45:48]

http://codereview.appspot.com/1987047/diff/31001/java/gadgets/src/main/java/o...
File
java/gadgets/src/main/java/org/apache/shindig/gadgets/servlet/ProxyServlet.java
(right):

http://codereview.appspot.com/1987047/diff/31001/java/gadgets/src/main/java/o...
java/gadgets/src/main/java/org/apache/shindig/gadgets/servlet/ProxyServlet.java:88:
(resourceUri != null ? resourceUri.toString() : "n/a")
On 2010/10/04 10:35:36, Kuntal Loya wrote:
> style comment - keep both the breaks either after or before the +.

Done.

[zhoresh, 2010-10-04 17:31:26]

I would argue that this break encapsulation.
The idea is that ProxyHandler can be used by other services not just
ProxyServlet (i.e. GadgetHandler).

So the ProxyHandler should handle any User-Agent value, and if needed to pass
more info down, then maybe ProxyUri is the place to change.