From 3436672f3a791e8910ce6da27e8097b3006208ee Mon Sep 17 00:00:00 2001
From: Matt Basta <me@mattbasta.com>
Date: Mon, 7 Jan 2013 13:46:53 -0800
Subject: [PATCH] Even better handling of redirects and duplicate HTTP
 requests.

---
 appvalidator/errorbundle/metadatamixin.py | 15 ++++++++++++++
 appvalidator/testcases/webappbase.py      | 24 +++++++++++++++--------
 tests/test_webapp_resources.py            | 22 ++++++++++-----------
 3 files changed, 41 insertions(+), 20 deletions(-)

diff --git a/appvalidator/errorbundle/metadatamixin.py b/appvalidator/errorbundle/metadatamixin.py
index 0e36ad5..77b02ea 100644
--- a/appvalidator/errorbundle/metadatamixin.py
+++ b/appvalidator/errorbundle/metadatamixin.py
@@ -26,6 +26,19 @@ class MetadataMixin(object):
         else:
             return False
 
+    def get_or_create(self, name, default, pushable=False):
+        """Retrieve an object that has been stored by another test, or create
+        it if it does not exist.
+
+        """
+
+        if name in self.resources:
+            return self.resources[name]
+        if name in self.pushable_resources:
+            return self.pushable_resources[name]
+        else:
+            return self.save_resource(name, default, pushable=pushable)
+
     def save_resource(self, name, resource, pushable=False):
         """Save an object such that it can be used by other tests."""
 
@@ -34,6 +47,8 @@ class MetadataMixin(object):
         else:
             self.resources[name] = resource
 
+        return resource
+
     def _extend_json(self):
         """Output the metadata as part of the main JSON blob."""
         extension = super(MetadataMixin, self)._extend_json() or {}
diff --git a/appvalidator/testcases/webappbase.py b/appvalidator/testcases/webappbase.py
index deea735..728f439 100644
--- a/appvalidator/testcases/webappbase.py
+++ b/appvalidator/testcases/webappbase.py
@@ -47,6 +47,7 @@ def try_get_data_uri(data_url):
 
 def try_get_resource(err, package, url, filename, resource_type="URL",
                      max_size=True):
+
     # Try to process data URIs first.
     if url.startswith("data:"):
         try:
@@ -76,6 +77,10 @@ def try_get_resource(err, package, url, filename, resource_type="URL",
                 filename=filename)
             return
 
+    http_cache = err.get_or_create('http_cache', {})
+    if url in http_cache:
+        return http_cache[url]
+
     def generic_http_error():
         err.error(
             err_id=("resources", "null_response"),
@@ -87,7 +92,8 @@ def try_get_resource(err, package, url, filename, resource_type="URL",
             filename=filename)
 
     try:
-        request = requests.get(url, prefetch=False)
+        request = requests.get(url, prefetch=False, allow_redirects=True,
+                               timeout=3)
         data = request.raw.read(constants.MAX_RESOURCE_SIZE)
         # Check that there's not more data than the max size.
         if max_size and request.raw.read(1):
@@ -107,10 +113,9 @@ def try_get_resource(err, package, url, filename, resource_type="URL",
             # Some versions of requests don't support close().
             pass
 
-        final_status = request.status_code
-        final_status -= final_status % 100
+        http_cache[url] = data
 
-        if not data and final_status != 300:
+        if not data:
             generic_http_error()
 
         return data
@@ -132,10 +137,6 @@ def try_get_resource(err, package, url, filename, resource_type="URL",
                          "an invalid URL was encountered.",
                          "URL: %s" % url],
             filename=filename)
-    except (requests.exceptions.ConnectionError,
-            requests.exceptions.Timeout,
-            requests.exceptions.HTTPError):
-        generic_http_error()
     except requests.exceptions.TooManyRedirects:
         err.error(
             err_id=("resources", "too_many_redirects"),
@@ -146,6 +147,13 @@ def try_get_resource(err, package, url, filename, resource_type="URL",
                          "permanent URL in an app.",
                          "Requested resource: %s" % url],
             filename=filename)
+    except (requests.exceptions.ConnectionError,
+            requests.exceptions.Timeout,
+            requests.exceptions.HTTPError):
+        generic_http_error()
+
+    # Save the failed request to the cache.
+    http_cache[url] = None
 
 
 def test_icon(err, data, url, size):
diff --git a/tests/test_webapp_resources.py b/tests/test_webapp_resources.py
index 552b06a..261f6e2 100644
--- a/tests/test_webapp_resources.py
+++ b/tests/test_webapp_resources.py
@@ -110,6 +110,16 @@ class TestResourceExceptions(TestCase):
         appbase.try_get_resource(self.err, None, "http://foo.bar/", "")
         self.assert_failed(with_errors=True)
 
+    @mock_requests(reqexc.TooManyRedirects, "Duplicate error")
+    def test_not_duplicated(self, r_g):
+        r_g.side_effect = reqexc.Timeout
+
+        appbase.try_get_resource(self.err, None, "http://foo.bar/", "")
+        self.assert_failed(with_errors=True)
+        appbase.try_get_resource(self.err, None, "http://foo.bar/", "")
+        assert len(self.err.errors) == 1, (
+            "HTTP errors should not be duplicated.")
+
 
 class TestDataOutput(TestCase):
 
@@ -152,18 +162,6 @@ class TestDataOutput(TestCase):
                 self.err, None, "http://foo.bar/", ""), "")
         self.assert_failed(with_errors=True)
 
-    @patch("requests.get")
-    @patch("appvalidator.constants.MAX_RESOURCE_SIZE", 100)
-    def test_empty_redirect(self, r_g):
-        empty_response = Mock()
-        empty_response.raw.read.return_value = ""
-        empty_response.status_code = 345
-        r_g.return_value = empty_response
-
-        eq_(appbase.try_get_resource(
-                self.err, None, "http://foo.bar/", ""), "")
-        self.assert_silent()
-
 
 class TestResourcePolling(TestCase):