-
Notifications
You must be signed in to change notification settings - Fork 4.8k
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
Add tests for customizing source-gen contracts #76531
Conversation
Tagging subscribers to this area: @dotnet/area-system-text-json, @gregsdennis Issue DetailsAttempting to understand/codify behavior expectations when users attempt to modify contracts generated by the source generator.
|
...System.Text.Json/tests/System.Text.Json.SourceGeneration.Tests/JsonSerializerContextTests.cs
Show resolved
Hide resolved
...System.Text.Json/tests/System.Text.Json.SourceGeneration.Tests/JsonSerializerContextTests.cs
Show resolved
Hide resolved
...System.Text.Json/tests/System.Text.Json.SourceGeneration.Tests/JsonSerializerContextTests.cs
Show resolved
Hide resolved
...System.Text.Json/tests/System.Text.Json.SourceGeneration.Tests/JsonSerializerContextTests.cs
Show resolved
Hide resolved
src/libraries/System.Text.Json/src/System/Text/Json/Serialization/Metadata/JsonPropertyInfo.cs
Show resolved
Hide resolved
3832bc5
to
a66a069
Compare
@@ -654,11 +654,25 @@ public string Name | |||
} | |||
|
|||
_name = value; | |||
|
|||
// This setter should only be called by end users. | |||
// Disable fast-path if a user modifies a property name. |
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.
If the user modifies source gen metadata, then they're necessarily using a contract resolver other than source generated JsonSerializerContext
instances. In such cases I would expect that CanUseSerializeHandler
is always flipped back to false:
runtime/src/libraries/System.Text.Json/src/System/Text/Json/Serialization/Metadata/JsonTypeInfo.cs
Line 537 in bce23ab
CanUseSerializeHandler &= Options.SerializerContext?.CanUseSerializationLogic == true; |
Is there any particular case that was missed out and there's a failing test without this check?
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.
That would be this:
JsonTypeInfo<Person> typeInfo = SourceGenContext.Default.Person;
foreach (JsonPropertyInfo property in typeInfo.Properties)
{
property.Name = property.Name.ToUpperInvariant();
}
string json = JsonSerializer.Serialize(person, typeInfo);
// Fail: regular-case is serialized instead (because fast-path is taken; metadata should be used instead since we have it).
JsonTestHelper.AssertJsonEqual(@"{""FIRSTNAME"":""Jane"",""LASTNAME"":""Doe""}", json);
Person person = JsonSerializer.Deserialize(json, typeInfo);
Assert.Equal("Jane", person.FirstName);
Assert.Equal("Doe", person.LastName);
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.
Oh wow. Fundamentally I think the problem here is that static metadata is mutable even though these should be locked for modification. We should try to service this.
cc @krwq
...System.Text.Json/tests/System.Text.Json.SourceGeneration.Tests/JsonSerializerContextTests.cs
Show resolved
Hide resolved
Closing this PR - the most succinct way to customize src-gen'd contracts IMO is by compisition, where a |
Attempting to understand/codify behavior expectations when users attempt to modify contracts generated by the source generator.