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

merged 5 commits into from
Aug 29, 2024

Conversation

diegomarquezp
Copy link
Contributor

@product-auto-label product-auto-label bot added the size: s Pull request size is small. label Aug 12, 2024
@diegomarquezp diegomarquezp marked this pull request as ready for review August 12, 2024 15:17
@diegomarquezp diegomarquezp requested a review from lqiu96 August 12, 2024 17:04
lqiu96
lqiu96 reviewed Aug 12, 2024
View reviewed changes
gax-java/gax/src/test/java/com/google/api/gax/util/TimeConversionUtilsTest.java

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

lqiu96
lqiu96 reviewed Aug 13, 2024
View reviewed changes
gax-java/gax/src/test/java/com/google/api/gax/util/TimeConversionUtilsTest.java Outdated
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
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

lqiu96
lqiu96 approved these changes Aug 13, 2024
View reviewed changes
Copy link
Contributor

@lqiu96 lqiu96 left a comment

Choose a reason for hiding this comment

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

LGTM added a small question/comment

@sonarqubecloud SonarQubeCloud
Copy link

Quality Gate Passed Quality Gate passed for 'gapic-generator-java-root'

Issues
9 New issues
0 Accepted issues

Measures
0 Security Hotspots
100.0% Coverage on New Code
0.0% Duplication on New Code

See analysis details on SonarCloud

@sonarqubecloud SonarQubeCloud
Copy link

Quality Gate Failed Quality Gate failed for 'java_showcase_integration_tests'

Failed conditions
50.0% Coverage on New Code (required ≥ 80%)

See analysis details on SonarCloud

