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

Handling of datetime is flawed #339

Closed
jedvardsson opened this issue Jun 14, 2017 · 5 comments
Closed

Handling of datetime is flawed #339

jedvardsson opened this issue Jun 14, 2017 · 5 comments
Assignees
Projects

Comments

@jedvardsson
Copy link

jedvardsson commented Jun 14, 2017

The way that datetime is handled by mssql-jdbc is flawed. datetime types are time-zoneless, however, the driver maps them to the JVM time zone when deserializing the datetime sent from SQLServer.

The test program below shows what happens when reading the output of select cast('2017-03-26T02:00:00' as datetime2).

import java.sql.Connection;
import java.sql.DriverManager;
import java.sql.PreparedStatement;
import java.sql.ResultSet;
import java.sql.SQLException;
import java.sql.Statement;
import java.time.ZoneId;
import java.time.format.DateTimeFormatter;

public class Main1 {
    public static void main(String[] args) throws SQLException {
        String url = "jdbc:sqlserver://localhost:1433";
        String user = "sa";
        String password = "Test1234!";
        System.out.println("-Duser.timezone=" + ZoneId.systemDefault());
        try (
                Connection c = DriverManager.getConnection(url, user, password);
        ) {
            String expected = "2017-03-26T02:00:00";
            try (Statement s = c.createStatement()) {
                s.execute(String.format("select cast('%s' as datetime2)", expected));
                try (ResultSet r = s.getResultSet()) {
                    r.next();
                    LocalDateTime localDateTime = r.getTimestamp(1).toLocalDateTime();
                    String actual = localDateTime.format(DateTimeFormatter.ISO_LOCAL_DATE_TIME);
                    System.out.format("Main1: %s == %s %s%n", expected, actual, (expected.equals(actual) ? "OK" : "FAILED!"));
                }
            }
        }
    }
}

When running the program in UTC all is working nicely.

-Duser.timezone=UTC
Main1: 2017-03-26T02:00:00 == 2017-03-26T02:00:00 OK

However, in Europe/Stockholm (or CET) things fails.

-Duser.timezone=Europe/Stockholm
Main1: 2017-03-26T02:00:00 == 2017-03-26T03:00:00 FAILED!

Obviously this comes down to that 2017-03-26T02:00:00 in Europe/Stockholm falls just on the start of the DST gap. The driver internally converts the local date time to a zoned date time using java.util.Calendar and java.sql.Timestamp. Those classes can't work with truly local date times since they automatically move date times inside gaps to the next non-gap datetime which is 2017-03-26T03:00:00.

The problem is that local date times are linear:

  • The second after 2017-03-26T01:59:59 is 2017-03-26T02:00:00

While zoned date times are non-linear (because of how they are mapped on to UTC):

  • In Europe/Stockholm the second after 2017-03-26T01:59:59 is 2017-03-26T03:00:00.

Therefore we can't use java.util.Calendar and java.sql.Timestamp to represent local date times correctly unless we work in UTC. For other time zones the conversion between local data times -> zoned date times -> local date times is destructive.

A workaround in application code is to replace the call r.getTimestamp(1).toLocalDateTime() above with the more complex:

 LocalDateTime localDateTime = r.getTimestamp(1, Calendar.getInstance(TimeZone.getTimeZone("UTC"))).toInstant().atZone(ZoneOffset.UTC).toLocalDateTime();

Note that database IDE:s with database integrations such as IntelliJ or DbVisualizer aren't aware of this. They simply call ResultSet.getObject(). For example, the result of the below query

select
  t.a,
  t.b,
  datediff(minute, a, b) diff_minutes
from (
  select
    cast('2017-03-26T02:00:00' as datetime2) a, cast('2017-03-26T03:00:00' as datetime2) b
) t

is displayed like this in IntelliJ (in fact, this was how I spotted the problem in the first place).

ij-datediff

