godotengine / godot

Godot Engine – Multi-platform 2D and 3D game engine
https://godotengine.org
MIT License
88.71k stars 20.12k forks source link

Resources.Load<Json> corrupts json data by adding extra symbols to null values #90688

Open amelkor opened 4 months ago

amelkor commented 4 months ago

Tested versions

System information

Godot v4.2.1.stable.mono - macOS 14.3.1 - Vulkan (Forward+) - integrated Apple M3 Max - Apple M3 Max (16 Threads)

Godot v4.2.1.stable.mono - Windows 10.0.19045 - Vulkan (Forward+) - dedicated NVIDIA GeForce RTX 3060 Laptop GPU (NVIDIA; 31.0.15.3129) - 11th Gen Intel(R) Core(TM) i7-11800H @ 2.30GHz (16 Threads)

Issue description

Godot.Json Resource corrupts the original json data by adding extra symbols like <> to null values.

Deserialization of a valid json with null values, which was loaded by Resources.Load<Json>(resPath) results in exception:

System.Text.Json.JsonException: '<' is an invalid start of a value.

The below serializers are unable to deserialize the json:

Reading with `Resources.Load fails:

var json = ResourceLoader.Load<Json>(resPath);
var sample2 = JsonSerializer.Deserialize<SampleModel[]>(json.Data.AsString());

The loaded json becomes corrupted:

[
    {
        "Id": 10,
        "Items": [
            "Item 1",
            "Item 2"
        ],
        "Sounds": [
            "Sound 1",
            "Sound 2"
        ]
    },
    {
        "Id": 11,
        "Items": <null>,
        "Sounds": <null>
    }
]

Reading with C# System.IO ok:

using var fs = File.OpenRead(ProjectSettings.GlobalizePath(resPath));
var sample1 = JsonSerializer.Deserialize<SampleModel[]>(fs);

Loaded json is valid, as the original file:

[
    {
        "Id": 10,
        "Items": [
            "Item 1",
            "Item 2"
        ],
        "Sounds": [
            "Sound 1",
            "Sound 2"
        ]
    },
    {
        "Id": 11,
        "Items": null,
        "Sounds": null
    }
]

Stumbled upon this while trying to embed json resources into the build. Because Json files are now treated as resources, there should be at least a property in the Godot.Json class to access the raw data of the original file without having those unexpected extra modifications from the Engine's side.

Steps to reproduce

  1. Obtain a valid json file with array null values inside
  2. Load the json using Resources.Load<Json>("res://data/sample.json")
  3. Deserialize the loaded json contend at json.Data.AsString() with System.Text.Json.JsonSerializer.Deserialize()
  4. Observe System.Text.Json.JsonException: '<' is an invalid start of a value. thrown

Minimal reproduction project (MRP)

JsonCorrupt.zip

RedMser commented 4 months ago

You must use the JSON stringify API to get a valid JSON string. Data.AsString() just converts the variant (in this case a dictionary) to godot-native string syntax, which is similar but not compatible with JSON.

Also you might want to use FileAccess API to load the file as string directly, since right now you're doing a roundtrip from string -> godot JSON -> string -> C# JSON

amelkor commented 4 months ago

The thing is that the json was produced by the above mentioned serializers in a tool separate from the engine with the default serialization options, don't see how Json.Stringify() helps here.

According to the Json spec null values are serialized as null, not <null> so null is valid for any mainstream consumer.

The workaround I use is indeed using FileAccess directly:

var json = FileAccess.Open("res://data/sample.json", FileAccess.ModeFlags.Read);
var sample = JsonConvert.DeserializeObject<SampleModel[]>(json.GetAsText());

I think the purpose of Godot.Json (leastwise in C#) becomes a bit vague at this point. To me, the naming suggests that this is the right way to work with jsons and one would expect to have access to the original unmodified data by either .RawData or .ToString()

RedMser commented 4 months ago

If you access the JSON.Data dictionary fields directly, you'll see they're parsed as null properly. It's just turning that into a string via Godot's Variant API that turns it to <null> since it was never the intent of that API to be used with JSON compatibility.

The purpose of importing JSON resource files is to retrieve fields directly, without any library interop (GDScript). I noticed a method to retrieve the original JSON string get_parsed_text but according to its docs, it needs a parameter in the parse method set, over which you have no control when using the resource loader.

But yeah either way, this is not a bug. Either it's a proposal to allow configuring that flag I mentioned, or you might just use FileAccess to get the file and handle it yourself.