From baaae99a03202641ae5b8024e57b691fe61a6688 Mon Sep 17 00:00:00 2001 From: John Firebaugh Date: Fri, 5 May 2017 16:31:03 -0700 Subject: [core, android] Factor JSON string conversions This adds a `convertJSON` template, to be used like: Error error optional foo = convertJSON(string, error); Internally, it parses the string with RapidJSON and then calls `convert(parsed, error)`. While here, rationalize GeoJSON converters and fix error handling for Tileset conversion in OfflineDownload. --- cmake/test-files.cmake | 2 +- include/mbgl/style/conversion/geojson.hpp | 14 ++++-- include/mbgl/style/conversion/source.hpp | 2 +- platform/android/src/style/conversion/geojson.hpp | 14 +----- platform/default/mbgl/storage/offline_download.cpp | 27 +++++++---- platform/node/src/node_geojson.hpp | 2 +- platform/qt/src/qmapboxgl.cpp | 2 +- platform/qt/src/qt_geojson.hpp | 11 ++--- src/mbgl/style/conversion/geojson.cpp | 31 +++++-------- src/mbgl/style/conversion/json.hpp | 28 ++++++++++++ src/mbgl/style/sources/geojson_source_impl.cpp | 35 +-------------- src/mbgl/style/tile_source_impl.cpp | 52 ++++------------------ src/mbgl/style/tile_source_impl.hpp | 2 - src/mbgl/util/mapbox.cpp | 10 +++++ src/mbgl/util/mapbox.hpp | 6 +++ test/fixtures/style_parser/tilejson.raster.json | 8 ---- test/fixtures/style_parser/tilejson.vector.json | 8 ---- test/style/tile_source.test.cpp | 37 --------------- test/util/mapbox.test.cpp | 27 +++++++++++ 19 files changed, 131 insertions(+), 187 deletions(-) create mode 100644 src/mbgl/style/conversion/json.hpp delete mode 100644 test/fixtures/style_parser/tilejson.raster.json delete mode 100644 test/fixtures/style_parser/tilejson.vector.json delete mode 100644 test/style/tile_source.test.cpp diff --git a/cmake/test-files.cmake b/cmake/test-files.cmake index 51dedb0052..6026374de6 100644 --- a/cmake/test-files.cmake +++ b/cmake/test-files.cmake @@ -59,6 +59,7 @@ set(MBGL_TEST_FILES test/src/mbgl/test/stub_file_source.hpp test/src/mbgl/test/stub_geometry_tile_feature.hpp test/src/mbgl/test/stub_layer_observer.hpp + test/src/mbgl/test/stub_render_source_observer.hpp test/src/mbgl/test/stub_style_observer.hpp test/src/mbgl/test/stub_tile_observer.hpp test/src/mbgl/test/test.cpp @@ -100,7 +101,6 @@ set(MBGL_TEST_FILES test/style/style_image.test.cpp test/style/style_layer.test.cpp test/style/style_parser.test.cpp - test/style/tile_source.test.cpp # text test/text/glyph_atlas.test.cpp diff --git a/include/mbgl/style/conversion/geojson.hpp b/include/mbgl/style/conversion/geojson.hpp index 6e6e97dbc0..0b594f066c 100644 --- a/include/mbgl/style/conversion/geojson.hpp +++ b/include/mbgl/style/conversion/geojson.hpp @@ -1,14 +1,22 @@ #pragma once #include -#include +#include namespace mbgl { namespace style { namespace conversion { -template -optional convertGeoJSON(const V& value, Error& error); +template <> +struct Converter { +public: + optional operator()(const std::string&, Error&) const; + + // This is explicitly specialized in the .cpp file for JSValue. It may also be explicitly + // specialized for SDK-specific types (e.g. mbgl::android::Value). + template + optional operator()(const V&, Error&) const; +}; } // namespace conversion } // namespace style diff --git a/include/mbgl/style/conversion/source.hpp b/include/mbgl/style/conversion/source.hpp index 2371183b1c..dc7cdc0d42 100644 --- a/include/mbgl/style/conversion/source.hpp +++ b/include/mbgl/style/conversion/source.hpp @@ -122,7 +122,7 @@ private: auto result = std::make_unique(id, *options); if (isObject(*dataValue)) { - optional geoJSON = convertGeoJSON(*dataValue, error); + optional geoJSON = convert(*dataValue, error); if (!geoJSON) { return {}; } diff --git a/platform/android/src/style/conversion/geojson.hpp b/platform/android/src/style/conversion/geojson.hpp index 32a473b092..748fe7361e 100644 --- a/platform/android/src/style/conversion/geojson.hpp +++ b/platform/android/src/style/conversion/geojson.hpp @@ -10,25 +10,15 @@ namespace style { namespace conversion { template <> -optional convertGeoJSON(const mbgl::android::Value& value, Error& error) { - +optional Converter::operator()(const mbgl::android::Value& value, Error& error) const { if(value.isNull() || !value.isString()) { error = { "no json data found" }; return {}; } - return convertGeoJSON(value.toString(), error); + return convert(value.toString(), error); } -template <> -struct Converter { - - optional operator()(const mbgl::android::Value& value, Error& error) const { - return convertGeoJSON(value, error); - } - -}; - } // namespace conversion } // namespace style } // namespace mbgl diff --git a/platform/default/mbgl/storage/offline_download.cpp b/platform/default/mbgl/storage/offline_download.cpp index cba2f70df8..901f996a4f 100644 --- a/platform/default/mbgl/storage/offline_download.cpp +++ b/platform/default/mbgl/storage/offline_download.cpp @@ -7,6 +7,8 @@ #include #include #include +#include +#include #include #include #include @@ -87,9 +89,12 @@ OfflineRegionStatus OfflineDownload::getStatus() const { const std::string& url = urlOrTileset.get(); optional sourceResponse = offlineDatabase.get(Resource::source(url)); if (sourceResponse) { - result.requiredResourceCount += - definition.tileCover(type, tileSize, style::TileSourceImpl::parseTileJSON( - *sourceResponse->data, url, type, tileSize).zoomRange).size(); + style::conversion::Error error; + optional tileset = style::conversion::convertJSON(*sourceResponse->data, error); + if (tileset) { + result.requiredResourceCount += + definition.tileCover(type, tileSize, (*tileset).zoomRange).size(); + } } else { result.requiredResourceCountIsPrecise = false; } @@ -152,12 +157,16 @@ void OfflineDownload::activateDownload() { requiredSourceURLs.insert(url); ensureResource(Resource::source(url), [=](Response sourceResponse) { - queueTiles(type, tileSize, style::TileSourceImpl::parseTileJSON( - *sourceResponse.data, url, type, tileSize)); - - requiredSourceURLs.erase(url); - if (requiredSourceURLs.empty()) { - status.requiredResourceCountIsPrecise = true; + style::conversion::Error error; + optional tileset = style::conversion::convertJSON(*sourceResponse.data, error); + if (tileset) { + util::mapbox::canonicalizeTileset(*tileset, url, type, tileSize); + queueTiles(type, tileSize, *tileset); + + requiredSourceURLs.erase(url); + if (requiredSourceURLs.empty()) { + status.requiredResourceCountIsPrecise = true; + } } }); } diff --git a/platform/node/src/node_geojson.hpp b/platform/node/src/node_geojson.hpp index e4b2ca47e7..9bae86b76a 100644 --- a/platform/node/src/node_geojson.hpp +++ b/platform/node/src/node_geojson.hpp @@ -5,7 +5,7 @@ namespace style { namespace conversion { template <> -optional convertGeoJSON(const v8::Local&, Error& error) { +optional Converter::operator()(const v8::Local&, Error& error) const { error = { "not implemented" }; return {}; } diff --git a/platform/qt/src/qmapboxgl.cpp b/platform/qt/src/qmapboxgl.cpp index edb8afa28a..ce7e237afb 100644 --- a/platform/qt/src/qmapboxgl.cpp +++ b/platform/qt/src/qmapboxgl.cpp @@ -1303,7 +1303,7 @@ void QMapboxGL::updateSource(const QString &id, const QVariantMap ¶ms) if (params.contains("data")) { Error error; - auto result = convertGeoJSON(params["data"], error); + auto result = convert(params["data"], error); if (result) { sourceGeoJSON->setGeoJSON(*result); } diff --git a/platform/qt/src/qt_geojson.hpp b/platform/qt/src/qt_geojson.hpp index 7d12660aec..a6958b7edc 100644 --- a/platform/qt/src/qt_geojson.hpp +++ b/platform/qt/src/qt_geojson.hpp @@ -178,25 +178,20 @@ namespace style { namespace conversion { template <> -optional convertGeoJSON(const QMapbox::Feature& feature, Error&) { - return GeoJSON { asMapboxGLFeature(feature) }; -} - -template <> -optional convertGeoJSON(const QVariant& value, Error& error) { +optional Converter::operator()(const QVariant& value, Error& error) const { #if QT_VERSION >= 0x050000 if (value.typeName() == QStringLiteral("QMapbox::Feature")) { #else if (value.typeName() == QString("QMapbox::Feature")) { #endif - return convertGeoJSON(value.value(), error); + return GeoJSON { asMapboxGLFeature(value.value()) }; } else if (value.type() != QVariant::ByteArray) { error = { "JSON data must be in QByteArray" }; return {}; } QByteArray data = value.toByteArray(); - return convertGeoJSON(std::string(data.constData(), data.size()), error); + return convert(std::string(data.constData(), data.size()), error); } } // namespace conversion diff --git a/src/mbgl/style/conversion/geojson.cpp b/src/mbgl/style/conversion/geojson.cpp index 0fba6e5b4d..8103e9014a 100644 --- a/src/mbgl/style/conversion/geojson.cpp +++ b/src/mbgl/style/conversion/geojson.cpp @@ -1,33 +1,26 @@ #include - +#include #include -#include -#include +#include +#include namespace mbgl { namespace style { namespace conversion { -template <> -optional convertGeoJSON(const std::string& string, Error& error) { - rapidjson::GenericDocument, rapidjson::CrtAllocator> d; - d.Parse(string.c_str()); - - if (d.HasParseError()) { - std::stringstream message; - message << d.GetErrorOffset() << " - " << rapidjson::GetParseError_En(d.GetParseError()); - error = { message.str() }; - return {}; - } +optional Converter::operator()(const std::string& value, Error& error) const { + return convertJSON(value, error); +} - optional geoJSON = conversion::convertGeoJSON(d, error); - if (!geoJSON) { - error = { error.message }; +template <> +optional Converter::operator()(const JSValue& value, Error& error) const { + try { + return mapbox::geojson::convert(value); + } catch (const std::exception& ex) { + error = { ex.what() }; return {}; } - - return geoJSON; } } // namespace conversion diff --git a/src/mbgl/style/conversion/json.hpp b/src/mbgl/style/conversion/json.hpp new file mode 100644 index 0000000000..0817ac09df --- /dev/null +++ b/src/mbgl/style/conversion/json.hpp @@ -0,0 +1,28 @@ +#include +#include + +#include +#include + +namespace mbgl { +namespace style { +namespace conversion { + +template +optional convertJSON(const std::string& json, Error& error, Args&&...args) { + JSDocument document; + document.Parse<0>(json.c_str()); + + if (document.HasParseError()) { + std::stringstream message; + message << document.GetErrorOffset() << " - " << rapidjson::GetParseError_En(document.GetParseError()); + error = { message.str() }; + return {}; + } + + return convert(document, error, std::forward(args)...); +} + +} // namespace conversion +} // namespace style +} // namespace mbgl diff --git a/src/mbgl/style/sources/geojson_source_impl.cpp b/src/mbgl/style/sources/geojson_source_impl.cpp index 08ed95ea3f..1a686ff9bc 100644 --- a/src/mbgl/style/sources/geojson_source_impl.cpp +++ b/src/mbgl/style/sources/geojson_source_impl.cpp @@ -1,37 +1,18 @@ #include +#include #include #include #include #include #include -#include #include #include -#include -#include #include -#include #include -#include - -#include - namespace mbgl { namespace style { -namespace conversion { - -template <> -optional convertGeoJSON(const JSValue& value, Error& error) { - try { - return mapbox::geojson::convert(value); - } catch (const std::exception& ex) { - error = { ex.what() }; - return {}; - } -} -} // namespace conversion class GeoJSONVTData : public GeoJSONData { public: @@ -129,20 +110,8 @@ void GeoJSONSource::Impl::loadDescription(FileSource& fileSource) { observer->onSourceError( base, std::make_exception_ptr(std::runtime_error("unexpectedly empty GeoJSON"))); } else { - rapidjson::GenericDocument, rapidjson::CrtAllocator> d; - d.Parse<0>(res.data->c_str()); - - if (d.HasParseError()) { - std::stringstream message; - message << d.GetErrorOffset() << " - " - << rapidjson::GetParseError_En(d.GetParseError()); - observer->onSourceError(base, - std::make_exception_ptr(std::runtime_error(message.str()))); - return; - } - conversion::Error error; - optional geoJSON = conversion::convertGeoJSON(d, error); + optional geoJSON = conversion::convertJSON(*res.data, error); if (!geoJSON) { Log::Error(Event::ParseStyle, "Failed to parse GeoJSON data: %s", error.message.c_str()); diff --git a/src/mbgl/style/tile_source_impl.cpp b/src/mbgl/style/tile_source_impl.cpp index 4623714794..d2ce3def9f 100644 --- a/src/mbgl/style/tile_source_impl.cpp +++ b/src/mbgl/style/tile_source_impl.cpp @@ -1,45 +1,13 @@ #include #include -#include +#include #include -#include #include #include -#include -#include - -#include - namespace mbgl { namespace style { -Tileset TileSourceImpl::parseTileJSON(const std::string& json, const std::string& sourceURL, SourceType type, uint16_t tileSize) { - rapidjson::GenericDocument, rapidjson::CrtAllocator> document; - document.Parse<0>(json.c_str()); - - if (document.HasParseError()) { - std::stringstream message; - message << document.GetErrorOffset() << " - " << rapidjson::GetParseError_En(document.GetParseError()); - throw std::runtime_error(message.str()); - } - - conversion::Error error; - optional result = conversion::convert(document, error); - if (!result) { - throw std::runtime_error(error.message); - } - - // TODO: Remove this hack by delivering proper URLs in the TileJSON to begin with. - if (util::mapbox::isMapboxURL(sourceURL)) { - for (auto& url : (*result).tiles) { - url = util::mapbox::canonicalizeTileURL(url, type, tileSize); - } - } - - return *result; -} - TileSourceImpl::TileSourceImpl(SourceType type_, std::string id_, Source& base_, variant urlOrTileset_, uint16_t tileSize_) @@ -70,21 +38,17 @@ void TileSourceImpl::loadDescription(FileSource& fileSource) { } else if (res.noContent) { observer->onSourceError(base, std::make_exception_ptr(std::runtime_error("unexpectedly empty TileJSON"))); } else { - Tileset newTileset; - - // Create a new copy of the Tileset object that holds the base values we've parsed - // from the stylesheet. Then merge in the values parsed from the TileJSON we retrieved - // via the URL. - try { - newTileset = parseTileJSON(*res.data, url, type, tileSize); - } catch (...) { - observer->onSourceError(base, std::current_exception()); + conversion::Error error; + optional newTileset = conversion::convertJSON(*res.data, error); + if (!newTileset) { + observer->onSourceError(base, std::make_exception_ptr(std::runtime_error(error.message))); return; } - bool attributionChanged = tileset.attribution != newTileset.attribution; + util::mapbox::canonicalizeTileset(*newTileset, url, type, tileSize); + bool attributionChanged = tileset.attribution != (*newTileset).attribution; - tileset = newTileset; + tileset = *newTileset; loaded = true; observer->onSourceLoaded(base); diff --git a/src/mbgl/style/tile_source_impl.hpp b/src/mbgl/style/tile_source_impl.hpp index 405408ff54..0e5a53add7 100644 --- a/src/mbgl/style/tile_source_impl.hpp +++ b/src/mbgl/style/tile_source_impl.hpp @@ -17,8 +17,6 @@ namespace style { */ class TileSourceImpl : public Source::Impl { public: - static Tileset parseTileJSON(const std::string& json, const std::string& sourceURL, SourceType, uint16_t tileSize); - TileSourceImpl(SourceType, std::string id, Source&, variant urlOrTileset, uint16_t tileSize); diff --git a/src/mbgl/util/mapbox.cpp b/src/mbgl/util/mapbox.cpp index c03cc2a8e7..8cbc85d492 100644 --- a/src/mbgl/util/mapbox.cpp +++ b/src/mbgl/util/mapbox.cpp @@ -2,6 +2,7 @@ #include #include #include +#include #include #include @@ -170,6 +171,15 @@ canonicalizeTileURL(const std::string& str, const SourceType type, const uint16_ return result; } +void canonicalizeTileset(Tileset& tileset, const std::string& sourceURL, SourceType type, uint16_t tileSize) { + // TODO: Remove this hack by delivering proper URLs in the TileJSON to begin with. + if (isMapboxURL(sourceURL)) { + for (auto& url : tileset.tiles) { + url = canonicalizeTileURL(url, type, tileSize); + } + } +} + const uint64_t DEFAULT_OFFLINE_TILE_COUNT_LIMIT = 6000; } // end namespace mapbox diff --git a/src/mbgl/util/mapbox.hpp b/src/mbgl/util/mapbox.hpp index 72f4e0a567..f3dfdd0b01 100644 --- a/src/mbgl/util/mapbox.hpp +++ b/src/mbgl/util/mapbox.hpp @@ -4,6 +4,9 @@ #include namespace mbgl { + +class Tileset; + namespace util { namespace mapbox { @@ -18,6 +21,9 @@ std::string normalizeTileURL(const std::string& baseURL, const std::string& url, // Return a "mapbox://tiles/..." URL (suitable for normalizeTileURL) for the given Mapbox tile URL. std::string canonicalizeTileURL(const std::string& url, SourceType, uint16_t tileSize); +// Replace URL templates with "mapbox://tiles/..." URLs (suitable for normalizeTileURL). +void canonicalizeTileset(Tileset&, const std::string& url, SourceType, uint16_t tileSize); + extern const uint64_t DEFAULT_OFFLINE_TILE_COUNT_LIMIT; } // namespace mapbox diff --git a/test/fixtures/style_parser/tilejson.raster.json b/test/fixtures/style_parser/tilejson.raster.json deleted file mode 100644 index 3fc819f292..0000000000 --- a/test/fixtures/style_parser/tilejson.raster.json +++ /dev/null @@ -1,8 +0,0 @@ -{ - "minzoom": 0, - "maxzoom": 15, - "center": [ 1, 2, 3 ], - "bounds": [ 4, 5, 6, 7 ], - "attribution": "attribution", - "tiles": [ "http://a.tiles.mapbox.com/v4/mapbox.satellite/{z}/{x}/{y}.png?access_token=key" ] -} diff --git a/test/fixtures/style_parser/tilejson.vector.json b/test/fixtures/style_parser/tilejson.vector.json deleted file mode 100644 index 9144bd502c..0000000000 --- a/test/fixtures/style_parser/tilejson.vector.json +++ /dev/null @@ -1,8 +0,0 @@ -{ - "minzoom": 0, - "maxzoom": 15, - "center": [ 1, 2, 3 ], - "bounds": [ 4, 5, 6, 7 ], - "attribution": "attribution", - "tiles": [ "http://a.tiles.mapbox.com/v4/mapbox.streets/{z}/{x}/{y}.vector.pbf?access_token=key" ] -} diff --git a/test/style/tile_source.test.cpp b/test/style/tile_source.test.cpp deleted file mode 100644 index 35d037a049..0000000000 --- a/test/style/tile_source.test.cpp +++ /dev/null @@ -1,37 +0,0 @@ -#include - -#include -#include - -using namespace mbgl; -using namespace mbgl::style; - -TEST(TileSourceImpl, ParseTileJSONRaster) { - auto result = TileSourceImpl::parseTileJSON( - util::read_file("test/fixtures/style_parser/tilejson.raster.json"), - "mapbox://mapbox.satellite", - SourceType::Raster, - 256); - - EXPECT_EQ(0, result.zoomRange.min); - EXPECT_EQ(15, result.zoomRange.max); - EXPECT_EQ("attribution", result.attribution); -#if !defined(__ANDROID__) && !defined(__APPLE__) - EXPECT_EQ("mapbox://tiles/mapbox.satellite/{z}/{x}/{y}{ratio}.webp", result.tiles[0]); -#else - EXPECT_EQ("mapbox://tiles/mapbox.satellite/{z}/{x}/{y}{ratio}.png", result.tiles[0]); -#endif -} - -TEST(TileSourceImpl, ParseTileJSONVector) { - auto result = TileSourceImpl::parseTileJSON( - util::read_file("test/fixtures/style_parser/tilejson.vector.json"), - "mapbox://mapbox.streets", - SourceType::Vector, - 256); - - EXPECT_EQ(0, result.zoomRange.min); - EXPECT_EQ(15, result.zoomRange.max); - EXPECT_EQ("attribution", result.attribution); - EXPECT_EQ("mapbox://tiles/mapbox.streets/{z}/{x}/{y}.vector.pbf", result.tiles[0]); -} diff --git a/test/util/mapbox.test.cpp b/test/util/mapbox.test.cpp index 299f0df833..ba867ccb5c 100644 --- a/test/util/mapbox.test.cpp +++ b/test/util/mapbox.test.cpp @@ -3,6 +3,7 @@ #include #include #include +#include #include using namespace mbgl; @@ -210,3 +211,29 @@ TEST(Mapbox, CanonicalURL) { "http://api.mapbox.com/v4/a.b/{z}/{x}/{y}/.", mbgl::util::mapbox::canonicalizeTileURL("http://api.mapbox.com/v4/a.b/{z}/{x}/{y}/.", SourceType::Raster, 256)); } + +TEST(Mapbox, CanonicalizeRasterTileset) { + mbgl::Tileset tileset; + tileset.tiles = { + "http://a.tiles.mapbox.com/v4/mapbox.satellite/{z}/{x}/{y}.png?access_token=key" + }; + + mbgl::util::mapbox::canonicalizeTileset(tileset, "mapbox://mapbox.satellite", SourceType::Raster, 256); + +#if !defined(__ANDROID__) && !defined(__APPLE__) + EXPECT_EQ("mapbox://tiles/mapbox.satellite/{z}/{x}/{y}{ratio}.webp", tileset.tiles[0]); +#else + EXPECT_EQ("mapbox://tiles/mapbox.satellite/{z}/{x}/{y}{ratio}.png", tileset.tiles[0]); +#endif +} + +TEST(Mapbox, CanonicalizeVectorTileset) { + mbgl::Tileset tileset; + tileset.tiles = { + "http://a.tiles.mapbox.com/v4/mapbox.streets/{z}/{x}/{y}.vector.pbf?access_token=key" + }; + + mbgl::util::mapbox::canonicalizeTileset(tileset, "mapbox://mapbox.streets", SourceType::Vector, 512); + + EXPECT_EQ("mapbox://tiles/mapbox.streets/{z}/{x}/{y}.vector.pbf", tileset.tiles[0]); +} -- cgit v1.2.1