From c9a5c76ed4783b99707274c1b49b25b0ab422f1c Mon Sep 17 00:00:00 2001 From: whytro Date: Fri, 10 Mar 2023 04:15:07 +0900 Subject: [PATCH 01/11] Require a radius parameter when using bearings --- docs/http.md | 2 +- include/engine/api/base_parameters.hpp | 2 +- include/server/service/utils.hpp | 7 +++--- src/server/service/match_service.cpp | 15 ++++++------ src/server/service/nearest_service.cpp | 15 ++++++------ src/server/service/route_service.cpp | 11 ++++++--- src/server/service/table_service.cpp | 34 ++++++-------------------- src/server/service/trip_service.cpp | 13 +++++----- 8 files changed, 44 insertions(+), 55 deletions(-) diff --git a/docs/http.md b/docs/http.md index 07acc968a..beb59af91 100644 --- a/docs/http.md +++ b/docs/http.md @@ -31,7 +31,7 @@ To pass parameters to each location some options support an array-like encoding: | Option | Values | Description | |----------------|--------------------------------------------------------|-----------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------| -|bearings |`{bearing};{bearing}[;{bearing} ...]` |Limits the search to segments with given bearing in degrees towards true north in a clockwise direction. | +|bearings |`{bearing};{bearing}[;{bearing} ...]` |Limits the search to segments with given bearing in degrees towards true north in a clockwise direction. Requires a corresponding radius. | |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. | diff --git a/include/engine/api/base_parameters.hpp b/include/engine/api/base_parameters.hpp index 9cc13ae01..99ce58b4b 100644 --- a/include/engine/api/base_parameters.hpp +++ b/include/engine/api/base_parameters.hpp @@ -106,7 +106,7 @@ struct BaseParameters bool IsValid() const { return (hints.empty() || hints.size() == coordinates.size()) && - (bearings.empty() || bearings.size() == coordinates.size()) && + (bearings.empty() || bearings.size() == radiuses.size()) && (radiuses.empty() || radiuses.size() == coordinates.size()) && (approaches.empty() || approaches.size() == coordinates.size()) && std::all_of(bearings.begin(), diff --git a/include/server/service/utils.hpp b/include/server/service/utils.hpp index 391211d69..a9a0a73e1 100644 --- a/include/server/service/utils.hpp +++ b/include/server/service/utils.hpp @@ -4,18 +4,19 @@ namespace osrm::server::service { const constexpr char PARAMETER_SIZE_MISMATCH_MSG[] = - "Number of elements in %1% size %2% does not match coordinate size %3%"; + "Number of elements in %1% size %2% does not match %3% size %4%"; template bool constrainParamSize(const char *msg_template, - const char *name, + const char *param_name, const ParamT ¶m, + const char *target_name, const std::size_t target_size, std::string &help) { if (param.size() > 0 && param.size() != target_size) { - help = (boost::format(msg_template) % name % param.size() % target_size).str(); + help = (boost::format(msg_template) % param_name % param.size() % target_name % target_size).str(); return true; } return false; diff --git a/src/server/service/match_service.cpp b/src/server/service/match_service.cpp index 3a74ec90b..c14f8d592 100644 --- a/src/server/service/match_service.cpp +++ b/src/server/service/match_service.cpp @@ -1,13 +1,11 @@ #include "server/service/match_service.hpp" +#include "server/service/utils.hpp" #include "server/api/parameters_parser.hpp" -#include "server/service/utils.hpp" #include "engine/api/match_parameters.hpp" #include "util/json_container.hpp" -#include - namespace osrm::server::service { namespace @@ -17,16 +15,19 @@ std::string getWrongOptionHelp(const engine::api::MatchParameters ¶meters) std::string help; const auto coord_size = parameters.coordinates.size(); + const auto bearings_size = parameters.bearings.size(); const bool param_size_mismatch = constrainParamSize( - PARAMETER_SIZE_MISMATCH_MSG, "hints", parameters.hints, coord_size, help) || + PARAMETER_SIZE_MISMATCH_MSG, "hints", parameters.hints, "coordinates", coord_size, help) || constrainParamSize( - PARAMETER_SIZE_MISMATCH_MSG, "bearings", parameters.bearings, coord_size, help) || + PARAMETER_SIZE_MISMATCH_MSG, "bearings", parameters.bearings, "coordinates", coord_size, help) || constrainParamSize( - PARAMETER_SIZE_MISMATCH_MSG, "radiuses", parameters.radiuses, coord_size, help) || + PARAMETER_SIZE_MISMATCH_MSG, "radiuses", parameters.radiuses, "bearings", bearings_size, help) || constrainParamSize( - PARAMETER_SIZE_MISMATCH_MSG, "timestamps", parameters.timestamps, coord_size, help); + PARAMETER_SIZE_MISMATCH_MSG, "radiuses", parameters.radiuses, "coordinates", coord_size, help) || + constrainParamSize( + PARAMETER_SIZE_MISMATCH_MSG, "timestamps", parameters.timestamps, "coordinates", coord_size, help); if (!param_size_mismatch && parameters.coordinates.size() < 2) { diff --git a/src/server/service/nearest_service.cpp b/src/server/service/nearest_service.cpp index 28aa75d69..2f78d14af 100644 --- a/src/server/service/nearest_service.cpp +++ b/src/server/service/nearest_service.cpp @@ -6,11 +6,8 @@ #include "util/json_container.hpp" -#include - namespace osrm::server::service { - namespace { std::string getWrongOptionHelp(const engine::api::NearestParameters ¶meters) @@ -18,14 +15,18 @@ std::string getWrongOptionHelp(const engine::api::NearestParameters ¶meters) std::string help; const auto coord_size = parameters.coordinates.size(); + const auto bearings_size = parameters.bearings.size(); - constrainParamSize(PARAMETER_SIZE_MISMATCH_MSG, "hints", parameters.hints, coord_size, help); constrainParamSize( - PARAMETER_SIZE_MISMATCH_MSG, "bearings", parameters.bearings, coord_size, help); + PARAMETER_SIZE_MISMATCH_MSG, "hints", parameters.hints, "coordinates", coord_size, help); constrainParamSize( - PARAMETER_SIZE_MISMATCH_MSG, "radiuses", parameters.radiuses, coord_size, help); + PARAMETER_SIZE_MISMATCH_MSG, "bearings", parameters.bearings, "coordinates", coord_size, help); constrainParamSize( - PARAMETER_SIZE_MISMATCH_MSG, "approaches", parameters.approaches, coord_size, help); + PARAMETER_SIZE_MISMATCH_MSG, "radiuses", parameters.radiuses, "bearings", bearings_size, help); + constrainParamSize( + PARAMETER_SIZE_MISMATCH_MSG, "radiuses", parameters.radiuses, "coordinates", coord_size, help); + constrainParamSize( + PARAMETER_SIZE_MISMATCH_MSG, "approaches", parameters.approaches, "coordinates", coord_size, help); return help; } diff --git a/src/server/service/route_service.cpp b/src/server/service/route_service.cpp index f5b61e82a..41f146abf 100644 --- a/src/server/service/route_service.cpp +++ b/src/server/service/route_service.cpp @@ -15,16 +15,19 @@ std::string getWrongOptionHelp(const engine::api::RouteParameters ¶meters) std::string help; const auto coord_size = parameters.coordinates.size(); + const auto bearings_size = parameters.bearings.size(); const bool param_size_mismatch = constrainParamSize( - PARAMETER_SIZE_MISMATCH_MSG, "hints", parameters.hints, coord_size, help) || + PARAMETER_SIZE_MISMATCH_MSG, "hints", parameters.hints, "coordinates", coord_size, help) || constrainParamSize( - PARAMETER_SIZE_MISMATCH_MSG, "bearings", parameters.bearings, coord_size, help) || + PARAMETER_SIZE_MISMATCH_MSG, "bearings", parameters.bearings, "coordinates", coord_size, help) || constrainParamSize( - PARAMETER_SIZE_MISMATCH_MSG, "radiuses", parameters.radiuses, coord_size, help) || + PARAMETER_SIZE_MISMATCH_MSG, "radiuses", parameters.radiuses, "bearings", bearings_size, help) || constrainParamSize( - PARAMETER_SIZE_MISMATCH_MSG, "approaches", parameters.approaches, coord_size, help); + PARAMETER_SIZE_MISMATCH_MSG, "radiuses", parameters.radiuses, "coordinates", coord_size, help) || + constrainParamSize( + PARAMETER_SIZE_MISMATCH_MSG, "approaches", parameters.approaches, "coordinates", coord_size, help); if (!param_size_mismatch && parameters.coordinates.size() < 2) { diff --git a/src/server/service/table_service.cpp b/src/server/service/table_service.cpp index 27341f6aa..88d0804d6 100644 --- a/src/server/service/table_service.cpp +++ b/src/server/service/table_service.cpp @@ -1,51 +1,33 @@ #include "server/service/table_service.hpp" +#include "server/service/utils.hpp" #include "server/api/parameters_parser.hpp" #include "engine/api/table_parameters.hpp" #include "util/json_container.hpp" -#include - namespace osrm::server::service { - namespace { - -const constexpr char PARAMETER_SIZE_MISMATCH_MSG[] = - "Number of elements in %1% size %2% does not match coordinate size %3%"; - -template -bool constrainParamSize(const char *msg_template, - const char *name, - const ParamT ¶m, - const std::size_t target_size, - std::string &help) -{ - if (param.size() > 0 && param.size() != target_size) - { - help = (boost::format(msg_template) % name % param.size() % target_size).str(); - return true; - } - return false; -} - std::string getWrongOptionHelp(const engine::api::TableParameters ¶meters) { std::string help; const auto coord_size = parameters.coordinates.size(); + const auto bearings_size = parameters.bearings.size(); const bool param_size_mismatch = constrainParamSize( - PARAMETER_SIZE_MISMATCH_MSG, "hints", parameters.hints, coord_size, help) || + PARAMETER_SIZE_MISMATCH_MSG, "hints", parameters.hints, "coordinates", coord_size, help) || constrainParamSize( - PARAMETER_SIZE_MISMATCH_MSG, "bearings", parameters.bearings, coord_size, help) || + PARAMETER_SIZE_MISMATCH_MSG, "bearings", parameters.bearings, "coordinates", coord_size, help) || constrainParamSize( - PARAMETER_SIZE_MISMATCH_MSG, "radiuses", parameters.radiuses, coord_size, help) || + PARAMETER_SIZE_MISMATCH_MSG, "radiuses", parameters.radiuses, "bearings", bearings_size, help) || constrainParamSize( - PARAMETER_SIZE_MISMATCH_MSG, "approaches", parameters.approaches, coord_size, help); + PARAMETER_SIZE_MISMATCH_MSG, "radiuses", parameters.radiuses, "coordinates", coord_size, help) || + constrainParamSize( + PARAMETER_SIZE_MISMATCH_MSG, "approaches", parameters.approaches, "coordinates", coord_size, help); if (!param_size_mismatch && parameters.coordinates.size() < 2) { diff --git a/src/server/service/trip_service.cpp b/src/server/service/trip_service.cpp index a9e44b896..5fc35698a 100644 --- a/src/server/service/trip_service.cpp +++ b/src/server/service/trip_service.cpp @@ -6,8 +6,6 @@ #include "util/json_container.hpp" -#include - namespace osrm::server::service { namespace @@ -17,16 +15,19 @@ std::string getWrongOptionHelp(const engine::api::TripParameters ¶meters) std::string help; const auto coord_size = parameters.coordinates.size(); + const auto bearings_size = parameters.bearings.size(); const bool param_size_mismatch = constrainParamSize( - PARAMETER_SIZE_MISMATCH_MSG, "hints", parameters.hints, coord_size, help) || + PARAMETER_SIZE_MISMATCH_MSG, "hints", parameters.hints, "coordinates", coord_size, help) || constrainParamSize( - PARAMETER_SIZE_MISMATCH_MSG, "bearings", parameters.bearings, coord_size, help) || + PARAMETER_SIZE_MISMATCH_MSG, "bearings", parameters.bearings, "coordinates", coord_size, help) || constrainParamSize( - PARAMETER_SIZE_MISMATCH_MSG, "radiuses", parameters.radiuses, coord_size, help) || + PARAMETER_SIZE_MISMATCH_MSG, "radiuses", parameters.radiuses, "bearings", bearings_size, help) || constrainParamSize( - PARAMETER_SIZE_MISMATCH_MSG, "approaches", parameters.approaches, coord_size, help); + PARAMETER_SIZE_MISMATCH_MSG, "radiuses", parameters.radiuses, "coordinates", coord_size, help) || + constrainParamSize( + PARAMETER_SIZE_MISMATCH_MSG, "approaches", parameters.approaches, "coordinates", coord_size, help); if (!param_size_mismatch && parameters.coordinates.size() < 2) { From aa1f97d25b2def8f257cf31e346bbaf0b1778b48 Mon Sep 17 00:00:00 2001 From: whytro Date: Fri, 10 Mar 2023 04:40:32 +0900 Subject: [PATCH 02/11] Added CHANGELOG.md entry for #2983 issue fix --- CHANGELOG.md | 2 ++ 1 file changed, 2 insertions(+) diff --git a/CHANGELOG.md b/CHANGELOG.md index 98a871bf4..61101424e 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -1,5 +1,7 @@ # Unreleased - Changes from 5.27.1 + - API: + - CHANGED: Require a `radius` parameter when using `bearings`. [#6572](https://github.com/Project-OSRM/osrm-backend/pull/6572) - Build: - ADDED: Add CI job which builds OSRM with gcc 12. [#6455](https://github.com/Project-OSRM/osrm-backend/pull/6455) - CHANGED: Upgrade to clang-tidy 15. [#6439](https://github.com/Project-OSRM/osrm-backend/pull/6439) From a2ff69c3aadcbaa1f158f30be7ab8774a934ea7a Mon Sep 17 00:00:00 2001 From: whytro Date: Sat, 11 Mar 2023 14:39:11 +0900 Subject: [PATCH 03/11] Check bearings, radiuses size in TablePlugin --- features/car/startpoint.feature | 2 ++ src/engine/plugins/table.cpp | 6 ++++++ unit_tests/library/table.cpp | 30 ++++++++++++++++++++++++++++++ 3 files changed, 38 insertions(+) diff --git a/features/car/startpoint.feature b/features/car/startpoint.feature index 696f1ae31..47aa5d0ed 100644 --- a/features/car/startpoint.feature +++ b/features/car/startpoint.feature @@ -70,6 +70,7 @@ Feature: Car - Allowed start/end modes Given the query options | snapping | any | | bearings | 90,180; | + | radiuses | unlimited; | And the ways | nodes | highway | access | @@ -112,6 +113,7 @@ Feature: Car - Allowed start/end modes Given the query options | snapping | any | | bearings | 90,180;0,180;; | + | radiuses | unlimited;;; | And the ways | nodes | highway | access | diff --git a/src/engine/plugins/table.cpp b/src/engine/plugins/table.cpp index 150cefb34..dd7a04649 100644 --- a/src/engine/plugins/table.cpp +++ b/src/engine/plugins/table.cpp @@ -42,6 +42,12 @@ Status TablePlugin::HandleRequest(const RoutingAlgorithmsInterface &algorithms, return Error( "InvalidOptions", "Number of bearings does not match number of coordinates", result); } + + if (!params.bearings.empty() && params.radiuses.size() != params.bearings.size()) + { + return Error( + "InvalidOptions", "Number of radiuses does not match number of bearings", result); + } // Empty sources or destinations means the user wants all of them included, respectively // The ManyToMany routing algorithm we dispatch to below already handles this perfectly. diff --git a/unit_tests/library/table.cpp b/unit_tests/library/table.cpp index 0fa27d9a1..39047b11e 100644 --- a/unit_tests/library/table.cpp +++ b/unit_tests/library/table.cpp @@ -387,4 +387,34 @@ BOOST_AUTO_TEST_CASE(test_table_serialiaze_fb_no_waypoints) BOOST_CHECK(fb->waypoints() == nullptr); } +void test_table_bearings_without_radius(bool use_json_only_api) +{ + using namespace osrm; + + auto osrm = getOSRM(OSRM_TEST_DATA_DIR "/ch/monaco.osrm"); + + TableParameters params; + params.coordinates.push_back(get_dummy_location()); + params.coordinates.push_back(get_dummy_location()); + params.bearings.push_back(engine::Bearing{100, 100}); + params.bearings.push_back(engine::Bearing{100, 100}); + + json::Object json_result; + const auto rc = run_table_json(osrm, params, json_result, use_json_only_api); + + BOOST_CHECK(rc == Status::Error); + const auto code = json_result.values.at("code").get().value; + BOOST_CHECK_EQUAL(code, "InvalidOptions"); + const auto message = json_result.values.at("message").get().value; + BOOST_CHECK_EQUAL(message, "Number of radiuses does not match number of bearings"); +} +BOOST_AUTO_TEST_CASE(test_table_bearings_without_radius_old_api) +{ + test_table_bearings_without_radius(true); +} +BOOST_AUTO_TEST_CASE(test_table_bearings_without_radius_new_api) +{ + test_table_bearings_without_radius(false); +} + BOOST_AUTO_TEST_SUITE_END() From 3de501b88fe2bdb69957127f43e7bb58b920d473 Mon Sep 17 00:00:00 2001 From: whytro Date: Sat, 11 Mar 2023 16:40:36 +0900 Subject: [PATCH 04/11] Included radiuses for bearings in annotations --- features/testbot/annotations.feature | 3 ++- 1 file changed, 2 insertions(+), 1 deletion(-) diff --git a/features/testbot/annotations.feature b/features/testbot/annotations.feature index cff8461b2..d6c6e2f1f 100644 --- a/features/testbot/annotations.feature +++ b/features/testbot/annotations.feature @@ -112,7 +112,8 @@ Feature: Annotations And the query options | annotations | speed,distance,duration,nodes | | bearings | 90,5;180,5 | + | radiuses | unlimited;unlimited | When I route I should get - | from | to | route | a:speed | a:distance | a:duration | a:nodes | + | from | to | route | a:speed | a:distance | a:duration | a:nodes | | a | c | abc,abc | 10:10 | 249.9876189:299.962882 | 25:30 | 1:2:3 | From 85b0d22723775787de1947d4b600ab724ef75495 Mon Sep 17 00:00:00 2001 From: whytro Date: Sat, 11 Mar 2023 16:41:47 +0900 Subject: [PATCH 05/11] Default include radiuses to tests with bearings --- features/support/route.js | 3 +++ 1 file changed, 3 insertions(+) diff --git a/features/support/route.js b/features/support/route.js index cd713372d..85183eb0d 100644 --- a/features/support/route.js +++ b/features/support/route.js @@ -66,6 +66,9 @@ module.exports = function () { if (bs.length === 2) return b; else return b += ',10'; }).join(';'); + params.radiuses = bearings.map(() => { + return 'unlimited'; + }).join(';'); } if (approaches.length) { From f883555a42911674f5ba730a82b951f6aae5fa22 Mon Sep 17 00:00:00 2001 From: Whytro Date: Sun, 12 Mar 2023 18:22:43 +0900 Subject: [PATCH 06/11] Fix formatting issues --- include/server/service/utils.hpp | 3 +- src/engine/plugins/table.cpp | 2 +- src/server/service/match_service.cpp | 41 +++++++++++++++++++------- src/server/service/nearest_service.cpp | 32 +++++++++++++++----- src/server/service/route_service.cpp | 41 +++++++++++++++++++------- src/server/service/table_service.cpp | 41 +++++++++++++++++++------- src/server/service/trip_service.cpp | 41 +++++++++++++++++++------- 7 files changed, 147 insertions(+), 54 deletions(-) diff --git a/include/server/service/utils.hpp b/include/server/service/utils.hpp index a9a0a73e1..223b42820 100644 --- a/include/server/service/utils.hpp +++ b/include/server/service/utils.hpp @@ -16,7 +16,8 @@ bool constrainParamSize(const char *msg_template, { if (param.size() > 0 && param.size() != target_size) { - help = (boost::format(msg_template) % param_name % param.size() % target_name % target_size).str(); + help = (boost::format(msg_template) % param_name % param.size() % target_name % target_size) + .str(); return true; } return false; diff --git a/src/engine/plugins/table.cpp b/src/engine/plugins/table.cpp index dd7a04649..b3094cdf7 100644 --- a/src/engine/plugins/table.cpp +++ b/src/engine/plugins/table.cpp @@ -42,7 +42,7 @@ Status TablePlugin::HandleRequest(const RoutingAlgorithmsInterface &algorithms, return Error( "InvalidOptions", "Number of bearings does not match number of coordinates", result); } - + if (!params.bearings.empty() && params.radiuses.size() != params.bearings.size()) { return Error( diff --git a/src/server/service/match_service.cpp b/src/server/service/match_service.cpp index c14f8d592..5acce7a73 100644 --- a/src/server/service/match_service.cpp +++ b/src/server/service/match_service.cpp @@ -17,17 +17,36 @@ std::string getWrongOptionHelp(const engine::api::MatchParameters ¶meters) const auto coord_size = parameters.coordinates.size(); const auto bearings_size = parameters.bearings.size(); - const bool param_size_mismatch = - constrainParamSize( - PARAMETER_SIZE_MISMATCH_MSG, "hints", parameters.hints, "coordinates", coord_size, help) || - constrainParamSize( - PARAMETER_SIZE_MISMATCH_MSG, "bearings", parameters.bearings, "coordinates", coord_size, help) || - constrainParamSize( - PARAMETER_SIZE_MISMATCH_MSG, "radiuses", parameters.radiuses, "bearings", bearings_size, help) || - constrainParamSize( - PARAMETER_SIZE_MISMATCH_MSG, "radiuses", parameters.radiuses, "coordinates", coord_size, help) || - constrainParamSize( - PARAMETER_SIZE_MISMATCH_MSG, "timestamps", parameters.timestamps, "coordinates", coord_size, help); + const bool param_size_mismatch = constrainParamSize(PARAMETER_SIZE_MISMATCH_MSG, + "hints", + parameters.hints, + "coordinates", + coord_size, + help) || + constrainParamSize(PARAMETER_SIZE_MISMATCH_MSG, + "bearings", + parameters.bearings, + "coordinates", + coord_size, + help) || + constrainParamSize(PARAMETER_SIZE_MISMATCH_MSG, + "radiuses", + parameters.radiuses, + "bearings", + bearings_size, + help) || + constrainParamSize(PARAMETER_SIZE_MISMATCH_MSG, + "radiuses", + parameters.radiuses, + "coordinates", + coord_size, + help) || + constrainParamSize(PARAMETER_SIZE_MISMATCH_MSG, + "timestamps", + parameters.timestamps, + "coordinates", + coord_size, + help); if (!param_size_mismatch && parameters.coordinates.size() < 2) { diff --git a/src/server/service/nearest_service.cpp b/src/server/service/nearest_service.cpp index 2f78d14af..94b75259d 100644 --- a/src/server/service/nearest_service.cpp +++ b/src/server/service/nearest_service.cpp @@ -19,14 +19,30 @@ std::string getWrongOptionHelp(const engine::api::NearestParameters ¶meters) constrainParamSize( PARAMETER_SIZE_MISMATCH_MSG, "hints", parameters.hints, "coordinates", coord_size, help); - constrainParamSize( - PARAMETER_SIZE_MISMATCH_MSG, "bearings", parameters.bearings, "coordinates", coord_size, help); - constrainParamSize( - PARAMETER_SIZE_MISMATCH_MSG, "radiuses", parameters.radiuses, "bearings", bearings_size, help); - constrainParamSize( - PARAMETER_SIZE_MISMATCH_MSG, "radiuses", parameters.radiuses, "coordinates", coord_size, help); - constrainParamSize( - PARAMETER_SIZE_MISMATCH_MSG, "approaches", parameters.approaches, "coordinates", coord_size, help); + constrainParamSize(PARAMETER_SIZE_MISMATCH_MSG, + "bearings", + parameters.bearings, + "coordinates", + coord_size, + help); + constrainParamSize(PARAMETER_SIZE_MISMATCH_MSG, + "radiuses", + parameters.radiuses, + "bearings", + bearings_size, + help); + constrainParamSize(PARAMETER_SIZE_MISMATCH_MSG, + "radiuses", + parameters.radiuses, + "coordinates", + coord_size, + help); + constrainParamSize(PARAMETER_SIZE_MISMATCH_MSG, + "approaches", + parameters.approaches, + "coordinates", + coord_size, + help); return help; } diff --git a/src/server/service/route_service.cpp b/src/server/service/route_service.cpp index 41f146abf..625a0815f 100644 --- a/src/server/service/route_service.cpp +++ b/src/server/service/route_service.cpp @@ -17,17 +17,36 @@ std::string getWrongOptionHelp(const engine::api::RouteParameters ¶meters) const auto coord_size = parameters.coordinates.size(); const auto bearings_size = parameters.bearings.size(); - const bool param_size_mismatch = - constrainParamSize( - PARAMETER_SIZE_MISMATCH_MSG, "hints", parameters.hints, "coordinates", coord_size, help) || - constrainParamSize( - PARAMETER_SIZE_MISMATCH_MSG, "bearings", parameters.bearings, "coordinates", coord_size, help) || - constrainParamSize( - PARAMETER_SIZE_MISMATCH_MSG, "radiuses", parameters.radiuses, "bearings", bearings_size, help) || - constrainParamSize( - PARAMETER_SIZE_MISMATCH_MSG, "radiuses", parameters.radiuses, "coordinates", coord_size, help) || - constrainParamSize( - PARAMETER_SIZE_MISMATCH_MSG, "approaches", parameters.approaches, "coordinates", coord_size, help); + const bool param_size_mismatch = constrainParamSize(PARAMETER_SIZE_MISMATCH_MSG, + "hints", + parameters.hints, + "coordinates", + coord_size, + help) || + constrainParamSize(PARAMETER_SIZE_MISMATCH_MSG, + "bearings", + parameters.bearings, + "coordinates", + coord_size, + help) || + constrainParamSize(PARAMETER_SIZE_MISMATCH_MSG, + "radiuses", + parameters.radiuses, + "bearings", + bearings_size, + help) || + constrainParamSize(PARAMETER_SIZE_MISMATCH_MSG, + "radiuses", + parameters.radiuses, + "coordinates", + coord_size, + help) || + constrainParamSize(PARAMETER_SIZE_MISMATCH_MSG, + "approaches", + parameters.approaches, + "coordinates", + coord_size, + help); if (!param_size_mismatch && parameters.coordinates.size() < 2) { diff --git a/src/server/service/table_service.cpp b/src/server/service/table_service.cpp index 88d0804d6..dee7429c0 100644 --- a/src/server/service/table_service.cpp +++ b/src/server/service/table_service.cpp @@ -17,17 +17,36 @@ std::string getWrongOptionHelp(const engine::api::TableParameters ¶meters) const auto coord_size = parameters.coordinates.size(); const auto bearings_size = parameters.bearings.size(); - const bool param_size_mismatch = - constrainParamSize( - PARAMETER_SIZE_MISMATCH_MSG, "hints", parameters.hints, "coordinates", coord_size, help) || - constrainParamSize( - PARAMETER_SIZE_MISMATCH_MSG, "bearings", parameters.bearings, "coordinates", coord_size, help) || - constrainParamSize( - PARAMETER_SIZE_MISMATCH_MSG, "radiuses", parameters.radiuses, "bearings", bearings_size, help) || - constrainParamSize( - PARAMETER_SIZE_MISMATCH_MSG, "radiuses", parameters.radiuses, "coordinates", coord_size, help) || - constrainParamSize( - PARAMETER_SIZE_MISMATCH_MSG, "approaches", parameters.approaches, "coordinates", coord_size, help); + const bool param_size_mismatch = constrainParamSize(PARAMETER_SIZE_MISMATCH_MSG, + "hints", + parameters.hints, + "coordinates", + coord_size, + help) || + constrainParamSize(PARAMETER_SIZE_MISMATCH_MSG, + "bearings", + parameters.bearings, + "coordinates", + coord_size, + help) || + constrainParamSize(PARAMETER_SIZE_MISMATCH_MSG, + "radiuses", + parameters.radiuses, + "bearings", + bearings_size, + help) || + constrainParamSize(PARAMETER_SIZE_MISMATCH_MSG, + "radiuses", + parameters.radiuses, + "coordinates", + coord_size, + help) || + constrainParamSize(PARAMETER_SIZE_MISMATCH_MSG, + "approaches", + parameters.approaches, + "coordinates", + coord_size, + help); if (!param_size_mismatch && parameters.coordinates.size() < 2) { diff --git a/src/server/service/trip_service.cpp b/src/server/service/trip_service.cpp index 5fc35698a..4b6d31970 100644 --- a/src/server/service/trip_service.cpp +++ b/src/server/service/trip_service.cpp @@ -17,17 +17,36 @@ std::string getWrongOptionHelp(const engine::api::TripParameters ¶meters) const auto coord_size = parameters.coordinates.size(); const auto bearings_size = parameters.bearings.size(); - const bool param_size_mismatch = - constrainParamSize( - PARAMETER_SIZE_MISMATCH_MSG, "hints", parameters.hints, "coordinates", coord_size, help) || - constrainParamSize( - PARAMETER_SIZE_MISMATCH_MSG, "bearings", parameters.bearings, "coordinates", coord_size, help) || - constrainParamSize( - PARAMETER_SIZE_MISMATCH_MSG, "radiuses", parameters.radiuses, "bearings", bearings_size, help) || - constrainParamSize( - PARAMETER_SIZE_MISMATCH_MSG, "radiuses", parameters.radiuses, "coordinates", coord_size, help) || - constrainParamSize( - PARAMETER_SIZE_MISMATCH_MSG, "approaches", parameters.approaches, "coordinates", coord_size, help); + const bool param_size_mismatch = constrainParamSize(PARAMETER_SIZE_MISMATCH_MSG, + "hints", + parameters.hints, + "coordinates", + coord_size, + help) || + constrainParamSize(PARAMETER_SIZE_MISMATCH_MSG, + "bearings", + parameters.bearings, + "coordinates", + coord_size, + help) || + constrainParamSize(PARAMETER_SIZE_MISMATCH_MSG, + "radiuses", + parameters.radiuses, + "bearings", + bearings_size, + help) || + constrainParamSize(PARAMETER_SIZE_MISMATCH_MSG, + "radiuses", + parameters.radiuses, + "coordinates", + coord_size, + help) || + constrainParamSize(PARAMETER_SIZE_MISMATCH_MSG, + "approaches", + parameters.approaches, + "coordinates", + coord_size, + help); if (!param_size_mismatch && parameters.coordinates.size() < 2) { From 64bcc9235f761bfd5867a5456fc54d0ffdc3b87d Mon Sep 17 00:00:00 2001 From: whytro Date: Thu, 16 Mar 2023 00:10:24 +0900 Subject: [PATCH 07/11] Require radiuses for bearings in nodejs bindings --- docs/nodejs/api.md | 3 ++- include/nodejs/node_osrm_support.hpp | 5 +++++ src/nodejs/node_osrm.cpp | 3 ++- test/nodejs/route.js | 15 ++++++++++++++- 4 files changed, 23 insertions(+), 3 deletions(-) diff --git a/docs/nodejs/api.md b/docs/nodejs/api.md index c4d389023..992fc9689 100644 --- a/docs/nodejs/api.md +++ b/docs/nodejs/api.md @@ -111,7 +111,8 @@ var osrm = new OSRM('network.osrm'); var options = { coordinates: [[13.388860,52.517037]], number: 3, - bearings: [[0,20]] + bearings: [[0,20]], + radiuses: [null] }; osrm.nearest(options, function(err, response) { console.log(response.waypoints); // array of Waypoint objects diff --git a/include/nodejs/node_osrm_support.hpp b/include/nodejs/node_osrm_support.hpp index a1d7b4473..d6c60e8ec 100644 --- a/include/nodejs/node_osrm_support.hpp +++ b/include/nodejs/node_osrm_support.hpp @@ -522,6 +522,11 @@ inline bool argumentsToParameter(const Napi::CallbackInfo &args, if (bearings.IsEmpty()) return false; + if (!obj.Has("radiuses")) { + ThrowError(args.Env(), "Bearings must be accompanied with radiuses"); + return false; + } + if (!bearings.IsArray()) { ThrowError(args.Env(), "Bearings must be an array of arrays of numbers"); diff --git a/src/nodejs/node_osrm.cpp b/src/nodejs/node_osrm.cpp index 1537ab4d5..e866b0a52 100644 --- a/src/nodejs/node_osrm.cpp +++ b/src/nodejs/node_osrm.cpp @@ -349,7 +349,8 @@ Napi::Value Engine::route(const Napi::CallbackInfo &info) * var options = { * coordinates: [[13.388860,52.517037]], * number: 3, - * bearings: [[0,20]] + * bearings: [[0,20]], + * radiuses: [null] * }; * osrm.nearest(options, function(err, response) { * console.log(response.waypoints); // array of Waypoint objects diff --git a/test/nodejs/route.js b/test/nodejs/route.js index d0f1ddf49..6ddf95f51 100644 --- a/test/nodejs/route.js +++ b/test/nodejs/route.js @@ -424,6 +424,7 @@ test('route: integer bearing values no longer supported', function(assert) { var options = { coordinates: two_test_coordinates, bearings: [200, 250], + radiuses: [null], }; assert.throws(function() { osrm.route(options, function(err, route) {}); }, /Bearing must be an array of \[bearing, range\] or null/); @@ -435,6 +436,7 @@ test('route: valid bearing values', function(assert) { var options = { coordinates: two_test_coordinates, bearings: [[200, 180], [250, 180]], + radiuses: [null, null], }; osrm.route(options, function(err, route) { assert.ifError(err); @@ -448,38 +450,49 @@ test('route: valid bearing values', function(assert) { }); test('route: invalid bearing values', function(assert) { - assert.plan(6); + assert.plan(7); var osrm = new OSRM(monaco_path); assert.throws(function() { osrm.route({ coordinates: two_test_coordinates, bearings: [[400, 180], [-250, 180]], + radiuses: [null, null], }, function(err, route) {}) }, /Bearing values need to be in range 0..360, 0..180/); assert.throws(function() { osrm.route({ coordinates: two_test_coordinates, bearings: [[200], [250, 180]], + radiuses: [null, null], }, function(err, route) {}) }, /Bearing must be an array of/); assert.throws(function() { osrm.route({ coordinates: two_test_coordinates, bearings: [[400, 109], [100, 720]], + radiuses: [null, null], }, function(err, route) {}) }, /Bearing values need to be in range 0..360, 0..180/); assert.throws(function() { osrm.route({ coordinates: two_test_coordinates, bearings: 400, + radiuses: [null], }, function(err, route) {}) }, /Bearings must be an array of arrays of numbers/); assert.throws(function() { osrm.route({ coordinates: two_test_coordinates, bearings: [[100, 100]], + radiuses: [null, null], }, function(err, route) {}) }, /Bearings array must have the same length as coordinates array/); assert.throws(function() { osrm.route({ coordinates: two_test_coordinates, bearings: [Infinity, Infinity], + radiuses: [null, null], }, function(err, route) {}) }, /Bearing must be an array of \[bearing, range\] or null/); + assert.throws(function() { osrm.route({ + coordinates: two_test_coordinates, + bearings: [[0, 180], [0, 180]], + }, function(err, route) {}) }, + /Bearings must be accompanied with radiuses/); }); test('route: routes Monaco with hints', function(assert) { From 7dfa2bdd1f9c06068552bc7553ab4300b5043c12 Mon Sep 17 00:00:00 2001 From: Whytro Date: Thu, 16 Mar 2023 00:44:14 +0900 Subject: [PATCH 08/11] Formatting fix --- include/nodejs/node_osrm_support.hpp | 3 ++- 1 file changed, 2 insertions(+), 1 deletion(-) diff --git a/include/nodejs/node_osrm_support.hpp b/include/nodejs/node_osrm_support.hpp index d6c60e8ec..2f9fa5c2f 100644 --- a/include/nodejs/node_osrm_support.hpp +++ b/include/nodejs/node_osrm_support.hpp @@ -522,7 +522,8 @@ inline bool argumentsToParameter(const Napi::CallbackInfo &args, if (bearings.IsEmpty()) return false; - if (!obj.Has("radiuses")) { + if (!obj.Has("radiuses")) + { ThrowError(args.Env(), "Bearings must be accompanied with radiuses"); return false; } From 09ff160dccd95c0cde2fb8343b0bf18e2108016c Mon Sep 17 00:00:00 2001 From: whytro Date: Fri, 28 Apr 2023 02:00:54 +0900 Subject: [PATCH 09/11] Revert previous commits for cleaner approach --- CHANGELOG.md | 2 - docs/http.md | 2 +- docs/nodejs/api.md | 3 +- features/car/startpoint.feature | 2 - features/support/route.js | 3 -- features/testbot/annotations.feature | 3 +- include/engine/api/base_parameters.hpp | 2 +- include/nodejs/node_osrm_support.hpp | 6 --- include/server/service/utils.hpp | 8 ++-- src/engine/plugins/table.cpp | 6 --- src/nodejs/node_osrm.cpp | 3 +- src/server/service/match_service.cpp | 44 +++++------------- src/server/service/nearest_service.cpp | 35 ++++---------- src/server/service/route_service.cpp | 40 ++++------------ src/server/service/table_service.cpp | 63 +++++++++++++------------- src/server/service/trip_service.cpp | 42 +++++------------ test/nodejs/route.js | 15 +----- unit_tests/library/table.cpp | 30 ------------ 18 files changed, 81 insertions(+), 228 deletions(-) diff --git a/CHANGELOG.md b/CHANGELOG.md index 61101424e..98a871bf4 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -1,7 +1,5 @@ # Unreleased - Changes from 5.27.1 - - API: - - CHANGED: Require a `radius` parameter when using `bearings`. [#6572](https://github.com/Project-OSRM/osrm-backend/pull/6572) - Build: - ADDED: Add CI job which builds OSRM with gcc 12. [#6455](https://github.com/Project-OSRM/osrm-backend/pull/6455) - CHANGED: Upgrade to clang-tidy 15. [#6439](https://github.com/Project-OSRM/osrm-backend/pull/6439) diff --git a/docs/http.md b/docs/http.md index beb59af91..07acc968a 100644 --- a/docs/http.md +++ b/docs/http.md @@ -31,7 +31,7 @@ To pass parameters to each location some options support an array-like encoding: | Option | Values | Description | |----------------|--------------------------------------------------------|-----------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------| -|bearings |`{bearing};{bearing}[;{bearing} ...]` |Limits the search to segments with given bearing in degrees towards true north in a clockwise direction. Requires a corresponding radius. | +|bearings |`{bearing};{bearing}[;{bearing} ...]` |Limits the search to segments with given bearing in degrees towards true north in a 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. | diff --git a/docs/nodejs/api.md b/docs/nodejs/api.md index 992fc9689..c4d389023 100644 --- a/docs/nodejs/api.md +++ b/docs/nodejs/api.md @@ -111,8 +111,7 @@ var osrm = new OSRM('network.osrm'); var options = { coordinates: [[13.388860,52.517037]], number: 3, - bearings: [[0,20]], - radiuses: [null] + bearings: [[0,20]] }; osrm.nearest(options, function(err, response) { console.log(response.waypoints); // array of Waypoint objects diff --git a/features/car/startpoint.feature b/features/car/startpoint.feature index 47aa5d0ed..696f1ae31 100644 --- a/features/car/startpoint.feature +++ b/features/car/startpoint.feature @@ -70,7 +70,6 @@ Feature: Car - Allowed start/end modes Given the query options | snapping | any | | bearings | 90,180; | - | radiuses | unlimited; | And the ways | nodes | highway | access | @@ -113,7 +112,6 @@ Feature: Car - Allowed start/end modes Given the query options | snapping | any | | bearings | 90,180;0,180;; | - | radiuses | unlimited;;; | And the ways | nodes | highway | access | diff --git a/features/support/route.js b/features/support/route.js index 85183eb0d..cd713372d 100644 --- a/features/support/route.js +++ b/features/support/route.js @@ -66,9 +66,6 @@ module.exports = function () { if (bs.length === 2) return b; else return b += ',10'; }).join(';'); - params.radiuses = bearings.map(() => { - return 'unlimited'; - }).join(';'); } if (approaches.length) { diff --git a/features/testbot/annotations.feature b/features/testbot/annotations.feature index d6c6e2f1f..cff8461b2 100644 --- a/features/testbot/annotations.feature +++ b/features/testbot/annotations.feature @@ -112,8 +112,7 @@ Feature: Annotations And the query options | annotations | speed,distance,duration,nodes | | bearings | 90,5;180,5 | - | radiuses | unlimited;unlimited | When I route I should get - | from | to | route | a:speed | a:distance | a:duration | a:nodes | + | from | to | route | a:speed | a:distance | a:duration | a:nodes | | a | c | abc,abc | 10:10 | 249.9876189:299.962882 | 25:30 | 1:2:3 | diff --git a/include/engine/api/base_parameters.hpp b/include/engine/api/base_parameters.hpp index 99ce58b4b..9cc13ae01 100644 --- a/include/engine/api/base_parameters.hpp +++ b/include/engine/api/base_parameters.hpp @@ -106,7 +106,7 @@ struct BaseParameters bool IsValid() const { return (hints.empty() || hints.size() == coordinates.size()) && - (bearings.empty() || bearings.size() == radiuses.size()) && + (bearings.empty() || bearings.size() == coordinates.size()) && (radiuses.empty() || radiuses.size() == coordinates.size()) && (approaches.empty() || approaches.size() == coordinates.size()) && std::all_of(bearings.begin(), diff --git a/include/nodejs/node_osrm_support.hpp b/include/nodejs/node_osrm_support.hpp index 2f9fa5c2f..a1d7b4473 100644 --- a/include/nodejs/node_osrm_support.hpp +++ b/include/nodejs/node_osrm_support.hpp @@ -522,12 +522,6 @@ inline bool argumentsToParameter(const Napi::CallbackInfo &args, if (bearings.IsEmpty()) return false; - if (!obj.Has("radiuses")) - { - ThrowError(args.Env(), "Bearings must be accompanied with radiuses"); - return false; - } - if (!bearings.IsArray()) { ThrowError(args.Env(), "Bearings must be an array of arrays of numbers"); diff --git a/include/server/service/utils.hpp b/include/server/service/utils.hpp index 223b42820..391211d69 100644 --- a/include/server/service/utils.hpp +++ b/include/server/service/utils.hpp @@ -4,20 +4,18 @@ namespace osrm::server::service { const constexpr char PARAMETER_SIZE_MISMATCH_MSG[] = - "Number of elements in %1% size %2% does not match %3% size %4%"; + "Number of elements in %1% size %2% does not match coordinate size %3%"; template bool constrainParamSize(const char *msg_template, - const char *param_name, + const char *name, const ParamT ¶m, - const char *target_name, const std::size_t target_size, std::string &help) { if (param.size() > 0 && param.size() != target_size) { - help = (boost::format(msg_template) % param_name % param.size() % target_name % target_size) - .str(); + help = (boost::format(msg_template) % name % param.size() % target_size).str(); return true; } return false; diff --git a/src/engine/plugins/table.cpp b/src/engine/plugins/table.cpp index b3094cdf7..150cefb34 100644 --- a/src/engine/plugins/table.cpp +++ b/src/engine/plugins/table.cpp @@ -43,12 +43,6 @@ Status TablePlugin::HandleRequest(const RoutingAlgorithmsInterface &algorithms, "InvalidOptions", "Number of bearings does not match number of coordinates", result); } - if (!params.bearings.empty() && params.radiuses.size() != params.bearings.size()) - { - return Error( - "InvalidOptions", "Number of radiuses does not match number of bearings", result); - } - // Empty sources or destinations means the user wants all of them included, respectively // The ManyToMany routing algorithm we dispatch to below already handles this perfectly. const auto num_sources = diff --git a/src/nodejs/node_osrm.cpp b/src/nodejs/node_osrm.cpp index e866b0a52..1537ab4d5 100644 --- a/src/nodejs/node_osrm.cpp +++ b/src/nodejs/node_osrm.cpp @@ -349,8 +349,7 @@ Napi::Value Engine::route(const Napi::CallbackInfo &info) * var options = { * coordinates: [[13.388860,52.517037]], * number: 3, - * bearings: [[0,20]], - * radiuses: [null] + * bearings: [[0,20]] * }; * osrm.nearest(options, function(err, response) { * console.log(response.waypoints); // array of Waypoint objects diff --git a/src/server/service/match_service.cpp b/src/server/service/match_service.cpp index 5acce7a73..3a74ec90b 100644 --- a/src/server/service/match_service.cpp +++ b/src/server/service/match_service.cpp @@ -1,11 +1,13 @@ #include "server/service/match_service.hpp" -#include "server/service/utils.hpp" #include "server/api/parameters_parser.hpp" +#include "server/service/utils.hpp" #include "engine/api/match_parameters.hpp" #include "util/json_container.hpp" +#include + namespace osrm::server::service { namespace @@ -15,38 +17,16 @@ std::string getWrongOptionHelp(const engine::api::MatchParameters ¶meters) std::string help; const auto coord_size = parameters.coordinates.size(); - const auto bearings_size = parameters.bearings.size(); - const bool param_size_mismatch = constrainParamSize(PARAMETER_SIZE_MISMATCH_MSG, - "hints", - parameters.hints, - "coordinates", - coord_size, - help) || - constrainParamSize(PARAMETER_SIZE_MISMATCH_MSG, - "bearings", - parameters.bearings, - "coordinates", - coord_size, - help) || - constrainParamSize(PARAMETER_SIZE_MISMATCH_MSG, - "radiuses", - parameters.radiuses, - "bearings", - bearings_size, - help) || - constrainParamSize(PARAMETER_SIZE_MISMATCH_MSG, - "radiuses", - parameters.radiuses, - "coordinates", - coord_size, - help) || - constrainParamSize(PARAMETER_SIZE_MISMATCH_MSG, - "timestamps", - parameters.timestamps, - "coordinates", - coord_size, - help); + const bool param_size_mismatch = + constrainParamSize( + PARAMETER_SIZE_MISMATCH_MSG, "hints", parameters.hints, coord_size, help) || + constrainParamSize( + PARAMETER_SIZE_MISMATCH_MSG, "bearings", parameters.bearings, coord_size, help) || + constrainParamSize( + PARAMETER_SIZE_MISMATCH_MSG, "radiuses", parameters.radiuses, coord_size, help) || + constrainParamSize( + PARAMETER_SIZE_MISMATCH_MSG, "timestamps", parameters.timestamps, coord_size, help); if (!param_size_mismatch && parameters.coordinates.size() < 2) { diff --git a/src/server/service/nearest_service.cpp b/src/server/service/nearest_service.cpp index 94b75259d..28aa75d69 100644 --- a/src/server/service/nearest_service.cpp +++ b/src/server/service/nearest_service.cpp @@ -6,8 +6,11 @@ #include "util/json_container.hpp" +#include + namespace osrm::server::service { + namespace { std::string getWrongOptionHelp(const engine::api::NearestParameters ¶meters) @@ -15,34 +18,14 @@ std::string getWrongOptionHelp(const engine::api::NearestParameters ¶meters) std::string help; const auto coord_size = parameters.coordinates.size(); - const auto bearings_size = parameters.bearings.size(); + constrainParamSize(PARAMETER_SIZE_MISMATCH_MSG, "hints", parameters.hints, coord_size, help); constrainParamSize( - PARAMETER_SIZE_MISMATCH_MSG, "hints", parameters.hints, "coordinates", coord_size, help); - constrainParamSize(PARAMETER_SIZE_MISMATCH_MSG, - "bearings", - parameters.bearings, - "coordinates", - coord_size, - help); - constrainParamSize(PARAMETER_SIZE_MISMATCH_MSG, - "radiuses", - parameters.radiuses, - "bearings", - bearings_size, - help); - constrainParamSize(PARAMETER_SIZE_MISMATCH_MSG, - "radiuses", - parameters.radiuses, - "coordinates", - coord_size, - help); - constrainParamSize(PARAMETER_SIZE_MISMATCH_MSG, - "approaches", - parameters.approaches, - "coordinates", - coord_size, - help); + PARAMETER_SIZE_MISMATCH_MSG, "bearings", parameters.bearings, coord_size, help); + constrainParamSize( + PARAMETER_SIZE_MISMATCH_MSG, "radiuses", parameters.radiuses, coord_size, help); + constrainParamSize( + PARAMETER_SIZE_MISMATCH_MSG, "approaches", parameters.approaches, coord_size, help); return help; } diff --git a/src/server/service/route_service.cpp b/src/server/service/route_service.cpp index 625a0815f..f5b61e82a 100644 --- a/src/server/service/route_service.cpp +++ b/src/server/service/route_service.cpp @@ -15,38 +15,16 @@ std::string getWrongOptionHelp(const engine::api::RouteParameters ¶meters) std::string help; const auto coord_size = parameters.coordinates.size(); - const auto bearings_size = parameters.bearings.size(); - const bool param_size_mismatch = constrainParamSize(PARAMETER_SIZE_MISMATCH_MSG, - "hints", - parameters.hints, - "coordinates", - coord_size, - help) || - constrainParamSize(PARAMETER_SIZE_MISMATCH_MSG, - "bearings", - parameters.bearings, - "coordinates", - coord_size, - help) || - constrainParamSize(PARAMETER_SIZE_MISMATCH_MSG, - "radiuses", - parameters.radiuses, - "bearings", - bearings_size, - help) || - constrainParamSize(PARAMETER_SIZE_MISMATCH_MSG, - "radiuses", - parameters.radiuses, - "coordinates", - coord_size, - help) || - constrainParamSize(PARAMETER_SIZE_MISMATCH_MSG, - "approaches", - parameters.approaches, - "coordinates", - coord_size, - help); + const bool param_size_mismatch = + constrainParamSize( + PARAMETER_SIZE_MISMATCH_MSG, "hints", parameters.hints, coord_size, help) || + constrainParamSize( + PARAMETER_SIZE_MISMATCH_MSG, "bearings", parameters.bearings, coord_size, help) || + constrainParamSize( + PARAMETER_SIZE_MISMATCH_MSG, "radiuses", parameters.radiuses, coord_size, help) || + constrainParamSize( + PARAMETER_SIZE_MISMATCH_MSG, "approaches", parameters.approaches, coord_size, help); if (!param_size_mismatch && parameters.coordinates.size() < 2) { diff --git a/src/server/service/table_service.cpp b/src/server/service/table_service.cpp index dee7429c0..27341f6aa 100644 --- a/src/server/service/table_service.cpp +++ b/src/server/service/table_service.cpp @@ -1,52 +1,51 @@ #include "server/service/table_service.hpp" -#include "server/service/utils.hpp" #include "server/api/parameters_parser.hpp" #include "engine/api/table_parameters.hpp" #include "util/json_container.hpp" +#include + namespace osrm::server::service { + namespace { + +const constexpr char PARAMETER_SIZE_MISMATCH_MSG[] = + "Number of elements in %1% size %2% does not match coordinate size %3%"; + +template +bool constrainParamSize(const char *msg_template, + const char *name, + const ParamT ¶m, + const std::size_t target_size, + std::string &help) +{ + if (param.size() > 0 && param.size() != target_size) + { + help = (boost::format(msg_template) % name % param.size() % target_size).str(); + return true; + } + return false; +} + std::string getWrongOptionHelp(const engine::api::TableParameters ¶meters) { std::string help; const auto coord_size = parameters.coordinates.size(); - const auto bearings_size = parameters.bearings.size(); - const bool param_size_mismatch = constrainParamSize(PARAMETER_SIZE_MISMATCH_MSG, - "hints", - parameters.hints, - "coordinates", - coord_size, - help) || - constrainParamSize(PARAMETER_SIZE_MISMATCH_MSG, - "bearings", - parameters.bearings, - "coordinates", - coord_size, - help) || - constrainParamSize(PARAMETER_SIZE_MISMATCH_MSG, - "radiuses", - parameters.radiuses, - "bearings", - bearings_size, - help) || - constrainParamSize(PARAMETER_SIZE_MISMATCH_MSG, - "radiuses", - parameters.radiuses, - "coordinates", - coord_size, - help) || - constrainParamSize(PARAMETER_SIZE_MISMATCH_MSG, - "approaches", - parameters.approaches, - "coordinates", - coord_size, - help); + const bool param_size_mismatch = + constrainParamSize( + PARAMETER_SIZE_MISMATCH_MSG, "hints", parameters.hints, coord_size, help) || + constrainParamSize( + PARAMETER_SIZE_MISMATCH_MSG, "bearings", parameters.bearings, coord_size, help) || + constrainParamSize( + PARAMETER_SIZE_MISMATCH_MSG, "radiuses", parameters.radiuses, coord_size, help) || + constrainParamSize( + PARAMETER_SIZE_MISMATCH_MSG, "approaches", parameters.approaches, coord_size, help); if (!param_size_mismatch && parameters.coordinates.size() < 2) { diff --git a/src/server/service/trip_service.cpp b/src/server/service/trip_service.cpp index 4b6d31970..a9e44b896 100644 --- a/src/server/service/trip_service.cpp +++ b/src/server/service/trip_service.cpp @@ -6,6 +6,8 @@ #include "util/json_container.hpp" +#include + namespace osrm::server::service { namespace @@ -15,38 +17,16 @@ std::string getWrongOptionHelp(const engine::api::TripParameters ¶meters) std::string help; const auto coord_size = parameters.coordinates.size(); - const auto bearings_size = parameters.bearings.size(); - const bool param_size_mismatch = constrainParamSize(PARAMETER_SIZE_MISMATCH_MSG, - "hints", - parameters.hints, - "coordinates", - coord_size, - help) || - constrainParamSize(PARAMETER_SIZE_MISMATCH_MSG, - "bearings", - parameters.bearings, - "coordinates", - coord_size, - help) || - constrainParamSize(PARAMETER_SIZE_MISMATCH_MSG, - "radiuses", - parameters.radiuses, - "bearings", - bearings_size, - help) || - constrainParamSize(PARAMETER_SIZE_MISMATCH_MSG, - "radiuses", - parameters.radiuses, - "coordinates", - coord_size, - help) || - constrainParamSize(PARAMETER_SIZE_MISMATCH_MSG, - "approaches", - parameters.approaches, - "coordinates", - coord_size, - help); + const bool param_size_mismatch = + constrainParamSize( + PARAMETER_SIZE_MISMATCH_MSG, "hints", parameters.hints, coord_size, help) || + constrainParamSize( + PARAMETER_SIZE_MISMATCH_MSG, "bearings", parameters.bearings, coord_size, help) || + constrainParamSize( + PARAMETER_SIZE_MISMATCH_MSG, "radiuses", parameters.radiuses, coord_size, help) || + constrainParamSize( + PARAMETER_SIZE_MISMATCH_MSG, "approaches", parameters.approaches, coord_size, help); if (!param_size_mismatch && parameters.coordinates.size() < 2) { diff --git a/test/nodejs/route.js b/test/nodejs/route.js index 6ddf95f51..d0f1ddf49 100644 --- a/test/nodejs/route.js +++ b/test/nodejs/route.js @@ -424,7 +424,6 @@ test('route: integer bearing values no longer supported', function(assert) { var options = { coordinates: two_test_coordinates, bearings: [200, 250], - radiuses: [null], }; assert.throws(function() { osrm.route(options, function(err, route) {}); }, /Bearing must be an array of \[bearing, range\] or null/); @@ -436,7 +435,6 @@ test('route: valid bearing values', function(assert) { var options = { coordinates: two_test_coordinates, bearings: [[200, 180], [250, 180]], - radiuses: [null, null], }; osrm.route(options, function(err, route) { assert.ifError(err); @@ -450,49 +448,38 @@ test('route: valid bearing values', function(assert) { }); test('route: invalid bearing values', function(assert) { - assert.plan(7); + assert.plan(6); var osrm = new OSRM(monaco_path); assert.throws(function() { osrm.route({ coordinates: two_test_coordinates, bearings: [[400, 180], [-250, 180]], - radiuses: [null, null], }, function(err, route) {}) }, /Bearing values need to be in range 0..360, 0..180/); assert.throws(function() { osrm.route({ coordinates: two_test_coordinates, bearings: [[200], [250, 180]], - radiuses: [null, null], }, function(err, route) {}) }, /Bearing must be an array of/); assert.throws(function() { osrm.route({ coordinates: two_test_coordinates, bearings: [[400, 109], [100, 720]], - radiuses: [null, null], }, function(err, route) {}) }, /Bearing values need to be in range 0..360, 0..180/); assert.throws(function() { osrm.route({ coordinates: two_test_coordinates, bearings: 400, - radiuses: [null], }, function(err, route) {}) }, /Bearings must be an array of arrays of numbers/); assert.throws(function() { osrm.route({ coordinates: two_test_coordinates, bearings: [[100, 100]], - radiuses: [null, null], }, function(err, route) {}) }, /Bearings array must have the same length as coordinates array/); assert.throws(function() { osrm.route({ coordinates: two_test_coordinates, bearings: [Infinity, Infinity], - radiuses: [null, null], }, function(err, route) {}) }, /Bearing must be an array of \[bearing, range\] or null/); - assert.throws(function() { osrm.route({ - coordinates: two_test_coordinates, - bearings: [[0, 180], [0, 180]], - }, function(err, route) {}) }, - /Bearings must be accompanied with radiuses/); }); test('route: routes Monaco with hints', function(assert) { diff --git a/unit_tests/library/table.cpp b/unit_tests/library/table.cpp index 39047b11e..0fa27d9a1 100644 --- a/unit_tests/library/table.cpp +++ b/unit_tests/library/table.cpp @@ -387,34 +387,4 @@ BOOST_AUTO_TEST_CASE(test_table_serialiaze_fb_no_waypoints) BOOST_CHECK(fb->waypoints() == nullptr); } -void test_table_bearings_without_radius(bool use_json_only_api) -{ - using namespace osrm; - - auto osrm = getOSRM(OSRM_TEST_DATA_DIR "/ch/monaco.osrm"); - - TableParameters params; - params.coordinates.push_back(get_dummy_location()); - params.coordinates.push_back(get_dummy_location()); - params.bearings.push_back(engine::Bearing{100, 100}); - params.bearings.push_back(engine::Bearing{100, 100}); - - json::Object json_result; - const auto rc = run_table_json(osrm, params, json_result, use_json_only_api); - - BOOST_CHECK(rc == Status::Error); - const auto code = json_result.values.at("code").get().value; - BOOST_CHECK_EQUAL(code, "InvalidOptions"); - const auto message = json_result.values.at("message").get().value; - BOOST_CHECK_EQUAL(message, "Number of radiuses does not match number of bearings"); -} -BOOST_AUTO_TEST_CASE(test_table_bearings_without_radius_old_api) -{ - test_table_bearings_without_radius(true); -} -BOOST_AUTO_TEST_CASE(test_table_bearings_without_radius_new_api) -{ - test_table_bearings_without_radius(false); -} - BOOST_AUTO_TEST_SUITE_END() From e20ae3c149928ee597f26d13dc5101dd3493abbb Mon Sep 17 00:00:00 2001 From: whytro Date: Fri, 28 Apr 2023 02:46:32 +0900 Subject: [PATCH 10/11] Add support for radius requirement for bearings --- CHANGELOG.md | 2 ++ docs/http.md | 2 +- include/nodejs/node_osrm_support.hpp | 6 ++++++ src/engine/plugins/match.cpp | 6 ++++++ src/engine/plugins/nearest.cpp | 6 ++++++ src/engine/plugins/table.cpp | 6 ++++++ src/engine/plugins/viaroute.cpp | 6 ++++++ 7 files changed, 33 insertions(+), 1 deletion(-) diff --git a/CHANGELOG.md b/CHANGELOG.md index 83e1fab4e..332238a02 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -1,5 +1,7 @@ # Unreleased - Changes from 5.27.1 + - API: + - CHANGED: Require a `radius` parameter when using `bearings`. [#6572](https://github.com/Project-OSRM/osrm-backend/pull/6572) - Features - ADDED: Add support for a default_radius flag. [#6575](https://github.com/Project-OSRM/osrm-backend/pull/6575) - Build: diff --git a/docs/http.md b/docs/http.md index 07acc968a..2b6d094f6 100644 --- a/docs/http.md +++ b/docs/http.md @@ -31,7 +31,7 @@ To pass parameters to each location some options support an array-like encoding: | Option | Values | Description | |----------------|--------------------------------------------------------|-----------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------| -|bearings |`{bearing};{bearing}[;{bearing} ...]` |Limits the search to segments with given bearing in degrees towards true north in a clockwise direction. | +|bearings |`{bearing};{bearing}[;{bearing} ...]` |Limits the search to segments with given bearing in degrees towards true north in a clockwise direction. Requires a corresponding radius or set default_radius flag. | |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. | diff --git a/include/nodejs/node_osrm_support.hpp b/include/nodejs/node_osrm_support.hpp index 5296dc4ed..5f0bdf92c 100644 --- a/include/nodejs/node_osrm_support.hpp +++ b/include/nodejs/node_osrm_support.hpp @@ -536,6 +536,12 @@ inline bool argumentsToParameter(const Napi::CallbackInfo &args, return false; } + if (!obj.Has("radiuses") && default_radius.IsUndefined()) + { + ThrowError(args.Env(), "Bearings must be accompanied with radiuses or a default_radius must be set."); + return false; + } + auto bearings_array = bearings.As(); if (bearings_array.Length() != params->coordinates.size()) diff --git a/src/engine/plugins/match.cpp b/src/engine/plugins/match.cpp index 90517df18..81f6b551e 100644 --- a/src/engine/plugins/match.cpp +++ b/src/engine/plugins/match.cpp @@ -131,6 +131,12 @@ Status MatchPlugin::HandleRequest(const RoutingAlgorithmsInterface &algorithms, return Error("InvalidValue", "Invalid coordinate value.", result); } + if(!parameters.bearings.empty() && !default_radius.has_value() && parameters.radiuses.size() != parameters.bearings.size()) + { + return Error( + "InvalidOptions", "Number of radiuses does not match number of bearings", result); + } + if (max_radius_map_matching > 0 && std::any_of(parameters.radiuses.begin(), parameters.radiuses.end(), [&](const auto &radius) { diff --git a/src/engine/plugins/nearest.cpp b/src/engine/plugins/nearest.cpp index 671dbe3f2..aafcbaa1e 100644 --- a/src/engine/plugins/nearest.cpp +++ b/src/engine/plugins/nearest.cpp @@ -43,6 +43,12 @@ Status NearestPlugin::HandleRequest(const RoutingAlgorithmsInterface &algorithms return Error("InvalidOptions", "Only one input coordinate is supported", result); } + if(!params.bearings.empty() && !default_radius.has_value() && params.radiuses.size() != params.bearings.size()) + { + return Error( + "InvalidOptions", "Number of radiuses does not match number of bearings", result); + } + auto phantom_nodes = GetPhantomNodes(facade, params, params.number_of_results); if (phantom_nodes.front().size() == 0) diff --git a/src/engine/plugins/table.cpp b/src/engine/plugins/table.cpp index 451811305..89254a08f 100644 --- a/src/engine/plugins/table.cpp +++ b/src/engine/plugins/table.cpp @@ -44,6 +44,12 @@ Status TablePlugin::HandleRequest(const RoutingAlgorithmsInterface &algorithms, "InvalidOptions", "Number of bearings does not match number of coordinates", result); } + if(!params.bearings.empty() && !default_radius.has_value() && params.radiuses.size() != params.bearings.size()) + { + return Error( + "InvalidOptions", "Number of radiuses does not match number of bearings", result); + } + // Empty sources or destinations means the user wants all of them included, respectively // The ManyToMany routing algorithm we dispatch to below already handles this perfectly. const auto num_sources = diff --git a/src/engine/plugins/viaroute.cpp b/src/engine/plugins/viaroute.cpp index fe0727936..4757a7dbc 100644 --- a/src/engine/plugins/viaroute.cpp +++ b/src/engine/plugins/viaroute.cpp @@ -82,6 +82,12 @@ Status ViaRoutePlugin::HandleRequest(const RoutingAlgorithmsInterface &algorithm if (!CheckAlgorithms(route_parameters, algorithms, result)) return Status::Error; + if(!route_parameters.bearings.empty() && !default_radius.has_value() && route_parameters.radiuses.size() != route_parameters.bearings.size()) + { + return Error( + "InvalidOptions", "Number of radiuses does not match number of bearings", result); + } + const auto &facade = algorithms.GetFacade(); auto phantom_node_pairs = GetPhantomNodes(facade, route_parameters); if (phantom_node_pairs.size() != route_parameters.coordinates.size()) From 1b68000ad873683e6d2529d7597d7bdbc034ff60 Mon Sep 17 00:00:00 2001 From: Whytro Date: Fri, 28 Apr 2023 03:12:25 +0900 Subject: [PATCH 11/11] Apply formatting --- include/nodejs/node_osrm_support.hpp | 4 +++- src/engine/plugins/match.cpp | 3 ++- src/engine/plugins/nearest.cpp | 3 ++- src/engine/plugins/table.cpp | 3 ++- src/engine/plugins/viaroute.cpp | 3 ++- 5 files changed, 11 insertions(+), 5 deletions(-) diff --git a/include/nodejs/node_osrm_support.hpp b/include/nodejs/node_osrm_support.hpp index 5f0bdf92c..3a176ed40 100644 --- a/include/nodejs/node_osrm_support.hpp +++ b/include/nodejs/node_osrm_support.hpp @@ -538,7 +538,9 @@ inline bool argumentsToParameter(const Napi::CallbackInfo &args, if (!obj.Has("radiuses") && default_radius.IsUndefined()) { - ThrowError(args.Env(), "Bearings must be accompanied with radiuses or a default_radius must be set."); + ThrowError( + args.Env(), + "Bearings must be accompanied with radiuses or a default_radius must be set."); return false; } diff --git a/src/engine/plugins/match.cpp b/src/engine/plugins/match.cpp index 81f6b551e..fa1f64de5 100644 --- a/src/engine/plugins/match.cpp +++ b/src/engine/plugins/match.cpp @@ -131,7 +131,8 @@ Status MatchPlugin::HandleRequest(const RoutingAlgorithmsInterface &algorithms, return Error("InvalidValue", "Invalid coordinate value.", result); } - if(!parameters.bearings.empty() && !default_radius.has_value() && parameters.radiuses.size() != parameters.bearings.size()) + if (!parameters.bearings.empty() && !default_radius.has_value() && + parameters.radiuses.size() != parameters.bearings.size()) { return Error( "InvalidOptions", "Number of radiuses does not match number of bearings", result); diff --git a/src/engine/plugins/nearest.cpp b/src/engine/plugins/nearest.cpp index aafcbaa1e..cbea26c0c 100644 --- a/src/engine/plugins/nearest.cpp +++ b/src/engine/plugins/nearest.cpp @@ -43,7 +43,8 @@ Status NearestPlugin::HandleRequest(const RoutingAlgorithmsInterface &algorithms return Error("InvalidOptions", "Only one input coordinate is supported", result); } - if(!params.bearings.empty() && !default_radius.has_value() && params.radiuses.size() != params.bearings.size()) + if (!params.bearings.empty() && !default_radius.has_value() && + params.radiuses.size() != params.bearings.size()) { return Error( "InvalidOptions", "Number of radiuses does not match number of bearings", result); diff --git a/src/engine/plugins/table.cpp b/src/engine/plugins/table.cpp index 89254a08f..74395ff3a 100644 --- a/src/engine/plugins/table.cpp +++ b/src/engine/plugins/table.cpp @@ -44,7 +44,8 @@ Status TablePlugin::HandleRequest(const RoutingAlgorithmsInterface &algorithms, "InvalidOptions", "Number of bearings does not match number of coordinates", result); } - if(!params.bearings.empty() && !default_radius.has_value() && params.radiuses.size() != params.bearings.size()) + if (!params.bearings.empty() && !default_radius.has_value() && + params.radiuses.size() != params.bearings.size()) { return Error( "InvalidOptions", "Number of radiuses does not match number of bearings", result); diff --git a/src/engine/plugins/viaroute.cpp b/src/engine/plugins/viaroute.cpp index 4757a7dbc..b4931c5c6 100644 --- a/src/engine/plugins/viaroute.cpp +++ b/src/engine/plugins/viaroute.cpp @@ -82,7 +82,8 @@ Status ViaRoutePlugin::HandleRequest(const RoutingAlgorithmsInterface &algorithm if (!CheckAlgorithms(route_parameters, algorithms, result)) return Status::Error; - if(!route_parameters.bearings.empty() && !default_radius.has_value() && route_parameters.radiuses.size() != route_parameters.bearings.size()) + if (!route_parameters.bearings.empty() && !default_radius.has_value() && + route_parameters.radiuses.size() != route_parameters.bearings.size()) { return Error( "InvalidOptions", "Number of radiuses does not match number of bearings", result);