System.Text.Json metadata writer does not check for property name conflicts

[eiriktsarpalis]

Originally posted by @ilya-scale in #63747 (comment)

This can be reproduced both with ReferenceHandler.Preserve:

var value = new Poco { Value = "string" };
var options = new JsonSerializerOptions { ReferenceHandler = ReferenceHandler.Preserve };
string json = JsonSerializer.Serialize(value, options);
Console.WriteLine(json); // {"$id":"1","$id":"string"}
JsonSerializer.Deserialize<Poco>(json, options); // Fails with 'The metadata property '$id' must be the first reference preservation property in the JSON object. 

public class Poco
{
    [JsonPropertyName("$id")]
    public string Value { get; set; }
}

and polymorphism

Base value = new Derived { Value = "value" };
string json = JsonSerializer.Serialize(value);
Console.WriteLine(json); // {"$type":"derived","$type":"value"}
Base result = JsonSerializer.Deserialize<Base>(json); // System.Text.Json.JsonException: 'Deserialized object contains a duplicate type discriminator metadata property. 
Console.WriteLine(result is Derived);

[JsonDerivedType(typeof(Base), "base")]
[JsonDerivedType(typeof(Derived), "derived")]
public class Base
{
    [JsonPropertyName("$type")]
    public string Value { get; set; }
}

public class Derived : Base
{
}

We should validation for both cases to ensure invalid JSON is not emitted over the wire.

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

---

Originally posted by @ilya-scale in #63747 (comment)

This can be reproduced both with ReferenceHandler.Preserve:

var value = new Poco { Value = "string" };
var options = new JsonSerializerOptions { ReferenceHandler = ReferenceHandler.Preserve };
string json = JsonSerializer.Serialize(value, options);
Console.WriteLine(json); // {"$id":"1","$id":"string"}
JsonSerializer.Deserialize<Poco>(json, options); // Fails with 'The metadata property '$id' must be the first reference preservation property in the JSON object. 

public class Poco
{
    [JsonPropertyName("$id")]
    public string Value { get; set; }
}

and polymorphism

Base value = new Derived { Value = "value" };
string json = JsonSerializer.Serialize(value);
Console.WriteLine(json); // {"$type":"derived","$type":"value"}
Base result = JsonSerializer.Deserialize<Base>(json); // System.Text.Json.JsonException: 'Deserialized object contains a duplicate type discriminator metadata property. 
Console.WriteLine(result is Derived);

[JsonDerivedType(typeof(Base), "base")]
[JsonDerivedType(typeof(Derived), "derived")]
public class Base
{
    [JsonPropertyName("$type")]
    public string Value { get; set; }
}

public class Derived : Base
{
}

We should validation for both cases to ensure invalid JSON is not emitted over the wire.

Author:	eiriktsarpalis
Assignees:	-
Labels:	area-System.Text.Json, untriaged
Milestone:	-

[ilya-scale]

Would there be a way to have both polymorphic deserialization and a possibility to read the property, i.e. the discriminator from the resulting C# object, or you want to explicitly prohibit it so that it will not be possible?

[eiriktsarpalis]

I think it would make sense to explicitly prohibit.

[ilya-scale]

It is probably not a bad idea though to allow reading the type discriminator unless it will lead to some unforeseen consequences or complicated implementation?

[eiriktsarpalis]

When I created this issue, I had assumed that this would be a metadata validation concern, but this is not the case with dictionary types; we'd need to validate individual key/value pairs at runtime which is not ideal from a performance perspective, and may not even be possible if we have a converter that overrides WriteAsPropertyName.

If not possible to enforce on Dictionary types, does it make sense to enforce it on object types from a consistency perspective?

Moving to Future, we can consider doing this in later releases should there be demand.

[eiriktsarpalis]

Related issue: #1780. That one suggests escaping $ to avoid conflicts, however polymorphism makes it possible to define metadata properties with $ prefixes.