Update changelog for issue #2896 fix.

Test checks that osrm-extract terminates for
http://www.openstreetmap.org/way/198481519

.travis.yml

@@ -13,6 +13,7 @@ notifications:
 branches:
   only:
     - master
+    - 5.4
 
 cache:
   ccache: true

CHANGELOG.md

@@ -20,6 +20,10 @@
       - Fixed an issue that could emit `invalid` as instruction when ending on a sliproad after a traffic-light
       - Fixed an issue that would detect turning circles as sliproads
       - Fixed a bug where post-processing instructions (e.g. left + left -> uturn) could result in false pronunciations
+      - Fixes a bug where a bearing range of zero would cause exhaustive graph traversals
+      - Fixes a bug where certain looped geometries could cause an infinite loop during extraction
+    - Infrastructure:
+      - Adds a feature to limit results in nearest service with a default of 100 in `osrm-routed`
 
 # 5.3.0
   Changes from 5.3.0-rc.3

CMakeLists.txt

@@ -9,7 +9,7 @@ endif()
 
 project(OSRM C CXX)
 set(OSRM_VERSION_MAJOR 5)
-set(OSRM_VERSION_MINOR 3)
+set(OSRM_VERSION_MINOR 4)
 set(OSRM_VERSION_PATCH 0)
 
 # these two functions build up custom variables:

docs/http.md

@@ -30,11 +30,11 @@ http://{server}/{service}/{version}/{profile}/{coordinates}[.{format}]?option=va
   
     | Service     |           Description                                     |
     |-------------|-----------------------------------------------------------|
