From ebf09e3fb08b004c70e0cbe84236cfc28bc57110 Mon Sep 17 00:00:00 2001 From: Nathan Reed Date: Thu, 30 May 2024 10:11:13 -0400 Subject: [PATCH 1/4] Revert "Loose failure check for cases when duplicate keys are mapping to different feature tags (#63)" This reverts commit 2a17b719f908e5ce1f81f4846ee9086aa06b4781. --- include/mapbox/vector_tile.hpp | 50 +++++++++------------------------ test/duplicate-keys-values.mvt | Bin 100 -> 0 bytes test/unit/vector_tile.test.cpp | 26 ----------------- 3 files changed, 13 insertions(+), 63 deletions(-) delete mode 100644 test/duplicate-keys-values.mvt diff --git a/include/mapbox/vector_tile.hpp b/include/mapbox/vector_tile.hpp index 1f3d784..7c18add 100644 --- a/include/mapbox/vector_tile.hpp +++ b/include/mapbox/vector_tile.hpp @@ -40,23 +40,7 @@ class feature { feature(protozero::data_view const&, layer const&); GeomType getType() const { return type; } - /** - * Retrieve the value associated with a given key from the feature. - * - * @param key The key used to look up the corresponding value. - * @param warning A pointer to a string that may be used to record any warnings that - * occur during the lookup process. - * The caller is responsible for managing the memory of this string. - * @return The value associated with the specified key, or a null value if the key is not found. - * - * Note: If the lookup process encounters a duplicate key in the feature, the function will - * return the value in the `values` set to which the associated tag index points to, and - * will append a message to the `warning` string (if provided) to alert the caller to the - * presence of the duplicate key. - * The caller should ensure that the `warning` string is properly initialized - * and cleaned up after use. - */ - mapbox::feature::value getValue(std::string const&, std::string* warning = nullptr) const; + mapbox::feature::value getValue(std::string const&) const; properties_type getProperties() const; mapbox::feature::identifier const& getID() const; std::uint32_t getExtent() const; @@ -88,7 +72,7 @@ class layer { std::string name; std::uint32_t version; std::uint32_t extent; - std::multimap keysMap; + std::map keysMap; std::vector> keys; std::vector values; std::vector features; @@ -169,19 +153,23 @@ inline feature::feature(protozero::data_view const& feature_view, layer const& l } } -inline mapbox::feature::value feature::getValue(const std::string& key, std::string* warning ) const { - const auto key_range = layer_.keysMap.equal_range(key); - const auto key_count = std::distance(key_range.first, key_range.second) ; - if (key_count < 1) { +inline mapbox::feature::value feature::getValue(const std::string& key) const { + auto keyIter = layer_.keysMap.find(key); + if (keyIter == layer_.keysMap.end()) { return mapbox::feature::null_value; } const auto values_count = layer_.values.size(); + const auto keymap_count = layer_.keysMap.size(); auto start_itr = tags_iter.begin(); const auto end_itr = tags_iter.end(); while (start_itr != end_itr) { std::uint32_t tag_key = static_cast(*start_itr++); + if (keymap_count <= tag_key) { + throw std::runtime_error("feature referenced out of range key"); + } + if (start_itr == end_itr) { throw std::runtime_error("uneven number of feature tag ids"); } @@ -191,19 +179,7 @@ inline mapbox::feature::value feature::getValue(const std::string& key, std::str throw std::runtime_error("feature referenced out of range value"); } - bool key_found = false; - for (auto i = key_range.first; i != key_range.second; ++i) { - if (i->second == tag_key) { - key_found = true; - break; - } - } - - if (key_found) { - // Continue process with case when same keys having multiple tag ids. - if (key_count > 1 && warning) { - *warning = std::string("duplicate keys with different tag ids are found"); - } + if (tag_key == keyIter->second) { return parseValue(layer_.values[tag_val]); } } @@ -427,8 +403,8 @@ inline layer::layer(protozero::data_view const& layer_view) : { // We want to keep the keys in the order of the vector tile // https://github.com/mapbox/mapbox-gl-native/pull/5183 - auto iter = keysMap.emplace(layer_pbf.get_string(), keys.size()); - keys.emplace_back(std::reference_wrapper(iter->first)); + auto iter = keysMap.emplace(layer_pbf.get_string(), keysMap.size()); + keys.emplace_back(std::reference_wrapper(iter.first->first)); } break; case LayerType::VALUES: diff --git a/test/duplicate-keys-values.mvt b/test/duplicate-keys-values.mvt deleted file mode 100644 index 67cdb8a97e78e5dc7e2be144bf86289b811f35f6..0000000000000000000000000000000000000000 GIT binary patch literal 0 HcmV?d00001 literal 100 zcmb12;^ImvEy&4CPAo|+782*E7Gh&yU}R)sl3-Ti$ od6|W!sY>iztmXMdIVlJR9~Vz?W?p(uYJ6E@PHCz}gF*!p0AG9<$N&HU diff --git a/test/unit/vector_tile.test.cpp b/test/unit/vector_tile.test.cpp index f419b18..db32273 100644 --- a/test/unit/vector_tile.test.cpp +++ b/test/unit/vector_tile.test.cpp @@ -128,29 +128,3 @@ TEST_CASE( "Prevent underflow in case of LineTo with 0 command count" ) { REQUIRE(!geom.empty()); } } - -TEST_CASE( "Allow multiple keys mapping to different tag ids" ) { - // duplicate key 'hello' and duplicate value 'world' - std::string buffer = open_tile("test/duplicate-keys-values.mvt"); - mapbox::vector_tile::buffer tile(buffer); - auto const layer_names = tile.layerNames(); - REQUIRE(layer_names.size() == 1); - REQUIRE(layer_names[0] == "duplicates"); - auto const layer = tile.getLayer("duplicates"); - REQUIRE(layer.featureCount() == 1); - auto const feature = mapbox::vector_tile::feature(layer.getFeature(0), layer); - REQUIRE(feature.getType() == mapbox::vector_tile::GeomType::POLYGON); - - std::string error; - auto const val = feature.getValue("hello", &error); - REQUIRE(!error.empty()); - REQUIRE(error == "duplicate keys with different tag ids are found"); - REQUIRE(val.is()); - REQUIRE(val.get() == "world"); - error.clear(); - REQUIRE(error.empty()); - auto const val1 = feature.getValue("unique", &error); - REQUIRE(error.empty()); - REQUIRE(val1.is()); - REQUIRE(val1.get() == "single_value"); -} \ No newline at end of file From 5d7a4c125a679754cbd04f0d47172c47795eb973 Mon Sep 17 00:00:00 2001 From: Nathan Gass Date: Mon, 21 Jan 2019 11:53:18 +0100 Subject: [PATCH 2/4] Handle duplicate keys in getValue. --- include/mapbox/vector_tile.hpp | 12 +----------- 1 file changed, 1 insertion(+), 11 deletions(-) diff --git a/include/mapbox/vector_tile.hpp b/include/mapbox/vector_tile.hpp index 7c18add..b12c581 100644 --- a/include/mapbox/vector_tile.hpp +++ b/include/mapbox/vector_tile.hpp @@ -154,22 +154,12 @@ inline feature::feature(protozero::data_view const& feature_view, layer const& l } inline mapbox::feature::value feature::getValue(const std::string& key) const { - auto keyIter = layer_.keysMap.find(key); - if (keyIter == layer_.keysMap.end()) { - return mapbox::feature::null_value; - } - const auto values_count = layer_.values.size(); - const auto keymap_count = layer_.keysMap.size(); auto start_itr = tags_iter.begin(); const auto end_itr = tags_iter.end(); while (start_itr != end_itr) { std::uint32_t tag_key = static_cast(*start_itr++); - if (keymap_count <= tag_key) { - throw std::runtime_error("feature referenced out of range key"); - } - if (start_itr == end_itr) { throw std::runtime_error("uneven number of feature tag ids"); } @@ -179,7 +169,7 @@ inline mapbox::feature::value feature::getValue(const std::string& key) const { throw std::runtime_error("feature referenced out of range value"); } - if (tag_key == keyIter->second) { + if (layer_.keys.at(tag_key).get() == key) { return parseValue(layer_.values[tag_val]); } } From e3695b177ad45bc7615c5cd7d1a1079a10f83c36 Mon Sep 17 00:00:00 2001 From: Nathan Gass Date: Thu, 24 Jan 2019 22:46:12 +0100 Subject: [PATCH 3/4] Add test case for duplicate keys in layer. --- test/test047.mvt | 4 ++++ test/unit/vector_tile.test.cpp | 10 ++++++++++ 2 files changed, 14 insertions(+) create mode 100644 test/test047.mvt diff --git a/test/test047.mvt b/test/test047.mvt new file mode 100644 index 0000000..f82cb47 --- /dev/null +++ b/test/test047.mvt @@ -0,0 +1,4 @@ +7x +hello " 2"typetype" +park" +lake(ÿ \ No newline at end of file diff --git a/test/unit/vector_tile.test.cpp b/test/unit/vector_tile.test.cpp index db32273..dfd639c 100644 --- a/test/unit/vector_tile.test.cpp +++ b/test/unit/vector_tile.test.cpp @@ -128,3 +128,13 @@ TEST_CASE( "Prevent underflow in case of LineTo with 0 command count" ) { REQUIRE(!geom.empty()); } } + +TEST_CASE( "Handle layer with duplicate keys" ) { + std::string buffer = open_tile("test/test047.mvt"); + mapbox::vector_tile::buffer tile(buffer); + auto const layer = tile.getLayer("hello"); + auto const feature = mapbox::vector_tile::feature(layer.getFeature(0),layer); + auto opt_val = feature.getValue("type"); \ + REQUIRE(opt_val.is()); \ + REQUIRE(opt_val.get() == "lake"); \ +} From f04e4dea7839dbd32338d36487ef1be8162c5fb4 Mon Sep 17 00:00:00 2001 From: Nathan Reed Date: Thu, 30 May 2024 10:13:07 -0400 Subject: [PATCH 4/4] Bump version to 1.0.5 --- include/mapbox/vector_tile/version.hpp | 4 ++-- test/unit/tags.test.cpp | 4 ++-- 2 files changed, 4 insertions(+), 4 deletions(-) diff --git a/include/mapbox/vector_tile/version.hpp b/include/mapbox/vector_tile/version.hpp index f5f8e04..743f63b 100644 --- a/include/mapbox/vector_tile/version.hpp +++ b/include/mapbox/vector_tile/version.hpp @@ -7,10 +7,10 @@ #define VECTOR_TILE_VERSION_MINOR 0 /// The patch number -#define VECTOR_TILE_VERSION_PATCH 4 +#define VECTOR_TILE_VERSION_PATCH 5 /// The complete version number #define VECTOR_TILE_VERSION_CODE (VECTOR_TILE_VERSION_MAJOR * 10000 + VECTOR_TILE_VERSION_MINOR * 100 + VECTOR_TILE_VERSION_PATCH) /// Version number as string -#define VECTOR_TILE_VERSION_STRING "1.0.4" +#define VECTOR_TILE_VERSION_STRING "1.0.5" diff --git a/test/unit/tags.test.cpp b/test/unit/tags.test.cpp index 0ce22d2..3a7fce0 100644 --- a/test/unit/tags.test.cpp +++ b/test/unit/tags.test.cpp @@ -4,8 +4,8 @@ #include TEST_CASE( "Version constant" ) { - CHECK(std::string(VECTOR_TILE_VERSION_STRING) == std::string("1.0.4")); - CHECK(VECTOR_TILE_VERSION_CODE == 10004); + CHECK(std::string(VECTOR_TILE_VERSION_STRING) == std::string("1.0.5")); + CHECK(VECTOR_TILE_VERSION_CODE == 10005); } TEST_CASE( "Protobuf Tag Constants" ) {