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(gax): prevent truncation/overflow when converting time values #3095

Merged
merged 5 commits into from
Aug 29, 2024
Merged

fix(gax): prevent truncation/overflow when converting time values #3095

Show file tree
Hide file tree
Changes from 1 commit
Commits
Show all changes
5 commits
Select commit Hold shift + click to select a range
0fb8160
fix(gax): prevent truncation/overflow when converting time values
diegomarquezp Aug 12, 2024
329c467
Update gax-java/gax/src/test/java/com/google/api/gax/util/TimeConvers…
diegomarquezp Aug 13, 2024
5014c4b
Update gax-java/gax/src/test/java/com/google/api/gax/util/TimeConvers…
diegomarquezp Aug 13, 2024
86d5855
add max possible nano quantity to max possible seconds quantity in du…
diegomarquezp Aug 15, 2024
cfa3b06
Merge branch 'fix-time-conversion-precision' of https://github.com/go…
diegomarquezp Aug 15, 2024
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
8 changes: 4 additions & 4 deletions gax-java/gax/src/main/java/com/google/api/gax/util/TimeConversionUtils.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -35,27 +35,27 @@ public static java.time.Duration toJavaTimeDuration(org.threeten.bp.Duration sou
if (source == null) {
return null;
}
return java.time.Duration.ofNanos(source.toNanos());
return java.time.Duration.ofSeconds(source.getSeconds(), source.getNano());
}

public static org.threeten.bp.Duration toThreetenDuration(java.time.Duration source) {
if (source == null) {
return null;
}
return org.threeten.bp.Duration.ofNanos(source.toNanos());
return org.threeten.bp.Duration.ofSeconds(source.getSeconds(), source.getNano());
}

public static java.time.Instant toJavaTimeInstant(org.threeten.bp.Instant source) {
if (source == null) {
return null;
}
return java.time.Instant.ofEpochMilli(source.toEpochMilli());
return java.time.Instant.ofEpochSecond(source.getEpochSecond(), source.getNano());
}

public static org.threeten.bp.Instant toThreetenInstant(java.time.Instant source) {
if (source == null) {
return null;
}
return org.threeten.bp.Instant.ofEpochMilli(source.toEpochMilli());
return org.threeten.bp.Instant.ofEpochSecond(source.getEpochSecond(), source.getNano());
}
}
40 changes: 40 additions & 0 deletions gax-java/gax/src/test/java/com/google/api/gax/util/TimeConversionUtilsTest.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -69,4 +69,44 @@ void testToThreetenTimeInstant_validInput_succeeds() {
jtInstant.toEpochMilli(), TimeConversionUtils.toThreetenInstant(jtInstant).toEpochMilli());
assertNull(TimeConversionUtils.toThreetenInstant(null));
}

@Test
void testToThreeteenInstant_bigInput_doesNotOverflow() {
// defaults to MAX_SECONDS plus the max value of long for the nanos part
Copy link
Contributor

Choose a reason for hiding this comment

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

I'm not too familiar with this. What do you mean by plus the max value of long for nanos?

I see that the MAX is 1000000000-12-31T23:59:59.999999999Z. Does this mean that this is essentially where jtInstant.getNano() and jtInstant.getEpochSecond() are both Interger.MAX_VALUE?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Instants and Durations have two components: the seconds part and the nanos part, both being long for Duration, and long for seconds + int for nanos in Instant.

Does this mean that this is essentially where jtInstant.getNano() and jtInstant.getEpochSecond() are both Interger.MAX_VALUE?

Yes, but with Long.MAX_VALUE for the seconds part instead.

Copy link
Contributor

Choose a reason for hiding this comment

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

Ok gotcha. I couldn't make sense of where the overflow was originally coming from since MAX_VALUE itself shouldn't trigger an overflow. If I'm understanding this correctly, it was triggered from the old way of how we were converting.

The toNanos() method was limited to Long.MAX_VALUE which has a limit of 292 years. Since Instant.MAX_VALUE has Long.MAX_VALUE for both seconds and nanos, it wouldn't be able to fit into toNanos() into a Long.MAX_VALUE as adding all the seconds + nanos would easily overfill.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

If I'm understanding this correctly, it was triggered from the old way of how we were converting.

Exactly.

it wouldn't be able to fit into toNanos() into a Long.MAX_VALUE as adding all the seconds + nanos would easily overfill

Yes. I don't think we will ever deal with durations instants that last longer than a few hours but it's good to support the theoretical limits of these classes, even because a user may just try a max value and that may break the library.

java.time.Instant jtInstant = java.time.Instant.MAX;
org.threeten.bp.Instant ttInstant = TimeConversionUtils.toThreetenInstant(jtInstant);
assertEquals(jtInstant.getEpochSecond(), ttInstant.getEpochSecond());
assertEquals(jtInstant.getNano(), ttInstant.getNano());
}

@Test
void testToJavaTimeInstant_bigInput_doesNotOverflow() {
// defaults to MAX_SECONDS plus the max value of long for the nanos part
org.threeten.bp.Instant ttInstant = org.threeten.bp.Instant.MAX;
java.time.Instant jtInstant = TimeConversionUtils.toJavaTimeInstant(ttInstant);
assertEquals(jtInstant.getEpochSecond(), ttInstant.getEpochSecond());
assertEquals(jtInstant.getNano(), ttInstant.getNano());
}

@Test
void testToThreeteenDuration_bigInput_doesNotOverflow() {
// we use the max long value for the seconds part and an arbitrary int for the nanos part, so we
// can confirm
// that both components are preserved
diegomarquezp marked this conversation as resolved.
Show resolved Hide resolved
java.time.Duration jtDuration = java.time.Duration.ofSeconds(Long.MAX_VALUE, 123);
Copy link
Contributor

Choose a reason for hiding this comment

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

qq, any specific reason for an arbitrary value of 123 and not Long/Integer.MAX_VALUE? Perhaps we use MAX_VALUE for consistency?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

No specific reason. I modified this to use the max possible nano value without overflowing the duration

org.threeten.bp.Duration ttDuration = TimeConversionUtils.toThreetenDuration(jtDuration);
assertEquals(jtDuration.getSeconds(), ttDuration.getSeconds());
assertEquals(jtDuration.getNano(), ttDuration.getNano());
}

@Test
void testToJavaTimeDuration_bigInput_doesNotOverflow() {
// we use the max long value for the seconds part and an arbitrary int for the nanos part, so we
// can confirm
// that both components are preserved
diegomarquezp marked this conversation as resolved.
Show resolved Hide resolved
org.threeten.bp.Duration ttDuration = org.threeten.bp.Duration.ofSeconds(Long.MAX_VALUE, 123);
java.time.Duration jtDuration = TimeConversionUtils.toJavaTimeDuration(ttDuration);
assertEquals(jtDuration.getSeconds(), ttDuration.getSeconds());
assertEquals(jtDuration.getNano(), ttDuration.getNano());
}
}
Loading