Skip to content

Conversation

seantleonard
Copy link
Contributor

@seantleonard seantleonard commented Jun 13, 2024

Why make this change?

I discovered another bug while working on this change #2268, which will be addressed separately.

What is this change?

  • Fixes a bug where GraphQL DateTime input with Timezone (TZ) offset was not honored.

    • e.g. 12-31-2024T12:00:00+03:00 was stored as 12-31-2024T12:00:00 in the backend database instead of 12-31-2024T09:00:00
    • This bug was discovered because some SupportedTypes tests involving dates always failed locally but not in the integration test pipeline. My local machine in UTC-8 while pipelines are UTC. Additionally, some test input 9999-12-31 23:59:59.9999999 was run for MySql where that value is an invalid date (max decimal is .499999).
  • Fixes false negative (passing) test cases in GraphQLSupportedTypesTests by adding more comprehensive asserts, updating DataRow test input, and ensuring tests specific to a database type are correctly executed.

    • As a result, MySQL tests in this specific test suite run way faster (~minutes down to less than 10 seconds because the conclusion of each data row execution doesn't reset the MySql database contents which takes a long time via ResetDbStateAsync().)
    • Updates test code queries and mutations to include the primary key (typeid) of the table type_table so that tests interact with determinate records:
    string gqlQuery = "{ supportedType_by_pk(typeid: " + id + ") { typeid, " + field + " } }";
  • A bug in the test

// Old test
[DataRow(TIME_TYPE, "\"23:59:59.9999999\"")]

// Fixed Test
[DataRow(TIME_TYPE, "23:59:59")]
public async Task InsertIntoTypeColumnWithArgument(string type, object value)

This test shows as green in main branch. However, looking at the log of the test, I saw this error:

GraphQL error: [{"message":"The variable `param` is not compatible with the type of the current location.","locations":[{"line":1,"column":65}],"path":["createSupportedType"],"extensions":{"variable":"param","variableType":"Time","locationType":"LocalTime","specifiedBy":"http://spec.graphql.org/October2021/#sec-All-Variable-Usages-are-Allowed"}}]

There are two layers of changes here:

  1. TIME_TYPE is technically not a GraphQL data type recognized or defined by DAB's generated GraphQL schema/ HotChocolate -> LocalTime is. So when the data row for TIME_TYPE comes in this code resolves the expected type:
        private static string TypeNameToGraphQLType(string typeName)
        {
            return typeName switch
            {
                DATETIMEOFFSET_TYPE => DATETIME_TYPE,
                TIME_TYPE => LOCALTIME_TYPE,
                _ => typeName
            };
        }
  1. Leaving quotes on the value -> \"23:59:59.9999999\" in results in the error:
GraphQL error: [{"message":"Unable to deserialize string to LocalTime","path":["param"]}]

why keep data row as "TIME_TYPE"? well, our test code resolves that type name not only as the GraphQL type but also as a way to determine which column in the data source to select which is named time_types. So we'd need to decouple the GraphQL Data Type value from being used to determine the SQL table column name.

How was this tested?

  • Integration Tests - Fixed tests in GraphQLSupportedTypesTests

…results can be correctly compared to db results.
…re readable . also add comments to describe how mysql datetime data type has certain restrictions and accommodate how GraphQL (Hotchocolate) resolves user supplied datetimeoffset and sends to database and how dab then processes and returns the database response.
…lte .33 because expected value query isn't parameterized and doesn't capture the proper data type. real data type must be used as real '0.33'. to get expected value.
@seantleonard
Copy link
Contributor Author

/azp run

…s. Removed commented out resetDbStateAsync(). fixed test input to include UTC offset because SqlClient assumes input without offset specifier is current timezone and not UTC. Tracking via bug #2265, but offset specified so that tests pass locally for Redmond devs.
@seantleonard seantleonard changed the title Dev/sean/improve type test exec time Fix MySQL test execution time + Fix DateTime GraphQL input handling Jun 15, 2024
@seantleonard seantleonard added this to the 1.2 milestone Jun 15, 2024
@seantleonard seantleonard marked this pull request as ready for review June 15, 2024 01:01
Copy link
Contributor

@ayush3797 ayush3797 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Neat! LGTM!

Copy link
Collaborator

@Aniruddh25 Aniruddh25 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Comments so far

abhishekkumams and others added 2 commits July 4, 2024 16:24
Co-authored-by: Aniruddh Munde <anmunde@microsoft.com>
…upportedTypesTestsBase.cs

Co-authored-by: Aniruddh Munde <anmunde@microsoft.com>
@abhishekkumams
Copy link
Contributor

/azp run

@abhishekkumams
Copy link
Contributor

/azp run

Copy link
Collaborator

@Aniruddh25 Aniruddh25 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Left few questions. But approved otherwise.

@abhishekkumams
Copy link
Contributor

/azp run

@abhishekkumams
Copy link
Contributor

/azp run

@abhishekkumams
Copy link
Contributor

/azp run

@abhishekkumams abhishekkumams merged commit 8a32f3f into main Jul 9, 2024
@abhishekkumams abhishekkumams deleted the dev/sean/improveTypeTestExecTime branch July 9, 2024 16:17
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.

5 participants