[all] Do not set Response data for 404s

For AssetFileSource and the node FileSource this was already the case; this makes the other implementations consistent.

6 files changed, 15 insertions, 29 deletions

diff --git a/include/mbgl/storage/response.hpp b/include/mbgl/storage/response.hpp
index a44902e1f9..278fce1dfe 100644
--- a/include/mbgl/storage/response.hpp
+++ b/include/mbgl/storage/response.hpp
@@ -23,7 +23,7 @@ public:
     // This is set to true for 304 Not Modified responses.
     bool notModified = false;
 
-    // The actual data of the response. This is null if notModified is true.
+    // The actual data of the response. Present only for non-error, non-notModified responses.
     std::shared_ptr<const std::string> data;
 
     optional<SystemTimePoint> modified;
diff --git a/platform/android/src/http_request_android.cpp b/platform/android/src/http_request_android.cpp
index 390334627a..105ba5860c 100644
--- a/platform/android/src/http_request_android.cpp
+++ b/platform/android/src/http_request_android.cpp
@@ -199,17 +199,14 @@ void HTTPAndroidRequest::onResponse(JNIEnv* env, int code, jstring /* message */
         response->expires = util::parseTimePoint(mbgl::android::std_string_from_jstring(env, expires).c_str());
     }
 
-    if (body != nullptr) {
-        jbyte* bodyData = env->GetByteArrayElements(body, nullptr);
-        response->data = std::make_shared<std::string>(reinterpret_cast<char*>(bodyData), env->GetArrayLength(body));
-        env->ReleaseByteArrayElements(body, bodyData, JNI_ABORT);
-    }
-
     if (code == 200) {
-        // Nothing to do; this is what we want
+        if (body != nullptr) {
+            jbyte* bodyData = env->GetByteArrayElements(body, nullptr);
+            response->data = std::make_shared<std::string>(reinterpret_cast<char*>(bodyData), env->GetArrayLength(body));
+            env->ReleaseByteArrayElements(body, bodyData, JNI_ABORT);
+        }
     } else if (code == 304) {
         response->notModified = true;
-        response->data.reset();
     } else if (code == 404) {
         response->error = std::make_unique<Error>(Error::Reason::NotFound, "HTTP status code 404");
     } else if (code >= 500 && code < 600) {
diff --git a/platform/darwin/http_request_nsurl.mm b/platform/darwin/http_request_nsurl.mm
index fa705638b4..890e347afc 100644
--- a/platform/darwin/http_request_nsurl.mm
+++ b/platform/darwin/http_request_nsurl.mm
@@ -194,8 +194,6 @@ void HTTPNSURLRequest::handleResult(NSData *data, NSURLResponse *res, NSError *e
     } else if ([res isKindOfClass:[NSHTTPURLResponse class]]) {
         const long responseCode = [(NSHTTPURLResponse *)res statusCode];
 
-        response->data = std::make_shared<std::string>((const char *)[data bytes], [data length]);
-
         NSDictionary *headers = [(NSHTTPURLResponse *)res allHeaderFields];
         NSString *cache_control = [headers objectForKey:@"Cache-Control"];
         if (cache_control) {
@@ -218,10 +216,9 @@ void HTTPNSURLRequest::handleResult(NSData *data, NSURLResponse *res, NSError *e
         }
 
         if (responseCode == 200) {
-            // Nothing to do; this is what we want.
+            response->data = std::make_shared<std::string>((const char *)[data bytes], [data length]);
         } else if (responseCode == 304) {
             response->notModified = true;
-            response->data.reset();
         } else if (responseCode == 404) {
             response->error =
                 std::make_unique<Error>(Error::Reason::NotFound, "HTTP status code 404");
diff --git a/platform/default/http_request_curl.cpp b/platform/default/http_request_curl.cpp
index f7fb27afc5..660da4c43e 100644
--- a/platform/default/http_request_curl.cpp
+++ b/platform/default/http_request_curl.cpp
@@ -509,19 +509,14 @@ void HTTPCURLRequest::handleResult(CURLcode code) {
         long responseCode = 0;
         curl_easy_getinfo(handle, CURLINFO_RESPONSE_CODE, &responseCode);
 
-        // Move over any data we got. We're storing this in a separate object because the Response
-        // object defines it as const.
-        if (data) {
-            response->data = std::move(data);
-        } else {
-            response->data = std::make_shared<std::string>();
-        }
-
         if (responseCode == 200) {
-            // Nothing to do; this is what we want.
+            if (data) {
+                response->data = std::move(data);
+            } else {
+                response->data = std::make_shared<std::string>();
+            }
         } else if (responseCode == 304) {
             response->notModified = true;
-            response->data.reset();
         } else if (responseCode == 404) {
             response->error =
                 std::make_unique<Error>(Error::Reason::NotFound, "HTTP status code 404");
diff --git a/test/storage/http_error.cpp b/test/storage/http_error.cpp
index 3f29e5c745..259ea825eb 100644
--- a/test/storage/http_error.cpp
+++ b/test/storage/http_error.cpp
@@ -26,8 +26,7 @@ TEST_F(Storage, HTTPTemporaryError) {
             ASSERT_NE(nullptr, res.error);
             EXPECT_EQ(Response::Error::Reason::Server, res.error->reason);
             EXPECT_EQ("HTTP status code 500", res.error->message);
-            ASSERT_TRUE(res.data.get());
-            EXPECT_EQ("", *res.data);
+            ASSERT_FALSE(bool(res.data));
             EXPECT_FALSE(bool(res.expires));
             EXPECT_FALSE(bool(res.modified));
             EXPECT_FALSE(bool(res.etag));
diff --git a/test/storage/http_reading.cpp b/test/storage/http_reading.cpp
index 36fbf43a14..d360ec0031 100644
--- a/test/storage/http_reading.cpp
+++ b/test/storage/http_reading.cpp
@@ -47,9 +47,8 @@ TEST_F(Storage, HTTP404) {
         EXPECT_TRUE(util::ThreadContext::currentlyOn(util::ThreadType::Main));
         ASSERT_NE(nullptr, res.error);
         EXPECT_EQ(Response::Error::Reason::NotFound, res.error->reason);
-        ASSERT_TRUE(res.data.get());
-        EXPECT_EQ("Cannot GET /doesnotexist\n", *res.data);
         EXPECT_EQ("HTTP status code 404", res.error->message);
+        EXPECT_FALSE(bool(res.data));
         EXPECT_FALSE(bool(res.expires));
         EXPECT_FALSE(bool(res.modified));
         EXPECT_FALSE(bool(res.etag));
@@ -74,9 +73,8 @@ TEST_F(Storage, HTTP500) {
         EXPECT_TRUE(util::ThreadContext::currentlyOn(util::ThreadType::Main));
         ASSERT_NE(nullptr, res.error);
         EXPECT_EQ(Response::Error::Reason::Server, res.error->reason);
-        ASSERT_TRUE(res.data.get());
-        EXPECT_EQ("Server Error!", *res.data);
         EXPECT_EQ("HTTP status code 500", res.error->message);
+        EXPECT_FALSE(bool(res.data));
         EXPECT_FALSE(bool(res.expires));
         EXPECT_FALSE(bool(res.modified));
         EXPECT_FALSE(bool(res.etag));