Fix json Serializer (all Proprties should be used)

[jogibear9988]

See : #41749, #73021
Needs still work for Json Generator and Tests

[ghost]

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

---

See : #41749, #73021
Needs still work for Json Generator and Tests

Author:	jogibear9988
Assignees:	-
Labels:	area-System.Text.Json
Milestone:	-

[jogibear9988]

Anyone a Idea how to access the OptionsAttribute from the GetOrAddTypeGenerationSpec method?

[eiriktsarpalis]

All new public APIs must follow the API review process before being added:

https://github.com/dotnet/runtime/blob/main/docs/project/api-review-process.md

To be honest though, I don't think should be made configurable, we should always serialize properties from base interfaces like we do with classes.

[jogibear9988]

I only thought cause it was mentioned here:
#41749 (comment)

If we remove the Configuration option, we also do not need the API Review process.

[eiriktsarpalis]

Do we really need to use different binding flags for interface types? The reason the bug exists is because currentType.BaseType always returns null for interface types so the for loop will only iterate once and not consider any base interfaces. What I would recommend doing instead is tweaking the loop so that it iterates over currentType.GetInterfaces() in cases where currentType is an interface specifically.

[jogibear9988]

Do you think that is faster? What is if some interfaces define the same properties?

[jogibear9988]

I've not yet tested the change. I need to check how to compile files of the .net runtime repo. I only wanted to know at first if fixing it like this would be okay.

[krwq]

I agree with @eiriktsarpalis here, removing DeclaredOnly might end up showing same properties couple of times.

I'm curious if we could simplify this code or repeat what other serializers do (i.e. what does XmlSerializer do)

[jogibear9988]

I did not simply remove it. I only removed it when the type is an interface

[eiriktsarpalis]

I believe the point is that we needn't use different binding flags when the type is an interface. The logic should follow classes in explicitly walking the type hierarchy and resolve any ambiguity following the same logic.

[eiriktsarpalis]

Do you think that is faster? What is if some interfaces define the same properties?

It's not an issue of performance. The CacheMember method applies its own logic to resolve conflicts and for the sake of consistency the same logic should be followed in interface.

That being said, one thing that needs to be taken into account is diamond ambiguity which is not possible in class hierarchies.

[krwq]

IMO we should see what other serializers are doing and match this behavior

[eiriktsarpalis]

I think this change is less trivial than one might have initially anticipated. Interfaces have concerns that are not directly applicable to class hierarchies, consider the following example:

JsonSerializer.Serialize<IDiamond>(someValue); // what JSON contract should IDiamond have?

public interface IBase
{
    [JsonPropertyOrder(1)]
    int X { get; set; }

    [JsonPropertyOrder(2)]
    int Y { get; set; }
}

public interface IDerived1 : IBase
{
    [JsonPropertyOrder(0)]
    [JsonPropertyName("CustomName1")]
    int IBase.Y
    {
        get => 42;
        set { }
    }
}

public interface IDerived2 : IBase
{
    [JsonIgnore]
    [JsonPropertyName("CustomName2")]
    int IBase.Y
    {
        get => -1;
        set { }
    }
}

public interface IDiamond : IDerived1, IDerived2
{
}

Interfaces support default interface members and admit multiple inheritance, so from the perspective of a serializer computing the JSON contract for IDiamond using attributes in its hierarchy (as already happens to be the case in linear class hierarchies) the diamond problem rears its head.

It is not clear to me what the right solution might be here, and it might be the case that your approach (ignoring any attributes higher up in the hierarchy) is the simple and right thing to do. That being said, we are currently busy wrapping up development for .NET 7, so the margin for error is very small. I would recommend we close this PR and revisit once development for .NET 8 commences. It will give us the opportunity to more holistically address all the virtual member issues that the library has been facing, such as #50078 and #66900.

cc @bartonjs @GrabYourPitchforks who might like this example.

[ramondeklein]

The only proper resolution would be to get rid of JsonPropertyOrderAttribute, because the ordering of fields inside a JSON object shouldn't matter:

"An object is an unordered collection of zero or more name/value pairs, where a name is a string and a value is a string, number, boolean, null, object, or array." (Source RFC 7159)

I think this attribute should be marked as deprecated, because it promotes writing non-compliant applications. But that's another discussion 😄

If you really want to "properly" deal with these attributes, then it would make sense that the derived object takes precedence. But people are already shooting themselves in the foot if they rely on the order of the JSON properties and/or redefine the JSON property name in a derived class.

[jogibear9988]

I don't think handling of interfaces with same property names all with json serializer attributes are a weird special case, wich could not automaticaly be solved. I think the simples approach to read all of the inherited properties in a interface would solve 99% of the cases

[jogibear9988]

I don't like that it was moved to NET8.
It means I need to wait over a year for a proper fix (if it would be solved in 8, it was already moved to 7 from 6 as first target)

[jogibear9988]

The only proper resolution would be to get rid of JsonPropertyOrderAttribute, because the ordering of fields inside a JSON object shouldn't matter:

but also multiple JsonpropertyName attributes are a problem

[eiriktsarpalis]

I don't like that it was moved to NET8.
It means I need to wait over a year for a proper fix (if it would be solved in 8, it was already moved to 7 from 6 as first target)

The issue backlog for System.Text.Json is fairly substantial, so we have to be strategic about what issues we do address in each release. Unfortunately, this means that legitimate bugs often get punted across releases. While I would like to see this fixed for .NET 7, as already mentioned, there are important open design questions to be addressed. Given that we are about lock down development for .NET 7, there is not enough leeway to do this.

I think the simples approach to read all of the inherited properties in a interface would solve 99% of the cases

That's quite possibly true, but any good design needs to account for 100% of the cases (which might include being explicit about which scenaria we don't want to support). Further to your point, I would contend that 99% of use cases define DTOs for their serialization contracts that don't employ interfaces or inheritance -- but that doesn't mean we shouldn't be consistent about the 1% that does use interface hierarchies.

[ghost]

This pull request has been automatically marked no-recent-activity because it has not had any activity for 14 days. It will be closed if no further activity occurs within 14 more days. Any new comment (by anyone, not necessarily the author) will remove no-recent-activity.

[eiriktsarpalis]

I'm going to close this PR, per earlier feedback we need to think closer about semantics w.r.t. multiple inheritance conflict resolution before we fix this. Thank you for your contribution!