Supporting http proxy in basic http fetcher

Adding support in BasicHttpFetcher to fetch the requested url from a proxy server rather that directly from the internet.

[zhoresh, 2010-06-10 01:31:45]

http://codereview.appspot.com/1635042/diff/2001/3003
File
java/gadgets/src/main/java/org/apache/shindig/gadgets/http/BasicHttpFetcher.java
(right):

http://codereview.appspot.com/1635042/diff/2001/3003#newcode101
java/gadgets/src/main/java/org/apache/shindig/gadgets/http/BasicHttpFetcher.java:101:
protected final String REQUEST_URI_HEADER_NAME = "Google-RequestUri";
I think the naming convention is to start with X-
so maybe name it "X-Shindig-RequestUri"

http://codereview.appspot.com/1635042/diff/2001/3003#newcode213
java/gadgets/src/main/java/org/apache/shindig/gadgets/http/BasicHttpFetcher.java:213:
// Add hook for overridden host header in order to support proxying.
Please add more details to what you try to do

http://codereview.appspot.com/1635042/diff/2001/3003#newcode219
java/gadgets/src/main/java/org/apache/shindig/gadgets/http/BasicHttpFetcher.java:219:
String requestUri = request.getHeaders(REQUEST_URI_HEADER_NAME)[0]
I think it can be empty list.

http://codereview.appspot.com/1635042/diff/2001/3003#newcode222
java/gadgets/src/main/java/org/apache/shindig/gadgets/http/BasicHttpFetcher.java:222:
request.removeHeader(request.getHeaders("Host")[0]);
Don't you want removeHeader("Host")  ?

http://codereview.appspot.com/1635042/diff/2001/3003#newcode310
java/gadgets/src/main/java/org/apache/shindig/gadgets/http/BasicHttpFetcher.java:310:
int port = 80; // default port
-1 will tell HttpHost to use default (which is 80) and not add port to url. So
new code should do the same.

http://codereview.appspot.com/1635042/diff/2001/3003#newcode507
java/gadgets/src/main/java/org/apache/shindig/gadgets/http/BasicHttpFetcher.java:507:
host = new HttpHost(splits[0], Integer.parseInt(splits[1]));
Handle NumberFormatException (even just to add message to exception that
indicate bad configuration)
And probably support no port (default to -1)

[Paul Lindner, 2010-06-10 03:51:25]

I believe the functionality you are trying to implement is already present in
the RoutePlanner feature  in httpclient.

Just set ConnRoutePNames.DEFAULT_PROXY with the proxy you want to use and be
done with it.

[gagan.goku, 2010-06-13 08:40:21]

http://codereview.appspot.com/1635042/diff/2001/3003
File
java/gadgets/src/main/java/org/apache/shindig/gadgets/http/BasicHttpFetcher.java
(right):

http://codereview.appspot.com/1635042/diff/2001/3003#newcode101
java/gadgets/src/main/java/org/apache/shindig/gadgets/http/BasicHttpFetcher.java:101:
protected final String REQUEST_URI_HEADER_NAME = "Google-RequestUri";
On 2010/06/10 01:31:45, zhoresh wrote:
> I think the naming convention is to start with X-
> so maybe name it "X-Shindig-RequestUri"
> 

removed.

http://codereview.appspot.com/1635042/diff/2001/3003#newcode213
java/gadgets/src/main/java/org/apache/shindig/gadgets/http/BasicHttpFetcher.java:213:
// Add hook for overridden host header in order to support proxying.
On 2010/06/10 01:31:45, zhoresh wrote:
> Please add more details to what you try to do

reverted

http://codereview.appspot.com/1635042/diff/2001/3003#newcode219
java/gadgets/src/main/java/org/apache/shindig/gadgets/http/BasicHttpFetcher.java:219:
String requestUri = request.getHeaders(REQUEST_URI_HEADER_NAME)[0]
On 2010/06/10 01:31:45, zhoresh wrote:
> I think it can be empty list.

reverted

http://codereview.appspot.com/1635042/diff/2001/3003#newcode222
java/gadgets/src/main/java/org/apache/shindig/gadgets/http/BasicHttpFetcher.java:222:
request.removeHeader(request.getHeaders("Host")[0]);
On 2010/06/10 01:31:45, zhoresh wrote:
> Don't you want removeHeader("Host")  ?

reverted

http://codereview.appspot.com/1635042/diff/2001/3003#newcode310
java/gadgets/src/main/java/org/apache/shindig/gadgets/http/BasicHttpFetcher.java:310:
int port = 80; // default port
On 2010/06/10 01:31:45, zhoresh wrote:
> -1 will tell HttpHost to use default (which is 80) and not add port to url. So
> new code should do the same.

