Repository: ndsev/mapget (language: python)

Issue:
Features and references with missing ID parts are detected and reported
The mapget model implementation currently creates features from a tile without validating if the feature ID fields are set. This will lead to problems with implementing e.g. jump-to-feature down the line, where a feature reference must contain all the information to be resolved to a feature. 

To fix the issue, the mapget model should check that the ID fields are set, before adding a feature or feature reference to the feature layer.


Discussion / hints:

Fix patch:
diff --git a/.github/workflows/cmake.yaml b/.github/workflows/cmake.yaml
index 4712f93d..85ef5a8c 100644
--- a/.github/workflows/cmake.yaml
+++ b/.github/workflows/cmake.yaml
@@ -76,7 +76,7 @@ jobs:
         if: matrix.os == 'windows-latest'
         working-directory: build
         run: |
-          choco install --no-progress -y openssl
+          choco install --no-progress -y openssl --version=1.1.1.2100
           echo "cmake -DPython3_ROOT_DIR=$env:pythonLocation"
           cmake "-DPython3_ROOT_DIR=$env:pythonLocation" -DPython3_FIND_REGISTRY=LAST -DHTTPLIB_USE_ZLIB_IF_AVAILABLE=OFF -DCMAKE_BUILD_TYPE=Release ..
           cmake --build . --config Release
