Skip to content
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

Option to parse and serialize JSON integers as int64 #8830

Open
lyuma opened this issue Jan 8, 2024 · 8 comments
Open

Option to parse and serialize JSON integers as int64 #8830

lyuma opened this issue Jan 8, 2024 · 8 comments

Comments

@lyuma
Copy link

lyuma commented Jan 8, 2024

Describe the project you are working on

A conversion tool for glTF files, as part of Unidot-importer

Describe the problem or limitation you are having in your project

Godot's JSON parser converts all integer values to float, which causes difficulty when using integers read from a json document as array indices or dictionary keys.

Having this option will allow for use of JSON objects serialized by java, c++ and other languages with int64 types without data loss.

This problem is described in issue godotengine/godot#24119 - seeing as this is technically a feature proposal, I am opening this proposal.

Describe the feature / enhancement and how it helps to overcome the problem or limitation

The feature is to add an option to JSON to parse and serialize 64-bit integer values (values without an e or . in them) (official json.org java implementation) (json-c library) as integers.

Describe how your proposal will work, with code, pseudo-code, mock-ups, and/or diagrams

A new flag will be added to preserve int64.
When the flag is present in the parser, all JSON numbers without a "." or "e" or "E" in them which fit within the range of INT64_MIN to UINT64_MAX will be parsed into an int64

Side node: I noticed json-c supports uint64 but this is not AFAIK present in the json.org java implementation, and it would be debatably incorrect to coerce a positive number to uint64 then to negative int64, so I would advise against supporting uint64. If needed and we do add a flags enum, this could always be added later as a parser feature.

As for how to expose this argument to scripting without compatibility breakage, I can think of three ways right now:

Option 1: Add yet another optional bool argument to JSON.parse, JSON.parse_string and JSON.stringify
Option 2: Add a flags enum and an additional optional flags argument to JSON.parse, JSON.parse_string and JSON.stringify
Option 3: (for parsing only) add a new stateful function such as enable_int64() which can be called before .parse().

If this enhancement will not be used often, can it be worked around with a few lines of script?

No, without this feature proposal, 64-bit integer data is lost (coerced to double with only 52-bit mantissa) the moment Godot parses or serializes the JSON

Is there a reason why this should be core and not an add-on in the asset library?

JSON parsing is in core.

@Mickeon
Copy link

Mickeon commented Jan 8, 2024

I like the enable_int64(). Perhaps it could return itself like a Tween. It makes it "smooth" and avoids potential parameter bloat if that would ever come to be the case.

Probably should be called something like with_int(). The integer type in Godot is inferred to be 64 bits.

@Calinou
Copy link
Member

Calinou commented Jan 8, 2024

Could int64 (de)serialization be automatically used for numbers that are too large to be represented by 64-bit floating-point? I think this is better behavior than truncating numbers when loading or saving JSON.

@AThousandShips
Copy link
Member

I'd suggest it be enabled by default and can be turned off when loading to enforce more "standard" JSON, to ensure at least a greater degree of parity with other software (the main reason to use JSON, portability, over other formats such as ConfigFile or binary or text serialisation)

And add an error when turned off

@lyuma
Copy link
Author

lyuma commented Jan 8, 2024

If we think it should be enabled by default then it should always be enabled. The reason I asked to make it optional is because enabling this by default will break compatibility with some gdscript code.

enforce more "standard" JSON, to ensure at least a greater degree of parity with other software

Many (I would go as far as to say most) other json parsers also encode integers this way. If we encode an integer as an integer, a json parser which only supports float can read it just fine, with rounding, so there is no conformance issue here.

The godot compatibility issue I hit is due to what I consider a mis-feature in Godot's dictionaries: if I insert 1.0 as a dict key, I cannot look it up with dict[1] because Dictionary does not seem to consider floats and ints equal. We do not know if some people depend on the values always being float.

I'm ok changing this assumption but it will need to be in the release notes

@lyuma
Copy link
Author

lyuma commented Jan 8, 2024

Could int64 (de)serialization be automatically used for numbers that are too large to be represented by 64-bit floating-point?

This would violate the principle of least surprise. Int and float datatypes have subtle incompatibilities which are not obvious at first. For example the dictionary key case I brought up in my previous comment. Or bitwise operators. Or float division.

If integers are sometimes serialized as int and sometimes serialized as float, that will cause difficult to diagnose bugs downstream in software which reads these json files, but only when integers are in the range ±2^53–2^63. This range might be difficult to hit during testing.

@usix79
Copy link

usix79 commented Jan 10, 2024

Another option to consider is to provide a new class with target behavior.

Pros: Less configuration mistakes, ability to make original JSON obsolete in later versions.

Cons: Possible confusion for newbies and need to come up with a good name for the new class.

Something similar was done with WebSocketClient in favor of WebSocketPeer

@Mickeon
Copy link

Mickeon commented Jan 10, 2024

I don't feel like a new class is necessary. The described newer behavior has been desired for a while and JSON as a class is pretty small in scope. Adding even more than a few methods, parameters, properties or whatever else is desired would not bloat it drastically.

@YuriSizov
Copy link
Contributor

YuriSizov commented Jan 10, 2024

Godot's JSON parser converts all integer values to float, which causes difficulty when using integers read from a json document as array indices or dictionary keys.

Godot parser follows JSON specification, and that's correct and expected. JSON doesn't have integers and we shouldn't deviate from the spec. That would be the least surprising option, when you consider all possible permutations of data being packed and truncated as it's sent across multiple platforms which treat JSON with a level of deviation.

If you really want to add further validation to JSON files into the parser, beyond of the scope for the JSON standard, then it would make sense to support some popular JSON schema spec instead of making ad hoc patches.

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

No branches or pull requests

7 participants