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

Add tests for customizing source-gen contracts #76531

Closed

Conversation

layomia
Copy link
Contributor

@layomia layomia commented Oct 3, 2022

Attempting to understand/codify behavior expectations when users attempt to modify contracts generated by the source generator.

@ghost
Copy link

ghost commented Oct 3, 2022

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

Attempting to understand/codify behavior expectations when users attempt to modify contracts generated by the source generator.

Author: layomia
Assignees: layomia
Labels:

area-System.Text.Json

Milestone: -

@layomia layomia force-pushed the customizing-source-gen-contracts branch from 3832bc5 to a66a069 Compare October 3, 2022 12:27
@@ -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.
Copy link
Member

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:

CanUseSerializeHandler &= Options.SerializerContext?.CanUseSerializationLogic == true;

Is there any particular case that was missed out and there's a failing test without this check?

Copy link
Contributor Author

@layomia layomia Oct 3, 2022

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);

Copy link
Member

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

@layomia
Copy link
Contributor Author

layomia commented Oct 6, 2022

Closing this PR - the most succinct way to customize src-gen'd contracts IMO is by compisition, where a JsonSerializerContext instance is nested within a custom user's IJsonTypeInfoResolver and probed for metadata which can then be modified. However, there are integral issues with modifying src-gen contracts (#76535). Further testing for src-gen contract modification should come with fixes for these issues.

@layomia layomia closed this Oct 6, 2022
@ghost ghost locked as resolved and limited conversation to collaborators Nov 5, 2022
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants