From 352d63d2ce98286cd854b11e85d66440501a6fd0 Mon Sep 17 00:00:00 2001 From: Anand Thakker Date: Thu, 12 Apr 2018 16:02:48 -0400 Subject: [PATCH] Don't enforce ["zoom"] constraints for filters (#11672) * Don't enforce ["zoom"] constraints for filters Fixes #11594 * Add a couple of comments * Fix ambiguous constructor call * ParsingContext(optional) => ParsingContext(Type) --- cmake/core-files.cmake | 1 - .../conversion/data_driven_property_value.hpp | 14 +++---- include/mbgl/style/conversion/expression.hpp | 39 ------------------- .../heatmap_color_property_value.hpp | 7 +++- .../mbgl/style/conversion/property_value.hpp | 9 ++++- .../mbgl/style/expression/parsing_context.hpp | 34 +++++++++++----- platform/node/src/node_expression.cpp | 4 +- src/mbgl/style/conversion/filter.cpp | 7 +++- src/mbgl/style/expression/parsing_context.cpp | 27 +++++++++++-- test/style/expression/expression.test.cpp | 2 +- test/style/filter.test.cpp | 14 ++++++- 11 files changed, 88 insertions(+), 70 deletions(-) delete mode 100644 include/mbgl/style/conversion/expression.hpp diff --git a/cmake/core-files.cmake b/cmake/core-files.cmake index dd39395e5..085309763 100644 --- a/cmake/core-files.cmake +++ b/cmake/core-files.cmake @@ -410,7 +410,6 @@ set(MBGL_CORE_FILES include/mbgl/style/conversion/coordinate.hpp include/mbgl/style/conversion/custom_geometry_source_options.hpp include/mbgl/style/conversion/data_driven_property_value.hpp - include/mbgl/style/conversion/expression.hpp include/mbgl/style/conversion/filter.hpp include/mbgl/style/conversion/function.hpp include/mbgl/style/conversion/geojson.hpp diff --git a/include/mbgl/style/conversion/data_driven_property_value.hpp b/include/mbgl/style/conversion/data_driven_property_value.hpp index b4297816d..07ed201c9 100644 --- a/include/mbgl/style/conversion/data_driven_property_value.hpp +++ b/include/mbgl/style/conversion/data_driven_property_value.hpp @@ -4,12 +4,12 @@ #include #include #include -#include #include #include #include #include #include +#include #include @@ -22,15 +22,15 @@ template struct Converter> { optional> operator()(const Convertible& value, Error& error) const { + using namespace mbgl::style::expression; + if (isUndefined(value)) { return DataDrivenPropertyValue(); - } else if (expression::isExpression(value)) { - optional> expression = convert>( - value, - error, - valueTypeToExpressionType()); - + } else if (isExpression(value)) { + ParsingContext ctx(valueTypeToExpressionType()); + ParseResult expression = ctx.parseLayerPropertyExpression(value); if (!expression) { + error = { ctx.getCombinedErrors() }; return {}; } diff --git a/include/mbgl/style/conversion/expression.hpp b/include/mbgl/style/conversion/expression.hpp deleted file mode 100644 index c5fcf906a..000000000 --- a/include/mbgl/style/conversion/expression.hpp +++ /dev/null @@ -1,39 +0,0 @@ -#pragma once - -#include -#include -#include - -#include - -namespace mbgl { -namespace style { -namespace conversion { - -using namespace mbgl::style::expression; - -template<> struct Converter> { - optional> operator()(const Convertible& value, Error& error, type::Type expected) const { - ParsingContext ctx(optional {expected}); - ParseResult parsed = ctx.parse(value); - if (parsed) { - return std::move(*parsed); - } - std::string combinedError; - for (const ParsingError& parsingError : ctx.getErrors()) { - if (combinedError.size() > 0) { - combinedError += "\n"; - } - if (parsingError.key.size() > 0) { - combinedError += parsingError.key + ": "; - } - combinedError += parsingError.message; - } - error = { combinedError }; - return {}; - }; -}; - -} // namespace conversion -} // namespace style -} // namespace mbgl diff --git a/include/mbgl/style/conversion/heatmap_color_property_value.hpp b/include/mbgl/style/conversion/heatmap_color_property_value.hpp index e3689c524..a4710792d 100644 --- a/include/mbgl/style/conversion/heatmap_color_property_value.hpp +++ b/include/mbgl/style/conversion/heatmap_color_property_value.hpp @@ -4,11 +4,11 @@ #include #include #include -#include #include #include #include #include +#include namespace mbgl { namespace style { @@ -17,11 +17,14 @@ namespace conversion { template <> struct Converter { optional operator()(const Convertible& value, Error& error) const { + using namespace mbgl::style::expression; if (isUndefined(value)) { return HeatmapColorPropertyValue(); } else if (isExpression(value)) { - optional> expression = convert>(value, error, expression::type::Color); + ParsingContext ctx(type::Color); + ParseResult expression = ctx.parseLayerPropertyExpression(value); if (!expression) { + error = { ctx.getCombinedErrors() }; return {}; } assert(*expression); diff --git a/include/mbgl/style/conversion/property_value.hpp b/include/mbgl/style/conversion/property_value.hpp index 97117de2e..3130661f6 100644 --- a/include/mbgl/style/conversion/property_value.hpp +++ b/include/mbgl/style/conversion/property_value.hpp @@ -4,11 +4,11 @@ #include #include #include -#include #include #include #include #include +#include namespace mbgl { namespace style { @@ -17,13 +17,18 @@ namespace conversion { template struct Converter> { optional> operator()(const Convertible& value, Error& error) const { + using namespace mbgl::style::expression; + if (isUndefined(value)) { return PropertyValue(); } else if (isExpression(value)) { - optional> expression = convert>(value, error, valueTypeToExpressionType()); + ParsingContext ctx(valueTypeToExpressionType()); + ParseResult expression = ctx.parseLayerPropertyExpression(value); if (!expression) { + error = { ctx.getCombinedErrors() }; return {}; } + if (isFeatureConstant(**expression)) { return { CameraFunction(std::move(*expression)) }; } else { diff --git a/include/mbgl/style/expression/parsing_context.hpp b/include/mbgl/style/expression/parsing_context.hpp index f92a4c95e..c19974a4f 100644 --- a/include/mbgl/style/expression/parsing_context.hpp +++ b/include/mbgl/style/expression/parsing_context.hpp @@ -55,7 +55,7 @@ class ParsingContext { public: ParsingContext() : errors(std::make_shared>()) {} ParsingContext(std::string key_) : key(std::move(key_)), errors(std::make_shared>()) {} - explicit ParsingContext(optional expected_) + explicit ParsingContext(type::Type expected_) : expected(std::move(expected_)), errors(std::make_shared>()) {} @@ -67,6 +67,7 @@ public: std::string getKey() const { return key; } optional getExpected() const { return expected; } const std::vector& getErrors() const { return *errors; } + const std::string getCombinedErrors() const; enum TypeAnnotationOption { includeTypeAnnotations, @@ -74,16 +75,21 @@ public: }; /* - Parse the given style-spec JSON value into an Expression object. - Specifically, this function is responsible for determining the expression - type (either Literal, or the one named in value[0]) and dispatching to the - appropriate ParseXxxx::parse(const V&, ParsingContext) method. + Parse the given style-spec JSON value as an expression. */ - ParseResult parse(const mbgl::style::conversion::Convertible& value, - TypeAnnotationOption typeAnnotationOption = includeTypeAnnotations); + ParseResult parseExpression(const mbgl::style::conversion::Convertible& value, + TypeAnnotationOption typeAnnotationOption = includeTypeAnnotations); /* - Parse a child expression. + Parse the given style-spec JSON value as an expression intended to be used + in a layout or paint property. This entails checking additional constraints + that exist in that context but not, e.g., for filters. + */ + ParseResult parseLayerPropertyExpression(const mbgl::style::conversion::Convertible& value, + TypeAnnotationOption typeAnnotationOption = includeTypeAnnotations); + + /* + Parse a child expression. For use by individual Expression::parse() methods. */ ParseResult parse(const mbgl::style::conversion::Convertible&, std::size_t, @@ -91,7 +97,7 @@ public: TypeAnnotationOption typeAnnotationOption = includeTypeAnnotations); /* - Parse a child expression. + Parse a child expression. For use by individual Expression::parse() methods. */ ParseResult parse(const mbgl::style::conversion::Convertible&, std::size_t index, @@ -141,6 +147,16 @@ private: errors(std::move(errors_)) {} + + /* + Parse the given style-spec JSON value into an Expression object. + Specifically, this function is responsible for determining the expression + type (either Literal, or the one named in value[0]) and dispatching to the + appropriate ParseXxxx::parse(const V&, ParsingContext) method. + */ + ParseResult parse(const mbgl::style::conversion::Convertible& value, + TypeAnnotationOption typeAnnotationOption = includeTypeAnnotations); + std::string key; optional expected; std::shared_ptr scope; diff --git a/platform/node/src/node_expression.cpp b/platform/node/src/node_expression.cpp index 84515060a..27866ccbe 100644 --- a/platform/node/src/node_expression.cpp +++ b/platform/node/src/node_expression.cpp @@ -75,8 +75,8 @@ void NodeExpression::Parse(const Nan::FunctionCallbackInfo& info) { auto expr = info[0]; try { - ParsingContext ctx(expected); - ParseResult parsed = ctx.parse(mbgl::style::conversion::Convertible(expr)); + ParsingContext ctx = expected ? ParsingContext(*expected) : ParsingContext(); + ParseResult parsed = ctx.parseLayerPropertyExpression(mbgl::style::conversion::Convertible(expr)); if (parsed) { assert(ctx.getErrors().size() == 0); auto nodeExpr = new NodeExpression(std::move(*parsed)); diff --git a/src/mbgl/style/conversion/filter.cpp b/src/mbgl/style/conversion/filter.cpp index fba149da1..3c941945f 100644 --- a/src/mbgl/style/conversion/filter.cpp +++ b/src/mbgl/style/conversion/filter.cpp @@ -2,7 +2,7 @@ #include #include #include -#include +#include namespace mbgl { namespace style { @@ -236,10 +236,13 @@ optional convertCompoundFilter(const Convertible& value, Error& error) { } optional convertExpressionFilter(const Convertible& value, Error& error) { - optional> expression = convert>(value, error, expression::type::Boolean); + expression::ParsingContext ctx(expression::type::Boolean); + expression::ParseResult expression = ctx.parseExpression(value); if (!expression) { + error = { ctx.getCombinedErrors() }; return {}; } + return { ExpressionFilter { std::move(*expression) } }; } diff --git a/src/mbgl/style/expression/parsing_context.cpp b/src/mbgl/style/expression/parsing_context.cpp index 713c0b5d2..b522aeff9 100644 --- a/src/mbgl/style/expression/parsing_context.cpp +++ b/src/mbgl/style/expression/parsing_context.cpp @@ -206,8 +206,16 @@ ParseResult ParsingContext::parse(const Convertible& value, TypeAnnotationOption } } - // if this is the root expression, enforce constraints on the use ["zoom"]. - if (key.size() == 0 && !isZoomConstant(**parsed)) { + return parsed; +} + +ParseResult ParsingContext::parseExpression(const Convertible& value, TypeAnnotationOption typeAnnotationOption) { + return parse(value, typeAnnotationOption); +} + +ParseResult ParsingContext::parseLayerPropertyExpression(const Convertible& value, TypeAnnotationOption typeAnnotationOption) { + ParseResult parsed = parse(value, typeAnnotationOption); + if (parsed && !isZoomConstant(**parsed)) { optional> zoomCurve = findZoomCurve(parsed->get()); if (!zoomCurve) { error(R"("zoom" expression may only be used as input to a top-level "step" or "interpolate" expression.)"); @@ -217,10 +225,23 @@ ParseResult ParsingContext::parse(const Convertible& value, TypeAnnotationOption return ParseResult(); } } - return parsed; } +const std::string ParsingContext::getCombinedErrors() const { + std::string combinedError; + for (const ParsingError& parsingError : *errors) { + if (combinedError.size() > 0) { + combinedError += "\n"; + } + if (parsingError.key.size() > 0) { + combinedError += parsingError.key + ": "; + } + combinedError += parsingError.message; + } + return combinedError; +} + optional ParsingContext::checkType(const type::Type& t) { assert(expected); optional err = type::checkSubtype(*expected, t); diff --git a/test/style/expression/expression.test.cpp b/test/style/expression/expression.test.cpp index 694569695..fe5c261be 100644 --- a/test/style/expression/expression.test.cpp +++ b/test/style/expression/expression.test.cpp @@ -49,7 +49,7 @@ TEST_P(ExpressionEqualityTest, ExpressionEquality) { assert(!document.HasParseError()); const JSValue* expression = &document; expression::ParsingContext ctx; - expression::ParseResult parsed = ctx.parse(conversion::Convertible(expression)); + expression::ParseResult parsed = ctx.parseExpression(conversion::Convertible(expression)); if (!parsed) { error_ = ctx.getErrors().size() > 0 ? ctx.getErrors()[0].message : "failed to parse"; }; diff --git a/test/style/filter.test.cpp b/test/style/filter.test.cpp index 6f261c43e..49edcaef4 100644 --- a/test/style/filter.test.cpp +++ b/test/style/filter.test.cpp @@ -11,14 +11,19 @@ using namespace mbgl; using namespace mbgl::style; -bool filter(const char * json, const PropertyMap& featureProperties = {{}}, optional featureId = {}, FeatureType featureType = FeatureType::Point, GeometryCollection featureGeometry = {}) { +bool filter(const char * json, + const PropertyMap& featureProperties = {{}}, + optional featureId = {}, + FeatureType featureType = FeatureType::Point, + GeometryCollection featureGeometry = {}, + float zoom = 0.0f) { conversion::Error error; optional filter = conversion::convertJSON(json, error); EXPECT_TRUE(bool(filter)); EXPECT_EQ(error.message, ""); StubGeometryTileFeature feature { featureId, featureType, featureGeometry, featureProperties }; - expression::EvaluationContext context = { &feature }; + expression::EvaluationContext context = { zoom, &feature }; return (*filter)(context); } @@ -109,3 +114,8 @@ TEST(Filter, PropertyExpression) { ASSERT_TRUE(filter("[\"==\", [\"get\", \"two\"], 2]", {{"two", int64_t(2)}})); ASSERT_FALSE(filter("[\"==\", [\"get\", \"two\"], 4]", {{"two", int64_t(2)}})); } + +TEST(Filter, ZoomExpressionNested) { + ASSERT_TRUE(filter(R"(["==", ["get", "two"], ["zoom"]])", {{"two", int64_t(2)}}, {}, FeatureType::Point, {}, 2.0f)); + ASSERT_FALSE(filter(R"(["==", ["get", "two"], ["+", ["zoom"], 1]])", {{"two", int64_t(2)}}, {}, FeatureType::Point, {}, 2.0f)); +}