-
Notifications
You must be signed in to change notification settings - Fork 3.5k
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
Add Flatbuffers support to NodeJS bindings #6338
Changes from all commits
8913634
44db19a
b296165
53f418c
455815e
8394e08
c2da4c3
e45fb31
41c80d5
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -2,8 +2,7 @@ | |
#define OSRM_BINDINGS_NODE_SUPPORT_HPP | ||
|
||
#include "nodejs/json_v8_renderer.hpp" | ||
#include "util/json_renderer.hpp" | ||
|
||
#include "engine/api/flatbuffers/fbresult_generated.h" | ||
#include "osrm/approach.hpp" | ||
#include "osrm/bearing.hpp" | ||
#include "osrm/coordinate.hpp" | ||
|
@@ -18,6 +17,7 @@ | |
#include "osrm/table_parameters.hpp" | ||
#include "osrm/tile_parameters.hpp" | ||
#include "osrm/trip_parameters.hpp" | ||
#include "util/json_renderer.hpp" | ||
|
||
#include <boost/assert.hpp> | ||
#include <boost/optional.hpp> | ||
|
@@ -26,6 +26,7 @@ | |
#include <iostream> | ||
#include <iterator> | ||
#include <sstream> | ||
#include <stdexcept> | ||
#include <string> | ||
#include <vector> | ||
|
||
|
@@ -46,7 +47,7 @@ using table_parameters_ptr = std::unique_ptr<osrm::TableParameters>; | |
|
||
struct PluginParameters | ||
{ | ||
bool renderJSONToBuffer = false; | ||
bool renderToBuffer = false; | ||
}; | ||
|
||
using ObjectOrString = typename mapbox::util::variant<osrm::json::Object, std::string>; | ||
|
@@ -96,6 +97,17 @@ inline void ParseResult(const osrm::Status &result_status, osrm::json::Object &r | |
} | ||
|
||
inline void ParseResult(const osrm::Status & /*result_status*/, const std::string & /*unused*/) {} | ||
inline void ParseResult(const osrm::Status &result_status, | ||
const flatbuffers::FlatBufferBuilder &fbs_builder) | ||
{ | ||
auto fbs_result = osrm::engine::api::fbresult::GetFBResult(fbs_builder.GetBufferPointer()); | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Done using ParseResult for JSON as a reference. We also erase |
||
|
||
if (result_status == osrm::Status::Error) | ||
{ | ||
BOOST_ASSERT(fbs_result->code()); | ||
throw std::logic_error(fbs_result->code()->message()->c_str()); | ||
} | ||
} | ||
|
||
inline engine_config_ptr argumentsToEngineConfig(const Nan::FunctionCallbackInfo<v8::Value> &args) | ||
{ | ||
|
@@ -725,6 +737,36 @@ inline bool argumentsToParameter(const Nan::FunctionCallbackInfo<v8::Value> &arg | |
} | ||
} | ||
|
||
if (Nan::Has(obj, Nan::New("format").ToLocalChecked()).FromJust()) | ||
{ | ||
v8::Local<v8::Value> format = | ||
Nan::Get(obj, Nan::New("format").ToLocalChecked()).ToLocalChecked(); | ||
if (format.IsEmpty()) | ||
{ | ||
return false; | ||
} | ||
|
||
if (!format->IsString()) | ||
{ | ||
Nan::ThrowError("format must be a string: \"json\" or \"flatbuffers\""); | ||
return false; | ||
} | ||
|
||
std::string format_str = *Nan::Utf8String(format); | ||
if (format_str == "json") | ||
{ | ||
params->format = osrm::engine::api::BaseParameters::OutputFormatType::JSON; | ||
} | ||
else if (format_str == "flatbuffers") | ||
{ | ||
params->format = osrm::engine::api::BaseParameters::OutputFormatType::FLATBUFFERS; | ||
} | ||
else | ||
{ | ||
Nan::ThrowError("format must be a string: \"json\" or \"flatbuffers\""); | ||
return false; | ||
} | ||
} | ||
return true; | ||
} | ||
|
||
|
@@ -885,17 +927,18 @@ inline bool parseCommonParameters(const v8::Local<v8::Object> &obj, ParamType &p | |
return true; | ||
} | ||
|
||
inline PluginParameters | ||
argumentsToPluginParameters(const Nan::FunctionCallbackInfo<v8::Value> &args) | ||
inline PluginParameters argumentsToPluginParameters( | ||
const Nan::FunctionCallbackInfo<v8::Value> &args, | ||
const boost::optional<osrm::engine::api::BaseParameters::OutputFormatType> &output_format = {}) | ||
{ | ||
if (args.Length() < 3 || !args[1]->IsObject()) | ||
{ | ||
return {}; | ||
// output to buffer by default for Flatbuffers | ||
return {output_format == osrm::engine::api::BaseParameters::OutputFormatType::FLATBUFFERS}; | ||
} | ||
v8::Local<v8::Object> obj = Nan::To<v8::Object>(args[1]).ToLocalChecked(); | ||
if (Nan::Has(obj, Nan::New("format").ToLocalChecked()).FromJust()) | ||
{ | ||
|
||
v8::Local<v8::Value> format = | ||
Nan::Get(obj, Nan::New("format").ToLocalChecked()).ToLocalChecked(); | ||
if (format.IsEmpty()) | ||
|
@@ -905,7 +948,7 @@ argumentsToPluginParameters(const Nan::FunctionCallbackInfo<v8::Value> &args) | |
|
||
if (!format->IsString()) | ||
{ | ||
Nan::ThrowError("format must be a string: \"object\" or \"json_buffer\""); | ||
Nan::ThrowError("format must be a string: \"object\" or \"buffer\""); | ||
return {}; | ||
} | ||
|
||
|
@@ -914,20 +957,35 @@ argumentsToPluginParameters(const Nan::FunctionCallbackInfo<v8::Value> &args) | |
|
||
if (format_str == "object") | ||
{ | ||
if (output_format == osrm::engine::api::BaseParameters::OutputFormatType::FLATBUFFERS) | ||
{ | ||
Nan::ThrowError("Flatbuffers result can only output to buffer."); | ||
return {true}; | ||
} | ||
return {false}; | ||
} | ||
else if (format_str == "buffer") | ||
{ | ||
return {true}; | ||
} | ||
else if (format_str == "json_buffer") | ||
{ | ||
if (output_format && | ||
output_format != osrm::engine::api::BaseParameters::OutputFormatType::JSON) | ||
{ | ||
Nan::ThrowError("Deprecated `json_buffer` can only be used with JSON format"); | ||
} | ||
return {true}; | ||
} | ||
else | ||
{ | ||
Nan::ThrowError("format must be a string: \"object\" or \"json_buffer\""); | ||
Nan::ThrowError("format must be a string: \"object\" or \"buffer\""); | ||
return {}; | ||
} | ||
} | ||
|
||
return {}; | ||
// output to buffer by default for Flatbuffers | ||
return {output_format == osrm::engine::api::BaseParameters::OutputFormatType::FLATBUFFERS}; | ||
} | ||
|
||
inline route_parameters_ptr | ||
|
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -10,6 +10,7 @@ | |
|
||
#include <exception> | ||
#include <sstream> | ||
#include <stdexcept> | ||
#include <type_traits> | ||
#include <utility> | ||
|
||
|
@@ -128,8 +129,7 @@ inline void async(const Nan::FunctionCallbackInfo<v8::Value> &info, | |
auto params = argsToParams(info, requires_multiple_coordinates); | ||
if (!params) | ||
return; | ||
|
||
auto pluginParams = argumentsToPluginParameters(info); | ||
auto pluginParams = argumentsToPluginParameters(info, params->format); | ||
|
||
BOOST_ASSERT(params->IsValid()); | ||
|
||
|
@@ -156,20 +156,41 @@ inline void async(const Nan::FunctionCallbackInfo<v8::Value> &info, | |
void Execute() override | ||
try | ||
{ | ||
osrm::engine::api::ResultT r; | ||
r = osrm::util::json::Object(); | ||
const auto status = ((*osrm).*(service))(*params, r); | ||
auto json_result = r.get<osrm::json::Object>(); | ||
ParseResult(status, json_result); | ||
if (pluginParams.renderJSONToBuffer) | ||
switch ( | ||
params->format.value_or(osrm::engine::api::BaseParameters::OutputFormatType::JSON)) | ||
{ | ||
std::ostringstream buf; | ||
osrm::util::json::render(buf, json_result); | ||
result = buf.str(); | ||
case osrm::engine::api::BaseParameters::OutputFormatType::JSON: | ||
{ | ||
osrm::engine::api::ResultT r; | ||
r = osrm::util::json::Object(); | ||
const auto status = ((*osrm).*(service))(*params, r); | ||
auto &json_result = r.get<osrm::json::Object>(); | ||
ParseResult(status, json_result); | ||
if (pluginParams.renderToBuffer) | ||
{ | ||
std::ostringstream buf; | ||
osrm::util::json::render(buf, json_result); | ||
result = buf.str(); | ||
} | ||
else | ||
{ | ||
result = json_result; | ||
} | ||
} | ||
else | ||
break; | ||
case osrm::engine::api::BaseParameters::OutputFormatType::FLATBUFFERS: | ||
{ | ||
result = json_result; | ||
osrm::engine::api::ResultT r = flatbuffers::FlatBufferBuilder(); | ||
const auto status = ((*osrm).*(service))(*params, r); | ||
const auto &fbs_result = r.get<flatbuffers::FlatBufferBuilder>(); | ||
ParseResult(status, fbs_result); | ||
BOOST_ASSERT(pluginParams.renderToBuffer); | ||
std::string result_str( | ||
reinterpret_cast<const char *>(fbs_result.GetBufferPointer()), | ||
fbs_result.GetSize()); | ||
result = std::move(result_str); | ||
} | ||
break; | ||
} | ||
} | ||
catch (const std::exception &e) | ||
|
@@ -298,6 +319,7 @@ inline void asyncForTiles(const Nan::FunctionCallbackInfo<v8::Value> &info, | |
* @param {Array} [options.approaches] Keep waypoints on curb side. Can be `null` (unrestricted, default) or `curb`. | ||
* `null`/`true`/`false` | ||
* @param {Array} [options.waypoints] Indices to coordinates to treat as waypoints. If not supplied, all coordinates are waypoints. Must include first and last coordinate index. | ||
* @param {String} [options.format] Which output format to use, either `json`, or [`flatbuffers`](https://github.com/Project-OSRM/osrm-backend/tree/master/include/engine/api/flatbuffers). | ||
* @param {String} [options.snapping] Which edges can be snapped to, either `default`, or `any`. `default` only snaps to edges marked by the profile as `is_startpoint`, `any` will allow snapping to any edge in the routing graph. | ||
* @param {Function} callback | ||
* | ||
|
@@ -338,6 +360,7 @@ NAN_METHOD(Engine::route) // | |
* @param {Number} [options.number=1] Number of nearest segments that should be returned. | ||
* Must be an integer greater than or equal to `1`. | ||
* @param {Array} [options.approaches] Keep waypoints on curb side. Can be `null` (unrestricted, default) or `curb`. | ||
* @param {String} [options.format] Which output format to use, either `json`, or [`flatbuffers`](https://github.com/Project-OSRM/osrm-backend/tree/master/include/engine/api/flatbuffers). | ||
* @param {String} [options.snapping] Which edges can be snapped to, either `default`, or `any`. `default` only snaps to edges marked by the profile as `is_startpoint`, `any` will allow snapping to any edge in the routing graph. | ||
* @param {Function} callback | ||
* | ||
|
@@ -604,12 +627,15 @@ NAN_METHOD(Engine::trip) // | |
* @name Configuration | ||
* @param {Object} [plugin_config] - Object literal containing parameters for the trip query. | ||
* @param {String} [plugin_config.format] The format of the result object to various API calls. | ||
* Valid options are `object` (default), which returns a | ||
* standard Javascript object, as described above, and `json_buffer`, which will return a NodeJS | ||
* **[Buffer](https://nodejs.org/api/buffer.html)** object, containing a JSON string. The latter has | ||
* the advantage that it can be immediately serialized to disk/sent over the network, and the | ||
* generation of the string is performed outside the main NodeJS event loop. This option is ignored | ||
* by the `tile` plugin. | ||
* Valid options are `object` (default if `options.format` is | ||
* `json`), which returns a standard Javascript object, as described above, and `buffer`(default if | ||
* `options.format` is `flatbuffers`), which will return a NodeJS | ||
* **[Buffer](https://nodejs.org/api/buffer.html)** object, containing a JSON string or Flatbuffers | ||
* object. The latter has the advantage that it can be immediately serialized to disk/sent over the | ||
* network, and the generation of the string is performed outside the main NodeJS event loop. This | ||
* option is ignored by the `tile` plugin. Also note that `options.format` set to `flatbuffers` | ||
* cannot be used with `plugin_config.format` set to `object`. `json_buffer` is deprecated alias for | ||
* `buffer`. | ||
* | ||
* @example | ||
* var osrm = new OSRM('network.osrm'); | ||
|
@@ -619,7 +645,7 @@ NAN_METHOD(Engine::trip) // | |
* [13.374481201171875, 52.506191342034576] | ||
* ] | ||
* }; | ||
* osrm.route(options, { format: "json_buffer" }, function(err, response) { | ||
* osrm.route(options, { format: "buffer" }, function(err, response) { | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Renamed it to just There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Unlikely, but I guess it's possible someone could ask for the
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Well, at the moment all our Node bindings accept options and plugin options. This That's the reason why I decided to do it exactly how I did it(btw initially I did almost like you proposed: WDYT? In general I don't have any strong opinion here, IMO any solution here is good enough, but just sharing motivation behind current implementation. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more.
I doubt that something except JSON may need There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Also may be the fact that both fields are called There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Another option I see is to deprecate plugin options at all and move buffer/object to There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Yeah I don't think the Node API was considered when flat buffers were added. I guess right now it would just return JSON. I think the issue I have is aliasing There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. In other words you just propose to add condition to throw error in the case if
Already mention that in docstring, not sure if it is possible to do that somehow better. WDYT? There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Yep, sounds good 👍 |
||
* if (err) throw err; | ||
* console.log(response.toString("utf-8")); | ||
* }); | ||
|
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -4,6 +4,24 @@ var data_path = require('./constants').data_path; | |
var mld_data_path = require('./constants').mld_data_path; | ||
var three_test_coordinates = require('./constants').three_test_coordinates; | ||
var two_test_coordinates = require('./constants').two_test_coordinates; | ||
const flatbuffers = require('../../features/support/flatbuffers').flatbuffers; | ||
const FBResult = require('../../features/support/fbresult_generated').osrm.engine.api.fbresult.FBResult; | ||
|
||
|
||
test('match: match in Monaco with flatbuffers format', function(assert) { | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Added just a couple of smoke tests for each plugin: I assume that it is enough for the beginning. |
||
assert.plan(2); | ||
var osrm = new OSRM(data_path); | ||
var options = { | ||
coordinates: three_test_coordinates, | ||
timestamps: [1424684612, 1424684616, 1424684620], | ||
format: 'flatbuffers' | ||
}; | ||
osrm.match(options, function(err, response) { | ||
assert.ifError(err); | ||
const fb = FBResult.getRootAsFBResult(new flatbuffers.ByteBuffer(response)); | ||
assert.equal(fb.routesLength(), 1); | ||
}); | ||
}); | ||
|
||
test('match: match in Monaco', function(assert) { | ||
assert.plan(5); | ||
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think these need to be reworded to clarify that flatbuffers can only be output to
buffer
. It's not just the default case, it's the only case.There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Hm, agree, but not sure how to better express that. And also we already have
A bit below these lines.
Do you have good ideas?