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

Json source generator should support JsonConverter on non-nullable and nullable types #66547

Closed
meziantou opened this issue Mar 12, 2022 · 6 comments · Fixed by #84208
Closed
Assignees
Labels
area-System.Text.Json bug help wanted [up-for-grabs] Good issue for external contributors source-generator Indicates an issue with a source generator feature
Milestone

Comments

@meziantou
Copy link
Contributor

Description

I have a custom JsonConverter like CustomConverter : JsonConverter<DateTime>. Without the source generator I can use the custom converter for DateTime and DateTime? properties. Using the source generator, the generated code doesn't compile for nullable properties.

I have to create 2 converters CustomConverter : JsonConverter<DateTime> and NullableCustomConverter : JsonConverter<DateTime?>.

Reproduction Steps

<Project Sdk="Microsoft.NET.Sdk">
  <PropertyGroup>
    <OutputType>Exe</OutputType>
    <TargetFramework>net6.0</TargetFramework>
    <ImplicitUsings>enable</ImplicitUsings>
  </PropertyGroup>
</Project>
using System.Text.Json;
using System.Text.Json.Serialization;

Console.WriteLine(JsonSerializer.Serialize(new Sample()));

class Sample
{
    [JsonConverter(typeof(DateTimeOffsetToTimestampJsonConverter))]
    public DateTimeOffset Start { get; set; }

    [JsonConverter(typeof(DateTimeOffsetToTimestampJsonConverter))]
    public DateTimeOffset? End { get; set; } // Without this property, this is fine
}

class DateTimeOffsetToTimestampJsonConverter : JsonConverter<DateTimeOffset>
{
    internal const long TicksPerMicroseconds = 10;

    public override DateTimeOffset Read(ref Utf8JsonReader reader, Type typeToConvert, JsonSerializerOptions options)
    {
        var value = reader.GetInt64();
        return new DateTimeOffset(value * TicksPerMicroseconds, TimeSpan.Zero);
    }

    public override void Write(Utf8JsonWriter writer, DateTimeOffset value, JsonSerializerOptions options)
    {
        writer.WriteNumberValue(value.Ticks / TicksPerMicroseconds);
    }
}

[JsonSourceGenerationOptions]
[JsonSerializable(typeof(Sample))]
internal sealed partial class SourceGenerationContext : JsonSerializerContext
{
}

Expected behavior

I can use the json source generator without compilation errors.

Actual behavior

error CS0029: Cannot implicitly convert type 'DateTimeOffsetToTimestampJsonConverter' to 'System.Text.Json.Serialization.JsonConverter<System.DateTimeOffset?>'

global::System.Text.Json.Serialization.Metadata.JsonPropertyInfoValues<global::System.Nullable<global::System.DateTimeOffset>> info1 = new global::System.Text.Json.Serialization.Metadata.JsonPropertyInfoValues<global::System.Nullable<global::System.DateTimeOffset>>()
{
    IsProperty = true,
    IsPublic = true,
    IsVirtual = false,
    DeclaringType = typeof(global::Sample),
    PropertyTypeInfo = jsonContext.NullableDateTimeOffset,
    Converter = new global::DateTimeOffsetToTimestampJsonConverter(), // 👈 Error on this line
    Getter = static (obj) => ((global::Sample)obj).End,
    Setter = static (obj, value) => ((global::Sample)obj).End = value!,
    IgnoreCondition = null,
    HasJsonInclude = false,
    IsExtensionData = false,
    NumberHandling = default,
    PropertyName = "End",
    JsonPropertyName = null
};

Regression?

No response

Known Workarounds

Use 2 different converters, one for non-nullable values and one for nullable values.

Configuration

.NET SDK (reflecting any global.json):
 Version:   7.0.100-preview.1.22110.4
 Commit:    129d2465c8

Runtime Environment:
 OS Name:     Windows
 OS Version:  10.0.22000
 OS Platform: Windows
 RID:         win10-x64
 Base Path:   C:\Program Files\dotnet\sdk\7.0.100-preview.1.22110.4\

Host (useful for support):
  Version: 7.0.0-preview.1.22076.8
  Commit:  405337939c

.NET SDKs installed:
  3.1.417 [C:\Program Files\dotnet\sdk]
  5.0.406 [C:\Program Files\dotnet\sdk]
  6.0.102 [C:\Program Files\dotnet\sdk]
  6.0.200 [C:\Program Files\dotnet\sdk]
  7.0.100-preview.1.22110.4 [C:\Program Files\dotnet\sdk]

