Skip to content

Conversation

rohkhann
Copy link
Contributor

@rohkhann rohkhann commented May 23, 2024

Why make this change?

When we moved to the new form of query execution: #2068, we got rid of the resolver middleware that had the function:
public static bool RepresentsNullValue(JsonElement element) { return (string.IsNullOrEmpty(element.ToString()) && element.GetRawText() == "null"); }

which would check if the returned string was null. This was useful for datetime values in datawarehouse where because of the use of string agg we have to return null as a string.

The error that is seen is: "The string 'null' was not recognized as a valid DateTime. There is an unknown word starting at index '0'.

What is this change?

This change switches to leveraging tryparse for the datetime field value. This allows us to handle the null case, as if the string is null, tryparse will just fail and return null for date.

How was this tested?

A test has been added to ensure that datetime null case is handled.
Before:
Running a sample query which checks for a date column:
query { tests { items { ReleaseDate } } }
image

After fix:

image

@abhishekkumams
Copy link
Contributor

is this issue only happening for DWSQL or also for other DBs?

@ayush3797
Copy link
Contributor

Please add before/after bug fix screenshots. That'll help.

@seantleonard
Copy link
Contributor

Please add before/after bug fix screenshots. That'll help.

sample requests in text format preferred so that way we can copy/paste into our own GraphQL requests

Co-authored-by: Ayush Agarwal <34566234+ayush3797@users.noreply.github.com>
@rohkhann
Copy link
Contributor Author

/azp run

@rohkhann
Copy link
Contributor Author

/azp run

@rohkhann
Copy link
Contributor Author

/azp run

@rohkhann
Copy link
Contributor Author

/azp run

@rohkhann rohkhann merged commit 343929f into main May 24, 2024
@rohkhann rohkhann deleted the rohkhann/FixingDateTimeNullParsingDW branch May 24, 2024 18:19
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

Successfully merging this pull request may close these issues.

6 participants