dotnet / runtime

.NET is a cross-platform runtime for cloud, mobile, desktop, and IoT apps.
https://docs.microsoft.com/dotnet/core/
MIT License
15.27k stars 4.73k forks source link

Source generated JSON deserialization fails with `JsonExtensionData` property using `init` accessor #107898

Open DL444 opened 1 month ago

DL444 commented 1 month ago

Description

Source generated JSON deserialization fails with JsonExtensionData property using init accessor:

using System.Text.Json.Serialization;
using System.Text.Json;

string json = "{\"Value\":42,\"OverflowValue\":\"42\"}";
Demo demo = JsonSerializer.Deserialize(json, DemoContext.Default.Demo)!;
string roundtrip = JsonSerializer.Serialize(demo, DemoContext.Default.Demo);
Console.WriteLine(string.Equals(json, roundtrip, StringComparison.Ordinal));

public sealed class Demo
{
    public int Value { get; init; }

    [JsonExtensionData]
    public Dictionary<string, JsonElement>? ExtensionData { get; init; }
}

[JsonSourceGenerationOptions(GenerationMode = JsonSourceGenerationMode.Metadata)]
[JsonSerializable(typeof(Demo))]
public sealed partial class DemoContext : JsonSerializerContext { }

Reproduction Steps

  1. Create console application from template with the sample code above
  2. Run application

Expected behavior

Application completes with no significant problems and prints "True".

Actual behavior

Application throws exception:

Unhandled exception. System.InvalidOperationException: The extension data property 'ExtensionData' on type 'Demo' cannot bind with a parameter in the deserialization constructor.
   at System.Text.Json.ThrowHelper.ThrowInvalidOperationException_ExtensionDataCannotBindToCtorParam(String propertyName, JsonPropertyInfo jsonPropertyInfo)
   at System.Text.Json.Serialization.Metadata.JsonTypeInfo.ConfigureConstructorParameters()
   at System.Text.Json.Serialization.Metadata.JsonTypeInfo.Configure()
   at System.Text.Json.Serialization.Metadata.JsonTypeInfo.<EnsureConfigured>g__ConfigureSynchronized|172_0()
   at System.Text.Json.JsonSerializerOptions.GetTypeInfoInternal(Type type, Boolean ensureConfigured, Nullable`1 ensureNotNull, Boolean resolveIfMutable, Boolean fallBackToNearestAncestorType)
   at System.Text.Json.JsonSerializerOptions.GetTypeInfo(Type type)
   at DemoContext.get_Demo() in D:\repos\SourceGenJsonDemo\SourceGenJsonDemo\obj\Debug\net8.0\System.Text.Json.SourceGeneration\System.Text.Json.SourceGeneration.JsonSourceGenerator\DemoContext.Demo.g.cs:line 18
   at Program.<Main>$(String[] args) in D:\repos\SourceGenJsonDemo\SourceGenJsonDemo\Program.cs:line 5

Regression?

Probably not: init property deserialization is not supported by System.Text.Json in previous .NET versions.

Known Workarounds

Change the overflow property to use a public or internal set accessor.

Configuration

.NET info:

.NET SDK:
 Version:           9.0.100-rc.1.24452.12
 Commit:            81a714c6d3
 Workload version:  9.0.100-manifests.67cd1eb6
 MSBuild version:   17.12.0-preview-24422-09+d17ec720d

Runtime Environment:
 OS Name:     Windows
 OS Version:  10.0.22635
 OS Platform: Windows
 RID:         win-x64
 Base Path:   C:\Program Files\dotnet\sdk\9.0.100-rc.1.24452.12\

.NET workloads installed:
Configured to use loose manifests when installing new manifests.
 [aspire]
   Installation Source: VS 17.11.35303.130, VS 17.12.35309.182
   Manifest Version:    8.2.0/8.0.100
   Manifest Path:       C:\Program Files\dotnet\sdk-manifests\8.0.100\microsoft.net.sdk.aspire\8.2.0\WorkloadManifest.json
   Install Type:        FileBased

Host:
  Version:      9.0.0-rc.1.24431.7
  Architecture: x64
  Commit:       static

.NET SDKs installed:
  8.0.304 [C:\Program Files\dotnet\sdk]
  8.0.400 [C:\Program Files\dotnet\sdk]
  8.0.401 [C:\Program Files\dotnet\sdk]
  9.0.100-rc.1.24452.12 [C:\Program Files\dotnet\sdk]

.NET runtimes installed:
  Microsoft.AspNetCore.App 6.0.33 [C:\Program Files\dotnet\shared\Microsoft.AspNetCore.App]
  Microsoft.AspNetCore.App 8.0.8 [C:\Program Files\dotnet\shared\Microsoft.AspNetCore.App]
  Microsoft.AspNetCore.App 9.0.0-rc.1.24452.1 [C:\Program Files\dotnet\shared\Microsoft.AspNetCore.App]
  Microsoft.NETCore.App 6.0.33 [C:\Program Files\dotnet\shared\Microsoft.NETCore.App]
  Microsoft.NETCore.App 8.0.8 [C:\Program Files\dotnet\shared\Microsoft.NETCore.App]
  Microsoft.NETCore.App 9.0.0-rc.1.24431.7 [C:\Program Files\dotnet\shared\Microsoft.NETCore.App]
  Microsoft.WindowsDesktop.App 6.0.33 [C:\Program Files\dotnet\shared\Microsoft.WindowsDesktop.App]
  Microsoft.WindowsDesktop.App 8.0.8 [C:\Program Files\dotnet\shared\Microsoft.WindowsDesktop.App]
  Microsoft.WindowsDesktop.App 9.0.0-rc.1.24452.1 [C:\Program Files\dotnet\shared\Microsoft.WindowsDesktop.App]

Other architectures found:
  x86   [C:\Program Files (x86)\dotnet]
    registered at [HKLM\SOFTWARE\dotnet\Setup\InstalledVersions\x86\InstallLocation]

Environment variables:
  Not set

global.json file:
  Not found

Project configuration:

<Project Sdk="Microsoft.NET.Sdk">
  <PropertyGroup>
    <OutputType>Exe</OutputType>
    <TargetFramework>net8.0</TargetFramework>
    <ImplicitUsings>enable</ImplicitUsings>
    <Nullable>enable</Nullable>
  </PropertyGroup>
  <ItemGroup>
    <PackageReference Include="System.Text.Json" Version="8.0.4" />
  </ItemGroup>
</Project>

Other information

No response

dotnet-policy-service[bot] commented 1 month ago

Tagging subscribers to this area: @dotnet/area-system-text-json, @gregsdennis See info in area-owners.md if you want to be subscribed.

DL444 commented 1 month ago

The problem also reproduces when targeting .NET 9 RC 1 with System.Text.Json 9.0.0-rc.1.24431.7.

eiriktsarpalis commented 1 month ago

Supporting JsonExtensionData init properties would require some rework of the extension data implementation, since it currently assumes that it can populate the property lazily. This can be observed if you run the following code using the reflection-based serializer which ignores init properties:

var result = JsonSerializer.Deserialize<Demo>("{\"Value\":42}");
Console.WriteLine(result.ExtensionData is null); // True

result = JsonSerializer.Deserialize<Demo>("{\"Value\":42,\"OverflowValue\":\"42\"}");
Console.WriteLine(result.ExtensionData is null); // False

This behaviour is not trivial to replicate in the source generator which cannot ignore init semantics. It might be safer if we just decide that this pattern is not supported and emit a better error message. The obvious workaround is making the property settable.

DL444 commented 3 weeks ago

I was just reading on UnsafeAccessorAttribute introduced in .NET 8 and I wonder if it can be used to solve the problem by ignoring the init semantics in a reflection-free way. I cobbled up the following code snippet and it seems to be working fine:

Demo demo = new();
SetDemoExtensionData(demo, []);
Console.WriteLine(demo.ExtensionData is not null); // Prints "True".

static void SetDemoExtensionData(Demo demo, Dictionary<string, JsonElement>? value)
{
    ref Dictionary<string, JsonElement>? reference = ref GetExtensionDataReference(demo);
    reference = value;

    [UnsafeAccessor(UnsafeAccessorKind.Field, Name = "<ExtensionData>k__BackingField")]
    extern static ref Dictionary<string, JsonElement>? GetExtensionDataReference(Demo demo);
}

public sealed class Demo
{
    public int Value { get; init; }

    [JsonExtensionData]
    public Dictionary<string, JsonElement>? ExtensionData { get; init; }
}

I imagine the source generator can generate the SetDemoExtensionData method in the middle and use it to set ExtensionData lazily. Not sure if there are any problems that preclude this solution. Some coming to mind:

eiriktsarpalis commented 3 weeks ago

Unsafe accessors are a useful tool, however System.Text.Json needs to be able to support netstandard2.0 which doesn't have those APIs. We could of course special case newer targets so that they use the feature, however that creates a degree of bifurcation that wouldn't be practical to maintain.