reverted

http://codereview.appspot.com/1635042/diff/2001/3003#newcode507
java/gadgets/src/main/java/org/apache/shindig/gadgets/http/BasicHttpFetcher.java:507:
host = new HttpHost(splits[0], Integer.parseInt(splits[1]));
On 2010/06/10 01:31:45, zhoresh wrote:
> Handle NumberFormatException (even just to add message to exception that
> indicate bad configuration)
> And probably support no port (default to -1)

reverted

[gagan.goku, 2010-06-13 08:42:07]

On 2010/06/10 03:51:25, Paul Lindner wrote:
> I believe the functionality you are trying to implement is already present in
> the RoutePlanner feature  in httpclient.
> 
> Just set ConnRoutePNames.DEFAULT_PROXY with the proxy you want to use and be
> done with it.

I tried adding this to line 371:
httpMethod.getParams().setParameter(ConnRoutePNames.DEFAULT_PROXY,
basicHttpFetcherProxy)
but this did not work

Could you show me where to add this 1 line so i can remove the
getSelectorForProxy method.

[Paul Lindner, 2010-06-15 06:52:20]

I think you can do it like this:

ConnRouteParams.setDefaultProxy(params, new HttpHost(host,port, scheme));

(have not tested it, but looks like the proper way to do this.)

If you set this value I believe that you should not instantiate
the ProxySelectorRoutePlanner later on in the module.



On Sun, Jun 13, 2010 at 1:42 AM, <gagan.goku@gmail.com> wrote:

> On 2010/06/10 03:51:25, Paul Lindner wrote:
>
>> I believe the functionality you are trying to implement is already
>>
> present in
>
>> the RoutePlanner feature  in httpclient.
>>
>
>  Just set ConnRoutePNames.DEFAULT_PROXY with the proxy you want to use
>>
> and be
>
>> done with it.
>>
>
> I tried adding this to line 371:
> httpMethod.getParams().setParameter(ConnRoutePNames.DEFAULT_PROXY,
> basicHttpFetcherProxy)
> but this did not work
>
> Could you show me where to add this 1 line so i can remove the
> getSelectorForProxy method.
>
>
> http://codereview.appspot.com/1635042/show
>

[gagan.goku, 2010-06-15 23:17:13]

On 2010/06/15 06:52:20, Paul Lindner wrote:
> I think you can do it like this:
> 
> ConnRouteParams.setDefaultProxy(params, new HttpHost(host,port, scheme));
>
This worked perfectly.
Thanks

> (have not tested it, but looks like the proper way to do this.)
> 
> If you set this value I believe that you should not instantiate
> the ProxySelectorRoutePlanner later on in the module.
> 
> 
> 
> On Sun, Jun 13, 2010 at 1:42 AM, <mailto:gagan.goku@gmail.com> wrote:
> 
> > On 2010/06/10 03:51:25, Paul Lindner wrote:
> >
> >> I believe the functionality you are trying to implement is already
> >>
> > present in
> >
> >> the RoutePlanner feature  in httpclient.
> >>
> >
> >  Just set ConnRoutePNames.DEFAULT_PROXY with the proxy you want to use
> >>
> > and be
> >
> >> done with it.
> >>
> >
> > I tried adding this to line 371:
> > httpMethod.getParams().setParameter(ConnRoutePNames.DEFAULT_PROXY,
> > basicHttpFetcherProxy)
> > but this did not work
> >
> > Could you show me where to add this 1 line so i can remove the
> > getSelectorForProxy method.
> >
> >
> > http://codereview.appspot.com/1635042/show
> >
>

[gagan.goku, 2010-06-21 05:08:16]

On 2010/06/15 23:17:13, gagan.goku wrote:
> On 2010/06/15 06:52:20, Paul Lindner wrote:
> > I think you can do it like this:
> > 
> > ConnRouteParams.setDefaultProxy(params, new HttpHost(host,port, scheme));
> >
> This worked perfectly.
> Thanks
> 
> > (have not tested it, but looks like the proper way to do this.)
> > 
> > If you set this value I believe that you should not instantiate
> > the ProxySelectorRoutePlanner later on in the module.
> > 
> > 
> > 
> > On Sun, Jun 13, 2010 at 1:42 AM, <mailto:gagan.goku@gmail.com> wrote:
> > 
> > > On 2010/06/10 03:51:25, Paul Lindner wrote:
> > >
> > >> I believe the functionality you are trying to implement is already
> > >>
> > > present in
> > >
> > >> the RoutePlanner feature  in httpclient.
> > >>
> > >
> > >  Just set ConnRoutePNames.DEFAULT_PROXY with the proxy you want to use
> > >>
> > > and be
> > >
> > >> done with it.
> > >>
> > >
> > > I tried adding this to line 371:
> > > httpMethod.getParams().setParameter(ConnRoutePNames.DEFAULT_PROXY,
> > > basicHttpFetcherProxy)
> > > but this did not work
> > >
> > > Could you show me where to add this 1 line so i can remove the
> > > getSelectorForProxy method.
> > >
> > >
> > > http://codereview.appspot.com/1635042/show
> > >
> >

