[core] Use better types for modified / expires / etag

5 files changed, 89 insertions, 36 deletions

diff --git a/platform/default/http_request_curl.cpp b/platform/default/http_request_curl.cpp
index f5fc395862..078a172c2b 100644
--- a/platform/default/http_request_curl.cpp
+++ b/platform/default/http_request_curl.cpp
@@ -363,12 +363,12 @@ HTTPCURLRequest::HTTPCURLRequest(HTTPCURLContext* context_, const std::string& u
     // If there's already a response, set the correct etags/modified headers to make sure we are
     // getting a 304 response if possible. This avoids redownloading unchanged data.
     if (existingResponse) {
-        if (!existingResponse->etag.empty()) {
-            const std::string header = std::string("If-None-Match: ") + existingResponse->etag;
+        if (existingResponse->etag) {
+            const std::string header = std::string("If-None-Match: ") + *existingResponse->etag;
             headers = curl_slist_append(headers, header.c_str());
-        } else if (existingResponse->modified != Seconds::zero()) {
+        } else if (existingResponse->modified) {
             const std::string time =
-                std::string("If-Modified-Since: ") + util::rfc1123(existingResponse->modified.count());
+                std::string("If-Modified-Since: ") + util::rfc1123(SystemClock::to_time_t(*existingResponse->modified));
             headers = curl_slist_append(headers, time.c_str());
         }
     }
@@ -466,15 +466,15 @@ size_t HTTPCURLRequest::headerCallback(char *const buffer, const size_t size, co
         // Always overwrite the modification date; We might already have a value here from the
         // Date header, but this one is more accurate.
         const std::string value { buffer + begin, length - begin - 2 }; // remove \r\n
-        baton->response->modified = Seconds(curl_getdate(value.c_str(), nullptr));
+        baton->response->modified = SystemClock::from_time_t(curl_getdate(value.c_str(), nullptr));
     } else if ((begin = headerMatches("etag: ", buffer, length)) != std::string::npos) {
-        baton->response->etag = { buffer + begin, length - begin - 2 }; // remove \r\n
+        baton->response->etag = std::string(buffer + begin, length - begin - 2); // remove \r\n
     } else if ((begin = headerMatches("cache-control: ", buffer, length)) != std::string::npos) {
         const std::string value { buffer + begin, length - begin - 2 }; // remove \r\n
         baton->response->expires = parseCacheControl(value.c_str());
     } else if ((begin = headerMatches("expires: ", buffer, length)) != std::string::npos) {
         const std::string value { buffer + begin, length - begin - 2 }; // remove \r\n
-        baton->response->expires = Seconds(curl_getdate(value.c_str(), nullptr));
+        baton->response->expires = SystemClock::from_time_t(curl_getdate(value.c_str(), nullptr));
     }
 
     return length;
@@ -534,15 +534,15 @@ void HTTPCURLRequest::handleResult(CURLcode code) {
             if (existingResponse) {
                 response->data = existingResponse->data;
 
-                if (response->expires == Seconds::zero()) {
+                if (!response->expires) {
                     response->expires = existingResponse->expires;
                 }
 
-                if (response->modified == Seconds::zero()) {
+                if (!response->modified) {
                     response->modified = existingResponse->modified;
                 }
 
-                if (response->etag.empty()) {
+                if (!response->etag) {
                     response->etag = existingResponse->etag;
                 }
             }
diff --git a/platform/default/online_file_source.cpp b/platform/default/online_file_source.cpp
index 18f166b833..89a9d019f0 100644
--- a/platform/default/online_file_source.cpp
+++ b/platform/default/online_file_source.cpp
@@ -192,14 +192,13 @@ void OnlineFileRequestImpl::scheduleCacheRequest(OnlineFileSource::Impl& impl) {
             callback(*response);
         }
 
-        // Force an immediate request if the cached response is stale. Note that this is not
-        // quite the same as what `expirationTimeout` would calculate, because in `expirationTimeout`
-        // Seconds::zero() is treated as "no expiration", but here we want it to force a revalidation.
-        scheduleRealRequest(impl, response && response->expires <= toSeconds(SystemClock::now()));
+        // Force immediate revalidation if the cached response didn't indicate an expiration. If
+        // it did indicate an expiration, revalidation will happen in the normal scheduling flow.
+        scheduleRealRequest(impl, response && !response->expires);
     });
 }
 
-static Seconds errorRetryTimeout(const Response& response, uint32_t failedRequests) {
+static Duration errorRetryTimeout(const Response& response, uint32_t failedRequests) {
     if (response.error && response.error->reason == Response::Error::Reason::Server) {
         // Retry after one second three times, then start exponential backoff.
         return Seconds(failedRequests <= 3 ? 1 : 1 << std::min(failedRequests - 3, 31u));
@@ -209,16 +208,15 @@ static Seconds errorRetryTimeout(const Response& response, uint32_t failedReques
         return Seconds(1 << std::min(failedRequests - 1, 31u));
     } else {
         // No error, or not an error that triggers retries.
-        return Seconds::max();
+        return Duration::max();
     }
 }
 
-static Seconds expirationTimeout(const Response& response) {
-    // Seconds::zero() is a special value meaning "no expiration".
-    if (response.expires > Seconds::zero()) {
-        return std::max(Seconds::zero(), response.expires - toSeconds(SystemClock::now()));
+static Duration expirationTimeout(const Response& response) {
+    if (response.expires) {
+        return std::max(SystemDuration::zero(), *response.expires - SystemClock::now());
     } else {
-        return Seconds::max();
+        return Duration::max();
     }
 }
 
@@ -228,7 +226,7 @@ void OnlineFileRequestImpl::scheduleRealRequest(OnlineFileSource::Impl& impl, bo
         return;
     }
 
-    Seconds timeout = Seconds::zero();
+    Duration timeout = Duration::zero();
 
     // If there was a prior response and we're not being asked for a forced refresh, calculate a timeout
     // that depends on how many consecutive errors we've encountered, and on the expiration time.
@@ -236,7 +234,7 @@ void OnlineFileRequestImpl::scheduleRealRequest(OnlineFileSource::Impl& impl, bo
         timeout = std::min(errorRetryTimeout(*response, failedRequests),
                            expirationTimeout(*response));
 
-        if (timeout == Seconds::max()) {
+        if (timeout == Duration::max()) {
             return;
         }
     }
diff --git a/platform/default/sqlite3.cpp b/platform/default/sqlite3.cpp
index 7e8a56f7f9..5122e01015 100644
--- a/platform/default/sqlite3.cpp
+++ b/platform/default/sqlite3.cpp
@@ -4,6 +4,8 @@
 #include <cassert>
 #include <cstring>
 #include <cstdio>
+#include <chrono>
+#include <experimental/optional>
 
 #pragma GCC diagnostic push
 #pragma GCC diagnostic ignored "-Wunused-variable"
@@ -24,6 +26,9 @@ const static bool sqliteVersionCheck = []() {
 namespace mapbox {
 namespace sqlite {
 
+template <typename T>
+using optional = std::experimental::optional<T>;
+
 Database::Database(const std::string &filename, int flags) {
     const int err = sqlite3_open_v2(filename.c_str(), &db, flags, nullptr);
     if (err != SQLITE_OK) {
@@ -105,6 +110,11 @@ void Statement::check(int err) {
     }
 }
 
+template <> void Statement::bind(int offset, std::nullptr_t) {
+    assert(stmt);
+    check(sqlite3_bind_null(stmt, offset));
+}
+
 template <> void Statement::bind(int offset, int value) {
     assert(stmt);
     check(sqlite3_bind_int(stmt, offset, value));
@@ -130,12 +140,33 @@ template <> void Statement::bind(int offset, const char *value) {
     check(sqlite3_bind_text(stmt, offset, value, -1, SQLITE_STATIC));
 }
 
-void Statement::bind(int offset, const std::string &value, bool retain) {
+void Statement::bind(int offset, const std::string& value, bool retain) {
     assert(stmt);
     check(sqlite3_bind_blob(stmt, offset, value.data(), int(value.size()),
                             retain ? SQLITE_TRANSIENT : SQLITE_STATIC));
 }
 
+template <> void Statement::bind(int offset, std::chrono::system_clock::time_point value) {
+    assert(stmt);
+    check(sqlite3_bind_int64(stmt, offset, std::chrono::system_clock::to_time_t(value)));
+}
+
+template <> void Statement::bind(int offset, optional<std::string> value) {
+    if (!value) {
+        bind(offset, nullptr);
+    } else {
+        bind(offset, *value);
+    }
+}
+
+template <> void Statement::bind(int offset, optional<std::chrono::system_clock::time_point> value) {
+    if (!value) {
+        bind(offset, nullptr);
+    } else {
+        bind(offset, *value);
+    }
+}
+
 bool Statement::run() {
     assert(stmt);
     const int err = sqlite3_step(stmt);
@@ -173,6 +204,29 @@ template <> std::string Statement::get(int offset) {
     };
 }
 
+template <> std::chrono::system_clock::time_point Statement::get(int offset) {
+    assert(stmt);
+    return std::chrono::system_clock::from_time_t(sqlite3_column_int64(stmt, offset));
+}
+
+template <> optional<std::string> Statement::get(int offset) {
+    assert(stmt);
+    if (sqlite3_column_type(stmt, offset) == SQLITE_NULL) {
+        return optional<std::string>();
+    } else {
+        return get<std::string>(offset);
+    }
+}
+
+template <> optional<std::chrono::system_clock::time_point> Statement::get(int offset) {
+    assert(stmt);
+    if (sqlite3_column_type(stmt, offset) == SQLITE_NULL) {
+        return optional<std::chrono::system_clock::time_point>();
+    } else {
+        return get<std::chrono::system_clock::time_point>(offset);
+    }
+}
+
 void Statement::reset() {
     assert(stmt);
     sqlite3_reset(stmt);
diff --git a/platform/default/sqlite_cache.cpp b/platform/default/sqlite_cache.cpp
index 5816467aa5..1980b3dead 100644
--- a/platform/default/sqlite_cache.cpp
+++ b/platform/default/sqlite_cache.cpp
@@ -332,9 +332,9 @@ void SQLiteCache::Impl::get(const Resource &resource, Callback callback) {
                 // Status codes > 1 indicate an error
                 response->error = std::make_unique<Response::Error>(Response::Error::Reason(status));
             }
-            response->modified = Seconds(getStmt->get<int64_t>(1));
-            response->etag = getStmt->get<std::string>(2);
-            response->expires = Seconds(getStmt->get<int64_t>(3));
+            response->modified = getStmt->get<optional<SystemTimePoint>>(1);
+            response->etag = getStmt->get<optional<std::string>>(2);
+            response->expires = getStmt->get<optional<SystemTimePoint>>(3);
             response->data = std::make_shared<std::string>(getStmt->get<std::string>(4));
             if (getStmt->get<int>(5)) { // == compressed
                 response->data = std::make_shared<std::string>(util::decompress(*response->data));
@@ -357,7 +357,7 @@ void SQLiteCache::Impl::get(const Resource &resource, Callback callback) {
                 accessedStmt->reset();
             }
 
-            accessedStmt->bind(1, int64_t(toSeconds(SystemClock::now()).count()));
+            accessedStmt->bind(1, SystemClock::now());
             accessedStmt->bind(2, canonicalURL.c_str());
             accessedStmt->run();
         }
@@ -419,10 +419,10 @@ void SQLiteCache::Impl::put(const Resource& resource, const Response& response)
             putStmt->bind(2 /* status */, 1 /* success */);
         }
         putStmt->bind(3 /* kind */, int(resource.kind));
-        putStmt->bind(4 /* modified */, int64_t(response.modified.count()));
-        putStmt->bind(5 /* etag */, response.etag.c_str());
-        putStmt->bind(6 /* expires */, int64_t(response.expires.count()));
-        putStmt->bind(7 /* accessed */, int64_t(toSeconds(SystemClock::now()).count()));
+        putStmt->bind(4 /* modified */, response.modified);
+        putStmt->bind(5 /* etag */, response.etag);
+        putStmt->bind(6 /* expires */, response.expires);
+        putStmt->bind(7 /* accessed */, SystemClock::now());
 
         std::string data;
         if (resource.kind != Resource::SpriteImage && response.data) {
@@ -452,7 +452,7 @@ void SQLiteCache::Impl::put(const Resource& resource, const Response& response)
     }
 }
 
-void SQLiteCache::Impl::refresh(const Resource& resource, Seconds expires) {
+void SQLiteCache::Impl::refresh(const Resource& resource, optional<SystemTimePoint> expires) {
     try {
         initializeDatabase();
 
@@ -466,8 +466,8 @@ void SQLiteCache::Impl::refresh(const Resource& resource, Seconds expires) {
         }
 
         const auto canonicalURL = util::mapbox::canonicalURL(resource.url);
-        refreshStmt->bind(1, int64_t(toSeconds(SystemClock::now()).count()));
-        refreshStmt->bind(2, int64_t(expires.count()));
+        refreshStmt->bind(1, SystemClock::now());
+        refreshStmt->bind(2, expires);
         refreshStmt->bind(3, canonicalURL.c_str());
         refreshStmt->run();
     } catch (mapbox::sqlite::Exception& ex) {
diff --git a/platform/default/sqlite_cache_impl.hpp b/platform/default/sqlite_cache_impl.hpp
index aef91efec9..e156532402 100644
--- a/platform/default/sqlite_cache_impl.hpp
+++ b/platform/default/sqlite_cache_impl.hpp
@@ -3,6 +3,7 @@
 
 #include <mbgl/storage/sqlite_cache.hpp>
 #include <mbgl/util/chrono.hpp>
+#include <mbgl/util/optional.hpp>
 
 namespace mapbox {
 namespace sqlite {
@@ -23,7 +24,7 @@ public:
 
     void get(const Resource&, Callback);
     void put(const Resource&, const Response&);
-    void refresh(const Resource&, Seconds expires);
+    void refresh(const Resource&, optional<SystemTimePoint> expires);
 
 private:
     void initializeDatabase();