Author: johnh Date: Fri Jan 8 20:07:25 2010 New Revision: 897314 URL: http://svn.apache.org/viewvc?rev=897314&view=rev Log: Don't cache HTTP error responses in GadgetSpec cache. HTTP error responses are already cached in HttpCache.
@see http://codereview.appspot.com/183129/show for details. Modified: incubator/shindig/trunk/java/gadgets/src/main/java/org/apache/shindig/gadgets/AbstractSpecFactory.java incubator/shindig/trunk/java/gadgets/src/test/java/org/apache/shindig/gadgets/DefaultGadgetSpecFactoryTest.java Modified: incubator/shindig/trunk/java/gadgets/src/main/java/org/apache/shindig/gadgets/AbstractSpecFactory.java URL: http://svn.apache.org/viewvc/incubator/shindig/trunk/java/gadgets/src/main/java/org/apache/shindig/gadgets/AbstractSpecFactory.java?rev=897314&r1=897313&r2=897314&view=diff ============================================================================== --- incubator/shindig/trunk/java/gadgets/src/main/java/org/apache/shindig/gadgets/AbstractSpecFactory.java (original) +++ incubator/shindig/trunk/java/gadgets/src/main/java/org/apache/shindig/gadgets/AbstractSpecFactory.java Fri Jan 8 20:07:25 2010 @@ -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); + } + } } Modified: incubator/shindig/trunk/java/gadgets/src/test/java/org/apache/shindig/gadgets/DefaultGadgetSpecFactoryTest.java URL: http://svn.apache.org/viewvc/incubator/shindig/trunk/java/gadgets/src/test/java/org/apache/shindig/gadgets/DefaultGadgetSpecFactoryTest.java?rev=897314&r1=897313&r2=897314&view=diff ============================================================================== --- incubator/shindig/trunk/java/gadgets/src/test/java/org/apache/shindig/gadgets/DefaultGadgetSpecFactoryTest.java (original) +++ incubator/shindig/trunk/java/gadgets/src/test/java/org/apache/shindig/gadgets/DefaultGadgetSpecFactoryTest.java Fri Jan 8 20:07:25 2010 @@ -239,19 +239,23 @@ specFactory.getGadgetSpec(createContext(SPEC_URL, true)); } - @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(); + expect(pipeline.execute(firstRequest)).andReturn(new HttpResponse(LOCAL_SPEC_XML)).times(2); 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)); + specFactory.getGadgetSpec(createContext(SPEC_URL, false)); - assertEquals(original.getUrl(), cached.getUrl()); - assertEquals(original.getChecksum(), cached.getChecksum()); + try { + specFactory.getGadgetSpec(createContext(SPEC_URL, true)); + } catch (GadgetException e) { + // Expected condition. + } + + // Now make sure the cache wasn't populated w/ the error. + specFactory.getGadgetSpec(createContext(SPEC_URL, false)); } @Test(expected = GadgetException.class)
