From 224d49cc9083f8ecb418a68ae0fea839bc51def6 Mon Sep 17 00:00:00 2001
From: Mikhail Pozdnyakov <mikhail.pozdnyakov@mapbox.com>
Date: Tue, 17 Sep 2019 18:51:53 +0300
Subject: [core] Check layer compatibility with source

---
 include/mbgl/style/source.hpp                      |  3 ++
 .../mbgl/style/sources/custom_geometry_source.hpp  |  1 +
 include/mbgl/style/sources/geojson_source.hpp      |  2 ++
 include/mbgl/style/sources/image_source.hpp        |  2 ++
 include/mbgl/style/sources/raster_dem_source.hpp   |  2 +-
 include/mbgl/style/sources/raster_source.hpp       |  2 ++
 include/mbgl/style/sources/vector_source.hpp       |  2 ++
 src/mbgl/annotation/annotation_source.cpp          |  7 +++-
 src/mbgl/annotation/annotation_source.hpp          |  7 ++--
 src/mbgl/style/sources/custom_geometry_source.cpp  | 18 +++++++---
 src/mbgl/style/sources/geojson_source.cpp          | 15 ++++++---
 src/mbgl/style/sources/image_source.cpp            |  9 +++--
 src/mbgl/style/sources/raster_dem_source.cpp       | 11 +++++--
 src/mbgl/style/sources/raster_source.cpp           | 15 ++++++---
 src/mbgl/style/sources/vector_source.cpp           | 17 +++++++---
 src/mbgl/style/style_impl.cpp                      | 38 ++++++++++++++--------
 test/style/style_layer.test.cpp                    | 18 ++++++++++
 17 files changed, 125 insertions(+), 44 deletions(-)

diff --git a/include/mbgl/style/source.hpp b/include/mbgl/style/source.hpp
index 2507b67fdc..c3c0609a9f 100644
--- a/include/mbgl/style/source.hpp
+++ b/include/mbgl/style/source.hpp
@@ -22,6 +22,7 @@ class RasterSource;
 class RasterDEMSource;
 class GeoJSONSource;
 class SourceObserver;
