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

Forbid trappy methods from java.time #28476

Merged
merged 6 commits into from Feb 2, 2018
Merged

Forbid trappy methods from java.time #28476

Show file tree
Hide file tree
Changes from 2 commits
Commits
Show all changes
6 commits
Select commit Hold shift + click to select a range
94a4b3f
Forbid trappy-looking methods in java.time
DaveCTurner Feb 1, 2018
b5a536d
Preserve offset of (ZonedDateTime) value in IngestDocument#deepCopy()
DaveCTurner Feb 1, 2018
3b2a672
Add test showing problem in IngestDocument#deepCopy()
DaveCTurner Feb 2, 2018
838dfac
Treat ZonedDateTime as a value type
DaveCTurner Feb 2, 2018
f88b92e
Merge branch 'master' into 2018-02-01-java-time-forbid-trappy-methods
DaveCTurner Feb 2, 2018
e28be62
Merge branch 'master' into 2018-02-01-java-time-forbid-trappy-methods
DaveCTurner Feb 2, 2018
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Jump to
Jump to file
Failed to load files.
Diff view
Diff view
53 changes: 53 additions & 0 deletions buildSrc/src/main/resources/forbidden/es-server-signatures.txt
View file
Open in desktop
Expand Up @@ -82,3 +82,56 @@ org.joda.time.DateTime#<init>(int, int, int, int, int, int)
org.joda.time.DateTime#<init>(int, int, int, int, int, int, int)
org.joda.time.DateTime#now()
org.joda.time.DateTimeZone#getDefault()

@defaultMessage Local times may be ambiguous or nonexistent in a specific time zones. Use ZoneRules#getValidOffsets() instead.
java.time.LocalDateTime#atZone(java.time.ZoneId)
java.time.ZonedDateTime#of(int, int, int, int, int, int, int, java.time.ZoneId)
java.time.ZonedDateTime#of(java.time.LocalDate, java.time.LocalTime, java.time.ZoneId)
java.time.ZonedDateTime#of(java.time.LocalDateTime, java.time.ZoneId)
java.time.ZonedDateTime#truncatedTo(java.time.temporal.TemporalUnit)
java.time.ZonedDateTime#of(int, int, int, int, int, int, int, java.time.ZoneId)
java.time.ZonedDateTime#of(java.time.LocalDate, java.time.LocalTime, java.time.ZoneId)
java.time.ZonedDateTime#of(java.time.LocalDateTime, java.time.ZoneId)
java.time.ZonedDateTime#ofLocal(java.time.LocalDateTime, java.time.ZoneId, java.time.ZoneOffset)
java.time.OffsetDateTime#atZoneSimilarLocal(java.time.ZoneId)
java.time.zone.ZoneRules#getOffset(java.time.LocalDateTime)

@defaultMessage Manipulation of an OffsetDateTime may yield a time that is not valid in the desired time zone. Use ZonedDateTime instead.
java.time.OffsetDateTime#minus(long, java.time.temporal.TemporalUnit)
java.time.OffsetDateTime#minus(long, java.time.temporal.TemporalUnit)
java.time.OffsetDateTime#minus(java.time.temporal.TemporalAmount)
java.time.OffsetDateTime#minusDays(long)
java.time.OffsetDateTime#minusHours(long)
java.time.OffsetDateTime#minusMinutes(long)
java.time.OffsetDateTime#minusMonths(long)
java.time.OffsetDateTime#minusNanos(long)
java.time.OffsetDateTime#minusSeconds(long)
java.time.OffsetDateTime#minusWeeks(long)
java.time.OffsetDateTime#minusYears(long)
java.time.OffsetDateTime#plus(long, java.time.temporal.TemporalUnit)
java.time.OffsetDateTime#plus(java.time.temporal.TemporalAmount)
java.time.OffsetDateTime#plusDays(long)
java.time.OffsetDateTime#plusHours(long)
java.time.OffsetDateTime#plusMinutes(long)
java.time.OffsetDateTime#plusMonths(long)
java.time.OffsetDateTime#plusNanos(long)
java.time.OffsetDateTime#plusSeconds(long)
java.time.OffsetDateTime#plusWeeks(long)
java.time.OffsetDateTime#plusYears(long)
java.time.OffsetDateTime#with(java.time.temporal.TemporalAdjuster)
java.time.OffsetDateTime#with(java.time.temporal.TemporalField, long)
java.time.OffsetDateTime#withDayOfMonth(int)
java.time.OffsetDateTime#withDayOfYear(int)
java.time.OffsetDateTime#withHour(int)
java.time.OffsetDateTime#withMinute(int)
java.time.OffsetDateTime#withMonth(int)
java.time.OffsetDateTime#withNano(int)
java.time.OffsetDateTime#withOffsetSameInstant(java.time.ZoneOffset)
java.time.OffsetDateTime#withOffsetSameLocal(java.time.ZoneOffset)
java.time.OffsetDateTime#withSecond(int)
java.time.OffsetDateTime#withYear(int)

@defaultMessage Daylight saving is not the only reason for a change in timezone offset.
java.time.zone.ZoneRules#getStandardOffset(java.time.Instant)
java.time.zone.ZoneRules#getDaylightSavings(java.time.Instant)
java.time.zone.ZoneRules#isDaylightSavings(java.time.Instant)
2 changes: 1 addition & 1 deletion server/src/main/java/org/elasticsearch/ingest/IngestDocument.java
View file
Open in desktop
Expand Up @@ -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());
Copy link
Contributor Author

Choose a reason for hiding this comment

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

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?

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 I understand. does it matter?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

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.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

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.

Copy link
Contributor

Choose a reason for hiding this comment

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

++ on that

} else {
throw new IllegalArgumentException("unexpected value type [" + value.getClass() + "]");
}
Expand Down