Introducing parameters' DbType before passing on to the database - MsSql - #1442
Merged
Conversation
ayush3797
marked this pull request as ready for review
April 26, 2023 07:54
ayush3797
requested review from
Aniruddh25,
JelteF,
mbhaskar and
seantleonard
as code owners
April 26, 2023 07:54
Aniruddh25
reviewed
Apr 28, 2023
Aniruddh25
reviewed
Apr 28, 2023
Aniruddh25
reviewed
Apr 28, 2023
Aniruddh25
reviewed
Apr 28, 2023
Aniruddh25
reviewed
Apr 28, 2023
Aniruddh25
reviewed
Apr 28, 2023
Aniruddh25
approved these changes
Apr 28, 2023
Aniruddh25
left a comment
Collaborator
There was a problem hiding this comment.
Thanks for this refactoring, discovering new issues and making sure the datatype is correct, no data loss for datetime/offset.
LGTM, after providing explanation in Code why DbType is nullable.
seantleonard
reviewed
May 1, 2023
seantleonard
reviewed
May 1, 2023
seantleonard
reviewed
May 1, 2023
seantleonard
reviewed
May 1, 2023
seantleonard
reviewed
May 1, 2023
seantleonard
reviewed
May 1, 2023
seantleonard
reviewed
May 1, 2023
seantleonard
reviewed
May 1, 2023
ayush3797
enabled auto-merge (squash)
May 3, 2023 20:17
seantleonard
reviewed
May 3, 2023
seantleonard
approved these changes
May 3, 2023
1 task
1 task
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Why make this change?
Fixes #475
Most column types will accept a string null and the db will automatically infer our intention, but for varbinary in MsSql the db complains that it cannot convert varchar to varbinary. For more details about the issue refer: https://stackoverflow.com/questions/29254690/why-does-dbnull-value-require-a-proper-sqldbtype
What is this change?
Changing how we form the
parametersas we pass them to the database. Previously we used to only pass the parameter's value. Now, we would also pass the parameter'sDbTypealong with the value as suggested in the stack overflow article.Notes:
DbTypefor Sql Server data types other thanvarbinary. However, we will still be supplying the DbType for all other parameter's for which we are confirmed about the 1:1 mapping from SystemType to DbType. Sql server datatypes likedatetime,datetime2are all treated asdatetimeby dotnet, so there is no 1:1 mapping for datetime datatypes. So we are skipping supplying the DbType for these.InsertOneInSupportedTypesis written, we have explicitly castguid_typesto lower case.Additional change
realsql server datatype was incorrectly mapped tolongdotnet type. It has been correctly mapped todecimalnow.Issue with DateTime/DateTimeOffset Sql Server data types
For a mutation request like this, we get a response:

and the parameters are generated as:

the parameter generated is of type
DateTimeOffset(as evident from the value highlighted) but the underlying Sql Server datatype for thedatetime_typesfield isdatetime. Even though we supplied a value1999-01-08 09:20:00in the mutation, it became08-01-1999 09:20:00 +05:30, because that's how graphql treats datetime/datetimoffset/date types. Then this implicit conversion from a value of type DateTimeOffset to DateTime cannot happen throwing an exception with message :Failed to convert parameter value from a DateTimeOffset to a DateTime.Hence we cannot add a mapping fromtypeof(DateTime) = DbType.DateTime.Another option that came to my mind and would probably come to your mind as well would be to have a mapping from
typeof(DateTime) = DbType.DateTimeOffset, since then there can be implicit conversion from value ofDateTimeOffsettype toDbType.DateTimeOffset. But that has its own flaws. Lets look at the below screenshots:Consider this mutation:

and the parameters generated for the query (mutation ran successfully because the default value for the
instantPK column was inserted into the table):
The parameter value is of sql server type datetime but the DbType is populated as `DbType.DateTimeOffset`. This upcasting cannot happen correctly and so we are returned with 0 records. So, this approach won't work as well.Conclusion: We won't populate DbType for these sql server types as it is not required and would prevent data loss and any unforeseen exceptions.
How was this tested?
Sample Request(s)
Request method: POST
Request URL: https://localhost:5001/api/SupportedType/
Request:
{ "bytearray_types": null }Response:
{ "value": [ { "typeid": 5001, "byte_types": null, "short_types": null, "int_types": null, "long_types": null, "string_types": null, "single_types": null, "float_types": null, "decimal_types": null, "boolean_types": null, "date_types": null, "datetime_types": null, "datetime2_types": null, "datetimeoffset_types": null, "smalldatetime_types": null, "bytearray_types": null, "guid_types": "27742da6-99f2-4e58-9fd8-4f143aea46b0" } ] }