diff --git a/CLAUDE.md b/CLAUDE.md index 7416f41..7745590 100644 --- a/CLAUDE.md +++ b/CLAUDE.md @@ -71,7 +71,9 @@ Design decisions are documented in `adr/` — consult these before changing hash - PHP 8.1+ features: enums, readonly properties, named arguments, `match` expressions - PSR-4 autoloading: `src/` → `Cog\DbLocker\`, `test/` → `Cog\Test\DbLocker\` - All classes use `declare(strict_types=1)` +- **Abstract classes** must be prefixed with `Abstract` (e.g., `AbstractLockException`, `AbstractIntegrationTestCase`) - SQL comments with `humanReadableValue` appended to lock queries for debugging — these must be sanitized to prevent SQL comment injection (see `sanitizeSqlComment()`) +- **DocBlock formatting**: Always separate `@throws` tags from other tags (like `@param`, `@return`) with a blank line for better readability ## Git Workflow diff --git a/src/ConnectionAdapterInterface.php b/src/ConnectionAdapterInterface.php index cbaaf77..6a5f014 100644 --- a/src/ConnectionAdapterInterface.php +++ b/src/ConnectionAdapterInterface.php @@ -29,7 +29,8 @@ interface ConnectionAdapterInterface * @param string $sql SQL query with named parameters (e.g., :class_id) * @param array $params Named parameters (e.g., ['class_id' => 1, 'object_id' => 2]) * @return mixed The value of the first column (typically bool or string) - * @throws \Throwable Database-specific exception on error + * + * @throws \Exception Database-specific exception on error */ public function fetchColumn(string $sql, array $params = []): mixed; @@ -44,7 +45,8 @@ public function fetchColumn(string $sql, array $params = []): mixed; * * @param string $sql SQL statement with optional named parameters * @param array $params Named parameters (e.g., ['class_id' => 1]) - * @throws \Throwable Database-specific exception on error + * + * @throws \Exception Database-specific exception on error */ public function execute(string $sql, array $params = []): void; @@ -65,8 +67,8 @@ public function isTransactionActive(): bool; * - Doctrine DBAL: Doctrine\DBAL\Exception with getSQLState() method * - Cycle ORM: wraps \PDOException in its own exception * - * @param \Throwable $exception Exception to inspect + * @param \Exception $exception Exception to inspect * @return bool True if the exception indicates a lock timeout (SQLSTATE 55P03) */ - public function isLockNotAvailable(\Throwable $exception): bool; + public function isLockNotAvailable(\Exception $exception): bool; } diff --git a/src/DbConnection/PdoConnectionAdapter.php b/src/DbConnection/PdoConnectionAdapter.php index a777f63..96f78aa 100644 --- a/src/DbConnection/PdoConnectionAdapter.php +++ b/src/DbConnection/PdoConnectionAdapter.php @@ -56,7 +56,7 @@ public function isTransactionActive(): bool return $this->pdo->inTransaction(); } - public function isLockNotAvailable(\Throwable $exception): bool + public function isLockNotAvailable(\Exception $exception): bool { // PDOException: getCode() returns SQLSTATE string if ($exception instanceof \PDOException) { diff --git a/src/Exception/AbstractLockException.php b/src/Exception/AbstractLockException.php new file mode 100644 index 0000000..0ad81ed --- /dev/null +++ b/src/Exception/AbstractLockException.php @@ -0,0 +1,25 @@ + + * + * For the full copyright and license information, please view the LICENSE + * file that was distributed with this source code. + */ + +declare(strict_types=1); + +namespace Cog\DbLocker\Exception; + +/** + * Base exception for all database lock-related errors. + * + * This exception is thrown only for exceptional situations (database errors, connection issues, etc.), + * NOT for normal lock contention. When a lock is unavailable due to competition from other processes, + * methods return `false` instead of throwing this exception. + */ +abstract class AbstractLockException extends \RuntimeException +{ +} diff --git a/src/Exception/LockAcquireException.php b/src/Exception/LockAcquireException.php new file mode 100644 index 0000000..d2cea4e --- /dev/null +++ b/src/Exception/LockAcquireException.php @@ -0,0 +1,40 @@ + + * + * For the full copyright and license information, please view the LICENSE + * file that was distributed with this source code. + */ + +declare(strict_types=1); + +namespace Cog\DbLocker\Exception; + +use Cog\DbLocker\Postgres\PostgresLockKey; + +/** + * Exception thrown when a lock cannot be acquired due to a database error. + * + * This exception is NOT thrown when a lock is simply unavailable due to competition + * from other processes. In that case, methods return `false` instead. + * + * This exception IS thrown for genuine errors such as: + * - Connection failures + * - Query execution errors (excluding lock_not_available SQLSTATE 55P03) + * - Transaction state errors + */ +final class LockAcquireException extends AbstractLockException +{ + public static function fromDatabaseError( + PostgresLockKey $key, + \Exception $previous, + ): self { + return new self( + message: "Failed to acquire lock for key `{$key->humanReadableValue}`: {$previous->getMessage()}", + previous: $previous, + ); + } +} diff --git a/src/Exception/LockReleaseException.php b/src/Exception/LockReleaseException.php new file mode 100644 index 0000000..333b7a8 --- /dev/null +++ b/src/Exception/LockReleaseException.php @@ -0,0 +1,37 @@ + + * + * For the full copyright and license information, please view the LICENSE + * file that was distributed with this source code. + */ + +declare(strict_types=1); + +namespace Cog\DbLocker\Exception; + +use Cog\DbLocker\Postgres\PostgresLockKey; + +/** + * Exception thrown when a lock cannot be released due to a database error. + * + * This indicates a genuine error condition such as: + * - Connection failures + * - Query execution errors + * - Unexpected database state + */ +final class LockReleaseException extends AbstractLockException +{ + public static function fromDatabaseError( + PostgresLockKey $key, + \Exception $previous, + ): self { + return new self( + message: "Failed to release lock for key `{$key->humanReadableValue}`: {$previous->getMessage()}", + previous: $previous, + ); + } +} diff --git a/src/Postgres/PostgresAdvisoryLocker.php b/src/Postgres/PostgresAdvisoryLocker.php index ca94117..54bb017 100644 --- a/src/Postgres/PostgresAdvisoryLocker.php +++ b/src/Postgres/PostgresAdvisoryLocker.php @@ -14,6 +14,8 @@ namespace Cog\DbLocker\Postgres; use Cog\DbLocker\ConnectionAdapterInterface; +use Cog\DbLocker\Exception\LockAcquireException; +use Cog\DbLocker\Exception\LockReleaseException; use Cog\DbLocker\Postgres\Enum\PostgresLockAccessModeEnum; use Cog\DbLocker\Postgres\Enum\PostgresLockLevelEnum; use Cog\DbLocker\Postgres\LockHandle\SessionLevelLockHandle; @@ -26,6 +28,10 @@ final class PostgresAdvisoryLocker * Acquire a transaction-level advisory lock with configurable timeout and access mode. * * @param TimeoutDuration $timeoutDuration Maximum wait time. Use TimeoutDuration::zero() for an immediate (non-blocking) attempt. + * @return TransactionLevelLockHandle Handle with wasAcquired=false if lock is held by another process (normal competition). + * + * @throws LockAcquireException If a database error occurs (connection failures, query errors, etc.). NOT thrown for normal lock contention. + * @throws \LogicException If attempting to acquire outside of an active transaction. */ public function acquireTransactionLevelLock( ConnectionAdapterInterface $dbConnection, @@ -63,6 +69,9 @@ public function acquireTransactionLevelLock( * @param TimeoutDuration $timeoutDuration Maximum wait time. Use TimeoutDuration::zero() for an immediate (non-blocking) attempt. * @return TReturn The return value of the callback. * + * @throws LockAcquireException If a database error occurs during lock acquisition. NOT thrown for normal lock contention. + * @throws LockReleaseException If a database error occurs during lock release (only thrown if no other exception occurred during callback execution). + * * @see acquireTransactionLevelLock() for preferred locking strategy. * * @template TReturn @@ -117,6 +126,9 @@ public function withinSessionLevelLock( * lock handle's release() method. * * @param TimeoutDuration $timeoutDuration Maximum wait time. Use TimeoutDuration::zero() for an immediate (non-blocking) attempt. + * @return SessionLevelLockHandle Handle with wasAcquired=false if lock is held by another process (normal competition). + * + * @throws LockAcquireException If a database error occurs (connection failures, query errors, etc.). NOT thrown for normal lock contention. * * @see acquireTransactionLevelLock() for preferred locking strategy. * @see withinSessionLevelLock() for automatic session lock management. @@ -144,25 +156,33 @@ public function acquireSessionLevelLock( /** * Release session level advisory lock. + * + * @return bool True if the lock was successfully released, false if it was not held by this session. + * + * @throws LockReleaseException If a database error occurs during release. */ public function releaseSessionLevelLock( ConnectionAdapterInterface $dbConnection, PostgresLockKey $key, PostgresLockAccessModeEnum $accessMode = PostgresLockAccessModeEnum::Exclusive, ): bool { - $sql = match ($accessMode) { - PostgresLockAccessModeEnum::Exclusive - => 'SELECT PG_ADVISORY_UNLOCK(:class_id, :object_id);', + try { + $sql = match ($accessMode) { + PostgresLockAccessModeEnum::Exclusive + => 'SELECT PG_ADVISORY_UNLOCK(:class_id, :object_id);', - PostgresLockAccessModeEnum::Share - => 'SELECT PG_ADVISORY_UNLOCK_SHARED(:class_id, :object_id);', - }; - $sql .= " -- $key->humanReadableValue"; + PostgresLockAccessModeEnum::Share + => 'SELECT PG_ADVISORY_UNLOCK_SHARED(:class_id, :object_id);', + }; + $sql .= " -- $key->humanReadableValue"; - return $dbConnection->fetchColumn($sql, [ - 'class_id' => $key->classId, - 'object_id' => $key->objectId, - ]); + return $dbConnection->fetchColumn($sql, [ + 'class_id' => $key->classId, + 'object_id' => $key->objectId, + ]); + } catch (\Exception $exception) { + throw LockReleaseException::fromDatabaseError($key, $exception); + } } /** @@ -209,25 +229,29 @@ private function tryAcquireLock( PostgresLockLevelEnum $level, PostgresLockAccessModeEnum $accessMode, ): bool { - $sql = match ([$level, $accessMode]) { - [PostgresLockLevelEnum::Session, PostgresLockAccessModeEnum::Exclusive] - => 'SELECT PG_TRY_ADVISORY_LOCK(:class_id, :object_id);', + try { + $sql = match ([$level, $accessMode]) { + [PostgresLockLevelEnum::Session, PostgresLockAccessModeEnum::Exclusive] + => 'SELECT PG_TRY_ADVISORY_LOCK(:class_id, :object_id);', - [PostgresLockLevelEnum::Session, PostgresLockAccessModeEnum::Share] - => 'SELECT PG_TRY_ADVISORY_LOCK_SHARED(:class_id, :object_id);', + [PostgresLockLevelEnum::Session, PostgresLockAccessModeEnum::Share] + => 'SELECT PG_TRY_ADVISORY_LOCK_SHARED(:class_id, :object_id);', - [PostgresLockLevelEnum::Transaction, PostgresLockAccessModeEnum::Exclusive] - => 'SELECT PG_TRY_ADVISORY_XACT_LOCK(:class_id, :object_id);', + [PostgresLockLevelEnum::Transaction, PostgresLockAccessModeEnum::Exclusive] + => 'SELECT PG_TRY_ADVISORY_XACT_LOCK(:class_id, :object_id);', - [PostgresLockLevelEnum::Transaction, PostgresLockAccessModeEnum::Share] - => 'SELECT PG_TRY_ADVISORY_XACT_LOCK_SHARED(:class_id, :object_id);', - }; - $sql .= " -- $key->humanReadableValue"; + [PostgresLockLevelEnum::Transaction, PostgresLockAccessModeEnum::Share] + => 'SELECT PG_TRY_ADVISORY_XACT_LOCK_SHARED(:class_id, :object_id);', + }; + $sql .= " -- $key->humanReadableValue"; - return $dbConnection->fetchColumn($sql, [ - 'class_id' => $key->classId, - 'object_id' => $key->objectId, - ]); + return $dbConnection->fetchColumn($sql, [ + 'class_id' => $key->classId, + 'object_id' => $key->objectId, + ]); + } catch (\Exception $exception) { + throw LockAcquireException::fromDatabaseError($key, $exception); + } } private function acquireLockWithTimeout( @@ -276,32 +300,36 @@ private function acquireTransactionLockWithTimeout( PostgresLockKey $key, TimeoutDuration $timeoutDuration, ): bool { - $timeoutMs = $timeoutDuration->toMilliseconds(); - $dbConnection->execute("SET LOCAL lock_timeout = '$timeoutMs'"); + try { + $timeoutMs = $timeoutDuration->toMilliseconds(); + $dbConnection->execute("SET LOCAL lock_timeout = '$timeoutMs'"); - /** - * Use a savepoint so that a lock_timeout error does not abort the entire transaction. - * PostgreSQL handles same-name savepoints as a stack, so nested calls are safe. - */ - $dbConnection->execute('SAVEPOINT _lock_timeout_savepoint'); + /** + * Use a savepoint so that a lock_timeout error does not abort the entire transaction. + * PostgreSQL handles same-name savepoints as a stack, so nested calls are safe. + */ + $dbConnection->execute('SAVEPOINT _lock_timeout_savepoint'); - try { - $dbConnection->execute($sql, [ - 'class_id' => $key->classId, - 'object_id' => $key->objectId, - ]); + try { + $dbConnection->execute($sql, [ + 'class_id' => $key->classId, + 'object_id' => $key->objectId, + ]); - $dbConnection->execute('RELEASE SAVEPOINT _lock_timeout_savepoint'); + $dbConnection->execute('RELEASE SAVEPOINT _lock_timeout_savepoint'); - return true; - } catch (\Throwable $exception) { - if ($dbConnection->isLockNotAvailable($exception)) { - $dbConnection->execute('ROLLBACK TO SAVEPOINT _lock_timeout_savepoint'); + return true; + } catch (\Exception $exception) { + if ($dbConnection->isLockNotAvailable($exception)) { + $dbConnection->execute('ROLLBACK TO SAVEPOINT _lock_timeout_savepoint'); - return false; - } + return false; + } - throw $exception; + throw $exception; + } + } catch (\Exception $exception) { + throw LockAcquireException::fromDatabaseError($key, $exception); } } @@ -311,26 +339,30 @@ private function acquireSessionLockWithTimeout( PostgresLockKey $key, TimeoutDuration $timeoutDuration, ): bool { - $timeoutMs = $timeoutDuration->toMilliseconds(); - $originalLockTimeout = $dbConnection->fetchColumn('SHOW lock_timeout'); - $dbConnection->execute("SET lock_timeout = '$timeoutMs'"); - try { - $dbConnection->execute($sql, [ - 'class_id' => $key->classId, - 'object_id' => $key->objectId, - ]); + $timeoutMs = $timeoutDuration->toMilliseconds(); + $originalLockTimeout = $dbConnection->fetchColumn('SHOW lock_timeout'); + $dbConnection->execute("SET lock_timeout = '$timeoutMs'"); + + try { + $dbConnection->execute($sql, [ + 'class_id' => $key->classId, + 'object_id' => $key->objectId, + ]); + + return true; + } catch (\Exception $exception) { + if ($dbConnection->isLockNotAvailable($exception)) { + return false; + } - return true; - } catch (\Throwable $exception) { - if ($dbConnection->isLockNotAvailable($exception)) { - return false; + throw $exception; } - - throw $exception; - } - finally { - $dbConnection->execute("SET lock_timeout = '$originalLockTimeout'"); + finally { + $dbConnection->execute("SET lock_timeout = '$originalLockTimeout'"); + } + } catch (\Exception $exception) { + throw LockAcquireException::fromDatabaseError($key, $exception); } } } diff --git a/test/Integration/AbstractIntegrationTestCase.php b/test/Integration/AbstractIntegrationTestCase.php index 9f64ac3..c871cd9 100644 --- a/test/Integration/AbstractIntegrationTestCase.php +++ b/test/Integration/AbstractIntegrationTestCase.php @@ -16,6 +16,7 @@ use Cog\DbLocker\DbConnection\PdoConnectionAdapter; use Cog\DbLocker\ConnectionAdapterInterface; use Cog\DbLocker\Postgres\Enum\PostgresLockAccessModeEnum; +use Cog\DbLocker\Postgres\PostgresAdvisoryLocker; use Cog\DbLocker\Postgres\PostgresLockKey; use PDO; use PHPUnit\Framework\TestCase; @@ -52,6 +53,11 @@ protected function initConnectionAdapter(): ConnectionAdapterInterface return new PdoConnectionAdapter($this->initPostgresPdoConnection()); } + protected function initLocker(): PostgresAdvisoryLocker + { + return new PostgresAdvisoryLocker(); + } + protected function assertPgAdvisoryLockExistsInConnection( PDO $dbConnection, PostgresLockKey $postgresLockKey, diff --git a/test/Integration/Postgres/PostgresAdvisoryLockerExceptionTest.php b/test/Integration/Postgres/PostgresAdvisoryLockerExceptionTest.php new file mode 100644 index 0000000..763622c --- /dev/null +++ b/test/Integration/Postgres/PostgresAdvisoryLockerExceptionTest.php @@ -0,0 +1,168 @@ + + * + * For the full copyright and license information, please view the LICENSE + * file that was distributed with this source code. + */ + +declare(strict_types=1); + +namespace Cog\Test\DbLocker\Integration\Postgres; + +use Cog\DbLocker\ConnectionAdapterInterface; +use Cog\DbLocker\Exception\LockAcquireException; +use Cog\DbLocker\Exception\LockReleaseException; +use Cog\DbLocker\Postgres\Enum\PostgresLockAccessModeEnum; +use Cog\DbLocker\Postgres\PostgresAdvisoryLocker; +use Cog\DbLocker\Postgres\PostgresLockKey; +use Cog\DbLocker\TimeoutDuration; +use Cog\Test\DbLocker\Integration\AbstractIntegrationTestCase; + +final class PostgresAdvisoryLockerExceptionTest extends AbstractIntegrationTestCase +{ + public function testItThrowsLockAcquireExceptionOnDatabaseErrorForTransactionLock(): void + { + // GIVEN: A mock connection adapter that throws a database error (not lock_not_available) + $locker = $this->initLocker(); + $lockKey = PostgresLockKey::create('test', 'exception-test'); + $connectionAdapter = $this->createMockConnectionWithDatabaseError(); + + // WHEN: Attempting to acquire a transaction-level lock + // THEN: LockAcquireException should be thrown wrapping the original exception + $this->expectException(LockAcquireException::class); + $this->expectExceptionMessage("Failed to acquire lock for key `[test:exception-test]`"); + + $locker->acquireTransactionLevelLock( + $connectionAdapter, + $lockKey, + TimeoutDuration::zero(), + ); + } + + public function testItThrowsLockAcquireExceptionOnDatabaseErrorForSessionLock(): void + { + // GIVEN: A mock connection adapter that throws a database error + $locker = $this->initLocker(); + $lockKey = PostgresLockKey::create('test', 'exception-test'); + $connectionAdapter = $this->createMockConnectionWithDatabaseError(); + + // WHEN: Attempting to acquire a session-level lock + // THEN: LockAcquireException should be thrown wrapping the original exception + $this->expectException(LockAcquireException::class); + $this->expectExceptionMessage("Failed to acquire lock for key `[test:exception-test]`"); + + $locker->acquireSessionLevelLock( + $connectionAdapter, + $lockKey, + TimeoutDuration::zero(), + ); + } + + public function testItReturnsWasAcquiredFalseForLockNotAvailableTimeout(): void + { + // GIVEN: Two database connections and a lock key + $locker = $this->initLocker(); + $connection1 = $this->initConnectionAdapter(); + $connection2 = $this->initConnectionAdapter(); + $lockKey = PostgresLockKey::create('test', 'timeout-test'); + + // GIVEN: First connection holds the lock + $connection1->execute('BEGIN'); + $handle1 = $locker->acquireTransactionLevelLock( + $connection1, + $lockKey, + TimeoutDuration::zero(), + ); + $this->assertTrue($handle1->wasAcquired); + + // WHEN: Second connection attempts to acquire the same lock with a short timeout + $connection2->execute('BEGIN'); + $handle2 = $locker->acquireTransactionLevelLock( + $connection2, + $lockKey, + TimeoutDuration::ofMilliseconds(100), + ); + + // THEN: Lock should not be acquired, but no exception should be thrown + $this->assertFalse($handle2->wasAcquired); + } + + public function testItThrowsLockReleaseExceptionOnDatabaseErrorForRelease(): void + { + // GIVEN: A mock connection adapter that throws a database error on fetchColumn + $locker = $this->initLocker(); + $lockKey = PostgresLockKey::create('test', 'release-exception-test'); + $connectionAdapter = $this->createMockConnectionWithDatabaseError(); + + // WHEN: Attempting to release a session-level lock + // THEN: LockReleaseException should be thrown wrapping the original exception + $this->expectException(LockReleaseException::class); + $this->expectExceptionMessage("Failed to release lock for key `[test:release-exception-test]`"); + + $locker->releaseSessionLevelLock( + $connectionAdapter, + $lockKey, + ); + } + + public function testItDistinguishesLockNotAvailableFromDatabaseErrors(): void + { + // GIVEN: Two connections where first holds the lock + $locker = $this->initLocker(); + $connection1 = $this->initConnectionAdapter(); + $connection2 = $this->initConnectionAdapter(); + $lockKey = PostgresLockKey::create('test', 'distinguish-test'); + + // GIVEN: First connection begins transaction and holds the lock + $connection1->execute('BEGIN'); + $handle1 = $locker->acquireTransactionLevelLock( + $connection1, + $lockKey, + TimeoutDuration::zero(), + ); + $this->assertTrue($handle1->wasAcquired); + + // WHEN: Second connection tries with immediate timeout (lock_not_available is expected) + $connection2->execute('BEGIN'); + $handle2 = $locker->acquireTransactionLevelLock( + $connection2, + $lockKey, + TimeoutDuration::zero(), + ); + + // THEN: Should return false, not throw exception (this is normal lock contention) + $this->assertFalse($handle2->wasAcquired); + } + + private function createMockConnectionWithDatabaseError(): ConnectionAdapterInterface + { + return new class implements ConnectionAdapterInterface { + public function fetchColumn(string $sql, array $params = []): mixed + { + // Simulate a database error (not lock_not_available) + throw new \PDOException('Connection lost'); + } + + public function execute(string $sql, array $params = []): void + { + // Simulate a database error + throw new \PDOException('Connection lost'); + } + + public function isTransactionActive(): bool + { + return true; + } + + public function isLockNotAvailable(\Exception $exception): bool + { + // This mock returns false to ensure the exception is NOT treated as lock_not_available + return false; + } + }; + } +} diff --git a/test/Integration/Postgres/PostgresAdvisoryLockerTest.php b/test/Integration/Postgres/PostgresAdvisoryLockerTest.php index d12006c..58ab4ae 100644 --- a/test/Integration/Postgres/PostgresAdvisoryLockerTest.php +++ b/test/Integration/Postgres/PostgresAdvisoryLockerTest.php @@ -1096,9 +1096,4 @@ public function testItRestoresOriginalLockTimeoutAfterAcquisition(): void $lockTimeout = $statement->fetchColumn(0); $this->assertSame('10s', $lockTimeout); } - - private function initLocker(): PostgresAdvisoryLocker - { - return new PostgresAdvisoryLocker(); - } }