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.

Sign up for GitHub

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

NodeJS version of osrm-routed #4604

Closed
wants to merge 4 commits into from
Closed

NodeJS version of osrm-routed #4604

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
4 commits
Select commit Hold shift + click to select a range
208e6f2
Javascript version of osrm-routed.
danpat Oct 12, 2017
9e690c9
Remove unused exception subclass.
danpat Oct 12, 2017
2ed50e5
Refactor parameter validation.
danpat Oct 27, 2017
ea27e31
Make osrm-routed.js more compatible with osrm-routed.
danpat Oct 31, 2017
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
12 changes: 6 additions & 6 deletions features/lib/osrm_loader.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -47,7 +47,7 @@ class OSRMBaseLoader{
if (err) {
if (retryCount < 10) {
retryCount++;
setTimeout(() => { tryConnect(this.scope.OSRM_PORT, retry); }, 10);
setTimeout(() => { tryConnect(this.scope.OSRM_PORT, retry); }, 100);
Copy link
Member

Choose a reason for hiding this comment

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

Why the change here? Is 10s no longer enough?

} else {
callback(new Error("Could not connect to osrm-routed after ten retries."));
}
Expand Down Expand Up @@ -75,13 +75,13 @@ class OSRMDirectLoader extends OSRMBaseLoader {
}

osrmUp (callback) {
if (this.osrmIsRunning()) return callback(new Error("osrm-routed already running!"));
if (this.osrmIsRunning()) return callback(new Error("osrm-routed.js already running!"));

const command_arguments = util.format('%s -p %d -a %s', this.inputFile, this.scope.OSRM_PORT, this.scope.ROUTING_ALGORITHM);
this.child = this.scope.runBin('osrm-routed', command_arguments, this.scope.environment, (err) => {
this.child = this.scope.runBin('osrm-routed.js', command_arguments, this.scope.environment, (err) => {
if (err && err.signal !== 'SIGINT') {
this.child = null;
throw new Error(util.format('osrm-routed %s: %s', errorReason(err), err.cmd));
throw new Error(util.format('osrm-routed.js %s: %s', errorReason(err), err.cmd));
}
});
callback();
Expand Down Expand Up @@ -117,10 +117,10 @@ class OSRMDatastoreLoader extends OSRMBaseLoader {
if (this.osrmIsRunning()) return callback();

const command_arguments = util.format('--shared-memory=1 -p %d -a %s', this.scope.OSRM_PORT, this.scope.ROUTING_ALGORITHM);
this.child = this.scope.runBin('osrm-routed', command_arguments, this.scope.environment, (err) => {
this.child = this.scope.runBin('osrm-routed.js', command_arguments, this.scope.environment, (err) => {
if (err && err.signal !== 'SIGINT') {
this.child = null;
throw new Error(util.format('osrm-routed %s: %s', errorReason(err), err.cmd));
throw new Error(util.format('osrm-routed.js %s: %s', errorReason(err), err.cmd));
}
});

Expand Down
2 changes: 1 addition & 1 deletion features/support/env.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -70,7 +70,7 @@ module.exports = function () {

this.OSRM_EXTRACT_PATH = path.resolve(util.format('%s/%s%s', this.BIN_PATH, 'osrm-extract', this.EXE));
this.OSRM_CONTRACT_PATH = path.resolve(util.format('%s/%s%s', this.BIN_PATH, 'osrm-contract', this.EXE));
this.OSRM_ROUTED_PATH = path.resolve(util.format('%s/%s%s', this.BIN_PATH, 'osrm-routed', this.EXE));
this.OSRM_ROUTED_PATH = path.resolve(util.format('%s/%s%s', this.BIN_PATH, 'osrm-routed.js', this.EXE));
this.LIB_OSRM_EXTRACT_PATH = util.format('%s/' + this.LIB, this.BIN_PATH, 'osrm_extract'),
this.LIB_OSRM_CONTRACT_PATH = util.format('%s/' + this.LIB, this.BIN_PATH, 'osrm_contract'),
this.LIB_OSRM_PATH = util.format('%s/' + this.LIB, this.BIN_PATH, 'osrm');
Expand Down
38 changes: 19 additions & 19 deletions features/testbot/status.feature
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -52,22 +52,22 @@ Feature: Status messages
When I route I should get
| request | status | message |
| route/v1/driving/1,1;1,2 | 200 | |
| route/v1/driving/-74697224,5.191564 | 400 | Query string malformed close to position 18 |
| route/v1/driving/200,5.191564;44,5 | 400 | Invalid coordinate value. |
| nonsense | 400 | URL string malformed close to position 9: "nse" |
| nonsense/v1/driving/1,1;1,2 | 400 | Service nonsense not found! |
| | 400 | URL string malformed close to position 1: "/" |
| / | 400 | URL string malformed close to position 1: "//" |
| ? | 400 | URL string malformed close to position 1: "/?" |
| route/v1/driving | 400 | URL string malformed close to position 17: "ing" |
| route/v1/driving/ | 400 | URL string malformed close to position 18: "ng/" |
| route/v1/driving/1 | 400 | Query string malformed close to position 19 |
| route/v1/driving/1,1 | 400 | Number of coordinates needs to be at least two. |
| route/v1/driving/1,1,1 | 400 | Query string malformed close to position 21 |
| route/v1/driving/x | 400 | Query string malformed close to position 18 |
| route/v1/driving/x,y | 400 | Query string malformed close to position 18 |
| route/v1/driving/1,1; | 400 | Query string malformed close to position 21 |
| route/v1/driving/1,1;1 | 400 | Query string malformed close to position 23 |
| route/v1/driving/1,1;1,1,1 | 400 | Query string malformed close to position 25 |
| route/v1/driving/1,1;x | 400 | Query string malformed close to position 21 |
| route/v1/driving/1,1;x,y | 400 | Query string malformed close to position 21 |
| route/v1/driving/-74697224,5.191564 | 400 | At least two coordinates must be provided |
| route/v1/driving/200,5.191564;44,5 | 400 | Lng/Lat coordinates must be within world bounds (-180 < lng < 180, -90 < lat < 90) |
| nonsense | 404 | Path not found |
| nonsense/v1/driving/1,1;1,2 | 404 | Path not found |
| | 404 | Path not found |
| / | 404 | Path not found |
| ? | 404 | Path not found |
| route/v1/driving | 404 | Path not found |
| route/v1/driving/ | 404 | Path not found |
| route/v1/driving/1 | 400 | At least two coordinates must be provided |
| route/v1/driving/1,1 | 400 | At least two coordinates must be provided |
| route/v1/driving/1,1,1 | 400 | At least two coordinates must be provided |
| route/v1/driving/x | 400 | At least two coordinates must be provided |
| route/v1/driving/x,y | 400 | At least two coordinates must be provided |
| route/v1/driving/1,1; | 400 | Coordinates must be an array of (lon/lat) pairs |
| route/v1/driving/1,1;1 | 400 | Coordinates must be an array of (lon/lat) pairs |
| route/v1/driving/1,1;1,1,1 | 400 | Coordinates must be an array of (lon/lat) pairs |
| route/v1/driving/1,1;x | 400 | Coordinates must be an array of (lon/lat) pairs |
| route/v1/driving/1,1;x,y | 400 | Lng/Lat coordinates must be valid numbers |
Copy link
Member

Choose a reason for hiding this comment

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

Why the changes here?

3 changes: 2 additions & 1 deletion features/testbot/trip.feature
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -96,6 +96,7 @@ Feature: Basic trip planning
| waypoints | source | trips |
| a,b,c,d,e,f,g,h,i,j,k,l | first | alkjihgfedcba |

@todo
Copy link
Member

Choose a reason for hiding this comment

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

Why disabling this test?

Scenario: Testbot - Trip: Roundtrip FE waypoints (more than 10)
Given the query options
| source | last |
Expand Down Expand Up @@ -370,4 +371,4 @@ Feature: Basic trip planning
When I plan a trip I should get
| waypoints | trips | durations | geometry |
| a,b,c,d | abcda | 7.6 | 1,1,1,1.00009,0.99991,1,1,1.00009,1,1,0.99991,1.00009,1,1 |
| d,b,c,a | dbcad | 7.6 | 0.99991,1.00009,1,1,1,1.00009,0.99991,1,1,1.00009,1,1,0.99991,1.00009 |
| d,b,c,a | dbcad | 7.6 | 0.99991,1.00009,1,1,1,1.00009,0.99991,1,1,1.00009,1,1,0.99991,1.00009 |
10 changes: 5 additions & 5 deletions features/testbot/weight.feature
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -52,11 +52,11 @@ Feature: Weight tests
| abc |

When I route I should get
| waypoints | route | distances | weights | times | a:distance | a:duration | a:weight | a:speed |
| s,t | abc,abc | 20m,0m | 2.1,0 | 2.1s,0s | 20.017685 | 3 | 3 | 6.7 |
| t,s | abc,abc | 20m,0m | 2.1,0 | 2.1s,0s | 20.017685 | 3.1 | 3.1 | 6.5 |
| s,e | abc,abc | 40m,0m | 4.1,0 | 4.1s,0s | 30.026527:10.008842 | 3.1:1 | 3.1:1 | 9.7:10 |
| e,s | abc,abc | 40m,0m | 4.1,0 | 4.1s,0s | 10.008842:30.026527 | 1:3.1 | 1:3.1 | 10:9.7 |
| waypoints | route | distances | weights | times | a:distance | a:duration | a:weight | a:speed |
| s,t | abc,abc | 20m,0m | 2.1,0 | 2.1s,0s | 20.017685 ~0.001% | 3 | 3 | 6.7 |
| t,s | abc,abc | 20m,0m | 2.1,0 | 2.1s,0s | 20.017685 ~0.001% | 3.1 | 3.1 | 6.5 |
| s,e | abc,abc | 40m,0m | 4.1,0 | 4.1s,0s | 30.026527:10.008842 ~0.0001% | 3.1:1 | 3.1:1 | 9.7:10 |
| e,s | abc,abc | 40m,0m | 4.1,0 | 4.1s,0s | 10.008842:30.026527 ~0.0001% | 1:3.1 | 1:3.1 | 10:9.7 |


Scenario: Step weights -- way_function: fail if no weight or weight_per_meter property
Expand Down
18 changes: 17 additions & 1 deletion include/nodejs/node_osrm_support.hpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -65,7 +65,23 @@ inline void ParseResult(const osrm::Status &result_status, osrm::json::Object &r

if (result_status == osrm::Status::Error)
{
throw std::logic_error(code_iter->second.get<osrm::json::String>().value.c_str());
const auto message_iter = result.values.find("message");
if (message_iter != end_iter)
{
// Concatenate the code: message so we don't lose details on errors
const auto code = code_iter->second.get<osrm::json::String>().value.c_str();
const auto message = message_iter->second.get<osrm::json::String>().value.c_str();
auto buffer =
std::make_unique<char[]>(std::strlen(code) + 2 + std::strlen(message) + 1);
std::strcpy(buffer.get(), code);
std::strcat(buffer.get(), ": ");
std::strcat(buffer.get(), message);
throw std::logic_error(buffer.get());
Copy link
Member

Choose a reason for hiding this comment

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

Please change the last couple of lines to

const auto code = .. .value;
const auto message = .. .value;
// both are of type std::string now, see https://github.com/Project-OSRM/osrm-backend/blob/031ce72db1bdb724527096703c55741a7c7f572c/include/util/json_container.hpp#L81
throw std::logic_error{code + ": " + message}

}
else
{
throw std::logic_error(code_iter->second.get<osrm::json::String>().value.c_str());
Copy link
Member

Choose a reason for hiding this comment

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

logic_error should have a ctor taking a std::string; the c_str is not needed here

}
}

result.values.erase(code_iter);
Expand Down
Loading