ChilliCream / graphql-platform

Welcome to the home of the Hot Chocolate GraphQL server for .NET, the Strawberry Shake GraphQL client for .NET and Banana Cake Pop the awesome Monaco based GraphQL IDE.
https://chillicream.com
MIT License
5.27k stars 748 forks source link

HC14 Strict RFC 3339 DateTime parsing should be opt-in/out - able? #7402

Open randyridge opened 3 months ago

randyridge commented 3 months ago

Product

Hot Chocolate

Is your feature request related to a problem?

In updating from HC 13 -> 14 we encountered a runtime behavior change, with respect to dates, I see that this is documented explicitly in the migration notes and I agree that having a common datetime spec is beneficial and rfc 3339 is a reasonable choice. I also realize that 13->14 is a major semver and could be breaking. All that said, I have clients sending me dates that are not strictly compatible, and while I agree they're not doing the correct thing and can and should be fixed, it's unfortunate this new spec enforcement is coupled directly with upgrading to 14.

My proposal would be to allow the ability to opt-in to the legacy datetime parser similar to how I can do the same for the legacy node ID serializer. It gives me time to migrate clients while the server can go from 13 to 14. For dates though, I have published clients, who from their point of view are sending the same query to the same schema and are now broken just for me having updated the server from 13 to 14. I need some way to decouple the 13 -> 14 upgrade from the strict enforcement of the datetime spec the same way I can decouple the upgrade from the new ID serializer.

I'm left with few options, either the client has to change, our clients go through app approval processes on various platforms, which again I can't always force (some platforms won't force an update even if one is available), especially at an exact time (like when the hc14 server starts taking traffic), or I have to upgrade and deploy elsewhere so new clients that are sending rfc compatible dates can hit an hc 14 instance (without a schema change) and those that haven't updated can still point to the hc13 version (with the same schema) and run dual processes for some period of time.

Specifically the error was: DateTime cannot parse the given literal of type StringValueNode

For this date (missing seconds) "2024-08-23T00:00-04:00"

The solution you'd like

allow the ability to opt-in to the legacy datetime parser

randyridge commented 3 months ago

I was able to work around this by modifying your private static scalar lookups using just the right amount of unholy reflection and replacing your datetime scalar with a child of your datetime scalar that doesn't call the DateTimeScalarRegex. This is a less-than-ideal solution.

michaelstaib commented 3 months ago

Why dont you use BindRuntimeType?

magahl commented 2 months ago

Is there a way to replace the DateTime scalar type that HotChcoalte register? BindRuntimeType adds a new scalar type. That cannot have the same name. This makes client generation harder down the line.

glen-84 commented 2 months ago
builder.Services
    .AddGraphQLServer()
    .BindRuntimeType<DateTime, CustomDateTimeType>()
    .AddTypes();

CustomDateTimeType is a copy of DateTimeType, with the format check removed.

magahl commented 2 months ago

Yeah but that would end up with me having two types of scalars for date time?

image

Thats what i meant with problems generating clients down the line. Since they cant both have the name DateTime. It will throw an exception on startup. Key already exist. Thats why i wanted to remove the old one.

glen-84 commented 2 months ago

I don't see that.

image