Add Flatbuffers support to NodeJS bindings #6338

SiarheiFedartsou · 2022-08-27T12:02:20Z

Issue

closes #5936

Tasklist

CHANGELOG.md entry (How to write a changelog entry)
update relevant Wiki pages
add tests (see testing documentation)
review
adjust for comments
cherry pick to release branch

Requirements / Relations

Link any requirements here. Other pull requests this PR is based on?

SiarheiFedartsou · 2022-08-27T12:12:18Z

test/nodejs/route.js

@@ -5,6 +5,43 @@ var monaco_mld_path = require('./constants').mld_data_path;
 var monaco_corech_path = require('./constants').corech_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;


Here I had to import flatbuffers related things from features folder, IMO it is a bit ugly, but not a big problem - as an alternative we probably can re-arrange folders somehow or add symlink to that folder in test/nodejs.

SiarheiFedartsou · 2022-08-27T12:14:09Z

include/nodejs/node_osrm_support.hpp

+inline void ParseResult(const osrm::Status &result_status,
+                        const flatbuffers::FlatBufferBuilder &fbs_builder)
+{
+    auto fbs_result = osrm::engine::api::fbresult::GetFBResult(fbs_builder.GetBufferPointer());


Done using ParseResult for JSON as a reference. We also erase message and code there, but it is not so easy for flatbuffers(it is kind of readonly) and tbh I don't get why are we doing that at all.

SiarheiFedartsou · 2022-08-27T12:17:22Z

test/nodejs/match.js

+const FBResult = require('../../features/support/fbresult_generated').osrm.engine.api.fbresult.FBResult;
+
+
+test('match: match in Monaco with flatbuffers format', function(assert) {


Added just a couple of smoke tests for each plugin: I assume that it is enough for the beginning.

SiarheiFedartsou · 2022-08-27T12:19:02Z

src/nodejs/node_osrm.cpp

@@ -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) {


Renamed it to just buffer having json_buffer as deprecated alias.

Unlikely, but I guess it's possible someone could ask for the json_buffer plugin format and the Flatbuffer response format. Maybe it's worth keeping the three plugin options separate for clarity.

Object -> json object

json_buffer -> json string

flatbuffer...buffer -> flatbuffer bytes

Well, at the moment all our Node bindings accept options and plugin options. This format which currently exists in Node bindings and can be either object or json_buffer is the only citizen of these plugin options. At the same time options contain everything which we usually accept through our HTTP API and now we have new format parameter there which can be either json or flatbuffers and it 100% mirrors what we have in HTTP API(we have format there too). So my idea here is that: format from plugin options now controls the way how we receive data on Node side(in JSON or in byte buffer) and format from options controls representation of data itself(imagine tomorrow we can start supporting yaml as response representation and return it as both object and buffer)

That's the reason why I decided to do it exactly how I did it(btw initially I did almost like you proposed: object/json_buffer/flat_buffer, but then realized that plugin options and options are a bit different things and if we have this new format as separate field in options it will be a bit more consistent).

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.

imagine tomorrow we can start supporting yaml as response representation and return it as both object and buffer

I doubt that something except JSON may need object though :)

Also may be the fact that both fields are called format is a bit confusing 🤔

Another option I see is to deprecate plugin options at all and move buffer/object to format in usual options.

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 json_buffer to buffer. If we can limit json_buffer to JSON responses, error when they try to set flatbuffers, and mark json_buffer as deprecated, then I'm ok with this.

In other words you just propose to add condition to throw error in the case if json_buffer is used along with flatbuffers? Done.

and mark json_buffer as deprecated

Already mention that in docstring, not sure if it is possible to do that somehow better.

WDYT?

Yep, sounds good 👍

mjjbell · 2022-08-27T15:15:32Z

docs/nodejs/api.md

-        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


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.

Hm, agree, but not sure how to better express that. And also we already have

Also note that `options.format` set to `flatbuffers` cannot be used with `plugin_config.format` set to `object`

A bit below these lines.

Do you have good ideas?

v5.27.0 - Changes from 5.26.0 - API: - ADDED: Add Flatbuffers support to NodeJS bindings. [Project-OSRM#6338](Project-OSRM#6338) - CHANGED: Add `data_version` field to responses of all services. [Project-OSRM#5387](Project-OSRM#5387) - FIXED: Use Boost.Beast to parse HTTP request. [Project-OSRM#6294](Project-OSRM#6294) - FIXED: Fix inefficient osrm-routed connection handling [Project-OSRM#6113](https://gihub.com/Project-OSRM/osrm-backend/pull/6113) - FIXED: Fix HTTP compression precedence [Project-OSRM#6113](Project-OSRM#6113) - NodeJS: - FIXED: Support `skip_waypoints` in Node bindings [Project-OSRM#6060](Project-OSRM#6060) - Misc: - ADDED: conanbuildinfo.json for easy reading of dependencies [Project-OSRM#6388](Project-OSRM#6388) - CHANGED: Improve performance of JSON rendering. Fix undefined behaviour in JSON numbers formatting. [Project-OSRM#6380](Project-OSRM#6380) - ADDED: Add timestamps for logs. [Project-OSRM#6375](Project-OSRM#6375) - CHANGED: Improve performance of map matching via getPathDistance optimization. [Project-OSRM#6378](Project-OSRM#6378) - CHANGED: Optimize RestrictionParser performance. [Project-OSRM#6344](Project-OSRM#6344) - ADDED: Support floats for speed value in traffic updates CSV. [Project-OSRM#6327](Project-OSRM#6327) - CHANGED: Use Lua 5.4 in Docker image. [Project-OSRM#6346](Project-OSRM#6346) - CHANGED: Remove redundant nullptr check. [Project-OSRM#6326](Project-OSRM#6326) - CHANGED: missing files list is included in exception message. [Project-OSRM#5360](Project-OSRM#5360) - CHANGED: Do not use deprecated Callback::Call overload in Node bindings. [Project-OSRM#6318](Project-OSRM#6318) - FIXED: Fix distance calculation consistency. [Project-OSRM#6315](Project-OSRM#6315) - FIXED: Fix performance issue after migration to sol2 3.3.0. [Project-OSRM#6304](Project-OSRM#6304) - CHANGED: Pass osm_node_ids by reference in osrm::updater::Updater class. [Project-OSRM#6298](Project-OSRM#6298) - FIXED: Fix bug with reading Set values from Lua scripts. [Project-OSRM#6285](Project-OSRM#6285) - FIXED: Bug in bicycle profile that caused exceptions if there is a highway=bicycle in the data. [Project-OSRM#6296](Project-OSRM#6296) - FIXED: Internal refactoring of identifier types used in data facade [Project-OSRM#6044](Project-OSRM#6044) - CHANGED: Update docs to reflect recent build and dependency changes [Project-OSRM#6383](Project-OSRM#6383) - Build: - REMOVED: Get rid of Mason. [Project-OSRM#6387](Project-OSRM#6387) - CHANGED: Use clang-format from CI base image. [Project-OSRM#6391](Project-OSRM#6391) - ADDED: Build Node bindings on Windows. [Project-OSRM#6334](Project-OSRM#6334) - ADDED: Configure cross-compilation for Apple Silicon. [Project-OSRM#6360](Project-OSRM#6360) - CHANGED: Use apt-get to install Clang on CI. [Project-OSRM#6345](Project-OSRM#6345) - CHANGED: Fix TBB in case of Conan + NodeJS build. [Project-OSRM#6333](Project-OSRM#6333) - CHANGED: Migrate to modern TBB version. [Project-OSRM#6300](Project-OSRM#6300) - CHANGED: Enable performance-move-const-arg clang-tidy check. [Project-OSRM#6319](Project-OSRM#6319) - CHANGED: Use the latest node on CI. [Project-OSRM#6317](Project-OSRM#6317) - CHANGED: Migrate Windows CI to GitHub Actions. [Project-OSRM#6312](Project-OSRM#6312) - ADDED: Add smoke test for Docker image. [Project-OSRM#6313](Project-OSRM#6313) - CHANGED: Update libosmium to version 2.18.0. [Project-OSRM#6303](Project-OSRM#6303) - CHANGED: Remove EXACT from find_package if using Conan. [Project-OSRM#6299](Project-OSRM#6299) - CHANGED: Configure Undefined Behaviour Sanitizer. [Project-OSRM#6290](Project-OSRM#6290) - CHANGED: Use Conan instead of Mason to install code dependencies. [Project-OSRM#6284](Project-OSRM#6284) - CHANGED: Migrate to C++17. Update sol2 to 3.3.0. [Project-OSRM#6279](Project-OSRM#6279) - CHANGED: Update macOS CI image to macos-11. [Project-OSRM#6286](Project-OSRM#6286) - CHANGED: Enable even more clang-tidy checks. [Project-OSRM#6273](Project-OSRM#6273) - CHANGED: Configure CMake to not build flatbuffers tests and samples. [Project-OSRM#6274](Project-OSRM#6274) - CHANGED: Enable more clang-tidy checks. [Project-OSRM#6270](Project-OSRM#6270) - CHANGED: Configure clang-tidy job on CI. [Project-OSRM#6261](Project-OSRM#6261) - CHANGED: Use Github Actions for building container images [Project-OSRM#6138](Project-OSRM#6138) - CHANGED: Upgrade Boost dependency to 1.70 [Project-OSRM#6113](Project-OSRM#6113) - CHANGED: Upgrade Ubuntu CI builds to 20.04 [Project-OSRM#6119](Project-OSRM#6119) - CHANGED: Make building osrm-routed optional [Project-OSRM#6144](Project-OSRM#6144) - FIXED: Run all unit tests in CI [Project-OSRM#5248](Project-OSRM#5248) - FIXED: Fix installation of Mason CMake and 32 bit CI build [Project-OSRM#6170](Project-OSRM#6170) - FIXED: Fixed Node docs generation check in CI. [Project-OSRM#6058](Project-OSRM#6058) - CHANGED: Docker build, enabled arm64 build layer [Project-OSRM#6172](Project-OSRM#6172) - CHANGED: Docker build, enabled apt-get update/install caching in separate layer for build phase [Project-OSRM#6175](Project-OSRM#6175) - FIXED: Bump CI complete meta job to ubuntu-20.04 [Project-OSRM#6323](Project-OSRM#6323) - CHANGED: Node packages are now scoped by @Project-OSRM [Project-OSRM#6386](Project-OSRM#6386) - Routing: - CHANGED: Lazily generate optional route path data [Project-OSRM#6045](Project-OSRM#6045) - FIXED: Completed support for no_entry and no_exit turn restrictions. [Project-OSRM#5988](Project-OSRM#5988) - ADDED: Add support for non-round-trips with a single fixed endpoint. [Project-OSRM#6050](Project-OSRM#6050) - FIXED: Improvements to maneuver override processing [Project-OSRM#6125](Project-OSRM#6125) - ADDED: Support snapping to multiple ways at an input location. [Project-OSRM#5953](Project-OSRM#5953) - FIXED: Fix snapping target locations to ways used in turn restrictions. [Project-OSRM#6339](Project-OSRM#6339) - ADDED: Support OSM traffic signal directions. [Project-OSRM#6153](Project-OSRM#6153) - FIXED: Ensure u-turn exists in intersection view. [Project-OSRM#6376](Project-OSRM#6376) - FIXED: Gracefully handle no-turn intersections in guidance processing. [Project-OSRM#6382](Project-OSRM#6382) - Profile: - CHANGED: Bicycle surface speeds [Project-OSRM#6212](Project-OSRM#6212) - Tools: - CHANGED: Do not generate intermediate .osrm file in osrm-extract. [Project-OSRM#6354](Project-OSRM#6354)

SiarheiFedartsou added 3 commits August 27, 2022 14:00

Add Flatbuffers support to NodeJS bindings

8913634

Add Flatbuffers support to NodeJS bindings

44db19a

Add Flatbuffers support to NodeJS bindings

b296165

SiarheiFedartsou commented Aug 27, 2022

View reviewed changes

Add Flatbuffers support to NodeJS bindings

53f418c

SiarheiFedartsou commented Aug 27, 2022

View reviewed changes

Add Flatbuffers support to NodeJS bindings

455815e

SiarheiFedartsou mentioned this pull request Aug 27, 2022

Does nodejs bindings support flatbuffer #5936

Closed

SiarheiFedartsou added 2 commits August 27, 2022 14:30

Add Flatbuffers support to NodeJS bindings

8394e08

Add Flatbuffers support to NodeJS bindings

c2da4c3

SiarheiFedartsou marked this pull request as ready for review August 27, 2022 13:18

SiarheiFedartsou added the Ready for Review label Aug 27, 2022

mjjbell reviewed Aug 27, 2022

View reviewed changes

SiarheiFedartsou added 2 commits August 29, 2022 17:49

Add Flatbuffers support to NodeJS bindings

e45fb31

Add Flatbuffers support to NodeJS bindings

41c80d5

mjjbell approved these changes Aug 29, 2022

View reviewed changes

SiarheiFedartsou merged commit b4142cf into master Aug 29, 2022

mattwigway pushed a commit to mattwigway/osrm-backend that referenced this pull request Jul 20, 2023

Add Flatbuffers support to NodeJS bindings (Project-OSRM#6338)

5422051

DennisOSRM deleted the sf-flatbuffers-nodejs branch May 3, 2024 12:29

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Add Flatbuffers support to NodeJS bindings #6338

Add Flatbuffers support to NodeJS bindings #6338

SiarheiFedartsou commented Aug 27, 2022 •

edited

Loading

SiarheiFedartsou Aug 27, 2022

SiarheiFedartsou Aug 27, 2022

SiarheiFedartsou Aug 27, 2022

SiarheiFedartsou Aug 27, 2022

mjjbell Aug 27, 2022 •

edited

Loading

SiarheiFedartsou Aug 27, 2022

SiarheiFedartsou Aug 27, 2022 •

edited

Loading

SiarheiFedartsou Aug 27, 2022

SiarheiFedartsou Aug 27, 2022

mjjbell Aug 28, 2022

SiarheiFedartsou Aug 29, 2022

mjjbell Aug 29, 2022

mjjbell Aug 27, 2022

SiarheiFedartsou Aug 27, 2022

		const FBResult = require('../../features/support/fbresult_generated').osrm.engine.api.fbresult.FBResult;


		test('match: match in Monaco with flatbuffers format', function(assert) {

Add Flatbuffers support to NodeJS bindings #6338

Add Flatbuffers support to NodeJS bindings #6338

Conversation

SiarheiFedartsou commented Aug 27, 2022 • edited Loading

Issue

Tasklist

Requirements / Relations

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

mjjbell Aug 27, 2022 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

SiarheiFedartsou Aug 27, 2022 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

SiarheiFedartsou commented Aug 27, 2022 •

edited

Loading

mjjbell Aug 27, 2022 •

edited

Loading

SiarheiFedartsou Aug 27, 2022 •

edited

Loading