Add support for parsing DateTime values#3250
Add support for parsing DateTime values#3250ArjunNarendra wants to merge 6 commits intoAzure:mainfrom
Conversation
There was a problem hiding this comment.
Pull request overview
This PR addresses #3094 by ensuring GraphQL DateTime (and DateTimeOffset) argument values are parsed into typed CLR values before being passed to database providers, avoiding PostgreSQL treating them as text parameters in comparisons.
Changes:
- Added explicit handling for
SupportedHotChocolateTypes.DATETIME_TYPEandSupportedHotChocolateTypes.DATETIMEOFFSET_TYPEinExecutionHelper.ExtractValueFromIValueNode. - Introduced local parsing helpers to convert string inputs into
DateTime/DateTimeOffset-typed values using invariant culture + roundtrip parsing.
| SupportedHotChocolateTypes.DATETIME_TYPE => ParseDateTimeValue(value.Value), | ||
| SupportedHotChocolateTypes.DATETIMEOFFSET_TYPE => ParseDateTimeOffsetValue(value.Value), | ||
| SupportedHotChocolateTypes.UUID_TYPE => Guid.TryParse(value.Value!.ToString(), out Guid guidValue) ? guidValue : value.Value, | ||
| _ => value.Value |
There was a problem hiding this comment.
This does not make too much sense to me...if we are dealing with SupportedHotChocolateTypes.DATETIME_TYPE shouldn't we return a DateTime (not a DateTimeOffset)?
| if (DateTimeOffset.TryParse(s, CultureInfo.InvariantCulture, DateTimeStyles.RoundtripKind, out DateTimeOffset parsedDto)) | ||
| { | ||
| return parsedDto.UtcDateTime; | ||
| } | ||
|
|
||
| if (DateTime.TryParse(s, CultureInfo.InvariantCulture, DateTimeStyles.RoundtripKind, out DateTime parsedDt)) | ||
| { | ||
| return parsedDt; |
|
|
||
| if (raw is DateTime dt) | ||
| { | ||
| return new DateTimeOffset(dt); |
There was a problem hiding this comment.
Ok, this makes sense although I am having some difficulty thinking of a concrete example when this would come into play.
| SupportedHotChocolateTypes.FLOAT_TYPE => value is IntValueNode intValueNode ? intValueNode.ToDouble() : ((FloatValueNode)value).ToDouble(), | ||
| SupportedHotChocolateTypes.DECIMAL_TYPE => value is IntValueNode intValueNode ? intValueNode.ToDecimal() : ((FloatValueNode)value).ToDecimal(), | ||
| SupportedHotChocolateTypes.DATETIME_TYPE => ParseDateTimeValue(value.Value), | ||
| SupportedHotChocolateTypes.DATETIMEOFFSET_TYPE => ParseDateTimeOffsetValue(value.Value), |
There was a problem hiding this comment.
Ok, I can add a regression test.
…_TYPE and ensure that timezone is configured to UTC for standardization
…e are anyways returning UtcDateTime
a9a9b1a to
ecd6403
Compare
…s.DATETIMEOFFSET_TYPE
Co-authored-by: Copilot <copilot@github.com>
|
@ArjunNarendra I haven't managed to run the integration test slices against local backends successfully yet. Without that information, from my review we need to make sure these changes don't cause regressions in backends other than PGSQL. My main concern is that the normalization is currently implemented in shared GraphQL datetime extraction rather than a PostgreSQL-specific path, so it changes behavior for every backend that consumes the datetime scalar, including DateTimeOffset cases. I also do not see regression coverage proving SQL Server, MySQL, and Cosmos query generation still behaves correctly after this change, especially for offset-bearing filters. We need to make sure it doesn't break any other functionality. If you've already tested for regression, please disregard this comment. Otherwise, I've forked this repo and branch and started playing with local regression testing with docker here |
Why make this change?
This PR is to address #3094, which is a bug that happens because the default type conversion for C# to Npgsql is to convert a string to a TEXT field, but this doesn't work when trying to compare a DATE type for PostgreSql databases.
What is this change?
I have updated
ExecutionHelper.csinExtractValueFromIValueNodeto explicitly handleSupportedHotChocolateTypes.DATETIME_TYPEandSupportedHotChocolateTypes.DATETIMEOFFSET_TYPE.How was this tested?
Sample Request(s)
With query:
Before:
After: