[SHINDIG-1266] - Use correct HTTP status codes in GadgetRenderingServlet

Shindig's GadgetRenderingServlet should respond with an appropriate 4xx/5xx HTTP status code when there are errors. It currently responds with 200 OK when something goes wrong.

JIRA Issue: https://issues.apache.org/jira/browse/SHINDIG-1266

[chirag, 2010-01-20 03:25:13]

Added shindig.remailer

[johnfargo, 2010-01-20 21:10:20]

http://codereview.appspot.com/186233/diff/1/7
File
java/gadgets/src/main/java/org/apache/shindig/gadgets/render/ProxyRenderer.java
(right):

http://codereview.appspot.com/186233/diff/1/7#newcode109
java/gadgets/src/main/java/org/apache/shindig/gadgets/render/ProxyRenderer.java:109:
throw new RenderingException("Unable to reach remote host. HTTP status " + code,
code);
I don't think we should pass the code through verbatim. It's not an internal
server error from the perspective of Shindig Gadget Server to find that a target
server of a proxy render emits a 500. I vote 404/SC_NOT_FOUND here.

http://codereview.appspot.com/186233/diff/1/8
File
java/gadgets/src/main/java/org/apache/shindig/gadgets/render/RenderingException.java
(right):

http://codereview.appspot.com/186233/diff/1/8#newcode28
java/gadgets/src/main/java/org/apache/shindig/gadgets/render/RenderingException.java:28:
public int httpStatusCode;
should be final

http://codereview.appspot.com/186233/diff/1/8#newcode36
java/gadgets/src/main/java/org/apache/shindig/gadgets/render/RenderingException.java:36:
this.httpStatusCode = httpStatusCode;
may as well add a proper getter for this.

http://codereview.appspot.com/186233/diff/1/8#newcode40
java/gadgets/src/main/java/org/apache/shindig/gadgets/render/RenderingException.java:40:
super(message, t);
no default specified here (and rightly so, IMO httpStatusCode needs to be added
to this ctor)

[chirag, 2010-01-21 01:26:53]

Fixed issues raised by John. Thanks!

http://codereview.appspot.com/186233/diff/1/7
File
java/gadgets/src/main/java/org/apache/shindig/gadgets/render/ProxyRenderer.java
(right):

http://codereview.appspot.com/186233/diff/1/7#newcode109
java/gadgets/src/main/java/org/apache/shindig/gadgets/render/ProxyRenderer.java:109:
throw new RenderingException("Unable to reach remote host. HTTP status " + code,
code);
On 2010/01/20 21:10:20, johnfargo wrote:
> I don't think we should pass the code through verbatim. It's not an internal
> server error from the perspective of Shindig Gadget Server to find that a
target
> server of a proxy render emits a 500. I vote 404/SC_NOT_FOUND here.

Done.

http://codereview.appspot.com/186233/diff/1/8
File
java/gadgets/src/main/java/org/apache/shindig/gadgets/render/RenderingException.java
(right):

http://codereview.appspot.com/186233/diff/1/8#newcode28
java/gadgets/src/main/java/org/apache/shindig/gadgets/render/RenderingException.java:28:
public int httpStatusCode;
On 2010/01/20 21:10:20, johnfargo wrote:
> should be final

Done.

http://codereview.appspot.com/186233/diff/1/8#newcode36
java/gadgets/src/main/java/org/apache/shindig/gadgets/render/RenderingException.java:36:
this.httpStatusCode = httpStatusCode;
On 2010/01/20 21:10:20, johnfargo wrote:
> may as well add a proper getter for this.

Done.

http://codereview.appspot.com/186233/diff/1/8#newcode40
java/gadgets/src/main/java/org/apache/shindig/gadgets/render/RenderingException.java:40:
super(message, t);
On 2010/01/20 21:10:20, johnfargo wrote:
> no default specified here (and rightly so, IMO httpStatusCode needs to be
added
> to this ctor)

Done.

[chirag, 2010-01-21 01:28:05]

New patch fixing issues raised by John.

[johnfargo, 2010-01-21 04:20:47]

Looks great. I had 2 comments but cleaned it up myself and committed. Thanks!

http://codereview.appspot.com/186233/diff/18/22
File
java/gadgets/src/main/java/org/apache/shindig/gadgets/render/RenderingResults.java
(right):

http://codereview.appspot.com/186233/diff/18/22#newcode38
java/gadgets/src/main/java/org/apache/shindig/gadgets/render/RenderingResults.java:38:
private RenderingResults(Status status, String content, String errorMessage, int
httpStatusCode, Uri redirect) {
>100char

http://codereview.appspot.com/186233/diff/18/22#newcode57
java/gadgets/src/main/java/org/apache/shindig/gadgets/render/RenderingResults.java:57:
return new RenderingResults(Status.MUST_REDIRECT, null, null,
HttpServletResponse.SC_FOUND, redirect);
same