+struct LayerTypeInfo;
 
 /**
  * The runtime representation of a [source](https://www.mapbox.com/mapbox-gl-style-spec/#sources) from the Mapbox Style
@@ -74,6 +75,8 @@ public:
     virtual void loadDescription(FileSource&) = 0;
     void dumpDebugLogs() const;
 
+    virtual bool supportsLayerType(const mbgl::style::LayerTypeInfo*) const = 0;
+
     bool loaded = false;
 
     // For use in SDK bindings, which store a reference to a platform-native peer
diff --git a/include/mbgl/style/sources/custom_geometry_source.hpp b/include/mbgl/style/sources/custom_geometry_source.hpp
index a5e545f445..ff04505699 100644
--- a/include/mbgl/style/sources/custom_geometry_source.hpp
+++ b/include/mbgl/style/sources/custom_geometry_source.hpp
@@ -46,6 +46,7 @@ public:
     // Private implementation
     class Impl;
     const Impl& impl() const;
+    bool supportsLayerType(const mbgl::style::LayerTypeInfo*) const override;
     mapbox::base::WeakPtr<Source> makeWeakPtr() override {
         return weakFactory.makeWeakPtr();
     }
diff --git a/include/mbgl/style/sources/geojson_source.hpp b/include/mbgl/style/sources/geojson_source.hpp
index c99687fad6..a256ad6f15 100644
--- a/include/mbgl/style/sources/geojson_source.hpp
+++ b/include/mbgl/style/sources/geojson_source.hpp
@@ -50,6 +50,8 @@ public:
 
     void loadDescription(FileSource&) final;
 
+    bool supportsLayerType(const mbgl::style::LayerTypeInfo*) const override;
+
     mapbox::base::WeakPtr<Source> makeWeakPtr() override {
         return weakFactory.makeWeakPtr();
     }
diff --git a/include/mbgl/style/sources/image_source.hpp b/include/mbgl/style/sources/image_source.hpp
index 84faab33c9..699a3c6494 100644
--- a/include/mbgl/style/sources/image_source.hpp
+++ b/include/mbgl/style/sources/image_source.hpp
@@ -28,6 +28,8 @@ public:
 
     void loadDescription(FileSource&) final;
 
+    bool supportsLayerType(const mbgl::style::LayerTypeInfo*) const override;
+
     mapbox::base::WeakPtr<Source> makeWeakPtr() override {
         return weakFactory.makeWeakPtr();
     }
diff --git a/include/mbgl/style/sources/raster_dem_source.hpp b/include/mbgl/style/sources/raster_dem_source.hpp
index 82588613bc..42e27cd078 100644
--- a/include/mbgl/style/sources/raster_dem_source.hpp
+++ b/include/mbgl/style/sources/raster_dem_source.hpp
@@ -13,7 +13,7 @@ namespace style {
 class RasterDEMSource : public RasterSource {
 public:
     RasterDEMSource(std::string id, variant<std::string, Tileset> urlOrTileset, uint16_t tileSize);
-
+    bool supportsLayerType(const mbgl::style::LayerTypeInfo*) const override;
 };
 
 template <>
diff --git a/include/mbgl/style/sources/raster_source.hpp b/include/mbgl/style/sources/raster_source.hpp
index 1bdced8da7..00a3b788c2 100644
--- a/include/mbgl/style/sources/raster_source.hpp
+++ b/include/mbgl/style/sources/raster_source.hpp
@@ -25,6 +25,8 @@ public:
 
     void loadDescription(FileSource&) final;
 
+    bool supportsLayerType(const mbgl::style::LayerTypeInfo*) const override;
+
     mapbox::base::WeakPtr<Source> makeWeakPtr() final {
         return weakFactory.makeWeakPtr();
     }
diff --git a/include/mbgl/style/sources/vector_source.hpp b/include/mbgl/style/sources/vector_source.hpp
index 97f0a7e5a8..4165af0a61 100644
--- a/include/mbgl/style/sources/vector_source.hpp
+++ b/include/mbgl/style/sources/vector_source.hpp
@@ -24,6 +24,8 @@ public:
 
     void loadDescription(FileSource&) final;
 
+    bool supportsLayerType(const mbgl::style::LayerTypeInfo*) const override;
+
     mapbox::base::WeakPtr<Source> makeWeakPtr() override {
         return weakFactory.makeWeakPtr();
     }
diff --git a/src/mbgl/annotation/annotation_source.cpp b/src/mbgl/annotation/annotation_source.cpp
index 68f36f2d3a..7a137f1881 100644
--- a/src/mbgl/annotation/annotation_source.cpp
+++ b/src/mbgl/annotation/annotation_source.cpp
@@ -1,5 +1,6 @@
-#include <mbgl/annotation/annotation_source.hpp>
 #include <mbgl/annotation/annotation_manager.hpp>
+#include <mbgl/annotation/annotation_source.hpp>
+#include <mbgl/style/layer.hpp>
 
 namespace mbgl {
 
@@ -21,4 +22,8 @@ optional<std::string> AnnotationSource::Impl::getAttribution() const {
     return {};
 }
 
+bool AnnotationSource::supportsLayerType(const mbgl::style::LayerTypeInfo* info) const {
+    return !std::strcmp(info->type, "line") || !std::strcmp(info->type, "symbol") || !std::strcmp(info->type, "fill");
+}
+
 } // namespace mbgl
diff --git a/src/mbgl/annotation/annotation_source.hpp b/src/mbgl/annotation/annotation_source.hpp
index 018e2136ea..0379426b3e 100644
--- a/src/mbgl/annotation/annotation_source.hpp
+++ b/src/mbgl/annotation/annotation_source.hpp
@@ -12,13 +12,12 @@ public:
     class Impl;
     const Impl& impl() const;
 
+private:
+    void loadDescription(FileSource&) final;
+    bool supportsLayerType(const mbgl::style::LayerTypeInfo*) const override;
     mapbox::base::WeakPtr<Source> makeWeakPtr() override {
         return weakFactory.makeWeakPtr();
     }
-
-private:
-    void loadDescription(FileSource&) final;
-
     Mutable<Impl> mutableImpl() const;
     mapbox::base::WeakPtrFactory<Source> weakFactory {this};
 };
diff --git a/src/mbgl/style/sources/custom_geometry_source.cpp b/src/mbgl/style/sources/custom_geometry_source.cpp
index 73675c056f..5576277de8 100644
--- a/src/mbgl/style/sources/custom_geometry_source.cpp
+++ b/src/mbgl/style/sources/custom_geometry_source.cpp
@@ -1,12 +1,14 @@
-#include <mbgl/style/sources/custom_geometry_source.hpp>
-#include <mbgl/style/custom_tile_loader.hpp>
-#include <mbgl/style/sources/custom_geometry_source_impl.hpp>
-#include <mbgl/style/source_observer.hpp>
+#include <cstring>
+#include <map>
 #include <mbgl/actor/actor.hpp>
 #include <mbgl/actor/scheduler.hpp>
+#include <mbgl/style/custom_tile_loader.hpp>
+#include <mbgl/style/layer.hpp>
+#include <mbgl/style/source_observer.hpp>
+#include <mbgl/style/sources/custom_geometry_source.hpp>
+#include <mbgl/style/sources/custom_geometry_source_impl.hpp>
 #include <mbgl/tile/tile_id.hpp>
 #include <tuple>
-#include <map>
 
 namespace mbgl {
 namespace style {
@@ -29,6 +31,12 @@ void CustomGeometrySource::loadDescription(FileSource&) {
     observer->onSourceLoaded(*this);
 }
 
+bool CustomGeometrySource::supportsLayerType(const mbgl::style::LayerTypeInfo* info) const {
+    return !std::strcmp(info->type, "line") || !std::strcmp(info->type, "symbol") ||
+           !std::strcmp(info->type, "circle") || !std::strcmp(info->type, "fill") ||
+           !std::strcmp(info->type, "fill-extrusion") || !std::strcmp(info->type, "heatmap");
+}
+
 void CustomGeometrySource::setTileData(const CanonicalTileID& tileID,
                                      const GeoJSON& data) {
     loader->self().invoke(&CustomTileLoader::setTileData, tileID, data);
diff --git a/src/mbgl/style/sources/geojson_source.cpp b/src/mbgl/style/sources/geojson_source.cpp
index 72a51e212f..b1a5dd981a 100644
--- a/src/mbgl/style/sources/geojson_source.cpp
+++ b/src/mbgl/style/sources/geojson_source.cpp
@@ -1,9 +1,10 @@
+#include <mbgl/storage/file_source.hpp>
+#include <mbgl/style/conversion/geojson.hpp>
+#include <mbgl/style/conversion/json.hpp>
+#include <mbgl/style/layer.hpp>
+#include <mbgl/style/source_observer.hpp>
 #include <mbgl/style/sources/geojson_source.hpp>
 #include <mbgl/style/sources/geojson_source_impl.hpp>
-#include <mbgl/style/source_observer.hpp>
-#include <mbgl/style/conversion/json.hpp>
-#include <mbgl/style/conversion/geojson.hpp>
-#include <mbgl/storage/file_source.hpp>
 #include <mbgl/util/logging.hpp>
 
 namespace mbgl {
@@ -78,5 +79,11 @@ void GeoJSONSource::loadDescription(FileSource& fileSource) {
     });
 }
 
+bool GeoJSONSource::supportsLayerType(const mbgl::style::LayerTypeInfo* info) const {
+    return !std::strcmp(info->type, "line") || !std::strcmp(info->type, "symbol") ||
+           !std::strcmp(info->type, "circle") || !std::strcmp(info->type, "fill") ||
+           !std::strcmp(info->type, "fill-extrusion") || !std::strcmp(info->type, "heatmap");
+}
+
 } // namespace style
 } // namespace mbgl
diff --git a/src/mbgl/style/sources/image_source.cpp b/src/mbgl/style/sources/image_source.cpp
index fa268da0ef..abd01af701 100644
--- a/src/mbgl/style/sources/image_source.cpp
+++ b/src/mbgl/style/sources/image_source.cpp
@@ -1,9 +1,10 @@
+#include <mbgl/storage/file_source.hpp>
+#include <mbgl/style/layer.hpp>
+#include <mbgl/style/source_observer.hpp>
 #include <mbgl/style/sources/image_source.hpp>
 #include <mbgl/style/sources/image_source_impl.hpp>
 #include <mbgl/util/geo.hpp>
-#include <mbgl/style/source_observer.hpp>
 #include <mbgl/util/premultiply.hpp>
-#include <mbgl/storage/file_source.hpp>
 
 namespace mbgl {
 namespace style {
@@ -80,5 +81,9 @@ void ImageSource::loadDescription(FileSource& fileSource) {
     });
 }
 
+bool ImageSource::supportsLayerType(const mbgl::style::LayerTypeInfo* info) const {
+    return !std::strcmp(info->type, "raster");
+}
+
 } // namespace style
 } // namespace mbgl
diff --git a/src/mbgl/style/sources/raster_dem_source.cpp b/src/mbgl/style/sources/raster_dem_source.cpp
index bb745561b1..724a322917 100644
--- a/src/mbgl/style/sources/raster_dem_source.cpp
+++ b/src/mbgl/style/sources/raster_dem_source.cpp
@@ -1,8 +1,9 @@
-#include <mbgl/style/sources/raster_dem_source.hpp>
-#include <mbgl/style/sources/raster_source_impl.hpp>
-#include <mbgl/style/source_observer.hpp>
 #include <mbgl/style/conversion/json.hpp>
 #include <mbgl/style/conversion/tileset.hpp>
+#include <mbgl/style/layer.hpp>
+#include <mbgl/style/source_observer.hpp>
+#include <mbgl/style/sources/raster_dem_source.hpp>
+#include <mbgl/style/sources/raster_source_impl.hpp>
 #include <mbgl/util/mapbox.hpp>
 
 namespace mbgl {
@@ -12,5 +13,9 @@ RasterDEMSource::RasterDEMSource(std::string id, variant<std::string, Tileset> u
     : RasterSource(std::move(id), urlOrTileset_, tileSize, SourceType::RasterDEM){
 }
 
+bool RasterDEMSource::supportsLayerType(const mbgl::style::LayerTypeInfo* info) const {
+    return !std::strcmp(info->type, "hillshade");
+}
+
 } // namespace style
 } // namespace mbgl
diff --git a/src/mbgl/style/sources/raster_source.cpp b/src/mbgl/style/sources/raster_source.cpp
index 115887d004..c758a7bc8b 100644
--- a/src/mbgl/style/sources/raster_source.cpp
+++ b/src/mbgl/style/sources/raster_source.cpp
@@ -1,11 +1,12 @@
-#include <mbgl/style/sources/raster_source.hpp>
-#include <mbgl/style/sources/raster_source_impl.hpp>
-#include <mbgl/style/source_observer.hpp>
+#include <mbgl/storage/file_source.hpp>
 #include <mbgl/style/conversion/json.hpp>
 #include <mbgl/style/conversion/tileset.hpp>
-#include <mbgl/storage/file_source.hpp>
-#include <mbgl/util/mapbox.hpp>
+#include <mbgl/style/layer.hpp>
+#include <mbgl/style/source_observer.hpp>
+#include <mbgl/style/sources/raster_source.hpp>
+#include <mbgl/style/sources/raster_source_impl.hpp>
 #include <mbgl/util/exception.hpp>
+#include <mbgl/util/mapbox.hpp>
 
 namespace mbgl {
 namespace style {
@@ -80,5 +81,9 @@ void RasterSource::loadDescription(FileSource& fileSource) {
     });
 }
 
+bool RasterSource::supportsLayerType(const mbgl::style::LayerTypeInfo* info) const {
+    return !std::strcmp(info->type, "raster");
+}
+
 } // namespace style
 } // namespace mbgl
diff --git a/src/mbgl/style/sources/vector_source.cpp b/src/mbgl/style/sources/vector_source.cpp
index a69ff632d8..7cb89a9474 100644
--- a/src/mbgl/style/sources/vector_source.cpp
+++ b/src/mbgl/style/sources/vector_source.cpp
@@ -1,12 +1,13 @@
-#include <mbgl/style/sources/vector_source.hpp>
-#include <mbgl/style/sources/vector_source_impl.hpp>
-#include <mbgl/style/source_observer.hpp>
+#include <mbgl/storage/file_source.hpp>
 #include <mbgl/style/conversion/json.hpp>
 #include <mbgl/style/conversion/tileset.hpp>
-#include <mbgl/storage/file_source.hpp>
-#include <mbgl/util/mapbox.hpp>
+#include <mbgl/style/layer.hpp>
+#include <mbgl/style/source_observer.hpp>
+#include <mbgl/style/sources/vector_source.hpp>
+#include <mbgl/style/sources/vector_source_impl.hpp>
 #include <mbgl/util/constants.hpp>
 #include <mbgl/util/exception.hpp>
+#include <mbgl/util/mapbox.hpp>
 
 namespace mbgl {
 namespace style {
@@ -84,5 +85,11 @@ void VectorSource::loadDescription(FileSource& fileSource) {
     });
 }
 
+bool VectorSource::supportsLayerType(const mbgl::style::LayerTypeInfo* info) const {
+    return !std::strcmp(info->type, "line") || !std::strcmp(info->type, "symbol") ||
+           !std::strcmp(info->type, "circle") || !std::strcmp(info->type, "fill") ||
+           !std::strcmp(info->type, "fill-extrusion") || !std::strcmp(info->type, "heatmap");
+}
+
 } // namespace style
 } // namespace mbgl
diff --git a/src/mbgl/style/style_impl.cpp b/src/mbgl/style/style_impl.cpp
index d3298c5cac..95a39819fc 100644
--- a/src/mbgl/style/style_impl.cpp
+++ b/src/mbgl/style/style_impl.cpp
@@ -1,26 +1,27 @@
-#include <mbgl/style/style_impl.hpp>
-#include <mbgl/style/observer.hpp>
-#include <mbgl/style/source_impl.hpp>
-#include <mbgl/style/layers/symbol_layer.hpp>
-#include <mbgl/style/layers/custom_layer.hpp>
+#include <mbgl/sprite/sprite_loader.hpp>
+#include <mbgl/storage/file_source.hpp>
+#include <mbgl/storage/resource.hpp>
+#include <mbgl/storage/response.hpp>
+#include <mbgl/style/layer_impl.hpp>
 #include <mbgl/style/layers/background_layer.hpp>
-#include <mbgl/style/layers/fill_layer.hpp>
+#include <mbgl/style/layers/circle_layer.hpp>
+#include <mbgl/style/layers/custom_layer.hpp>
 #include <mbgl/style/layers/fill_extrusion_layer.hpp>
+#include <mbgl/style/layers/fill_layer.hpp>
 #include <mbgl/style/layers/heatmap_layer.hpp>
+#include <mbgl/style/layers/hillshade_layer.hpp>
 #include <mbgl/style/layers/line_layer.hpp>
-#include <mbgl/style/layers/circle_layer.hpp>
 #include <mbgl/style/layers/raster_layer.hpp>
-#include <mbgl/style/layers/hillshade_layer.hpp>
-#include <mbgl/style/layer_impl.hpp>
+#include <mbgl/style/layers/symbol_layer.hpp>
+#include <mbgl/style/observer.hpp>
 #include <mbgl/style/parser.hpp>
+#include <mbgl/style/source_impl.hpp>
+#include <mbgl/style/style_impl.hpp>
 #include <mbgl/style/transition_options.hpp>
-#include <mbgl/sprite/sprite_loader.hpp>
 #include <mbgl/util/exception.hpp>
-#include <mbgl/util/string.hpp>
 #include <mbgl/util/logging.hpp>
-#include <mbgl/storage/file_source.hpp>
-#include <mbgl/storage/resource.hpp>
-#include <mbgl/storage/response.hpp>
+#include <mbgl/util/string.hpp>
+#include <sstream>
 
 namespace mbgl {
 namespace style {
@@ -177,6 +178,15 @@ Layer* Style::Impl::getLayer(const std::string& id) const {
 
 Layer* Style::Impl::addLayer(std::unique_ptr<Layer> layer, optional<std::string> before) {
     // TODO: verify source
+    if (Source* source = sources.get(layer->getSourceID())) {
+        if (!source->supportsLayerType(layer->baseImpl->getTypeInfo())) {
+            std::ostringstream message;
+            message << "Layer '" << layer->getID() << "' is not compatible with source '" << layer->getSourceID()
+                    << "'";
+
+            throw std::runtime_error(message.str());
+        }
+    }
 
     if (layers.get(layer->getID())) {
         throw std::runtime_error(std::string{"Layer "} + layer->getID() + " already exists");
diff --git a/test/style/style_layer.test.cpp b/test/style/style_layer.test.cpp
index d6a926c631..dfced634b7 100644
--- a/test/style/style_layer.test.cpp
+++ b/test/style/style_layer.test.cpp
@@ -299,6 +299,24 @@ TEST(Layer, DuplicateLayer) {
     }
 }
 
+TEST(Layer, IncompatibleLayer) {
+    util::RunLoop loop;
+
+    // Setup style
+    StubFileSource fileSource;
+    Style::Impl style{fileSource, 1.0};
+    style.loadJSON(util::read_file("test/fixtures/resources/style-unused-sources.json"));
+
+    // Try to add duplicate
+    try {
+        style.addLayer(std::make_unique<RasterLayer>("raster", "unusedsource"));
+        FAIL() << "Should not have been allowed to add an incompatible layer to the source";
+    } catch (const std::runtime_error& e) {
+        // Expected
+        ASSERT_STREQ("Layer 'raster' is not compatible with source 'unusedsource'", e.what());
+    }
+}
+
 namespace {
 
 template<template<typename> class PropertyValueType, typename LayoutType>
-- 
cgit v1.2.1