Avoid name collision for covariant and hidden properties (#63443) #63823
There are no files selected for viewing
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -882,5 +882,121 @@ public virtual void NullableStruct() | |
| Assert.Equal("Jane", person.Value.FirstName); | ||
| Assert.Equal("Doe", person.Value.LastName); | ||
| } | ||
|
|
||
| [Fact] | ||
| public void JsonIgnoreForCovariantProperties() | ||
|
There was a problem hiding this comment. 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. |
||
| { | ||
| var derived = new CovariantDerived(); | ||
|
|
||
| if (DefaultContext.JsonSourceGenerationMode == JsonSourceGenerationMode.Serialization) | ||
| { | ||
| Assert.Throws<InvalidOperationException>(() => JsonSerializer.Serialize(derived, DefaultContext.CovariantDerived)); | ||
| } | ||
| else | ||
| { | ||
| string json = JsonSerializer.Serialize(derived, DefaultContext.CovariantDerived); | ||
| JsonTestHelper.AssertJsonEqual(@"{}", json); | ||
| derived = JsonSerializer.Deserialize(json, DefaultContext.CovariantDerived); | ||
| Assert.Null(derived.Id); | ||
| } | ||
| } | ||
|
|
||
| [Fact] | ||
| public void JsonIgnoreForCovariantGenericProperties() | ||
| { | ||
| var derived = new CovariantDerivedGeneric<string>(); | ||
| if (DefaultContext.JsonSourceGenerationMode == JsonSourceGenerationMode.Serialization) | ||
| { | ||
| Assert.Throws<InvalidOperationException>(() => JsonSerializer.Serialize(derived, DefaultContext.CovariantDerivedGenericString)); | ||
| } | ||
| else | ||
| { | ||
| string json = JsonSerializer.Serialize(derived, DefaultContext.CovariantDerivedGenericString); | ||
| JsonTestHelper.AssertJsonEqual(@"{}", json); | ||
|
|
||
| derived = JsonSerializer.Deserialize(json, DefaultContext.CovariantDerivedGenericString); | ||
| Assert.Null(derived.Id); | ||
| } | ||
| } | ||
|
|
||
| [Fact] | ||
| public void JsonIgnoreForHiddenProperties_IgnoredBase_NotIgnoredDerived() | ||
| { | ||
| var derived = new IgnoredPropertyBase_NotIgnoredPropertyDerived { Id = "Test" }; | ||
|
|
||
| string json = JsonSerializer.Serialize(derived, DefaultContext.IgnoredPropertyBase_NotIgnoredPropertyDerived); | ||
| JsonTestHelper.AssertJsonEqual("{\"Id\":\"Test\"}", json); | ||
|
|
||
| if (DefaultContext.JsonSourceGenerationMode == JsonSourceGenerationMode.Serialization) | ||
| { | ||
| // Deserialization not supported in fast path serialization only mode | ||
| Assert.Throws<InvalidOperationException>(() => JsonSerializer.Deserialize(json, DefaultContext.IgnoredPropertyBase_NotIgnoredPropertyDerived)); | ||
| } | ||
| else | ||
| { | ||
| derived = JsonSerializer.Deserialize(json, DefaultContext.IgnoredPropertyBase_NotIgnoredPropertyDerived); | ||
| Assert.Equal("Test", derived.Id); | ||
| } | ||
| } | ||
|
|
||
| [Fact] | ||
| public void JsonIgnoreForHiddenProperties_NotIgnoredBase_IgnoredDerived() | ||
| { | ||
| var derived = new NotIgnoredPropertyBase_IgnoredPropertyDerived { Id = "Test" }; | ||
|
|
||
| string json = JsonSerializer.Serialize(derived, DefaultContext.NotIgnoredPropertyBase_IgnoredPropertyDerived); | ||
| JsonTestHelper.AssertJsonEqual("{\"Id\":null}", json); | ||
|
There was a problem hiding this comment. I'm not sure this should be expected, considering that for There was a problem hiding this comment. I'm not sure either, what are the semantics in the reflection-based for that scenario? There was a problem hiding this comment. 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"}There was a problem hiding this comment. 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: This error would be also fixed by the PR: There was a problem hiding this comment. 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
{} |
||
|
|
||
| if (DefaultContext.JsonSourceGenerationMode == JsonSourceGenerationMode.Serialization) | ||
| { | ||
| // Deserialization not supported in fast path serialization only mode | ||
| Assert.Throws<InvalidOperationException>(() => JsonSerializer.Deserialize(json, DefaultContext.NotIgnoredPropertyBase_IgnoredPropertyDerived)); | ||
| } | ||
| else | ||
| { | ||
| derived = JsonSerializer.Deserialize(json, DefaultContext.NotIgnoredPropertyBase_IgnoredPropertyDerived); | ||
| Assert.Null(derived.Id); | ||
| } | ||
| } | ||
|
|
||
| [Fact] | ||
| public void JsonIgnoreForHiddenProperties_IgnoredBase_IgnoredDerived() | ||
| { | ||
| var derived = new IgnoredPropertyBase_IgnoredPropertyDerived { Id = "Test" }; | ||
|
|
||
| string json = JsonSerializer.Serialize(derived, DefaultContext.IgnoredPropertyBase_IgnoredPropertyDerived); | ||
| JsonTestHelper.AssertJsonEqual(@"{}", json); | ||
|
|
||
| if (DefaultContext.JsonSourceGenerationMode == JsonSourceGenerationMode.Serialization) | ||
| { | ||
| // Deserialization not supported in fast path serialization only mode | ||
| Assert.Throws<InvalidOperationException>(() => JsonSerializer.Deserialize(json, DefaultContext.IgnoredPropertyBase_IgnoredPropertyDerived)); | ||
| } | ||
| else | ||
| { | ||
| derived = JsonSerializer.Deserialize(json, DefaultContext.IgnoredPropertyBase_IgnoredPropertyDerived); | ||
| Assert.Null(derived.Id); | ||
| } | ||
| } | ||
|
|
||
| [Fact] | ||
| public void JsonIgnoreForHiddenProperties_NotIgnoredBase_NotIgnoredDerived() | ||
| { | ||
| var derived = new NotIgnoredPropertyBase_NotIgnoredPropertyDerived { Id = "Test" }; | ||
|
|
||
| string json = JsonSerializer.Serialize(derived, DefaultContext.NotIgnoredPropertyBase_NotIgnoredPropertyDerived); | ||
| JsonTestHelper.AssertJsonEqual("{\"Id\":\"Test\"}", json); | ||
|
|
||
| if (DefaultContext.JsonSourceGenerationMode == JsonSourceGenerationMode.Serialization) | ||
| { | ||
| // Deserialization not supported in fast path serialization only mode | ||
| Assert.Throws<InvalidOperationException>(() => JsonSerializer.Deserialize(json, DefaultContext.NotIgnoredPropertyBase_NotIgnoredPropertyDerived)); | ||
| } | ||
| else | ||
| { | ||
| derived = JsonSerializer.Deserialize(json, DefaultContext.NotIgnoredPropertyBase_NotIgnoredPropertyDerived); | ||
| Assert.Equal("Test", derived.Id); | ||
| } | ||
| } | ||
| } | ||
| } | ||
Oops, something went wrong.
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Should really we be ignoring the result of
TryAddif it returns false? Is there a possibility thatPropertyGenerationSpecvaries significantly between the base and derived properties and the order of traversal can substantially impact how source code is being generated? cc @layomiaThere was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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
Addinstead ofTryAdd).