Require a radius parameter when using bearings

[whytro]

Issue

What issue is this PR targeting? If there is no issue that addresses the problem, please open a corresponding issue and link it here.

#2983 Require a radius parameter other than unlimited when using bearings

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?

[nilsnolde]

Is this for GSoC by any chance?

[whytro]

Is this for GSoC by any chance?

Yes, this was intended for GSoC - I apologize if I was to explicitly state that beforehand!

[nilsnolde]

It’s just easier to keep track of things that way. Anyways, thanks for the contribution. I’m not familiar with the codebase to give it a review, but I’m sure others will soonish.

Should we flag this as pre-gsoc so it’s easier to find when the time comes?

[whytro]

Should we flag this as pre-gsoc so it’s easier to find when the time comes?

I think that would be a good idea!

[whytro]

I should note that I did fix the formatting by rerunning the format script - though it did seem to come with some strange looking results due to the increased length.

[nilsnolde]

As this is for GSoC, would you have time to have a look @SiarheiFedartsou ?

[SiarheiFedartsou]

@nilsnolde sure, it was already in my todo-list, but just was a bit busy last days - will try to do that in the nearest days 👍

[SiarheiFedartsou]

Hey @whytro
Thanks for working on this!
I took a quick look into this PR and it seems that everything is okay in general(but I will additionally look into details later), but it seems that we don't fix behaviour for NodeJs bindings here in any way. And it seems to be the reason why we have NodeJs tests failure on CI:

✓ route: integer bearing values no longer supported
# route: valid bearing values
[assert][139638292518848] /home/runner/work/osrm-backend/osrm-backend/src/nodejs/node_osrm.cpp:117
in: void node_osrm::async(const Napi::CallbackInfo &, ParameterParser, ServiceMemFn, bool) [ParameterParser = std::unique_ptr<osrm::engine::api::RouteParameters, std::default_delete<osrm::engine::api::RouteParameters> > (*)(const Napi::CallbackInfo &, bool), ServiceMemFn = osrm::engine::Status (osrm::OSRM::*)(const osrm::engine::api::RouteParameters &, mapbox::util::variant<osrm::util::json::Object, std::__cxx11::basic_string<char>, flatbuffers::FlatBufferBuilder> &) const]: params->IsValid()

This assert comes from here:

osrm-backend/src/nodejs/node_osrm.cpp

Line 117 in 82b73ed

BOOST_ASSERT(params->IsValid());

So I think we need to at least fix NodeJs tests to respect this new requirement. In perfect scenario I would even add some user-friendly exception in NodeJs bindings for the case of this error(but we can do it in separate PR later). It can be done somewhere here:

osrm-backend/include/nodejs/node_osrm_support.hpp

Line 519 in 82b73ed

if (obj.Has("bearings"))

Let me please know if something is not clear or you will need a help with it :)

[whytro]

Ah, that was sloppy of me! I've gone ahead and adjusted the bindings and the tests accordingly. Thank you for the pointers!

[whytro]

I originally thought that the unit tests were for release builds (hence not asserting on parameter checks - which, in hindsight, is a nonsensical assumption), but now I'm realizing that parameter checking tests are maybe not applicable for unit testing, and are mainly to be covered in the cucumber tests. I'll take a closer look at implementing those tests in the appropriate (cucumber) feature sections, and revert the unit tests change as soon as possible!

[SiarheiFedartsou]

I originally thought that the unit tests were for release builds (hence not asserting on parameter checks - which, in hindsight, is a nonsensical assumption), but now I'm realizing that parameter checking tests are maybe not applicable for unit testing, and are mainly to be covered in the cucumber tests. I'll take a closer look at implementing those tests in the appropriate (cucumber) feature sections, and revert the unit tests change as soon as possible!

But can we change unit tests in such a way to not cause those asserts?

[danpat]

I also want to note that this is an API-breaking change - clients that were making requests using just bearings before will suddenly stop working. Merging this feature will require a new major release version - is anyone game for doing that?

[SiarheiFedartsou]

I also want to note that this is an API-breaking change - clients that were making requests using just bearings before will suddenly stop working. Merging this feature will require a new major release version - is anyone game for doing that?

Hm, indeed 🤔 For new major version it would be great to also do other "cleanup" tasks like #2879

I will think how can we merge it without breaking changes though, may be go with 2nd option we had in original issue bearings implies that radius=SOME_DEFAULT_VALUE? WDYT?

[danpat]

Even defaulting radiuses= to something other than unlimited I would consider a breaking change. Adding an option to allow it to be configurable (e.g. something like osrm-routed --default-radius-for-bearings=100) would be OK, as long as the default remained unlimited - users that upgrade their server should expect their clients to receive the same results if they make no other changes - anything else is a breaking change IMO. They can always explicitly add radiuses= today if the current default behaviour is not what they want.

I think it's OK to merge this feature as a breaking change, but I do think it's worth considering the implications - it does require a major version bump, and if this is the only feature to trigger that, then it seems a bit silly. Perhaps merging this unblocks a period of other major breaking changes and starts the train rolling towards a reasonably significant major release.

[SiarheiFedartsou]

Perhaps merging this unblocks a period of other major breaking changes and starts the train rolling towards a reasonably significant major release.

It is what I am also thinking about 🤔 But the problem is that there are actually almost no people actively working on this project, so not sure we really want it...
On another hand I don't see next release on a horizon at all and we can just agree that next release will be major update and it would unblock very slow work on adding other breaking changes we wanted to do.

[whytro]

Hmm, I'm not sure if the assertion is avoidable in this case without significant alteration to the library. It also doesn't seem like similar tests (ie. for param validation checking) exist, and instead seem to focus on the calculation results (please let me know if I'm wrong here).

I did also consider that this change would probably be breaking given that it changes behavior significantly for bearings. Should I look to either implement a flag or add in default radius values when bearings are used, so that it can be easily changed should a major release happens - or rather what would be the best approach here?

[SiarheiFedartsou]

I did also consider that this change would probably be breaking given that it changes behavior significantly for bearings. Should I look to either implement a flag or add in default radius values when bearings are used, so that it can be easily changed should a major release happens - or rather what would be the best approach here?

I would probably start from flag, I totally agree with Daniel's points above. Sorry, that I didn't notice it in the beginning - it seems to be quite big rework of this PR... Also I still not sure if we want to go ahead with major version update - probably it can be a good idea to start new PR and to leave this PR "as is" as it seems what you did here is better default behaviour than it used to be (the only problem is backward compatibility).

[whytro]

Okay, I'll look into adding the configurable default radius flags and open up a separate PR for it when ready!

[whytro]

Sorry for the force-push, I messed up a rebase and polluted the commit history on the branch, and rolled it back.
These changes are mainly aimed at taking a cleaner approach to the problem overall, dependent on #6599, and expectant on a future change where the server doesn't default to unlimited (only one line would need a change) - and as the previous version of this would not have been drop-in compatible with the default_radius flag.
Essentially, the goal for this was to just add support for if/when the server doesn't default to an infinite value, or have a flag in which radii must be specified - where the error becomes more relevant.