aaubry / YamlDotNet

YamlDotNet is a .NET library for YAML
MIT License
2.57k stars 483 forks source link

Invalid date format used in serialized JSON #891

Closed pm64 closed 4 months ago

pm64 commented 10 months ago

Describe the bug

Imagine the following YAML..

Example date: 2023-10-12

.. is deserialized into an object defined as follows:

    public class ExampleClass
    {
        [YamlMember(Alias = "Example date")]
        public DateOnly ExampleDate { get; set; }
    }

With an instance of the above instantiated as "myExample", we convert to JSON as follows:

var serializer = new SerializerBuilder().JsonCompatible().Build();
var json = serializer.Serialize(myExample);

Result:

{"Example date": "10/12/2023"}

The above result is invalid because the date is formatted incorrectly.

The resultant JSON should pass validation against the following JSON schema:

{
  "$schema": "http://json-schema.org/draft-07/schema#",
  "type": "object",
  "properties": {
    "Example date": {
      "type": "string",
      "format": "date"
    }
  },
  "required": ["Example date"]
}

As such, the following result, which passes validation, is expected:

{"Example date":"2023-10-12"}

To Reproduce To reproduce, simply follow the above steps and observe the output's validation result.

I used the following site to validate the JSON in this example:

https://www.jsonschemavalidator.net/

EdwardCooke commented 10 months ago

The reason the current format was used is to match the current DateTimeFormatter output of yyyy/MM/dd hh:mm:ss Which in theory would also fail the date time format of that json spec. However, from what I'm seeing, the JSON spec itself doesn't specify what the format of a date/time is supposed to be. It's up to the json parsers as to what to support. I'm a little hesitant to change what the current format is of the date/time because of how many people are using this library and consistency between the different date/time objects is a good thing. That being said, there is a strong case to be made for changing the format for JsonCompatible methods to match what most parsers are expecting for dates and times.

What you could do in the meantime is specify the patterns to use if you want to do something other than what we have currently. You would want to do something similar to this for the DateTimeConverter and potentially the TimeOnlyConverter if needed.

For example:

using YamlDotNet.Serialization;
using YamlDotNet.Serialization.Converters;

var serializer = new SerializerBuilder()
    .JsonCompatible()
    .WithoutTypeConverter<DateOnlyConverter>()
    .WithTypeConverter(new DateOnlyConverter(formats: new[] { "yyyy-MM-dd" }, doubleQuotes: true))
    .Build();

var o = new O();
Console.WriteLine(serializer.Serialize(o));

class O
{
    public DateOnly MyDate { get; set; } = new DateOnly(2024, 1, 16);
}

Produces:

{"MyDate": "2024-01-16"}
pm64 commented 10 months ago

@EdwardCooke, thank you for this guidance, it is greatly appreciated.

I agree that changing the format could cause disruption, and would definitely save this for a major version bump.

But if you do reconsider the date format for JsonCompatible methods at that time, I think the JSON Schema spec provides a solid framework.

For reference, here's how the spec would have you format CLR types DateTime, TimeOnly, DateOnly, and TimeSpan (from https://json-schema.org/understanding-json-schema/reference/string):

Dates and times are represented in RFC 3339, section 5.6. This is a subset of the date format also commonly known as ISO8601 format.

DateTime: 2018-11-13T20:20:39+00:00 TimeOnly: 20:20:39+00:00 DateOnly: 2018-11-13 TimeSpan: A duration as defined by the ISO 8601 ABNF for "duration". For example, P3D expresses a duration of 3 days.

I think this would minimize the number of developers needing to override the default formats.

EdwardCooke commented 4 months ago

If I get time before the next release I’ll add this in as default. 8601 is widely accepted standard so this does make sense. I’ll want to do a couple of things. It might not be next release (next week).