-    | [`route`](#service-route)     | shortest path between given coordinates                   |
+    | [`route`](#service-route)     | fastest path between given coordinates                   |
     | [`nearest`](#service-nearest)   | returns the nearest street segment for a given coordinate |
     | [`table`](#service-table)     | computes distance tables for given coordinates            |
     | [`match`](#service-match)     | matches given coordinates to the road network             |
-    | [`trip`](#service-trip)      | Compute the shortest round trip between given coordinates |
+    | [`trip`](#service-trip)      | Compute the fastest round trip between given coordinates |
     | [`tile`](#service-tile)      | Return vector tiles containing debugging info             |
   
 - `version`: Version of the protocol implemented by the service.
@@ -302,7 +302,7 @@ All other fields might be undefined.
 ## Service `trip`
 
 The trip plugin solves the Traveling Salesman Problem using a greedy heuristic (farthest-insertion algorithm).
-The returned path does not have to be the shortest path, as TSP is NP-hard it is only an approximation.
+The returned path does not have to be the fastest path, as TSP is NP-hard it is only an approximation.
 Note that if the input coordinates can not be joined by a single trip (e.g. the coordinates are on several disconnected islands)
 multiple trips for each connected component are returned.
 

features/car/names.feature

@@ -35,6 +35,23 @@ Feature: Car - Street names in instructions
             | a    | d  | My Way,My Way      | ,meyeway             | ,A1   |
             | 1    | c  | Your Way,Your Way  | yourewaye,yourewaye  | ,     |
 
+    # See #2860
+    Scenario: Car - same street name but different pronunciation
+        Given the node map
+            | a | b | c |
+            |   | d |   |
+            |   | e |   |
+
+        And the ways
+            | nodes | name       | name:pronunciation |
+            | abc   | Houston St | hew-stun           |
+            | bde   | Houston St | how-stun           |
+
+        When I route I should get
+            | from | to | route                            | pronunciations             |
+            | a    | c  | Houston St,Houston St            | hew-stun,hew-stun          |
+            | a    | e  | Houston St,Houston St,Houston St | hew-stun,how-stun,how-stun |
+
     @todo
     Scenario: Car - Use way type to describe unnamed ways
         Given the node map

features/guidance/advanced-lanes.feature

@@ -218,3 +218,19 @@ Feature: Turn Lane Guidance
             | a,h       | ghough,market,market | depart,turn slight right,arrive    | ,none:false straight:false straight:true straight:true,  |
             | a,j       | ghough,market,market | depart,turn left,arrive            | ,none:true straight:false straight:false straight:false, |
             | a,f       | ghough,ghough,ghough | depart,continue slight left,arrive | ,none:true straight:true straight:false straight:false,  |
+
+    Scenario: Check sliproad handler loop's exit condition, Issue #2896
+      # http://www.openstreetmap.org/way/198481519
+        Given the node locations
+            | node | lat        | lon         |
+            | a    | 7.6125350  | 126.5708309 |
+            | b    | 7.6125156  | 126.5707219 |
+            | c    | 7.6125363  | 126.5708337 |
+
+        And the ways
+            | nodes | name |
+            | cbac  |      |
+
+        When I route I should get
+            | from | to | route | turns         |
+            | a    | c  | ,     | depart,arrive |

include/engine/engine_config.hpp

@@ -51,6 +51,7 @@ namespace engine
  *  - Route
  *  - Table
  *  - Match
+ *  - Nearest
  *
  * In addition, shared memory can be used for datasets loaded with osrm-datastore.
  *
@@ -65,6 +66,7 @@ struct EngineConfig final
     int max_locations_viaroute = -1;
     int max_locations_distance_table = -1;
     int max_locations_map_matching = -1;
+    int max_results_nearest = -1;
     bool use_shared_memory = true;
 };
 }

include/engine/plugins/nearest.hpp

@@ -15,9 +15,12 @@ namespace plugins
 class NearestPlugin final : public BasePlugin
 {
   public:
-    explicit NearestPlugin(datafacade::BaseDataFacade &facade);
+    explicit NearestPlugin(datafacade::BaseDataFacade &facade, const int max_results);
 
     Status HandleRequest(const api::NearestParameters &params, util::json::Object &result);
+
+  private:
+    const int max_results;
 };
 }
 }

include/extractor/extractor_callbacks.hpp

@@ -18,14 +18,17 @@ class Way;
 
 namespace std
 {
-template <> struct hash<std::tuple<std::string, std::string, std::string>>
+template <> struct hash<std::tuple<std::string, std::string, std::string, std::string>>
 {
-    std::size_t operator()(const std::tuple<std::string, std::string, std::string> &mk) const noexcept
+    std::size_t
+    operator()(const std::tuple<std::string, std::string, std::string, std::string> &mk) const
+        noexcept
     {
         std::size_t seed = 0;
         boost::hash_combine(seed, std::get<0>(mk));
         boost::hash_combine(seed, std::get<1>(mk));
         boost::hash_combine(seed, std::get<2>(mk));
+        boost::hash_combine(seed, std::get<3>(mk));
         return seed;
     }
 };
@@ -51,8 +54,9 @@ struct ExtractionWay;
 class ExtractorCallbacks
 {
   private:
-    // used to deduplicate street names and street destinations: actually maps to name ids
-    using MapKey = std::tuple<std::string, std::string, std::string>;
+    // used to deduplicate street names, refs, destinations, pronunciation: actually maps to name
+    // ids
+    using MapKey = std::tuple<std::string, std::string, std::string, std::string>;
     using MapVal = unsigned;
     std::unordered_map<MapKey, MapVal> string_map;
     guidance::LaneDescriptionMap lane_description_map;

include/util/bearing.hpp

@@ -67,7 +67,7 @@ inline bool CheckInBounds(const int A, const int B, const int range)
 
     if (range >= 180)
         return true;
-    if (range <= 0)
+    if (range < 0)
         return false;
 
     // Map both bearings into positive modulo 360 space

include/util/static_rtree.hpp

@@ -17,6 +17,7 @@
 #include <boost/assert.hpp>
 #include <boost/filesystem.hpp>
 #include <boost/filesystem/fstream.hpp>
+#include <boost/format.hpp>
 #include <boost/iostreams/device/mapped_file.hpp>
 
 #include <tbb/parallel_for.h>

src/engine/api/json_factory.cpp

@@ -235,7 +235,8 @@ util::json::Object makeRouteStep(guidance::RouteStep step, util::json::Value geo
     route_step.values["distance"] = std::round(step.distance * 10) / 10.;
     route_step.values["duration"] = std::round(step.duration * 10) / 10.;
     route_step.values["name"] = std::move(step.name);
-    if (!step.ref.empty()) route_step.values["ref"] = std::move(step.ref);
+    if (!step.ref.empty())
+        route_step.values["ref"] = std::move(step.ref);
     if (!step.pronunciation.empty())
         route_step.values["pronunciation"] = std::move(step.pronunciation);
     if (!step.destinations.empty())

src/engine/engine.cpp

@@ -146,7 +146,7 @@ Engine::Engine(EngineConfig &config)
 
     route_plugin = create<ViaRoutePlugin>(*query_data_facade, config.max_locations_viaroute);
     table_plugin = create<TablePlugin>(*query_data_facade, config.max_locations_distance_table);
-    nearest_plugin = create<NearestPlugin>(*query_data_facade);
+    nearest_plugin = create<NearestPlugin>(*query_data_facade, config.max_results_nearest);
     trip_plugin = create<TripPlugin>(*query_data_facade, config.max_locations_trip);
     match_plugin = create<MatchPlugin>(*query_data_facade, config.max_locations_map_matching);
     tile_plugin = create<TilePlugin>(*query_data_facade);

src/engine/engine_config.cpp

@@ -15,11 +15,15 @@ bool EngineConfig::IsValid() const
         storage_config.datasource_names_path.empty() &&
         storage_config.datasource_indexes_path.empty() && storage_config.names_data_path.empty();
 
-    const bool limits_valid =
-        (max_locations_distance_table == -1 || max_locations_distance_table > 2) &&
-        (max_locations_map_matching == -1 || max_locations_map_matching > 2) &&
-        (max_locations_trip == -1 || max_locations_trip > 2) &&
-        (max_locations_viaroute == -1 || max_locations_viaroute > 2);
+    const auto unlimited_or_more_than = [](const int v, const int limit) {
+        return v == -1 || v > limit;
+    };
+
+    const bool limits_valid = unlimited_or_more_than(max_locations_distance_table, 2) &&
+                              unlimited_or_more_than(max_locations_map_matching, 2) &&
+                              unlimited_or_more_than(max_locations_trip, 2) &&
+                              unlimited_or_more_than(max_locations_viaroute, 2) &&
+                              unlimited_or_more_than(max_results_nearest, 0);
 
     return ((use_shared_memory && all_path_are_empty) || storage_config.IsValid()) && limits_valid;
 }

src/engine/plugins/nearest.cpp

@@ -16,13 +16,24 @@ namespace engine
 namespace plugins
 {
 
-NearestPlugin::NearestPlugin(datafacade::BaseDataFacade &facade) : BasePlugin{facade} {}
+NearestPlugin::NearestPlugin(datafacade::BaseDataFacade &facade, const int max_results_)
+    : BasePlugin{facade}, max_results{max_results_}
+{
+}
 
 Status NearestPlugin::HandleRequest(const api::NearestParameters &params,
                                     util::json::Object &json_result)
 {
     BOOST_ASSERT(params.IsValid());
 
+    if (params.number_of_results > max_results)
+    {
+        return Error("TooBig",
+                     "Number of results " + std::to_string(params.number_of_results) +
+                         " is higher than current maximum (" + std::to_string(max_results) + ")",
+                     json_result);
+    }
+
     if (!CheckAllCoordinates(params.coordinates))
         return Error("InvalidOptions", "Coordinates are invalid", json_result);
 

src/extractor/extractor_callbacks.cpp

@@ -34,8 +34,8 @@ namespace TurnLaneType = guidance::TurnLaneType;
 ExtractorCallbacks::ExtractorCallbacks(ExtractionContainers &extraction_containers)
     : external_memory(extraction_containers)
 {
-    // we reserved 0, 1, 2 for the empty case
-    string_map[MapKey("", "", "")] = 0;
+    // we reserved 0, 1, 2, 3 for the empty case
+    string_map[MapKey("", "", "", "")] = 0;
     lane_description_map[TurnLaneDescription()] = 0;
 }
 
@@ -233,15 +233,16 @@ void ExtractorCallbacks::ProcessWay(const osmium::Way &input_way, const Extracti
         }
     };
 
-    // Deduplicates street names, destination names and refs based on the string_map map.
-    // In case we do not already store the name, inserts (name, id) tuple and return id.
+    // Deduplicates street names, refs, destinations, pronunciation based on the string_map.
+    // In case we do not already store the key, inserts (key, id) tuple and return id.
     // Otherwise fetches the id based on the name and returns it without insertion.
     const auto turn_lane_id_forward = requestId(parsed_way.turn_lanes_forward);
     const auto turn_lane_id_backward = requestId(parsed_way.turn_lanes_backward);
 
     const constexpr auto MAX_STRING_LENGTH = 255u;
     // Get the unique identifier for the street name, destination, and ref
-    const auto name_iterator = string_map.find(MapKey(parsed_way.name, parsed_way.destinations, parsed_way.ref));
+    const auto name_iterator = string_map.find(
+        MapKey(parsed_way.name, parsed_way.destinations, parsed_way.ref, parsed_way.pronunciation));
     unsigned name_id = EMPTY_NAMEID;
     if (string_map.end() == name_iterator)
     {
@@ -256,7 +257,8 @@ void ExtractorCallbacks::ProcessWay(const osmium::Way &input_way, const Extracti
         name_id = external_memory.name_offsets.size() - 1;
 
         external_memory.name_char_data.reserve(external_memory.name_char_data.size() + name_length +
-                                               destinations_length + pronunciation_length);
+                                               destinations_length + pronunciation_length +
+                                               ref_length);
 
         std::copy(parsed_way.name.c_str(),
                   parsed_way.name.c_str() + name_length,
@@ -278,7 +280,8 @@ void ExtractorCallbacks::ProcessWay(const osmium::Way &input_way, const Extracti
                   std::back_inserter(external_memory.name_char_data));
         external_memory.name_offsets.push_back(external_memory.name_char_data.size());
 
-        auto k = MapKey{parsed_way.name, parsed_way.destinations, parsed_way.ref};
+        auto k = MapKey{
+            parsed_way.name, parsed_way.destinations, parsed_way.ref, parsed_way.pronunciation};
         auto v = MapVal{name_id};
         string_map.emplace(std::move(k), std::move(v));
     }

src/extractor/guidance/sliproad_handler.cpp

@@ -37,9 +37,9 @@ SliproadHandler::SliproadHandler(const IntersectionGenerator &intersection_gener
 // included for interface reasons only
 bool SliproadHandler::canProcess(const NodeID /*nid*/,
                                  const EdgeID /*via_eid*/,
-                                 const Intersection & /*intersection*/) const
+                                 const Intersection &intersection) const
 {
-    return true;
+    return intersection.size() > 2;
 }
 
 Intersection SliproadHandler::
@@ -59,6 +59,13 @@ operator()(const NodeID, const EdgeID source_edge_id, Intersection intersection)
             while (intersection.size() == 2)
             {
                 const auto node = node_based_graph.GetTarget(in_edge);
+                if (node == at_node)
+                {
+                    // we ended up in a loop without exit
+                    output_node = SPECIAL_NODEID;
+                    intersection.clear();
+                    return intersection;
+                }
                 in_edge = intersection[1].turn.eid;
                 output_node = node_based_graph.GetTarget(in_edge);
                 intersection = intersection_generator(node, in_edge);
@@ -82,6 +89,10 @@ operator()(const NodeID, const EdgeID source_edge_id, Intersection intersection)
                 intersection_node_id, intersection[1], intersection_node_one);
             const auto intersection_following_index_two = findNextIntersectionForRoad(
                 intersection_node_id, intersection[2], intersection_node_two);
+            // in case of broken roads, we return
+            if (intersection_following_index_one.empty() ||
+                intersection_following_index_two.empty())
+                return 0;
 
             // In case of loops at the end of the road, we will arrive back at the intersection
             // itself. If that is the case, the road is obviously not a sliproad.

src/extractor/guidance/turn_analysis.cpp

@@ -94,7 +94,8 @@ Intersection TurnAnalysis::assignTurnTypes(const NodeID from_nid,
         }
     }
     // Handle sliproads
-    intersection = sliproad_handler(from_nid, via_eid, std::move(intersection));
+    if (sliproad_handler.canProcess(from_nid, via_eid, intersection))
+        intersection = sliproad_handler(from_nid, via_eid, std::move(intersection));
 
     // Turn On Ramps Into Off Ramps, if we come from a motorway-like road
     if (node_based_graph.GetEdgeData(via_eid).road_classification.IsMotorwayClass())

src/tools/routed.cpp

@@ -63,7 +63,8 @@ inline unsigned generateServerProgramOptions(const int argc,
                                              int &max_locations_trip,
                                              int &max_locations_viaroute,
                                              int &max_locations_distance_table,
-                                             int &max_locations_map_matching)
+                                             int &max_locations_map_matching,
+                                             int &max_results_nearest)
 {
     using boost::program_options::value;
     using boost::filesystem::path;
@@ -100,7 +101,10 @@ inline unsigned generateServerProgramOptions(const int argc,
          "Max. locations supported in distance table query") //
         ("max-matching-size",
          value<int>(&max_locations_map_matching)->default_value(100),
-         "Max. locations supported in map matching query");
+         "Max. locations supported in map matching query") //
+        ("max-nearest-size",
+         value<int>(&max_results_nearest)->default_value(100),
+         "Max. results supported in nearest query");
 
     // hidden options, will be allowed on command line, but will not be shown to the user
     boost::program_options::options_description hidden_options("Hidden options");
@@ -189,7 +193,8 @@ int main(int argc, const char *argv[]) try
                                                               config.max_locations_trip,
                                                               config.max_locations_viaroute,
                                                               config.max_locations_distance_table,
-                                                              config.max_locations_map_matching);
+                                                              config.max_locations_map_matching,
+                                                              config.max_results_nearest);
     if (init_result == INIT_OK_DO_NOT_START_ENGINE)
     {
         return EXIT_SUCCESS;

unit_tests/library/limits.cpp

@@ -4,6 +4,7 @@
 #include "args.hpp"
 
 #include "osrm/match_parameters.hpp"
+#include "osrm/nearest_parameters.hpp"
 #include "osrm/route_parameters.hpp"
 #include "osrm/table_parameters.hpp"
 #include "osrm/trip_parameters.hpp"
@@ -136,4 +137,33 @@ BOOST_AUTO_TEST_CASE(test_match_limits)
     BOOST_CHECK(code == "TooBig"); // per the New-Server API spec
 }
 
+BOOST_AUTO_TEST_CASE(test_nearest_limits)
+{
+    const auto args = get_args();
+    BOOST_REQUIRE_EQUAL(args.size(), 1);
+
+    using namespace osrm;
+
+    EngineConfig config;
+    config.storage_config = {args[0]};
+    config.use_shared_memory = false;
+    config.max_results_nearest = 2;
+
+    OSRM osrm{config};
+
+    NearestParameters params;
+    params.coordinates.emplace_back(util::FloatLongitude{}, util::FloatLatitude{});
+    params.number_of_results = 10000;
+
+    json::Object result;
+
+    const auto rc = osrm.Nearest(params, result);
+
+    BOOST_CHECK(rc == Status::Error);
+
+    // Make sure we're not accidentally hitting a guard code path before
+    const auto code = result.values["code"].get<json::String>().value;
+    BOOST_CHECK(code == "TooBig"); // per the New-Server API spec
+}
+
 BOOST_AUTO_TEST_SUITE_END()

unit_tests/util/bearing.cpp

@@ -42,6 +42,9 @@ BOOST_AUTO_TEST_CASE(bearing_range_test)
 
     BOOST_CHECK_EQUAL(true, bearing::CheckInBounds(-721, 5, 10));
     BOOST_CHECK_EQUAL(true, bearing::CheckInBounds(719, 5, 10));
+
+    BOOST_CHECK_EQUAL(false, bearing::CheckInBounds(1, 1, -1));
+    BOOST_CHECK_EQUAL(true, bearing::CheckInBounds(1, 1, 0));
 }
 
 BOOST_AUTO_TEST_SUITE_END()