[feat]: More precise handling of errors when verifying connectivity (#…

…242) * Improve error handling when creating connections --------- Co-authored-by: exaby73 <[email protected]>
neo4j-php · Nov 29, 2024 · 7492476 · 7492476
1 parent 5bf29c7
commit 7492476
Show file tree

Hide file tree

Showing 7 changed files with 149 additions and 23 deletions.
diff --git a/docker-compose-neo4j-4.yml b/docker-compose-neo4j-4.yml
@@ -3,6 +3,7 @@ x-shared:
   NEO4J_AUTH: neo4j/testtest
   NEO4J_ACCEPT_LICENSE_AGREEMENT: "yes"
   NEO4J_dbms_security_allow__csv__import__from__file__urls: "true"
+  NEO4J_dbms_security_auth__lock__time: 0s
   NEO4JLABS_PLUGINS: '["apoc"]'
 
 x-shared-cluster:
@@ -31,7 +32,7 @@ services:
       context: .
       dockerfile: Dockerfile
       args:
-        PHP_VERSION: ${PHP_VERSION}
+        PHP_VERSION: "${PHP_VERSION-8.1}"
     networks:
       - neo4j
     volumes:
@@ -46,7 +47,7 @@ services:
       context: .
       dockerfile: Dockerfile
       args:
-        PHP_VERSION: ${PHP_VERSION}
+        PHP_VERSION: "${PHP_VERSION-8.1}"
         WITH_XDEBUG: true
     working_dir: /opt/project
     volumes:

diff --git a/docker-compose.yml b/docker-compose.yml
@@ -2,6 +2,7 @@ x-definitions:
   x-shared-env:
     &common-env
     NEO4J_AUTH: neo4j/testtest
+    NEO4J_dbms_security_auth__lock__time: 0s
     NEO4J_PLUGINS: '["apoc"]'
   x-shared-cluster-env:
     &common-cluster-env
@@ -25,7 +26,7 @@ x-definitions:
       context: .
       dockerfile: Dockerfile
       args:
-        PHP_VERSION: ${PHP_VERSION}
+        PHP_VERSION: "${PHP_VERSION-8.1}"
     volumes:
       - .:/opt/project
   x-common-cluster:

diff --git a/src/Bolt/BoltDriver.php b/src/Bolt/BoltDriver.php
@@ -13,6 +13,7 @@
 
 namespace Laudis\Neo4j\Bolt;
 
+use Bolt\error\ConnectException;
 use Exception;
 
 use function is_string;
@@ -28,7 +29,7 @@
 use Laudis\Neo4j\Databags\SessionConfiguration;
 use Laudis\Neo4j\Formatter\OGMFormatter;
 use Psr\Http\Message\UriInterface;
-use Throwable;
+use Psr\Log\LogLevel;
 
 /**
  * Drives a singular bolt connections.
@@ -103,7 +104,9 @@ public function verifyConnectivity(?SessionConfiguration $config = null): bool
         $config ??= SessionConfiguration::default();
         try {
             GeneratorHelper::getReturnFromGenerator($this->pool->acquire($config));
-        } catch (Throwable) {
+        } catch (ConnectException $e) {
+            $this->pool->getLogger()?->log(LogLevel::WARNING, 'Could not connect to server on URI '.$this->parsedUrl->__toString(), ['error' => $e]);
+
             return false;
         }
 

diff --git a/src/Common/DriverSetupManager.php b/src/Common/DriverSetupManager.php
@@ -17,6 +17,7 @@
 use function array_key_first;
 use function array_reduce;
 
+use Bolt\error\ConnectException;
 use Countable;
 use InvalidArgumentException;
 use Laudis\Neo4j\Authentication\Authenticate;
@@ -29,6 +30,7 @@
 
 use const PHP_INT_MIN;
 
+use Psr\Log\LogLevel;
 use RuntimeException;
 use SplPriorityQueue;
 
@@ -144,7 +146,13 @@ public function verifyConnectivity(SessionConfiguration $config, ?string $alias
     {
         try {
             $this->getDriver($config, $alias);
-        } catch (RuntimeException) {
+        } catch (ConnectException $e) {
+            $this->getLogger()?->log(
+                LogLevel::WARNING,
+                sprintf('Could not connect to server using alias (%s)', $alias ?? '<default>'),
+                ['exception' => $e]
+            );
+
             return false;
         }
 

diff --git a/src/Neo4j/Neo4jConnectionPool.php b/src/Neo4j/Neo4jConnectionPool.php
@@ -14,6 +14,9 @@
 namespace Laudis\Neo4j\Neo4j;
 
 use function array_unique;
+
+use Bolt\error\ConnectException;
+
 use function count;
 
 use Exception;
@@ -50,9 +53,6 @@
 
 use function sprintf;
 use function str_replace;
-
-use Throwable;
-
 use function time;
 
 /**
@@ -146,7 +146,7 @@ public function acquire(SessionConfiguration $config): Generator
                     /** @var BoltConnection $connection */
                     $connection = GeneratorHelper::getReturnFromGenerator($pool->acquire($config));
                     $table = $this->routingTable($connection, $config);
-                } catch (Throwable $e) {
+                } catch (ConnectException $e) {
                     // todo - once client side logging is implemented it must be conveyed here.
                     $latestError = $e;
                     continue; // We continue if something is wrong with the current server

diff --git a/src/Neo4j/Neo4jDriver.php b/src/Neo4j/Neo4jDriver.php
@@ -13,6 +13,7 @@
 
 namespace Laudis\Neo4j\Neo4j;
 
+use Bolt\error\ConnectException;
 use Exception;
 
 use function is_string;
@@ -31,7 +32,7 @@
 use Laudis\Neo4j\Databags\SessionConfiguration;
 use Laudis\Neo4j\Formatter\OGMFormatter;
 use Psr\Http\Message\UriInterface;
-use Throwable;
+use Psr\Log\LogLevel;
 
 /**
  * Driver for auto client-side routing.
@@ -105,7 +106,9 @@ public function verifyConnectivity(?SessionConfiguration $config = null): bool
         $config ??= SessionConfiguration::default();
         try {
             GeneratorHelper::getReturnFromGenerator($this->pool->acquire($config));
-        } catch (Throwable) {
+        } catch (ConnectException $e) {
+            $this->pool->getLogger()?->log(LogLevel::WARNING, 'Could not connect to server on URI '.$this->parsedUrl->__toString(), ['error' => $e]);
+
             return false;
         }
 

diff --git a/tests/Integration/ClientIntegrationTest.php b/tests/Integration/ClientIntegrationTest.php
@@ -13,22 +13,95 @@
 
 namespace Laudis\Neo4j\Tests\Integration;
 
+use Exception;
 use InvalidArgumentException;
 use Laudis\Neo4j\Authentication\Authenticate;
 use Laudis\Neo4j\Basic\Driver;
 use Laudis\Neo4j\Bolt\BoltDriver;
 use Laudis\Neo4j\Bolt\ConnectionPool;
 use Laudis\Neo4j\ClientBuilder;
+use Laudis\Neo4j\Common\DriverSetupManager;
 use Laudis\Neo4j\Contracts\DriverInterface;
 use Laudis\Neo4j\Contracts\TransactionInterface;
+use Laudis\Neo4j\Databags\DriverConfiguration;
+use Laudis\Neo4j\Databags\DriverSetup;
 use Laudis\Neo4j\Databags\SessionConfiguration;
 use Laudis\Neo4j\Databags\Statement;
+use Laudis\Neo4j\Databags\TransactionConfiguration;
 use Laudis\Neo4j\Exception\Neo4jException;
+use Laudis\Neo4j\Formatter\SummarizedResultFormatter;
 use Laudis\Neo4j\Tests\EnvironmentAwareIntegrationTest;
+use Psr\Log\LoggerInterface;
+use Psr\Log\LogLevel;
 use ReflectionClass;
+use RuntimeException;
 
 final class ClientIntegrationTest extends EnvironmentAwareIntegrationTest
 {
+    public function setUp(): void
+    {
+        parent::setUp();
+        $this->driver->closeConnections();
+    }
+
+    public function testDriverAuthFailureVerifyConnectivity(): void
+    {
+        if (str_starts_with($this->uri->getScheme(), 'http')) {
+            $this->markTestSkipped('HTTP does not support auth failure connectivity passing');
+        }
+
+        $uri = $this->uri->withUserInfo('neo4j', 'absolutelyonehundredpercentawrongpassword');
+
+        $conf = DriverConfiguration::default()->withLogger(LogLevel::DEBUG, $this->createMock(LoggerInterface::class));
+        $logger = $conf->getLogger();
+        if ($logger === null) {
+            throw new RuntimeException('Logger not set');
+        }
+
+        $driver = Driver::create($uri, $conf);
+
+        $this->expectException(Neo4jException::class);
+        $this->expectExceptionMessage(
+            'Neo4j errors detected. First one with code "Neo.ClientError.Security.Unauthorized" and message "The client is unauthorized due to authentication failure."'
+        );
+
+        $driver->verifyConnectivity();
+    }
+
+    public function testClientAuthFailureVerifyConnectivity(): void
+    {
+        if (str_starts_with($this->uri->getScheme(), 'http')) {
+            $this->markTestSkipped('HTTP does not support auth failure connectivity passing');
+        }
+
+        $uri = $this->uri->withUserInfo('neo4j', 'absolutelyonehundredpercentawrongpassword');
+
+        /** @noinspection PhpUnhandledExceptionInspection */
+        $conf = DriverConfiguration::default()->withLogger(LogLevel::DEBUG, $this->createMock(LoggerInterface::class));
+        $logger = $conf->getLogger();
+        if ($logger === null) {
+            throw new RuntimeException('Logger not set');
+        }
+
+        $client = (new ClientBuilder(
+            SessionConfiguration::create(),
+            TransactionConfiguration::create(),
+            (new DriverSetupManager(
+                SummarizedResultFormatter::create(),
+                $conf,
+            ))->withSetup(
+                new DriverSetup($uri, Authenticate::fromUrl($uri, $logger))
+            )
+        ))->build();
+
+        $this->expectException(Neo4jException::class);
+        $this->expectExceptionMessage(
+            'Neo4j errors detected. First one with code "Neo.ClientError.Security.Unauthorized" and message "The client is unauthorized due to authentication failure."'
+        );
+
+        $client->getDriver(null);
+    }
+
     public function testDifferentAuth(): void
     {
         $auth = Authenticate::fromUrl($this->getUri());
@@ -53,28 +126,37 @@ public function testAvailabilityFullImplementation(): void
 
     public function testTransactionFunction(): void
     {
-        $result = $this->getSession()->transaction(static fn (TransactionInterface $tsx) => $tsx->run('UNWIND [1] AS x RETURN x')->first()->getAsInt('x'));
+        $result = $this->getSession()->transaction(
+            static fn (TransactionInterface $tsx) => $tsx->run('UNWIND [1] AS x RETURN x')->first()->getAsInt('x')
+        );
 
         self::assertEquals(1, $result);
 
-        $result = $this->getSession()->readTransaction(static fn (TransactionInterface $tsx) => $tsx->run('UNWIND [1] AS x RETURN x')->first()->getAsInt('x'));
+        $result = $this->getSession()->readTransaction(
+            static fn (TransactionInterface $tsx) => $tsx->run('UNWIND [1] AS x RETURN x')->first()->getAsInt('x')
+        );
 
         self::assertEquals(1, $result);
 
-        $result = $this->getSession()->writeTransaction(static fn (TransactionInterface $tsx) => $tsx->run('UNWIND [1] AS x RETURN x')->first()->getAsInt('x'));
+        $result = $this->getSession()->writeTransaction(
+            static fn (TransactionInterface $tsx) => $tsx->run('UNWIND [1] AS x RETURN x')->first()->getAsInt('x')
+        );
 
         self::assertEquals(1, $result);
     }
 
     public function testValidRun(): void
     {
-        $response = $this->getSession()->transaction(static fn (TransactionInterface $tsx) => $tsx->run(<<<'CYPHER'
+        $response = $this->getSession()->transaction(static fn (TransactionInterface $tsx) => $tsx->run(
+            <<<'CYPHER'
 MERGE (x:TestNode {test: $test})
 WITH x
 MERGE (y:OtherTestNode {test: $otherTest})
 WITH x, y, {c: 'd'} AS map, [1, 2, 3] AS list
 RETURN x, y, x.test AS test, map, list
-CYPHER, ['test' => 'a', 'otherTest' => 'b']));
+CYPHER,
+            ['test' => 'a', 'otherTest' => 'b']
+        ));
 
         self::assertEquals(1, $response->count());
         $map = $response->first();
@@ -89,7 +171,12 @@ public function testValidRun(): void
     public function testInvalidRun(): void
     {
         $this->expectException(Neo4jException::class);
-        $this->getSession()->transaction(static fn (TransactionInterface $tsx) => $tsx->run('MERGE (x:Tes0342hdm21.())', ['test' => 'a', 'otherTest' => 'b']));
+        $this->getSession()->transaction(
+            static fn (TransactionInterface $tsx) => $tsx->run(
+                'MERGE (x:Tes0342hdm21.())',
+                ['test' => 'a', 'otherTest' => 'b']
+            )
+        );
     }
 
     public function testInvalidRunRetry(): void
@@ -108,13 +195,18 @@ public function testInvalidRunRetry(): void
 
     public function testValidStatement(): void
     {
-        $response = $this->getSession()->transaction(static fn (TransactionInterface $tsx) => $tsx->runStatement(Statement::create(<<<'CYPHER'
+        $response = $this->getSession()->transaction(static fn (TransactionInterface $tsx) => $tsx->runStatement(
+            Statement::create(
+                <<<'CYPHER'
 MERGE (x:TestNode {test: $test})
 WITH x
 MERGE (y:OtherTestNode {test: $otherTest})
 WITH x, y, {c: 'd'} AS map, [1, 2, 3] AS list
 RETURN x, y, x.test AS test, map, list
-CYPHER, ['test' => 'a', 'otherTest' => 'b'])));
+CYPHER,
+                ['test' => 'a', 'otherTest' => 'b']
+            )
+        ));
 
         self::assertEquals(1, $response->count());
         $map = $response->first();
@@ -187,9 +279,27 @@ public function testInvalidConnectionCheck(): void
             ->withDriver('http', 'http://localboast')
             ->build();
 
-        self::assertFalse($client->verifyConnectivity('bolt'));
-        self::assertFalse($client->verifyConnectivity('neo4j'));
-        self::assertFalse($client->verifyConnectivity('http'));
+        $exceptionThrownCount = 0;
+        try {
+            $client->verifyConnectivity('bolt');
+        } catch (Exception $e) {
+            self::assertInstanceOf(RuntimeException::class, $e);
+            ++$exceptionThrownCount;
+        }
+        try {
+            $client->verifyConnectivity('neo4j');
+        } catch (Exception $e) {
+            self::assertInstanceOf(RuntimeException::class, $e);
+            ++$exceptionThrownCount;
+        }
+        try {
+            $client->verifyConnectivity('http');
+        } catch (Exception $e) {
+            self::assertInstanceOf(RuntimeException::class, $e);
+            ++$exceptionThrownCount;
+        }
+
+        self::assertEquals(3, $exceptionThrownCount);
     }
 
     public function testValidConnectionCheck(): void