You are viewing a plain text version of this content. The canonical link for it is here.
Posted to dev@shindig.apache.org by zh...@gmail.com on 2010/06/10 03:31:45 UTC

Supporting http proxy in basic http fetcher (issue1635042)

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)

http://codereview.appspot.com/1635042/show