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.

Sign up for GitHub

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

Jump to bottom

[JSON source gen 1/3] Implement APIs needed by JSON source generator #51149

Merged
merged 7 commits into from
Apr 15, 2021
Merged

[JSON source gen 1/3] Implement APIs needed by JSON source generator #51149

merged 7 commits into from
Apr 15, 2021

Conversation

layomia
Copy link
Contributor

@layomia layomia commented Apr 12, 2021 •
edited
Loading

Contributes to #45448.

Note to reviewers

This PR primarily implements the JsonMetadataServices APIs needed by the JSON source generator when generating code in user assemblies. It is essentially a no-op to existing serializer functionality, hence, no test changes are added in this PR. Tests for the new APIs will be included in an upcoming PR that adds the source generator itself.

The changes are being made in chunks to avoid having one huge PR that might be way more difficult to review. The target completion date for PRs in this series is the preview 4 snap.

PR review can start from the third commit "Add JsonMetadataServices and APIs needed by source generator". The first two commits simply perform some refactoring:

  • Rename JsonClassInfo to JsonTypeInfo
  • Move existing metadata types to new Metadata sub-folder/sub-namespace

Upcoming PRs to complete the implementation

I expect all 3 PRs to be open and reviewed concurrently, with reviewers focusing on the goal of each PR in isolation (to the degree possible). Thanks!

@layomia layomia added this to the 6.0.0 milestone Apr 12, 2021
@layomia layomia self-assigned this Apr 12, 2021
@layomia layomia requested a review from jozkee as a code owner April 12, 2021 22:11
@ghost
Copy link

ghost commented Apr 12, 2021

Tagging subscribers to this area: @eiriktsarpalis, @layomia
See info in area-owners.md if you want to be subscribed.

Issue Details

Contributes to #45448.

This PR primarily implements the JsonMetadataServices APIs needed by the JSON source generator when generating code in user assemblies. It is essentially a no-op to existing serializer functionality, hence, no test changes are added in this PR. Tests for the new APIs will be included in an upcoming PR that adds the source generator itself.

The changes are being made in chunks to avoid having one huge PR that might be way more difficult to review. The target completion date for PRs in this series is the preview 4 snap.

Note to reviewers

PR review can start from the third commit "Add JsonMetadataServices and APIs needed by source generator". The first two commits simply perform some refactoring:

  • Rename JsonClassInfo to JsonTypeInfo
  • Move existing metadata types to new Metadata sub-folder/sub-namespace

Upcoming PRs to complete the implementation

  • PR that adds and tests source generator
    • will also test the APIs added in this PR
    • will also add and test a selection of overloads described in the next bullet
  • PR that adds and tests new overloads to JsonSerializer and System.Net.Http.Json methods that take the new metadata classes being added in this PR
Author: layomia
Assignees: layomia
Labels:

area-System.Text.Json
Milestone: 6.0.0

@dotnet-issue-labeler
Copy link

Note regarding the new-api-needs-documentation label:

This serves as a reminder for when your PR is modifying a ref *.cs file and adding/modifying public APIs, to please make sure the API implementation in the src *.cs file is documented with triple slash comments, so the PR reviewers can sign off that change.

@layomia layomia requested a review from tdykstra April 13, 2021 01:22
steveharter
steveharter reviewed Apr 13, 2021
View reviewed changes
...ibraries/System.Text.Json/src/System/Text/Json/Serialization/Metadata/JsonPropertyInfoOfT.cs
JsonIgnoreCondition ignoreCondition,
JsonNumberHandling numberHandling,
string propertyName,
JsonEncodedText jsonPropertyName)
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It the idea we can cache common property names together on the caller site, instead of here on the callee side which wouldn't know about the duplicated names?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yes that's the idea. The source generator/caller will keep track of duplicate property names and resolve them within the callback to return the list of JsonPropertyInfos. We still detect collisions when putting the infos in the dictionary for deserialization, and throw IOE for those cases.

However, it will be possible for the same property name to be written multiple times if the caller is only serializing (and not deserializing). The source generator will not create metadata that has this behavior, but the user can construct this themselves unless we decide to validate both sides (essentially populating the deserialization cache even though we don't need to)

eiriktsarpalis
eiriktsarpalis approved these changes Apr 14, 2021
View reviewed changes
Copy link
Member

@eiriktsarpalis eiriktsarpalis left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Other than minor feedback LGTM.

tdykstra
tdykstra reviewed Apr 14, 2021
View reviewed changes
Copy link

@tdykstra tdykstra left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM, just noted a few typos.

src/libraries/System.Text.Json/src/System/Text/Json/Serialization/JsonSerializerContext.cs Outdated Show resolved Hide resolved
src/libraries/System.Text.Json/src/System/Text/Json/Serialization/JsonSerializerContext.cs Outdated Show resolved Hide resolved
...em.Text.Json/src/System/Text/Json/Serialization/Metadata/JsonMetadataServices.Collections.cs Outdated Show resolved Hide resolved
...em.Text.Json/src/System/Text/Json/Serialization/Metadata/JsonMetadataServices.Collections.cs Outdated Show resolved Hide resolved
...braries/System.Text.Json/src/System/Text/Json/Serialization/Metadata/JsonMetadataServices.cs Outdated Show resolved Hide resolved
@layomia layomia merged commit 354ab2a into dotnet:main Apr 15, 2021
@layomia layomia deleted the JsonSourceGen branch April 15, 2021 02:55
This was referenced Apr 19, 2021
Merged
Merged
@layomia layomia mentioned this pull request May 17, 2021
Merged
@ghost ghost locked as resolved and limited conversation to collaborators May 21, 2021
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Reviewers

@safern safern safern left review comments

@CoffeeFlux CoffeeFlux CoffeeFlux left review comments

@eiriktsarpalis eiriktsarpalis eiriktsarpalis approved these changes

@tdykstra tdykstra tdykstra left review comments

@steveharter steveharter steveharter approved these changes

@jozkee jozkee Awaiting requested review from jozkee

Assignees

@layomia layomia

Labels
area-System.Text.Json new-api-needs-documentation
Projects
None yet
Milestone
6.0.0
Development

Successfully merging this pull request may close these issues.
6 participants
@layomia @tdykstra @eiriktsarpalis @CoffeeFlux @steveharter @safern