Skip to content

Commit 933fc4e

Browse files
robsdedudeinjectives
robsdedude
and
injectives
authored
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 <[email protected]> * Changed to Tx config builter `.withDefaultTimeout` + clean-up Co-authored-by: Dmitriy Tverdiakov <[email protected]> * Fix typos Co-authored-by: Dmitriy Tverdiakov <[email protected]>
1 parent 868f846 commit 933fc4e

File tree

6 files changed

+146
-34
lines changed

6 files changed

+146
-34
lines changed

‎driver/src/main/java/org/neo4j/driver/TransactionConfig.java

Lines changed: 15 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -189,25 +189,38 @@ private Builder()
189189

190190
/**
191191
* Set the transaction timeout. Transactions that execute longer than the configured timeout will be terminated by the database.
192+
* See also {@link #withDefaultTimeout}.
192193
* <p>
193194
* This functionality allows to limit query/transaction execution time. Specified timeout overrides the default timeout configured in the database
194195
* using {@code dbms.transaction.timeout} setting.
195196
* <p>
196-
* Provided value should not be {@code null} and should not represent a duration of zero or negative duration.
197+
* Provided value should not represent a negative duration.
197198
*
198199
* @param timeout the timeout.
199200
* @return this builder.
200201
*/
201202
public Builder withTimeout( Duration timeout )
202203
{
203204
requireNonNull( timeout, "Transaction timeout should not be null" );
204-
checkArgument( !timeout.isZero(), "Transaction timeout should not be zero" );
205205
checkArgument( !timeout.isNegative(), "Transaction timeout should not be negative" );
206206

207207
this.timeout = timeout;
208208
return this;
209209
}
210210

211+
/**
212+
* Set the transaction timeout to the server-side configured default timeout. This is the default behaviour if
213+
* {@link #withTimeout} has not been called.
214+
* See also {@link #withTimeout}.
215+
*
216+
* @return this builder.
217+
*/
218+
public Builder withDefaultTimeout()
219+
{
220+
this.timeout = null;
221+
return this;
222+
}
223+
211224
/**
212225
* Set the transaction metadata. Specified metadata will be attached to the executing transaction and visible in the output of
213226
* {@code dbms.listQueries} and {@code dbms.listTransactions} procedures. It will also get logged to the {@code query.log}.

‎driver/src/test/java/org/neo4j/driver/TransactionConfigTest.java

Lines changed: 24 additions & 15 deletions
Original file line numberDiff line numberDiff line change
@@ -24,10 +24,10 @@
2424
import java.util.HashMap;
2525
import java.util.Map;
2626

27+
import org.neo4j.driver.exceptions.ClientException;
2728
import org.neo4j.driver.internal.InternalNode;
2829
import org.neo4j.driver.internal.InternalPath;
2930
import org.neo4j.driver.internal.InternalRelationship;
30-
import org.neo4j.driver.exceptions.ClientException;
3131
import org.neo4j.driver.util.TestUtil;
3232

3333
import static java.util.Collections.emptyMap;
@@ -51,18 +51,6 @@ void emptyConfigShouldHaveNoMetadata()
5151
assertEquals( emptyMap(), TransactionConfig.empty().metadata() );
5252
}
5353

54-
@Test
55-
void shouldDisallowNullTimeout()
56-
{
57-
assertThrows( NullPointerException.class, () -> TransactionConfig.builder().withTimeout( null ) );
58-
}
59-
60-
@Test
61-
void shouldDisallowZeroTimeout()
62-
{
63-
assertThrows( IllegalArgumentException.class, () -> TransactionConfig.builder().withTimeout( Duration.ZERO ) );
64-
}
65-
6654
@Test
6755
void shouldDisallowNegativeTimeout()
6856
{
@@ -92,12 +80,33 @@ void shouldDisallowMetadataWithIllegalValues()
9280
void shouldHaveTimeout()
9381
{
9482
TransactionConfig config = TransactionConfig.builder()
95-
.withTimeout( Duration.ofSeconds( 3 ) )
96-
.build();
83+
.withTimeout( Duration.ofSeconds( 3 ) )
84+
.build();
9785

9886
assertEquals( Duration.ofSeconds( 3 ), config.timeout() );
9987
}
10088

89+
@Test
90+
void shouldAllowDefaultTimeout()
91+
{
92+
TransactionConfig config = TransactionConfig.builder()
93+
.withTimeout( Duration.ofSeconds( 3 ) )
94+
.withDefaultTimeout()
95+
.build();
96+
97+
assertNull( config.timeout() );
98+
}
99+
100+
@Test
101+
void shouldAllowZeroTimeout()
102+
{
103+
TransactionConfig config = TransactionConfig.builder()
104+
.withTimeout( Duration.ZERO )
105+
.build();
106+
107+
assertEquals( Duration.ZERO, config.timeout() );
108+
}
109+
101110
@Test
102111
void shouldHaveMetadata()
103112
{

‎testkit-backend/src/main/java/neo4j/org/testkit/backend/CustomDriverError.java

Lines changed: 27 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,27 @@
1+
/*
2+
* Copyright (c) "Neo4j"
3+
* Neo4j Sweden AB [http://neo4j.com]
4+
*
5+
* This file is part of Neo4j.
6+
*
7+
* Licensed under the Apache License, Version 2.0 (the "License");
8+
* you may not use this file except in compliance with the License.
9+
* You may obtain a copy of the License at
10+
*
11+
* http://www.apache.org/licenses/LICENSE-2.0
12+
*
13+
* Unless required by applicable law or agreed to in writing, software
14+
* distributed under the License is distributed on an "AS IS" BASIS,
15+
* WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
16+
* See the License for the specific language governing permissions and
17+
* limitations under the License.
18+
*/
19+
package neo4j.org.testkit.backend;
20+
21+
public class CustomDriverError extends RuntimeException
22+
{
23+
public CustomDriverError( Throwable cause )
24+
{
25+
super( cause );
26+
}
27+
}

