From 86cccc4d24804d40ea9d15d47ee1edb975bbf2e3 Mon Sep 17 00:00:00 2001
From: John Firebaugh <john.firebaugh@gmail.com>
Date: Thu, 21 Jul 2016 16:37:18 -0700
Subject: [PATCH] [core] Don't allow style mutations to be overwritten by
 revalidation

* Once we get a fresh style, stop revalidating.
* If the style is mutated, stop revalidating and preserve the existing mutations.
---
 include/mbgl/storage/response.hpp       |  4 ++
 src/mbgl/map/map.cpp                    | 20 ++++++++
 test/map/map.cpp                        | 64 +++++++++++++++++++++++++
 test/src/mbgl/test/fake_file_source.hpp | 60 +++++++++++++++++++++++
 4 files changed, 148 insertions(+)
 create mode 100644 test/src/mbgl/test/fake_file_source.hpp

diff --git a/include/mbgl/storage/response.hpp b/include/mbgl/storage/response.hpp
index 6c79f5e18..4dc3f3668 100644
--- a/include/mbgl/storage/response.hpp
+++ b/include/mbgl/storage/response.hpp
@@ -32,6 +32,10 @@ public:
     optional<Timestamp> modified;
     optional<Timestamp> expires;
     optional<std::string> etag;
+
+    bool isFresh() const {
+        return !expires || *expires > util::now();
+    }
 };
 
 class Response::Error {
diff --git a/src/mbgl/map/map.cpp b/src/mbgl/map/map.cpp
index 15f7779f8..2e7dc06e8 100644
--- a/src/mbgl/map/map.cpp
+++ b/src/mbgl/map/map.cpp
@@ -67,6 +67,7 @@ public:
 
     std::string styleURL;
     std::string styleJSON;
+    bool styleMutated = false;
 
     std::unique_ptr<AsyncRequest> styleRequest;
 
@@ -293,10 +294,21 @@ void Map::setStyleURL(const std::string& url) {
     impl->styleRequest = nullptr;
     impl->styleURL = url;
     impl->styleJSON.clear();
+    impl->styleMutated = false;
 
     impl->style = std::make_unique<Style>(impl->fileSource, impl->pixelRatio);
 
     impl->styleRequest = impl->fileSource.request(Resource::style(impl->styleURL), [this](Response res) {
+        // Once we get a fresh style, or the style is mutated, stop revalidating.
+        if (res.isFresh() || impl->styleMutated) {
+            impl->styleRequest.reset();
+        }
+
+        // Don't allow a loaded, mutated style to be overwritten with a new version.
+        if (impl->styleMutated && impl->style->loaded) {
+            return;
+        }
+
         if (res.error) {
             if (res.error->reason == Response::Error::Reason::NotFound &&
                 util::mapbox::isMapboxURL(impl->styleURL)) {
@@ -323,6 +335,8 @@ void Map::setStyleJSON(const std::string& json) {
 
     impl->styleURL.clear();
     impl->styleJSON.clear();
+    impl->styleMutated = false;
+
     impl->style = std::make_unique<Style>(impl->fileSource, impl->pixelRatio);
 
     impl->loadStyleJSON(json);
@@ -746,22 +760,27 @@ AnnotationIDs Map::queryPointAnnotations(const ScreenBox& box) {
 #pragma mark - Style API
 
 style::Source* Map::getSource(const std::string& sourceID) {
+    impl->styleMutated = true;
     return impl->style ? impl->style->getSource(sourceID) : nullptr;
 }
 
 void Map::addSource(std::unique_ptr<style::Source> source) {
+    impl->styleMutated = true;
     impl->style->addSource(std::move(source));
 }
 
 void Map::removeSource(const std::string& sourceID) {
+    impl->styleMutated = true;
     impl->style->removeSource(sourceID);
 }
 
 style::Layer* Map::getLayer(const std::string& layerID) {
+    impl->styleMutated = true;
     return impl->style ? impl->style->getLayer(layerID) : nullptr;
 }
 
 void Map::addLayer(std::unique_ptr<Layer> layer, const optional<std::string>& before) {
+    impl->styleMutated = true;
     impl->view.activate();
 
     impl->style->addLayer(std::move(layer), before);
@@ -772,6 +791,7 @@ void Map::addLayer(std::unique_ptr<Layer> layer, const optional<std::string>& be
 }
 
 void Map::removeLayer(const std::string& id) {
+    impl->styleMutated = true;
     impl->view.activate();
 
     impl->style->removeLayer(id);
diff --git a/test/map/map.cpp b/test/map/map.cpp
index 1f6e59f3e..79c1993cb 100644
--- a/test/map/map.cpp
+++ b/test/map/map.cpp
@@ -1,5 +1,6 @@
 #include <mbgl/test/util.hpp>
 #include <mbgl/test/stub_file_source.hpp>
+#include <mbgl/test/fake_file_source.hpp>
 #include <mbgl/test/fixture_log_observer.hpp>
 
 #include <mbgl/map/map.hpp>
@@ -80,6 +81,69 @@ TEST(Map, DoubleStyleLoad) {
     map.setStyleJSON("");
 }
 
+TEST(Map, StyleFresh) {
+    // The map should not revalidate fresh styles.
+
+    MapTest test;
+    FakeFileSource fileSource;
+
+    Map map(test.view, fileSource, MapMode::Still);
+    map.setStyleURL("mapbox://styles/test");
+    EXPECT_EQ(1u, fileSource.requests.size());
+
+    Response response;
+    response.data = std::make_shared<std::string>(util::read_file("test/fixtures/api/empty.json"));
+    response.expires = Timestamp::max();
+
+    fileSource.respond(Resource::Style, response);
+    EXPECT_EQ(0u, fileSource.requests.size());
+}
+
+TEST(Map, StyleExpired) {
+    // The map should allow expired styles to be revalidated, so long as no mutations are made.
+
+    using namespace std::chrono_literals;
+
+    MapTest test;
+    FakeFileSource fileSource;
+
+    Map map(test.view, fileSource, MapMode::Still);
+    map.setStyleURL("mapbox://styles/test");
+    EXPECT_EQ(1u, fileSource.requests.size());
+
+    Response response;
+    response.data = std::make_shared<std::string>(util::read_file("test/fixtures/api/empty.json"));
+    response.expires = util::now() - 1h;
+
+    fileSource.respond(Resource::Style, response);
+    EXPECT_EQ(1u, fileSource.requests.size());
+
+    map.addLayer(std::make_unique<style::BackgroundLayer>("bg"));
+    EXPECT_EQ(1u, fileSource.requests.size());
+
+    fileSource.respond(Resource::Style, response);
+    EXPECT_EQ(0u, fileSource.requests.size());
+    EXPECT_NE(nullptr, map.getLayer("bg"));
+}
+
+TEST(Map, StyleEarlyMutation) {
+    // An early mutation should not prevent the initial style load.
+
+    MapTest test;
+    FakeFileSource fileSource;
+
+    Map map(test.view, fileSource, MapMode::Still);
+    map.setStyleURL("mapbox://styles/test");
+    map.addLayer(std::make_unique<style::BackgroundLayer>("bg"));
+
+    Response response;
+    response.data = std::make_shared<std::string>(util::read_file("test/fixtures/api/water.json"));
+    fileSource.respond(Resource::Style, response);
+
+    EXPECT_EQ(0u, fileSource.requests.size());
+    EXPECT_NE(nullptr, map.getLayer("water"));
+}
+
 TEST(Map, AddLayer) {
     MapTest test;
 
diff --git a/test/src/mbgl/test/fake_file_source.hpp b/test/src/mbgl/test/fake_file_source.hpp
new file mode 100644
index 000000000..7ebb4cf0c
--- /dev/null
+++ b/test/src/mbgl/test/fake_file_source.hpp
@@ -0,0 +1,60 @@
+#pragma once
+
+#include <mbgl/storage/file_source.hpp>
+
+#include <list>
+
+namespace mbgl {
+
+/*
+   FakeFileSource is similar to StubFileSource, but it follows a post hoc, "push" model rather than
+   a pre hoc, "pull" model. You pass it to code that makes requests, it records what requests are
+   made, then you can examine them, make assertions about them, and respond as desired.
+
+   This is particularly useful if you want to simulate multiple responses, e.g. as part of a resource
+   revalidation flow. StubFileSource allows only a single response.
+*/
+class FakeFileSource : public FileSource {
+public:
+    class FakeFileRequest : public AsyncRequest {
+    public:
+        Resource resource;
+        Callback callback;
+
+        std::list<FakeFileRequest*>& list;
+        std::list<FakeFileRequest*>::iterator link;
+
+        FakeFileRequest(Resource resource_, Callback callback_, std::list<FakeFileRequest*>& list_)
+            : resource(std::move(resource_)),
+              callback(std::move(callback_)),
+              list(list_),
+              link((list.push_back(this), std::prev(list.end()))) {
+        }
+
+        ~FakeFileRequest() override {
+            list.erase(link);
+        }
+    };
+
+    std::unique_ptr<AsyncRequest> request(const Resource& resource, Callback callback) override {
+        return std::make_unique<FakeFileRequest>(resource, callback, requests);
+    }
+
+    bool respond(Resource::Kind kind, const Response& response) {
+        auto it = std::find_if(requests.begin(), requests.end(), [&] (FakeFileRequest* request) {
+            return request->resource.kind == kind;
+        });
+
+        if (it != requests.end()) {
+            // Copy the callback, in case calling it deallocates the AsyncRequest.
+            Callback callback_ = (*it)->callback;
+            callback_(response);
+        }
+
+        return it != requests.end();
+    }
+
+    std::list<FakeFileRequest*> requests;
+};
+
+} // namespace mbgl