diff --git a/examples/cpp/http-datasource/main.cpp b/examples/cpp/http-datasource/main.cpp
index f82d2909..69fc4422 100644
--- a/examples/cpp/http-datasource/main.cpp
+++ b/examples/cpp/http-datasource/main.cpp
@@ -32,7 +32,7 @@ class MyRemoteDataSource
     void fill(TileFeatureLayer::Ptr const& tile)
     {
         // Add some ID parts that are shared by all features in the tile.
-        tile->setPrefix({{"areadId", "BestArea"}});
+        tile->setPrefix({{"areaId", "BestArea"}});
 
         // Create a feature with line geometry
         auto feature1 = tile->newFeature("Way", {{"wayId", 42}});
diff --git a/examples/cpp/local-datasource/main.cpp b/examples/cpp/local-datasource/main.cpp
index 7070d7f2..f39ded6b 100644
--- a/examples/cpp/local-datasource/main.cpp
+++ b/examples/cpp/local-datasource/main.cpp
@@ -41,7 +41,7 @@ class MyLocalDataSource : public mapget::DataSource
 
     void fill(TileFeatureLayer::Ptr const& tile) override {
         // Add some ID parts that are shared by all features in the tile.
-        tile->setPrefix({{"areadId", "BestArea"}});
+        tile->setPrefix({{"areaId", "BestArea"}});
 
         // Create a feature with line geometry
         auto feature1 = tile->newFeature("Way", {{"wayId", 42}});
diff --git a/libs/model/include/mapget/model/featurelayer.h b/libs/model/include/mapget/model/featurelayer.h
index 5bf76beb..8442f337 100644
--- a/libs/model/include/mapget/model/featurelayer.h
+++ b/libs/model/include/mapget/model/featurelayer.h
@@ -50,6 +50,7 @@ class TileFeatureLayer : public TileLayer, public simfil::ModelPool
      *  Each feature in this layer must have a feature type which is also present in
      *  the layer. Therefore, feature ids from this layer can be verified to conform
      *  to one of the allowed feature id compositions for the allowed type.
+     * TODO "for the allowed type" -> "for the feature type" ?
      * @param fields Shared field name dictionary, which allows compressed storage
      *  of object field name strings. It is auto-filled, and one instance may be used
      *  by multiple TileFeatureLayer instances.
@@ -86,12 +87,13 @@ class TileFeatureLayer : public TileLayer, public simfil::ModelPool
 
     /**
      * Creates a new feature and insert it into this tile layer.
-     * The unique identifying information, prepended with the featureIdPrefix,
+     * The featureIdParts (which do not include the featureIdPrefix of the layer)
      * must conform to an existing UniqueIdComposition for the feature typeId
-     * within the associated layer.
+     * within the associated layer, or a runtime error will be raised.
      * @param typeId Specifies the type of the feature.
      * @param featureIdParts Uniquely identifying information for the feature,
-     * according to the requirements of typeId.
+     * according to the requirements of typeId. Do not include the tile feature
+     * prefix. If empty, an error will be thrown.
      */
     model_ptr<Feature> newFeature(
         std::string_view const& typeId,
@@ -100,12 +102,26 @@ class TileFeatureLayer : public TileLayer, public simfil::ModelPool
     /**
      * Create a new feature id. Use this function to create a reference to another
      * feature. The created feature id will not use the common feature id prefix from
-     * this tile feature layer.
+     * this tile feature layer, since the reference may be to a feature stored in a
+     * different tile.
      */
     model_ptr<FeatureId> newFeatureId(
         std::string_view const& typeId,
         KeyValuePairs const& featureIdParts);
 
+    /**
+     * Validate that a unique id composition exists that matches this feature id,
+     * The field values must match the limitations of the IdPartDataType, and
+     * The order of values in KeyValuePairs must be the same as in the composition!
+     * @param typeId Feature type id, throws error if the type was not registered.
+     * @param featureIdParts Uniquely identifying information for the feature.
+     * @param includeTilePrefix True if the id should be evaluated with this tile's prefix prepended.
+     */
+    bool validFeatureId(
+        const std::string_view& typeId,
+        KeyValuePairs const& featureIdParts,
+        bool includeTilePrefix);
+
     /**
      * Create a new named attribute, which may be inserted into an attribute layer.
      */
diff --git a/libs/model/include/mapget/model/info.h b/libs/model/include/mapget/model/info.h
index 44e1d892..cfa3d72e 100644
--- a/libs/model/include/mapget/model/info.h
+++ b/libs/model/include/mapget/model/info.h
@@ -11,13 +11,12 @@ namespace mapget
 {
 
 /**
- * Version Definition - This is used to recognize whether
- * a stored blob of a TileFeatureLayer should be parsed by this
- * version of the mapget library. When a TileFeatureLayer is serialized,
- * the current Version value for the map layer and and the stream protocol are
- * also stored. Upon deserialization, the Major-Minor version values
- * must match the parsed version Major-Minor value, for both the for the map layer
- * and and the stream protocol.
+ * Version Definition - This is used to recognize whether a stored blob of a
+ * TileFeatureLayer should be parsed by this version of the mapget library.
+ * When a TileFeatureLayer is serialized, the current Version value for the
+ * map layer and the stream protocol are also stored. Upon deserialization,
+ * the Major-Minor version values must match the parsed version Major-Minor
+ * values, for both the map layer and the stream protocol.
  */
 struct Version {
     uint16_t major_ = 0;
@@ -68,28 +67,29 @@ NLOHMANN_JSON_SERIALIZE_ENUM(
         {LayerType::GLTF, "GLTF"},
     })
 
-/** Structure to represent a part of a unique feature id composition. */
-struct UniqueIdPart
+/** Structure to represent a part of a feature id composition. */
+struct IdPart
 {
-    /** Unique identifier part */
-    std::string partId_;
+    /** Label/identifier for this ID part.
+     * Unique under all ID parts of a feature. */
+    std::string idPartLabel_;
 
-    /** Description of the unique identifier */
+    /** Description of the identifier. */
     std::string description_;
 
-    /** Datatype of the unique identifier */
+    /** Data type of the identifier. */
     IdPartDataType datatype_;
 
     /** Is the identifier synthetic or part of a map specification? */
     bool isSynthetic_ = false;
 
-    /** Is the identifier actually optional? */
+    /** Is the identifier optional in feature queries? */
     bool isOptional_ = false;
 
-    /** Create UniqueIdPart from JSON. */
-    static UniqueIdPart fromJson(const nlohmann::json& j);
+    /** Create IdPart from JSON. */
+    static IdPart fromJson(const nlohmann::json& j);
 
-    /** Serialize UniqueIdPart to JSON. */
+    /** Serialize IdPart to JSON. */
     [[nodiscard]] nlohmann::json toJson() const;
 };
 
@@ -103,7 +103,7 @@ struct FeatureTypeInfo
      * List of allowed unique id compositions (each id composition is a list of id parts)
      * A single id composition must never have more than 16 parts.
      */
-    std::vector<std::vector<UniqueIdPart>> uniqueIdCompositions_;
+    std::vector<std::vector<IdPart>> uniqueIdCompositions_;
 
     /**
      * Deserializes a FeatureTypeInfo object from JSON.
@@ -118,6 +118,7 @@ struct FeatureTypeInfo
      *         "partId": <string>,          // Mandatory: The ID of the unique ID part.
      *         "description": <string>,     // Optional: The description of the unique ID part.
      *         "datatype": <IdPartDataType>,// Optional: The data type of the unique ID part, must be one of the enum values from IdPartDataType. Defaults to I64.
+     *         TODO why "datatype"  and not "dataType" to match other fields?
      *         "isSynthetic": <bool>,       // Optional: A flag indicating if the unique ID part is synthetic. Defaults to false.
      *         "isOptional": <bool>         // Optional: A flag indicating if the unique ID part is optional. Defaults to false.
      *       },
diff --git a/libs/model/src/featurelayer.cpp b/libs/model/src/featurelayer.cpp
index ede7e0c5..56b8ff9f 100644
--- a/libs/model/src/featurelayer.cpp
+++ b/libs/model/src/featurelayer.cpp
@@ -83,11 +83,135 @@ TileFeatureLayer::TileFeatureLayer(
 
 TileFeatureLayer::~TileFeatureLayer() = default;
 
+namespace
+{
+
+/**
+ * Check that starting from a given index, the parts of an id composition
+ * match the featureIdParts segment from start for the given length.
+ */
+bool idPartsMatchComposition(
+    std::vector<IdPart> const& candidateComposition,
+    uint32_t compositionMatchStartIdx,
+    KeyValuePairs const& featureIdParts,
+    unsigned long matchLength)
+{
+    auto featureIdIter = featureIdParts.begin();
+    auto compositionIter = candidateComposition.begin();
+
+    while (compositionMatchStartIdx > 0) {
+        ++compositionIter;
+        --compositionMatchStartIdx;
+    }
+
+    while (matchLength > 0 && compositionIter != candidateComposition.end()) {
+        // Have we exhausted feature ID parts while there's still composition parts?
+        if (featureIdIter == featureIdParts.end()) {
+            return false;
+        }
+
+        auto& [idPartKey, idPartValue] = *featureIdIter;
+
+        // Does this ID part's field name match?
+        if (compositionIter->idPartLabel_ != idPartKey) {
+            return false;
+        }
+
+        // Does the ID part's value match?
+        auto& compositionDataType = compositionIter->datatype_;
+
+        if (std::holds_alternative<int64_t>(idPartValue)) {
+            auto value = std::get<int64_t>(idPartValue);
+            switch (compositionDataType) {
+            case IdPartDataType::I32:
+                // Value must fit an I32.
+                if (value < INT32_MIN || value > INT32_MAX) {
+                    return false;
+                }
+                break;
+            case IdPartDataType::U32:
+                if (value < 0 || value > UINT32_MAX) {
+                    return false;
+                }
+                break;
+            case IdPartDataType::U64:
+                if (value < 0 || value > UINT64_MAX) {
+                    return false;
+                }
+                break;
+            default:;
+            }
+        }
+        else if (std::holds_alternative<std::string_view>(idPartValue)) {
+            auto value = std::get<std::string_view>(idPartValue);
+            // UUID128 should be a 128 bit sequence.
+            if (compositionDataType == IdPartDataType::UUID128 && value.size() != 16) {
+                return false;
+            }
+        }
+        else {
+            throw logRuntimeError("Id part data type not supported!");
+        }
+
+        ++featureIdIter;
+        ++compositionIter;
+        --matchLength;
+    }
+
+    // Match means we either checked the required length, or all the values.
+    return matchLength == 0;
+}
+
+}  // namespace
+
+bool TileFeatureLayer::validFeatureId(
+    const std::string_view& typeId,
+    KeyValuePairs const& featureIdParts,
+    bool includeTilePrefix) {
+
+    auto typesIterator = this->layerInfo_->featureTypes_.begin();
+    while (typesIterator != this->layerInfo_->featureTypes_.end()) {
+        auto& type = *typesIterator;
+        if (type.name_ == typeId) {
+            break;
+        }
+    }
+    if (typesIterator == this->layerInfo_->featureTypes_.end()) {
+        throw logRuntimeError(stx::format("Could not find feature type {}", typeId));
+    }
+
+    for (auto& candidateComposition : typesIterator->uniqueIdCompositions_) {
+        uint32_t compositionMatchStartIndex = 0;
+        if (includeTilePrefix && this->featureIdPrefix().has_value()) {
+            // Iterate past the prefix in the unique id composition.
+            compositionMatchStartIndex = this->featureIdPrefix().value()->size();
+        }
+
+        if (idPartsMatchComposition(
+                candidateComposition,
+                compositionMatchStartIndex,
+                featureIdParts,
+                featureIdParts.size()))
+        {
+            return true;
+        }
+    }
+
+    return false;
+}
+
 simfil::shared_model_ptr<Feature> TileFeatureLayer::newFeature(
     const std::string_view& typeId,
     const KeyValuePairs& featureIdParts)
 {
-    // TODO: Validate ID parts
+    if (featureIdParts.empty()) {
+        throw logRuntimeError("Tried to create an empty feature ID.");
+    }
+
+    if (!validFeatureId(typeId, featureIdParts, true)) {
+        throw logRuntimeError("Could not find a matching ID composition.");
+    }
+
     auto featureIdIndex = impl_->featureIds_.size();
     auto featureIdObject = newObject(featureIdParts.size());
     impl_->featureIds_.emplace_back(FeatureId::Data{
@@ -127,7 +251,10 @@ TileFeatureLayer::newFeatureId(
     const std::string_view& typeId,
     const KeyValuePairs& featureIdParts)
 {
-    // TODO: Validate ID parts
+    if (!validFeatureId(typeId, featureIdParts, false)) {
+        throw logRuntimeError("Could not find a matching ID composition.");
+    }
+
     auto featureIdObject = newObject(featureIdParts.size());
     auto featureIdIndex = impl_->featureIds_.size();
     impl_->featureIds_.emplace_back(FeatureId::Data{
@@ -288,6 +415,17 @@ simfil::ExprPtr const& TileFeatureLayer::compiledExpression(const std::string_vi
 
 void TileFeatureLayer::setPrefix(const KeyValuePairs& prefix)
 {
+    // Check that the prefix is compatible with all existing id composites.
+    for (auto& featureType : this->layerInfo_->featureTypes_) {
+        for (auto& candidateComposition : featureType.uniqueIdCompositions_) {
+            if (!idPartsMatchComposition(candidateComposition, 0, prefix, prefix.size())) {
+                throw logRuntimeError(stx::format(
+                    "Prefix not compatible with an id composite in type: {}",
+                    featureType.name_));
+            }
+        }
+    }
+
     auto idPrefix = newObject(prefix.size());
     for (auto const& [k, v] : prefix) {
         auto&& kk = k;
diff --git a/libs/model/src/info.cpp b/libs/model/src/info.cpp
index ee2420b8..cee7906e 100644
--- a/libs/model/src/info.cpp
+++ b/libs/model/src/info.cpp
@@ -71,7 +71,7 @@ nlohmann::json Version::toJson() const
     return nlohmann::json{{"major", major_}, {"minor", minor_}, {"patch", patch_}};
 }
 
-UniqueIdPart UniqueIdPart::fromJson(const nlohmann::json& j)
+IdPart IdPart::fromJson(const nlohmann::json& j)
 {
     try {
         return {
@@ -86,10 +86,10 @@ UniqueIdPart UniqueIdPart::fromJson(const nlohmann::json& j)
     }
 }
 
-nlohmann::json UniqueIdPart::toJson() const
+nlohmann::json IdPart::toJson() const
 {
     return nlohmann::json{
-        {"partId", partId_},
+        {"partId", idPartLabel_},
         {"description", description_},
         {"datatype", datatype_},
         {"isSynthetic", isSynthetic_},
@@ -99,11 +99,11 @@ nlohmann::json UniqueIdPart::toJson() const
 FeatureTypeInfo FeatureTypeInfo::fromJson(const nlohmann::json& j)
 {
     try {
-        std::vector<std::vector<UniqueIdPart>> idCompositions;
+        std::vector<std::vector<IdPart>> idCompositions;
         for (auto& item : j.at("uniqueIdCompositions")) {
-            std::vector<UniqueIdPart> idParts;
+            std::vector<IdPart> idParts;
             for (auto& idPart : item) {
-                idParts.push_back(UniqueIdPart::fromJson(idPart));
+                idParts.push_back(IdPart::fromJson(idPart));
             }
             idCompositions.push_back(idParts);
         }


Test patch:
diff --git a/test/unit/test-model.cpp b/test/unit/test-model.cpp
index 0d744296..8b4aa2f2 100644
--- a/test/unit/test-model.cpp
+++ b/test/unit/test-model.cpp
@@ -33,6 +33,50 @@ TEST_CASE("FeatureLayer", "[test.featurelayer]")
                             "description": "Globally Unique 32b integer.",
                             "datatype": "U32"
                         }
+                    ],
+                    [
+                        {
+                            "partId": "areaId",
+                            "description": "String which identifies the map area.",
+                            "datatype": "STR"
+                        },
+                        {
+                            "partId": "wayIdU32",
+                            "description": "A 32b uinteger.",
+                            "datatype": "U32"
+                        },
+                        {
+                            "partId": "wayIdU64",
+                            "description": "A 64b uinteger.",
+                            "datatype": "U64"
+                        },
+                        {
+                            "partId": "wayIdUUID128",
+                            "description": "A UUID128, must have 16 bytes.",
+                            "datatype": "UUID128"
+                        }
+                    ],
+                    [
+                        {
+                            "partId": "areaId",
+                            "description": "String which identifies the map area.",
+                            "datatype": "STR"
+                        },
+                        {
+                            "partId": "wayIdI32",
+                            "description": "A 32b integer.",
+                            "datatype": "I32"
+                        },
+                        {
+                            "partId": "wayIdI64",
+                            "description": "A 64b integer.",
+                            "datatype": "I64"
+                        },
+                        {
+                            "partId": "wayIdUUID128",
+                            "description": "A UUID128, must have 16 bytes.",
+                            "datatype": "UUID128"
+                        }
                     ]
                 ]
             }
@@ -49,6 +93,11 @@ TEST_CASE("FeatureLayer", "[test.featurelayer]")
         "Tropico",
         layerInfo,
         fieldNames);
+
+    // Test creating a feature while tile prefix is not set
+    auto feature0 = tile->newFeature("Way", {{"areaId", "MediocreArea"}, {"wayId", 24}});
+
+    // Set the tile feature id prefix
     tile->setPrefix({{"areaId", "TheBestArea"}});
 
     // Create a feature with line geometry
@@ -73,6 +122,14 @@ TEST_CASE("FeatureLayer", "[test.featurelayer]")
     attr->setDirection(Attribute::Direction::Positive);
     attr->addField("smell", "neutral");
 
+    // Add other features using different ID compositions
+    auto featureForId1 = tile->newFeature(
+        "Way",
+        {{"wayIdU32", 42}, {"wayIdU64", 84}, {"wayIdUUID128", "0123456789abcdef"}});
+    auto featureForId2 = tile->newFeature(
+        "Way",
+        {{"wayIdI32", -42}, {"wayIdI64", -84}, {"wayIdUUID128", "0123456789abcdef"}});
+
     SECTION("toGeoJSON")
     {
         constexpr auto expected = R"({"areaId":"TheBestArea","geometry":{"geometries":[{"coordinates":[[41.0,10.0,0.0],[43.0,11.0,0.0]],"type":"LineString"},{"coordinates":[[41.5,10.5,0.0]],"type":"MultiPoint"},{"coordinates":[[41.5,10.5,0.0],[41.600000001490116,10.700000002980232,0.0]],"type":"MultiPoint"},{"coordinates":[[41.5,10.5,0.0],[41.600000001490116,10.700000002980232,0.0]],"type":"LineString"},{"coordinates":[[41.5,10.5,0.0],[41.600000001490116,10.700000002980232,0.0],[41.5,10.299999997019768,0.0]],"type":"MultiPolygon"},{"coordinates":[[41.5,10.5,0.0],[41.600000001490116,10.700000002980232,0.0],[41.5,10.299999997019768,0.0],[41.80000001192093,10.900000005960464,0.0]],"type":"Polygon"}],"type":"GeometryCollection"},"id":"Way.TheBestArea.42","properties":{"layer":{"cheese":{"mozzarella":{"direction":1,"smell":"neutral"}}},"main_ingredient":"Pepper"},"type":"Feature","typeId":"Way","wayId":42})";
@@ -100,10 +157,34 @@ TEST_CASE("FeatureLayer", "[test.featurelayer]")
     SECTION("Range-based for loop")
     {
         for (auto feature : *tile) {
-            REQUIRE(feature->id()->toString() == feature1->id()->toString());
+            REQUIRE(feature->id()->toString().substr(0, 16) == "Way.TheBestArea.");
         }
     }
 
+    SECTION("Create feature ID with negative value in uint") {
+        CHECK_THROWS(tile->newFeature(
+            "Way",
+            {{"wayIdU32", -4},
+             {"wayIdU64", -2},
+             {"wayIdUUID128", "0123456789abcdef"}}));
+    }
+
+    SECTION("Create feature ID with non-16-byte UUID128") {
+        CHECK_THROWS(tile->newFeature(
+            "Way",
+            {{"wayIdU32", 4},
+             {"wayIdU64", 2},
+             {"wayIdUUID128", "not what you would expect"}}));
+    }
+
+    SECTION("Create feature ID with no matching composition") {
+        CHECK_THROWS(tile->newFeature(
+            "Way",
+            {{"wayIdI32", -4},
+             {"wayIdU64", 2},
+             {"wayIdUUID128", "0123456789abcdef"}}));
+    }
+
     SECTION("Serialization")
     {
         std::stringstream tileBytes;
@@ -134,7 +215,7 @@ TEST_CASE("FeatureLayer", "[test.featurelayer]")
 
         REQUIRE(deserializedTile->fieldNames() == tile->fieldNames());
         for (auto feature : *deserializedTile) {
-            REQUIRE(feature->id()->toString() == feature1->id()->toString());
+            REQUIRE(feature->id()->toString().substr(0, 16) == "Way.TheBestArea.");
         }
     }
 
@@ -189,9 +270,9 @@ TEST_CASE("FeatureLayer", "[test.featurelayer]")
         REQUIRE(readTiles.size() == 3);
         REQUIRE(readTiles[0]->fieldNames() == readTiles[1]->fieldNames());
         REQUIRE(readTiles[1]->fieldNames() == readTiles[2]->fieldNames());
-        REQUIRE(readTiles[0]->numRoots() == 1);
-        REQUIRE(readTiles[1]->numRoots() == 1);
-        REQUIRE(readTiles[2]->numRoots() == 2);
+        REQUIRE(readTiles[0]->numRoots() == 4);
+        REQUIRE(readTiles[1]->numRoots() == 4);
+        REQUIRE(readTiles[2]->numRoots() == 5);
     }
 }