Skip to content
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

java.time.OffsetDateTime value sent to the server should not be affected by the default time zone. #831

Merged
merged 6 commits into from
Dec 28, 2018

Conversation

harawata
Copy link
Contributor

Although SQLServerPreparedStatement#setObject(int, Object) supports java.time.OffsetDateTime, the driver sends an incorrect temporal data when the parameter has a different time zone than the default time zone.
Please see the attached test case.

java.sql.Timestamp#getTime() used in DTV#sendTemporal() subsequently invokes java.util.Date#normalize() which takes the default TimeZone into account.

…as a different time zone than the default time zone is passed to `PreparedStatement#setObject()`, the driver sends an incorrect value.
…Time().

`java.sql.Timestamp#getTime()` subsequently invokes `java.util.Date#normalize()` which takes the default TimeZone into account.
@codecov-io
Copy link

codecov-io commented Oct 14, 2018

Codecov Report

Merging #831 into dev will increase coverage by 0.23%.
The diff coverage is 100%.

Impacted file tree graph

@@             Coverage Diff              @@
##                dev     #831      +/-   ##
============================================
+ Coverage     49.69%   49.93%   +0.23%     
- Complexity     2880     2884       +4     
============================================
  Files           117      118       +1     
  Lines         27998    28006       +8     
  Branches       4666     4673       +7     
============================================
+ Hits          13915    13984      +69     
+ Misses        11839    11772      -67     
- Partials       2244     2250       +6
Flag Coverage Δ Complexity Δ
#JDBC42 49.45% <100%> (?) 2840 <0> (?)
#JDBC43 49.9% <100%> (+0.2%) 2883 <0> (+3) ⬆️
Impacted Files Coverage Δ Complexity Δ
...rc/main/java/com/microsoft/sqlserver/jdbc/dtv.java 66.72% <100%> (+1.99%) 0 <0> (ø) ⬇️
.../com/microsoft/sqlserver/jdbc/SQLServerJdbc42.java 25% <0%> (ø) 0% <0%> (?)
...in/java/com/microsoft/sqlserver/jdbc/IOBuffer.java 56.22% <0%> (+0.13%) 0% <0%> (ø) ⬇️
...ncurrentlinkedhashmap/ConcurrentLinkedHashMap.java 39% <0%> (+0.21%) 43% <0%> (ø) ⬇️
.../com/microsoft/sqlserver/jdbc/SQLServerDriver.java 80.04% <0%> (+0.22%) 25% <0%> (ø) ⬇️
...oft/sqlserver/jdbc/SQLServerPreparedStatement.java 52.28% <0%> (+0.22%) 213% <0%> (ø) ⬇️
.../microsoft/sqlserver/jdbc/SQLServerDataSource.java 45.12% <0%> (+0.24%) 66% <0%> (ø) ⬇️
...c/main/java/com/microsoft/sqlserver/jdbc/Util.java 62.2% <0%> (+0.86%) 90% <0%> (+1%) ⬆️
...n/java/com/microsoft/sqlserver/jdbc/Parameter.java 66.33% <0%> (+1.36%) 64% <0%> (ø) ⬇️
... and 2 more

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update ec052ca...aef9c06. Read the comment docs.

@petarov
Copy link

petarov commented Dec 6, 2018

Any news on this PR? This is really a problem on my side as well.

@lilgreenbird
Copy link
Member

hi @petarov,

Sorry it had taken us a while to get to this. We are now in the process of reviewing this PR and I am tasked to run this through our test lab. Once this is completed we will be looking at merging this for the upcoming release.

Thanks for your patience!

cheenamalhotra
cheenamalhotra previously approved these changes Dec 10, 2018
@cheenamalhotra cheenamalhotra merged commit 10fd845 into microsoft:dev Dec 28, 2018
MSSQL JDBC automation moved this from Under Investigation to Closed/Merged PRs Dec 28, 2018
@harawata harawata deleted the setobject-offsetdatetime-bug branch December 28, 2018 18:29
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
MSSQL JDBC
  
Closed/Merged PRs
Development

Successfully merging this pull request may close these issues.

None yet

6 participants