Fix annotations=true handling in NodeJS bindings & libosrm #6415
Conversation
| assert.ok(first.routes[0].legs.every(l => { return l.annotation.nodes;}), 'every leg has annotations for nodes'); | ||
| assert.ok(first.routes[0].legs.every(l => { return l.annotation.weight; }), 'every leg has annotations for weight'); | ||
| assert.ok(first.routes[0].legs.every(l => { return l.annotation.datasources; }), 'every leg has annotations for datasources'); | ||
| assert.ok(first.routes[0].legs.every(l => { return l.annotation.speed; }), 'every leg has annotations for speed'); |
There was a problem hiding this comment.
Btw problem was happening only for speed, because we have these checks here which it seems were added for backward compatibility, but I am not sure if we should use parameters.annotations_type(but not requested_annotations) when checking if speed annotations were requested.
osrm-backend/include/engine/api/route_api.hpp
Line 770 in 5e5f1f4
There was a problem hiding this comment.
but I am not sure if we should use parameters.annotations_type(but not requested_annotations) when checking if speed annotations were requested.
It looks as a bug, because it is different from other annotations, so I changed it to use requested_annotations
| @@ -845,6 +848,9 @@ inline bool parseCommonParameters(const v8::Local<v8::Object> &obj, ParamType &p | |||
| Nan::ThrowError("this 'annotations' param is not supported"); | |||
| return false; | |||
| } | |||
|
|
|||
| params->annotations = | |||
There was a problem hiding this comment.
I would remove this annotations boolean field completely, but it seems it would break libosrm backward compatibility
There was a problem hiding this comment.
We should think about all the breaking changes we would make for a v6 release.
SiarheiFedartsou
changed the title
| @@ -439,7 +439,7 @@ class RouteAPI : public BaseAPI | |||
| { | |||
| // AnnotationsType uses bit flags, & operator checks if a property is set | |||
| flatbuffers::Offset<flatbuffers::Vector<float>> speed; | |||
| if (parameters.annotations_type & RouteParameters::AnnotationsType::Speed) | |||
| if (requested_annotations & RouteParameters::AnnotationsType::Speed) | |||
There was a problem hiding this comment.
We can leave it "as is" and NodeJs tests will still pass, but this seems to fix the problem for libosrm(when annotations = true was used).
See:
https://github.com/Project-OSRM/osrm-backend/pull/6415/files#r998472037
|
Fix for CI issues #6416 |
| assert.ok(first.routes[0].legs.every(l => { return l.annotation.nodes;}), 'every leg has annotations for nodes'); | ||
| assert.ok(first.routes[0].legs.every(l => { return l.annotation.weight; }), 'every leg has annotations for weight'); | ||
| assert.ok(first.routes[0].legs.every(l => { return l.annotation.datasources; }), 'every leg has annotations for datasources'); | ||
| assert.ok(first.routes[0].legs.every(l => { return l.annotation.speed; }), 'every leg has annotations for speed'); |
| @@ -845,6 +848,9 @@ inline bool parseCommonParameters(const v8::Local<v8::Object> &obj, ParamType &p | |||
| Nan::ThrowError("this 'annotations' param is not supported"); | |||
| return false; | |||
| } | |||
|
|
|||
| params->annotations = | |||
There was a problem hiding this comment.
We should think about all the breaking changes we would make for a v6 release.
Issue
Found this problem while working on #6411 (it was actually caught by one of our tests when I started running them against NodeJS based server - one more reason to rewrite osrm-router in NodeJs :) ).
Now it works the same as in osrm-routed:
osrm-backend/include/server/api/route_parameters_grammar.hpp
Line 94 in 5e5f1f4
Or in constructors of
RouteParameters:osrm-backend/include/engine/api/route_parameters.hpp
Line 129 in 5e5f1f4
Tasklist
Requirements / Relations
Link any requirements here. Other pull requests this PR is based on?