From 2aafdbc1b84192d9a51ff7f5f9d9830e0951bb2a Mon Sep 17 00:00:00 2001 From: John Firebaugh Date: Wed, 13 Jan 2016 16:47:36 -0800 Subject: [core] Add an explicit "not modified" indicator to Response --- include/mbgl/storage/response.hpp | 3 +++ platform/android/src/http_request_android.cpp | 21 ++++++++++++--------- platform/darwin/http_request_nsurl.mm | 22 ++++++++++++---------- platform/default/http_request_curl.cpp | 22 ++++++++++++---------- platform/default/online_file_source.cpp | 6 +----- src/mbgl/storage/response.cpp | 1 + test/storage/cache_revalidate.cpp | 8 ++------ 7 files changed, 43 insertions(+), 40 deletions(-) diff --git a/include/mbgl/storage/response.hpp b/include/mbgl/storage/response.hpp index 852110cb2a..532a91da97 100644 --- a/include/mbgl/storage/response.hpp +++ b/include/mbgl/storage/response.hpp @@ -24,6 +24,9 @@ public: // Stale responses are fetched from cache and are expired. bool stale = false; + // This is set to true for 304 Not Modified responses. + bool notModified = false; + // The actual data of the response. This is guaranteed to never be empty. std::shared_ptr data; diff --git a/platform/android/src/http_request_android.cpp b/platform/android/src/http_request_android.cpp index a6e00ff2b5..3458040083 100644 --- a/platform/android/src/http_request_android.cpp +++ b/platform/android/src/http_request_android.cpp @@ -206,17 +206,20 @@ void HTTPAndroidRequest::onResponse(int code, std::string message, std::string e if (code == 200) { // Nothing to do; this is what we want } else if (code == 304) { + response->notModified = true; + if (existingResponse) { - if (existingResponse->error) { - response->error = std::make_unique(*existingResponse->error); + if (response->expires == Seconds::zero()) { + response->expires = existingResponse->expires; + } + + if (response->modified == Seconds::zero()) { + response->modified = existingResponse->modified; + } + + if (response->etag.empty()) { + response->etag = existingResponse->etag; } - response->data = existingResponse->data; - response->modified = existingResponse->modified; - // We're not updating `expired`, it was probably set during the request. - response->etag = existingResponse->etag; - } else { - // This is an unsolicited 304 response and should only happen on malfunctioning - // HTTP servers. It likely doesn't include any data, but we don't have much options. } } else if (code == 404) { response->error = std::make_unique(Error::Reason::NotFound, "HTTP status code 404"); diff --git a/platform/darwin/http_request_nsurl.mm b/platform/darwin/http_request_nsurl.mm index 11c8100e60..a43210cbc0 100644 --- a/platform/darwin/http_request_nsurl.mm +++ b/platform/darwin/http_request_nsurl.mm @@ -226,18 +226,20 @@ void HTTPNSURLRequest::handleResult(NSData *data, NSURLResponse *res, NSError *e if (responseCode == 200) { // Nothing to do; this is what we want. } else if (responseCode == 304) { + response->notModified = true; + if (existingResponse) { - // We're going to copy over the existing response's data. - if (existingResponse->error) { - response->error = std::make_unique(*existingResponse->error); + if (response->expires == Seconds::zero()) { + response->expires = existingResponse->expires; + } + + if (response->modified == Seconds::zero()) { + response->modified = existingResponse->modified; + } + + if (response->etag.empty()) { + response->etag = existingResponse->etag; } - response->data = existingResponse->data; - response->modified = existingResponse->modified; - // We're not updating `expired`, it was probably set during the request. - response->etag = existingResponse->etag; - } else { - // This is an unsolicited 304 response and should only happen on malfunctioning - // HTTP servers. It likely doesn't include any data, but we don't have much options. } } else if (responseCode == 404) { response->error = diff --git a/platform/default/http_request_curl.cpp b/platform/default/http_request_curl.cpp index e1ea931fbe..7f95d187cb 100644 --- a/platform/default/http_request_curl.cpp +++ b/platform/default/http_request_curl.cpp @@ -529,18 +529,20 @@ void HTTPCURLRequest::handleResult(CURLcode code) { if (responseCode == 200) { // Nothing to do; this is what we want. } else if (responseCode == 304) { + response->notModified = true; + if (existingResponse) { - // We're going to copy over the existing response's data. - if (existingResponse->error) { - response->error = std::make_unique(*existingResponse->error); + if (response->expires == Seconds::zero()) { + response->expires = existingResponse->expires; + } + + if (response->modified == Seconds::zero()) { + response->modified = existingResponse->modified; + } + + if (response->etag.empty()) { + response->etag = existingResponse->etag; } - response->data = existingResponse->data; - response->modified = existingResponse->modified; - // We're not updating `expired`, it was probably set during the request. - response->etag = existingResponse->etag; - } else { - // This is an unsolicited 304 response and should only happen on malfunctioning - // HTTP servers. It likely doesn't include any data, but we don't have much options. } } else if (responseCode == 404) { response->error = diff --git a/platform/default/online_file_source.cpp b/platform/default/online_file_source.cpp index 634a6710a7..657871b907 100644 --- a/platform/default/online_file_source.cpp +++ b/platform/default/online_file_source.cpp @@ -252,11 +252,7 @@ void OnlineFileRequestImpl::scheduleRealRequest(OnlineFileSource::Impl& impl, bo (!response_->error || (response_->error->reason == Response::Error::Reason::NotFound))) { // Store response in database. Make sure we only refresh the expires column if the data // didn't change. - SQLiteCache::Hint hint = SQLiteCache::Hint::Full; - if (response && response_->data == response->data) { - hint = SQLiteCache::Hint::Refresh; - } - impl.cache->put(resource, *response_, hint); + impl.cache->put(resource, *response_, response_->notModified ? SQLiteCache::Hint::Refresh : SQLiteCache::Hint::Full); } response = response_; diff --git a/src/mbgl/storage/response.cpp b/src/mbgl/storage/response.cpp index 4b1125c110..b42fb211c8 100644 --- a/src/mbgl/storage/response.cpp +++ b/src/mbgl/storage/response.cpp @@ -10,6 +10,7 @@ Response::Response(const Response& res) { Response& Response::operator=(const Response& res) { error = res.error ? std::make_unique(*res.error) : nullptr; stale = res.stale; + notModified = res.notModified; data = res.data; modified = res.modified; expires = res.expires; diff --git a/test/storage/cache_revalidate.cpp b/test/storage/cache_revalidate.cpp index ffc622f37c..fc65986a17 100644 --- a/test/storage/cache_revalidate.cpp +++ b/test/storage/cache_revalidate.cpp @@ -48,9 +48,7 @@ TEST_F(Storage, CacheRevalidateSame) { EXPECT_EQ(nullptr, res2.error); EXPECT_EQ(false, res2.stale); - ASSERT_TRUE(res2.data.get()); - EXPECT_EQ("Response", *res2.data); - // We use this to indicate that a 304 reply came back. + EXPECT_TRUE(res2.notModified); EXPECT_LT(Seconds::zero(), res2.expires); EXPECT_EQ(Seconds::zero(), res2.modified); // We're not sending the ETag in the 304 reply, but it should still be there. @@ -108,9 +106,7 @@ TEST_F(Storage, CacheRevalidateModified) { EXPECT_EQ(nullptr, res2.error); EXPECT_EQ(false, res2.stale); - ASSERT_TRUE(res2.data.get()); - EXPECT_EQ("Response", *res2.data); - // We use this to indicate that a 304 reply came back. + EXPECT_TRUE(res2.notModified); EXPECT_LT(Seconds::zero(), res2.expires); EXPECT_EQ(1420070400, res2.modified.count()); EXPECT_EQ("", res2.etag); -- cgit v1.2.1