From 01ca32c81c36699c1d21f2945e4d8b50cf603035 Mon Sep 17 00:00:00 2001 From: Kajari Ghosh Date: Mon, 10 Dec 2018 14:53:30 -0500 Subject: [PATCH] Fix fallback speed validity checks (#5300) * fix fallback_speeds check to only accept values > 0 * add invalid_fallback_speed --- features/testbot/distance_matrix.feature | 65 +++++++++------------ features/testbot/duration_matrix.feature | 73 +++++++++++++++++------- include/engine/api/table_parameters.hpp | 4 +- include/nodejs/node_osrm_support.hpp | 2 +- include/util/typedefs.hpp | 1 + src/engine/plugins/table.cpp | 4 +- src/server/service/table_service.cpp | 2 +- test/nodejs/table.js | 23 ++++++++ unit_tests/server/parameters_parser.cpp | 58 ++++++++++++++++--- 9 files changed, 157 insertions(+), 75 deletions(-) diff --git a/features/testbot/distance_matrix.feature b/features/testbot/distance_matrix.feature index f48db123d..399698ad6 100644 --- a/features/testbot/distance_matrix.feature +++ b/features/testbot/distance_matrix.feature @@ -596,7 +596,6 @@ Feature: Basic Distance Matrix | e | 198.8 | 298.9 | 499 | 710.3 | 0 | 1592.8 | | f | 596.4 | 696.5 | 896.6 | 1107.9 | 397.6 | 0 | - Scenario: Testbot - Filling in noroutes with estimates (defaults to input coordinate location) Given a grid size of 300 meters Given the extract extra arguments "--small-component-size 4" @@ -620,7 +619,18 @@ Feature: Basic Distance Matrix | f | 900.7 | 600.5 | 0 | 302.2 | | 1 | 1501.1 | 1200.9 | 300.2 | 0 | - Scenario: Testbot - Filling in noroutes with estimates - use input coordinate + When I request a travel distance matrix I should get + | | a | b | f | 1 | + | a | 0 | 300.2 | 900.7 | 1501.1 | + + When I request a travel distance matrix I should get + | | a | + | a | 0 | + | b | 300.2 | + | f | 900.7 | + | 1 | 1501.1 | + + Scenario: Testbot - Fise input coordinate Given a grid size of 300 meters Given the extract extra arguments "--small-component-size 4" Given the query options @@ -644,6 +654,18 @@ Feature: Basic Distance Matrix | f | 900.7 | 600.5 | 0 | 302.2 | | 1 | 1501.1 | 1200.9 | 300.2 | 0 | + When I request a travel distance matrix I should get + | | a | b | f | 1 | + | a | 0 | 300.2 | 900.7 | 1501.1 | + + When I request a travel distance matrix I should get + | | a | + | a | 0 | + | b | 300.2 | + | f | 900.7 | + | 1 | 1501.1 | + + Scenario: Testbot - Filling in noroutes with estimates - use snapped coordinate Given a grid size of 300 meters Given the extract extra arguments "--small-component-size 4" @@ -668,22 +690,9 @@ Feature: Basic Distance Matrix | f | 900.7 | 600.5 | 0 | 302.2 | | 1 | 1200.9 | 900.7 | 300.2 | 0 | - Scenario: Testbot - Asymetric fallback_speed - more sources than destinations - Given a grid size of 300 meters - Given the extract extra arguments "--small-component-size 4" - Given the query options - | fallback_speed | 5 | - | fallback_coordinate | snapped | - Given the node map - """ - a b f h 1 - d e g i - """ - - And the ways - | nodes | - | abeda | - | fhigf | + When I request a travel distance matrix I should get + | | a | b | f | 1 | + | a | 0 | 300.2 | 900.7 | 1200.9 | When I request a travel distance matrix I should get | | a | @@ -692,23 +701,3 @@ Feature: Basic Distance Matrix | f | 900.7 | | 1 | 1200.9 | - Scenario: Testbot - Asymetric fallback_speed - more destinations than sources - Given a grid size of 300 meters - Given the extract extra arguments "--small-component-size 4" - Given the query options - | fallback_speed | 5 | - | fallback_coordinate | snapped | - Given the node map - """ - a b f h 1 - d e g i - """ - - And the ways - | nodes | - | abeda | - | fhigf | - - When I request a travel distance matrix I should get - | | a | b | f | 1 | - | a | 0 | 300.2 | 900.7 | 1200.9 | diff --git a/features/testbot/duration_matrix.feature b/features/testbot/duration_matrix.feature index 9e2b3f05b..4143e61b0 100644 --- a/features/testbot/duration_matrix.feature +++ b/features/testbot/duration_matrix.feature @@ -534,6 +534,17 @@ Feature: Basic Duration Matrix | f | 18 | 12 | 0 | 30 | | 1 | 30 | 24 | 30 | 0 | + When I request a travel time matrix I should get + | | a | b | f | 1 | + | a | 0 | 30 | 18 | 30 | + + When I request a travel time matrix I should get + | | a | + | a | 0 | + | b | 30 | + | f | 18 | + | 1 | 30 | + Scenario: Testbot - Filling in noroutes with estimates - use input coordinate Given a grid size of 300 meters Given the extract extra arguments "--small-component-size 4" @@ -558,6 +569,17 @@ Feature: Basic Duration Matrix | f | 18 | 12 | 0 | 30 | | 1 | 30 | 24 | 30 | 0 | + When I request a travel time matrix I should get + | | a | b | f | 1 | + | a | 0 | 30 | 18 | 30 | + + When I request a travel time matrix I should get + | | a | + | a | 0 | + | b | 30 | + | f | 18 | + | 1 | 30 | + Scenario: Testbot - Filling in noroutes with estimates - use snapped coordinate Given a grid size of 300 meters Given the extract extra arguments "--small-component-size 4" @@ -582,25 +604,33 @@ Feature: Basic Duration Matrix | f | 18 | 12 | 0 | 30 | | 1 | 24 | 18 | 30 | 0 | + When I request a travel time matrix I should get + | | a | b | f | 1 | + | a | 0 | 30 | 18 | 24 | + + When I request a travel time matrix I should get + | | a | + | a | 0 | + | b | 30 | + | f | 18 | + | 1 | 24 | + + Scenario: Testbot - Travel time matrix of minimal network with scale factor Given the query options | scale_factor | 2 | - - Given the node map + Given the node map """ a b """ - - And the ways + And the ways | nodes | | ab | - - When I request a travel time matrix I should get + When I request a travel time matrix I should get | | a | b | | a | 0 | 20 | | b | 20 | 0 | - - Scenario: Testbot - Test fallback speeds and scale factor + Scenario: Testbot - Test fallback speeds and scale factor Given a grid size of 300 meters Given the extract extra arguments "--small-component-size 4" Given the query options @@ -608,68 +638,67 @@ Feature: Basic Duration Matrix | fallback_speed | 5 | | fallback_coordinate | snapped | - Given the node map + Given the node map """ a b f h 1 d e g i """ - And the ways + And the ways | nodes | | abeda | | fhigf | - When I request a travel time matrix I should get + When I request a travel time matrix I should get | | a | b | f | 1 | | a | 0 | 60 | 36 | 48 | | b | 60 | 0 | 24 | 36 | | f | 36 | 24 | 0 | 60 | | 1 | 48 | 36 | 60 | 0 | - When I request a travel time matrix I should get + When I request a travel time matrix I should get | | a | b | f | 1 | | a | 0 | 60 | 36 | 48 | - When I request a travel time matrix I should get + When I request a travel time matrix I should get | | a | | a | 0 | | b | 60 | | f | 36 | | 1 | 48 | - - Scenario: Testbot - Travel time matrix of minimal network with overflow scale factor + Scenario: Testbot - Travel time matrix of minimal network with overflow scale factor Given the query options | scale_factor | 2147483647 | - Given the node map + Given the node map """ a b """ - And the ways + And the ways | nodes | | ab | - When I request a travel time matrix I should get + When I request a travel time matrix I should get | | a | b | | a | 0 | 214748364.6 | | b | 214748364.6 | 0 | - Scenario: Testbot - Travel time matrix of minimal network with fraction scale factor + Scenario: Testbot - Travel time matrix of minimal network with fraction scale factor Given the query options | scale_factor | 0.5 | - Given the node map + Given the node map """ a b """ - And the ways + And the ways | nodes | | ab | - When I request a travel time matrix I should get + When I request a travel time matrix I should get | | a | b | | a | 0 | 5 | | b | 5 | 0 | diff --git a/include/engine/api/table_parameters.hpp b/include/engine/api/table_parameters.hpp index 8cb1fe7d7..fbbf6831e 100644 --- a/include/engine/api/table_parameters.hpp +++ b/include/engine/api/table_parameters.hpp @@ -59,7 +59,7 @@ struct TableParameters : public BaseParameters { std::vector sources; std::vector destinations; - double fallback_speed = 0; + double fallback_speed = INVALID_FALLBACK_SPEED; enum class FallbackCoordinateType { @@ -137,7 +137,7 @@ struct TableParameters : public BaseParameters if (std::any_of(begin(destinations), end(destinations), not_in_range)) return false; - if (fallback_speed < 0) + if (fallback_speed <= 0) return false; if (scale_factor <= 0) diff --git a/include/nodejs/node_osrm_support.hpp b/include/nodejs/node_osrm_support.hpp index a7c38a071..4ff697576 100644 --- a/include/nodejs/node_osrm_support.hpp +++ b/include/nodejs/node_osrm_support.hpp @@ -1192,7 +1192,7 @@ argumentsToTableParameter(const Nan::FunctionCallbackInfo &args, Nan::ThrowError("fallback_speed must be a number"); return table_parameters_ptr(); } - else if (fallback_speed->NumberValue() < 0) + else if (fallback_speed->NumberValue() <= 0) { Nan::ThrowError("fallback_speed must be > 0"); return table_parameters_ptr(); diff --git a/include/util/typedefs.hpp b/include/util/typedefs.hpp index afe8350b4..d647a7db7 100644 --- a/include/util/typedefs.hpp +++ b/include/util/typedefs.hpp @@ -116,6 +116,7 @@ static const EdgeDuration MAXIMAL_EDGE_DURATION = std::numeric_limits::max(); static const TurnPenalty INVALID_TURN_PENALTY = std::numeric_limits::max(); static const EdgeDistance INVALID_EDGE_DISTANCE = std::numeric_limits::max(); +static const EdgeDistance INVALID_FALLBACK_SPEED = std::numeric_limits::max(); // FIXME the bitfields we use require a reduced maximal duration, this should be kept consistent // within the code base. For now we have to ensure that we don't case 30 bit to -1 and break any diff --git a/src/engine/plugins/table.cpp b/src/engine/plugins/table.cpp index 4b4dfc6dc..8dc765168 100644 --- a/src/engine/plugins/table.cpp +++ b/src/engine/plugins/table.cpp @@ -96,7 +96,7 @@ Status TablePlugin::HandleRequest(const RoutingAlgorithmsInterface &algorithms, } // Scan table for null results - if any exist, replace with distance estimates - if (params.fallback_speed > 0 || params.scale_factor > 0) + if (params.fallback_speed != INVALID_FALLBACK_SPEED || params.scale_factor > 0) { for (std::size_t row = 0; row < num_sources; row++) { @@ -104,7 +104,7 @@ Status TablePlugin::HandleRequest(const RoutingAlgorithmsInterface &algorithms, { const auto &table_index = row * num_destinations + column; BOOST_ASSERT(table_index < result_tables_pair.first.size()); - if (params.fallback_speed > 0 && + if (params.fallback_speed != INVALID_FALLBACK_SPEED && params.fallback_speed > 0 && result_tables_pair.first[table_index] == MAXIMAL_EDGE_DURATION) { const auto &source = diff --git a/src/server/service/table_service.cpp b/src/server/service/table_service.cpp index cf0213b01..ad5f16ab1 100644 --- a/src/server/service/table_service.cpp +++ b/src/server/service/table_service.cpp @@ -56,7 +56,7 @@ std::string getWrongOptionHelp(const engine::api::TableParameters ¶meters) help = "Number of coordinates needs to be at least two."; } - if (parameters.fallback_speed < 0) + if (parameters.fallback_speed <= 0) { help = "fallback_speed must be > 0"; } diff --git a/test/nodejs/table.js b/test/nodejs/table.js index 5c567fff9..a6b0884aa 100644 --- a/test/nodejs/table.js +++ b/test/nodejs/table.js @@ -260,6 +260,29 @@ tables.forEach(function(annotation) { }); }); + test('table: ' + annotation + ' table in Monaco with invalid fallback speeds and fallback coordinates', function(assert) { + assert.plan(4); + var osrm = new OSRM({path: mld_data_path, algorithm: 'MLD'}); + var options = { + coordinates: two_test_coordinates, + annotations: [annotation.slice(0,-1)], + fallback_speed: -1 + }; + + assert.throws(()=>osrm.table(options, (err, res) => {}), /fallback_speed must be > 0/, "should throw on invalid fallback_speeds"); + + options.fallback_speed = '10'; + assert.throws(()=>osrm.table(options, (err, res) => {}), /fallback_speed must be a number/, "should throw on invalid fallback_speeds"); + + options.fallback_speed = 10; + options.fallback_coordinate = 'bla'; + assert.throws(()=>osrm.table(options, (err, res) => {}), /fallback_coordinate' param must be one of \[input, snapped\]/, "should throw on invalid fallback_coordinate"); + + options.fallback_coordinate = 10; + assert.throws(()=>osrm.table(options, (err, res) => {}), /fallback_coordinate must be a string: \[input, snapped\]/, "should throw on invalid fallback_coordinate"); + + }); + test('table: ' + annotation + ' table in Monaco with invalid scale factor', function(assert) { assert.plan(3); var osrm = new OSRM({path: mld_data_path, algorithm: 'MLD'}); diff --git a/unit_tests/server/parameters_parser.cpp b/unit_tests/server/parameters_parser.cpp index 9367dfbad..42a4284b2 100644 --- a/unit_tests/server/parameters_parser.cpp +++ b/unit_tests/server/parameters_parser.cpp @@ -91,10 +91,23 @@ BOOST_AUTO_TEST_CASE(invalid_table_urls) 49UL); BOOST_CHECK_EQUAL(testInvalidOptions("1,2;3,4?fallback_coordinate=asdf"), 28UL); + BOOST_CHECK_EQUAL(testInvalidOptions("1,2;3,4?fallback_coordinate=10"), 28UL); BOOST_CHECK_EQUAL( testInvalidOptions("1,2;3,4?annotations=durations&scale_factor=-1"), 28UL); BOOST_CHECK_EQUAL( testInvalidOptions("1,2;3,4?annotations=durations&scale_factor=0"), 28UL); + BOOST_CHECK_EQUAL( + testInvalidOptions("1,2;3,4?annotations=durations&fallback_speed=0"), + 28UL); + BOOST_CHECK_EQUAL( + testInvalidOptions("1,2;3,4?annotations=durations&fallback_speed=-1"), + 28UL); + BOOST_CHECK_EQUAL( + testInvalidOptions("1,2;3,4?annotations=durations&fallback_speed=0"), + 28UL); + BOOST_CHECK_EQUAL( + testInvalidOptions("1,2;3,4?annotations=durations&fallback_speed=-1"), + 28UL); } BOOST_AUTO_TEST_CASE(valid_route_hint) @@ -570,16 +583,43 @@ BOOST_AUTO_TEST_CASE(valid_table_urls) CHECK_EQUAL_RANGE(reference_7.sources, result_7->sources); CHECK_EQUAL_RANGE(reference_7.destinations, result_7->destinations); - auto result_8 = parseParameters("1,2;3,4?sources=all&destinations=all&" - "annotations=duration&fallback_speed=1&" - "fallback_coordinate=snapped&scale_factor=2"); + TableParameters reference_8{}; + reference_8.coordinates = coords_1; + auto result_8 = + parseParameters("1,2;3,4?annotations=distance&fallback_speed=2.5"); BOOST_CHECK(result_8); - CHECK_EQUAL_RANGE(reference_1.sources, result_3->sources); - CHECK_EQUAL_RANGE(reference_1.destinations, result_3->destinations); - CHECK_EQUAL_RANGE(reference_1.bearings, result_3->bearings); - CHECK_EQUAL_RANGE(reference_1.radiuses, result_3->radiuses); - CHECK_EQUAL_RANGE(reference_1.approaches, result_3->approaches); - CHECK_EQUAL_RANGE(reference_1.coordinates, result_3->coordinates); + BOOST_CHECK_EQUAL(result_8->annotations & TableParameters::AnnotationsType::Distance, true); + CHECK_EQUAL_RANGE(reference_8.sources, result_8->sources); + CHECK_EQUAL_RANGE(reference_8.destinations, result_8->destinations); + + TableParameters reference_9{}; + reference_9.coordinates = coords_1; + auto result_9 = parseParameters( + "1,2;3,4?annotations=distance&fallback_speed=2.5&fallback_coordinate=input"); + BOOST_CHECK(result_9); + BOOST_CHECK_EQUAL(result_9->annotations & TableParameters::AnnotationsType::Distance, true); + CHECK_EQUAL_RANGE(reference_9.sources, result_9->sources); + CHECK_EQUAL_RANGE(reference_9.destinations, result_9->destinations); + + TableParameters reference_10{}; + reference_10.coordinates = coords_1; + auto result_10 = parseParameters( + "1,2;3,4?annotations=distance&fallback_speed=20&fallback_coordinate=snapped"); + BOOST_CHECK(result_10); + BOOST_CHECK_EQUAL(result_10->annotations & TableParameters::AnnotationsType::Distance, true); + CHECK_EQUAL_RANGE(reference_10.sources, result_10->sources); + CHECK_EQUAL_RANGE(reference_10.destinations, result_10->destinations); + + auto result_11 = parseParameters("1,2;3,4?sources=all&destinations=all&" + "annotations=duration&fallback_speed=1&" + "fallback_coordinate=snapped&scale_factor=2"); + BOOST_CHECK(result_11); + CHECK_EQUAL_RANGE(reference_1.sources, result_11->sources); + CHECK_EQUAL_RANGE(reference_1.destinations, result_11->destinations); + CHECK_EQUAL_RANGE(reference_1.bearings, result_11->bearings); + CHECK_EQUAL_RANGE(reference_1.radiuses, result_11->radiuses); + CHECK_EQUAL_RANGE(reference_1.approaches, result_11->approaches); + CHECK_EQUAL_RANGE(reference_1.coordinates, result_11->coordinates); } BOOST_AUTO_TEST_CASE(valid_match_urls)