Skip to content
This repository has been archived by the owner on Dec 3, 2023. It is now read-only.

fix: Timestamp.of(java.sql.Timestamp) pre-epoch on exact second #179

Merged
merged 4 commits into from Mar 12, 2020
Merged

fix: Timestamp.of(java.sql.Timestamp) pre-epoch on exact second #179

Show file tree
Hide file tree
Changes from 1 commit
Commits
Show all changes
4 commits
Select commit Hold shift + click to select a range
e10e4c0
fix: Timestamp.of(java.sql.Timestamp) pre-epoch on exact second
olavloite Mar 12, 2020
9bdd7f1
fix: review comments
olavloite Mar 12, 2020
7fb96d8
fix: review comments
olavloite Mar 12, 2020
b9e2161
fix: make all testOf method consistent
olavloite Mar 12, 2020
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
6 changes: 3 additions & 3 deletions google-cloud-core/src/main/java/com/google/cloud/Timestamp.java
View file
Open in desktop
Expand Up @@ -121,13 +121,13 @@ public static Timestamp now() {
public static Timestamp of(java.sql.Timestamp timestamp) {
int nanos = timestamp.getNanos();

// A pre-epoch timestamp will be off by one second because of the way that integer division
// A pre-epoch timestamp could be off by one second because of the way that integer division
olavloite marked this conversation as resolved.
Show resolved Hide resolved
// works. For example, -1001 / 1000 == -1. In this case of timestamps, we want this result to be
olavloite marked this conversation as resolved.
Show resolved Hide resolved
// -2. This causes any pre-epoch timestamp to be off by 1 second - fix this by adjusting the
olavloite marked this conversation as resolved.
Show resolved Hide resolved
// seconds value by 1 if the timestamp < 0.
// seconds value by 1 if the timestamp < 0 and the division by 1000 has a remainder.
Copy link
Contributor

Choose a reason for hiding this comment

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

to be technical, a non-zero remainder. Suggest rephrasing as "the timestamp is less than zero and is not divisible by 1000."

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Changed to fix this by subtracting 1 from the seconds value if the seconds value is less than zero and is not divisible by 1000.

// TODO: replace with Math.floorDiv when we drop Java 7 support
long seconds = timestamp.getTime() / 1000;
if (seconds < 0) {
if (seconds < 0 && timestamp.getTime() % 1000 != 0) {
--seconds;
}

Expand Down
16 changes: 16 additions & 0 deletions google-cloud-core/src/test/java/com/google/cloud/TimestampTest.java
View file
Open in desktop
Expand Up @@ -88,6 +88,14 @@ public void ofSqlTimestamp() {
assertThat(timestamp.toString()).isEqualTo(expectedTimestampString);
}

@Test
public void ofSqlTimestampOnExactSecond() {
Copy link
Contributor

Choose a reason for hiding this comment

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

Can you make this method name more conventional? e.g. testOf_exactSecond or some such.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Done. Although this means that the name of this test is not in line with the other test cases in this file, such as for example ofSqlTimestampPreEpoch.

Copy link
Contributor

Choose a reason for hiding this comment

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

Section 8.5.2 of the Google style guide addresses this:

When adding new code to a file that is not in Google Style, reformatting the existing code first is recommended, subject to the advice in Section 8.5.1, Reformatting existing code.

If this reformatting is not done, new code is still required to be added in Google Style, even if this creates inconsistencies.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I've updated all the test cases for Timestamp.of(java.sql.Timestamp) to use consistent naming and assertEquals instead of assertThat....

String expectedTimestampString = "1970-01-01T00:00:12Z";
java.sql.Timestamp input = new java.sql.Timestamp(12000);
Timestamp timestamp = Timestamp.of(input);
assertThat(timestamp.toString()).isEqualTo(expectedTimestampString);
olavloite marked this conversation as resolved.
Show resolved Hide resolved
}

@Test
public void ofSqlTimestampPreEpoch() {
String expectedTimestampString = "1969-12-31T23:59:47.655000000Z";
Expand All @@ -104,6 +112,14 @@ public void ofSqlTimestampOnEpoch() {
assertThat(timestamp.toString()).isEqualTo(expectedTimestampString);
}

@Test
public void ofSqlTimestampPreEpochOnExactSecond() {
olavloite marked this conversation as resolved.
Show resolved Hide resolved
String expectedTimestampString = "1969-12-31T23:59:59Z";
java.sql.Timestamp input = new java.sql.Timestamp(-1000);
Timestamp timestamp = Timestamp.of(input);
assertThat(timestamp.toString()).isEqualTo(expectedTimestampString);
olavloite marked this conversation as resolved.
Show resolved Hide resolved
}

@Test
public void ofDatePreEpoch() {
Timestamp timestamp = Timestamp.of(TEST_DATE_PRE_EPOCH);
Expand Down