IDE DB Driver Displays correctly
Intellij sqlserver mssql NO
Intellij sqlserver jtdsl YES
Intellij maridb mysql NO
Intellij oracle-xe orcle-thin NO
DbVis sqlserver mssql NO
DbVis sqlserver mssql NO
DbVis mariadb mysql NO
SqlDeveloper oracle-xe oracle-thin YES
  1. Note that jtds works. This is because their getObject() returns a String and not a Timestamp.
  2. Note that Oracles SqlDeveloper also works (for oracle). They might have implemented the workaround.

I understand that the problem here is bigger then the mssql-jdbc and that it comes down to JDBC and its use of the old legacy classes Calendar and Timestamp. However, maybe mssql-jdbc can work with date times (and such types) in an internal format so that e.g. getTimestamp() keeps its old behavior and that getObject() returns a LocalDateTime?

@ajlam
Copy link
Member

ajlam commented Jun 19, 2017

Hi @jedvardsson , thank you for providing these details. We will have to dig into this a bit more. We are currently working to stabilize the code base to release and we'll circle back to this issue once that's complete.

@ajlam ajlam added the Under Review Used for pull requests under review label Jul 17, 2017
@cheenamalhotra
Copy link
Member

cheenamalhotra commented Aug 2, 2017

Hi @jedvardsson, thank you for raising this issue this us. We investigated your problem as above, but it doesn't look an ideal solution to provide different values when retrieved by different functions: getTimestamp() vs getObject()

As per JDBC specs (since 2.0), getObject() method is expected to return value of column type TIMESTAMP in the form of java.sql.Timestamp object, i.e. same object retrieved from getTimestamp() method. And as you are aware java.sql.Timestamp class object always holds its Timezone information in it, which means it will always give you the result with the DST settings.

The workaround mentioned by you is intended for advanced globalization uses only and is not recommended. As the function .toLocalDateTime() used below is from ChronoZonedDateTime interface which returns date-time with a time-zone in an arbitrary chronology (non-real).

LocalDateTime localDateTime = r.getTimestamp(1, Calendar.getInstance(TimeZone.getTimeZone("UTC"))).toInstant().atZone(ZoneOffset.UTC).toLocalDateTime();

From driver's perspective, we shall continue to follow JDBC Specs such that getTimestamp(), getString() and getObject() methods return exactly same value when fetched from the resultset.

Let us know if you have any further queries.

@cheenamalhotra
Copy link
Member

Hi @jedvardsson would like to know your views if you got chance to read my comment above.

@jedvardsson
Copy link
Author

Hi @cheenamalhotra ,

Thank you for taking your time and looking into this issue. I understand your point about not updating getObject(). Too bad that JDBC is flawed. However, do you agree with my points that the DATETIME on the SQL Server side is infact timezone-less and in a perfect world should be treated as such? I just want to make sure that I am under standing things correctly.

Secondly, I don't understand your point about toLocalDateTime() and ChronoZonedDateTime. Yes, the API states that the interface ChronoZonedDateTime is intended for advanced globalization issues. For example, if you need to implement your own type of chronology. The documentation recommends using ZonedDateTime which implements this interface.

And that is what I am doing: calling ZonedDateTime.toLocalDateTime(). Follow the call chain and you'll see that this is what happens. There is nothing wrong about it. In fact is simply reversion the bug: that local datetimes are wrongly passed as zoned datetimes. What the fix does is that it converts it back to a local datetime agin.

Let me know if I understood you correctly. Thanks.

@cheenamalhotra
Copy link
Member

cheenamalhotra commented Aug 24, 2017

Hi @jedvardsson,

Thanks for writing back. I agree with you for the workaround, calling ZonedDateTime.toLocalDateTime(). There is nothing wrong about it. It's just defined for advanced usage which is fine if it gets you the desired results. It is true that DATETIME and DATETIME2 datatypes in SQL Server contain timezone information and unfortunately JDBC Timestamp would always contain Daylight Savings Time and we can't remove support for that from the driver.

I am glad to know we both are on the same page and you agree we must not update getObject() method to return timezone without DST. I am closing this issue as I believe there is no change needed from the driver and workaround mentioned by you would solve your query.

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 Issues
Development

No branches or pull requests

4 participants