Skip to content

Fix json rendering of large osm ids #7142

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

Open
wants to merge 1 commit into
base: master
Choose a base branch
from

Conversation

MarcelloPerathoner
Copy link
Contributor

@MarcelloPerathoner MarcelloPerathoner commented Mar 31, 2025

Issue

Fixes #7069 #7016 #6758 #6594 #5716

This patch introduces a new class for OSM ids to the JSON stack.

The current JSON stack rounds OSM ids to a precision of 10 digits, which is insufficient to represent current OSM node ids.

Also, the current JSON stack converts OSM ids from uint64_t to double. The double type has ~53bits of accuracy, the rest going into exponent and sign, which are wasted here. Why is this important even if OSM is still far away from using 53 bits for node ids? Because some people use high ids for their own private data to avoid conflicts with the ids in the OSM database. See: #7069

This patch stores OSM ids into uint64_t and provides a different output format thus guaranteeing correct output under all circumstances.

This pull request is an alternative to: #7096

Tasklist

@imannamdari
Copy link

Why not separate integers from doubles instead of introducing a new type (osm_id) in the JSON renderer package? Integers and doubles are well-defined types in general-purpose packages like JSON rendering, which should not have knowledge of osm ids. Otherwise, there would be no limit to custom-defined classes creeping into it.

Other popular JSON rendering frameworks also handle rendering based on types, as seen here:
protobuf-go protojson encoding

In my opinion, we should have separate Double and Integer classes instead of a single Number class, and we should not define an osm_id class within the JSON renderer, as it falls outside its scope.

@MarcelloPerathoner
Copy link
Contributor Author

Because doubles implicitly convert to integer so there would be conflicts between Json::Number and Json::Integer. Even if you fix it with templates and requires, you will still mess up a lot of tests that expect Number and not Integer.

@imannamdari
Copy link

imannamdari commented Apr 3, 2025

The point is that we should explicitly specify the type. If the value is an integer, we should use json::Integer; if it’s a double, we should use json::Double.

For example, in this line:
https://github.com/Project-OSRM/osrm-backend/blob/master/include/engine/api/nearest_api.hpp#L107
we should use emplace_back(json::Integer(node_values.first)) instead of just emplace_back(node_values.first), which leaves it up to the compiler to decide the type—potentially causing ambiguity between the two.

@MarcelloPerathoner
Copy link
Contributor Author

We should have done that. But programmers are lazy and the existing code relies on the compiler to select the right type. If we require explicit construction we'd have to change the code in a lot of places. All construction of Json::Value and all assignments to Json::Value will have to be changed.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Node IDs > 2^34 are reported by route matching incorrect.
2 participants