Project-OSRM
Fix json rendering of large osm ids
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
- [ ] CHANGELOG.md entry (How to write a changelog entry)
- [ ] update relevant Wiki pages
- [X] add tests (see testing documentation)
- [ ] review
- [ ] adjust for comments
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.
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: 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.
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
@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?
I ran into this issue as well. It seems like there are many nodes now with >10 digits Any chance we'll see the fix in the next release?