.NET runtimes installed:
  Microsoft.AspNetCore.App 3.1.22 [C:\Program Files\dotnet\shared\Microsoft.AspNetCore.App]
  Microsoft.AspNetCore.App 3.1.23 [C:\Program Files\dotnet\shared\Microsoft.AspNetCore.App]
  Microsoft.AspNetCore.App 5.0.11 [C:\Program Files\dotnet\shared\Microsoft.AspNetCore.App]
  Microsoft.AspNetCore.App 5.0.14 [C:\Program Files\dotnet\shared\Microsoft.AspNetCore.App]
  Microsoft.AspNetCore.App 5.0.15 [C:\Program Files\dotnet\shared\Microsoft.AspNetCore.App]
  Microsoft.AspNetCore.App 6.0.2 [C:\Program Files\dotnet\shared\Microsoft.AspNetCore.App]
  Microsoft.AspNetCore.App 7.0.0-preview.1.22109.13 [C:\Program Files\dotnet\shared\Microsoft.AspNetCore.App]
  Microsoft.NETCore.App 3.1.22 [C:\Program Files\dotnet\shared\Microsoft.NETCore.App]
  Microsoft.NETCore.App 3.1.23 [C:\Program Files\dotnet\shared\Microsoft.NETCore.App]
  Microsoft.NETCore.App 5.0.11 [C:\Program Files\dotnet\shared\Microsoft.NETCore.App]
  Microsoft.NETCore.App 5.0.14 [C:\Program Files\dotnet\shared\Microsoft.NETCore.App]
  Microsoft.NETCore.App 5.0.15 [C:\Program Files\dotnet\shared\Microsoft.NETCore.App]
  Microsoft.NETCore.App 6.0.0 [C:\Program Files\dotnet\shared\Microsoft.NETCore.App]
  Microsoft.NETCore.App 6.0.2 [C:\Program Files\dotnet\shared\Microsoft.NETCore.App]
  Microsoft.NETCore.App 7.0.0-preview.1.22076.8 [C:\Program Files\dotnet\shared\Microsoft.NETCore.App]
  Microsoft.WindowsDesktop.App 3.1.22 [C:\Program Files\dotnet\shared\Microsoft.WindowsDesktop.App]
  Microsoft.WindowsDesktop.App 3.1.23 [C:\Program Files\dotnet\shared\Microsoft.WindowsDesktop.App]
  Microsoft.WindowsDesktop.App 5.0.11 [C:\Program Files\dotnet\shared\Microsoft.WindowsDesktop.App]
  Microsoft.WindowsDesktop.App 5.0.14 [C:\Program Files\dotnet\shared\Microsoft.WindowsDesktop.App]
  Microsoft.WindowsDesktop.App 5.0.15 [C:\Program Files\dotnet\shared\Microsoft.WindowsDesktop.App]
  Microsoft.WindowsDesktop.App 6.0.0 [C:\Program Files\dotnet\shared\Microsoft.WindowsDesktop.App]
  Microsoft.WindowsDesktop.App 6.0.2 [C:\Program Files\dotnet\shared\Microsoft.WindowsDesktop.App]
  Microsoft.WindowsDesktop.App 7.0.0-preview.1.22077.5 [C:\Program Files\dotnet\shared\Microsoft.WindowsDesktop.App]

Other information

No response

@dotnet-issue-labeler dotnet-issue-labeler bot added area-System.Text.Json untriaged New issue has not been triaged by the area owner labels Mar 12, 2022
@ghost
Copy link

ghost commented Mar 12, 2022

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

Issue Details

Description

I have a custom JsonConverter like CustomConverter : JsonConverter<DateTime>. Without the source generator I can use the custom converter for DateTime and DateTime? properties. Using the source generator, the generated code doesn't compile for nullable properties.

I have to create 2 converters CustomConverter : JsonConverter<DateTime> and NullableCustomConverter : JsonConverter<DateTime?>.

Reproduction Steps

<Project Sdk="Microsoft.NET.Sdk">
  <PropertyGroup>
    <OutputType>Exe</OutputType>
    <TargetFramework>net6.0</TargetFramework>
    <ImplicitUsings>enable</ImplicitUsings>
  </PropertyGroup>
</Project>
using System.Text.Json;
using System.Text.Json.Serialization;

Console.WriteLine(JsonSerializer.Serialize(new Sample()));

class Sample
{
    [JsonConverter(typeof(DateTimeOffsetToTimestampJsonConverter))]
    public DateTimeOffset Start { get; set; }

    [JsonConverter(typeof(DateTimeOffsetToTimestampJsonConverter))]
    public DateTimeOffset? End { get; set; } // Without this property, this is fine
}

