Refactor annotations parameter to accept explicit values #3628

karenzshea · 2017-01-30T14:15:48Z

Issue

Tasklist

~~Adapt annotations parsing for map matching route~~ matching inherits from route
Abstract annotation insertion to steps into a function
update relevant Wiki pages
add cucumber cases (see docs/testing.md)
review
adjust for comments

…es annotations_type, add unit_tests

daniel-j-h · 2017-01-31T12:28:46Z

features/testbot/matching.feature

-            | abci  | abci      | 1:1:10.008842:1,0:0:0:0,1:1:10.008842:0,0:0:0:0,1:1:10.010367:0                                                 |
+            | trace | matchings | annotation                                                                                                                      |
+            | abeh  | abeh      | 1:10.008842:1:1:1,0:0:0:2:0,1:10.008842:1:2:0,1:10.008842:1:3:0,1:10.008842:1:4:0,0:0:0:4:0,2:19.906475:2:5:0,1:10.008842:1:6:0 |
+            | abci  | abci      | 1:10.008842:1:1:1,0:0:0:2:0,1:10.008842:1:2:0,0:0:0:2:0,1:10.010367:1:3:0                                                       |


What are the changes here?

daniel-j-h · 2017-01-31T12:31:20Z

features/testbot/matching.feature

+
+        When I match I should get
+            | trace | matchings | annotation    |
+            | ach   | ach       | 1,1,0,1,1,2,1 |


The annotations here are durations, right? How do I read those in the context of the node map? Why is there a duration 0 in there?

daniel-j-h · 2017-01-31T12:31:43Z

features/testbot/matching.feature

+            | node | id |
+            | a    | 1  |
+            | b    | 2  |
+            | c    | 3  |


Why is this needed?

daniel-j-h · 2017-01-31T12:35:40Z

include/engine/api/route_api.hpp

-                annotation.values["weight"] = std::move(weights);
-                annotation.values["nodes"] = std::move(nodes);
-                annotation.values["datasources"] = std::move(datasources);
+                if (parameters.annotations_type & RouteParameters::AnnotationsType::Duration)


Hm either you do the bit fiddling here or what about abstracting it away? The code here could work no matter how the AnnotationsType is implemented.

daniel-j-h · 2017-01-31T12:46:38Z

include/engine/api/route_api.hpp

+                                      nodes.values.push_back(static_cast<std::uint64_t>(node_id));
+                                  });
+                    annotation.values["nodes"] = std::move(nodes);
+                }


Hm the ~100 lines above are all doing the same:

check if annotation x was requested

create json::Array for many x

extract step.x from RouteStep and add into array

add array to annotations response as "x" : [...]

I think you could abstract over this and make it extensible for the future at the same time.

What about abstracting this behind a function which takes

a AnnotationsType

a lambda taking a RouteStep and returning annotations items out of it (e.g. step.duration for AnnotationsType::Duration

a string describing the annotations type for the json object's key as in "key": [...] above

Example callsite:

makeAnnotation(AnnotationsType::Duration, "duration", [](auto& step){ return step.duration });

which then does all of the steps above behind the scenes and wires the parts together.

👍 refactoring this part is already in the OP as the todo item Abstract annotation insertion to steps into a function

I was going to take a template based approach to the function, but I can consider your suggestion as possible solution as well.

daniel-j-h · 2017-01-31T12:49:51Z

include/engine/api/route_parameters.hpp

@@ -67,6 +67,16 @@ struct RouteParameters : public BaseParameters
        Full,
        False
    };
+    enum class AnnotationsType : int


Why the explicit : int here? Should be int by default.

daniel-j-h · 2017-01-31T12:51:26Z

include/engine/api/route_parameters.hpp

+                    Args... args_)
+        : BaseParameters{std::forward<Args>(args_)...}, steps{steps_}, alternatives{alternatives_},
+          annotations_type{annotations_}, annotations{true}, geometries{geometries_},
+          overview{overview_}, continue_straight{continue_straight_}


Nice 👍 This should keep the API stable for the time being. We may want to ticket the removal of all those hacks for v6. I think we already have tickets for these kinds of hacks.

