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

Support DateOnly and TimeOnly in JsonSerializer #53539

Closed
Tracked by #63762
layomia opened this issue Jun 1, 2021 · 68 comments · Fixed by #69160
Closed
Tracked by #63762

Support DateOnly and TimeOnly in JsonSerializer #53539

layomia opened this issue Jun 1, 2021 · 68 comments · Fixed by #69160
Assignees
Labels
api-approved API was approved in API review, it can be implemented area-System.Text.Json blocking Marks issues that we want to fast track in order to unblock other important work Cost:S Work that requires one engineer up to 1 week Priority:2 Work that is important, but not critical for the release Team:Libraries User Story A single user-facing feature. Can be grouped under an epic.
Milestone

Comments

@layomia
Copy link
Contributor

layomia commented Jun 1, 2021

#51302 proposes adding DateOnly and TimeOnly support to System.Text.Json: on the serializer, reader, writer, and DOM types. We won't get to all of this in 6.0 but can support them at the JsonSerializer level now, using internal converters and building on ISO 8601 support for DateTime and DateTimeOffset.

API Proposal

We don't need new API to support these types in the existing (non source-gen) serializer programming model. The implementation will be based on new, internal JsonConverter<T>s. Support will be automatic since the serializer initializes converters for all types during its warm-up phase.

// No new API or opt-in needed
JsonSerializer.Deserialize<DateOnly>(json);
JsonSerializer.Serialize(new MyClassWithTimeOnlyProp());

When the JSON source generation feature (#45448) is used, we'll need API like the following so that the generator can generate code that initializes those converters as-needed in a user's project.

namespace System.Text.Json.Serialization.Metadata
{
    public static partial class JsonMetadataServices
    {
        public static JsonConverter<DateOnly> DateOnlyConverter { get; }
        public static JsonConverter<TimeOnly> TimeOnlyConverter { get; }
    }
}

Format

DateOnly and TimeOnly will be (de)serialized using the following formats:

Format Description
DateOnly "yyyy'-'MM'-'dd" ISO 8601: Calendar date
TimeOnly "HH':'mm':'ss[FFFFFFF]" ISO 8601: Time without UTC offset information

See Date and time components for more information.


EDIT @eiriktsarpalis should be reviewed in conjunction with #29932

FYI @devsko, @mattjohnsonpint, @eiriktsarpalis, @steveharter, @tarekgh, @ericstj.

@layomia layomia added this to the 6.0.0 milestone Jun 1, 2021
@dotnet-issue-labeler dotnet-issue-labeler bot added the untriaged New issue has not been triaged by the area owner label Jun 1, 2021
@ghost
Copy link

ghost commented Jun 1, 2021

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

Issue Details

#51302 proposes adding DateOnly and TimeOnly support to System.Text.Json: on the serializer, reader, writer, and DOM types. We'll not get to all of this in 6.0 but can support them at JsonSerializer level now, using internal converters and building on ISO 8601 support.

FYI @devsko, @mattjohnsonpint, @eiriktsarpalis, @steveharter, @tarekgh, @ericstj.

Author: layomia
Assignees: -
Labels:

area-System.Text.Json

Milestone: 6.0.0

@tarekgh
Copy link
Member

tarekgh commented Jun 1, 2021

@layomia can you move the needed part of the proposal here and we can proceed with that?

@layomia
Copy link
Contributor Author

layomia commented Jun 1, 2021

@tarekgh 👍 . I added some notes in the description.

@layomia
Copy link
Contributor Author

layomia commented Jun 1, 2021

I believe that supporting the DateOnly and TimeOnly types at the JsonSerializer level alone will cover most usage scenarios for users that want this support in System.Text.Json for .NET 6.0. I'm proposing that we scope it this way given other STJ work for this release. We could consider more support from #51302 given more user feedback, but for now I think we can address the rest in .NET 7/future.

@ericstj
Copy link
Member

ericstj commented Jun 3, 2021

Sounds good to me. Is this ready for API review? Should it be marked to get it on the backlog?

@eiriktsarpalis
Copy link
Member

We're intentionally only doing the internal converters in this iteration, so no new APIs will be added at this stage.

@eiriktsarpalis
Copy link
Member

This should probably be done in conjunction with #29932 (implementation PR #51492) since they're largely related work.

@ericstj
Copy link
Member

ericstj commented Jun 3, 2021

I thought @layomia's API suggestion was needed to expose the converter to the source generator? Could be wrong.

public static System.Text.Json.Serialization.JsonConverter<bool> BooleanConverter { get { throw null; } }
public static System.Text.Json.Serialization.JsonConverter<byte[]> ByteArrayConverter { get { throw null; } }
public static System.Text.Json.Serialization.JsonConverter<byte> ByteConverter { get { throw null; } }
public static System.Text.Json.Serialization.JsonConverter<char> CharConverter { get { throw null; } }
public static System.Text.Json.Serialization.JsonConverter<System.DateTime> DateTimeConverter { get { throw null; } }
public static System.Text.Json.Serialization.JsonConverter<System.DateTimeOffset> DateTimeOffsetConverter { get { throw null; } }
public static System.Text.Json.Serialization.JsonConverter<decimal> DecimalConverter { get { throw null; } }
public static System.Text.Json.Serialization.JsonConverter<double> DoubleConverter { get { throw null; } }
public static System.Text.Json.Serialization.JsonConverter<System.Guid> GuidConverter { get { throw null; } }
public static System.Text.Json.Serialization.JsonConverter<short> Int16Converter { get { throw null; } }
public static System.Text.Json.Serialization.JsonConverter<int> Int32Converter { get { throw null; } }
public static System.Text.Json.Serialization.JsonConverter<long> Int64Converter { get { throw null; } }
public static System.Text.Json.Serialization.JsonConverter<object> ObjectConverter { get { throw null; } }
[System.CLSCompliantAttribute(false)]
public static System.Text.Json.Serialization.JsonConverter<sbyte> SByteConverter { get { throw null; } }
public static System.Text.Json.Serialization.JsonConverter<float> SingleConverter { get { throw null; } }
public static System.Text.Json.Serialization.JsonConverter<string> StringConverter { get { throw null; } }
[System.CLSCompliantAttribute(false)]
public static System.Text.Json.Serialization.JsonConverter<ushort> UInt16Converter { get { throw null; } }
[System.CLSCompliantAttribute(false)]
public static System.Text.Json.Serialization.JsonConverter<uint> UInt32Converter { get { throw null; } }
[System.CLSCompliantAttribute(false)]
public static System.Text.Json.Serialization.JsonConverter<ulong> UInt64Converter { get { throw null; } }
public static System.Text.Json.Serialization.JsonConverter<System.Uri> UriConverter { get { throw null; } }
public static System.Text.Json.Serialization.JsonConverter<System.Version> VersionConverter { get { throw null; } }

@layomia
Copy link
Contributor Author

layomia commented Jun 7, 2021

@ericstj @eiriktsarpalis yup the source generator would need these APIs. No harm in getting this API approved and getting generator support implemented at the same time as the non-source gen code-paths.

@layomia layomia self-assigned this Jun 7, 2021
@layomia layomia added api-ready-for-review API is ready for review, it is NOT ready for implementation blocking Marks issues that we want to fast track in order to unblock other important work labels Jun 7, 2021
@terrajobst
Copy link
Member

These APIs would be in net6.0 only, correct? IOW, the JSON source generator would gracefully do something sane when targeting netstandard2.0 where these don't exist, correct?

namespace System.Text.Json.Serialization.Metadata
{
    public static partial class JsonMetadataServices
    {
        public static JsonConverter<DateOnly> DateOnlyConverter { get; }
        public static JsonConverter<TimeOnly> TimeOnlyConverter { get; }
    }
}

@KevinCathcart
Copy link
Contributor

Surely, for netstandard 2.0 targeted library it would not be possible to even reference types that need those converters as something to create serializers for. The source generator would never reference a converter for a type not seen in the object hierarchy, as that would defeat being trimer friendly.

I suppose it might be possible for a user created .netstandard2.0 library (that uses the generator for serialization and will ultimately be used in a .net6+ app) to reference another package that is multitargeted which adds some properties of these types for a .net6+ build, but generally those properties would not be serialization critical, since they totally won’t exist on older runtimes. So this normally won’t make a difference. In the rare case it does, the netstandard2.0 user library using source generators could multitarget, and would then have the extra properties in the .net6+ version(s).

That is kind of fundamental though. Any shape change only seen in a multitargeted version of a referenced package will never be included in a .netstandard2.0 single-targeted library’s source generated serializers, since to do otherwise would require reflection, and avoiding that reflection is basically the whole point of the source generator.

It might be nice to have some form of diagnostic warning for the library author if any such omited properties are detected, and thus won’t be included in the generated serializers. However, I’m not sure that is something that is even feasible for an analyzer, as it requires knowledge of the tfm graph, and knowing what is contained in the binaries of the other targeted tfms. In any case that would be a topic for a different issue.

@layomia
Copy link
Contributor Author

layomia commented Jun 8, 2021

These APIs would be in net6.0 only, correct? IOW, the JSON source generator would gracefully do something sane when targeting netstandard2.0 where these don't exist, correct?

STJ builds for .NET Core, Framework, and Standard. The API shape is consistent across the TFMs. It is valid for a project targeting .NET Standard to use the generator so long as there's a reference to STJ.dll. The generator only runs if the [JsonSerializable] attribute is used in the user code. We can assume that if the attribute is present, STJ is also present and we can generate code that references source-gen APIs.

@layomia
Copy link
Contributor Author

layomia commented Jun 9, 2021

It might be nice to have some form of diagnostic warning for the library author if any such omited properties are detected, and thus won’t be included in the generated serializers. However, I’m not sure that is something that is even feasible for an analyzer, as it requires knowledge of the tfm graph, and knowing what is contained in the binaries of the other targeted tfms. In any case that would be a topic for a different issue.

I'm not sure what can be done regarding this specific issue.

Related issues might arise when stale metadata is passed to the serializer to represent a given type. Actionable mitigations for right now are to include a versioning mechanism in the generated code so that the serializer can reject state implementations at runtime. This work is part of #45448. Also we'd encourage users to always (re-)run the generator when building rather than generate once and save the output. This would reduce chances of having stale/mismatched generated code.

In general we do want to provide diagnostics & additional tooling if they can prevent issues, where possible, as the situations arise.

@terrajobst
Copy link
Member

STJ builds for .NET Core, Framework, and Standard. The API shape is consistent across the TFMs. It is valid for a project targeting .NET Standard to use the generator so long as there's a reference to STJ.dll. The generator only runs if the [JsonSerializable] attribute is used in the user code. We can assume that if the attribute is present, STJ is also present and we can generate code that references source-gen APIs.

Sounds good. I just want to make sure we don't accidentally generate code that doesn't work in .NET Standard 2.0 anymore. @KevinCathcart's comment makes sense but we need to keep in mind that people sometimes copy types and make them internal in their project. Not sure it's worth hardening the generator for this, but we know that some people do that.

@KevinCathcart
Copy link
Contributor

If somebody did add an internal copy of System.DateOnly to their downlevel library, what would the options even be?

One (not ideal) option is that the Source Generator tries to use the built-in converter, and fails with a compile error, because it is not present. Option 2 is that the source generator sees the built-in converter does not exist, and generates it own error. Option 3 would be is that in the absence of the built-in converter it generates code treating it like any other user defined type.

Options 2 or 3 are nicer than option one. Both look at a quick glance like they should not be hard to implement. Number 3 would presumably be incompatible with .net6’s serialization by default, but I think the user could register a custom converter for the type to make it compatible. Number 2 would not allow for that possibility, but avoids incompatible serialization if the user provided nothing custom.

One last last possible option would be to emit a compatible version of the converter into the generated code. (Yeah, A decent extra chunk of code, and generally a bad idea, especially for an uncommon edge case. I only mention it for completeness.)

There may be other reasonable options, but none spring to mind.

@niemyjski
Copy link

I'm pretty shocked this didn't make it into 6.x and is slated for 7. This really hurts productivity and prevents people from using these new types.

@jeffhandley
Copy link
Member

Thanks, @niemyjski. We understand the frustration that we didn't get this included in .NET 6. We do have it planned for to land in .NET 7 though, as you noted. It's expected to be included in the Preview 5-6 timeframe.

@eiriktsarpalis
Copy link
Member

I think we should bring this back in for API review. We are in a position to answer most of the questions raised in last year's review. There's now precedent in bifurcating ns2.0/netcoreapp APIs, cf. System.Format.Cbor's handling of System.Half APIs:

namespace System.Formats.Cbor
{
public partial class CborReader
{
public System.Half ReadHalf() { throw null; }
}
public partial class CborWriter
{
public void WriteHalf(System.Half value) { }
}
}

@eiriktsarpalis eiriktsarpalis added api-ready-for-review API is ready for review, it is NOT ready for implementation and removed api-needs-work API needs work before it is approved, it is NOT ready for implementation labels Apr 20, 2022
@eiriktsarpalis eiriktsarpalis self-assigned this Apr 20, 2022
@eiriktsarpalis eiriktsarpalis added the blocking Marks issues that we want to fast track in order to unblock other important work label Apr 20, 2022
@terrajobst
Copy link
Member

terrajobst commented Apr 20, 2022

There's now precedent in bifurcating ns2.0/netcoreapp APIs

I don't think we want to bifurcate, but we're OK with later versions of TFMs having more APIs than previous versions. At least to me, bifurcation would mean that ns2.0 would have APIs that the netcoreapp TFM would not have. That would be violation of versioning rules.

FWIW, I think S.T.J needs to support DateOnly and TimeOnly. In general, I don't think it's acceptable for flag ship tech like this to be held back because we also happen to support .NET Standard 2.0 because people targeting the latest version see this as built-in technology.

@mattjohnsonpint
Copy link
Contributor

@terrajobst - FYI, There's a similar issue with dotnet/maui#1100

@terrajobst
Copy link
Member

terrajobst commented May 5, 2022

Video

  • Looks good as proposed
  • First time we're splitting the reference assembly for System.Text.Json between .NET Standard 2.0 and .NET X, but we have confidence that we have validation to avoid making accidental breaks
namespace System.Text.Json.Serialization.Metadata;

public static partial class JsonMetadataServices
{
    public static JsonConverter<DateOnly> DateOnlyConverter { get; }
    public static JsonConverter<TimeOnly> TimeOnlyConverter { get; }
}

@terrajobst terrajobst added api-approved API was approved in API review, it can be implemented and removed api-ready-for-review API is ready for review, it is NOT ready for implementation labels May 5, 2022
@ghost ghost added the in-pr There is an active PR which will close this issue when it is merged label May 10, 2022
@sreejith-ms
Copy link

I'm getting exceptions System.NotSupportedException : Serialization and deserialization of 'System.DateOnly' instances are not supported from the integration tests. The API is working fine with DateOnlyTimeOnly.AspNet package. But the integration tests are failing while using Date/Time only properties.

httpClient.PostAsJsonAsync()
response.Content.ReadFromJsonAsync<T>()

I'm not passing JsonSerializerOptions

@maxkoshevoi
Copy link
Contributor

maxkoshevoi commented May 13, 2022

@sreejith-ms You can pass JsonSerializerOptions to PostAsJsonAsync and ReadFromJsonAsync. Use this extension method from DateOnlyTimeOnly.AspNet to configure them (or add the converters manually):

https://github.com/maxkoshevoi/DateOnlyTimeOnly.AspNet/blob/d239472270aaac83196a2ce1d701e60e3af670b9/DateOnlyTimeOnly.AspNet/Extensions/JsonOptionsExtensions.cs#L11-L14

@sreejith-ms
Copy link

Yes @maxkoshevoi, I can manually pass JsonSerializerOptions by adding the converters but too many places I need to change. I'm using the extension method with the following HttpClient for the integration tests,

var client = factory.WithWebHostBuilder(builder =>
            {
                builder.ConfigureServices(services =>
                {
                    services
                        .AddMvc()
                        .AddJsonOptions(options => options.UseDateOnlyTimeOnlyStringConverters());
                }).UseTestServer()
                    .UseStartup<Startup>();
            })
            
            .CreateClient(new WebApplicationFactoryClientOptions
            {
                AllowAutoRedirect = false
            });

@ghost ghost removed the in-pr There is an active PR which will close this issue when it is merged label May 13, 2022
@terrajobst
Copy link
Member

@sreejith-ms @maxkoshevoi

The final version of this will not require passing options as the serializer will -- by default -- understand these types.

@sreejith-ms
Copy link

@sreejith-ms @maxkoshevoi

The final version of this will not require passing options as the serializer will -- by default -- understand these types.

@terrajobst The final version - you mean .NET 7 release ?
I'm looking for a workaround to support integration tests without passing JsonSerializerOptions on every httpClient calls.

@eiriktsarpalis
Copy link
Member

The final version - you mean .NET 7 release ?

Correct, this change will be first released in .NET 7.

@zdenek-jelinek
Copy link

During our migration to .NET 6, I have implemented a delegating converter akin to how it's done in #69160 and I have found these edge cases:

  • Times such as "00:000:00" are successfully converted - as this is in already released TimeSpanConverter, I am not sure this can be changed and maybe alignment is more valuable here.
  • Negative zero time "-00:00:00" is successfully converted to TimeOnly since it is converted to TimeSpan.Zero which is valid TimeOnly.

I'm not sure this is intended.

Since .NET 7 Preview 5 is not available yet, I wasn't sure if I should create a new issue for this.

@eiriktsarpalis
Copy link
Member

The first issue is surprising, but it seems to reproduce in both TimeSpan.Parse and Utf8Parser.TryParse methods. Guessing it's by design? cc @dotnet/area-system-runtime

The second issue is a quirk of the current TimeOnlyConverter implementation, which delegates parsing to TimeSpanConverter. We could add logic excluding negative zero specifically, but I'm not necessarily convinced it's worth the added code.

@tarekgh
Copy link
Member

tarekgh commented May 31, 2022

We could add logic excluding negative zero specifically, but I'm not necessarily convinced it's worth the added code.

It is worth coding that. allowing such wrong behavior can end up supporting it forever and will be hard to fix it later. Please try to fix that.

@ghost ghost locked as resolved and limited conversation to collaborators Jun 30, 2022
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
api-approved API was approved in API review, it can be implemented area-System.Text.Json blocking Marks issues that we want to fast track in order to unblock other important work Cost:S Work that requires one engineer up to 1 week Priority:2 Work that is important, but not critical for the release Team:Libraries User Story A single user-facing feature. Can be grouped under an epic.
Projects
None yet
Development

Successfully merging a pull request may close this issue.