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

Shouldn't a JsonIgnore property be included in JsonExtensionData property during deserialzation? #81521

Closed
SilentCC opened this issue Feb 2, 2023 · 4 comments
Labels
area-System.Text.Json question Answer questions and provide assistance, not an issue with source code or documentation.
Milestone

Comments

@SilentCC
Copy link
Contributor

SilentCC commented Feb 2, 2023

According to the definition of JsonExtensionData:

When placed on a property of type IDictionary<TKey,TValue>, any properties that do not have a matching member are added to that dictionary during deserialization and written during serialization.

When deserializing a property with JsonIgnore attribute, shouldn't it be a mismatching member because we ignore the property?

Currently, System.Text.Json doesn't fill the JsonExtensionData property with a JsonIngore property:
This is the unit test function in ExtensionDataTest

public class ClassWithIgnoredData
{
    [JsonExtensionData]
    public Dictionary<string, object> MyOverflow { get; set; }

    [JsonIgnore]
    public int MyInt { get; set; }
}

[Fact]
public async Task IgnoredDataShouldNotBeExtensionData()
{
    ClassWithIgnoredData obj = await Serializer.DeserializeWrapper<ClassWithIgnoredData>(@"{""MyInt"":1}");

    Assert.Equal(0, obj.MyInt);
    Assert.Null(obj.MyOverflow);
}
@ghost ghost added the untriaged New issue has not been triaged by the area owner label Feb 2, 2023
@ghost
Copy link

ghost commented Feb 2, 2023

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

According to the definition of JsonExtensionData:

When placed on a property of type IDictionary<TKey,TValue>, any properties that do not have a matching member are added to that dictionary during deserialization and written during serialization.

When deserializing a property with JsonIgnore attribute, shouldn't it be a mismatching member because we ignore the property?

Currently, System.Text.Json doesn't fill the JsonExtensionData property with a JsonIngore property:
This is the unit test function in ExtensionDataTest

 public class ClassWithIgnoredData
        {
            [JsonExtensionData]
            public Dictionary<string, object> MyOverflow { get; set; }

            [JsonIgnore]
            public int MyInt { get; set; }
        }

        [Fact]
        public async Task IgnoredDataShouldNotBeExtensionData()
        {
            ClassWithIgnoredData obj = await Serializer.DeserializeWrapper<ClassWithIgnoredData>(@"{""MyInt"":1}");

            Assert.Equal(0, obj.MyInt);
            Assert.Null(obj.MyOverflow);
        }
Author: SilentCC
Assignees: -
Labels:

area-System.Text.Json

Milestone: -

@krwq
Copy link
Member

krwq commented Feb 2, 2023

I agree this seems counter intuitive (at least to me). I'd expect whatever is not being used to go to extension data but at the same time:

  • changing that will be a breaking change and will require documenting that in multiple places (although IMO there will be very little people if any at all relying on that)
  • most likely if we change JsonIgnore-d properties to be completely ignored by parser we should do the same for parametrized ctors - ignored properties should not be matched with parameters on ctor if we go this path

@krwq krwq added this to the Future milestone Feb 2, 2023
@ghost ghost removed the untriaged New issue has not been triaged by the area owner label Feb 2, 2023
@krwq krwq added question Answer questions and provide assistance, not an issue with source code or documentation. untriaged New issue has not been triaged by the area owner and removed untriaged New issue has not been triaged by the area owner labels Feb 2, 2023
@gregsdennis
Copy link
Contributor

gregsdennis commented Feb 2, 2023

To voice a contrary opinion, I would expect MyInt not to appear in the extension data as it's been explicitly ignored.

The reasoning for this is that we've indicated in the model that we know the property should be in the data and that it should be ignored if it is.

@krwq
Copy link
Member

krwq commented Feb 2, 2023

Closing as duplicate of #68895

@gregsdennis fair but currently contract model doesn't expose IsIgnored and IsIgnored was supposed to mean the same as not present in the model. I can see translating ignored as properties without getter or setter which would be equivalent to current behavior. The problem starts appearing when we have properties with same name on the derived class but not ignoring the property. Let's continue discussion in the other issue if needed

@krwq krwq closed this as completed Feb 2, 2023
@ghost ghost locked as resolved and limited conversation to collaborators Mar 4, 2023
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
area-System.Text.Json question Answer questions and provide assistance, not an issue with source code or documentation.
Projects
None yet
Development

No branches or pull requests

3 participants