Fix annotations=true handling in NodeJS bindings & libosrm#6415
Fix annotations=true handling in NodeJS bindings & libosrm#6415SiarheiFedartsou merged 7 commits intomasterfrom
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
| 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
| // 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).
|
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'); |
| 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.
2 participants
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?