Don't cache failed fetches in spec caches (issue183129)

[johnfargo]

Reviewers: shindig.remailer_gmail.com,

Description:
Slightly modifies AbstractSpecFactory behavior to avoid caching
GadgetException in the (local) spec cache when the RequestPipeline
returns a non-SC_OK result.
Rationale: A RequestPipeline may serve the same spec in two different
ways (namely, OK vs. not-OK) depending on additional request context,
such as the user's IP or other contextual information. It will similarly
perform its own HttpCaching logic, in particular performing negative
caching for non-OK results. Caching exceptions in the spec cache has the
main value of avoiding re-parsing of known-faulty specs. Caching a fetch
exception is simply duplicative of the HttpCache's spec while making
subtle RequestPipeline-focused logic more difficult to implement. As
such, I'm removing this caching behavior.

Test added to DefaultGadgetSpecFactoryTest for simplicity. Turns out one
test in it was misnamed anyway, so was repurposed.

Please review this at http://codereview.appspot.com/183129

Affected files:
   
java/gadgets/src/main/java/org/apache/shindig/gadgets/AbstractSpecFactory.java
   
java/gadgets/src/test/java/org/apache/shindig/gadgets/DefaultGadgetSpecFactoryTest.java


Index:  
java/gadgets/src/test/java/org/apache/shindig/gadgets/DefaultGadgetSpecFactoryTest.java
===================================================================
---  
java/gadgets/src/test/java/org/apache/shindig/gadgets/DefaultGadgetSpecFactoryTest.java	 
(revision 896594)
+++  
java/gadgets/src/test/java/org/apache/shindig/gadgets/DefaultGadgetSpecFactoryTest.java	 
(working copy)
@@ -21,6 +21,7 @@
 import static org.easymock.EasyMock.expect;
 import static org.easymock.classextension.EasyMock.replay;
 import static org.junit.Assert.assertEquals;
+import static org.junit.Assert.fail;

 import org.apache.shindig.common.cache.CacheProvider;
 import org.apache.shindig.common.cache.LruCacheProvider;
@@ -240,18 +241,21 @@
   }

   @Test(expected = GadgetException.class)
-  public void badFetchServesCached() throws Exception {
+  public void badFetchThrowsExceptionOverridingCache() throws Exception {
     HttpRequest firstRequest = createCacheableRequest();
     expect(pipeline.execute(firstRequest)).andReturn(new  
HttpResponse(LOCAL_SPEC_XML)).once();
     HttpRequest secondRequest = createIgnoreCacheRequest();
      
expect(pipeline.execute(secondRequest)).andReturn(HttpResponse.error()).once();
     replay(pipeline);

-    GadgetSpec original =  
specFactory.getGadgetSpec(createContext(SPEC_URL, false));
-    GadgetSpec cached = specFactory.getGadgetSpec(createContext(SPEC_URL,  
true));
-
-    assertEquals(original.getUrl(), cached.getUrl());
-    assertEquals(original.getChecksum(), cached.getChecksum());
+    try {
+      specFactory.getGadgetSpec(createContext(SPEC_URL, false));
+    } catch (GadgetException e) {
+      // This is not where the GadgetException should be thrown
+      // This results in a valid, cached response.
+      fail("Should not have failed on first spec fetch");
+    }
+    specFactory.getGadgetSpec(createContext(SPEC_URL, true));
   }

   @Test(expected = GadgetException.class)
Index:  
java/gadgets/src/main/java/org/apache/shindig/gadgets/AbstractSpecFactory.java
===================================================================
---  
java/gadgets/src/main/java/org/apache/shindig/gadgets/AbstractSpecFactory.java	 
(revision 896594)
+++  
java/gadgets/src/main/java/org/apache/shindig/gadgets/AbstractSpecFactory.java	 
(working copy)
@@ -85,12 +85,24 @@
     }

     if (obj == null) {
+      boolean bypassCache = false;
       try {
         obj = fetchFromNetwork(query);
+      } catch (SpecRetrievalFailedException e) {
+        // Don't cache the resulting exception.
+        // The underlying RequestPipeline may (and should) cache non-OK  
HTTP responses
+        // independently, and may do so for the same spec in different  
ways depending
+        // on context. There's no computational benefit to caching this  
exception in
+        // the spec cache since we won't try to re-parse the data anyway,  
as we would
+        // an OK response with a faulty spec.
+        bypassCache = true;
+        obj = e;
       } catch (GadgetException e) {
         obj = e;
       }
-      cache.addElement(query.specUri, obj, refresh);
+      if (!bypassCache) {
+        cache.addElement(query.specUri, obj, refresh);
+      }
     }

     if (obj instanceof GadgetException) {
@@ -104,7 +116,7 @@
   /**
    * Retrieves a spec from the network, parses, and adds it to the cache.
    */
-  protected T fetchFromNetwork(Query query) throws GadgetException {
+  protected T fetchFromNetwork(Query query) throws  
SpecRetrievalFailedException, GadgetException {
     HttpRequest request = new HttpRequest(query.specUri)
         .setIgnoreCache(query.ignoreCache)
         .setGadget(query.gadgetUri)
@@ -207,4 +219,11 @@
       }
     }
   }
+
+  private static class SpecRetrievalFailedException extends  
GadgetException {
+    SpecRetrievalFailedException(Uri specUri, int code) {
+      super(GadgetException.Code.FAILED_TO_RETRIEVE_CONTENT,
+            "Unable to retrieve spec for " + specUri + ". HTTP error " +  
code);
+    }
+  }
 }