Skip to content

Commit

Permalink
Only normalize OffsetDateTimes when they are normalizable
Browse files Browse the repository at this point in the history 
Fix for FasterXML#166. This commit introduces static constants for the minimum and the maximum OffsetDateTimes that are adjustable to all possible time zones in InstantDeserializer and adds a check that an OffsetDateTime lies in these boundaries before it is adjusted.
  • Loading branch information
@morth
morth committed May 16, 2020
1 parent 5669145 commit 1761f54
Show file tree
Hide file tree
Showing 2 changed files with 102 additions and 1 deletion.
30 changes: 29 additions & 1 deletion datetime/src/main/java/com/fasterxml/jackson/datatype/jsr310/deser/InstantDeserializer.java
View file
Original file line number Diff line number Diff line change
Expand Up @@ -59,6 +59,18 @@ public class InstantDeserializer<T extends Temporal>
*/
private static final Pattern ISO8601_UTC_ZERO_OFFSET_SUFFIX_REGEX = Pattern.compile("\\+00:?(00)?$");

/**
* Constant that defines the lower bound for {@link OffsetDateTime}s being adjustable to any
* other time zone. See [jackson-modules-java8#166].
*/
private static final OffsetDateTime OFFSET_DATE_TIME_MIN_ADJ = OffsetDateTime.MIN.plusHours(36);

/**
* Constant that defines the upper bound for {@link OffsetDateTime}s being adjustable to any
* other time zone. See [jackson-modules-java8#166].
*/
private static final OffsetDateTime OFFSET_DATE_TIME_MAX_ADJ = OffsetDateTime.MAX.minusHours(36);

public static final InstantDeserializer<Instant> INSTANT = new InstantDeserializer<>(
Instant.class, DateTimeFormatter.ISO_INSTANT,
Instant::from,
Expand All @@ -73,7 +85,7 @@ public class InstantDeserializer<T extends Temporal>
OffsetDateTime::from,
a -> OffsetDateTime.ofInstant(Instant.ofEpochMilli(a.value), a.zoneId),
a -> OffsetDateTime.ofInstant(Instant.ofEpochSecond(a.integer, a.fraction), a.zoneId),
(d, z) -> d.withOffsetSameInstant(z.getRules().getOffset(d.toLocalDateTime())),
(d, z) -> isAdjustable(d) ? d.withOffsetSameInstant(z.getRules().getOffset(d.toLocalDateTime())) : d,
true // yes, replace zero offset with Z
);

Expand Down Expand Up @@ -326,6 +338,22 @@ private String replaceZeroOffsetAsZIfNecessary(String text)
return text;
}

/**
* Checks whether an {@link OffsetDateTime} is adjustable to any other time offset. We can
* safely regard an {@link OffsetDateTime} as adjustable to all time zones when it's geq than
* OffsetDateTime.MIN + 36 hours and leq than OffsetDateTime.MAX - 36 hours.
* See [jackson-modules-java8#166].
*
* @param d The {@link OffsetDateTime} that should be adjusted
* @return true, when the {@link OffsetDateTime} can be adjusted to any other time zone, false else
*/
private static boolean isAdjustable(OffsetDateTime d)
{
return (d.isAfter(OFFSET_DATE_TIME_MIN_ADJ) && d.isBefore(OFFSET_DATE_TIME_MAX_ADJ))
|| d.isEqual(OFFSET_DATE_TIME_MIN_ADJ)

This comment has been minimized.

Sign in to view

Copy link
@kupci

kupci May 17, 2020 •
edited
Loading

In trying to think of a way around the 4 checks, and then realizing for majority of the time, the logic would short-circuit to only two checks, and also in keeping with @cowtowncoders point about preferring the simpler solution from the two originally proposed, wouldn't it be even simpler to just check MAX and MIN?

(d, z) -> {
               return (d.isEqual(OffsetDateTime.MIN) || d.isEqual(OffsetDateTime.MAX) ? d : d.withOffsetSameInstant(z.getRules().getOffset(d.toLocalDateTime())));
           },

Here's the argument: If someone sets a date, and they have the ADJUST_DATES_TO_CONTEXT_TIME_ZONE set, then Jackson Java 8 will strive to meet that requirement, unless the date is set to a constant, like OffsetDateTime.MIN or OffsetDateTime.MAX. That would seem to keep the documentation* simpler, not only the logic. This change could safely go in 2.11.1, since it is fixing a bug, but with the normalization (i.e. MIN/MAX adjusted), would be a little safer to go in 2.12.

Wondering** about the change from the 18 hour offset (original changes) to 36 hours with the new change set? Could there even be a case where the timezone isn't adjusted when it should be, with the 36 hour range? Found this in the documentation:

In 2008, time-zone offsets around the world extended from -12:00 to +14:00. To prevent any problems with that range being extended, yet still provide validation, the range of offsets is restricted to -18:00 to 18:00 inclusive.
https://docs.oracle.com/javase/8/docs/api/java/time/ZoneOffset.html

In summary, while I'd lean towards a more fine-tuned fix, I don't have a super-strongly held position on this. @cowtowncoder for the tie-breaker?

*Since this looks close to being ready (pending questions above), the documentation on the ADJUST_DATES_TO_CONTEXT_TIME_ZONE should be updated to reflect the new logic. This is one more plus for keeping the logic simpler, in that it's easier to explain in the documentation. If the user has a requirement to put in dates really close to the MIN/MAX of OffsetDateTime, then they'll need to set the ADJUST_DATES_TO_CONTEXT_TIME_ZONE flag to false.

** I missed your explanation here, just went through my e-mail and see you explained it:

This would lead to a check whether it is geq than OffsetDateTime.MIN + 36 hours and leq than OffsetDateTime.MAX - 36 hours. This would be the fastest solution. The edge cases here are normalizations to -18 for date times near OffsetDateTime.MIN and to +18 for date times near OffsetDateTime.MAX.
|| d.isEqual(OFFSET_DATE_TIME_MAX_ADJ);
}