Please review.

[Paul Lindner, 2010-06-21 18:18:58]

can you please retain the previous method of setting the proxy from environment
variables?  It can be used when there is no proxy set via guice.

I know there are some people using that feature and removing this would break
them.  Otherwise looks good.

Thanks!

[gagan.goku, 2010-06-21 20:01:27]

On 2010/06/21 18:18:58, Paul Lindner wrote:
> can you please retain the previous method of setting the proxy from
environment
> variables?  It can be used when there is no proxy set via guice.
> 
> I know there are some people using that feature and removing this would break
> them.  Otherwise looks good.
> 
> Thanks!

Hi Paul

Im not sure i understood what you meant by previous method of setting proxy from
environment variables.
Do you want me to keep the following block in case proxy is not specified using
shindig.properties? 

// Use Java's built-in proxy logic	
if (StringUtils.isEmpty(basicHttpFetcherProxy)) {
  ProxySelectorRoutePlanner routePlanner = new ProxySelectorRoutePlanner(	
      client.getConnectionManager().getSchemeRegistry(),	
      ProxySelector.getDefault());	
  client.setRoutePlanner(routePlanner);
}

[Paul Lindner, 2010-06-21 20:04:46]

yes please.

On Mon, Jun 21, 2010 at 1:01 PM, <gagan.goku@gmail.com> wrote:

> On 2010/06/21 18:18:58, Paul Lindner wrote:
>
>> can you please retain the previous method of setting the proxy from
>>
> environment
>
>> variables?  It can be used when there is no proxy set via guice.
>>
>
>  I know there are some people using that feature and removing this
>>
> would break
>
>> them.  Otherwise looks good.
>>
>
>  Thanks!
>>
>
> Hi Paul
>
> Im not sure i understood what you meant by previous method of setting
> proxy from environment variables.
> Do you want me to keep the following block in case proxy is not
> specified using shindig.properties?
>
> // Use Java's built-in proxy logic
> if (StringUtils.isEmpty(basicHttpFetcherProxy)) {
>  ProxySelectorRoutePlanner routePlanner = new
> ProxySelectorRoutePlanner(
>      client.getConnectionManager().getSchemeRegistry(),
>      ProxySelector.getDefault());
>  client.setRoutePlanner(routePlanner);
> }
>
>
> http://codereview.appspot.com/1635042/show
>

[gagan.goku, 2010-06-21 20:12:47]

On 2010/06/21 20:04:46, Paul Lindner wrote:
> yes please.
> 
Done.

> On Mon, Jun 21, 2010 at 1:01 PM, <mailto:gagan.goku@gmail.com> wrote:
> 
> > On 2010/06/21 18:18:58, Paul Lindner wrote:
> >
> >> can you please retain the previous method of setting the proxy from
> >>
> > environment
> >
> >> variables?  It can be used when there is no proxy set via guice.
> >>
> >
> >  I know there are some people using that feature and removing this
> >>
> > would break
> >
> >> them.  Otherwise looks good.
> >>
> >
> >  Thanks!
> >>
> >
> > Hi Paul
> >
> > Im not sure i understood what you meant by previous method of setting
> > proxy from environment variables.
> > Do you want me to keep the following block in case proxy is not
> > specified using shindig.properties?
> >
> > // Use Java's built-in proxy logic
> > if (StringUtils.isEmpty(basicHttpFetcherProxy)) {
> >  ProxySelectorRoutePlanner routePlanner = new
> > ProxySelectorRoutePlanner(
> >      client.getConnectionManager().getSchemeRegistry(),
> >      ProxySelector.getDefault());
> >  client.setRoutePlanner(routePlanner);
> > }
> >
> >
> > http://codereview.appspot.com/1635042/show
> >
>

[Paul Lindner, 2010-06-21 23:07:44]

thanks! committed.

[gagan.goku, 2010-06-22 00:51:28]

On 2010/06/21 23:07:44, Paul Lindner wrote:
> thanks! committed.

thank you