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

JsonSerializer.Serialize produces invalid JSON for [JsonExtensionData] property #97225

Open
KalininAndreyVictorovich opened this issue Jan 19, 2024 · 9 comments

Comments

@KalininAndreyVictorovich

Description

When serializing object containing property marked with [JsonExtensionData] attribute, serializer produces invalid JSON like this one: {"Id":1,{"nested":true}}

Reproduction Steps

Run the following code

var mix = new Mix
{
    Id = 1,
    Extra = new() { ["nested"] = true, }
};

var text = System.Text.Json.JsonSerializer.Serialize(mix);
Console.WriteLine(text);
// output {"Id":1,{"nested":true}}


public class Mix
{
    public int Id { get; set; }

    [System.Text.Json.Serialization.JsonExtensionData]
    public System.Text.Json.Nodes.JsonObject? Extra { get; set; }
}

Expected behavior

Correct JSON like {"Id":1,"nested":true} or at least valid JSON as if there was not [JsonExtensionData] attribute ({"Id":1,"Extra":{"nested":true}} ).

Actual behavior

Invalid JSON {"Id":1,{"nested":true}}

Regression?

Reproducible at least on .Net 6 to .Net 8

Known Workarounds

No response

Configuration

.Net 8
Windows 11
x64

Other information

No response

@ghost ghost added the untriaged New issue has not been triaged by the area owner label Jan 19, 2024
@ghost
Copy link

ghost commented Jan 19, 2024

Tagging subscribers to this area: @dotnet/area-system-text-json, @gregsdennis
See info in area-owners.md if you want to be subscribed.

Issue Details

Description

When serializing object containing property marked with [JsonExtensionData] attribute, serializer produces invalid JSON like this one: {"Id":1,{"nested":true}}

Reproduction Steps

Run the following code

var mix = new Mix
{
    Id = 1,
    Extra = new() { ["nested"] = true, }
};

var text = System.Text.Json.JsonSerializer.Serialize(mix);
Console.WriteLine(text);
// output {"Id":1,{"nested":true}}


public class Mix
{
    public int Id { get; set; }

    [System.Text.Json.Serialization.JsonExtensionData]
    public System.Text.Json.Nodes.JsonObject? Extra { get; set; }
}

Expected behavior

Correct JSON like {"Id":1,"nested":true} or at least valid JSON as if there was not [JsonExtensionData] attribute ({"Id":1,"Extra":{"nested":true}} ).

Actual behavior

Invalid JSON {"Id":1,{"nested":true}}

Regression?

Reproducible at least on .Net 6 to .Net 8

Known Workarounds

No response

Configuration

.Net 8
Windows 11
x64

Other information

No response

Author: KalininAndreyVictorovich
Assignees: -
Labels:

area-System.Text.Json

Milestone: -

@eiriktsarpalis eiriktsarpalis added bug and removed untriaged New issue has not been triaged by the area owner labels Jan 19, 2024
@eiriktsarpalis eiriktsarpalis added this to the Future milestone Jan 19, 2024
@eiriktsarpalis
Copy link
Member

Can confirm that this occurs. It seems we never added testing for the serialization scenario -- deserialization appears to be working as expected.

@elgonzo
Copy link

elgonzo commented Jan 19, 2024

There is a related question as to whether this scenario is supposed to be allowed or not. Because the documentation for JsonExtensionDataAttribute states:

The dictionary's TKey value must be String, and TValue must be JsonElement or Object.

However, the example code in the report uses JsonObject as extension data property - an IDictionary<string, JsonNode>, which according to the JsonExtensionDataAttribute documentation is not an allowed dictionary type.

So, basically the problem is either

  • the documentation is correct, but the (de)serializer fails to respond with an appropriate error/exception when encountering an unsupported extension data property type.
  • IDictionary<string, JsonNode> is supposed to be supported as extension data property type, in which case there is both a bug in the implementation and an error in the JsonExtensionDataAttribute documentation.

@eiriktsarpalis

deserialization appears to be working as expected

Does this then mean the documentation for JsonExtensionDataAttribute is wrong about the types allowed for extension data properties?

@eiriktsarpalis
Copy link
Member

The documentation is correctly stating that JsonObject is one of the supported types. However there is a bug specifically impacting serialization for the particular type (FWIW JsonExtensionData is a feature primarily oriented towards dserialization).

@elgonzo
Copy link

elgonzo commented Jan 19, 2024

The documentation is correctly stating that JsonObject is one of the supported types. [...]

You seem to be mistaken. Where does the documentation for JsonExtensionDataAttribute state this?

The related documentation page "How to handle overflow JSON or use JsonElement or JsonNode in System.Text.Json" also does not support your claim:

To capture extra data such as these properties, apply the [JsonExtensionData] attribute to a property of type Dictionary<string,object> or Dictionary<string,JsonElement>.

@eiriktsarpalis
Copy link
Member

You're right, I misread the documentation which appears to be out of date. The correct statement on supported types can actually be found in the error messages of the implementation itself:

<data name="DataExtensionPropertyInvalidFormat" xml:space="preserve">
<value>The data extension property '{0}.{1}' is invalid. It must implement 'IDictionary&lt;string, JsonElement&gt;' or 'IDictionary&lt;string, object&gt;', or be 'JsonObject'.</value>
</data>

In other words, what I mentioned earlier holds. JsonObject is supported and there is a bug specifically concerning serialization.

@elgonzo
Copy link

elgonzo commented Jan 19, 2024

Okay. Regarding the documentation being out of date, should i file an issue report with the docs repo, or do you handle this internally between teams?

@eiriktsarpalis
Copy link
Member

It would help if you could file a separate issue in dotnet-api-docs. Thanks!

@RutulPatel8
Copy link

Can confirm that this occurs. It seems we never added testing for the serialization scenario -- deserialization appears to be working as expected.

Yes I have tested with Dot net core 6 version.
It is reproducible.
image

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

No branches or pull requests

4 participants