Parse table annotations param correctly (#5050)

* fix incorrect parameter parsing for node osrm and add tests

* fix boost spirit grammar parsing for annotations

* return NotImplemented when distance annotation is requested for MLD in table plugin

* update docs

docs/http.md

@@ -222,7 +222,7 @@ curl 'http://router.project-osrm.org/route/v1/driving/13.388860,52.517037;13.397
 
 ### Table service
 
-Computes the duration of the fastest route between all pairs of supplied coordinates. Optionally, also returns the distances between the coordinate pairs. Note that the distances are not the shortest distance between two coordinates, but rather the distances of the fastest routes.
+Computes the duration of the fastest route between all pairs of supplied coordinates. Returns the durations or distances or both between the coordinate pairs. Note that the distances are not the shortest distance between two coordinates, but rather the distances of the fastest routes. Duration is in seconds and distances is in meters.
 
 ```endpoint
 GET /table/v1/{profile}/{coordinates}?{sources}=[{elem}...];&{destinations}=[{elem}...]&annotations={duration|distance|duration,distance}
@@ -236,7 +236,8 @@ In addition to the [general options](#general-options) the following options are
 |------------|--------------------------------------------------|---------------------------------------------|
 |sources     |`{index};{index}[;{index} ...]` or `all` (default)|Use location with given index as source.     |
 |destinations|`{index};{index}[;{index} ...]` or `all` (default)|Use location with given index as destination.|
-|annotations |`duration` (default), `distance`, or `duration,distance`|Return additional table with distances to the response. Whether requested or not, the duration table is always returned.|
+|annotations |`duration` (default), `distance`, or `duration,distance`|Return the requested table or tables in response. Note that computing the `distances` table is currently only implemented for CH. If `annotations=distance` or `annotations=duration,distance` is requested when running a MLD router, a `NotImplemented` error will be returned.
+|
 
 Unlike other array encoded options, the length of `sources` and `destinations` can be **smaller or equal**
 to number of input locations;
@@ -266,10 +267,10 @@ curl 'http://router.project-osrm.org/table/v1/driving/polyline(egs_Iq_aqAppHzbHu
 # Returns a 3x3 duration matrix:
 curl 'http://router.project-osrm.org/table/v1/driving/13.388860,52.517037;13.397634,52.529407;13.428555,52.523219&annotations=duration'
 
-# Returns a 3x3 distance matrix:
+# Returns a 3x3 distance matrix for CH:
 curl 'http://router.project-osrm.org/table/v1/driving/13.388860,52.517037;13.397634,52.529407;13.428555,52.523219&annotations=distance'
 
-# Returns a 3x3 duration matrix and a 3x3 distance matrix:
+# Returns a 3x3 duration matrix and a 3x3 distance matrix for CH:
 curl 'http://router.project-osrm.org/table/v1/driving/13.388860,52.517037;13.397634,52.529407;13.428555,52.523219&annotations=distance,duration'
 ```
 
@@ -279,7 +280,7 @@ curl 'http://router.project-osrm.org/table/v1/driving/13.388860,52.517037;13.397
 - `durations` array of arrays that stores the matrix in row-major order. `durations[i][j]` gives the travel time from
   the i-th waypoint to the j-th waypoint. Values are given in seconds. Can be `null` if no route between `i` and `j` can be found.
 - `distances` array of arrays that stores the matrix in row-major order. `distances[i][j]` gives the travel distance from
-  the i-th waypoint to the j-th waypoint. Values are given in meters. Can be `null` if no route between `i` and `j` can be found.
+  the i-th waypoint to the j-th waypoint. Values are given in meters. Can be `null` if no route between `i` and `j` can be found. Note that computing the `distances` table is currently only implemented for CH. If `annotations=distance` or `annotations=duration,distance` is requested when running a MLD router, a `NotImplemented` error will be returned.
 - `sources` array of `Waypoint` objects describing all sources in order
 - `destinations` array of `Waypoint` objects describing all destinations in order
 
@@ -288,6 +289,7 @@ In case of error the following `code`s are supported in addition to the general
 | Type             | Description     |
 |------------------|-----------------|
 | `NoTable`        | No route found. |
+| `NotImplemented`  | This request is not supported |
 
 All other properties might be undefined.
 

docs/nodejs/api.md

@@ -126,7 +126,6 @@ tables. Optionally returns distance table.
         location with given index as source. Default is to use all.
     -   `options.destinations` **[Array](https://developer.mozilla.org/en-US/docs/Web/JavaScript/Reference/Global_Objects/Array)?** An array of `index` elements (`0 <= integer <
         #coordinates`) to use location with given index as destination. Default is to use all.
-    -   `options.annotations` **[Array](https://developer.mozilla.org/en-US/docs/Web/JavaScript/Reference/Global_Objects/Array)?** An array of the table types to return. Values can be `duration` or `distance` or both. Default is to return only duration table.
     -   `options.approaches` **[Array](https://developer.mozilla.org/en-US/docs/Web/JavaScript/Reference/Global_Objects/Array)?** Keep waypoints on curb side. Can be `null` (unrestricted, default) or `curb`.
 -   `callback` **[Function](https://developer.mozilla.org/en-US/docs/Web/JavaScript/Reference/Statements/function)** 
 
@@ -143,7 +142,6 @@ var options = {
 };
 osrm.table(options, function(err, response) {
   console.log(response.durations); // array of arrays, matrix in row-major order
-  console.log(response.distances); // array of arrays, matrix in row-major order
   console.log(response.sources); // array of Waypoint objects
   console.log(response.destinations); // array of Waypoint objects
 });

features/support/hooks.js

@@ -51,7 +51,7 @@ module.exports = function () {
             .defer(rimraf, this.scenarioLogFile)
             .awaitAll(callback);
         // uncomment to get path to logfile
-        console.log('  Writing logging output to ' + this.scenarioLogFile);
+        // console.log('  Writing logging output to ' + this.scenarioLogFile);
     });
 
     this.After((scenario, callback) => {

include/engine/algorithm.hpp

@@ -50,6 +50,9 @@ template <typename AlgorithmT> struct HasMapMatching final : std::false_type
 template <typename AlgorithmT> struct HasManyToManySearch final : std::false_type
 {
 };
+template <typename AlgorithmT> struct SupportsDistanceAnnotationType final : std::false_type
+{
+};
 template <typename AlgorithmT> struct HasGetTileTurns final : std::false_type
 {
 };
@@ -73,6 +76,9 @@ template <> struct HasMapMatching<ch::Algorithm> final : std::true_type
 template <> struct HasManyToManySearch<ch::Algorithm> final : std::true_type
 {
 };
+template <> struct SupportsDistanceAnnotationType<ch::Algorithm> final : std::true_type
+{
+};
 template <> struct HasGetTileTurns<ch::Algorithm> final : std::true_type
 {
 };
@@ -96,6 +102,9 @@ template <> struct HasMapMatching<mld::Algorithm> final : std::true_type
 template <> struct HasManyToManySearch<mld::Algorithm> final : std::true_type
 {
 };
+template <> struct SupportsDistanceAnnotationType<mld::Algorithm> final : std::false_type
+{
+};
 template <> struct HasGetTileTurns<mld::Algorithm> final : std::true_type
 {
 };

include/engine/api/table_parameters.hpp

@@ -135,8 +135,8 @@ inline TableParameters::AnnotationsType operator|(TableParameters::AnnotationsTy
         static_cast<std::underlying_type_t<TableParameters::AnnotationsType>>(rhs));
 }
 
-inline TableParameters::AnnotationsType operator|=(TableParameters::AnnotationsType lhs,
+inline TableParameters::AnnotationsType &operator|=(TableParameters::AnnotationsType &lhs,
-                                                   TableParameters::AnnotationsType rhs)
+                                                    TableParameters::AnnotationsType rhs)
 {
     return lhs = lhs | rhs;
 }

include/engine/routing_algorithms.hpp

@@ -55,6 +55,7 @@ class RoutingAlgorithmsInterface
     virtual bool HasDirectShortestPathSearch() const = 0;
     virtual bool HasMapMatching() const = 0;
     virtual bool HasManyToManySearch() const = 0;
+    virtual bool SupportsDistanceAnnotationType() const = 0;
     virtual bool HasGetTileTurns() const = 0;
     virtual bool HasExcludeFlags() const = 0;
     virtual bool IsValid() const = 0;
@@ -128,6 +129,11 @@ template <typename Algorithm> class RoutingAlgorithms final : public RoutingAlgo
         return routing_algorithms::HasManyToManySearch<Algorithm>::value;
     }
 
+    bool SupportsDistanceAnnotationType() const final override
+    {
+        return routing_algorithms::SupportsDistanceAnnotationType<Algorithm>::value;
+    }
+
     bool HasGetTileTurns() const final override
     {
         return routing_algorithms::HasGetTileTurns<Algorithm>::value;

include/nodejs/node_osrm_support.hpp

@@ -1077,6 +1077,8 @@ argumentsToTableParameter(const Nan::FunctionCallbackInfo<v8::Value> &args,
             return table_parameters_ptr();
         }
 
+        params->annotations = osrm::TableParameters::AnnotationsType::None;
+
         v8::Local<v8::Array> annotations_array = v8::Local<v8::Array>::Cast(annotations);
         for (std::size_t i = 0; i < annotations_array->Length(); ++i)
         {

include/server/api/table_parameter_grammar.hpp

@@ -58,17 +58,15 @@ struct TableParametersGrammar : public BaseParametersGrammar<Iterator, Signature
     {
         using AnnotationsType = engine::api::TableParameters::AnnotationsType;
 
-        const auto add_annotation = [](engine::api::TableParameters &table_parameters,
-                                       AnnotationsType table_param) {
-            table_parameters.annotations = table_parameters.annotations | table_param;
-        };
-
         annotations.add("duration", AnnotationsType::Duration)("distance",
                                                                AnnotationsType::Distance);
 
+        annotations_list = annotations[qi::_val |= qi::_1] % ',';
+
         base_rule = BaseGrammar::base_rule(qi::_r1) |
                     (qi::lit("annotations=") >
-                     (annotations[ph::bind(add_annotation, qi::_r1, qi::_1)] % ','));
+                     annotations_list[ph::bind(&engine::api::TableParameters::annotations,
+                                               qi::_r1) = qi::_1]);
     }
 
   protected:
@@ -81,6 +79,7 @@ struct TableParametersGrammar : public BaseParametersGrammar<Iterator, Signature
     qi::rule<Iterator, Signature> destinations_rule;
     qi::rule<Iterator, std::size_t()> size_t_;
     qi::symbols<char, engine::api::TableParameters::AnnotationsType> annotations;
+    qi::rule<Iterator, engine::api::TableParameters::AnnotationsType()> annotations_list;
 };
 }
 }

src/engine/plugins/table.cpp

@@ -85,10 +85,18 @@ Status TablePlugin::HandleRequest(const RoutingAlgorithmsInterface &algorithms,
     bool request_distance = params.annotations & api::TableParameters::AnnotationsType::Distance;
     bool request_duration = params.annotations & api::TableParameters::AnnotationsType::Duration;
 
+    if (request_distance && !algorithms.SupportsDistanceAnnotationType())
+    {
+        return Error("NotImplemented",
+                     "The distance annotations calculation is not implemented for the chosen "
+                     "search algorithm.",
+                     result);
+    }
+
     auto result_tables_pair = algorithms.ManyToManySearch(
         snapped_phantoms, params.sources, params.destinations, request_distance, request_duration);
 
-    if ((request_duration & result_tables_pair.first.empty()) ||
+    if ((request_duration && result_tables_pair.first.empty()) ||
         (request_distance && result_tables_pair.second.empty()))
     {
         return Error("NoTable", "No table found", result);

src/nodejs/node_osrm.cpp

@@ -274,17 +274,20 @@ NAN_METHOD(Engine::nearest) //
  *                                   Can be `null` or an array of `[{value},{range}]` with `integer 0 .. 360,integer 0 .. 180`.
  * @param {Array} [options.radiuses] Limits the coordinate snapping to streets in the given radius in meters. Can be `null` (unlimited, default) or `double >= 0`.
  * @param {Array} [options.hints] Hints for the coordinate snapping. Array of base64 encoded strings.
- * @param {Array} [options.sources] An array of `index` elements (`0 <= integer < #coordinates`) to
+ * @param {Array} [options.sources] An array of `index` elements (`0 <= integer < #coordinates`) to use
- * use
+ *                                  location with given index as source. Default is to use all.
- * location with given index as source. Default is to use all.
+ * @param {Array} [options.destinations] An array of `index` elements (`0 <= integer < #coordinates`) to use location with given index as destination. Default is to use all.
- * @param {Array} [options.destinations] An array of `index` elements (`0 <= integer <
- * #coordinates`) to use location with given index as destination. Default is to use all.
  * @param {Array} [options.approaches] Keep waypoints on curb side. Can be `null` (unrestricted, default) or `curb`.
+ * @param {Array} [options.annotations] An array of the table types to return. Values can be `duration` or `distance` or both. If no annotations parameter is added, the default is to return the `durations` table. If `annotations=distance` or `annotations=duration,distance` is requested when running a MLD router, a `NotImplemented` error will be returned.
+
  * @param {Function} callback
  *
  * @returns {Object} containing `durations`, `sources`, and `destinations`.
  * **`durations`**: array of arrays that stores the matrix in row-major order. `durations[i][j]` gives the travel time from the i-th waypoint to the j-th waypoint.
  *                  Values are given in seconds.
+ * **`distances`**: array of arrays that stores the matrix in row-major order. `distances[i][j]` gives the travel time from the i-th waypoint to the j-th waypoint.
+ *                  Values are given in meters. Note that computing the `distances` table is currently only implemented for CH. If `annotations=distance` or
+ *                  `annotations=duration,distance` is requested when running a MLD router, a `NotImplemented` error will be returned.
  * **`sources`**: array of [`Ẁaypoint`](#waypoint) objects describing all sources in order.
  * **`destinations`**: array of [`Ẁaypoint`](#waypoint) objects describing all destinations in order.
  *
@@ -299,7 +302,7 @@ NAN_METHOD(Engine::nearest) //
  * };
  * osrm.table(options, function(err, response) {
  *   console.log(response.durations); // array of arrays, matrix in row-major order
- *   console.log(response.distances); // array of arrays, matrix in row-major order
+ *   console.log(response.distances); // array of arrays, matrix in row-major order (currently only implemented for CH router)
  *   console.log(response.sources); // array of Waypoint objects
  *   console.log(response.destinations); // array of Waypoint objects
  * });

test/nodejs/table.js

@@ -5,6 +5,48 @@ var mld_data_path = require('./constants').mld_data_path;
 var three_test_coordinates = require('./constants').three_test_coordinates;
 var two_test_coordinates = require('./constants').two_test_coordinates;
 
+test('table: test annotations paramater combination', function(assert) {
+    assert.plan(12);
+    var osrm = new OSRM(data_path);
+    var options = {
+        coordinates: [three_test_coordinates[0], three_test_coordinates[1]],
+        annotations: ['distance']
+    };
+    osrm.table(options, function(err, table) {
+        assert.ifError(err);
+        assert.ok(table['distances'], 'distances table result should exist');
+        assert.notOk(table['durations'], 'durations table result should not exist');
+    });
+
+    options = {
+        coordinates: [three_test_coordinates[0], three_test_coordinates[1]],
+        annotations: ['duration']
+    };
+    osrm.table(options, function(err, table) {
+        assert.ifError(err);
+        assert.ok(table['durations'], 'durations table result should exist');
+        assert.notOk(table['distances'], 'distances table result should not exist');
+    });
+
+    options = {
+        coordinates: [three_test_coordinates[0], three_test_coordinates[1]],
+        annotations: ['duration', 'distance']
+    };
+    osrm.table(options, function(err, table) {
+        assert.ifError(err);
+        assert.ok(table['durations'], 'durations table result should exist');
+        assert.ok(table['distances'], 'distances table result should exist');
+    });
+
+    options = {
+        coordinates: [three_test_coordinates[0], three_test_coordinates[1]]
+    };
+    osrm.table(options, function(err, table) {
+        assert.ifError(err);
+        assert.ok(table['durations'], 'durations table result should exist');
+        assert.notOk(table['distances'], 'distances table result should not exist');
+    });
+});
 
 var tables = ['distances', 'durations'];
 
@@ -143,7 +185,6 @@ tables.forEach(function(annotation) {
             annotations: [annotation.slice(0,-1)]
         };
         osrm.table(options, function(err, table) {
-            console.log(table);
             assert.ifError(err);
 
             function assertHasHints(waypoint) {
@@ -176,7 +217,7 @@ tables.forEach(function(annotation) {
     });
 
     test('table: ' + annotation + ' table in Monaco without motorways', function(assert) {
-        assert.plan(2);
+        assert.plan(1);
         var osrm = new OSRM({path: mld_data_path, algorithm: 'MLD'});
         var options = {
             coordinates: two_test_coordinates,
@@ -184,8 +225,8 @@ tables.forEach(function(annotation) {
             annotations: [annotation.slice(0,-1)]
         };
         osrm.table(options, function(err, response) {
-            assert.ifError(err);
+            if (annotation === 'durations') assert.equal(response[annotation].length, 2);
-            assert.equal(response[annotation].length, 2);
+            else assert.error(response, 'NotImplemented');
         });
     });
 });