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

Add support for nanosecond precision when parsing rfc3339 strings #752

Merged
merged 5 commits into from
Jul 31, 2019

Conversation

olavloite
Copy link
Contributor

The original parseRfc3339 method of DateTime returns a DateTime instance that has millisecond precision. The parse method therefore also disregards any time information beyond millisecond precision. For Spanner we would like to add nanosecond precision, and instead of replicating the parse method in the Spanner specific libraries, this change will add nanosecond support to this parse method, while retaining the original behavior of the parseRfc3339 method.

The original parseRfc3339 method allows strings to contain more than millisecond precision. It does however not specify exactly what it does with any additional information, and the behavior is not consistent:

  • The input value 2018-12-31T23:59:59.9999Z would be truncated to 2018-12-31T23:59:59.999Z
  • The input value 2018-12-31T23:59:59.999999999Z would be rounded up to 2019-01-01T00:00:00Z

After this change the method will always truncate any time information beyond millisecond precision when returning a DateTime instance.

@olavloite olavloite requested a review from kolea2 July 25, 2019 11:21
@olavloite olavloite requested a review from a team as a code owner July 25, 2019 11:21
@googlebot googlebot added the cla: yes This human has signed the Contributor License Agreement. label Jul 25, 2019
Copy link
Contributor

@elharo elharo left a comment

Choose a reason for hiding this comment

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

No comment on the design yet, but adding public API normally suggests incrementing the minor version. In this case, I think it would need to bump to 1.4.0 in the pom.xml.

@chingor13 chingor13 added the semver: minor A new feature was added. No breaking changes. label Jul 25, 2019
@olavloite
Copy link
Contributor Author

No comment on the design yet, but adding public API normally suggests incrementing the minor version. In this case, I think it would need to bump to 1.4.0 in the pom.xml.

Shouldn't that be 1.31.0?

@elharo
Copy link
Contributor

elharo commented Jul 26, 2019

You're right. It should be 1.31.0 I get confused when version numbers hit double digits. :-)

}

/** A timestamp represented as the number of seconds and nanoseconds since Epoch. */
public static final class SecondsAndNanos implements Serializable {
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 sure what our plans are for Java 8, but if we could use java.time I think we could avoid introducing this class and associated public API. @chingor13 has any final decision been made on that?

Copy link
Collaborator

Choose a reason for hiding this comment

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

This library is very heavily used on android, so even if we move google-cloud-java to Java 8, we'd likely avoid using new features for a long time. java.time was introduced in Android at API level 26 (Oreo).

* </pre>
*/
public void testParseRfc3339ToSecondsAndNanos() {
Map<String, SecondsAndNanos> map = new HashMap<>();
Copy link
Collaborator

Choose a reason for hiding this comment

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

nit: it seems odd to collect these into a map to just iterate over them. We could implement a helper test assertion like assertParsedRdc3339(String, SecondsAndNanos)

Copy link
Contributor Author

Choose a reason for hiding this comment

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

That's a good point. I've changed the test case accordingly.

@kolea2 kolea2 merged commit 1d496dc into googleapis:master Jul 31, 2019
clundin25 pushed a commit to clundin25/google-http-java-client that referenced this pull request Aug 11, 2022
clundin25 pushed a commit to clundin25/google-http-java-client that referenced this pull request Aug 11, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
cla: yes This human has signed the Contributor License Agreement. semver: minor A new feature was added. No breaking changes.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants