Enforce constraints and add tests.

danpat · danpat · commit 0a6ef581385d · 2016-09-07T18:17:49.000-07:00
diff --git a/include/engine/bearing.hpp b/include/engine/bearing.hpp
@@ -38,7 +38,7 @@ struct Bearing
     short bearing;
     short range;
 
-    bool IsValid() const { return bearing >= 0 && bearing <= 360 && range >= 0 && range <= 180; }
+    bool IsValid() const { return bearing >= 0 && bearing <= 360 && range > 0 && range <= 180; }
 };
 
 inline bool operator==(const Bearing lhs, const Bearing rhs)
diff --git a/include/engine/geospatial_query.hpp b/include/engine/geospatial_query.hpp
@@ -71,6 +71,7 @@ template <typename RTreeT, typename DataFacadeT> class GeospatialQuery
                                const int bearing,
                                const int bearing_range) const
     {
+        if (bearing_range == 0) return {};
         auto results = rtree.Nearest(
             input_coordinate,
             [this, bearing, bearing_range, max_distance](const CandidateSegment &segment) {
@@ -93,6 +94,7 @@ template <typename RTreeT, typename DataFacadeT> class GeospatialQuery
                         const int bearing,
                         const int bearing_range) const
     {
+        if (bearing_range == 0) return {};
         auto results = rtree.Nearest(
             input_coordinate,
             [this, bearing, bearing_range](const CandidateSegment &segment) {
@@ -116,6 +118,7 @@ template <typename RTreeT, typename DataFacadeT> class GeospatialQuery
                         const int bearing,
                         const int bearing_range) const
     {
+        if (bearing_range == 0) return {};
         auto results = rtree.Nearest(
             input_coordinate,
             [this, bearing, bearing_range](const CandidateSegment &segment) {
@@ -136,6 +139,7 @@ template <typename RTreeT, typename DataFacadeT> class GeospatialQuery
     std::vector<PhantomNodeWithDistance>
     NearestPhantomNodes(const util::Coordinate input_coordinate, const unsigned max_results) const
     {
+        if (max_results == 0) return {};
         auto results =
             rtree.Nearest(input_coordinate,
                           [this](const CandidateSegment &segment) { return HasValidEdge(segment); },
@@ -153,6 +157,7 @@ template <typename RTreeT, typename DataFacadeT> class GeospatialQuery
                         const unsigned max_results,
                         const double max_distance) const
     {
+        if (max_results == 0) return {};
         auto results =
             rtree.Nearest(input_coordinate,
                           [this](const CandidateSegment &segment) { return HasValidEdge(segment); },
@@ -251,6 +256,7 @@ template <typename RTreeT, typename DataFacadeT> class GeospatialQuery
     std::pair<PhantomNode, PhantomNode> NearestPhantomNodeWithAlternativeFromBigComponent(
         const util::Coordinate input_coordinate, const int bearing, const int bearing_range) const
     {
+        if (bearing_range == 0) return {};
         bool has_small_component = false;
         bool has_big_component = false;
         auto results = rtree.Nearest(
@@ -298,6 +304,7 @@ template <typename RTreeT, typename DataFacadeT> class GeospatialQuery
                                                       const int bearing,
                                                       const int bearing_range) const
     {
+        if (bearing_range == 0) return {};
         bool has_small_component = false;
         bool has_big_component = false;
         auto results = rtree.Nearest(
diff --git a/src/engine/plugins/nearest.cpp b/src/engine/plugins/nearest.cpp
@@ -44,14 +44,22 @@ Status NearestPlugin::HandleRequest(const api::NearestParameters &params,
     // We want to keep the user-interface as is, so make a copy. Should be cheap.
     auto constraint_params = params;
 
-    const auto max_radius_m = static_cast<double>(max_radius);
+    if (max_radius != -1)
+    {
+        const auto max_radius_m = static_cast<double>(max_radius);
+
+        if (constraint_params.radiuses.empty())
+            constraint_params.radiuses.push_back(boost::make_optional(max_radius_m));
+        else if (constraint_params.radiuses[0])
+            boost::algorithm::clamp(*constraint_params.radiuses[0], 0., max_radius_m);
+    }
 
-    if (constraint_params.radiuses.empty())
-        constraint_params.radiuses.push_back(boost::make_optional(max_radius_m));
-    else if (constraint_params.radiuses[0])
-        boost::algorithm::clamp(*constraint_params.radiuses[0], 0., max_radius_m);
+    if (max_results != -1)
+    {
+        constraint_params.number_of_results = boost::algorithm::clamp(params.number_of_results, 0, max_results);
+    }
 
-    auto phantom_nodes = GetPhantomNodes(constraint_params, params.number_of_results);
+    auto phantom_nodes = GetPhantomNodes(constraint_params, constraint_params.number_of_results);
 
     if (phantom_nodes.front().size() == 0)
     {
diff --git a/unit_tests/library/fixture.hpp b/unit_tests/library/fixture.hpp
@@ -8,14 +8,20 @@
 
 // I couldn't get Boost.UnitTest to provide a test suite level fixture with custom
 // arguments per test suite (osrm base path from argv), so this has to suffice.
-
-inline osrm::OSRM getOSRM(const std::string &base_path)
+inline osrm::OSRM getOSRM(const std::string &base_path, osrm::EngineConfig &config)
 {
-    osrm::EngineConfig config;
     config.storage_config = {base_path};
     config.use_shared_memory = false;
 
     return osrm::OSRM{config};
 }
 
+inline osrm::OSRM getOSRM(const std::string &base_path)
+{
+    osrm::EngineConfig config;
+
+    return getOSRM(base_path, config);
+}
+
+
 #endif
diff --git a/unit_tests/library/nearest.cpp b/unit_tests/library/nearest.cpp
@@ -43,6 +43,67 @@ BOOST_AUTO_TEST_CASE(test_nearest_response)
     }
 }
 
+BOOST_AUTO_TEST_CASE(test_nearest_response_max_results)
+{
+    const auto args = get_args();
+    osrm::EngineConfig config;
+    config.max_results_nearest = 2;
+    // Default radius is unlimited
+    auto osrm = getOSRM(args.at(0), config);
+
+    using namespace osrm;
+
+    NearestParameters params;
+    params.number_of_results = 10;
+    params.coordinates.push_back(get_dummy_location());
+
+    json::Object result;
+    const auto rc = osrm.Nearest(params, result);
+    BOOST_REQUIRE(rc == Status::Ok);
+
+    const auto code = result.values.at("code").get<json::String>().value;
+    BOOST_CHECK_EQUAL(code, "Ok");
+
+    const auto &waypoints = result.values.at("waypoints").get<json::Array>().values;
+    BOOST_CHECK(waypoints.size() == 2); // make sure our results were capped at max_results_nearest
+
+    // Test that we get nothing when we restrict to no matches
+    config.max_results_nearest = 0;
+    osrm = getOSRM(args.at(0), config);
+
+    json::Object result2;
+    const auto rc2 = osrm.Nearest(params, result2);
+    BOOST_REQUIRE(rc2 == Status::Error);
+    const auto code2 = result2.values.at("code").get<json::String>().value;
+    BOOST_CHECK_EQUAL(code2, "NoSegment");
+}
+
+BOOST_AUTO_TEST_CASE(test_nearest_response_max_radius)
+{
+    const auto args = get_args();
+    osrm::EngineConfig config;
+    config.max_radius_nearest = 1;
+    auto osrm = getOSRM(args.at(0), config);
+
+    using namespace osrm;
+
+    NearestParameters params;
+    params.number_of_results = 10;
+    params.coordinates.push_back(get_dummy_location());
+
+    json::Object result;
+    const auto rc = osrm.Nearest(params, result);
+    BOOST_REQUIRE(rc == Status::Ok);
+
+    const auto code = result.values.at("code").get<json::String>().value;
+    BOOST_CHECK_EQUAL(code, "Ok");
+
+    const auto &waypoints = result.values.at("waypoints").get<json::Array>().values;
+    BOOST_CHECK(waypoints.size() == 1); // Check that we only got 1 match within 1m, even though
+                                        // we asked for 10 results.
+}
+
+
 BOOST_AUTO_TEST_CASE(test_nearest_response_no_coordinates)
 {
     const auto args = get_args();
diff --git a/unit_tests/util/static_rtree.cpp b/unit_tests/util/static_rtree.cpp
@@ -394,6 +394,11 @@ BOOST_AUTO_TEST_CASE(bearing_tests)
         BOOST_CHECK_EQUAL(results.back().phantom_node.reverse_segment_id.id, 1);
     }
 
+    {
+        auto results = query.NearestPhantomNodes(input, 0);
+        BOOST_CHECK_EQUAL(results.size(), 0);
+    }
+
     {
         auto results = query.NearestPhantomNodes(input, 5, 270, 10);
         BOOST_CHECK_EQUAL(results.size(), 0);
@@ -412,6 +417,16 @@ BOOST_AUTO_TEST_CASE(bearing_tests)
         BOOST_CHECK_EQUAL(results[1].phantom_node.reverse_segment_id.id, 1);
     }
 
+    {
+        auto results = query.NearestPhantomNodes(input, 0, 45, 10);
+        BOOST_CHECK_EQUAL(results.size(), 0);
+    }
+
+    {
+        auto results = query.NearestPhantomNodes(input, 5, 45, 0);
+        BOOST_CHECK_EQUAL(results.size(), 0);
+    }
+
     {
         auto results = query.NearestPhantomNodesInRange(input, 11000);
         BOOST_CHECK_EQUAL(results.size(), 2);
@@ -434,6 +449,12 @@ BOOST_AUTO_TEST_CASE(bearing_tests)
         BOOST_CHECK(results[1].phantom_node.reverse_segment_id.enabled);
         BOOST_CHECK_EQUAL(results[1].phantom_node.reverse_segment_id.id, 1);
     }
+
+    {
+        auto results = query.NearestPhantomNodesInRange(input, 11000, 45, 0);
+        BOOST_CHECK_EQUAL(results.size(), 0);
+    }
+
 }
 
 BOOST_AUTO_TEST_CASE(bbox_search_tests)
