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

Forbid trappy methods from java.time #28476

Conversation

Projects
None yet
5 participants
@DaveCTurner
Copy link
Contributor

commented Feb 1, 2018

java.time has the functionality needed to deal with timezones with varying offsets correctly, but it also has a bunch of methods that silently let you forget about the hard cases, which raises the risk that we'll quietly do the wrong thing at some point in the future.

This change adds (what I think to be) the trappy methods to the list of forbidden methods to try and help stop this from happening.

There was only one use of these methods in the codebase so far: IngestDocument#deepCopy() used ZonedDateTime.of() which may alter the offset of the given time in cases where the offset is ambiguous.

@@ -615,7 +615,7 @@ private static Object deepCopy(Object value) {
return ((Date) value).clone();
} else if (value instanceof ZonedDateTime) {
ZonedDateTime zonedDateTime = (ZonedDateTime) value;
return ZonedDateTime.of(zonedDateTime.toLocalDate(), zonedDateTime.toLocalTime(), zonedDateTime.getZone());
return ZonedDateTime.ofInstant(zonedDateTime.toLocalDateTime(), zonedDateTime.getOffset(), zonedDateTime.getZone());

This comment has been minimized.

Copy link
@DaveCTurner

DaveCTurner Feb 1, 2018

Author Contributor

TBH I'm wondering whether this could just use zonedDateTime directly, since it is supposed to have value semantics. Put differently, why not also deep-copy the LocalDateTime and ZoneId and so on?

This comment has been minimized.

Copy link
@talevy

talevy Feb 1, 2018

Contributor

I'm not sure I understand. does it matter?

This comment has been minimized.

Copy link
@DaveCTurner

DaveCTurner Feb 2, 2018

Author Contributor

It does but I should have added a test to show this. It only very rarely matters, which is why it's trappy. I added a test.

This comment has been minimized.

Copy link
@DaveCTurner

DaveCTurner Feb 2, 2018

Author Contributor

Hmm, ok, I forgot I'd made that original comment, but I think I was answering a different question in my previous reply.

The question is why not simply this:

else if (value instanceof ZonedDateTime) {
    return value;

I thought this would be ok because ZonedDateTime is immutable and we shouldn't be looking at reference equality on it. If that's not fine, because we do want the contents of the copy to share no references with the original for some reason, then what we do here doesn't do that since it makes a shallow copy of the ZonedDateTime, because it copies the LocalDateTime instance within.

This comment has been minimized.

Copy link
@spinscale

spinscale Feb 2, 2018

Member

++ on that

@talevy

talevy approved these changes Feb 1, 2018

Copy link
Contributor

left a comment

LGTM.

thanks for looking at this, I'm not entirely sure about the repercussions of your code comment, but the current changes look reasonable enough?

@spinscale
Copy link
Member

left a comment

LGTM, thanks for doing that! Just returning the ZonedDateTime looks good to me as well, but I might be missing why it was added in the first place (didnt check history)

@DaveCTurner

This comment has been minimized.

Copy link
Contributor Author

commented Feb 2, 2018

Ok, there was already a case for value types like String, Integer, ..., so I just added ZonedDateTime to that list.

@DaveCTurner DaveCTurner merged commit ab8f5ea into elastic:master Feb 2, 2018

2 checks passed

CLA Commit author has signed the CLA
Details
elasticsearch-ci Build finished.
Details

@DaveCTurner DaveCTurner deleted the DaveCTurner:2018-02-01-java-time-forbid-trappy-methods branch Feb 2, 2018

DaveCTurner added a commit that referenced this pull request Feb 2, 2018

Forbid trappy methods from java.time (#28476)
ava.time has the functionality needed to deal with timezones with varying 
offsets correctly, but it also has a bunch of methods that silently let you
forget about the hard cases, which raises the risk that we'll quietly do the
wrong thing at some point in the future.

This change adds the trappy methods to the list of forbidden methods to try and
help stop this from happening.

It also fixes the only use of these methods in the codebase so far:
IngestDocument#deepCopy() used ZonedDateTime.of() which may alter the offset of
the given time in cases where the offset is ambiguous.

@colings86 colings86 added v7.0.0-beta1 and removed v7.0.0 labels Feb 7, 2019

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
You can’t perform that action at this time.