Skip to content

Handling datetime types - #1636

Merged
ayush3797 merged 110 commits into
mainfrom
dev/agarwalayush/bugWithHandlingTimeDataTypes
Dec 6, 2023
Merged

Handling datetime types#1636
ayush3797 merged 110 commits into
mainfrom
dev/agarwalayush/bugWithHandlingTimeDataTypes

Conversation

@ayush3797

@ayush3797 ayush3797 commented Aug 16, 2023

Copy link
Copy Markdown
Contributor

Why make this change?

Currently, we are incorrectly handling the datetime datatypes. A snippet from BaseSqlQueryStructure class:

image

As evident from the highlighted portion, even for DateTime .NET type, we are parsing it with DateTimeOffset which is wrong. Just for an example, if you execute a GET request on the stocks_price table which has (categoryid,pieceid,instant) as PK, you would not get any result back even when the record exists for that PK in the table. Why? Because even though instant is a datetime column, it gets parsed as datetimeoffset column. This is shown in the next 2 images.

image image

Also, the existing tests (like the ones fixed in MsSqlGraphQLPaginationTests and GraphQLSupportedTypesTestsBase) were wrongly written, in which values in the range of datetime2 were supplied for datetime column.

What is this change?

  1. Correctly return value using DateTime field of the DateTimeOffset parsed value in BaseSqlQueryStructure class,

  2. Populate the DbType of the datetime parameters when they are sent to the database so that the database knows the exact type of the parameter. Eg. by default, if you send a datetime2 param without a DbType, it is assumed to be a datetime and an exception is thrown incorrectly by the database. This is done using the map TypeHelper._timeSqlDbTypeToDbType.
    It should be noted 4 sql server types - date, smalldatetime, datetime, datetime2 map to the same .NET system type System.DateTime. The existing TypeHelper._systemTypeToDbTypeMap would not be of any use to us because it has mapping from system type to DbType and all the above 3 sql server types have the same system type. So, we need to rely on the actual mapping from sql server type to DbType which is:
    datetime -> DbType.DateTime, smalldatetime -> DbType.DateTime, datetime2 -> DbType.DateTime2
    date -> DbType.Date

  3. Fix the existing tests.

How was this tested?

  • Integration Tests - Added test for getting single record by PK having a datetime column as a PK column to FindApiTestBase.
  • Unit Tests - Added filter/orderby and insert tests to GraphQLSupportedTypesTestsBase.QueryTypeColumnFilterAndOrderByDateTime and GraphQLSupportedTypesTestsBase.InsertIntoTypeColumnfor datetime types - datetime2, smalldatetime, date.

Sample Request(s)

  • Example REST request to demonstrate modifications
image
  • Example GQL request to demonstrate modifications
image

Comment thread src/Service.GraphQLBuilder/GraphQLTypes/SupportedTypes.cs Outdated
Comment thread src/Service.GraphQLBuilder/Queries/StandardQueryInputs.cs
@ayush3797

Copy link
Copy Markdown
Contributor Author

/azp run

@azure-pipelines

Copy link
Copy Markdown
Azure Pipelines successfully started running 6 pipeline(s).

@ayush3797

Copy link
Copy Markdown
Contributor Author

/azp run

@azure-pipelines

Copy link
Copy Markdown
Azure Pipelines successfully started running 6 pipeline(s).

@ayush3797

Copy link
Copy Markdown
Contributor Author

/azp run

@azure-pipelines

Copy link
Copy Markdown
Azure Pipelines successfully started running 6 pipeline(s).

Comment thread src/Core/Services/MetadataProviders/MsSqlMetadataProvider.cs
Comment thread src/Core/Services/MetadataProviders/MsSqlMetadataProvider.cs
Comment thread src/Core/Services/MetadataProviders/MsSqlMetadataProvider.cs
Comment thread src/Core/Services/MetadataProviders/MsSqlMetadataProvider.cs

@aaronburtle aaronburtle left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Looks good just a few nits before merging if possible.

@ayush3797

Copy link
Copy Markdown
Contributor Author

/azp run

@azure-pipelines

Copy link
Copy Markdown
Azure Pipelines successfully started running 6 pipeline(s).

@seantleonard

Copy link
Copy Markdown
Contributor

@microsoft-github-policy-service rerun

@ayush3797
ayush3797 dismissed Aniruddh25’s stale review December 6, 2023 17:14

Comments addressed.

@ayush3797

Copy link
Copy Markdown
Contributor Author

Ignoring the failing test for DwSql FindByDateTimePKTest(). @rohkhann will take a look. Not a blocker.

@ayush3797
ayush3797 merged commit 0829d85 into main Dec 6, 2023
@ayush3797
ayush3797 deleted the dev/agarwalayush/bugWithHandlingTimeDataTypes branch December 6, 2023 17:15
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

bug Something isn't working mssql an issue thats specific to mssql

Projects

Status: Done

Development

Successfully merging this pull request may close these issues.

[Bug]: datetime types incorrectly handled for MsSql

5 participants