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.

Sign up for GitHub

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

Jump to bottom

Fix #1390 LocalDateTime string serialization #1391

Closed
wants to merge 2 commits into from
Closed

Fix #1390 LocalDateTime string serialization #1391

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
b2e149e
Stop tethering `LocalDateTime` to current zone on serialization
kdubb Jan 18, 2019
306e287
Test binary modes in SetObject310Test
vlsi Jan 20, 2019
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Jump to
Jump to file
Failed to load files.
Diff view
Diff view
5 changes: 2 additions & 3 deletions pgjdbc/src/main/java/org/postgresql/jdbc/TimestampUtils.java
View file
Open in desktop
Expand Up @@ -830,9 +830,8 @@ public synchronized String toString(LocalDateTime localDateTime) {
return "-infinity";
}

// LocalDateTime is always passed with time zone so backend can decide between timestamp and timestamptz
ZonedDateTime zonedDateTime = localDateTime.atZone(getDefaultTz().toZoneId());
return toString(zonedDateTime.toOffsetDateTime());
OffsetDateTime offsetDateTime = localDateTime.atOffset(ZoneOffset.UTC);
Copy link
Contributor

Choose a reason for hiding this comment

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

I think we should do the opposite there. LocalDateTime has no time zone, we should not make up one. I would simply not send a time zone if we don't have one.

return toString(offsetDateTime);
Copy link

Choose a reason for hiding this comment

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

This renders something like 2019-07-30 09:17:25.676+00
Why can't this be 2019-07-30 09:17:25.676, i.e. without zone offset?

Consider use-cases

  • inserting ? bound to LocalDateTime to a timestamp column. Then zone-less literal would be OK.
  • inserting ? bound to LocalDateTime to a timestamp with time zone column (which represents point in time in PostgreSQL). Then zone-less literal should probably be coerced on server side to session zone, so zone-less literal should be OK. UTC-zoned literal will likely be not OK as it will represent a different point in time.
  • (....)

If this needs to stay this way, please add some explanation.

}

private static void appendDate(StringBuilder sb, LocalDate localDate) {
Expand Down
27 changes: 21 additions & 6 deletions pgjdbc/src/test/java/org/postgresql/test/jdbc42/SetObject310Test.java
View file
Open in desktop
Expand Up @@ -11,12 +11,14 @@
import static org.junit.Assume.assumeTrue;

import org.postgresql.test.TestUtil;
import org.postgresql.test.jdbc2.BaseTest4;

import org.junit.After;
import org.junit.Before;
import org.junit.Test;
import org.junit.runner.RunWith;
import org.junit.runners.Parameterized;

import java.sql.Connection;
import java.sql.PreparedStatement;
import java.sql.ResultSet;
import java.sql.SQLException;
Expand All @@ -38,10 +40,12 @@
import java.time.temporal.ChronoField;
import java.util.ArrayList;
import java.util.Arrays;
import java.util.Collection;
import java.util.List;
import java.util.TimeZone;

public class SetObject310Test {
@RunWith(Parameterized.class)
public class SetObject310Test extends BaseTest4 {
private static final TimeZone saveTZ = TimeZone.getDefault();

public static final DateTimeFormatter LOCAL_TIME_FORMATTER =
Expand All @@ -64,11 +68,22 @@ public class SetObject310Test {
.withResolverStyle(ResolverStyle.LENIENT)
.withChronology(IsoChronology.INSTANCE);

private Connection con;
public SetObject310Test(BaseTest4.BinaryMode binaryMode) {
setBinaryMode(binaryMode);
}

@Parameterized.Parameters(name = "binary = {0}")
public static Iterable<Object[]> data() {
Collection<Object[]> ids = new ArrayList<Object[]>();
for (BaseTest4.BinaryMode binaryMode : BaseTest4.BinaryMode.values()) {
ids.add(new Object[]{binaryMode});
}
return ids;
}

@Before
public void setUp() throws Exception {
con = TestUtil.openDB();
super.setUp();
TestUtil.createTable(con, "table1", "timestamp_without_time_zone_column timestamp without time zone,"
+ "timestamp_with_time_zone_column timestamp with time zone,"
+ "date_column date,"
Expand All @@ -81,7 +96,7 @@ public void setUp() throws Exception {
public void tearDown() throws SQLException {
TimeZone.setDefault(saveTZ);
TestUtil.dropTable(con, "table1");
TestUtil.closeDB(con);
super.tearDown();
}

private void insert(Object data, String columnName, Integer type) throws SQLException {
Expand Down Expand Up @@ -199,7 +214,7 @@ public void testSetLocalDateTime() throws SQLException {
ZoneId zone = ZoneId.of(zoneId);
for (String date : datesToTest) {
LocalDateTime localDateTime = LocalDateTime.parse(date);
String expected = localDateTime.atZone(zone)
Copy link
Contributor

Choose a reason for hiding this comment

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

IMHO it is correct this was changed, asserting that 2000-03-26T02:00:00 is stored or retrieved as 2000-03-26T03:00:00 when the system time zone is a specific one is wrong IMHO. 2000-03-26T02:00:00 is 2000-03-26T02:00:00. As it has no time zone the behaviour should be the same and independent of any time zone.

I think the tests should be changed though to not call #getString as it currently calls PgResultSet#internalGetObject which ends up calling java.sql.Timestamp#toString() which is bound to the JVM time zone and can therefore not display timestamps that don't exist in that time zone. As LocalDateTime has no time zone this is wrong IMHO.

Copy link
Member

Choose a reason for hiding this comment

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

makes sense.. care to provide a PR ?

Copy link
Member

Choose a reason for hiding this comment

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

I'm looking at this and wondering why we are using timezones at all on LocalDateTime?

Copy link
Contributor

Choose a reason for hiding this comment

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

makes sense.. care to provide a PR ?

I can have a look. I'll have to check if it breaks any tests. It may take a few days.

Copy link
Member

Choose a reason for hiding this comment

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

Probably not worth your time until we decide how to deal with this.

String expected = localDateTime
.format(DateTimeFormatter.ISO_LOCAL_DATE_TIME)
.replace('T', ' ');
localTimestamps(zone, localDateTime, expected);
Expand Down