Skip to content

fix millisecond parsing errors #895

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

Merged
merged 2 commits into from
Nov 25, 2019
Merged

fix millisecond parsing errors #895

Show file tree
Hide file tree
Changes from 1 commit
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
d6a5c44
fix millisecond parsing errors
elharo Nov 25, 2019
baa24ea
nits
elharo Nov 25, 2019
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
11 changes: 6 additions & 5 deletions google-http-client/src/main/java/com/google/api/client/util/DateTime.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -273,7 +273,7 @@ public int hashCode() {
* exception is thrown if {@code str} doesn't match {@code RFC3339_REGEX} or if it contains a
* time zone shift but no time.
*/
public static DateTime parseRfc3339(String str) throws NumberFormatException {
public static DateTime parseRfc3339(String str) {
Copy link
Member

Choose a reason for hiding this comment

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

Nice. NumberFormatException is a RuntimeException and thus no need to explicitly mark it.
https://docs.oracle.com/javase/8/docs/api/java/lang/NumberFormatException.html

return parseRfc3339WithNanoSeconds(str).toDateTime();
}

Expand All @@ -285,9 +285,9 @@ public static DateTime parseRfc3339(String str) throws NumberFormatException {
* exception is thrown if {@code str} doesn't match {@code RFC3339_REGEX} or if it contains a
* time zone shift but no time.
*/
public static SecondsAndNanos parseRfc3339ToSecondsAndNanos(String str)
throws IllegalArgumentException {
return parseRfc3339WithNanoSeconds(str).toSecondsAndNanos();
public static SecondsAndNanos parseRfc3339ToSecondsAndNanos(String str) {
Rfc3339ParseResult time = parseRfc3339WithNanoSeconds(str);
return time.toSecondsAndNanos();
}

/** A timestamp represented as the number of seconds and nanoseconds since Epoch. */
Expand Down Expand Up @@ -335,7 +335,7 @@ public String toString() {
}
}

/** Result of parsing a Rfc3339 string. */
/** Result of parsing an RFC 3339 string. */
private static class Rfc3339ParseResult implements Serializable {
private final long seconds;
private final int nanos;
Expand Down Expand Up @@ -400,6 +400,7 @@ private static Rfc3339ParseResult parseRfc3339WithNanoSeconds(String str)
}
}
Calendar dateTime = new GregorianCalendar(GMT);
dateTime.clear();
dateTime.set(year, month, day, hourOfDay, minute, second);
long value = dateTime.getTimeInMillis();

Expand Down
21 changes: 13 additions & 8 deletions google-http-client/src/test/java/com/google/api/client/util/DateTimeTest.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -15,6 +15,7 @@
package com.google.api.client.util;

import com.google.api.client.util.DateTime.SecondsAndNanos;

Copy link
Member

Choose a reason for hiding this comment

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

Nit?

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

import java.util.Date;
import java.util.TimeZone;
import junit.framework.TestCase;
Expand All @@ -28,12 +29,6 @@ public class DateTimeTest extends TestCase {

private TimeZone originalTimeZone;

public DateTimeTest() {}

public DateTimeTest(String testName) {
super(testName);
}

@Override
protected void setUp() throws Exception {
originalTimeZone = TimeZone.getDefault();
Expand Down Expand Up @@ -225,8 +220,18 @@ public void testParseRfc3339ToSecondsAndNanos() {
assertParsedRfc3339(
"2018-03-01T10:11:12.1000Z", SecondsAndNanos.ofSecondsAndNanos(1519899072L, 100000000));
}

public void testEpoch() {
assertParsedRfc3339(
"1970-01-01T00:00:00.000Z", SecondsAndNanos.ofSecondsAndNanos(0, 0));
}

public void testOneSecondBeforeEpoch() {
assertParsedRfc3339(
"1969-12-31T23:59:59.000Z", SecondsAndNanos.ofSecondsAndNanos(-1, 0));
Copy link
Member

Choose a reason for hiding this comment

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

Great test.

}
Copy link
Member

Choose a reason for hiding this comment

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

Would you add the following test too?

public void testGregorianCalendarStartDate() {
    assertParsedRfc3339(
        "1582-10-15T01:23:45.123Z", SecondsAndNanos.ofSecondsAndNanos(-12219287774L, -877000000));
}

(not 100% sure about the value of nanosecond part to represent -12219287774877 milliseconds)

As per Beam's PubsubClientTest, date 1582-10-15 is an important date when Gregorian Calendar was adopted.
https://github.com/apache/beam/blob/master/sdks/java/io/google-cloud-platform/src/test/java/org/apache/beam/sdk/io/gcp/pubsub/PubsubClientTest.java#L144

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 can't verify that this value is correct, so I don't want to add it here.

Copy link
Member

Choose a reason for hiding this comment

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

I will try to add this in a separate PR then.


private void assertParsedRfc3339(String input, SecondsAndNanos expected) {
private static void assertParsedRfc3339(String input, SecondsAndNanos expected) {
SecondsAndNanos actual = DateTime.parseRfc3339ToSecondsAndNanos(input);
assertEquals(
"Seconds for " + input + " do not match", expected.getSeconds(), actual.getSeconds());
Expand All @@ -249,7 +254,7 @@ public void testParseAndFormatRfc3339() {
assertEquals(expected, output);
}

private void expectExceptionForParseRfc3339(String input) {
private static void expectExceptionForParseRfc3339(String input) {
try {
DateTime.parseRfc3339(input);
fail("expected NumberFormatException");
Expand Down