From c8c61c0de6894cd20f2d12698892ded579bb7839 Mon Sep 17 00:00:00 2001 From: Dmitriy Tverdiakov Date: Wed, 20 Jul 2022 15:53:10 +0100 Subject: [PATCH 1/2] Defer unknown zone information failures to datetime value access stage If server supplies zone information that is not supported by driver runtime, it will be represented by an unknown datetime value that fails on datetime access. This prevents an immediate failure during data consumption and allows usage of other values. --- .../src/main/java/org/neo4j/driver/Value.java | 6 +- .../messaging/common/CommonValueUnpacker.java | 59 ++++----- .../value/UnsupportedDateTimeValue.java | 85 +++++++++++++ .../value/UnsupportedDateTimeValueTest.java | 116 ++++++++++++++++++ .../backend/messages/requests/ResultNext.java | 3 + .../backend/messages/requests/StartTest.java | 6 - 6 files changed, 236 insertions(+), 39 deletions(-) create mode 100644 driver/src/main/java/org/neo4j/driver/internal/value/UnsupportedDateTimeValue.java create mode 100644 driver/src/test/java/org/neo4j/driver/internal/value/UnsupportedDateTimeValueTest.java diff --git a/driver/src/main/java/org/neo4j/driver/Value.java b/driver/src/main/java/org/neo4j/driver/Value.java index 1c6113f0d1..5631c2194a 100644 --- a/driver/src/main/java/org/neo4j/driver/Value.java +++ b/driver/src/main/java/org/neo4j/driver/Value.java @@ -18,6 +18,7 @@ */ package org.neo4j.driver; +import java.time.DateTimeException; import java.time.LocalDate; import java.time.LocalDateTime; import java.time.LocalTime; @@ -197,7 +198,8 @@ public interface Value extends MapAccessor, MapAccessorWithDefaultValue { * 64-bit precision. This is why these types return java {@link Long} and * {@link Double}, respectively. * - * @return the value as a Java Object + * @return the value as a Java Object. + * @throws DateTimeException if zone information supplied by server is not supported by driver runtime. Applicable to datetime values only. */ Object asObject(); @@ -416,12 +418,14 @@ public interface Value extends MapAccessor, MapAccessorWithDefaultValue { /** * @return the value as a {@link java.time.OffsetDateTime}, if possible. * @throws Uncoercible if value types are incompatible. + * @throws DateTimeException if zone information supplied by server is not supported by driver runtime. */ OffsetDateTime asOffsetDateTime(); /** * @return the value as a {@link ZonedDateTime}, if possible. * @throws Uncoercible if value types are incompatible. + * @throws DateTimeException if zone information supplied by server is not supported by driver runtime. */ ZonedDateTime asZonedDateTime(); diff --git a/driver/src/main/java/org/neo4j/driver/internal/messaging/common/CommonValueUnpacker.java b/driver/src/main/java/org/neo4j/driver/internal/messaging/common/CommonValueUnpacker.java index 439947a953..86ae0f5ac0 100644 --- a/driver/src/main/java/org/neo4j/driver/internal/messaging/common/CommonValueUnpacker.java +++ b/driver/src/main/java/org/neo4j/driver/internal/messaging/common/CommonValueUnpacker.java @@ -24,6 +24,7 @@ import static org.neo4j.driver.Values.value; import java.io.IOException; +import java.time.DateTimeException; import java.time.Instant; import java.time.LocalDate; import java.time.LocalDateTime; @@ -37,6 +38,7 @@ import java.util.Collections; import java.util.List; import java.util.Map; +import java.util.function.Supplier; import org.neo4j.driver.Value; import org.neo4j.driver.exceptions.ClientException; import org.neo4j.driver.exceptions.ProtocolException; @@ -54,6 +56,7 @@ import org.neo4j.driver.internal.value.NodeValue; import org.neo4j.driver.internal.value.PathValue; import org.neo4j.driver.internal.value.RelationshipValue; +import org.neo4j.driver.internal.value.UnsupportedDateTimeValue; import org.neo4j.driver.types.Node; import org.neo4j.driver.types.Path; import org.neo4j.driver.types.Relationship; @@ -189,28 +192,28 @@ private Value unpackStruct(long size, byte type) throws IOException { case DATE_TIME_WITH_ZONE_OFFSET: if (!dateTimeUtcEnabled) { ensureCorrectStructSize(TypeConstructor.DATE_TIME, DATE_TIME_STRUCT_SIZE, size); - return unpackDateTimeWithZoneOffset(); + return unpackDateTime(true, false); } else { throw instantiateExceptionForUnknownType(type); } case DATE_TIME_WITH_ZONE_OFFSET_UTC: if (dateTimeUtcEnabled) { ensureCorrectStructSize(TypeConstructor.DATE_TIME, DATE_TIME_STRUCT_SIZE, size); - return unpackDateTimeUtcWithZoneOffset(); + return unpackDateTime(true, true); } else { throw instantiateExceptionForUnknownType(type); } case DATE_TIME_WITH_ZONE_ID: if (!dateTimeUtcEnabled) { ensureCorrectStructSize(TypeConstructor.DATE_TIME, DATE_TIME_STRUCT_SIZE, size); - return unpackDateTimeWithZoneId(); + return unpackDateTime(false, false); } else { throw instantiateExceptionForUnknownType(type); } case DATE_TIME_WITH_ZONE_ID_UTC: if (dateTimeUtcEnabled) { ensureCorrectStructSize(TypeConstructor.DATE_TIME, DATE_TIME_STRUCT_SIZE, size); - return unpackDateTimeUtcWithZoneId(); + return unpackDateTime(false, true); } else { throw instantiateExceptionForUnknownType(type); } @@ -374,34 +377,26 @@ private Value unpackLocalDateTime() throws IOException { return value(LocalDateTime.ofEpochSecond(epochSecondUtc, nano, UTC)); } - private Value unpackDateTimeWithZoneOffset() throws IOException { - long epochSecondLocal = unpacker.unpackLong(); - int nano = Math.toIntExact(unpacker.unpackLong()); - int offsetSeconds = Math.toIntExact(unpacker.unpackLong()); - return value(newZonedDateTime(epochSecondLocal, nano, ZoneOffset.ofTotalSeconds(offsetSeconds))); - } - - private Value unpackDateTimeUtcWithZoneOffset() throws IOException { - long epochSecondLocal = unpacker.unpackLong(); - int nano = Math.toIntExact(unpacker.unpackLong()); - int offsetSeconds = Math.toIntExact(unpacker.unpackLong()); - ZoneOffset offset = ZoneOffset.ofTotalSeconds(offsetSeconds); - return value(newZonedDateTimeUsingUtcBaseline(epochSecondLocal, nano, offset)); - } - - private Value unpackDateTimeWithZoneId() throws IOException { - long epochSecondLocal = unpacker.unpackLong(); - int nano = Math.toIntExact(unpacker.unpackLong()); - String zoneIdString = unpacker.unpackString(); - return value(newZonedDateTime(epochSecondLocal, nano, ZoneId.of(zoneIdString))); - } - - private Value unpackDateTimeUtcWithZoneId() throws IOException { - long epochSecondLocal = unpacker.unpackLong(); - int nano = Math.toIntExact(unpacker.unpackLong()); - String zoneIdString = unpacker.unpackString(); - ZoneId zoneId = ZoneId.of(zoneIdString); - return value(newZonedDateTimeUsingUtcBaseline(epochSecondLocal, nano, zoneId)); + private Value unpackDateTime(boolean unpackOffset, boolean useUtcBaseline) throws IOException { + var epochSecondLocal = unpacker.unpackLong(); + var nano = Math.toIntExact(unpacker.unpackLong()); + Supplier zoneIdSupplier; + if (unpackOffset) { + var offsetSeconds = Math.toIntExact(unpacker.unpackLong()); + zoneIdSupplier = () -> ZoneOffset.ofTotalSeconds(offsetSeconds); + } else { + var zoneIdString = unpacker.unpackString(); + zoneIdSupplier = () -> ZoneId.of(zoneIdString); + } + ZoneId zoneId; + try { + zoneId = zoneIdSupplier.get(); + } catch (DateTimeException e) { + return new UnsupportedDateTimeValue(e); + } + return useUtcBaseline + ? value(newZonedDateTimeUsingUtcBaseline(epochSecondLocal, nano, zoneId)) + : value(newZonedDateTime(epochSecondLocal, nano, zoneId)); } private Value unpackDuration() throws IOException { diff --git a/driver/src/main/java/org/neo4j/driver/internal/value/UnsupportedDateTimeValue.java b/driver/src/main/java/org/neo4j/driver/internal/value/UnsupportedDateTimeValue.java new file mode 100644 index 0000000000..446cd0d3ca --- /dev/null +++ b/driver/src/main/java/org/neo4j/driver/internal/value/UnsupportedDateTimeValue.java @@ -0,0 +1,85 @@ +/* + * Copyright (c) "Neo4j" + * Neo4j Sweden AB [http://neo4j.com] + * + * This file is part of Neo4j. + * + * Licensed under the Apache License, Version 2.0 (the "License"); + * you may not use this file except in compliance with the License. + * You may obtain a copy of the License at + * + * http://www.apache.org/licenses/LICENSE-2.0 + * + * Unless required by applicable law or agreed to in writing, software + * distributed under the License is distributed on an "AS IS" BASIS, + * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. + * See the License for the specific language governing permissions and + * limitations under the License. + */ +package org.neo4j.driver.internal.value; + +import java.lang.reflect.InvocationTargetException; +import java.time.DateTimeException; +import java.time.OffsetDateTime; +import java.time.ZonedDateTime; +import org.neo4j.driver.internal.types.InternalTypeSystem; +import org.neo4j.driver.types.Type; + +public class UnsupportedDateTimeValue extends ValueAdapter { + final DateTimeException exception; + + public UnsupportedDateTimeValue(DateTimeException exception) { + this.exception = exception; + } + + @Override + public OffsetDateTime asOffsetDateTime() { + throw instantiateDateTimeException(); + } + + @Override + public ZonedDateTime asZonedDateTime() { + throw instantiateDateTimeException(); + } + + @Override + public Object asObject() { + throw instantiateDateTimeException(); + } + + @Override + public Type type() { + return InternalTypeSystem.TYPE_SYSTEM.DATE_TIME(); + } + + @Override + public boolean equals(Object obj) { + return this == obj; + } + + @Override + public int hashCode() { + return System.identityHashCode(this); + } + + @Override + public String toString() { + return "Unsupported datetime value."; + } + + private DateTimeException instantiateDateTimeException() { + DateTimeException newException; + try { + newException = exception + .getClass() + .getDeclaredConstructor(String.class, Throwable.class) + .newInstance(exception.getMessage(), exception); + } catch (NoSuchMethodException + | InvocationTargetException + | InstantiationException + | IllegalAccessException e) { + newException = new DateTimeException(exception.getMessage(), exception); + } + return newException; + } +} diff --git a/driver/src/test/java/org/neo4j/driver/internal/value/UnsupportedDateTimeValueTest.java b/driver/src/test/java/org/neo4j/driver/internal/value/UnsupportedDateTimeValueTest.java new file mode 100644 index 0000000000..5a088c0f69 --- /dev/null +++ b/driver/src/test/java/org/neo4j/driver/internal/value/UnsupportedDateTimeValueTest.java @@ -0,0 +1,116 @@ +/* + * Copyright (c) "Neo4j" + * Neo4j Sweden AB [http://neo4j.com] + * + * This file is part of Neo4j. + * + * Licensed under the Apache License, Version 2.0 (the "License"); + * you may not use this file except in compliance with the License. + * You may obtain a copy of the License at + * + * http://www.apache.org/licenses/LICENSE-2.0 + * + * Unless required by applicable law or agreed to in writing, software + * distributed under the License is distributed on an "AS IS" BASIS, + * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. + * See the License for the specific language governing permissions and + * limitations under the License. + */ +package org.neo4j.driver.internal.value; + +import static org.junit.jupiter.api.Assertions.assertEquals; +import static org.junit.jupiter.api.Assertions.assertNotEquals; +import static org.junit.jupiter.api.Assertions.assertThrows; +import static org.mockito.BDDMockito.given; +import static org.mockito.MockitoAnnotations.openMocks; + +import java.time.DateTimeException; +import java.time.OffsetDateTime; +import java.time.ZonedDateTime; +import java.util.List; +import java.util.function.Function; +import org.junit.jupiter.api.BeforeEach; +import org.junit.jupiter.api.Named; +import org.junit.jupiter.api.Test; +import org.junit.jupiter.params.ParameterizedTest; +import org.junit.jupiter.params.provider.Arguments; +import org.junit.jupiter.params.provider.MethodSource; +import org.mockito.Mock; +import org.neo4j.driver.internal.types.InternalTypeSystem; + +public class UnsupportedDateTimeValueTest { + @Mock + private DateTimeException exception; + + @BeforeEach + void beforeEach() { + openMocks(this); + } + + @MethodSource("throwingDateTimeAccessMethods") + @ParameterizedTest + void shouldThrowOnDateTimeAccess(Function throwingMethod) { + // GIVEN + given(exception.getMessage()).willReturn("message"); + var value = new UnsupportedDateTimeValue(exception); + + // WHEN + var actualException = assertThrows(DateTimeException.class, () -> throwingMethod.apply(value)); + + // THEN + assertEquals(actualException.getMessage(), exception.getMessage()); + assertEquals(actualException.getCause(), exception); + } + + static List throwingDateTimeAccessMethods() { + return List.of( + Arguments.of(Named.>of( + "asOffsetDateTime", UnsupportedDateTimeValue::asOffsetDateTime)), + Arguments.of(Named.>of( + "asOffsetDateTime(OffsetDateTime)", v -> v.asOffsetDateTime(OffsetDateTime.now()))), + Arguments.of(Named.>of( + "asZonedDateTime", UnsupportedDateTimeValue::asZonedDateTime)), + Arguments.of(Named.>of( + "asZonedDateTime(ZonedDateTime)", v -> v.asZonedDateTime(ZonedDateTime.now()))), + Arguments.of(Named.>of( + "asObject", UnsupportedDateTimeValue::asObject))); + } + + @Test + void shouldEqualToItself() { + // GIVEN + var value = new UnsupportedDateTimeValue(exception); + + // WHEN & THEN + assertEquals(value, value); + } + + @Test + void shouldNotEqualToAnotherInstance() { + // GIVEN + var value0 = new UnsupportedDateTimeValue(exception); + var value1 = new UnsupportedDateTimeValue(exception); + + // WHEN & THEN + assertNotEquals(value0, value1); + } + + @Test + void shouldSupplyIdentityHashcode() { + // GIVEN + var value0 = new UnsupportedDateTimeValue(exception); + var value1 = new UnsupportedDateTimeValue(exception); + + // WHEN & THEN + assertNotEquals(value0.hashCode(), value1.hashCode()); + } + + @Test + void shouldSupplyDateTimeType() { + // GIVEN + var value = new UnsupportedDateTimeValue(exception); + + // WHEN & THEN + assertEquals(InternalTypeSystem.TYPE_SYSTEM.DATE_TIME(), value.type()); + } +} diff --git a/testkit-backend/src/main/java/neo4j/org/testkit/backend/messages/requests/ResultNext.java b/testkit-backend/src/main/java/neo4j/org/testkit/backend/messages/requests/ResultNext.java index 7cd0151c71..7d8c2c07bb 100644 --- a/testkit-backend/src/main/java/neo4j/org/testkit/backend/messages/requests/ResultNext.java +++ b/testkit-backend/src/main/java/neo4j/org/testkit/backend/messages/requests/ResultNext.java @@ -22,14 +22,17 @@ import lombok.Setter; import neo4j.org.testkit.backend.messages.AbstractResultNext; import org.neo4j.driver.Record; +import org.neo4j.driver.Value; @Setter @Getter public class ResultNext extends AbstractResultNext { + private static final String DATE_TIME = "DATE_TIME"; private ResultNextBody data; @Override protected neo4j.org.testkit.backend.messages.responses.TestkitResponse createResponse(Record record) { + record.values().stream().filter(v -> DATE_TIME.equals(v.type().name())).forEach(Value::asObject); return neo4j.org.testkit.backend.messages.responses.Record.builder() .data(neo4j.org.testkit.backend.messages.responses.Record.RecordBody.builder() .values(record) diff --git a/testkit-backend/src/main/java/neo4j/org/testkit/backend/messages/requests/StartTest.java b/testkit-backend/src/main/java/neo4j/org/testkit/backend/messages/requests/StartTest.java index 7330919331..be346e3668 100644 --- a/testkit-backend/src/main/java/neo4j/org/testkit/backend/messages/requests/StartTest.java +++ b/testkit-backend/src/main/java/neo4j/org/testkit/backend/messages/requests/StartTest.java @@ -83,12 +83,6 @@ public class StartTest implements TestkitRequest { "^.*\\.TestOptimizations\\.test_uses_implicit_default_arguments_multi_query$", skipMessage); COMMON_SKIP_PATTERN_TO_REASON.put( "^.*\\.TestOptimizations\\.test_uses_implicit_default_arguments_multi_query_nested$", skipMessage); - COMMON_SKIP_PATTERN_TO_REASON.put( - "^.*\\.test_unknown_then_known_zoned_date_time(_patched)?$", - "Unknown zone names make the driver close the connection."); - COMMON_SKIP_PATTERN_TO_REASON.put( - "^.*\\.test_unknown_zoned_date_time(_patched)?$", - "Unknown zone names make the driver close the connection."); ASYNC_SKIP_PATTERN_TO_REASON.putAll(COMMON_SKIP_PATTERN_TO_REASON); From 23f3a1e131e941e47f2a14771e7666989682a035 Mon Sep 17 00:00:00 2001 From: Dmitriy Tverdiakov Date: Thu, 21 Jul 2022 12:08:40 +0100 Subject: [PATCH 2/2] Add enums for readability --- .../messaging/common/CommonValueUnpacker.java | 25 +++++++++++++------ 1 file changed, 17 insertions(+), 8 deletions(-) diff --git a/driver/src/main/java/org/neo4j/driver/internal/messaging/common/CommonValueUnpacker.java b/driver/src/main/java/org/neo4j/driver/internal/messaging/common/CommonValueUnpacker.java index 86ae0f5ac0..b625d497c4 100644 --- a/driver/src/main/java/org/neo4j/driver/internal/messaging/common/CommonValueUnpacker.java +++ b/driver/src/main/java/org/neo4j/driver/internal/messaging/common/CommonValueUnpacker.java @@ -62,7 +62,6 @@ import org.neo4j.driver.types.Relationship; public class CommonValueUnpacker implements ValueUnpacker { - public static final byte DATE = 'D'; public static final int DATE_STRUCT_SIZE = 1; @@ -192,28 +191,28 @@ private Value unpackStruct(long size, byte type) throws IOException { case DATE_TIME_WITH_ZONE_OFFSET: if (!dateTimeUtcEnabled) { ensureCorrectStructSize(TypeConstructor.DATE_TIME, DATE_TIME_STRUCT_SIZE, size); - return unpackDateTime(true, false); + return unpackDateTime(ZoneMode.OFFSET, BaselineMode.LEGACY); } else { throw instantiateExceptionForUnknownType(type); } case DATE_TIME_WITH_ZONE_OFFSET_UTC: if (dateTimeUtcEnabled) { ensureCorrectStructSize(TypeConstructor.DATE_TIME, DATE_TIME_STRUCT_SIZE, size); - return unpackDateTime(true, true); + return unpackDateTime(ZoneMode.OFFSET, BaselineMode.UTC); } else { throw instantiateExceptionForUnknownType(type); } case DATE_TIME_WITH_ZONE_ID: if (!dateTimeUtcEnabled) { ensureCorrectStructSize(TypeConstructor.DATE_TIME, DATE_TIME_STRUCT_SIZE, size); - return unpackDateTime(false, false); + return unpackDateTime(ZoneMode.ZONE_ID, BaselineMode.LEGACY); } else { throw instantiateExceptionForUnknownType(type); } case DATE_TIME_WITH_ZONE_ID_UTC: if (dateTimeUtcEnabled) { ensureCorrectStructSize(TypeConstructor.DATE_TIME, DATE_TIME_STRUCT_SIZE, size); - return unpackDateTime(false, true); + return unpackDateTime(ZoneMode.ZONE_ID, BaselineMode.UTC); } else { throw instantiateExceptionForUnknownType(type); } @@ -377,11 +376,11 @@ private Value unpackLocalDateTime() throws IOException { return value(LocalDateTime.ofEpochSecond(epochSecondUtc, nano, UTC)); } - private Value unpackDateTime(boolean unpackOffset, boolean useUtcBaseline) throws IOException { + private Value unpackDateTime(ZoneMode unpackOffset, BaselineMode useUtcBaseline) throws IOException { var epochSecondLocal = unpacker.unpackLong(); var nano = Math.toIntExact(unpacker.unpackLong()); Supplier zoneIdSupplier; - if (unpackOffset) { + if (unpackOffset == ZoneMode.OFFSET) { var offsetSeconds = Math.toIntExact(unpacker.unpackLong()); zoneIdSupplier = () -> ZoneOffset.ofTotalSeconds(offsetSeconds); } else { @@ -394,7 +393,7 @@ private Value unpackDateTime(boolean unpackOffset, boolean useUtcBaseline) throw } catch (DateTimeException e) { return new UnsupportedDateTimeValue(e); } - return useUtcBaseline + return useUtcBaseline == BaselineMode.UTC ? value(newZonedDateTimeUsingUtcBaseline(epochSecondLocal, nano, zoneId)) : value(newZonedDateTime(epochSecondLocal, nano, zoneId)); } @@ -445,4 +444,14 @@ protected int getNodeFields() { protected int getRelationshipFields() { return RELATIONSHIP_FIELDS; } + + private enum ZoneMode { + OFFSET, + ZONE_ID + } + + private enum BaselineMode { + UTC, + LEGACY + } }