Returning same status code as that of the fetched resource

Returning the error http status code as that of the fetched resource. Since we want to proxy the resource through, we want the user to see the exact code returned by the server.

[Kuntal Loya, 2010-07-12 08:59:11]

http://codereview.appspot.com/1811042/diff/1/2
File servlet/AccelHandler.java (right):

http://codereview.appspot.com/1811042/diff/1/2#newcode217
servlet/AccelHandler.java:217: response.sendError(results.getHttpStatusCode(),
ERROR_FETCHING_DATA);
Shouldn't we retain the error mesg in the results and pass it to the response?

[gagan.goku, 2010-07-12 09:27:19]

fixing tests

[gagan.goku, 2010-07-12 09:31:23]

On 2010/07/12 09:27:19, gagan.goku wrote:
> fixing tests

Comment addressed.

[anupama.dutta, 2010-07-13 14:09:44]

http://codereview.appspot.com/1811042/diff/6001/7001
File main/java/org/apache/shindig/gadgets/servlet/AccelHandler.java (right):

http://codereview.appspot.com/1811042/diff/6001/7001#newcode89
main/java/org/apache/shindig/gadgets/servlet/AccelHandler.java:89: // In case of
errors where we want to short circuit the rewriting and
Update this comment?

http://codereview.appspot.com/1811042/diff/6001/7001#newcode105
main/java/org/apache/shindig/gadgets/servlet/AccelHandler.java:105:
sendResponseToUser(req, results, response);
How about just sendResponse?

http://codereview.appspot.com/1811042/diff/6001/7001#newcode229
main/java/org/apache/shindig/gadgets/servlet/AccelHandler.java:229: return new
HttpResponseBuilder()
When can results be null? Earlier, we used to return SC_BAD_REQUEST, now we
return SC_NOT_FOUND - is this intentional?

[gagan.goku, 2010-07-13 14:41:12]

http://codereview.appspot.com/1811042/diff/6001/7001
File main/java/org/apache/shindig/gadgets/servlet/AccelHandler.java (right):

http://codereview.appspot.com/1811042/diff/6001/7001#newcode89
main/java/org/apache/shindig/gadgets/servlet/AccelHandler.java:89: // In case of
errors where we want to short circuit the rewriting and
On 2010/07/13 14:09:44, anupama.dutta wrote:
> Update this comment?

Done.

http://codereview.appspot.com/1811042/diff/6001/7001#newcode105
main/java/org/apache/shindig/gadgets/servlet/AccelHandler.java:105:
sendResponseToUser(req, results, response);
On 2010/07/13 14:09:44, anupama.dutta wrote:
> How about just sendResponse?

Done.

http://codereview.appspot.com/1811042/diff/6001/7001#newcode229
main/java/org/apache/shindig/gadgets/servlet/AccelHandler.java:229: return new
HttpResponseBuilder()
On 2010/07/13 14:09:44, anupama.dutta wrote:
> When can results be null? Earlier, we used to return SC_BAD_REQUEST, now we
> return SC_NOT_FOUND - is this intentional?

Ideally results should never be null. It might be null in case of some bug in
caching mechanism if we add the key but say the request times out and we never
remove it from the cache.
But still in those cases as well, the request sent by user was probably well
formed, so we should not throw a bad request.

I think we should throw a 404 not found. What do you think ?

[gagan.goku, 2010-07-13 16:12:57]

addressing comments

[Kuntal Loya, 2010-07-13 18:47:58]

http://codereview.appspot.com/1811042/diff/16001/17001
File main/java/org/apache/shindig/gadgets/servlet/AccelHandler.java (right):