class DateTimeOffsetToTimestampJsonConverter : JsonConverter<DateTimeOffset>
{
    internal const long TicksPerMicroseconds = 10;

    public override DateTimeOffset Read(ref Utf8JsonReader reader, Type typeToConvert, JsonSerializerOptions options)
    {
        var value = reader.GetInt64();
        return new DateTimeOffset(value * TicksPerMicroseconds, TimeSpan.Zero);
    }

    public override void Write(Utf8JsonWriter writer, DateTimeOffset value, JsonSerializerOptions options)
    {
        writer.WriteNumberValue(value.Ticks / TicksPerMicroseconds);
    }
}

[JsonSourceGenerationOptions]
[JsonSerializable(typeof(Sample))]
internal sealed partial class SourceGenerationContext : JsonSerializerContext
{
}

Expected behavior

I can use the json source generator without compilation errors.

Actual behavior

error CS0029: Cannot implicitly convert type 'DateTimeOffsetToTimestampJsonConverter' to 'System.Text.Json.Serialization.JsonConverter<System.DateTimeOffset?>'

global::System.Text.Json.Serialization.Metadata.JsonPropertyInfoValues<global::System.Nullable<global::System.DateTimeOffset>> info1 = new global::System.Text.Json.Serialization.Metadata.JsonPropertyInfoValues<global::System.Nullable<global::System.DateTimeOffset>>()
{
    IsProperty = true,
    IsPublic = true,
    IsVirtual = false,
    DeclaringType = typeof(global::Sample),
    PropertyTypeInfo = jsonContext.NullableDateTimeOffset,
    Converter = new global::DateTimeOffsetToTimestampJsonConverter(), // 👈 Error on this line
    Getter = static (obj) => ((global::Sample)obj).End,
    Setter = static (obj, value) => ((global::Sample)obj).End = value!,
    IgnoreCondition = null,
    HasJsonInclude = false,
    IsExtensionData = false,
    NumberHandling = default,
    PropertyName = "End",
    JsonPropertyName = null
};

Regression?

No response

Known Workarounds

Use 2 different converters, one for non-nullable values and one for nullable values.

Configuration

.NET SDK (reflecting any global.json):
 Version:   7.0.100-preview.1.22110.4
 Commit:    129d2465c8

Runtime Environment:
 OS Name:     Windows
 OS Version:  10.0.22000
 OS Platform: Windows
 RID:         win10-x64
 Base Path:   C:\Program Files\dotnet\sdk\7.0.100-preview.1.22110.4\

Host (useful for support):
  Version: 7.0.0-preview.1.22076.8
  Commit:  405337939c

.NET SDKs installed:
  3.1.417 [C:\Program Files\dotnet\sdk]
  5.0.406 [C:\Program Files\dotnet\sdk]
  6.0.102 [C:\Program Files\dotnet\sdk]
  6.0.200 [C:\Program Files\dotnet\sdk]
  7.0.100-preview.1.22110.4 [C:\Program Files\dotnet\sdk]

.NET runtimes installed:
  Microsoft.AspNetCore.App 3.1.22 [C:\Program Files\dotnet\shared\Microsoft.AspNetCore.App]
  Microsoft.AspNetCore.App 3.1.23 [C:\Program Files\dotnet\shared\Microsoft.AspNetCore.App]
  Microsoft.AspNetCore.App 5.0.11 [C:\Program Files\dotnet\shared\Microsoft.AspNetCore.App]
  Microsoft.AspNetCore.App 5.0.14 [C:\Program Files\dotnet\shared\Microsoft.AspNetCore.App]
  Microsoft.AspNetCore.App 5.0.15 [C:\Program Files\dotnet\shared\Microsoft.AspNetCore.App]
  Microsoft.AspNetCore.App 6.0.2 [C:\Program Files\dotnet\shared\Microsoft.AspNetCore.App]
  Microsoft.AspNetCore.App 7.0.0-preview.1.22109.13 [C:\Program Files\dotnet\shared\Microsoft.AspNetCore.App]
  Microsoft.NETCore.App 3.1.22 [C:\Program Files\dotnet\shared\Microsoft.NETCore.App]
  Microsoft.NETCore.App 3.1.23 [C:\Program Files\dotnet\shared\Microsoft.NETCore.App]
  Microsoft.NETCore.App 5.0.11 [C:\Program Files\dotnet\shared\Microsoft.NETCore.App]
  Microsoft.NETCore.App 5.0.14 [C:\Program Files\dotnet\shared\Microsoft.NETCore.App]
  Microsoft.NETCore.App 5.0.15 [C:\Program Files\dotnet\shared\Microsoft.NETCore.App]
  Microsoft.NETCore.App 6.0.0 [C:\Program Files\dotnet\shared\Microsoft.NETCore.App]
  Microsoft.NETCore.App 6.0.2 [C:\Program Files\dotnet\shared\Microsoft.NETCore.App]
  Microsoft.NETCore.App 7.0.0-preview.1.22076.8 [C:\Program Files\dotnet\shared\Microsoft.NETCore.App]
  Microsoft.WindowsDesktop.App 3.1.22 [C:\Program Files\dotnet\shared\Microsoft.WindowsDesktop.App]
  Microsoft.WindowsDesktop.App 3.1.23 [C:\Program Files\dotnet\shared\Microsoft.WindowsDesktop.App]
  Microsoft.WindowsDesktop.App 5.0.11 [C:\Program Files\dotnet\shared\Microsoft.WindowsDesktop.App]
  Microsoft.WindowsDesktop.App 5.0.14 [C:\Program Files\dotnet\shared\Microsoft.WindowsDesktop.App]
  Microsoft.WindowsDesktop.App 5.0.15 [C:\Program Files\dotnet\shared\Microsoft.WindowsDesktop.App]
  Microsoft.WindowsDesktop.App 6.0.0 [C:\Program Files\dotnet\shared\Microsoft.WindowsDesktop.App]
  Microsoft.WindowsDesktop.App 6.0.2 [C:\Program Files\dotnet\shared\Microsoft.WindowsDesktop.App]
  Microsoft.WindowsDesktop.App 7.0.0-preview.1.22077.5 [C:\Program Files\dotnet\shared\Microsoft.WindowsDesktop.App]

Other information

No response

Author: meziantou
Assignees: -
Labels:

area-System.Text.Json, untriaged

Milestone: -

@AaronRobinsonMSFT AaronRobinsonMSFT added the source-generator Indicates an issue with a source generator feature label Mar 13, 2022
@layomia layomia removed the untriaged New issue has not been triaged by the area owner label Apr 7, 2022
@layomia layomia self-assigned this Apr 7, 2022
@layomia layomia added this to the 7.0.0 milestone Apr 7, 2022
@krwq krwq modified the milestones: 7.0.0, 8.0.0 Jul 6, 2022
@krwq krwq added the bug label Sep 28, 2022
@layomia
Copy link
Contributor

layomia commented Dec 2, 2022

Up for grabs. I believe the starting point for the fix is this method which determines the construction logic for a converter instance.

Related: #59041.

@layomia layomia added the help wanted [up-for-grabs] Good issue for external contributors label Dec 2, 2022
@smokedlinq
Copy link

Interesting note, you can override the default options (not sure how safe this is but it works, would be nice to have an extensible method for this instead, but I digress) that will let your source generator work for the majority of scenarios.

[JsonSourceGenerationOptions(DefaultIgnoreCondition = JsonIgnoreCondition.WhenWritingNull, PropertyNamingPolicy = JsonKnownNamingPolicy.CamelCase)]
[JsonSerializable(typeof(MyObject))]
public partial class MyJsonSerializerContext : JsonSerializerContext
{
    static MyJsonSerializerContext()
    {
        s_defaultOptions.Converters.Add(new JsonStringEnumConverter());
    }
}

@ghost ghost added the in-pr There is an active PR which will close this issue when it is merged label Apr 1, 2023
@ghost ghost removed the in-pr There is an active PR which will close this issue when it is merged label May 30, 2023
@pedrobsaila
Copy link
Contributor

pedrobsaila commented May 30, 2023

@eiriktsarpalis
Copy link
Member

If anything was fixed, it was probably done accidentally. Before closing we should at least add testing validating that the scenario works.

@ghost ghost added the in-pr There is an active PR which will close this issue when it is merged label May 30, 2023
@eiriktsarpalis
Copy link
Member

@pedrobsaila I don't believe that the issue has been addressed. Even though the changes in #86526 mean that the code now compiles, the example will still fail at run time with a cast exception.

@eiriktsarpalis eiriktsarpalis self-assigned this May 31, 2023
@ghost ghost removed the in-pr There is an active PR which will close this issue when it is merged label May 31, 2023
@ghost ghost locked as resolved and limited conversation to collaborators Jun 30, 2023
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
area-System.Text.Json bug help wanted [up-for-grabs] Good issue for external contributors source-generator Indicates an issue with a source generator feature
Projects
None yet
Development

Successfully merging a pull request may close this issue.

7 participants