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

DateTime type does not follow its own specification #6887

Closed
cmeeren opened this issue Feb 12, 2024 · 7 comments
Closed

DateTime type does not follow its own specification #6887

cmeeren opened this issue Feb 12, 2024 · 7 comments
Assignees
Labels
Area: Scalars Issue is related to scalars 🌶️ hot chocolate

Comments

@cmeeren
Copy link
Contributor

cmeeren commented Feb 12, 2024

Product

Hot Chocolate

Version

13.8.1

Link to minimal reproduction

See zip below

Steps to reproduce

Repro solution: HotChocolateBugRepro.zip

Relevant code:

public class Query
{
    public DateTimeOffset Date(DateTimeOffset dateTimeOffset) => dateTimeOffset;
}

When using DateTimeOffset, the schema contains this:

"""
The `DateTime` scalar represents an ISO-8601 compliant date time type.
"""
scalar DateTime @specifiedBy(url: "https://www.graphql-scalars.com/date-time")

The spec on that URL gives examples of valid and invalid strings. Currently, HotChocolate accepts half of the invalid patterns, and more. This includes for example a date only (where the time will be taken to be midnight in the server's time zone, which is bad and can easily cause bugs and confusion for API clients - ask me how I know...).

It also accepts formats such as 2010/02/11 and 11.02.2010 (at least on my computer with my regional settings).

HotChocolate needs to be much stricter if it is to actually represent the linked DateTime type.

Note that this may be relevant for other of HotChocolate's built-in scalars, too. (I haven't checked, but I assume it applies to at least DateTime).

What is expected?

Parsing conforms to the linked spec.

What is actually happening?

Parsing is much more lenient than the linked spec and accepts invalid input.

Relevant log output

No response

Additional context

No response

@cmeeren
Copy link
Contributor Author

cmeeren commented May 15, 2024

This is mistakenly labelled Area: F#. The repro code is C#.

@cmeeren
Copy link
Contributor Author

cmeeren commented Aug 23, 2024

This is improved in 14.0.0-rc.0, but there is still not perfect overlap with the valid/invalid patterns in https://www.graphql-scalars.com/date-time.

@glen-84
Copy link
Collaborator

glen-84 commented Aug 24, 2024

@cmeeren Could you please provide some examples? We do have tests for the valid and invalid date/times.

@cmeeren
Copy link
Contributor Author

cmeeren commented Aug 24, 2024

It's about the requirement of exactly three digits millisecond part. The value 2011-08-30T13:22:53.108912Z is listed as invalid on the spec page, but passes the repro solution (when updated to rc.0).

@cmeeren
Copy link
Contributor Author

cmeeren commented Aug 24, 2024

Additionally, the spec page lists 2011-08-30T24:22:53Z as an invalid value because "Fractions of a second are missing", but that value is primarily invalid (at least in HC) because the hour is 24. If you try a valid date-time without exactly three fractional seconds, such as 2011-08-30T13:22:53.10Z or 2011-08-30T13:22:53Z, they are also considered valid in rc.0, but should not be valid according to the spec.

(I have raised an issue about that on the spec repo: andimarek/graphql-scalars.com#13)

@glen-84
Copy link
Collaborator

glen-84 commented Aug 24, 2024

This is intentional:

We felt that requiring a fixed 3 fractional digits was unnecessarily arbitrary, and inconsistent with RFC 3339. We made fractional seconds optional (as they are in RFC 3339), but decided to limit them to 7, as most languages support 7 or fewer digits of precision.

An issue has been raised before regarding the number and requirement for fractional seconds.

It's not ideal to have this "specified by" URL pointing to a specification that isn't a 100% match, but Michael did mention that he may discuss this with Andi.

Regardless, I'll add a note to the documentation about this difference next week.

@cmeeren
Copy link
Contributor Author

cmeeren commented Aug 24, 2024

Thanks for the clarification! Feel free to close this issue.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Area: Scalars Issue is related to scalars 🌶️ hot chocolate
Projects
None yet
Development

No branches or pull requests

4 participants