daniel-j-h · 2017-01-31T12:54:32Z

include/server/api/route_parameters_grammar.hpp

+            "nodes", engine::api::RouteParameters::AnnotationsType::Nodes)(
+            "distance", engine::api::RouteParameters::AnnotationsType::Distance)(
+            "weight", engine::api::RouteParameters::AnnotationsType::Weight)(
+            "datasources", engine::api::RouteParameters::AnnotationsType::Datasources);


Hm we need a mapping AnnotationsType -> string here, too. The same is needed for the response generation (as commented above). Maybe we should add this mapping already next to the AnnotationsType enum class definition. This would guarantee that we're in sync with name and enum item.

using karma or a map value->string? would it make sense to make json output completely in karma?

a value->string mapping would be good enough for now - our json handling is all based on a variant, not sure how karma fits in there

daniel-j-h · 2017-01-31T12:55:18Z

include/server/api/route_parameters_grammar.hpp

-             overview_type[ph::bind(&engine::api::RouteParameters::overview, qi::_r1) = qi::_1]);
+             overview_type[ph::bind(&engine::api::RouteParameters::overview, qi::_r1) = qi::_1]) |
+            (qi::lit("annotations=") >
+             annotations_type[ph::bind(add_annotation, qi::_r1, qi::_1)] % ',');


Is an empty list allowed by this? (I don't know => make a unit test)

Added! to the parameters_parser unit test

daniel-j-h · 2017-01-31T12:56:57Z

unit_tests/server/parameters_parser.cpp

+                                                        RouteParameters::AnnotationsType::Weight |
+                                                        RouteParameters::AnnotationsType::Nodes)),
+        true);
+    BOOST_CHECK_EQUAL(result_15->annotations, true);


Order should not matter - care to create a test expressing this?
Also edge cases such as

empty list

all possibilities

all possibilities including All, None and combinations

oxidase · 2017-01-31T13:37:07Z

include/server/api/route_parameters_grammar.hpp

+            "nodes", engine::api::RouteParameters::AnnotationsType::Nodes)(
+            "distance", engine::api::RouteParameters::AnnotationsType::Distance)(
+            "weight", engine::api::RouteParameters::AnnotationsType::Weight)(
+            "datasources", engine::api::RouteParameters::AnnotationsType::Datasources);


using karma or a map value->string? would it make sense to make json output completely in karma?

oxidase · 2017-01-31T13:38:25Z

include/engine/api/route_parameters.hpp

+    enum class AnnotationsType : int
+    {
+        None = 0,
+        Duration = 1 << 1,


here it can start from 1<<0, but better to use explicit numerals 0x01, 0x02, ...

daniel-j-h

👍

daniel-j-h · 2017-02-02T13:02:45Z

features/support/shared_steps.js

+                                        return cb(new Error('Annotation not found in response', a_type));
+                                    got[k] = annotation[a_type];
+                                }
+                            });


Hm this is dup. code with above

daniel-j-h · 2017-02-02T13:03:50Z

include/engine/api/route_api.hpp

