From 09ff160dccd95c0cde2fb8343b0bf18e2108016c Mon Sep 17 00:00:00 2001 From: whytro Date: Fri, 28 Apr 2023 02:00:54 +0900 Subject: [PATCH] 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()