[core] only cache successful or NotFound responses

We don't want other types of error end up in our cache, since it'll likely evict perfectly good content.

2 files changed, 176 insertions, 0 deletions

diff --git a/test/storage/cache_response.cpp b/test/storage/cache_response.cpp
index 16a9c3d47a..8339d8aa84 100644
--- a/test/storage/cache_response.cpp
+++ b/test/storage/cache_response.cpp
@@ -50,3 +50,175 @@ TEST_F(Storage, CacheResponse) {
 
     loop.run();
 }
+
+// Make sure we don't store a bad server response into the cache
+TEST_F(Storage, CacheNotFound) {
+    SCOPED_TEST(CacheNotFound);
+
+    using namespace mbgl;
+
+    util::RunLoop loop;
+    SQLiteCache cache(":memory:");
+    OnlineFileSource fs(&cache);
+
+    const Resource resource{ Resource::Unknown, "http://127.0.0.1:3000/not-found" };
+
+    // Insert existing data into the cache that will be marked as stale.
+    auto response = std::make_shared<Response>();
+    response->data = std::make_shared<const std::string>("existing data");
+    cache.put(resource, response, FileCache::Hint::Full);
+
+    std::unique_ptr<FileRequest> req1;
+    std::unique_ptr<WorkRequest> req2;
+
+    int counter = 0;
+
+    // Then, request the actual URL and check that we're getting the rigged cache response first,
+    // then the connection error message.
+    req1 = fs.request(resource, [&](Response res) {
+        if (counter == 0) {
+            EXPECT_EQ(nullptr, res.error);
+            EXPECT_EQ(true, res.stale);
+            ASSERT_TRUE(res.data.get());
+            EXPECT_EQ("existing data", *res.data);
+            EXPECT_EQ(Seconds::zero(), res.expires);
+            EXPECT_EQ(Seconds::zero(), res.modified);
+            EXPECT_EQ("", res.etag);
+        } else if (counter == 1) {
+            EXPECT_NE(nullptr, res.error);
+            EXPECT_EQ(Response::Error::Reason::NotFound, res.error->reason);
+            ASSERT_TRUE(res.data.get());
+            EXPECT_EQ("Not Found!", *res.data);
+            req1.reset();
+
+            // Finally, check the cache to make sure we cached the 404 response.
+            req2 = cache.get(resource, [&](std::unique_ptr<Response> res2) {
+                EXPECT_NE(nullptr, res2->error);
+                EXPECT_EQ(Response::Error::Reason::NotFound, res2->error->reason);
+                ASSERT_TRUE(res2->data.get());
+                EXPECT_EQ("Not Found!", *res2->data);
+                CacheNotFound.finish();
+                loop.stop();
+            });
+        } else {
+            FAIL() << "Got too many responses";
+        }
+        counter++;
+    });
+
+    loop.run();
+}
+
+// Make sure we don't store a bad server response into the cache
+TEST_F(Storage, DontCacheConnectionErrors) {
+    SCOPED_TEST(DontCacheConnectionErrors);
+
+    using namespace mbgl;
+
+    util::RunLoop loop;
+    SQLiteCache cache(":memory:");
+    OnlineFileSource fs(&cache);
+
+    const Resource resource{ Resource::Unknown, "http://127.0.0.1:3001" };
+
+    // Insert existing data into the cache that will be marked as stale.
+    auto response = std::make_shared<Response>();
+    response->data = std::make_shared<const std::string>("existing data");
+    cache.put(resource, response, FileCache::Hint::Full);
+
+    std::unique_ptr<FileRequest> req1;
+    std::unique_ptr<WorkRequest> req2;
+
+    int counter = 0;
+
+    // Then, request the actual URL and check that we're getting the rigged cache response first,
+    // then the connection error message.
+    req1 = fs.request(resource, [&](Response res) {
+        if (counter == 0) {
+            EXPECT_EQ(nullptr, res.error);
+            EXPECT_EQ(true, res.stale);
+            ASSERT_TRUE(res.data.get());
+            EXPECT_EQ("existing data", *res.data);
+            EXPECT_EQ(Seconds::zero(), res.expires);
+            EXPECT_EQ(Seconds::zero(), res.modified);
+            EXPECT_EQ("", res.etag);
+        } else if (counter == 1) {
+            EXPECT_NE(nullptr, res.error);
+            EXPECT_EQ(Response::Error::Reason::Connection, res.error->reason);
+            req1.reset();
+
+            // Finally, check the cache to make sure we still have our original data in there rather
+            // than the failed connection attempt.
+            req2 = cache.get(resource, [&](std::unique_ptr<Response> res2) {
+                EXPECT_EQ(nullptr, res2->error);
+                ASSERT_TRUE(res2->data.get());
+                EXPECT_EQ("existing data", *res2->data);
+                DontCacheConnectionErrors.finish();
+                loop.stop();
+            });
+        } else {
+            FAIL() << "Got too many responses";
+        }
+        counter++;
+    });
+
+    loop.run();
+}
+
+// Make sure we don't store a bad server response into the cache
+TEST_F(Storage, DontCacheServerErrors) {
+    SCOPED_TEST(DontCacheServerErrors);
+
+    using namespace mbgl;
+
+    util::RunLoop loop;
+    SQLiteCache cache(":memory:");
+    OnlineFileSource fs(&cache);
+
+    const Resource resource{ Resource::Unknown, "http://127.0.0.1:3000/permanent-error" };
+
+    // Insert existing data into the cache that will be marked as stale.
+    auto response = std::make_shared<Response>();
+    response->data = std::make_shared<const std::string>("existing data");
+    cache.put(resource, response, FileCache::Hint::Full);
+
+    std::unique_ptr<FileRequest> req1;
+    std::unique_ptr<WorkRequest> req2;
+
+    int counter = 0;
+
+    // Then, request the actual URL and check that we're getting the rigged cache response first,
+    // then the server error message.
+    req1 = fs.request(resource, [&](Response res) {
+        if (counter == 0) {
+            EXPECT_EQ(nullptr, res.error);
+            EXPECT_EQ(true, res.stale);
+            ASSERT_TRUE(res.data.get());
+            EXPECT_EQ("existing data", *res.data);
+            EXPECT_EQ(Seconds::zero(), res.expires);
+            EXPECT_EQ(Seconds::zero(), res.modified);
+            EXPECT_EQ("", res.etag);
+        } else if (counter == 1) {
+            EXPECT_NE(nullptr, res.error);
+            EXPECT_EQ(Response::Error::Reason::Server, res.error->reason);
+            ASSERT_TRUE(res.data.get());
+            EXPECT_EQ("Server Error!", *res.data);
+            req1.reset();
+
+            // Finally, check the cache to make sure we still have our original data in there rather
+            // than the failed connection attempt.
+            req2 = cache.get(resource, [&](std::unique_ptr<Response> res2) {
+                EXPECT_EQ(nullptr, res2->error);
+                ASSERT_TRUE(res2->data.get());
+                EXPECT_EQ("existing data", *res2->data);
+                DontCacheServerErrors.finish();
+                loop.stop();
+            });
+        } else {
+            FAIL() << "Got too many responses";
+        }
+        counter++;
+    });
+
+    loop.run();
+}
diff --git a/test/storage/server.js b/test/storage/server.js
index a814ce15e6..5d547b1506 100755
--- a/test/storage/server.js
+++ b/test/storage/server.js
@@ -81,6 +81,10 @@ app.get('/revalidate-etag', function(req, res) {
     revalidateEtagCounter++;
 });
 
+app.get('/not-found', function(req, res) {
+    res.status(404).send('Not Found!');
+});
+
 app.get('/permanent-error', function(req, res) {
     res.status(500).send('Server Error!');
 });