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

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

Merged
merged 4 commits into from Mar 12, 2020

Conversation

olavloite
Copy link
Contributor

@olavloite olavloite commented Mar 12, 2020

The fix for negative timestamps in #160 did not take into account the possibility that the pre-epoch timestamp could be on an exact second value. In that case, the additional correction of the calculated second value should not be applied.

Fixes googleapis/java-spanner-jdbc#85

@olavloite olavloite requested review from chingor13, elharo and codyoss Mar 12, 2020
@googlebot googlebot added the cla: yes label Mar 12, 2020
@codecov
Copy link

@codecov codecov bot commented Mar 12, 2020

Codecov Report

Merging #179 into master will not change coverage.
The diff coverage is 100%.

Impacted file tree graph

@@            Coverage Diff            @@
##             master     #179   +/-   ##
=========================================
  Coverage     67.76%   67.76%           
- Complexity      378      379    +1     
=========================================
  Files            36       36           
  Lines          1967     1967           
  Branches        259      259           
=========================================
  Hits           1333     1333           
  Misses          526      526           
  Partials        108      108
Impacted Files Coverage Δ Complexity Δ
...core/src/main/java/com/google/cloud/Timestamp.java 90.62% <100%> (ø) 26 <0> (+1) ⬆️

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 ae35d0e...b9e2161. Read the comment docs.

@@ -88,6 +88,14 @@ public void ofSqlTimestamp() {
assertThat(timestamp.toString()).isEqualTo(expectedTimestampString);
}

@Test
public void ofSqlTimestampOnExactSecond() {
Copy link
Contributor

@elharo elharo Mar 12, 2020

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

@olavloite olavloite Mar 12, 2020

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

@elharo elharo Mar 12, 2020

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

@olavloite olavloite Mar 12, 2020

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....

@olavloite olavloite requested a review from elharo Mar 12, 2020
// works. For example, -1001 / 1000 == -1. In this case of timestamps, we want this result to be
// -2. This causes any pre-epoch timestamp to be off by 1 second - fix this by adjusting the
// 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

@elharo elharo Mar 12, 2020

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

@olavloite olavloite Mar 12, 2020

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.

@olavloite olavloite requested a review from elharo Mar 12, 2020
Copy link
Member

@codyoss codyoss left a comment

Good catch

elharo
elharo approved these changes Mar 12, 2020
@chingor13 chingor13 merged commit 9bfb54c into master Mar 12, 2020
16 checks passed
@chingor13 chingor13 deleted the fix-sql-timestamp-on-exact-second-pre-epoch branch Mar 12, 2020
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
cla: yes
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants