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

First-class support for enforcing full RFC 3339 date-time (mandatory timezone) #1631

Closed
slawomir-brzezinski-at-interxion opened this issue Mar 2, 2018 · 7 comments

Comments

@slawomir-brzezinski-at-interxion
Copy link

RFC 3339 is the standard date/time format used by most popular JSON protocol tools today:

If you look closely however, the date-time ABNF definition in that standard makes the timezone mandatory.

Despite that, JSON deserialization&validation, as offered by most out-of-the-box platforms using Newtonsoft JSON, such as ASP.NET Core, allow timezoneless (so, invalid) date-time values just fine, without informing the client of the problem.

Although there can be use cases where local time is desired (say, flight arrival time at destination, without having to know the timezone offset at that date, as providing this would require complex political knowledge not essential to solving the problem), in a great number of cases allowing the lack of timezone information is undesired, because it is unclear what the local time is supposed to be (browser clients can be in any timezone).

In fact, from my experience, sending a 'timezoneless' value will most often be a sign of a bug in the client (someone forgetting to use a proper UTC backed value or forgot to apply correct formatting), while the server accepting such a value will be a bug on the server - insufficient validation. Both of them combined will inevitably lead to a hard-to-spot data corruption.
I really feel the library should allow, if not even encourage a design that prevents these common pitfalls, while it currently makes it 'extremely difficult', as one StackOverflow user stated.

I understand that it would be a breaking change to just make it the new default, but I feel this library should at least offer a way to enable it, perhaps as a SerializeDateTimeZoneHandling option.

When adopting this though, there would also need to be a consideration for the other DateTime-backed format, the date, which, in turn, doesn't have the timezone indicator (but strictly speaking should also report invalidity for a value with the time segment). I would think that the easiest-to-use way would be using an attribute on the member, to tell one from another.

Context trivia: I only raised this here after mistakenly assuming it's OpenAPI's problem

@JamesNK
Copy link
Owner

JamesNK commented Mar 2, 2018

@zlamma
Copy link

zlamma commented Mar 2, 2018

Thanks for the quick response. I will give it a try on next work week, where I will have the sources in question available, but my first thought is that the page you linked to says the default actually already includes the K, so would that mean the default should already reject non RFC 3339 values?

I don't think our setup (ASP.NET Core) overwrites that default, yet it deserializes 2018-03-02T23:11:30.000 into a DateTime instance (with DateTime.Kind=Unspecified) instead of bouncing the request back with a 400 validation error.

Otherwise I don't see any other Format specifier that can cover RFC 3339. There exist z, zz and zzz for the numerical offset, but even if they didn't allow not specifying them, this would then make the special UTC's Z indicator invalid, while the standard does allow it.

UPDATE: Seems like the fact that K doesn't complain about the missing Timezone info is by design - note how the docs for it say "2009-06-15T13:45:30" will indeed be considered fine, just exactly with 'Kind Unspecified'

@zlamma
Copy link

zlamma commented Mar 6, 2018

@JamesNK: Why don't you just set DateFormatString?

What format value are you suggesting?

Having looked at the options, I am afraid that there is not a format string that can cover this.

This is due to usage of Microsoft's TryParse here and their treatment of K I highlighted above.

A simple snipped to try the options:

            var isParseSuccessful = DateTime.TryParseExact(
                    "2018-03-02T23:11:30.000",
                    format: "yyyy'-'MM'-'dd'T'HH':'mm':'ss.FFFFFFFK",
                    provider: CultureInfo.InvariantCulture,
                    style: DateTimeStyles.RoundtripKind,
                    result:
                    out var result
                );
            isParseSuccessful.Should().BeFalse();

@JamesNK
Copy link
Owner

JamesNK commented Mar 11, 2018

@zlamma
Copy link

zlamma commented Mar 11, 2018

What about https://www.newtonsoft.com/json/help/html/T_Newtonsoft_Json_DateTimeZoneHandling.htm

I can't see any option there either. What is needed for RFC 3339 is a setting that will consider it to be an error, when the value doesn't have a timezone designator.

What is available is:

  • RoundtripKind (the default) - timezone is optional, and represented as Kind=Unspecified. If it is given, the DateTime will be converted to be Kind=Utc
  • Unspecified - timezone is optional, and represented as Kind=Unspecified. If it is given, the DateTime will be converted to be Kind=Local
  • Utc - timezone is optional, and when not specified is simply assumed to be UTC (So Kind=Utc)
  • Local - timezone is optional, and when not specified is simply assumed to be Local (So Kind=Local)

@JamesNK
Copy link
Owner

JamesNK commented Mar 12, 2018

If you really desire an error rather than automatically converting then I suggest you write a date time converter that will error when the given DateTime isn't valid or the incoming date string is isn't valid.

If there is more demand in the future I'll consider adding it as a core feature but for now Json.NET serializes and deserializes as ISO 8601, not the RFC.

@JamesNK JamesNK closed this as completed Mar 12, 2018
@zlamma
Copy link

zlamma commented Mar 19, 2018

Thanks for considering.

Just one question for the general public that are trying to address the problem (e.g. the NSwag guys here -
which, BTW, seem to be finding the converter approach still leaving certain use cases problematic).

So, in order to allow addressing this easily in any frameworks based on the JSON SChema or OpenAPI, would you consider a pull request to addresses any of this?

(I'm not sure I will find time myself, but it'd be good to know that the doors are open for a comprehensive fix)

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

No branches or pull requests

3 participants