+        annotations_store.values.reserve(leg.annotations.size());
+        std::for_each(leg.annotations.begin(),
+                      leg.annotations.end(),
+                      [Get, &annotations_store](const guidance::LegGeometry::Annotation &step) {


I think you can simply say const auto& step here

daniel-j-h · 2017-02-02T13:06:24Z

include/engine/api/route_api.hpp

+                    annotation.values["duration"] =
+                        GetAnnotations(leg_geometry,
+                                       std::bind(&guidance::LegGeometry::Annotation::duration,
+                                                 std::placeholders::_1));


Instead of bind you can simply use a lambda, which is probably easier to read and does not have all the weird quirks bind has (see https://www.youtube.com/watch?v=zt7ThwVfap0). E.g.

[](const auto& v) { return v.distance };

daniel-j-h · 2017-02-02T13:12:00Z

unit_tests/server/parameters_parser.cpp

@@ -128,6 +132,9 @@ BOOST_AUTO_TEST_CASE(valid_route_urls)
    CHECK_EQUAL_RANGE(reference_2.radiuses, result_2->radiuses);
    CHECK_EQUAL_RANGE(reference_2.coordinates, result_2->coordinates);
    CHECK_EQUAL_RANGE(reference_2.hints, result_2->hints);
+    BOOST_CHECK_EQUAL(
+        static_cast<bool>(result_2->annotations_type & RouteParameters::AnnotationsType::All),
+        true);


Hm maybe add operator== overload then? We already have | and &.

daniel-j-h · 2017-02-02T13:12:20Z

unit_tests/server/parameters_parser.cpp

+    BOOST_CHECK_EQUAL(reference_17.geometries, result_17->geometries);
+    BOOST_CHECK_EQUAL(
+        static_cast<bool>(result_2->annotations_type & RouteParameters::AnnotationsType::All),
+        true);


Same for all of these tests, type == All

oxidase

LGTM! I added some change requests, but they are minor

oxidase · 2017-02-02T13:09:55Z

features/step_definitions/matching.js

-                    }
+                    // if header matches 'a:*', parse out the values for *
+                    // and return in that header
+                    headers.forEach((k, v) => {


👍 but lint complains about unused v variable

oxidase · 2017-02-02T13:10:30Z

features/support/shared_steps.js

-                            }
+                            // if header matches 'a:*', parse out the values for *
+                            // and return in that header
+                            headers.forEach((k, v) => {


and here v is unused

oxidase · 2017-02-02T13:13:12Z

include/engine/api/route_parameters.hpp

+                    Args... args_)
+        : BaseParameters{std::forward<Args>(args_)...}, steps{steps_}, alternatives{alternatives_},
+          annotations_type{annotations_}, annotations{true}, geometries{geometries_},
+          overview{overview_}, continue_straight{continue_straight_}
    {
    }

    bool steps = false;
    bool alternatives = false;
    bool annotations = false;


this is now a duplication, better

bool annotations() const { return annotations_type != AnnotationsType::None; };

But it is not used in code base anymore, the best would be delete it and update unit tests.

We can't do that - the annotations member attribute is in our public API - we can't change it to a function :(

oxidase · 2017-02-02T13:14:48Z

include/server/api/route_parameters_grammar.hpp

@@ -42,6 +42,13 @@ struct RouteParametersGrammar : public BaseParametersGrammar<Iterator, Signature

    RouteParametersGrammar(qi::rule<Iterator, Signature> &root_rule_) : BaseGrammar(root_rule_)
    {
+        const auto add_annotation = [](engine::api::RouteParameters &route_parameters,


this lambda is not needed with operator |= and in-place phoenix bind (i added related comments)

due to #3628 (comment) both members must be updated, so add_annotation must remain

oxidase · 2017-02-02T13:17:34Z

include/engine/api/route_parameters.hpp

+                    const boost::optional<bool> continue_straight_,
+                    Args... args_)
+        : BaseParameters{std::forward<Args>(args_)...}, steps{steps_}, alternatives{alternatives_},
+          annotations_type{annotations_}, annotations{true}, geometries{geometries_},


here annotations is true even if annotations_ is None

oxidase · 2017-02-02T13:18:57Z

include/server/api/route_parameters_grammar.hpp

-             overview_type[ph::bind(&engine::api::RouteParameters::overview, qi::_r1) = qi::_1]);
+             overview_type[ph::bind(&engine::api::RouteParameters::overview, qi::_r1) = qi::_1]) |
+            (qi::lit("annotations=") >
+             annotations_type[ph::bind(add_annotation, qi::_r1, qi::_1)] % ',');


... here in-place functor

annotations_type[ph::bind(&engine::api::RouteParameters::annotations_type, qi::_r1) |= qi::_1] % ',');

oxidase · 2017-02-02T13:21:23Z

include/engine/api/route_parameters.hpp

+        static_cast<std::underlying_type_t<RouteParameters::AnnotationsType>>(lhs) |
+        static_cast<std::underlying_type_t<RouteParameters::AnnotationsType>>(rhs));
+}
+


... and here compound operator

inline RouteParameters::AnnotationsType& operator|=(RouteParameters::AnnotationsType& lhs, RouteParameters::AnnotationsType rhs) { return lhs = lhs | rhs; }

oxidase · 2017-02-02T13:45:40Z

include/server/api/route_parameters_grammar.hpp

+                                       engine::api::RouteParameters::AnnotationsType &route_param) {
+            route_parameters.annotations_type = route_parameters.annotations_type | route_param;
+            route_parameters.annotations = route_parameters.annotations_type != engine::api::RouteParameters::AnnotationsType::None;
+            return route_parameters;


returned value is not used and return can be removed

TheMarex

Code and test refactor looks great. 👍 Two thinks missing: Updating docs/http.md to reflect the new parameters and adding an entry to CHANGELOG.md.

TheMarex · 2017-02-07T09:14:57Z

This is still missing the speed parameter. Capturing this in #3664 to be fixed in another PR.

Otherwise ready to merge.

karenzshea added 4 commits January 27, 2017 11:30

store/accept individual annotations parameters in addition to bool

7e45949

implement bitwise and operator

c5bfc1b

fix bitwise operators, selectively add annotations to response obj

64b4916

set RouteParameters annotations member, when constructor only specifi…

0f4f840

…es annotations_type, add unit_tests

karenzshea added the Work In Progress label Jan 30, 2017

update support code to handle variable annotations fields

45a8d81

karenzshea force-pushed the annotations-params branch from c877747 to 45a8d81 Compare January 30, 2017 18:25

fix cucumber outputs

9a67fb4

karenzshea force-pushed the annotations-params branch from 4079ace to 9a67fb4 Compare January 31, 2017 11:56

daniel-j-h requested changes Jan 31, 2017

View reviewed changes

oxidase reviewed Jan 31, 2017

View reviewed changes

karenzshea added 3 commits January 31, 2017 15:20

revert

015a198

a few more parameter tests

4e4f60d

GetAnnotations function

47a02df

karenzshea force-pushed the annotations-params branch from 54721eb to 47a02df Compare February 1, 2017 14:55

karenzshea requested a review from TheMarex February 2, 2017 12:30

daniel-j-h approved these changes Feb 2, 2017

View reviewed changes

oxidase approved these changes Feb 2, 2017

View reviewed changes

oxidase reviewed Feb 2, 2017

View reviewed changes

update cucumber suport code to return separate annotations headers

4ca5cd4

karenzshea force-pushed the annotations-params branch 2 times, most recently from 793ad55 to 58322d8 Compare February 2, 2017 15:41

karenzshea mentioned this pull request Feb 2, 2017

Remove redundant bool member for annotations param in RouteParameters #3644

Open

use lambda instead of bindy

9c14285

karenzshea force-pushed the annotations-params branch from 58322d8 to 9c14285 Compare February 2, 2017 15:48

clang format

823e66c

TheMarex approved these changes Feb 3, 2017

View reviewed changes

daniel-j-h added this to the 5.6.0 milestone Feb 6, 2017

TheMarex added the Ready To Merge label Feb 7, 2017

TheMarex removed the Work In Progress label Feb 7, 2017

TheMarex merged commit 1e9e420 into master Feb 7, 2017

TheMarex deleted the annotations-params branch February 7, 2017 09:15

TheMarex mentioned this pull request Feb 7, 2017

Refactor post-processing and add test for turn location #3495

Merged

4 tasks

chaupow mentioned this pull request Feb 10, 2017

Refactor annotations paramater #3684

Closed

daniel-j-h mentioned this pull request Feb 10, 2017

Wrap: Allow annotations parameter to accept specific fields Project-OSRM/node-osrm#298

Closed

3 tasks

Refactor annotations parameter to accept explicit values #3628

Refactor annotations parameter to accept explicit values #3628

Conversation

karenzshea commented Jan 30, 2017 • edited by TheMarex Loading

Issue

Tasklist

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

daniel-j-h left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

oxidase left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

TheMarex left a comment

Choose a reason for hiding this comment

TheMarex commented Feb 7, 2017

karenzshea commented Jan 30, 2017 •

edited by TheMarex

Loading