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

[release/6.0-rc2] Update source-gen APIs according to review #59243

Merged
merged 4 commits into from
Sep 18, 2021

Conversation

github-actions[bot]
Copy link
Contributor

@github-actions github-actions bot commented Sep 17, 2021

Backport of #59042 to release/6.0-rc2

/cc @layomia

Customer Impact

Refactors the APIs that JSON-source-generated output calls to make them more resilient to forward versioning. Taking this change means that it will be easier to add new features to the source generator in future versions of STJ to satisfy our customers, including existing features offered by the reflection-based serializer that we were not able to get src-gen support for in .NET 6.0. Taking this change helps avoid the APIs that we are shipping as-is in 6.0 from becoming instant legacy and dead weight in user applications.

Changes are generally of this form:

namespace System.Text.Json.Serialization.Metadata
{
    public static partial class JsonMetadataServices
    {        
        // Serialization-metadata creator for an object property
-       public static JsonPropertyInfo CreatePropertyInfo<T>(JsonSerializerOptions options, bool isProperty, bool isPublic, bool isVirtual, Type declaringType, JsonTypeInfo propertyTypeInfo, JsonConverter<T>? converter, Func<object, T>? getter, Action<object, T>? setter, JsonIgnoreCondition? ignoreCondition, bool hasJsonInclude, JsonNumberHandling? numberHandling, string propertyName, string? jsonPropertyName);
+       public static JsonPropertyInfo CreatePropertyInfo<T>(JsonSerializerOptions options, JsonPropertyInfoValues<T> propertyInfo);
    }
}

Here we are encapsulating a bunch of method parameters on the CreatePropertyInfo<T> method into their own dedicated JsonPropertyInfoValues<T> class which allows us to simply add a new property on the class to support a new feature, rather than needing a new overload of the CreatePropertyInfo<T>. This is a much cleaner design that helps reduce the likelihood complex deprecations in the future. We are very likely to add new features to JsonSerializer in .NET 7 and beyond. We are not blocked on doing this with the current API shape, but the implementation would be less than ideal.

For instance, if we wanted to implement a new “[JsonRequired]” feature to indicate that a property needs a value when deserialization, we would need to add a new overload to the CreatePropertyInfo(...) method accepting a new bool isRequired parameter. We would likely need a new overload on every release of STJ, leading to a lot of cruft over time. The ideal way supported by the new API shape is simply to add a new bool IsRequired { get; init; } property to the JsonPropertyInfoValues class. This is more maintainable over multiple releases.

This PR also adds support for the src-gen JsonSerializer implementation interop-ing with the System.Text.Json.Nodes types. This was done for completeness to align with approved API for 6.0, and also add src-gen support for these popular types. These changes are not essential to the goal of this PR, but the risk with the associated changes is really low as backed by associated tests.

Testing

The new API shape is fully covered by way of existing tests for current functionality.

Risk

With respect to test/program failures, the risk with this change is low since it is largely cosmetic - the same metadata is being passed to the serializer, just in a cleaner way. The tests in the System.Text.Json.SourceGeneration.*Tests projects exercise the new code thoroughly. Pre-existing non src-gen JsonSerializer functionality is completely unaffected by this change.

This is a binary-compat breaking change between RC1/2 - users would need to recompile their applications to avoid runtime exceptions when using a newer version of the SDK. A breaking change doc would need to be published and advertised.

Documentation for these APIs would need to updated outside of the normal doc channels. However, a new pass at the metadata APIs is already planned, to call out to users that these APIs are only meant to be consumed by the source generator. If this change is accepted, these doc updates would be made together. cc @carlossanlop

Code flow

I looked through our downstream consumers and identified two projects that use the source generator, both for benchmarks:

I confirmed that they are always recompiled when the latest bits of the runtime flow in. This means that they shouldn’t experience any breaking changes. If we proceed, I’ll look out for downstream integration and help fix up any issues if they do arise. cc @pranavkm @sebastienros

@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.

@ghost
Copy link

ghost commented Sep 17, 2021

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

Issue Details

Backport of #59042 to release/6.0-rc2

/cc @layomia

Customer Impact

Testing

Risk

Author: github-actions[bot]
Assignees: -
Labels:

area-System.Text.Json, new-api-needs-documentation

Milestone: -

@layomia layomia self-assigned this Sep 17, 2021
@layomia layomia added this to the 6.0.0 milestone Sep 17, 2021
Copy link
Member

@danmoseley danmoseley left a comment

Choose a reason for hiding this comment

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

appears to match main commit

@danmoseley danmoseley merged commit 52324fe into release/6.0-rc2 Sep 18, 2021
@danmoseley danmoseley deleted the backport/pr-59042-to-release/6.0-rc2 branch September 18, 2021 16:12
@ghost ghost locked as resolved and limited conversation to collaborators Nov 3, 2021
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