public static class FromIntegerArguments // since 2.8.3
{
public final long value;
Expand Down
73 changes: 73 additions & 0 deletions ...me/src/test/java/com/fasterxml/jackson/datatype/jsr310/deser/OffsetDateTimeDeserTest.java
View file
Original file line number Diff line number Diff line change
Expand Up @@ -614,6 +614,79 @@ public void testRoundTripOfOffsetDateTimeAndJavaUtilDate() throws Exception
assertEquals(givenInstant.atOffset(ZoneOffset.UTC), actual);
}

/*
* Tests for the deserialization of OffsetDateTimes that cannot be
* normalized with ADJUST_DATES_TO_CONTEXT_TIME_ZONE enabled. The expected
* behaviour is that normalization is skipped for those OffsetDateTimes
* that cannot be normalized. See [jackson-modules-java8#166].
*/

@Test
public void testDeserializationOfOffsetDateTimeMin() throws Exception
{
OffsetDateTime date = OffsetDateTime.MIN;
OffsetDateTime value = MAPPER.readerFor(OffsetDateTime.class)
.with(DeserializationFeature.ADJUST_DATES_TO_CONTEXT_TIME_ZONE)
.readValue('"' + FORMATTER.format(date) + '"');
assertIsEqual(date, value);
assertNotEquals("The time zone has been normalized.", ZoneOffset.UTC, value.getOffset());
}

@Test
public void testDeserializationOfUnadjustableOffsetDateTimeNearMin() throws Exception
{
OffsetDateTime date = OffsetDateTime.MIN.plusHours(36).minusNanos(1);
OffsetDateTime value = MAPPER.readerFor(OffsetDateTime.class)
.with(DeserializationFeature.ADJUST_DATES_TO_CONTEXT_TIME_ZONE)
.readValue('"' + FORMATTER.format(date) + '"');
assertIsEqual(date, value);
assertNotEquals("The time zone has been normalized.", ZoneOffset.UTC, value.getOffset());
}

@Test
public void testDeserializationOfAdjustableOffsetDateTimeNearMin() throws Exception
{
OffsetDateTime date = OffsetDateTime.MIN.plusHours(36);
OffsetDateTime value = MAPPER.readerFor(OffsetDateTime.class)
.with(DeserializationFeature.ADJUST_DATES_TO_CONTEXT_TIME_ZONE)
.readValue('"' + FORMATTER.format(date) + '"');
assertIsEqual(date, value);
assertEquals("The time zone is not correct.", ZoneOffset.UTC, value.getOffset());
}

@Test
public void testDeserializationOfOffsetDateTimeMax() throws Exception
{
OffsetDateTime date = OffsetDateTime.MAX;
OffsetDateTime value = MAPPER.readerFor(OffsetDateTime.class)
.with(DeserializationFeature.ADJUST_DATES_TO_CONTEXT_TIME_ZONE)
.readValue('"' + FORMATTER.format(date) + '"');
assertIsEqual(date, value);
assertNotEquals("The time zone has been normalized.", ZoneOffset.UTC, value.getOffset());
}

@Test
public void testDeserializationOfUnadjustableOffsetDateTimeNearMax() throws Exception
{
OffsetDateTime date = OffsetDateTime.MAX.minusHours(36).plusNanos(1);
OffsetDateTime value = MAPPER.readerFor(OffsetDateTime.class)
.with(DeserializationFeature.ADJUST_DATES_TO_CONTEXT_TIME_ZONE)
.readValue('"' + FORMATTER.format(date) + '"');
assertIsEqual(date, value);
assertNotEquals("The time zone has been normalized.", ZoneOffset.UTC, value.getOffset());
}

@Test
public void testDeserializationOfAdjustableOffsetDateTimeNearMax() throws Exception
{
OffsetDateTime date = OffsetDateTime.MAX.minusHours(36);
OffsetDateTime value = MAPPER.readerFor(OffsetDateTime.class)
.with(DeserializationFeature.ADJUST_DATES_TO_CONTEXT_TIME_ZONE)
.readValue('"' + FORMATTER.format(date) + '"');
assertIsEqual(date, value);
assertEquals("The time zone is not correct.", ZoneOffset.UTC, value.getOffset());
}

/*
/**********************************************************
/* Tests for empty string handling
Expand Down

0 comments on commit 1761f54

Please sign in to comment.