-
Notifications
You must be signed in to change notification settings - Fork 423
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Fixes #1590 - [FEATURE REQUEST] Ability to explicitly specify conversion to datetime or datetime2 #1687
Conversation
* Added support for the dateTimeType connection string, which allows you to define how date/time values are parameterized in SQL Server. By default, for SQL Server 2008+ the "datetime2" datatype is used, where as older versions of SQL Server always use the "datetime". This allows users migrating older database application that exclusively use "datetime" datatypes to run in SQL Server compatibility modes newer than 2014. * Added unit test coverage for changes
Hello @dswitzer, |
This reverts commit 4e8d787.
The merge was corrupted and blew out most of the datetimeParameterType feature changes, so I've re-applied the changes manually.
@v-zhangw , Ok, I believe the merge should be fixed, some of the checks are failing, but they not in portions of the code that should have been affected by my changes. |
@v-zhangw Just checking up to see if there's anything more you need from me. |
@lilgreenbird @v-zhangw, this pull request would be really useful, as updating to a later driver would unblock migration to jdk17 for us (ssl issues with existing very old driver). Is there any chance someone can take a look at it? |
It should be pretty easy to resolve the conflicts, I just do not have write access to the repository to do it. |
@dswitzer not sure but I think you have to update your forked branch and fix the conflicts there. |
I'd be happy to update the fork, but the origin code keeps changing so fast, that the PR keeps becoming invalidated. It wasn't worth the time to keep fixing the code when there's no movement on them approving the PR. If they're willing to actually move on the PR, I'm happy to fix the conflicts. I just don't want to keep spending time to fix my PR for no reason. |
@v-zhangw @lilgreenbird @Jeffery-Wasty what do you think about the pr? |
Hi @AlBundy33, The issue and associated PR are on our radar and we'll look into both of them as soon as we are able to. |
Hi @dswitzer Apologies this has been put off for so long. We had determined that the #1590 needed more investigation and then we have had problems juggling this as this a feature request so it kept getting bumped by other higher priority issues. We are now getting back to looking into this again. Once you get this PR up to date we will run this thru our test lab to verify. |
Syncing fork to origin
I've updated the pull request. The failures do not appear to be related to my changes. |
When I run the Format Document command from VS Code, it's making a lot of changes to the files in places of the code I did not change—especially in the comments. Is that what you're wanting me to do? If not, if you could provide me with explicit instructions on what you need me to do, I would appreciate it very much! |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'm good with the public-facing aspects of this change (public APIs, connection string additions). I had one javadoc-related suggestion and I commented on a few code issues, too.
src/main/java/com/microsoft/sqlserver/jdbc/ISQLServerConnection.java
Outdated
Show resolved
Hide resolved
src/main/java/com/microsoft/sqlserver/jdbc/ISQLServerDataSource.java
Outdated
Show resolved
Hide resolved
switch (con.getDatetimeParameterType()){ | ||
case "datetime2": | ||
datatype = SSType.DATETIME2.toString(); | ||
if (scale != null){ | ||
datatype += "(" + scale + ")"; | ||
} | ||
return datatype; | ||
case "datetimeoffset": | ||
datatype = SSType.DATETIMEOFFSET.toString(); | ||
if (scale != null){ | ||
datatype += "(" + scale + ")"; | ||
} | ||
return datatype; | ||
case "datetime": |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Can we reference SQLServerDriver.DatetimeType instead of hard-coding "datetime2"/"datetime"/"datetimeoffset" strings here?
You should be able to switch on the enum:
switch (DatetimeType.valueOfString(con.getDatetimeParameterType()){
case DatetimeType.DATETIME2:
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
this will be updated in a subsequent PR
if (!isValidDatetimeParameterType(datetimeParameterTypeValue)){ | ||
String errorMessage = "The timestamp encoding value (i.e. " + datetimeParameterTypeValue.toString() + ") must be: datetime, datetime2 or datetimeoffset."; | ||
SQLServerException newe = new SQLServerException(errorMessage, null); | ||
throw newe; | ||
} | ||
datetimeParameterType = DatetimeType.valueOfString(datetimeParameterTypeValue); | ||
} | ||
|
||
private boolean isValidDatetimeParameterType(String datetimeParameterTypeValue) { | ||
return (datetimeParameterTypeValue.equals("datetime") || datetimeParameterTypeValue.equals("datetime2") || datetimeParameterTypeValue.equals("datetimeoffset")); | ||
} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
You can run all this through the DatetimeType.valueOfString(datetimeParameterValueType)
method to validate the input parameter and eliminate the error message instead of hard coding more strings here.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
this will be updated in a subsequent PR
/azp run CI-MacOS |
Azure Pipelines successfully started running 1 pipeline(s). |
Co-authored-by: David Engel <dengel1012@gmail.com>
Co-authored-by: David Engel <dengel1012@gmail.com>
Co-authored-by: David Engel <dengel1012@gmail.com>
@lilgreenbird is this already available in a release? |
Hi @AlBundy33, This is planned to be included in the 12.2.0 release, scheduled for the end of this month. |
I've implemented a new connection string (i.e.
datetimeParameterType
) which fixes #1590 by allowing users to specify the datatype to use for date/timestamp parameters.This pull request adds the ability to define the
datetimeParameterType
connection string with any of the following values:By default, the value defaults to
datetime2
, which is the same behavior as today. Setting this value to any of the above will cause any SQL version of Katmai or higher to always use that datatype for date/time values when generating parameters. For older versions of SQL Server that do not supportdatetime2
ordatetimeoffset
, thendatetime
is always used (just as it has been working).I've added unit test coverage for the changes.
There should be no behavioral change for users who do not set the
datetimeParameterType
connection string value. It should work as it has been. However, when supporting older databases running in SQL Server 2016 or higher compatibility mode, thedatetimeParameterType
can be set todatetime
to support databases only using that data type without worrying about casting issues in SQL Server with conversion betweendatetime2
anddatetime
.