Skip to content

Commit 4658aa6

Browse files
injectivesinjectives
authored
Abort discovery on bookmark failures and continue on authorization expired error (#1043)
This update ensures that discovery gets aborted on `ClientException` with the following codes: - `Neo.ClientError.Transaction.InvalidBookmark` - `Neo.ClientError.Transaction.InvalidBookmarkMixture` In addition, it makes sure that it continues on `AuthorizationExpiredException`. All security exceptions are mapped to `SecurityException`.
1 parent 45bebf2 commit 4658aa6

File tree

4 files changed

+135
-22
lines changed

4 files changed

+135
-22
lines changed

‎driver/src/main/java/org/neo4j/driver/internal/cluster/RediscoveryImpl.java

Lines changed: 30 additions & 3 deletions
Original file line numberDiff line numberDiff line change
@@ -34,6 +34,8 @@
3434
import org.neo4j.driver.Bookmark;
3535
import org.neo4j.driver.Logger;
3636
import org.neo4j.driver.Logging;
37+
import org.neo4j.driver.exceptions.AuthorizationExpiredException;
38+
import org.neo4j.driver.exceptions.ClientException;
3739
import org.neo4j.driver.exceptions.DiscoveryException;
3840
import org.neo4j.driver.exceptions.FatalDiscoveryException;
3941
import org.neo4j.driver.exceptions.SecurityException;
@@ -61,6 +63,8 @@ public class RediscoveryImpl implements Rediscovery
6163
private static final String RECOVERABLE_DISCOVERY_ERROR_WITH_SERVER = "Received a recoverable discovery error with server '%s', " +
6264
"will continue discovery with other routing servers if available. " +
6365
"Complete failure is reported separately from this entry.";
66+
private static final String INVALID_BOOKMARK_CODE = "Neo.ClientError.Transaction.InvalidBookmark";
67+
private static final String INVALID_BOOKMARK_MIXTURE_CODE = "Neo.ClientError.Transaction.InvalidBookmarkMixture";
6468

6569
private final BoltServerAddress initialRouter;
6670
private final RoutingSettings settings;
@@ -278,10 +282,8 @@ private CompletionStage<ClusterComposition> lookupOnRouter( BoltServerAddress ro
278282
private ClusterComposition handleRoutingProcedureError( Throwable error, RoutingTable routingTable,
279283
BoltServerAddress routerAddress, Throwable baseError )
280284
{
281-
if ( error instanceof SecurityException || error instanceof FatalDiscoveryException ||
282-
(error instanceof IllegalStateException && ConnectionPool.CONNECTION_POOL_CLOSED_ERROR_MESSAGE.equals( error.getMessage() )) )
285+
if ( mustAbortDiscovery( error ) )
283286
{
284-
// auth error or routing error happened, terminate the discovery procedure immediately
285287
throw new CompletionException( error );
286288
}
287289

@@ -295,6 +297,31 @@ private ClusterComposition handleRoutingProcedureError( Throwable error, Routing
295297
return null;
296298
}
297299

300+
private boolean mustAbortDiscovery( Throwable throwable )
301+
{
302+
boolean abort = false;
303+
304+
if ( !(throwable instanceof AuthorizationExpiredException) && throwable instanceof SecurityException )
305+
{
306+
abort = true;
307+
}
308+
else if ( throwable instanceof FatalDiscoveryException )
309+
{
310+
abort = true;
311+
}
312+
else if ( throwable instanceof IllegalStateException && ConnectionPool.CONNECTION_POOL_CLOSED_ERROR_MESSAGE.equals( throwable.getMessage() ) )
313+
{
314+
abort = true;
315+
}
316+
else if ( throwable instanceof ClientException )
317+
{
318+
String code = ((ClientException) throwable).code();
319+
abort = INVALID_BOOKMARK_CODE.equals( code ) || INVALID_BOOKMARK_MIXTURE_CODE.equals( code );
320+
}
321+
322+
return abort;
323+
}
324+
298325
@Override
299326
public List<BoltServerAddress> resolve() throws UnknownHostException
300327
{

‎driver/src/main/java/org/neo4j/driver/internal/util/ErrorUtil.java

Lines changed: 38 additions & 18 deletions
Original file line numberDiff line numberDiff line change
@@ -31,6 +31,7 @@
3131
import org.neo4j.driver.exceptions.FatalDiscoveryException;
3232
import org.neo4j.driver.exceptions.Neo4jException;
3333
import org.neo4j.driver.exceptions.ResultConsumedException;
34+
import org.neo4j.driver.exceptions.SecurityException;
3435
import org.neo4j.driver.exceptions.ServiceUnavailableException;
3536
import org.neo4j.driver.exceptions.TokenExpiredException;
3637
import org.neo4j.driver.exceptions.TransientException;
@@ -65,29 +66,38 @@ public static ResultConsumedException newResultConsumedError()
6566

6667
public static Neo4jException newNeo4jError( String code, String message )
6768
{
68-
String classification = extractClassification( code );
69-
switch ( classification )
69+
switch ( extractErrorClass( code ) )
7070
{
7171
case "ClientError":
72-
if ( code.equalsIgnoreCase( "Neo.ClientError.Security.Unauthorized" ) )
72+
if ( "Security".equals( extractErrorSubClass( code ) ) )
7373
{
74-
return new AuthenticationException( code, message );
75-
}
76-
else if ( code.equalsIgnoreCase( "Neo.ClientError.Database.DatabaseNotFound" ) )
77-
{
78-
return new FatalDiscoveryException( code, message );
79-
}
80-
else if ( code.equalsIgnoreCase( "Neo.ClientError.Security.AuthorizationExpired" ) )
81-
{
82-
return new AuthorizationExpiredException( code, message );
83-
}
84-
else if ( code.equalsIgnoreCase( "Neo.ClientError.Security.TokenExpired" ) )
85-
{
86-
return new TokenExpiredException( code, message );
74+
if ( code.equalsIgnoreCase( "Neo.ClientError.Security.Unauthorized" ) )
75+
{
76+
return new AuthenticationException( code, message );
77+
}
78+
else if ( code.equalsIgnoreCase( "Neo.ClientError.Security.AuthorizationExpired" ) )
79+
{
80+
return new AuthorizationExpiredException( code, message );
81+
}
82+
else if ( code.equalsIgnoreCase( "Neo.ClientError.Security.TokenExpired" ) )
83+
{
84+
return new TokenExpiredException( code, message );
85+
}
86+
else
87+
{
88+
return new SecurityException( code, message );
89+
}
8790
}
8891
else
8992
{
90-
return new ClientException( code, message );
93+
if ( code.equalsIgnoreCase( "Neo.ClientError.Database.DatabaseNotFound" ) )
94+
{
95+
return new FatalDiscoveryException( code, message );
96+
}
97+
else
98+
{
99+
return new ClientException( code, message );
100+
}
91101
}
92102
case "TransientError":
93103
return new TransientException( code, message );
@@ -140,7 +150,7 @@ private static boolean isClientOrTransientError( Neo4jException error )
140150
return errorCode != null && (errorCode.contains( "ClientError" ) || errorCode.contains( "TransientError" ));
141151
}
142152

143-
private static String extractClassification( String code )
153+
private static String extractErrorClass( String code )
144154
{
145155
String[] parts = code.split( "\\." );
146156
if ( parts.length < 2 )
@@ -150,6 +160,16 @@ private static String extractClassification( String code )
150160
return parts[1];
151161
}
152162

163+
private static String extractErrorSubClass( String code )
164+
{
165+
String[] parts = code.split( "\\." );
166+
if ( parts.length < 3 )
167+
{
168+
return "";
169+
}
170+
return parts[2];
171+
}
172+
153173
public static void addSuppressed( Throwable mainError, Throwable error )
154174
{
155175
if ( mainError != error )

‎driver/src/test/java/org/neo4j/driver/internal/cluster/RediscoveryTest.java

Lines changed: 65 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -20,6 +20,8 @@
2020

2121
import io.netty.util.concurrent.GlobalEventExecutor;
2222
import org.junit.jupiter.api.Test;
23+
import org.junit.jupiter.params.ParameterizedTest;
24+
import org.junit.jupiter.params.provider.ValueSource;
2325
import org.mockito.ArgumentCaptor;
2426

2527
import java.io.IOException;
@@ -33,6 +35,8 @@
3335
import org.neo4j.driver.Logger;
3436
import org.neo4j.driver.Logging;
3537
import org.neo4j.driver.exceptions.AuthenticationException;
38+
import org.neo4j.driver.exceptions.AuthorizationExpiredException;
39+
import org.neo4j.driver.exceptions.ClientException;
3640
import org.neo4j.driver.exceptions.DiscoveryException;
3741
import org.neo4j.driver.exceptions.ProtocolException;
3842
import org.neo4j.driver.exceptions.ServiceUnavailableException;
@@ -143,6 +147,67 @@ void shouldFailImmediatelyOnAuthError()
143147
verify( table ).forget( A );
144148
}
145149

150+
@Test
151+
void shouldUseAnotherRouterOnAuthorizationExpiredException()
152+
{
153+
ClusterComposition expectedComposition =
154+
new ClusterComposition( 42, asOrderedSet( A, B, C ), asOrderedSet( B, C, D ), asOrderedSet( A, B ), null );
155+
156+
Map<BoltServerAddress,Object> responsesByAddress = new HashMap<>();
157+
responsesByAddress.put( A, new AuthorizationExpiredException( "Neo.ClientError.Security.AuthorizationExpired", "message" ) );
158+
responsesByAddress.put( B, expectedComposition );
159+
160+
ClusterCompositionProvider compositionProvider = compositionProviderMock( responsesByAddress );
161+
Rediscovery rediscovery = newRediscovery( A, compositionProvider, mock( ServerAddressResolver.class ) );
162+
RoutingTable table = routingTableMock( A, B, C );
163+
164+
ClusterComposition actualComposition = await( rediscovery.lookupClusterComposition( table, pool, empty(), null ) ).getClusterComposition();
165+
166+
assertEquals( expectedComposition, actualComposition );
167+
verify( table ).forget( A );
168+
verify( table, never() ).forget( B );
169+
verify( table, never() ).forget( C );
170+
}
171+
172+
@ParameterizedTest
173+
@ValueSource( strings = {"Neo.ClientError.Transaction.InvalidBookmark", "Neo.ClientError.Transaction.InvalidBookmarkMixture"} )
174+
void shouldFailImmediatelyOnBookmarkErrors( String code )
175+
{
176+
ClientException error = new ClientException( code, "Invalid" );
177+
178+
Map<BoltServerAddress,Object> responsesByAddress = new HashMap<>();
179+
responsesByAddress.put( A, new RuntimeException( "Hi!" ) );
180+
responsesByAddress.put( B, error );
181+
182+
ClusterCompositionProvider compositionProvider = compositionProviderMock( responsesByAddress );
183+
Rediscovery rediscovery = newRediscovery( A, compositionProvider, mock( ServerAddressResolver.class ) );
184+
RoutingTable table = routingTableMock( A, B, C );
185+
186+
ClientException actualError = assertThrows( ClientException.class,
187+
() -> await( rediscovery.lookupClusterComposition( table, pool, empty(), null ) ) );
188+
assertEquals( error, actualError );
189+
verify( table ).forget( A );
190+
}
191+
192+
@Test
193+
void shouldFailImmediatelyOnClosedPoolError()
194+
{
195+
IllegalStateException error = new IllegalStateException( ConnectionPool.CONNECTION_POOL_CLOSED_ERROR_MESSAGE );
196+
197+
Map<BoltServerAddress,Object> responsesByAddress = new HashMap<>();
198+
responsesByAddress.put( A, new RuntimeException( "Hi!" ) );
199+
responsesByAddress.put( B, error );
200+
201+
ClusterCompositionProvider compositionProvider = compositionProviderMock( responsesByAddress );
202+
Rediscovery rediscovery = newRediscovery( A, compositionProvider, mock( ServerAddressResolver.class ) );
203+
RoutingTable table = routingTableMock( A, B, C );
204+
205+
IllegalStateException actualError = assertThrows( IllegalStateException.class,
206+
() -> await( rediscovery.lookupClusterComposition( table, pool, empty(), null ) ) );
207+
assertEquals( error, actualError );
208+
verify( table ).forget( A );
209+
}
210+
146211
@Test
147212
void shouldFallbackToInitialRouterWhenKnownRoutersFail()
148213
{

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

Lines changed: 2 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -44,7 +44,8 @@ public class GetFeatures implements TestkitRequest
4444
"Feature:Auth:Kerberos",
4545
"Feature:Auth:Custom",
4646
"Feature:Bolt:4.4",
47-
"Feature:Impersonation"
47+
"Feature:Impersonation",
48+
"Temporary:FastFailingDiscovery"
4849
) );
4950

5051
private static final Set<String> SYNC_FEATURES = new HashSet<>( Arrays.asList(

0 commit comments

Comments
 (0)