@diegomarquezp diegomarquezp merged commit 699074e into main Aug 29, 2024
48 of 49 checks passed
@diegomarquezp diegomarquezp deleted the fix-time-conversion-precision branch August 29, 2024 15:47
@release-please release-please bot mentioned this pull request Aug 29, 2024
Merged
ldetmer added a commit that referenced this pull request Sep 9, 2024
@release-please @ldetmer
chore(main): release 2.45.0 (#3139)
3c05e8f 
🤖 I have created a release *beep* *boop*
---


<details><summary>2.45.0</summary>

##
[2.45.0](v2.44.0...v2.45.0)
(2024-09-09)


### Features

* add Batcher#close(timeout) and Batcher#cancelOutstanding
([#3141](#3141))
([b5a92e4](b5a92e4))
* add full RetrySettings sample code to Settings classes
([#3056](#3056))
([8fe3a2d](8fe3a2d))
* add toString to futures returned by operations
([#3140](#3140))
([afecb8c](afecb8c))
* bake gapic-generator-java into the hermetic build docker image
([#3067](#3067))
([a372e82](a372e82))


### Bug Fixes

* **gax:** prevent truncation/overflow when converting time values
([#3095](#3095))
([699074e](699074e))


### Dependencies

* add opentelemetry exporter-metrics and shared-resoucemapping to shared
dependencies
([#3078](#3078))
([fc8d80d](fc8d80d))
* update dependency certifi to v2024.8.30
([#3150](#3150))
([c18b705](c18b705))
* update dependency com.google.api-client:google-api-client-bom to
v2.7.0
([#3151](#3151))
([5f43e43](5f43e43))
* update dependency com.google.errorprone:error_prone_annotations to
v2.31.0
([#3153](#3153))
([3071509](3071509))
* update dependency com.google.errorprone:error_prone_annotations to
v2.31.0
([#3154](#3154))
([335ee63](335ee63))
* update dependency com.google.guava:guava to v33.3.0-jre
([#3119](#3119))
([41174b0](41174b0))
* update dependency dev.cel:cel to v0.7.1
([#3155](#3155))
([b1ddd16](b1ddd16))
* update dependency filelock to v3.16.0
([#3175](#3175))
([6681113](6681113))
* update dependency idna to v3.8
([#3156](#3156))
([82f5326](82f5326))
* update dependency io.netty:netty-tcnative-boringssl-static to
v2.0.66.final
([#3148](#3148))
([a7efaa8](a7efaa8))
* update dependency net.bytebuddy:byte-buddy to v1.15.1
([#3115](#3115))
([0e06c5f](0e06c5f))
* update dependency org.apache.commons:commons-lang3 to v3.17.0
([#3157](#3157))
([8d3b9fd](8d3b9fd))
* update dependency org.checkerframework:checker-qual to v3.47.0
([#3166](#3166))
([365674d](365674d))
* update dependency org.yaml:snakeyaml to v2.3
([#3158](#3158))
([e67ea9a](e67ea9a))
* update dependency platformdirs to v4.3.2
([#3176](#3176))
([4f2f9e0](4f2f9e0))
* update dependency virtualenv to v20.26.4
([#3177](#3177))
([080e078](080e078))
* update google api dependencies
([#3118](#3118))
([67342ea](67342ea))
* update google auth library dependencies to v1.25.0
([#3168](#3168))
([715884a](715884a))
* update google http client dependencies to v1.45.0
([#3159](#3159))
([a3fe612](a3fe612))
* update googleapis/java-cloud-bom digest to 6626f91
([#3147](#3147))
([658e40e](658e40e))
* update junit5 monorepo to v5.11.0
([#3111](#3111))
([6bf84c8](6bf84c8))
* update netty dependencies to v4.1.113.final
([#3165](#3165))
([9b5957d](9b5957d))
* update opentelemetry-java monorepo to v1.42.0
([#3172](#3172))
([413c44e](413c44e))


### Documentation

* Update DEVELOPMENT.md
([#3126](#3126))
([92bdf4e](92bdf4e))
</details>

---
This PR was generated with [Release
Please](https://github.com/googleapis/release-please). See
[documentation](https://github.com/googleapis/release-please#release-please).

Co-authored-by: release-please[bot] <55107282+release-please[bot]@users.noreply.github.com>
Co-authored-by: ldetmer <[email protected]>
ldetmer pushed a commit that referenced this pull request Sep 17, 2024
@diegomarquezp @lqiu96 @ldetmer
fix(gax): prevent truncation/overflow when converting time values (#3095
d7e5a02 
)

#1872 (comment)

---------

Co-authored-by: Lawrence Qiu <[email protected]>
ldetmer added a commit that referenced this pull request Sep 17, 2024
@release-please @ldetmer
chore(main): release 2.45.0 (#3139)
b0b3a6d 
🤖 I have created a release *beep* *boop*
---


<details><summary>2.45.0</summary>

##
[2.45.0](v2.44.0...v2.45.0)
(2024-09-09)


### Features

* add Batcher#close(timeout) and Batcher#cancelOutstanding
([#3141](#3141))
([b5a92e4](b5a92e4))
* add full RetrySettings sample code to Settings classes
([#3056](#3056))
([8fe3a2d](8fe3a2d))
* add toString to futures returned by operations
([#3140](#3140))
([afecb8c](afecb8c))
* bake gapic-generator-java into the hermetic build docker image
([#3067](#3067))
([a372e82](a372e82))


### Bug Fixes

* **gax:** prevent truncation/overflow when converting time values
([#3095](#3095))
([699074e](699074e))


### Dependencies

* add opentelemetry exporter-metrics and shared-resoucemapping to shared
dependencies
([#3078](#3078))
([fc8d80d](fc8d80d))
* update dependency certifi to v2024.8.30
([#3150](#3150))
([c18b705](c18b705))
* update dependency com.google.api-client:google-api-client-bom to
v2.7.0
([#3151](#3151))
([5f43e43](5f43e43))
* update dependency com.google.errorprone:error_prone_annotations to
v2.31.0
([#3153](#3153))
([3071509](3071509))
* update dependency com.google.errorprone:error_prone_annotations to
v2.31.0
([#3154](#3154))
([335ee63](335ee63))
* update dependency com.google.guava:guava to v33.3.0-jre
([#3119](#3119))
([41174b0](41174b0))
* update dependency dev.cel:cel to v0.7.1
([#3155](#3155))
([b1ddd16](b1ddd16))
* update dependency filelock to v3.16.0
([#3175](#3175))
([6681113](6681113))
* update dependency idna to v3.8
([#3156](#3156))
([82f5326](82f5326))
* update dependency io.netty:netty-tcnative-boringssl-static to
v2.0.66.final
([#3148](#3148))
([a7efaa8](a7efaa8))
* update dependency net.bytebuddy:byte-buddy to v1.15.1
([#3115](#3115))
([0e06c5f](0e06c5f))
* update dependency org.apache.commons:commons-lang3 to v3.17.0
([#3157](#3157))
([8d3b9fd](8d3b9fd))
* update dependency org.checkerframework:checker-qual to v3.47.0
([#3166](#3166))
([365674d](365674d))
* update dependency org.yaml:snakeyaml to v2.3
([#3158](#3158))
([e67ea9a](e67ea9a))
* update dependency platformdirs to v4.3.2
([#3176](#3176))
([4f2f9e0](4f2f9e0))
* update dependency virtualenv to v20.26.4
([#3177](#3177))
([080e078](080e078))
* update google api dependencies
([#3118](#3118))
([67342ea](67342ea))
* update google auth library dependencies to v1.25.0
([#3168](#3168))
([715884a](715884a))
* update google http client dependencies to v1.45.0
([#3159](#3159))
([a3fe612](a3fe612))
* update googleapis/java-cloud-bom digest to 6626f91
([#3147](#3147))
([658e40e](658e40e))
* update junit5 monorepo to v5.11.0
([#3111](#3111))
([6bf84c8](6bf84c8))
* update netty dependencies to v4.1.113.final
([#3165](#3165))
([9b5957d](9b5957d))
* update opentelemetry-java monorepo to v1.42.0
([#3172](#3172))
([413c44e](413c44e))


### Documentation

* Update DEVELOPMENT.md
([#3126](#3126))
([92bdf4e](92bdf4e))
</details>

---
This PR was generated with [Release
Please](https://github.com/googleapis/release-please). See
[documentation](https://github.com/googleapis/release-please#release-please).

Co-authored-by: release-please[bot] <55107282+release-please[bot]@users.noreply.github.com>
Co-authored-by: ldetmer <[email protected]>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@lqiu96 lqiu96 lqiu96 approved these changes

@blakeli0 blakeli0 blakeli0 approved these changes

Assignees
No one assigned
Labels
size: s Pull request size is small.
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
3 participants
@diegomarquezp @blakeli0 @lqiu96