From dc8b75b09fc3d4dd353116377beb8099b7fd3be6 Mon Sep 17 00:00:00 2001 From: "Daniel J. Hofmann" Date: Thu, 15 Dec 2016 15:28:54 +0100 Subject: [PATCH] Lets the user specify us dropping hints in response, resolves #1789. Adds an `?emit_hints=false` option which lets us skip generating and emitting hints for Waypoints. This can be used to decrease the response size when the user does not need hints anyway. We should think about making `false` the default here in v6. --- include/engine/api/base_api.hpp | 16 +++++++++----- include/engine/api/base_parameters.hpp | 3 +++ include/engine/api/json_factory.hpp | 4 ++++ include/engine/api/match_api.hpp | 4 +--- include/engine/api/route_api.hpp | 3 +-- include/engine/api/table_api.hpp | 3 +-- include/engine/api/trip_api.hpp | 4 +--- .../server/api/base_parameters_grammar.hpp | 11 +++++++++- src/engine/api/json_factory.cpp | 8 ++++++- unit_tests/library/route.cpp | 21 +++++++++++++++++++ unit_tests/server/parameters_parser.cpp | 14 +++++++++++++ 11 files changed, 74 insertions(+), 17 deletions(-) diff --git a/include/engine/api/base_api.hpp b/include/engine/api/base_api.hpp index 11416948d7e..1e0223eb7e4 100644 --- a/include/engine/api/base_api.hpp +++ b/include/engine/api/base_api.hpp @@ -44,13 +44,19 @@ class BaseAPI return waypoints; } - // FIXME gcc 4.8 doesn't support for lambdas to call protected member functions - // protected: + protected: util::json::Object MakeWaypoint(const PhantomNode &phantom) const { - return json::makeWaypoint(phantom.location, - facade.GetNameForID(phantom.name_id), - Hint{phantom, facade.GetCheckSum()}); + if (parameters.emit_hints) + { + return json::makeWaypoint(phantom.location, + facade.GetNameForID(phantom.name_id), + Hint{phantom, facade.GetCheckSum()}); + } + else + { + return json::makeWaypoint(phantom.location, facade.GetNameForID(phantom.name_id)); + } } const datafacade::BaseDataFacade &facade; diff --git a/include/engine/api/base_parameters.hpp b/include/engine/api/base_parameters.hpp index 303f4724ba8..ddbfde1b4d2 100644 --- a/include/engine/api/base_parameters.hpp +++ b/include/engine/api/base_parameters.hpp @@ -66,6 +66,9 @@ struct BaseParameters std::vector> radiuses; std::vector> bearings; + // Adds hints to response which can be included in subsequent requests, see `hints` above. + bool emit_hints = true; + // FIXME add validation for invalid bearing values bool IsValid() const { diff --git a/include/engine/api/json_factory.hpp b/include/engine/api/json_factory.hpp index 99c956af09e..d9e03a9682d 100644 --- a/include/engine/api/json_factory.hpp +++ b/include/engine/api/json_factory.hpp @@ -89,6 +89,10 @@ util::json::Object makeRoute(const guidance::Route &route, util::json::Array legs, boost::optional geometry); +// Creates a Waypoint without Hint, see the Hint overload below +util::json::Object makeWaypoint(const util::Coordinate location, std::string name); + +// Creates a Waypoint with Hint, see the overload above when Hint is not needed util::json::Object makeWaypoint(const util::Coordinate location, std::string name, const Hint &hint); diff --git a/include/engine/api/match_api.hpp b/include/engine/api/match_api.hpp index c931509bc83..9016f3b60a9 100644 --- a/include/engine/api/match_api.hpp +++ b/include/engine/api/match_api.hpp @@ -48,9 +48,7 @@ class MatchAPI final : public RouteAPI response.values["code"] = "Ok"; } - // FIXME gcc 4.8 doesn't support for lambdas to call protected member functions - // protected: - + protected: // FIXME this logic is a little backwards. We should change the output format of the // map_matching // routing algorithm to be easier to consume here. diff --git a/include/engine/api/route_api.hpp b/include/engine/api/route_api.hpp index d987da12017..1c62cb4b7ff 100644 --- a/include/engine/api/route_api.hpp +++ b/include/engine/api/route_api.hpp @@ -61,8 +61,7 @@ class RouteAPI : public BaseAPI response.values["code"] = "Ok"; } - // FIXME gcc 4.8 doesn't support for lambdas to call protected member functions - // protected: + protected: template util::json::Value MakeGeometry(ForwardIter begin, ForwardIter end) const { diff --git a/include/engine/api/table_api.hpp b/include/engine/api/table_api.hpp index 727d335e521..7ed768bd841 100644 --- a/include/engine/api/table_api.hpp +++ b/include/engine/api/table_api.hpp @@ -70,8 +70,7 @@ class TableAPI final : public BaseAPI response.values["code"] = "Ok"; } - // FIXME gcc 4.8 doesn't support for lambdas to call protected member functions - // protected: + protected: virtual util::json::Array MakeWaypoints(const std::vector &phantoms) const { util::json::Array json_waypoints; diff --git a/include/engine/api/trip_api.hpp b/include/engine/api/trip_api.hpp index 8c13d2517a1..b65ac1970d5 100644 --- a/include/engine/api/trip_api.hpp +++ b/include/engine/api/trip_api.hpp @@ -47,9 +47,7 @@ class TripAPI final : public RouteAPI response.values["code"] = "Ok"; } - // FIXME gcc 4.8 doesn't support for lambdas to call protected member functions - // protected: - + protected: // FIXME this logic is a little backwards. We should change the output format of the // trip plugin routing algorithm to be easier to consume here. util::json::Array MakeWaypoints(const std::vector> &sub_trips, diff --git a/include/server/api/base_parameters_grammar.hpp b/include/server/api/base_parameters_grammar.hpp index 8e59035e783..3f75f040bda 100644 --- a/include/server/api/base_parameters_grammar.hpp +++ b/include/server/api/base_parameters_grammar.hpp @@ -141,11 +141,18 @@ struct BaseParametersGrammar : boost::spirit::qi::grammar add_hint, qi::_r1, qi::_1)] % ';'; + emit_hints_rule = + qi::lit("emit_hints=") > + qi::bool_[ph::bind(&engine::api::BaseParameters::emit_hints, qi::_r1) = qi::_1]; + bearings_rule = qi::lit("bearings=") > (-(qi::short_ > ',' > qi::short_))[ph::bind(add_bearing, qi::_r1, qi::_1)] % ';'; - base_rule = radiuses_rule(qi::_r1) | hints_rule(qi::_r1) | bearings_rule(qi::_r1); + base_rule = radiuses_rule(qi::_r1) // + | hints_rule(qi::_r1) // + | bearings_rule(qi::_r1) // + | emit_hints_rule(qi::_r1); } protected: @@ -157,6 +164,8 @@ struct BaseParametersGrammar : boost::spirit::qi::grammar qi::rule radiuses_rule; qi::rule hints_rule; + qi::rule emit_hints_rule; + qi::rule bearing_rule; qi::rule location_rule; qi::rule()> polyline_rule; diff --git a/src/engine/api/json_factory.cpp b/src/engine/api/json_factory.cpp index 8218be982d1..068a28bf488 100644 --- a/src/engine/api/json_factory.cpp +++ b/src/engine/api/json_factory.cpp @@ -288,11 +288,17 @@ util::json::Object makeRoute(const guidance::Route &route, return json_route; } -util::json::Object makeWaypoint(const util::Coordinate location, std::string name, const Hint &hint) +util::json::Object makeWaypoint(const util::Coordinate location, std::string name) { util::json::Object waypoint; waypoint.values["location"] = detail::coordinateToLonLat(location); waypoint.values["name"] = std::move(name); + return waypoint; +} + +util::json::Object makeWaypoint(const util::Coordinate location, std::string name, const Hint &hint) +{ + auto waypoint = makeWaypoint(location, name); waypoint.values["hint"] = hint.ToBase64(); return waypoint; } diff --git a/unit_tests/library/route.cpp b/unit_tests/library/route.cpp index 06248ed6261..0ea8cf5bcf2 100644 --- a/unit_tests/library/route.cpp +++ b/unit_tests/library/route.cpp @@ -361,4 +361,25 @@ BOOST_AUTO_TEST_CASE(test_route_response_for_locations_across_components) } } +BOOST_AUTO_TEST_CASE(test_route_user_disables_emitting_hints) +{ + const auto args = get_args(); + auto osrm = getOSRM(args.at(0)); + + using namespace osrm; + + RouteParameters params; + params.steps = true; + params.coordinates.push_back(get_dummy_location()); + params.coordinates.push_back(get_dummy_location()); + params.emit_hints = false; + + json::Object result; + const auto rc = osrm.Route(params, result); + BOOST_CHECK(rc == Status::Ok); + + for (auto waypoint : result.values["waypoints"].get().values) + BOOST_CHECK_EQUAL(waypoint.get().values.count("hint"), 0); +} + BOOST_AUTO_TEST_SUITE_END() diff --git a/unit_tests/server/parameters_parser.cpp b/unit_tests/server/parameters_parser.cpp index 4dd8176156b..fe6f8cb7471 100644 --- a/unit_tests/server/parameters_parser.cpp +++ b/unit_tests/server/parameters_parser.cpp @@ -49,6 +49,7 @@ BOOST_AUTO_TEST_CASE(invalid_route_urls) 29UL); BOOST_CHECK_EQUAL(testInvalidOptions("1,2;3,4?overview=false&hints=;;; ;"), 32UL); + BOOST_CHECK_EQUAL(testInvalidOptions("1,2;3,4?emit_hints=notboolean"), 19UL); BOOST_CHECK_EQUAL(testInvalidOptions("1,2;3,4?overview=false&geometries=foo"), 34UL); BOOST_CHECK_EQUAL(testInvalidOptions("1,2;3,4?overview=false&overview=foo"), @@ -296,6 +297,19 @@ BOOST_AUTO_TEST_CASE(valid_route_urls) CHECK_EQUAL_RANGE(reference_10.radiuses, result_10->radiuses); CHECK_EQUAL_RANGE(reference_10.coordinates, result_10->coordinates); CHECK_EQUAL_RANGE(reference_10.hints, result_10->hints); + + // Do not emit Hints when they are explicitely disabled + auto result_11 = parseParameters("1,2;3,4?emit_hints=false"); + BOOST_CHECK(result_11); + BOOST_CHECK_EQUAL(result_11->emit_hints, false); + + auto result_12 = parseParameters("1,2;3,4?emit_hints=true"); + BOOST_CHECK(result_12); + BOOST_CHECK_EQUAL(result_12->emit_hints, true); + + auto result_13 = parseParameters("1,2;3,4"); + BOOST_CHECK(result_13); + BOOST_CHECK_EQUAL(result_13->emit_hints, true); } BOOST_AUTO_TEST_CASE(valid_table_urls)