http://codereview.appspot.com/1811042/diff/16001/17001#newcode88
main/java/org/apache/shindig/gadgets/servlet/AccelHandler.java:88: if
(errorResponse != null) {
if (errorResponse == null) {
  // Rewrite the content when there is no error.
  try...catch
} else {
  // In case of an error, send it as it is.
  results = errorResponse;
}
...

[gagan.goku, 2010-07-13 18:55:37]

addressing comments

[gagan.goku, 2010-07-13 18:55:55]

http://codereview.appspot.com/1811042/diff/16001/17001
File main/java/org/apache/shindig/gadgets/servlet/AccelHandler.java (right):

http://codereview.appspot.com/1811042/diff/16001/17001#newcode88
main/java/org/apache/shindig/gadgets/servlet/AccelHandler.java:88: if
(errorResponse != null) {
On 2010/07/13 18:47:58, kuntal.loya wrote:
> if (errorResponse == null) {
>   // Rewrite the content when there is no error.
>   try...catch
> } else {
>   // In case of an error, send it as it is.
>   results = errorResponse;
> }
> ...

Done.

[johnfargo, 2010-07-14 20:44:59]

http://codereview.appspot.com/1811042/diff/23001/24002
File main/java/org/apache/shindig/gadgets/uri/UriUtils.java (left):

http://codereview.appspot.com/1811042/diff/23001/24002#oldcode129
main/java/org/apache/shindig/gadgets/uri/UriUtils.java:129:
resp.sendError(HttpResponse.SC_BAD_GATEWAY);
this remapping is critical for many ppl's production monitoring. if you want to
send 500s for accel straight through that's fine but the change should be
limited to accel only.

http://codereview.appspot.com/1811042/diff/23001/24003
File test/java/org/apache/shindig/gadgets/servlet/HtmlAccelServletTest.java
(right):

http://codereview.appspot.com/1811042/diff/23001/24003#newcode197
test/java/org/apache/shindig/gadgets/servlet/HtmlAccelServletTest.java:197:
assertEquals(5001, recorder.getHttpStatusCode());
you should keep the test w/ status 500

[gagan.goku, 2010-07-15 05:24:32]

addressing comments

[gagan.goku, 2010-07-15 05:27:09]

reverting ProxyingContentRewriter

[gagan.goku, 2010-07-15 05:29:40]

On 2010/07/15 05:27:09, gagan.goku wrote:
> reverting ProxyingContentRewriter

Comments addressed. And to emphasize how important http status codes are,
Rietveld is throwing "500 Server Error" when i try to Publish+Mail comments.

[gagan.goku, 2010-07-15 16:17:58]

On 2010/07/15 05:29:40, gagan.goku wrote:
> On 2010/07/15 05:27:09, gagan.goku wrote:
> > reverting ProxyingContentRewriter
> 
> Comments addressed. And to emphasize how important http status codes are,
> Rietveld is throwing "500 Server Error" when i try to Publish+Mail comments.

On 2010/07/14 20:45:02, johnfargo wrote:
> this remapping is critical for many ppl's production monitoring. if you want
to
> send 500s for accel straight through that's fine but the change should be
> limited to accel only.
Done.

On 2010/07/14 20:45:02, johnfargo wrote:
> you should keep the test w/ status 500
Done.

[anupama.dutta, 2010-07-26 12:16:10]

LGTM.

http://codereview.appspot.com/1811042/diff/37001/38001
File common/conf/shindig.properties (right):

http://codereview.appspot.com/1811042/diff/37001/38001#newcode145
common/conf/shindig.properties:145: # Remap internal server errors received from
the server being proxied to
Rephrase as:
Remap "Internal server error"s received from the basicHttpFetcherProxy server to
"Bad Gateway error"s, so that it is clear to the user that the proxy server is
the one that threw the exception.

http://codereview.appspot.com/1811042/diff/37001/38002
File gadgets/src/main/java/org/apache/shindig/gadgets/servlet/AccelHandler.java
(right):

http://codereview.appspot.com/1811042/diff/37001/38002#newcode107
gadgets/src/main/java/org/apache/shindig/gadgets/servlet/AccelHandler.java:107:
sendResponse(req, results, response);
Since we don't have multiple calls to sendResponse, you can remove the method
and just add back the code here as before.

[thi.thu.thuy.vu_sap.com, 2010-07-26 13:29:23]

0612452548

-----Original Message-----
From: anupama.dutta@gmail.com [mailto:anupama.dutta@gmail.com] 
Sent: lundi 26 juillet 2010 14:16
To: gagan.goku@gmail.com; johnfargo@gmail.com; zhoresh@gmail.com;
shindig.remailer@gmail.com; dev-remailer@shindig.apache.org;
cool-shindig-committers@googlegroups.com; kuntal.loya@gmail.com
Cc: reply@codereview.appspotmail.com
Subject: Re: Returning same status code as fetched (issue1811042)

LGTM.


http://codereview.appspot.com/1811042/diff/37001/38001
File common/conf/shindig.properties (right):

http://codereview.appspot.com/1811042/diff/37001/38001#newcode145
common/conf/shindig.properties:145: # Remap internal server errors
received from the server being proxied to
Rephrase as:
Remap "Internal server error"s received from the basicHttpFetcherProxy
server to "Bad Gateway error"s, so that it is clear to the user that the
proxy server is the one that threw the exception.

http://codereview.appspot.com/1811042/diff/37001/38002
File
gadgets/src/main/java/org/apache/shindig/gadgets/servlet/AccelHandler.java
(right):

http://codereview.appspot.com/1811042/diff/37001/38002#newcode107
gadgets/src/main/java/org/apache/shindig/gadgets/servlet/AccelHandler.java:107:
sendResponse(req, results, response);
Since we don't have multiple calls to sendResponse, you can remove the
method and just add back the code here as before.

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

[gagan.goku, 2010-07-26 14:38:48]

On 2010/07/26 12:16:10, anupama.dutta wrote:
> LGTM.
> 
> http://codereview.appspot.com/1811042/diff/37001/38001
> File common/conf/shindig.properties (right):
> 
> http://codereview.appspot.com/1811042/diff/37001/38001#newcode145
> common/conf/shindig.properties:145: # Remap internal server errors received
from
> the server being proxied to
> Rephrase as:
> Remap "Internal server error"s received from the basicHttpFetcherProxy server
to
> "Bad Gateway error"s, so that it is clear to the user that the proxy server is
> the one that threw the exception.
Done.

> 
> http://codereview.appspot.com/1811042/diff/37001/38002
> File
gadgets/src/main/java/org/apache/shindig/gadgets/servlet/AccelHandler.java
> (right):
> 
> http://codereview.appspot.com/1811042/diff/37001/38002#newcode107
>
gadgets/src/main/java/org/apache/shindig/gadgets/servlet/AccelHandler.java:107:
> sendResponse(req, results, response);
> Since we don't have multiple calls to sendResponse, you can remove the method
> and just add back the code here as before.
Done.

[johnfargo, 2010-07-26 21:03:44]

http://codereview.appspot.com/1811042/diff/37001/38004
File
gadgets/src/test/java/org/apache/shindig/gadgets/servlet/HtmlAccelServletTest.java
(right):

http://codereview.appspot.com/1811042/diff/37001/38004#newcode111
gadgets/src/test/java/org/apache/shindig/gadgets/servlet/HtmlAccelServletTest.java:111:
((FakeCaptureRewriter) rewriter).setContentToRewrite(REWRITE_CONTENT);
avoid FakeCaptureRewriter casting by just creating a typed FakeCaptureRewriter
in this class. I don't remember all the interactions, but worst case you could
just alias rewriter = fakeCaptureRewriterInstance; if need be.

[johnfargo, 2010-07-26 22:03:02]

LGTM, committing w/ one FYI to those reading.

http://codereview.appspot.com/1811042/diff/54001/55005
File java/gadgets/src/main/java/org/apache/shindig/gadgets/uri/UriUtils.java
(right):

http://codereview.appspot.com/1811042/diff/54001/55005#newcode147
java/gadgets/src/main/java/org/apache/shindig/gadgets/uri/UriUtils.java:147:
resp.setStatus(HttpResponse.SC_BAD_GATEWAY);
This is the only potentially controversial change left in this CL, but it LGTM.
It presumes that the calling context will supply error content that exists, if
any. This is true for all callers (Accel only for the moment).

[gagan.goku, 2010-07-27 05:48:17]

Thanks John.

On Tue, Jul 27, 2010 at 3:33 AM, <johnfargo@gmail.com> wrote:

> LGTM, committing w/ one FYI to those reading.
>
>
> http://codereview.appspot.com/1811042/diff/54001/55005
> File
> java/gadgets/src/main/java/org/apache/shindig/gadgets/uri/UriUtils.java
> (right):
>
> http://codereview.appspot.com/1811042/diff/54001/55005#newcode147
>
> java/gadgets/src/main/java/org/apache/shindig/gadgets/uri/UriUtils.java:147:
> resp.setStatus(HttpResponse.SC_BAD_GATEWAY);
> This is the only potentially controversial change left in this CL, but
> it LGTM. It presumes that the calling context will supply error content
> that exists, if any. This is true for all callers (Accel only for the
> moment).

I think it should be clear to the caller since we clearly express that we
are only copying the headers and status code, not the content.
And since the use case for this function is to copy HttpResponse (which is
the result of fetching a web page) into HttpServletResponse, the user
anyways has to copy over the data if any.


>
> http://codereview.appspot.com/1811042/show
>