From 933fc4eee3665027daf3f15ea4140f6648e75da8 Mon Sep 17 00:00:00 2001 From: Robsdedude Date: Tue, 18 Jan 2022 15:06:55 +0100 Subject: [PATCH] Allow tx timeout to be 0 or null. (#1108) * Allow tx timeout to be 0 or null. Adjust TestKit back end to accept * `null` timeout: send `null` to server (or omit as it's the default) * and integer: configure as timeout in milliseconds * omitted timeout: don't explicitly configure the timeout (user driver default) * Adjust unit tests * Add header * Code review suggestions by Dmitriy Co-authored-by: Dmitriy Tverdiakov * Changed to Tx config builter `.withDefaultTimeout` + clean-up Co-authored-by: Dmitriy Tverdiakov * Fix typos Co-authored-by: Dmitriy Tverdiakov --- .../org/neo4j/driver/TransactionConfig.java | 17 ++++++- .../neo4j/driver/TransactionConfigTest.java | 39 +++++++++------- .../testkit/backend/CustomDriverError.java | 27 +++++++++++ .../TestkitRequestProcessorHandler.java | 15 +++++++ .../requests/SessionBeginTransaction.java | 45 ++++++++++++++----- .../backend/messages/requests/SessionRun.java | 37 ++++++++++++--- 6 files changed, 146 insertions(+), 34 deletions(-) create mode 100644 testkit-backend/src/main/java/neo4j/org/testkit/backend/CustomDriverError.java diff --git a/driver/src/main/java/org/neo4j/driver/TransactionConfig.java b/driver/src/main/java/org/neo4j/driver/TransactionConfig.java index fee9943fb3..251f1a6071 100644 --- a/driver/src/main/java/org/neo4j/driver/TransactionConfig.java +++ b/driver/src/main/java/org/neo4j/driver/TransactionConfig.java @@ -189,11 +189,12 @@ private Builder() /** * Set the transaction timeout. Transactions that execute longer than the configured timeout will be terminated by the database. + * See also {@link #withDefaultTimeout}. *

* This functionality allows to limit query/transaction execution time. Specified timeout overrides the default timeout configured in the database * using {@code dbms.transaction.timeout} setting. *

- * Provided value should not be {@code null} and should not represent a duration of zero or negative duration. + * Provided value should not represent a negative duration. * * @param timeout the timeout. * @return this builder. @@ -201,13 +202,25 @@ private Builder() public Builder withTimeout( Duration timeout ) { requireNonNull( timeout, "Transaction timeout should not be null" ); - checkArgument( !timeout.isZero(), "Transaction timeout should not be zero" ); checkArgument( !timeout.isNegative(), "Transaction timeout should not be negative" ); this.timeout = timeout; return this; } + /** + * Set the transaction timeout to the server-side configured default timeout. This is the default behaviour if + * {@link #withTimeout} has not been called. + * See also {@link #withTimeout}. + * + * @return this builder. + */ + public Builder withDefaultTimeout() + { + this.timeout = null; + return this; + } + /** * Set the transaction metadata. Specified metadata will be attached to the executing transaction and visible in the output of * {@code dbms.listQueries} and {@code dbms.listTransactions} procedures. It will also get logged to the {@code query.log}. diff --git a/driver/src/test/java/org/neo4j/driver/TransactionConfigTest.java b/driver/src/test/java/org/neo4j/driver/TransactionConfigTest.java index 31740bdb82..aebb5bac20 100644 --- a/driver/src/test/java/org/neo4j/driver/TransactionConfigTest.java +++ b/driver/src/test/java/org/neo4j/driver/TransactionConfigTest.java @@ -24,10 +24,10 @@ import java.util.HashMap; import java.util.Map; +import org.neo4j.driver.exceptions.ClientException; import org.neo4j.driver.internal.InternalNode; import org.neo4j.driver.internal.InternalPath; import org.neo4j.driver.internal.InternalRelationship; -import org.neo4j.driver.exceptions.ClientException; import org.neo4j.driver.util.TestUtil; import static java.util.Collections.emptyMap; @@ -51,18 +51,6 @@ void emptyConfigShouldHaveNoMetadata() assertEquals( emptyMap(), TransactionConfig.empty().metadata() ); } - @Test - void shouldDisallowNullTimeout() - { - assertThrows( NullPointerException.class, () -> TransactionConfig.builder().withTimeout( null ) ); - } - - @Test - void shouldDisallowZeroTimeout() - { - assertThrows( IllegalArgumentException.class, () -> TransactionConfig.builder().withTimeout( Duration.ZERO ) ); - } - @Test void shouldDisallowNegativeTimeout() { @@ -92,12 +80,33 @@ void shouldDisallowMetadataWithIllegalValues() void shouldHaveTimeout() { TransactionConfig config = TransactionConfig.builder() - .withTimeout( Duration.ofSeconds( 3 ) ) - .build(); + .withTimeout( Duration.ofSeconds( 3 ) ) + .build(); assertEquals( Duration.ofSeconds( 3 ), config.timeout() ); } + @Test + void shouldAllowDefaultTimeout() + { + TransactionConfig config = TransactionConfig.builder() + .withTimeout( Duration.ofSeconds( 3 ) ) + .withDefaultTimeout() + .build(); + + assertNull( config.timeout() ); + } + + @Test + void shouldAllowZeroTimeout() + { + TransactionConfig config = TransactionConfig.builder() + .withTimeout( Duration.ZERO ) + .build(); + + assertEquals( Duration.ZERO, config.timeout() ); + } + @Test void shouldHaveMetadata() { diff --git a/testkit-backend/src/main/java/neo4j/org/testkit/backend/CustomDriverError.java b/testkit-backend/src/main/java/neo4j/org/testkit/backend/CustomDriverError.java new file mode 100644 index 0000000000..3110446d6f --- /dev/null +++ b/testkit-backend/src/main/java/neo4j/org/testkit/backend/CustomDriverError.java @@ -0,0 +1,27 @@ +/* + * 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 neo4j.org.testkit.backend; + +public class CustomDriverError extends RuntimeException +{ + public CustomDriverError( Throwable cause ) + { + super( cause ); + } +} diff --git a/testkit-backend/src/main/java/neo4j/org/testkit/backend/channel/handler/TestkitRequestProcessorHandler.java b/testkit-backend/src/main/java/neo4j/org/testkit/backend/channel/handler/TestkitRequestProcessorHandler.java index 3d2ce940d0..5ea2d22b4e 100644 --- a/testkit-backend/src/main/java/neo4j/org/testkit/backend/channel/handler/TestkitRequestProcessorHandler.java +++ b/testkit-backend/src/main/java/neo4j/org/testkit/backend/channel/handler/TestkitRequestProcessorHandler.java @@ -21,6 +21,7 @@ import io.netty.channel.Channel; import io.netty.channel.ChannelHandlerContext; import io.netty.channel.ChannelInboundHandlerAdapter; +import neo4j.org.testkit.backend.CustomDriverError; import neo4j.org.testkit.backend.TestkitState; import neo4j.org.testkit.backend.messages.requests.TestkitRequest; import neo4j.org.testkit.backend.messages.responses.BackendError; @@ -145,6 +146,20 @@ else if ( isConnectionPoolClosedException( throwable ) || throwable instanceof U ) .build(); } + else if ( throwable instanceof CustomDriverError ) + { + throwable = throwable.getCause(); + String id = testkitState.newId(); + return DriverError.builder() + .data( + DriverError.DriverErrorBody.builder() + .id( id ) + .errorType( throwable.getClass().getName() ) + .msg( throwable.getMessage() ) + .build() + ) + .build(); + } else { return BackendError.builder().data( BackendError.BackendErrorBody.builder().msg( throwable.toString() ).build() ).build(); diff --git a/testkit-backend/src/main/java/neo4j/org/testkit/backend/messages/requests/SessionBeginTransaction.java b/testkit-backend/src/main/java/neo4j/org/testkit/backend/messages/requests/SessionBeginTransaction.java index 0b10511fa2..89c99424e6 100644 --- a/testkit-backend/src/main/java/neo4j/org/testkit/backend/messages/requests/SessionBeginTransaction.java +++ b/testkit-backend/src/main/java/neo4j/org/testkit/backend/messages/requests/SessionBeginTransaction.java @@ -20,6 +20,7 @@ import lombok.Getter; import lombok.Setter; +import neo4j.org.testkit.backend.CustomDriverError; import neo4j.org.testkit.backend.TestkitState; import neo4j.org.testkit.backend.holder.AsyncTransactionHolder; import neo4j.org.testkit.backend.holder.RxTransactionHolder; @@ -45,6 +46,28 @@ public class SessionBeginTransaction implements TestkitRequest { private SessionBeginTransactionBody data; + private void configureTimeout( TransactionConfig.Builder builder ) + { + if ( data.getTimeoutPresent() ) + { + try + { + if ( data.getTimeout() != null ) + { + builder.withTimeout( Duration.ofMillis( data.getTimeout() ) ); + } + else + { + builder.withDefaultTimeout(); + } + } + catch ( IllegalArgumentException e ) + { + throw new CustomDriverError( e ); + } + } + } + @Override public TestkitResponse process( TestkitState testkitState ) { @@ -53,10 +76,7 @@ public TestkitResponse process( TestkitState testkitState ) TransactionConfig.Builder builder = TransactionConfig.builder(); Optional.ofNullable( data.txMeta ).ifPresent( builder::withMetadata ); - if ( data.getTimeout() != null ) - { - builder.withTimeout( Duration.ofMillis( data.getTimeout() ) ); - } + configureTimeout( builder ); org.neo4j.driver.Transaction transaction = session.beginTransaction( builder.build() ); return transaction( testkitState.addTransactionHolder( new TransactionHolder( sessionHolder, transaction ) ) ); @@ -72,10 +92,7 @@ public CompletionStage processAsync( TestkitState testkitState TransactionConfig.Builder builder = TransactionConfig.builder(); Optional.ofNullable( data.txMeta ).ifPresent( builder::withMetadata ); - if ( data.getTimeout() != null ) - { - builder.withTimeout( Duration.ofMillis( data.getTimeout() ) ); - } + configureTimeout( builder ); return session.beginTransactionAsync( builder.build() ).thenApply( tx -> transaction( testkitState.addAsyncTransactionHolder( new AsyncTransactionHolder( sessionHolder, tx ) ) ) ); @@ -92,10 +109,7 @@ public Mono processRx( TestkitState testkitState ) TransactionConfig.Builder builder = TransactionConfig.builder(); Optional.ofNullable( data.txMeta ).ifPresent( builder::withMetadata ); - if ( data.getTimeout() != null ) - { - builder.withTimeout( Duration.ofMillis( data.getTimeout() ) ); - } + configureTimeout( builder ); return Mono.fromDirect( session.beginTransaction( builder.build() ) ) .map( tx -> transaction( @@ -115,5 +129,12 @@ public static class SessionBeginTransactionBody private String sessionId; private Map txMeta; private Integer timeout; + private Boolean timeoutPresent = false; + + public void setTimeout( Integer timeout ) + { + this.timeout = timeout; + timeoutPresent = true; + } } } diff --git a/testkit-backend/src/main/java/neo4j/org/testkit/backend/messages/requests/SessionRun.java b/testkit-backend/src/main/java/neo4j/org/testkit/backend/messages/requests/SessionRun.java index 37436f5d65..c9dab2b7ea 100644 --- a/testkit-backend/src/main/java/neo4j/org/testkit/backend/messages/requests/SessionRun.java +++ b/testkit-backend/src/main/java/neo4j/org/testkit/backend/messages/requests/SessionRun.java @@ -21,6 +21,7 @@ import com.fasterxml.jackson.databind.annotation.JsonDeserialize; import lombok.Getter; import lombok.Setter; +import neo4j.org.testkit.backend.CustomDriverError; import neo4j.org.testkit.backend.TestkitState; import neo4j.org.testkit.backend.holder.ResultCursorHolder; import neo4j.org.testkit.backend.holder.ResultHolder; @@ -50,6 +51,28 @@ public class SessionRun implements TestkitRequest { private SessionRunBody data; + private void configureTimeout( TransactionConfig.Builder builder ) + { + if ( data.getTimeoutPresent() ) + { + try + { + if ( data.getTimeout() != null ) + { + builder.withTimeout( Duration.ofMillis( data.getTimeout() ) ); + } + else + { + builder.withDefaultTimeout(); + } + } + catch ( IllegalArgumentException e ) + { + throw new CustomDriverError( e ); + } + } + } + @Override public TestkitResponse process( TestkitState testkitState ) { @@ -60,7 +83,7 @@ public TestkitResponse process( TestkitState testkitState ) .orElseGet( () -> new Query( data.cypher ) ); TransactionConfig.Builder transactionConfig = TransactionConfig.builder(); Optional.ofNullable( data.getTxMeta() ).ifPresent( transactionConfig::withMetadata ); - Optional.ofNullable( data.getTimeout() ).ifPresent( to -> transactionConfig.withTimeout( Duration.ofMillis( to ) ) ); + configureTimeout( transactionConfig ); org.neo4j.driver.Result result = session.run( query, transactionConfig.build() ); String id = testkitState.addResultHolder( new ResultHolder( sessionHolder, result ) ); @@ -79,8 +102,7 @@ public CompletionStage processAsync( TestkitState testkitState .orElseGet( () -> new Query( data.cypher ) ); TransactionConfig.Builder transactionConfig = TransactionConfig.builder(); Optional.ofNullable( data.getTxMeta() ).ifPresent( transactionConfig::withMetadata ); - Optional.ofNullable( data.getTimeout() ) - .ifPresent( to -> transactionConfig.withTimeout( Duration.ofMillis( to ) ) ); + configureTimeout( transactionConfig ); return session.runAsync( query, transactionConfig.build() ) .thenApply( resultCursor -> @@ -104,7 +126,7 @@ public Mono processRx( TestkitState testkitState ) .orElseGet( () -> new Query( data.cypher ) ); TransactionConfig.Builder transactionConfig = TransactionConfig.builder(); Optional.ofNullable( data.getTxMeta() ).ifPresent( transactionConfig::withMetadata ); - Optional.ofNullable( data.getTimeout() ).ifPresent( to -> transactionConfig.withTimeout( Duration.ofMillis( to ) ) ); + configureTimeout( transactionConfig ); RxResult result = session.run( query, transactionConfig.build() ); String id = testkitState.addRxResultHolder( new RxResultHolder( sessionHolder, result ) ); @@ -127,11 +149,16 @@ public static class SessionRunBody { @JsonDeserialize( using = TestkitCypherParamDeserializer.class ) private Map params; - private String sessionId; private String cypher; private Map txMeta; private Integer timeout; + private Boolean timeoutPresent = false; + public void setTimeout( Integer timeout ) + { + this.timeout = timeout; + timeoutPresent = true; + } } }