From ab8f5ea54c64098c575ed61dcd8e2e31f1b0a11e Mon Sep 17 00:00:00 2001 From: David Turner Date: Fri, 2 Feb 2018 18:24:02 +0000 Subject: [PATCH] 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. --- .../forbidden/es-server-signatures.txt | 53 +++++++++++++++++++ .../elasticsearch/ingest/IngestDocument.java | 6 +-- .../ingest/IngestDocumentTests.java | 18 +++++++ 3 files changed, 73 insertions(+), 4 deletions(-) diff --git a/buildSrc/src/main/resources/forbidden/es-server-signatures.txt b/buildSrc/src/main/resources/forbidden/es-server-signatures.txt index 6507f05be5c..89179350174 100644 --- a/buildSrc/src/main/resources/forbidden/es-server-signatures.txt +++ b/buildSrc/src/main/resources/forbidden/es-server-signatures.txt @@ -82,3 +82,56 @@ org.joda.time.DateTime#(int, int, int, int, int, int) org.joda.time.DateTime#(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) diff --git a/server/src/main/java/org/elasticsearch/ingest/IngestDocument.java b/server/src/main/java/org/elasticsearch/ingest/IngestDocument.java index 7edbccef5aa..b35ac567f9e 100644 --- a/server/src/main/java/org/elasticsearch/ingest/IngestDocument.java +++ b/server/src/main/java/org/elasticsearch/ingest/IngestDocument.java @@ -609,13 +609,11 @@ public final class IngestDocument { return Arrays.copyOf(bytes, bytes.length); } else if (value == null || value instanceof String || value instanceof Integer || value instanceof Long || value instanceof Float || - value instanceof Double || value instanceof Boolean) { + value instanceof Double || value instanceof Boolean || + value instanceof ZonedDateTime) { return value; } else if (value instanceof Date) { return ((Date) value).clone(); - } else if (value instanceof ZonedDateTime) { - ZonedDateTime zonedDateTime = (ZonedDateTime) value; - return ZonedDateTime.of(zonedDateTime.toLocalDate(), zonedDateTime.toLocalTime(), zonedDateTime.getZone()); } else { throw new IllegalArgumentException("unexpected value type [" + value.getClass() + "]"); } diff --git a/server/src/test/java/org/elasticsearch/ingest/IngestDocumentTests.java b/server/src/test/java/org/elasticsearch/ingest/IngestDocumentTests.java index 02627d131d7..9df2a38c6f1 100644 --- a/server/src/test/java/org/elasticsearch/ingest/IngestDocumentTests.java +++ b/server/src/test/java/org/elasticsearch/ingest/IngestDocumentTests.java @@ -22,6 +22,8 @@ package org.elasticsearch.ingest; import org.elasticsearch.test.ESTestCase; import org.junit.Before; +import java.time.Instant; +import java.time.ZoneId; import java.time.ZoneOffset; import java.time.ZonedDateTime; import java.util.ArrayList; @@ -986,6 +988,22 @@ public class IngestDocumentTests extends ESTestCase { assertIngestDocument(ingestDocument, copy); } + public void testCopyConstructorWithZonedDateTime() { + ZoneId timezone = ZoneId.of("Europe/London"); + + Map sourceAndMetadata = new HashMap<>(); + sourceAndMetadata.put("beforeClockChange", ZonedDateTime.ofInstant(Instant.ofEpochSecond(1509237000), timezone)); + sourceAndMetadata.put("afterClockChange", ZonedDateTime.ofInstant(Instant.ofEpochSecond(1509240600), timezone)); + + IngestDocument original = new IngestDocument(sourceAndMetadata, new HashMap<>()); + IngestDocument copy = new IngestDocument(original); + + assertThat(copy.getSourceAndMetadata().get("beforeClockChange"), + equalTo(original.getSourceAndMetadata().get("beforeClockChange"))); + assertThat(copy.getSourceAndMetadata().get("afterClockChange"), + equalTo(original.getSourceAndMetadata().get("afterClockChange"))); + } + public void testSetInvalidSourceField() throws Exception { Map document = new HashMap<>(); Object randomObject = randomFrom(new ArrayList<>(), new HashMap<>(), 12, 12.34);