svn commit: r1082941 - in /shindig/trunk/java/gadgets/src: main/java/org/apache/shindig/gadgets/servlet/AccelHandler.java test/java/org/apache/shindig/gadgets/servlet/HtmlAccelServletTest.java

[ga...@apache.org]

Author: gagan
Date: Fri Mar 18 15:03:29 2011
New Revision: 1082941

URL: http://svn.apache.org/viewvc?rev=1082941&view=rev
Log:
Patch by atulvasu | Issue 4240102: Serving the original response incase of RewriterException for Accel | http://codereview.appspot.com/4240102/

Modified:
    shindig/trunk/java/gadgets/src/main/java/org/apache/shindig/gadgets/servlet/AccelHandler.java
    shindig/trunk/java/gadgets/src/test/java/org/apache/shindig/gadgets/servlet/HtmlAccelServletTest.java

Modified: shindig/trunk/java/gadgets/src/main/java/org/apache/shindig/gadgets/servlet/AccelHandler.java
URL: http://svn.apache.org/viewvc/shindig/trunk/java/gadgets/src/main/java/org/apache/shindig/gadgets/servlet/AccelHandler.java?rev=1082941&r1=1082940&r2=1082941&view=diff
==============================================================================
--- shindig/trunk/java/gadgets/src/main/java/org/apache/shindig/gadgets/servlet/AccelHandler.java (original)
+++ shindig/trunk/java/gadgets/src/main/java/org/apache/shindig/gadgets/servlet/AccelHandler.java Fri Mar 18 15:03:29 2011
@@ -42,6 +42,8 @@ import org.apache.shindig.gadgets.uri.Ur
 
 import java.io.ByteArrayOutputStream;
 import java.io.IOException;
+import java.util.logging.Level;
+import java.util.logging.Logger;
 
 /**
  * Handles requests for accel servlet.
@@ -51,6 +53,8 @@ import java.io.IOException;
  */
 @Singleton
 public class AccelHandler {
+  private static final Logger logger = Logger.getLogger(
+      AccelHandler.class.getName());
   static final String ERROR_FETCHING_DATA = "Error fetching data";
   protected final RequestPipeline requestPipeline;
   protected final ResponseRewriterRegistry contentRewriterRegistry;
@@ -86,10 +90,8 @@ public class AccelHandler {
       try {
         results = contentRewriterRegistry.rewriteHttpResponse(req, results);
       } catch (RewritingException e) {
-        if (!isRecoverable(req, results, e)) {
-          throw new GadgetException(GadgetException.Code.INTERNAL_SERVER_ERROR, e,
-                                    e.getHttpStatusCode());
-        }
+        logger.log(Level.WARNING, "Rewriting failed, serving original results", e);
+        // In case of exception continue using original results.
       }
     } else {
       results = errorResponse;
@@ -143,28 +145,6 @@ public class AccelHandler {
   }
 
   /**
-   * Returns true in case the error encountered while rewriting the content
-   * is recoverable. The rationale behind it is that errors should be thrown
-   * only in case of serious grave errors (defined to be un recoverable).
-   * It should always be preferred to handle errors and return the original
-   * content at least.
-   *
-   * TODO: Think through all cases which are recoverable to enforce minimal
-   * possible set of constraints.
-   * TODO: Log the exception and context around it.
-   *
-   * @param req The http request for fetching the resource.
-   * @param results The result of rewriting.
-   * @param exception Exception caught.
-   * @return True if the error is recoverable, false otherwise.
-   */
-  protected boolean isRecoverable(HttpRequest req, HttpResponse results,
-                                  RewritingException exception) {
-    return !(Strings.isNullOrEmpty(results.getResponseAsString()) &&
-             results.getHeaders() == null);
-  }
-
-  /**
    * Build an HttpRequest object encapsulating the request details as requested
    * by the user.
    * @param httpRequest The http request.
@@ -188,7 +168,7 @@ public class AccelHandler {
 
     // Set and copy headers.
     ServletUtil.setXForwardedForHeader(httpRequest, req);
-    
+
     UriUtils.copyRequestHeaders(
         httpRequest, req,
         UriUtils.DisallowedHeaders.POST_INCOMPATIBLE_DIRECTIVES,

Modified: shindig/trunk/java/gadgets/src/test/java/org/apache/shindig/gadgets/servlet/HtmlAccelServletTest.java
URL: http://svn.apache.org/viewvc/shindig/trunk/java/gadgets/src/test/java/org/apache/shindig/gadgets/servlet/HtmlAccelServletTest.java?rev=1082941&r1=1082940&r2=1082941&view=diff
==============================================================================
--- shindig/trunk/java/gadgets/src/test/java/org/apache/shindig/gadgets/servlet/HtmlAccelServletTest.java (original)
+++ shindig/trunk/java/gadgets/src/test/java/org/apache/shindig/gadgets/servlet/HtmlAccelServletTest.java Fri Mar 18 15:03:29 2011
@@ -31,6 +31,7 @@ import org.apache.shindig.gadgets.http.H
 import org.apache.shindig.gadgets.rewrite.CaptureRewriter;
 import org.apache.shindig.gadgets.rewrite.DefaultResponseRewriterRegistry;
 import org.apache.shindig.gadgets.rewrite.ResponseRewriter;
+import org.apache.shindig.gadgets.rewrite.RewritingException;
 import org.apache.shindig.gadgets.uri.AccelUriManager;
 import org.apache.shindig.gadgets.uri.DefaultAccelUriManager;
 import org.apache.shindig.gadgets.uri.DefaultProxyUriManager;
@@ -51,6 +52,11 @@ import static org.easymock.EasyMock.expe
 
 public class HtmlAccelServletTest extends ServletTestFixture {
 
+  private final ContainerConfig config = new FakeContainerConfig();
+
+  private final AccelUriManager accelUriManager = new DefaultAccelUriManager(
+      config, new DefaultProxyUriManager(config, null));
+
   private static class FakeContainerConfig extends BasicContainerConfig {
     protected final Map<String, Object> data = ImmutableMap.<String, Object>builder()
         .put(AccelUriManager.PROXY_HOST_PARAM, "apache.org")
@@ -84,9 +90,6 @@ public class HtmlAccelServletTest extend
 
   @Before
   public void setUp() throws Exception {
-    ContainerConfig config = new FakeContainerConfig();
-    AccelUriManager accelUriManager = new DefaultAccelUriManager(
-        config, new DefaultProxyUriManager(config, null));
 
     rewriter = new FakeCaptureRewriter();
     rewriterRegistry = new DefaultResponseRewriterRegistry(
@@ -398,4 +401,39 @@ public class HtmlAccelServletTest extend
     assertEquals(200, recorder.getHttpStatusCode());
     assertTrue(rewriter.responseWasRewritten());
   }
+
+  @Test
+  public void testReturnOriginalResponseIfRewritingFails() throws Exception {
+    ResponseRewriter throwingRewriter = new ResponseRewriter() {
+      public void rewrite(HttpRequest request, HttpResponseBuilder response)
+          throws RewritingException {
+        response.setContent(REWRITE_CONTENT);
+        throw new RewritingException("", 404);
+      }
+    };
+    rewriterRegistry = new DefaultResponseRewriterRegistry(
+        Arrays.<ResponseRewriter>asList(throwingRewriter), null);
+    servlet = new HtmlAccelServlet();
+    servlet.setHandler(new AccelHandler(pipeline, rewriterRegistry,
+                                        accelUriManager, true));
+    String url = "http://example.org/data.html";
+    String data = "<html><body>Hello World</body></html>";
+
+    HttpRequest req = new HttpRequest(Uri.parse(url));
+    req.addHeader("Host", Uri.parse(url).getAuthority());
+    HttpResponse resp = new HttpResponseBuilder()
+        .setResponse(data.getBytes())
+        .setHeader("Content-Type", "text/html")
+        .setHttpStatusCode(200)
+        .create();
+    expect(pipeline.execute(req)).andReturn(resp).once();
+    expectRequest("", url);
+    replay();
+
+    servlet.doGet(request, recorder);
+
+    verify();
+    assertEquals(data, recorder.getResponseAsString());
+    assertEquals(200, recorder.getHttpStatusCode());
+  }
 }