Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Fix annotations=true handling in NodeJS bindings & libosrm #6415

Merged
merged 7 commits into from
Oct 19, 2022
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
1 change: 1 addition & 0 deletions CHANGELOG.md
Original file line number Diff line number Diff line change
@@ -1,6 +1,7 @@
# Unreleased
- Changes from 5.27.1
- Misc:
- FIXED: Fix annotations=true handling in NodeJS bindings & libosrm. [#6415](https://github.com/Project-OSRM/osrm-backend/pull/6415/)
- FIXED: Fix bindings compilation issue on the latest Node. Update NAN to 2.17.0. [#6416](https://github.com/Project-OSRM/osrm-backend/pull/6416)
# 5.27.1
- Changes from 5.27.0
Expand Down
4 changes: 2 additions & 2 deletions include/engine/api/route_api.hpp
Original file line number Diff line number Diff line change
Expand Up @@ -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)
Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

{
double prev_speed = 0;
speed =
Expand Down Expand Up @@ -778,7 +778,7 @@ class RouteAPI : public BaseAPI
util::json::Object annotation;

// AnnotationsType uses bit flags, & operator checks if a property is set
if (parameters.annotations_type & RouteParameters::AnnotationsType::Speed)
if (requested_annotations & RouteParameters::AnnotationsType::Speed)
{
double prev_speed = 0;
annotation.values["speed"] = GetAnnotations(
Expand Down
6 changes: 6 additions & 0 deletions include/nodejs/node_osrm_support.hpp
Original file line number Diff line number Diff line change
Expand Up @@ -799,6 +799,9 @@ inline bool parseCommonParameters(const v8::Local<v8::Object> &obj, ParamType &p
if (annotations->IsBoolean())
{
params->annotations = Nan::To<bool>(annotations).FromJust();
params->annotations_type = params->annotations
? osrm::RouteParameters::AnnotationsType::All
: osrm::RouteParameters::AnnotationsType::None;
}
else if (annotations->IsArray())
{
Expand Down Expand Up @@ -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 =
Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I would remove this annotations boolean field completely, but it seems it would break libosrm backward compatibility

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

We should think about all the breaking changes we would make for a v6 release.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Heh, it's already there: #3644

params->annotations_type != osrm::RouteParameters::AnnotationsType::None;
}
}
else
Expand Down
14 changes: 10 additions & 4 deletions test/nodejs/route.js
Original file line number Diff line number Diff line change
Expand Up @@ -286,9 +286,9 @@ test('route: routes Monaco with several (duration, distance, nodes) annotations
assert.ok(first.routes[0].legs.every(l => { return l.annotation.distance;}), 'every leg has annotations for distance');
assert.ok(first.routes[0].legs.every(l => { return l.annotation.duration;}), 'every leg has annotations for durations');
assert.ok(first.routes[0].legs.every(l => { return l.annotation.nodes;}), 'every leg has annotations for nodes');
assert.notOk(first.routes[0].legs.every(l => { return l.annotation.weight; }), 'has no annotations for weight')
assert.notOk(first.routes[0].legs.every(l => { return l.annotation.datasources; }), 'has no annotations for datasources')
assert.notOk(first.routes[0].legs.every(l => { return l.annotation.speed; }), 'has no annotations for speed')
assert.notOk(first.routes[0].legs.every(l => { return l.annotation.weight; }), 'has no annotations for weight');
assert.notOk(first.routes[0].legs.every(l => { return l.annotation.datasources; }), 'has no annotations for datasources');
assert.notOk(first.routes[0].legs.every(l => { return l.annotation.speed; }), 'has no annotations for speed');

options.overview = 'full';
osrm.route(options, function(err, full) {
Expand All @@ -303,7 +303,7 @@ test('route: routes Monaco with several (duration, distance, nodes) annotations
});

test('route: routes Monaco with options', function(assert) {
assert.plan(11);
assert.plan(17);
var osrm = new OSRM(monaco_path);
var options = {
coordinates: two_test_coordinates,
Expand All @@ -322,6 +322,12 @@ test('route: routes Monaco with options', function(assert) {
assert.ok(first.routes[0].legs[0]);
assert.ok(first.routes[0].legs.every(l => { return l.steps.length > 0; }), 'every leg has steps');
assert.ok(first.routes[0].legs.every(l => { return l.annotation;}), 'every leg has annotations');
assert.ok(first.routes[0].legs.every(l => { return l.annotation.distance;}), 'every leg has annotations for distance');
assert.ok(first.routes[0].legs.every(l => { return l.annotation.duration;}), 'every leg has annotations for durations');
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');
Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

requested_annotations = RouteParameters::AnnotationsType::All;

Screenshot 2022-10-18 at 18 35 51

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yep, looks like it.


options.overview = 'full';
osrm.route(options, function(err, full) {
Expand Down
2 changes: 1 addition & 1 deletion unit_tests/library/route.cpp
Original file line number Diff line number Diff line change
Expand Up @@ -584,7 +584,7 @@ void test_manual_setting_of_annotations_property(bool use_json_only_api)
.values["annotation"]
.get<json::Object>()
.values;
BOOST_CHECK_EQUAL(annotations.size(), 6);
BOOST_CHECK_EQUAL(annotations.size(), 7);
}
BOOST_AUTO_TEST_CASE(test_manual_setting_of_annotations_property_old_api)
{
Expand Down