From b1f6797aab453e141931af2389b8e16687e47fd6 Mon Sep 17 00:00:00 2001 From: "Daniel J. Hofmann" Date: Thu, 15 Dec 2016 15:28:54 +0100 Subject: [PATCH] Adds `generate_hints=true` for dropping hints in response, resolves #1789. Adds an `generate_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. --- CHANGELOG.md | 2 ++ docs/http.md | 11 +++++----- 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 | 15 +++++++++++++ 13 files changed, 83 insertions(+), 22 deletions(-) diff --git a/CHANGELOG.md b/CHANGELOG.md index 68a57036c..0a9552d06 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -1,5 +1,7 @@ # 5.5.1 - Changes from 5.5.0 + - API: + - Adds `generate_hints=true` (`true` by default) which lets user disable `Hint` generating in the response. Use if you don't need `Hint`s! - Bugfixes - Fix #3418 and ensure we only return bearings in the range 0-359 in API responses - Fixed a bug that could lead to emitting false instructions for staying on a roundabout diff --git a/docs/http.md b/docs/http.md index 132b454ba..e908f2940 100644 --- a/docs/http.md +++ b/docs/http.md @@ -24,11 +24,12 @@ To pass parameters to each location some options support an array like encoding: **Request options** -| Option | Values | Description | -|------------|--------------------------------------------------------|-------------------------------------------------------------------------------------------------------| -|bearings |`{bearing};{bearing}[;{bearing} ...]` |Limits the search to segments with given bearing in degrees towards true north in clockwise direction. | -|radiuses |`{radius};{radius}[;{radius} ...]` |Limits the search to given radius in meters. | -|hints |`{hint};{hint}[;{hint} ...]` |Hint from previous request to derive position in street network. | +| Option | Values | Description | +|----------------|--------------------------------------------------------|-------------------------------------------------------------------------------------------------------| +|bearings |`{bearing};{bearing}[;{bearing} ...]` |Limits the search to segments with given bearing in degrees towards true north in clockwise direction. | +|radiuses |`{radius};{radius}[;{radius} ...]` |Limits the search to given radius in meters. | +|generate\_hints |`true` (default), `false` |Adds a Hint to the response which can be used in subsequent requests, see `hints` parameter. | +|hints |`{hint};{hint}[;{hint} ...]` |Hint from previous request to derive position in street network. | Where the elements follow the following format: diff --git a/include/engine/api/base_api.hpp b/include/engine/api/base_api.hpp index 11416948d..681dbc233 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.generate_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 303f4724b..1063d6746 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 generate_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 99c956af0..d9e03a968 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 c931509bc..9016f3b60 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 d987da120..1c62cb4b7 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 727d335e5..7ed768bd8 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 8c13d2517..b65ac1970 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 8e59035e7..816fd6bea 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)] % ';'; + generate_hints_rule = + qi::lit("generate_hints=") > + qi::bool_[ph::bind(&engine::api::BaseParameters::generate_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) // + | generate_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 generate_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 8218be982..068a28bf4 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 06248ed62..4ef0b06d8 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_generating_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.generate_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 4dd817615..bbd0e5298 100644 --- a/unit_tests/server/parameters_parser.cpp +++ b/unit_tests/server/parameters_parser.cpp @@ -49,6 +49,8 @@ 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?generate_hints=notboolean"), + 23UL); 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 +298,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 generate Hints when they are explicitely disabled + auto result_11 = parseParameters("1,2;3,4?generate_hints=false"); + BOOST_CHECK(result_11); + BOOST_CHECK_EQUAL(result_11->generate_hints, false); + + auto result_12 = parseParameters("1,2;3,4?generate_hints=true"); + BOOST_CHECK(result_12); + BOOST_CHECK_EQUAL(result_12->generate_hints, true); + + auto result_13 = parseParameters("1,2;3,4"); + BOOST_CHECK(result_13); + BOOST_CHECK_EQUAL(result_13->generate_hints, true); } BOOST_AUTO_TEST_CASE(valid_table_urls)