diff --git a/include/mapnik/feature.hpp b/include/mapnik/feature.hpp index fff06a232..769ca0579 100644 --- a/include/mapnik/feature.hpp +++ b/include/mapnik/feature.hpp @@ -109,9 +109,7 @@ public: raster_() {} inline mapnik::value_integer id() const { return id_;} - inline void set_id(mapnik::value_integer id) { id_ = id;} - template inline void put(context_type::key_type const& key, T const& val) { diff --git a/plugins/input/geojson/geojson_index_featureset.cpp b/plugins/input/geojson/geojson_index_featureset.cpp index f5d4249fb..285691607 100644 --- a/plugins/input/geojson/geojson_index_featureset.cpp +++ b/plugins/input/geojson/geojson_index_featureset.cpp @@ -93,7 +93,7 @@ mapnik::feature_ptr geojson_index_featureset::next() static const mapnik::json::feature_grammar grammar(tr); using namespace boost::spirit; standard::space_type space; - mapnik::feature_ptr feature(mapnik::feature_factory::create(ctx_,1)); + mapnik::feature_ptr feature(mapnik::feature_factory::create(ctx_, feature_id_++)); if (!qi::phrase_parse(start, end, (grammar)(boost::phoenix::ref(*feature)), space) || start != end) { throw std::runtime_error("Failed to parse geojson feature"); diff --git a/plugins/input/geojson/geojson_index_featureset.hpp b/plugins/input/geojson/geojson_index_featureset.hpp index e683df28d..2cd934c25 100644 --- a/plugins/input/geojson/geojson_index_featureset.hpp +++ b/plugins/input/geojson/geojson_index_featureset.hpp @@ -56,6 +56,7 @@ private: using file_ptr = std::unique_ptr; file_ptr file_; #endif + mapnik::value_integer feature_id_ = 1; mapnik::context_ptr ctx_; std::vector positions_; std::vector::iterator itr_; diff --git a/plugins/input/geojson/geojson_memory_index_featureset.cpp b/plugins/input/geojson/geojson_memory_index_featureset.cpp index 14a1a6c14..99a79ff39 100644 --- a/plugins/input/geojson/geojson_memory_index_featureset.cpp +++ b/plugins/input/geojson/geojson_memory_index_featureset.cpp @@ -69,7 +69,7 @@ mapnik::feature_ptr geojson_memory_index_featureset::next() static const mapnik::json::feature_grammar grammar(tr); using namespace boost::spirit; standard::space_type space; - mapnik::feature_ptr feature(mapnik::feature_factory::create(ctx_,1)); + mapnik::feature_ptr feature(mapnik::feature_factory::create(ctx_, feature_id_++)); if (!qi::phrase_parse(start, end, (grammar)(boost::phoenix::ref(*feature)), space) || start != end) { throw std::runtime_error("Failed to parse geojson feature"); diff --git a/plugins/input/geojson/geojson_memory_index_featureset.hpp b/plugins/input/geojson/geojson_memory_index_featureset.hpp index 154460c58..a29751d67 100644 --- a/plugins/input/geojson/geojson_memory_index_featureset.hpp +++ b/plugins/input/geojson/geojson_memory_index_featureset.hpp @@ -42,7 +42,7 @@ public: private: file_ptr file_; - + mapnik::value_integer feature_id_ = 1; const array_type index_array_; array_type::const_iterator index_itr_; array_type::const_iterator index_end_; diff --git a/plugins/input/shape/shape_featureset.cpp b/plugins/input/shape/shape_featureset.cpp index 4cf9dbf0e..44f58ce84 100644 --- a/plugins/input/shape/shape_featureset.cpp +++ b/plugins/input/shape/shape_featureset.cpp @@ -69,7 +69,8 @@ feature_ptr shape_featureset::next() { int offset = shape_.shx().read_xdr_integer(); int record_length = shape_.shx().read_xdr_integer(); - shape_.move_to(2*offset); + shape_.move_to(2 * offset); + mapnik::value_integer feature_id = shape_.id(); assert(record_length == shape_.reclength_); shape_file::record_type record(record_length * 2); shape_.shp().read_record(record); @@ -78,7 +79,7 @@ feature_ptr shape_featureset::next() // skip null shapes if (type == shape_io::shape_null) continue; - feature_ptr feature(feature_factory::create(ctx_, shape_.id_)); + feature_ptr feature(feature_factory::create(ctx_, feature_id)); switch (type) { case shape_io::shape_point: @@ -133,8 +134,6 @@ feature_ptr shape_featureset::next() return feature_ptr(); } - // FIXME: https://github.com/mapnik/mapnik/issues/1020 - feature->set_id(shape_.id_); if (attr_ids_.size()) { shape_.dbf().move_to(shape_.id_); diff --git a/plugins/input/shape/shape_index_featureset.cpp b/plugins/input/shape/shape_index_featureset.cpp index 32f261979..e352efd48 100644 --- a/plugins/input/shape/shape_index_featureset.cpp +++ b/plugins/input/shape/shape_index_featureset.cpp @@ -85,10 +85,11 @@ feature_ptr shape_index_featureset::next() while ( itr_ != offsets_.end()) { shape_ptr_->move_to(*itr_++); + mapnik::value_integer feature_id = shape_ptr_->id(); shape_file::record_type record(shape_ptr_->reclength_ * 2); shape_ptr_->shp().read_record(record); int type = record.read_ndr_integer(); - feature_ptr feature(feature_factory::create(ctx_,shape_ptr_->id_)); + feature_ptr feature(feature_factory::create(ctx_, feature_id)); switch (type) { @@ -141,18 +142,14 @@ feature_ptr shape_index_featureset::next() return feature_ptr(); } - // FIXME: https://github.com/mapnik/mapnik/issues/1020 - feature->set_id(shape_ptr_->id_); if (attr_ids_.size()) { shape_ptr_->dbf().move_to(shape_ptr_->id_); - std::vector::const_iterator itr = attr_ids_.begin(); - std::vector::const_iterator end = attr_ids_.end(); try { - for (; itr!=end; ++itr) + for (auto id : attr_ids_) { - shape_ptr_->dbf().add_attribute(*itr, *tr_, *feature); + shape_ptr_->dbf().add_attribute(id, *tr_, *feature); } } catch (...) diff --git a/plugins/input/shape/shape_io.hpp b/plugins/input/shape/shape_io.hpp index cc8f2584c..ab28f9999 100644 --- a/plugins/input/shape/shape_io.hpp +++ b/plugins/input/shape/shape_io.hpp @@ -75,6 +75,7 @@ public: return (index_ && index_->is_open()); } + inline int id() const { return id_;} void move_to(std::streampos pos); static void read_bbox(shape_file::record_type & record, mapnik::box2d & bbox); static mapnik::geometry::geometry read_polyline(shape_file::record_type & record); diff --git a/test/unit/datasource/geojson.cpp b/test/unit/datasource/geojson.cpp index 86c3a09a4..6fec8483d 100644 --- a/test/unit/datasource/geojson.cpp +++ b/test/unit/datasource/geojson.cpp @@ -73,20 +73,9 @@ TEST_CASE("geojson") { { SECTION("GeoJSON Point") { + for (auto cache_features : {true, false}) { - // cache features in memory - auto feature = detail::fetch_first_feature("./test/data/json/point.json", true); - // test - auto const& geometry = feature->get_geometry(); - REQUIRE(mapnik::geometry::geometry_type(geometry) == mapnik::geometry::Point); - auto const& pt = mapnik::util::get >(geometry); - REQUIRE(pt.x == 100); - REQUIRE(pt.y == 0); - } - { - // on-fly in-memory r-tree index + read features from disk - auto feature = detail::fetch_first_feature("./test/data/json/point.json", false); - // test + auto feature = detail::fetch_first_feature("./test/data/json/point.json", cache_features); auto const& geometry = feature->get_geometry(); REQUIRE(mapnik::geometry::geometry_type(geometry) == mapnik::geometry::Point); auto const& pt = mapnik::util::get >(geometry); @@ -97,14 +86,9 @@ TEST_CASE("geojson") { SECTION("GeoJSON LineString") { - mapnik::parameters params; - params["type"] = "geojson"; - params["file"] = "./test/data/json/linestring.json"; - + for (auto cache_features : {true, false}) { - // cache features in memory - auto feature = detail::fetch_first_feature("./test/data/json/linestring.json", true); - // test + auto feature = detail::fetch_first_feature("./test/data/json/linestring.json", cache_features); auto const& geometry = feature->get_geometry(); REQUIRE(mapnik::geometry::geometry_type(geometry) == mapnik::geometry::LineString); auto const& line = mapnik::util::get >(geometry); @@ -112,28 +96,13 @@ TEST_CASE("geojson") { REQUIRE(mapnik::geometry::envelope(line) == mapnik::box2d(100,0,101,1)); } - { - // on-fly in-memory r-tree index + read features from disk - auto feature = detail::fetch_first_feature("./test/data/json/linestring.json", false); - // test - auto const& geometry = feature->get_geometry(); - REQUIRE(mapnik::geometry::geometry_type(geometry) == mapnik::geometry::LineString); - auto const& line = mapnik::util::get >(geometry); - REQUIRE(line.size() == 2); - REQUIRE(mapnik::geometry::envelope(line) == mapnik::box2d(100,0,101,1)); - } } SECTION("GeoJSON Polygon") { - mapnik::parameters params; - params["type"] = "geojson"; - params["file"] = "./test/data/json/polygon.json"; - + for (auto cache_features : {true, false}) { - // cache features in memory - auto feature = detail::fetch_first_feature("./test/data/json/polygon.json", true); - // test + auto feature = detail::fetch_first_feature("./test/data/json/polygon.json", cache_features); auto const& geometry = feature->get_geometry(); REQUIRE(mapnik::geometry::geometry_type(geometry) == mapnik::geometry::Polygon); auto const& poly = mapnik::util::get >(geometry); @@ -144,37 +113,13 @@ TEST_CASE("geojson") { REQUIRE(mapnik::geometry::envelope(poly) == mapnik::box2d(100,0,101,1)); } - { - // on-fly in-memory r-tree index + read features from disk - auto feature = detail::fetch_first_feature("./test/data/json/polygon.json", false); - // test - auto const& geometry = feature->get_geometry(); - REQUIRE(mapnik::geometry::geometry_type(geometry) == mapnik::geometry::Polygon); - auto const& poly = mapnik::util::get >(geometry); - REQUIRE(poly.num_rings() == 2); - REQUIRE(poly.exterior_ring.size() == 5); - REQUIRE(poly.interior_rings.size() == 1); - REQUIRE(poly.interior_rings[0].size() == 5); - REQUIRE(mapnik::geometry::envelope(poly) == mapnik::box2d(100,0,101,1)); - } } SECTION("GeoJSON MultiPoint") { + for (auto cache_features : {true, false}) { - // cache features in memory - auto feature = detail::fetch_first_feature("./test/data/json/multipoint.json", true); - // test - auto const& geometry = feature->get_geometry(); - REQUIRE(mapnik::geometry::geometry_type(geometry) == mapnik::geometry::MultiPoint); - auto const& multi_pt = mapnik::util::get >(geometry); - REQUIRE(multi_pt.size() == 2); - REQUIRE(mapnik::geometry::envelope(multi_pt) == mapnik::box2d(100,0,101,1)); - } - { - // on-fly in-memory r-tree index + read features from disk - auto feature = detail::fetch_first_feature("./test/data/json/multipoint.json", false); - // test + auto feature = detail::fetch_first_feature("./test/data/json/multipoint.json", cache_features); auto const& geometry = feature->get_geometry(); REQUIRE(mapnik::geometry::geometry_type(geometry) == mapnik::geometry::MultiPoint); auto const& multi_pt = mapnik::util::get >(geometry); @@ -185,10 +130,9 @@ TEST_CASE("geojson") { SECTION("GeoJSON MultiLineString") { + for (auto cache_features : {true, false}) { - // cache features in memory - auto feature = detail::fetch_first_feature("./test/data/json/multilinestring.json", true); - // test + auto feature = detail::fetch_first_feature("./test/data/json/multilinestring.json", cache_features); auto const& geometry = feature->get_geometry(); REQUIRE(mapnik::geometry::geometry_type(geometry) == mapnik::geometry::MultiLineString); auto const& multi_line = mapnik::util::get >(geometry); @@ -198,25 +142,13 @@ TEST_CASE("geojson") { REQUIRE(mapnik::geometry::envelope(multi_line) == mapnik::box2d(100,0,103,3)); } - { - // on-fly in-memory r-tree index + read features from disk - auto feature = detail::fetch_first_feature("./test/data/json/multilinestring.json", false); - // test - auto const& geometry = feature->get_geometry(); - REQUIRE(mapnik::geometry::geometry_type(geometry) == mapnik::geometry::MultiLineString); - auto const& multi_line = mapnik::util::get >(geometry); - REQUIRE(multi_line.size() == 2); - REQUIRE(multi_line[0].size() == 2); - REQUIRE(multi_line[1].size() == 2); - REQUIRE(mapnik::geometry::envelope(multi_line) == mapnik::box2d(100,0,103,3)); - } } SECTION("GeoJSON MultiPolygon") { + for (auto cache_features : {true, false}) { - // cache features in memory - auto feature = detail::fetch_first_feature("./test/data/json/multipolygon.json", true); + auto feature = detail::fetch_first_feature("./test/data/json/multipolygon.json", cache_features); // test auto const& geometry = feature->get_geometry(); REQUIRE(mapnik::geometry::geometry_type(geometry) == mapnik::geometry::MultiPolygon); @@ -227,37 +159,13 @@ TEST_CASE("geojson") { REQUIRE(mapnik::geometry::envelope(multi_poly) == mapnik::box2d(100,0,103,3)); } - { - // on-fly in-memory r-tree index + read features from disk - auto feature = detail::fetch_first_feature("./test/data/json/multipolygon.json", false); - // test - auto const& geometry = feature->get_geometry(); - REQUIRE(mapnik::geometry::geometry_type(geometry) == mapnik::geometry::MultiPolygon); - auto const& multi_poly = mapnik::util::get >(geometry); - REQUIRE(multi_poly.size() == 2); - REQUIRE(multi_poly[0].num_rings() == 1); - REQUIRE(multi_poly[1].num_rings() == 2); - REQUIRE(mapnik::geometry::envelope(multi_poly) == mapnik::box2d(100,0,103,3)); - } } SECTION("GeoJSON GeometryCollection") { + for (auto cache_features : {true, false}) { - // cache features in memory - auto feature = detail::fetch_first_feature("./test/data/json/geometrycollection.json", true); - // test - auto const& geometry = feature->get_geometry(); - REQUIRE(mapnik::geometry::geometry_type(geometry) == mapnik::geometry::GeometryCollection); - auto const& collection = mapnik::util::get >(geometry); - REQUIRE(collection.size() == 2); - REQUIRE(mapnik::geometry::geometry_type(collection[0]) == mapnik::geometry::Point); - REQUIRE(mapnik::geometry::geometry_type(collection[1]) == mapnik::geometry::LineString); - REQUIRE(mapnik::geometry::envelope(collection) == mapnik::box2d(100,0,102,1)); - } - { - // cache features in memory - auto feature = detail::fetch_first_feature("./test/data/json/geometrycollection.json", false); + auto feature = detail::fetch_first_feature("./test/data/json/geometrycollection.json", cache_features); // test auto const& geometry = feature->get_geometry(); REQUIRE(mapnik::geometry::geometry_type(geometry) == mapnik::geometry::GeometryCollection); @@ -269,16 +177,49 @@ TEST_CASE("geojson") { } } - SECTION("GeoJSON FeatureCollection *.index") + SECTION("GeoJSON Feature") + { + // Create datasource + mapnik::parameters params; + params["type"] = "geojson"; + params["file"] = "./test/data/json/feature.json"; + for (auto cache_features : {true, false}) + { + params["cache-features"] = cache_features; + auto ds = mapnik::datasource_cache::instance().create(params); + REQUIRE(bool(ds)); + auto fields = ds->get_descriptor().get_descriptors(); + mapnik::query query(ds->envelope()); + for (auto const& field : fields) + { + query.add_property_name(field.get_name()); + } + auto features = ds->features(query); + REQUIRE(features != nullptr); + auto feature = features->next(); + REQUIRE(feature != nullptr); + } + } + + SECTION("GeoJSON FeatureCollection") { std::string filename("./test/data/json/featurecollection.json"); - if (detail::create_disk_index(filename, true) == 0) + + for (auto create_index : { true, false }) { - if (mapnik::util::exists(filename + ".index")) + if (create_index) { - mapnik::parameters params; - params["type"] = "geojson"; - params["file"] = filename; + detail::create_disk_index(filename, true); + } + + mapnik::parameters params; + params["type"] = "geojson"; + params["file"] = filename; + + for (auto cache_features : {true, false}) + { + params["cache_features"] = cache_features; + auto ds = mapnik::datasource_cache::instance().create(params); auto fields = ds->get_descriptor().get_descriptors(); mapnik::query query(ds->envelope()); @@ -289,7 +230,7 @@ TEST_CASE("geojson") { auto features = ds->features(query); auto bounding_box = ds->envelope(); mapnik::box2d bbox; - std::size_t count = 0; + mapnik::value_integer count = 0; while (true) { auto feature = features->next(); @@ -297,107 +238,55 @@ TEST_CASE("geojson") { if (!bbox.valid()) bbox = feature->envelope(); else bbox.expand_to_include(feature->envelope()); ++count; + REQUIRE(feature->id() == count); } REQUIRE(count == 3); REQUIRE(bounding_box == bbox); + } + if (mapnik::util::exists(filename + ".index")) + { CHECK(mapnik::util::remove(filename + ".index")); } } } - SECTION("json feature cache-feature=\"true\"") - { - // Create datasource - mapnik::parameters params; - params["type"] = "geojson"; - params["file"] = "./test/data/json/feature.json"; - params["cache-features"] = true; - auto ds = mapnik::datasource_cache::instance().create(params); - REQUIRE(bool(ds)); - auto fields = ds->get_descriptor().get_descriptors(); - mapnik::query query(ds->envelope()); - for (auto const& field : fields) - { - query.add_property_name(field.get_name()); - } - auto features = ds->features(query); - REQUIRE(features != nullptr); - auto feature = features->next(); - REQUIRE(feature != nullptr); - } - - SECTION("json feature cache-feature=\"false\"") - { - mapnik::parameters params; - params["type"] = "geojson"; - params["file"] = "./test/data/json/feature.json"; - params["cache-features"] = false; - auto ds = mapnik::datasource_cache::instance().create(params); - REQUIRE(bool(ds)); - auto fields = ds->get_descriptor().get_descriptors(); - mapnik::query query(ds->envelope()); - for (auto const& field : fields) - { - query.add_property_name(field.get_name()); - } - auto features = ds->features(query); - REQUIRE(features != nullptr); - auto feature = features->next(); - REQUIRE(feature != nullptr); - } - - SECTION("json extra properties cache-feature=\"true\"") + SECTION("GeoJSON extra properties") { // Create datasource mapnik::parameters params; params["type"] = "geojson"; params["file"] = "./test/data/json/feature_collection_extra_properties.json"; - params["cache-features"] = true; - auto ds = mapnik::datasource_cache::instance().create(params); - REQUIRE(bool(ds)); - auto fields = ds->get_descriptor().get_descriptors(); - mapnik::query query(ds->envelope()); - for (auto const& field : fields) + for (auto cache_features : {true, false}) { - query.add_property_name(field.get_name()); + params["cache-features"] = cache_features; + auto ds = mapnik::datasource_cache::instance().create(params); + REQUIRE(bool(ds)); + auto fields = ds->get_descriptor().get_descriptors(); + mapnik::query query(ds->envelope()); + for (auto const& field : fields) + { + query.add_property_name(field.get_name()); + } + auto features = ds->features(query); + REQUIRE(features != nullptr); + auto feature = features->next(); + REQUIRE(feature != nullptr); + REQUIRE(feature->envelope() == mapnik::box2d(123,456,123,456)); } - auto features = ds->features(query); - REQUIRE(features != nullptr); - auto feature = features->next(); - REQUIRE(feature != nullptr); - REQUIRE(feature->envelope() == mapnik::box2d(123,456,123,456)); } - SECTION("json extra properties cache-feature=\"false\"") - { - // Create datasource - mapnik::parameters params; - params["type"] = "geojson"; - params["file"] = "./test/data/json/feature_collection_extra_properties.json"; - params["cache-features"] = false; - auto ds = mapnik::datasource_cache::instance().create(params); - REQUIRE(bool(ds)); - auto fields = ds->get_descriptor().get_descriptors(); - mapnik::query query(ds->envelope()); - for (auto const& field : fields) - { - query.add_property_name(field.get_name()); - } - auto features = ds->features(query); - REQUIRE(features != nullptr); - auto feature = features->next(); - REQUIRE(feature != nullptr); - REQUIRE(feature->envelope() == mapnik::box2d(123,456,123,456)); - } - SECTION("json - ensure input fully consumed and throw exception otherwise") + SECTION("GeoJSON ensure input fully consumed and throw exception otherwise") { mapnik::parameters params; params["type"] = "geojson"; params["file"] = "./test/data/json/points-malformed.geojson"; // mismatched parentheses - params["cache-features"] = false; - REQUIRE_THROWS(mapnik::datasource_cache::instance().create(params)); - params["cache-features"] = true; - REQUIRE_THROWS(mapnik::datasource_cache::instance().create(params)); + for (auto cache_features : {true, false}) + { + params["cache-features"] = cache_features; + REQUIRE_THROWS(mapnik::datasource_cache::instance().create(params)); + params["cache-features"] = true; + REQUIRE_THROWS(mapnik::datasource_cache::instance().create(params)); + } } } }