Skip to content

Add tests for customizing source-gen contracts - #76531

Closed
layomia wants to merge 1 commit into
dotnet:mainfrom
layomia:customizing-source-gen-contracts
Closed

Add tests for customizing source-gen contracts#76531
layomia wants to merge 1 commit into
dotnet:mainfrom
layomia:customizing-source-gen-contracts

Conversation

@layomia

Copy link
Copy Markdown
Contributor

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

@ghost

ghost commented Oct 3, 2022

Copy link
Copy Markdown

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
layomiaforce-pushed the customizing-source-gen-contracts branch from 3832bc5 to a66a069CompareOctober 3, 2022 12:27
_name = value;

// This setter should only be called by end users.
// Disable fast-path if a user modifies a property name.

Copy link
Copy Markdown
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?

@layomialayomiaOct 3, 2022

Copy link
Copy Markdown
ContributorAuthor

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(JsonPropertyInfopropertyintypeInfo.Properties){property.Name=property.Name.ToUpperInvariant();}stringjson=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);Personperson=JsonSerializer.Deserialize(json,typeInfo);Assert.Equal("Jane",person.FirstName);Assert.Equal("Doe",person.LastName);

Copy link
Copy Markdown
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
Copy Markdown
ContributorAuthor

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.

@layomialayomia closed this Oct 6, 2022
@ghostghost locked as resolved and limited conversation to collaborators Nov 5, 2022
@dotnet-policy-servicedotnet-policy-serviceBot added the in-pr There is an active PR which will close this issue when it is merged label Jan 29, 2025
Sign up for freeto subscribe to this conversation on GitHub. Already have an account? Sign in.

Labels

area-System.Text.Jsonin-prThere is an active PR which will close this issue when it is merged

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants

@layomia@eiriktsarpalis