-
Notifications
You must be signed in to change notification settings - Fork 3.6k
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
base: master
Are you sure you want to change the base?
Fix json rendering of large osm ids #7142
Conversation
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: 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. |
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. |
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: |
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. |
@MarcelloPerathoner and @imannamdari do you guys have an idea on whether this is incoorperated in the v6.0.0 version or not? I use the image ghcr.io/project-osrm/osrm-backend:v6.0.0 but get the issue that my id's appear in scientific notation |
AFAIK it is not included in 6.0.0. |
@MarcelloPerathoner that is too bad. I hoped it would have been and that someohow I missed something. Do you maybe know a docker image that does not ahve this issue? |
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
todouble
. Thedouble
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: #7069This 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