‎testkit-backend/src/main/java/neo4j/org/testkit/backend/channel/handler/TestkitRequestProcessorHandler.java

Lines changed: 15 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -21,6 +21,7 @@
2121
import io.netty.channel.Channel;
2222
import io.netty.channel.ChannelHandlerContext;
2323
import io.netty.channel.ChannelInboundHandlerAdapter;
24+
import neo4j.org.testkit.backend.CustomDriverError;
2425
import neo4j.org.testkit.backend.TestkitState;
2526
import neo4j.org.testkit.backend.messages.requests.TestkitRequest;
2627
import neo4j.org.testkit.backend.messages.responses.BackendError;
@@ -145,6 +146,20 @@ else if ( isConnectionPoolClosedException( throwable ) || throwable instanceof U
145146
)
146147
.build();
147148
}
149+
else if ( throwable instanceof CustomDriverError )
150+
{
151+
throwable = throwable.getCause();
152+
String id = testkitState.newId();
153+
return DriverError.builder()
154+
.data(
155+
DriverError.DriverErrorBody.builder()
156+
.id( id )
157+
.errorType( throwable.getClass().getName() )
158+
.msg( throwable.getMessage() )
159+
.build()
160+
)
161+
.build();
162+
}
148163
else
149164
{
150165
return BackendError.builder().data( BackendError.BackendErrorBody.builder().msg( throwable.toString() ).build() ).build();

‎testkit-backend/src/main/java/neo4j/org/testkit/backend/messages/requests/SessionBeginTransaction.java

Lines changed: 33 additions & 12 deletions
Original file line numberDiff line numberDiff line change
@@ -20,6 +20,7 @@
2020

2121
import lombok.Getter;
2222
import lombok.Setter;
23+
import neo4j.org.testkit.backend.CustomDriverError;
2324
import neo4j.org.testkit.backend.TestkitState;
2425
import neo4j.org.testkit.backend.holder.AsyncTransactionHolder;
2526
import neo4j.org.testkit.backend.holder.RxTransactionHolder;
@@ -45,6 +46,28 @@ public class SessionBeginTransaction implements TestkitRequest
4546
{
4647
private SessionBeginTransactionBody data;
4748

49+
private void configureTimeout( TransactionConfig.Builder builder )
50+
{
51+
if ( data.getTimeoutPresent() )
52+
{
53+
try
54+
{
55+
if ( data.getTimeout() != null )
56+
{
57+
builder.withTimeout( Duration.ofMillis( data.getTimeout() ) );
58+
}
59+
else
60+
{
61+
builder.withDefaultTimeout();
62+
}
63+
}
64+
catch ( IllegalArgumentException e )
65+
{
66+
throw new CustomDriverError( e );
67+
}
68+
}
69+
}
70+
4871
@Override
4972
public TestkitResponse process( TestkitState testkitState )
5073
{
@@ -53,10 +76,7 @@ public TestkitResponse process( TestkitState testkitState )
5376
TransactionConfig.Builder builder = TransactionConfig.builder();
5477
Optional.ofNullable( data.txMeta ).ifPresent( builder::withMetadata );
5578

56-
if ( data.getTimeout() != null )
57-
{
58-
builder.withTimeout( Duration.ofMillis( data.getTimeout() ) );
59-
}
79+
configureTimeout( builder );
6080

6181
org.neo4j.driver.Transaction transaction = session.beginTransaction( builder.build() );
6282
return transaction( testkitState.addTransactionHolder( new TransactionHolder( sessionHolder, transaction ) ) );
@@ -72,10 +92,7 @@ public CompletionStage<TestkitResponse> processAsync( TestkitState testkitState
7292
TransactionConfig.Builder builder = TransactionConfig.builder();
7393
Optional.ofNullable( data.txMeta ).ifPresent( builder::withMetadata );
7494

75-
if ( data.getTimeout() != null )
76-
{
77-
builder.withTimeout( Duration.ofMillis( data.getTimeout() ) );
78-
}
95+
configureTimeout( builder );
7996

8097
return session.beginTransactionAsync( builder.build() ).thenApply( tx -> transaction(
8198
testkitState.addAsyncTransactionHolder( new AsyncTransactionHolder( sessionHolder, tx ) ) ) );
@@ -92,10 +109,7 @@ public Mono<TestkitResponse> processRx( TestkitState testkitState )
92109
TransactionConfig.Builder builder = TransactionConfig.builder();
93110
Optional.ofNullable( data.txMeta ).ifPresent( builder::withMetadata );
94111

95-
if ( data.getTimeout() != null )
96-
{
97-
builder.withTimeout( Duration.ofMillis( data.getTimeout() ) );
98-
}
112+
configureTimeout( builder );
99113

100114
return Mono.fromDirect( session.beginTransaction( builder.build() ) )
101115
.map( tx -> transaction(
@@ -115,5 +129,12 @@ public static class SessionBeginTransactionBody
115129
private String sessionId;
116130
private Map<String,Object> txMeta;
117131
private Integer timeout;
132+
private Boolean timeoutPresent = false;
133+
134+
public void setTimeout( Integer timeout )
135+
{
136+
this.timeout = timeout;
137+
timeoutPresent = true;
138+
}
118139
}
119140
}

‎testkit-backend/src/main/java/neo4j/org/testkit/backend/messages/requests/SessionRun.java

Lines changed: 32 additions & 5 deletions
Original file line numberDiff line numberDiff line change
@@ -21,6 +21,7 @@
2121
import com.fasterxml.jackson.databind.annotation.JsonDeserialize;
2222
import lombok.Getter;
2323
import lombok.Setter;
24+
import neo4j.org.testkit.backend.CustomDriverError;
2425
import neo4j.org.testkit.backend.TestkitState;
2526
import neo4j.org.testkit.backend.holder.ResultCursorHolder;
2627
import neo4j.org.testkit.backend.holder.ResultHolder;
@@ -50,6 +51,28 @@ public class SessionRun implements TestkitRequest
5051
{
5152
private SessionRunBody data;
5253

54+
private void configureTimeout( TransactionConfig.Builder builder )
55+
{
56+
if ( data.getTimeoutPresent() )
57+
{
58+
try
59+
{
60+
if ( data.getTimeout() != null )
61+
{
62+
builder.withTimeout( Duration.ofMillis( data.getTimeout() ) );
63+
}
64+
else
65+
{
66+
builder.withDefaultTimeout();
67+
}
68+
}
69+
catch ( IllegalArgumentException e )
70+
{
71+
throw new CustomDriverError( e );
72+
}
73+
}
74+
}
75+
5376
@Override
5477
public TestkitResponse process( TestkitState testkitState )
5578
{
@@ -60,7 +83,7 @@ public TestkitResponse process( TestkitState testkitState )
6083
.orElseGet( () -> new Query( data.cypher ) );
6184
TransactionConfig.Builder transactionConfig = TransactionConfig.builder();
6285
Optional.ofNullable( data.getTxMeta() ).ifPresent( transactionConfig::withMetadata );
63-
Optional.ofNullable( data.getTimeout() ).ifPresent( to -> transactionConfig.withTimeout( Duration.ofMillis( to ) ) );
86+
configureTimeout( transactionConfig );
6487
org.neo4j.driver.Result result = session.run( query, transactionConfig.build() );
6588
String id = testkitState.addResultHolder( new ResultHolder( sessionHolder, result ) );
6689

@@ -79,8 +102,7 @@ public CompletionStage<TestkitResponse> processAsync( TestkitState testkitState
79102
.orElseGet( () -> new Query( data.cypher ) );
80103
TransactionConfig.Builder transactionConfig = TransactionConfig.builder();
81104
Optional.ofNullable( data.getTxMeta() ).ifPresent( transactionConfig::withMetadata );
82-
Optional.ofNullable( data.getTimeout() )
83-
.ifPresent( to -> transactionConfig.withTimeout( Duration.ofMillis( to ) ) );
105+
configureTimeout( transactionConfig );
84106

85107
return session.runAsync( query, transactionConfig.build() )
86108
.thenApply( resultCursor ->
@@ -104,7 +126,7 @@ public Mono<TestkitResponse> processRx( TestkitState testkitState )
104126
.orElseGet( () -> new Query( data.cypher ) );
105127
TransactionConfig.Builder transactionConfig = TransactionConfig.builder();
106128
Optional.ofNullable( data.getTxMeta() ).ifPresent( transactionConfig::withMetadata );
107-
Optional.ofNullable( data.getTimeout() ).ifPresent( to -> transactionConfig.withTimeout( Duration.ofMillis( to ) ) );
129+
configureTimeout( transactionConfig );
108130

109131
RxResult result = session.run( query, transactionConfig.build() );
110132
String id = testkitState.addRxResultHolder( new RxResultHolder( sessionHolder, result ) );
@@ -127,11 +149,16 @@ public static class SessionRunBody
127149
{
128150
@JsonDeserialize( using = TestkitCypherParamDeserializer.class )
129151
private Map<String,Object> params;
130-
131152
private String sessionId;
132153
private String cypher;
133154
private Map<String,Object> txMeta;
134155
private Integer timeout;
156+
private Boolean timeoutPresent = false;
135157

158+
public void setTimeout( Integer timeout )
159+
{
160+
this.timeout = timeout;
161+
timeoutPresent = true;
162+
}
136163
}
137164
}

0 commit comments

Comments
 (0)