Avoid name collision for covariant and hidden properties (#63443) #63823

Kloizdena · 2022-01-15T02:04:40Z

ghost · 2022-01-15T02:04:50Z

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

Issue Details

null

Author:	Kloizdena
Assignees:	-
Labels:	`area-System.Text.Json`
Milestone:	-

dnfadmin · 2022-01-15T02:04:53Z

All CLA requirements met.

src/libraries/System.Text.Json/tests/System.Text.Json.SourceGeneration.Tests/TestClasses.cs

krwq · 2022-01-24T12:25:18Z

...ries/System.Text.Json/tests/System.Text.Json.SourceGeneration.Tests/RealWorldContextTests.cs

+            var derived = new NotIgnoredPropertyBase_IgnoredPropertyDerived { Id = "Test" };
+
+            string json = JsonSerializer.Serialize(derived, DefaultContext.NotIgnoredPropertyBase_IgnoredPropertyDerived);
+            JsonTestHelper.AssertJsonEqual("{\"Id\":null}", json);


I'm not sure this should be expected, considering that for JsonIgnoreForHiddenProperties_NotIgnoredBase_NotIgnoredDerived we're not showing both Ids my gut tells me I should not be seeing any Ids here either. We should be consistent for hidden non-ignored properties I think. cc: @layomia @eiriktsarpalis @steveharter

I'm not sure either, what are the semantics in the reflection-based for that scenario?

I tried it out without JsonSerializerContext with the following code:

using System.Text.Json; using System.Text.Json.Serialization; NotIgnoredBase b1 = new NotIgnoredBase(); NotIgnoredBase_NotIgnoredDerived b2 = new NotIgnoredBase_NotIgnoredDerived(); NotIgnoredBase b3 = new NotIgnoredBase_NotIgnoredDerived(); NotIgnoredBase_IgnoredDerived b4 = new NotIgnoredBase_IgnoredDerived(); NotIgnoredBase b5 = new NotIgnoredBase_IgnoredDerived(); b1.Property = "1"; // Property on base b2.Property = "2"; b3.Property = "3"; // Property on base b4.Property = "4"; b5.Property = "5"; // Property on base Console.WriteLine(JsonSerializer.Serialize(b1)); Console.WriteLine(JsonSerializer.Serialize(b2)); Console.WriteLine(JsonSerializer.Serialize(b3)); Console.WriteLine(JsonSerializer.Serialize(b4)); Console.WriteLine(JsonSerializer.Serialize(b5)); Console.WriteLine(); Console.WriteLine(JsonSerializer.Serialize(b1, typeof(NotIgnoredBase))); Console.WriteLine(JsonSerializer.Serialize(b2, typeof(NotIgnoredBase))); Console.WriteLine(JsonSerializer.Serialize(b2, typeof(NotIgnoredBase_NotIgnoredDerived))); Console.WriteLine(JsonSerializer.Serialize(b3, typeof(NotIgnoredBase))); Console.WriteLine(JsonSerializer.Serialize(b4, typeof(NotIgnoredBase))); Console.WriteLine(JsonSerializer.Serialize(b4, typeof(NotIgnoredBase_IgnoredDerived))); Console.WriteLine(JsonSerializer.Serialize(b5, typeof(NotIgnoredBase))); public class NotIgnoredBase { public string Property { get; set; } = nameof(NotIgnoredBase); } public class NotIgnoredBase_NotIgnoredDerived : NotIgnoredBase { public new string Property { get; set; } = nameof(NotIgnoredBase_NotIgnoredDerived); } public class NotIgnoredBase_IgnoredDerived : NotIgnoredBase { [JsonIgnore] public new string Property { get; set; } = nameof(NotIgnoredBase_IgnoredDerived); }

Results:

{"Property":"1"} {"Property":"2"} {"Property":"3"} {"Property":"NotIgnoredBase"} {"Property":"5"} {"Property":"1"} {"Property":"NotIgnoredBase"} {"Property":"2"} {"Property":"3"} {"Property":"NotIgnoredBase"} {"Property":"NotIgnoredBase"} {"Property":"5"}

It turns out the reflection-based serializer also throws an exception if the properties are ignored both on the base and the derived class.

IgnoredBase_IgnoredDerived b4 = new IgnoredBase_IgnoredDerived(); Console.WriteLine(JsonSerializer.Serialize(b4)); public class IgnoredBase { [JsonIgnore] public string Property { get; set; } = nameof(IgnoredBase); } public class IgnoredBase_IgnoredDerived : IgnoredBase { [JsonIgnore] public new string Property { get; set; } = nameof(IgnoredBase_IgnoredDerived); }

Exception:

System.ArgumentException: An item with the same key has already been added. Key: Property at System.ThrowHelper.ThrowAddingDuplicateWithKeyArgumentException[T](T key) at System.Collections.Generic.Dictionary`2.TryInsert(TKey key, TValue value, InsertionBehavior behavior) at System.Collections.Generic.Dictionary`2.Add(TKey key, TValue value) at System.Text.Json.Serialization.Metadata.JsonTypeInfo.CacheMember(JsonPropertyInfo jsonPropertyInfo, JsonPropertyDictionary`1 propertyCache, Dictionary`2& ignoredMembers) in /_/src/libraries/System.Text.Json/src/System/Text/Json/Serialization/Metadata/JsonTypeInfo.cs:line 409 at System.Text.Json.Serialization.Metadata.JsonTypeInfo.CacheMember(Type declaringType, Type memberType, MemberInfo memberInfo, Boolean isVirtual, Nullable`1 typeNumberHandling, Boolean& propertyOrderSpecified, Dictionary`2& ignoredMembers) in /_/src/libraries/System.Text.Json/src/System/Text/Json/Serialization/Metadata/JsonTypeInfo.cs:line 373 at System.Text.Json.Serialization.Metadata.JsonTypeInfo..ctor(Type type, JsonConverter converter, Type runtimeType, JsonSerializerOptions options) in /_/src/libraries/System.Text.Json/src/System/Text/Json/Serialization/Metadata/JsonTypeInfo.cs:line 224 at System.Text.Json.JsonSerializerOptions.<InitializeForReflectionSerializer>g__CreateJsonTypeInfo|112_0(Type type, JsonSerializerOptions options) in /_/src/libraries/System.Text.Json/src/System/Text/Json/Serialization/JsonSerializerOptions.cs:line 596 at System.Text.Json.JsonSerializerOptions.GetClassFromContextOrCreate(Type type) in /_/src/libraries/System.Text.Json/src/System/Text/Json/Serialization/JsonSerializerOptions.cs:line 625 at System.Text.Json.JsonSerializerOptions.GetOrAddClass(Type type) in /_/src/libraries/System.Text.Json/src/System/Text/Json/Serialization/JsonSerializerOptions.cs:line 605 at System.Text.Json.JsonSerializer.GetTypeInfo(JsonSerializerOptions options, Type runtimeType) in /_/src/libraries/System.Text.Json/src/System/Text/Json/Serialization/JsonSerializer.Helpers.cs:line 26 at System.Text.Json.JsonSerializer.Serialize(Object value, Type inputType, JsonSerializerOptions options) in /_/src/libraries/System.Text.Json/src/System/Text/Json/Serialization/JsonSerializer.Write.String.cs:line 63 at Program.<Main>$(String[] args) in C:\src\JsonTest\JsonTest\Program.cs:line 54

This error would be also fixed by the PR:
https://github.com/dotnet/runtime/pull/63823/files#diff-8d841bf86f2a9da83758276bd17f55d002ee5b55f58b72da658df351a7c22bc0L412-R412

Full code to compare the source generator and reflection-based output:

using System; using System.Text.Json; using System.Text.Json.Serialization; { NotIgnoredBase b1 = new NotIgnoredBase(); NotIgnoredBase_NotIgnoredDerived b2 = new NotIgnoredBase_NotIgnoredDerived(); NotIgnoredBase b3 = new NotIgnoredBase_NotIgnoredDerived(); NotIgnoredBase_IgnoredDerived b4 = new NotIgnoredBase_IgnoredDerived(); NotIgnoredBase b5 = new NotIgnoredBase_IgnoredDerived(); b1.Property = "1"; // Property on base b2.Property = "2"; b3.Property = "3"; // Property on base b4.Property = "4"; b5.Property = "5"; // Property on base Console.WriteLine("NotIgnoredBase, Reflection-based:"); Console.WriteLine(JsonSerializer.Serialize(b1, typeof(NotIgnoredBase))); Console.WriteLine(JsonSerializer.Serialize(b2, typeof(NotIgnoredBase))); Console.WriteLine(JsonSerializer.Serialize(b2, typeof(NotIgnoredBase_NotIgnoredDerived))); Console.WriteLine(JsonSerializer.Serialize(b3, typeof(NotIgnoredBase))); Console.WriteLine(JsonSerializer.Serialize(b4, typeof(NotIgnoredBase))); Console.WriteLine(JsonSerializer.Serialize(b4, typeof(NotIgnoredBase_IgnoredDerived))); Console.WriteLine(JsonSerializer.Serialize(b5, typeof(NotIgnoredBase))); Console.WriteLine(); Console.WriteLine("NotIgnoredBase, Source generator-based:"); Console.WriteLine(JsonSerializer.Serialize(b1, JsonContext.Default.NotIgnoredBase)); Console.WriteLine(JsonSerializer.Serialize(b2, JsonContext.Default.NotIgnoredBase)); Console.WriteLine(JsonSerializer.Serialize(b2, JsonContext.Default.NotIgnoredBase_NotIgnoredDerived)); Console.WriteLine(JsonSerializer.Serialize(b3, JsonContext.Default.NotIgnoredBase)); Console.WriteLine(JsonSerializer.Serialize(b4, JsonContext.Default.NotIgnoredBase)); Console.WriteLine(JsonSerializer.Serialize(b4, JsonContext.Default.NotIgnoredBase_IgnoredDerived)); Console.WriteLine(JsonSerializer.Serialize(b5, JsonContext.Default.NotIgnoredBase)); } { IgnoredBase b1 = new IgnoredBase(); IgnoredBase_NotIgnoredDerived b2 = new IgnoredBase_NotIgnoredDerived(); IgnoredBase b3 = new IgnoredBase_NotIgnoredDerived(); IgnoredBase_IgnoredDerived b4 = new IgnoredBase_IgnoredDerived(); IgnoredBase b5 = new IgnoredBase_IgnoredDerived(); b1.Property = "1"; // Property on base b2.Property = "2"; b3.Property = "3"; // Property on base b4.Property = "4"; b5.Property = "5"; // Property on base Console.WriteLine(); Console.WriteLine("IgnoredBase, Reflection-based:"); Console.WriteLine(JsonSerializer.Serialize(b1, typeof(IgnoredBase))); Console.WriteLine(JsonSerializer.Serialize(b2, typeof(IgnoredBase))); Console.WriteLine(JsonSerializer.Serialize(b2, typeof(IgnoredBase_NotIgnoredDerived))); Console.WriteLine(JsonSerializer.Serialize(b3, typeof(IgnoredBase))); Console.WriteLine(JsonSerializer.Serialize(b4, typeof(IgnoredBase))); Console.WriteLine("Serialization error"/*JsonSerializer.Serialize(b4, typeof(IgnoredBase_IgnoredDerived))*/); Console.WriteLine(JsonSerializer.Serialize(b5, typeof(IgnoredBase))); Console.WriteLine(); Console.WriteLine("IgnoredBase, Source generator-based:"); Console.WriteLine(JsonSerializer.Serialize(b1, JsonContext.Default.IgnoredBase)); Console.WriteLine(JsonSerializer.Serialize(b2, JsonContext.Default.IgnoredBase)); Console.WriteLine(JsonSerializer.Serialize(b2, JsonContext.Default.IgnoredBase_NotIgnoredDerived)); Console.WriteLine(JsonSerializer.Serialize(b3, JsonContext.Default.IgnoredBase)); Console.WriteLine(JsonSerializer.Serialize(b4, JsonContext.Default.IgnoredBase)); Console.WriteLine("Source generator error"/*JsonSerializer.Serialize(b4, JsonContext.Default.IgnoredBase_IgnoredDerived)*/); Console.WriteLine(JsonSerializer.Serialize(b5, JsonContext.Default.IgnoredBase)); Console.WriteLine(); } public class NotIgnoredBase { public string Property { get; set; } = nameof(NotIgnoredBase); } public class NotIgnoredBase_NotIgnoredDerived : NotIgnoredBase { public new string Property { get; set; } = nameof(NotIgnoredBase_NotIgnoredDerived); } public class NotIgnoredBase_IgnoredDerived : NotIgnoredBase { [JsonIgnore] public new string Property { get; set; } = nameof(NotIgnoredBase_IgnoredDerived); } public class IgnoredBase { [JsonIgnore] public string Property { get; set; } = nameof(IgnoredBase); } public class IgnoredBase_NotIgnoredDerived : IgnoredBase { public new string Property { get; set; } = nameof(IgnoredBase_NotIgnoredDerived); } public class IgnoredBase_IgnoredDerived : IgnoredBase { [JsonIgnore] public new string Property { get; set; } = nameof(IgnoredBase_IgnoredDerived); } [JsonSerializable(typeof(NotIgnoredBase))] [JsonSerializable(typeof(NotIgnoredBase_NotIgnoredDerived))] [JsonSerializable(typeof(NotIgnoredBase_IgnoredDerived))] [JsonSerializable(typeof(IgnoredBase))] [JsonSerializable(typeof(IgnoredBase_NotIgnoredDerived))] //[JsonSerializable(typeof(IgnoredBase_IgnoredDerived))] public partial class JsonContext : JsonSerializerContext { }

Output:

NotIgnoredBase, Reflection-based: {"Property":"1"} {"Property":"NotIgnoredBase"} {"Property":"2"} {"Property":"3"} {"Property":"NotIgnoredBase"} {"Property":"NotIgnoredBase"} {"Property":"5"} NotIgnoredBase, Source generator-based: {"Property":"1"} {"Property":"NotIgnoredBase"} {"Property":"2"} {"Property":"3"} {"Property":"NotIgnoredBase"} {"Property":"NotIgnoredBase"} {"Property":"5"} IgnoredBase, Reflection-based: {} {} {"Property":"2"} {} {} Serialization error {} IgnoredBase, Source generator-based: {} {} {"Property":"2"} {} {} Source generator error {}

src/libraries/System.Text.Json/gen/TypeGenerationSpec.cs

src/libraries/System.Text.Json/src/System/Text/Json/Serialization/Metadata/JsonTypeInfo.cs

eiriktsarpalis · 2022-01-24T16:13:17Z

src/libraries/System.Text.Json/gen/JsonSourceGenerator.Parser.cs

@@ -1115,7 +1115,7 @@ private Type GetCompatibleGenericBaseClass(Type type, Type baseType)
                if (propGenSpec.DefaultIgnoreCondition == JsonIgnoreCondition.Always)
                {
                    ignoredMembers ??= new Dictionary<string, PropertyGenerationSpec>();
-                    ignoredMembers.Add(propGenSpec.ClrName, propGenSpec);
+                    ignoredMembers.TryAdd(propGenSpec.ClrName, propGenSpec);


Should really we be ignoring the result of TryAdd if it returns false? Is there a possibility that PropertyGenerationSpec varies significantly between the base and derived properties and the order of traversal can substantially impact how source code is being generated? cc @layomia

If for example we only want to take the spec for the base property into account, we might want to introduce so that only the base member is inserted into the dictionary (using Add instead of TryAdd).

eiriktsarpalis · 2022-01-24T16:35:45Z

...ries/System.Text.Json/tests/System.Text.Json.SourceGeneration.Tests/RealWorldContextTests.cs

@@ -882,5 +882,121 @@ public virtual void NullableStruct()
            Assert.Equal("Jane", person.Value.FirstName);
            Assert.Equal("Doe", person.Value.LastName);
        }
+
+        [Fact]
+        public void JsonIgnoreForCovariantProperties()


I can't tell just by looking at the diff, but we should make sure that these tests are exercised both by the fast-path and metadata-based source generator.

src/libraries/System.Text.Json/tests/System.Text.Json.SourceGeneration.Tests/TestClasses.cs

…ion/Metadata/JsonTypeInfo.cs Co-authored-by: Eirik Tsarpalis <eirik.tsarpalis@gmail.com>

Co-authored-by: Eirik Tsarpalis <eirik.tsarpalis@gmail.com>

krwq · 2022-03-23T11:36:52Z

...ries/System.Text.Json/tests/System.Text.Json.SourceGeneration.Tests/RealWorldContextTests.cs

+            else
+            {
+                string json = JsonSerializer.Serialize(derived, DefaultContext.CovariantBaseNotIgnored_CovariantDerivedIgnored);
+                JsonTestHelper.AssertJsonEqual("{\"Id\":\"CovariantBaseNotIgnored_CovariantDerivedIgnored\"}", json);


so this effectively means that [JsonIgnore] on the derived overriden member is effectively useless? I'd expect either ignore ({}) or error here

krwq · 2022-03-23T11:37:51Z

...ries/System.Text.Json/tests/System.Text.Json.SourceGeneration.Tests/RealWorldContextTests.cs

+            else
+            {
+                string json = JsonSerializer.Serialize(derived, DefaultContext.CovariantBaseNotIgnored_CovariantDerivedGenericIgnoredString);
+                JsonTestHelper.AssertJsonEqual("{\"Id\":\"CovariantBaseNotIgnored_CovariantDerivedGenericIgnored\"}", json);


same as with the one above

krwq · 2022-03-23T11:39:45Z

...ries/System.Text.Json/tests/System.Text.Json.SourceGeneration.Tests/RealWorldContextTests.cs

+            else
+            {
+                string json = JsonSerializer.Serialize(derived, DefaultContext.CovariantBaseIgnored_CovariantDerivedNotIgnored);
+                JsonTestHelper.AssertJsonEqual("{\"Id\":\"CovariantBaseIgnored_CovariantDerivedNotIgnored\"}", json);


here I would always expect this to not be present because base ignores it

krwq · 2022-03-23T11:44:32Z

@Kloizdena @eiriktsarpalis @layomia I think we should write down doc/spec on how JsonIgnore should work and based on the exploration and expectations I'd adjust the implementation here. Current design (at least first couple of cases) work opposite to what my expectations are. My personal opinion is that I'd rather invest time in specing this out and then eventually fix implementation according to that spec even if that means breaking changes.

eiriktsarpalis · 2022-03-23T11:48:16Z

@krwq there's a whole bunch of JsonIgnore consistency issues being filed (see the parent user story in #63918), so yeah, it might make sense to look at this holistically.

ghost · 2022-06-14T09:02:31Z

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.

ghost · 2022-06-28T12:02:35Z

This pull request will now be closed since it had been marked no-recent-activity but received no further activity in the past 14 days. It is still possible to reopen or comment on the pull request, but please note that it will be locked if it remains inactive for another 30 days.

Avoid name collision for covariant and hidden properties (#63443)

3a6e703

ghost added the community-contribution Indicates that the PR has been added by a community member label Jan 15, 2022

dotnet-issue-labeler bot added area-System.Text.Json and removed community-contribution Indicates that the PR has been added by a community member labels Jan 15, 2022

eiriktsarpalis requested a review from krwq January 17, 2022 10:37

eiriktsarpalis assigned krwq Jan 17, 2022

krwq reviewed Jan 24, 2022

View reviewed changes

src/libraries/System.Text.Json/tests/System.Text.Json.SourceGeneration.Tests/TestClasses.cs Outdated Show resolved Hide resolved

krwq reviewed Jan 24, 2022

View reviewed changes

src/libraries/System.Text.Json/tests/System.Text.Json.SourceGeneration.Tests/TestClasses.cs Show resolved Hide resolved

code style fix for TestClasses

4c520e4

krwq reviewed Jan 24, 2022

View reviewed changes

src/libraries/System.Text.Json/tests/System.Text.Json.SourceGeneration.Tests/TestClasses.cs Outdated Show resolved Hide resolved

krwq reviewed Jan 24, 2022

View reviewed changes

src/libraries/System.Text.Json/tests/System.Text.Json.SourceGeneration.Tests/TestClasses.cs Outdated Show resolved Hide resolved

test class reordering

5947879

krwq reviewed Jan 24, 2022

View reviewed changes

eiriktsarpalis requested a review from layomia January 24, 2022 16:06

eiriktsarpalis reviewed Jan 24, 2022

View reviewed changes

src/libraries/System.Text.Json/gen/TypeGenerationSpec.cs Outdated Show resolved Hide resolved

eiriktsarpalis reviewed Jan 24, 2022

View reviewed changes

src/libraries/System.Text.Json/src/System/Text/Json/Serialization/Metadata/JsonTypeInfo.cs Outdated Show resolved Hide resolved

eiriktsarpalis reviewed Jan 24, 2022

View reviewed changes

src/libraries/System.Text.Json/tests/System.Text.Json.SourceGeneration.Tests/TestClasses.cs Outdated Show resolved Hide resolved

Kloizdena and others added 3 commits January 24, 2022 20:43

Update src/libraries/System.Text.Json/src/System/Text/Json/Serializat…

0a2fe14

…ion/Metadata/JsonTypeInfo.cs Co-authored-by: Eirik Tsarpalis <eirik.tsarpalis@gmail.com>

Update src/libraries/System.Text.Json/gen/TypeGenerationSpec.cs

e79d396

Co-authored-by: Eirik Tsarpalis <eirik.tsarpalis@gmail.com>

Additional inheritance tests for JsonIgnore

7d75c92

krwq reviewed Mar 23, 2022

View reviewed changes

krwq added the needs-author-action An issue or pull request that requires more info or actions from the author. label May 31, 2022

krwq mentioned this pull request May 31, 2022

System.Text.Json source generator fails for covariant properties when [JsonIgnore] is specified #63443

Closed

ghost added the no-recent-activity label Jun 14, 2022

ghost closed this Jun 28, 2022

ghost locked as resolved and limited conversation to collaborators Jul 28, 2022

This pull request was closed.

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Avoid name collision for covariant and hidden properties (#63443) #63823

Avoid name collision for covariant and hidden properties (#63443) #63823

Kloizdena commented Jan 15, 2022 •

edited

Loading

ghost commented Jan 15, 2022

dnfadmin commented Jan 15, 2022 •

edited

Loading

krwq Jan 24, 2022

eiriktsarpalis Jan 24, 2022

Kloizdena Jan 24, 2022

Kloizdena Jan 25, 2022

Kloizdena Jan 25, 2022 •

edited

Loading

eiriktsarpalis Jan 24, 2022

eiriktsarpalis Jan 24, 2022

eiriktsarpalis Jan 24, 2022

krwq Mar 23, 2022 •

edited

Loading

krwq Mar 23, 2022

krwq Mar 23, 2022

krwq commented Mar 23, 2022

eiriktsarpalis commented Mar 23, 2022

ghost commented Jun 14, 2022

ghost commented Jun 28, 2022

Avoid name collision for covariant and hidden properties (#63443) #63823

Avoid name collision for covariant and hidden properties (#63443) #63823

Conversation

Kloizdena commented Jan 15, 2022 • edited Loading

ghost commented Jan 15, 2022

dnfadmin commented Jan 15, 2022 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Kloizdena Jan 25, 2022 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

krwq Mar 23, 2022 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

krwq commented Mar 23, 2022

eiriktsarpalis commented Mar 23, 2022

ghost commented Jun 14, 2022

ghost commented Jun 28, 2022

Kloizdena commented Jan 15, 2022 •

edited

Loading

dnfadmin commented Jan 15, 2022 •

edited

Loading

Kloizdena Jan 25, 2022 •

edited

Loading

krwq Mar 23, 2022 •

edited

Loading