From 85a358b40c1a2f075b6b2d8b9bf75f6f7ca6654b Mon Sep 17 00:00:00 2001 From: Vladislav Date: Fri, 6 Nov 2026 11:42:18 +0200 Subject: [PATCH 01/14] feat(PLT-13586): add transaction search_path mode --- .github/workflows/ci.yaml | 63 +++ README.md | 79 +++- composer.json | 2 +- docker-compose.yml | 38 ++ docker/php/Dockerfile | 8 + phpunit.xml.dist | 6 +- .../AbstractDoctrineSchemaCommand.php | 7 + .../TransactionSearchPathConnection.php | 223 +++++++++++ .../TransactionSearchPathDriver.php | 46 +++ .../TransactionSearchPathStatement.php | 24 ++ src/Doctrine/SchemaConnection.php | 88 ++++- src/Doctrine/SearchPathMode.php | 14 + src/Exception/UnsupportedDriverException.php | 17 + tests/Doctrine/SchemaConnectionTest.php | 48 +++ tests/Integration/SearchPathPgbouncerTest.php | 364 ++++++++++++++++++ 15 files changed, 1020 insertions(+), 7 deletions(-) create mode 100644 docker-compose.yml create mode 100644 docker/php/Dockerfile create mode 100644 src/Doctrine/Middleware/TransactionSearchPathConnection.php create mode 100644 src/Doctrine/Middleware/TransactionSearchPathDriver.php create mode 100644 src/Doctrine/Middleware/TransactionSearchPathStatement.php create mode 100644 src/Doctrine/SearchPathMode.php create mode 100644 src/Exception/UnsupportedDriverException.php create mode 100644 tests/Integration/SearchPathPgbouncerTest.php diff --git a/.github/workflows/ci.yaml b/.github/workflows/ci.yaml index 0564467..e682399 100644 --- a/.github/workflows/ci.yaml +++ b/.github/workflows/ci.yaml @@ -103,3 +103,66 @@ jobs: uses: codecov/codecov-action@v4.0.1 with: token: ${{ secrets.CODECOV_TOKEN }} + + integration: + name: Integration (PostgreSQL ${{ matrix.postgres }} + PgBouncer transaction pooling) + runs-on: ubuntu-latest + strategy: + fail-fast: false + matrix: + postgres: + - '14' + - '18' + services: + postgres: + image: postgres:${{ matrix.postgres }}-alpine + env: + POSTGRES_USER: app + POSTGRES_PASSWORD: app + POSTGRES_DB: app + POSTGRES_HOST_AUTH_METHOD: scram-sha-256 + POSTGRES_INITDB_ARGS: --auth=scram-sha-256 + ports: + - 5432:5432 + options: >- + --health-cmd "pg_isready -U app" + --health-interval 2s + --health-timeout 5s + --health-retries 20 + pgbouncer: + image: edoburu/pgbouncer:v1.24.1-p1 + env: + DB_HOST: postgres + DB_USER: app + DB_PASSWORD: app + DB_NAME: app + AUTH_TYPE: scram-sha-256 + POOL_MODE: transaction + DEFAULT_POOL_SIZE: 1 + MAX_PREPARED_STATEMENTS: 200 + ports: + - 6432:5432 + + env: + DIRECT_DATABASE_URL: pgsql://app:app@127.0.0.1:5432/app + PGBOUNCER_DATABASE_URL: pgsql://app:app@127.0.0.1:6432/app + + steps: + - name: Checkout + uses: actions/checkout@v4 + + - name: Setup PHP + uses: shivammathur/setup-php@v2 + with: + php-version: '8.3' + extensions: pdo_pgsql + coverage: none + + - name: Install dependencies + run: composer install --no-interaction --no-progress + + - name: Wait for PgBouncer + run: timeout 60 sh -c 'until pg_isready -h 127.0.0.1 -p 6432 -U app; do sleep 1; done' + + - name: Run integration tests + run: vendor/bin/phpunit --testsuite integration --fail-on-skipped diff --git a/README.md b/README.md index 6b15856..ca30f9c 100644 --- a/README.md +++ b/README.md @@ -68,6 +68,66 @@ class Kernel extends BaseKernel } ``` +### Search path mode (PgBouncer transaction pooling) + +By default the bundle sets `search_path` once per database session (`SET search_path TO ...`). +This is safe only with a direct connection or PgBouncer in **session** pooling mode. +Behind PgBouncer in **transaction** pooling mode a session setting stays on the pooled server connection +and leaks to other clients: their queries run in another schema. + +Switch to the transaction mode at kernel boot, next to `setSchemaResolver()`: + +```php +use Macpaw\PostgresSchemaBundle\Doctrine\SchemaConnection; +use Macpaw\PostgresSchemaBundle\Doctrine\SearchPathMode; + +SchemaConnection::setSearchPathMode(SearchPathMode::Transaction); +``` + +`SchemaConnection::setSearchPathMode(SearchPathMode $mode, ?string $serverDefaultSchema = null)`: + +| Argument | Value | Behaviour | +|---|---|---| +| `$mode` | `SearchPathMode::Session` (default) | `SET search_path` once per connection; current behaviour | +| | `SearchPathMode::Transaction` | `SET LOCAL search_path` inside every transaction; the setting never outlives the transaction | +| `$serverDefaultSchema` | `null` (strict, default) | `SET LOCAL` for every schema, including the default one | +| | `'public'` or another schema | queries for this schema run without `SET LOCAL` and rely on the server default `search_path` | + +In transaction mode: + +- A statement outside a transaction runs as `BEGIN; SET LOCAL search_path TO ""`, the statement, then `COMMIT` + (`ROLLBACK` on error): two extra round trips. +- Inside a transaction `SET LOCAL` runs once after `BEGIN`, and again only when the schema changes or after + `ROLLBACK TO SAVEPOINT`. +- Plain queries, executed statements and prepared statements (Doctrine ORM) are all covered. +- The schema commands below only set the schema in the resolver; they do not issue a session `SET`. +- Transaction control SQL (`BEGIN`, `COMMIT`, `ROLLBACK`, savepoints) runs as is, without `SET LOCAL` before it. +- In the default `Session` mode statements pass through unchanged. + +Choosing `$serverDefaultSchema`: + +- Use `null` while any client of the same database still sets a session `search_path`: pods on an older bundle + version during rollout, session-mode services sharing the pool, manual `psql` sessions. + Otherwise default-schema queries inherit the leaked setting. +- Pass the schema only when every client of the database uses transaction mode and the server default + `search_path` (role or database setting) resolves to that schema. + For an environment whose default schema is not `public`, set it first with + `ALTER ROLE ... SET search_path` or `ALTER DATABASE ... SET search_path`. + +Limitations: + +- Only the `pdo_pgsql` driver is supported. It prepares a statement when it is executed, inside `SET LOCAL`. + The native `pgsql` driver prepares immediately, against the server default `search_path`, so it is rejected + with `UnsupportedDriverException`. +- A statement outside a transaction is committed before its result is fetched. This relies on `pdo_pgsql` + buffering the whole result (the default); do not enable lazy (unbuffered) result fetching. +- Statements that cannot run inside a transaction block (`CREATE INDEX CONCURRENTLY`, `VACUUM`, migrations with + `isTransactional() = false`) fail for a schema that needs `SET LOCAL`. +- `lastInsertId()` outside a transaction runs after the implicit `COMMIT`, possibly on another server connection + and without the schema `search_path`: `LASTVAL()` and `CURRVAL('')` can fail or return another value. + Fetch ids with `nextval()` (Doctrine ORM `SEQUENCE` strategy), `INSERT ... RETURNING`, or call `lastInsertId()` + inside the same explicit transaction as the insert. + Make sure you configure the context bundle properly: See https://github.com/MacPaw/schema-context-bundle/blob/develop/README.md @@ -83,7 +143,8 @@ schema_context: ## How it Works * A request comes in with a header like X-Tenant-Id: tenant123. * The SchemaRequestListener sets this schema in the context. -* When Doctrine connects to PostgreSQL, it sets the search_path to the specified schema. +* When Doctrine connects to PostgreSQL, it sets the search_path to the specified schema + (per session, or per transaction in `SearchPathMode::Transaction`). * If the schema does not exist or DB is not PostgreSQL, an exception is thrown. ## Optional Commands @@ -148,10 +209,22 @@ php bin/console doctrine:schema:fixtures:load [options] **Note:** These commands are optional and should only be registered if you're using the corresponding Doctrine features (migrations and/or fixtures) in your project. ## Testing -To run tests: +Unit tests: +```bash +vendor/bin/phpunit --testsuite unit +``` + +Integration tests run against PostgreSQL (14 by default, `POSTGRES_VERSION=18` for 18) behind PgBouncer in transaction pooling mode +with a single server connection, so every client shares one server session: ```bash -vendor/bin/phpunit +docker compose up -d --build +docker compose run --rm php composer install +docker compose run --rm php vendor/bin/phpunit +docker compose down ``` + +Without `DIRECT_DATABASE_URL` and `PGBOUNCER_DATABASE_URL` the integration tests are skipped. +CI runs them in the `integration` job against PostgreSQL 14 and 18. ## Contributing Feel free to open issues and submit pull requests. diff --git a/composer.json b/composer.json index 6b4e291..391b51d 100644 --- a/composer.json +++ b/composer.json @@ -25,7 +25,7 @@ "doctrine/doctrine-fixtures-bundle": "^3.5 || ^4.0", "phpstan/phpstan": "^1.10", "phpunit/phpunit": "^10.0", - "squizlabs/php_codesniffer": "3.7.*", + "squizlabs/php_codesniffer": "^3.13", "dg/bypass-finals": "^1.9" }, "config": { diff --git a/docker-compose.yml b/docker-compose.yml new file mode 100644 index 0000000..0cf90d3 --- /dev/null +++ b/docker-compose.yml @@ -0,0 +1,38 @@ +services: + postgres: + image: postgres:${POSTGRES_VERSION:-14}-alpine + environment: + POSTGRES_USER: app + POSTGRES_PASSWORD: app + POSTGRES_DB: app + POSTGRES_HOST_AUTH_METHOD: scram-sha-256 + POSTGRES_INITDB_ARGS: --auth=scram-sha-256 + healthcheck: + test: ["CMD", "pg_isready", "-U", "app"] + interval: 2s + retries: 20 + + pgbouncer: + image: edoburu/pgbouncer:v1.24.1-p1 + environment: + DB_HOST: postgres + DB_USER: app + DB_PASSWORD: app + DB_NAME: app + AUTH_TYPE: scram-sha-256 + POOL_MODE: transaction + DEFAULT_POOL_SIZE: 1 + MAX_PREPARED_STATEMENTS: 200 + depends_on: + postgres: + condition: service_healthy + + php: + build: docker/php + volumes: + - .:/app + environment: + DIRECT_DATABASE_URL: pgsql://app:app@postgres:5432/app + PGBOUNCER_DATABASE_URL: pgsql://app:app@pgbouncer:5432/app + depends_on: + - pgbouncer diff --git a/docker/php/Dockerfile b/docker/php/Dockerfile new file mode 100644 index 0000000..dfa7cbc --- /dev/null +++ b/docker/php/Dockerfile @@ -0,0 +1,8 @@ +FROM php:8.3-cli-alpine + +RUN apk add --no-cache postgresql-dev \ + && docker-php-ext-install pdo_pgsql pgsql + +COPY --from=composer:2 /usr/bin/composer /usr/bin/composer + +WORKDIR /app diff --git a/phpunit.xml.dist b/phpunit.xml.dist index 2ad4f0b..3adc3da 100644 --- a/phpunit.xml.dist +++ b/phpunit.xml.dist @@ -6,8 +6,12 @@ xsi:noNamespaceSchemaLocation="https://schema.phpunit.de/10.5/phpunit.xsd" > - + ./tests + ./tests/Integration + + + ./tests/Integration diff --git a/src/Command/Doctrine/AbstractDoctrineSchemaCommand.php b/src/Command/Doctrine/AbstractDoctrineSchemaCommand.php index ff142a8..4cb4c27 100644 --- a/src/Command/Doctrine/AbstractDoctrineSchemaCommand.php +++ b/src/Command/Doctrine/AbstractDoctrineSchemaCommand.php @@ -6,6 +6,8 @@ use Doctrine\DBAL\Connection; use Error; +use Macpaw\PostgresSchemaBundle\Doctrine\SchemaConnection; +use Macpaw\PostgresSchemaBundle\Doctrine\SearchPathMode; use Macpaw\SchemaContextBundle\Service\BaggageSchemaResolver; use Symfony\Component\Console\Command\Command; use Symfony\Component\Console\Input\InputArgument; @@ -56,6 +58,11 @@ protected function isSchemaExist(string $schema): bool protected function switchToSchema(string $schema): void { $this->schemaResolver->setSchema($schema); + + if (SchemaConnection::getSearchPathMode() === SearchPathMode::Transaction) { + return; + } + $quotedSchema = $this->connection->quoteIdentifier($schema); $this->connection->executeStatement("SET search_path TO {$quotedSchema}"); diff --git a/src/Doctrine/Middleware/TransactionSearchPathConnection.php b/src/Doctrine/Middleware/TransactionSearchPathConnection.php new file mode 100644 index 0000000..acd58bc --- /dev/null +++ b/src/Doctrine/Middleware/TransactionSearchPathConnection.php @@ -0,0 +1,223 @@ +runWithSearchPath(fn (): Result => parent::query($sql)); + } + + public function exec(string $sql): int + { + if ($this->isTransactionControl($sql)) { + return $this->execTransactionControl($sql); + } + + return $this->runWithSearchPath(fn (): int => parent::exec($sql)); + } + + public function beginTransaction() + { + $result = parent::beginTransaction(); + $this->startTransaction(); + + try { + $this->applyInTransaction(); + } catch (Throwable $exception) { + try { + parent::rollBack(); + } catch (Throwable) { + } + + $this->endTransaction(); + + throw $exception; + } + + return $result; + } + + public function commit() + { + try { + return parent::commit(); + } finally { + $this->endTransaction(); + } + } + + public function rollBack() + { + try { + return parent::rollBack(); + } finally { + $this->endTransaction(); + } + } + + /** + * @template T + * + * @param Closure(): T $operation + * + * @return T + */ + public function runWithSearchPath(Closure $operation): mixed + { + if (!($this->isActive)()) { + return $operation(); + } + + if ($this->inTransaction) { + $this->applyInTransaction(); + + return $operation(); + } + + $schema = ($this->schemaProvider)(); + + if ($schema === null) { + return $operation(); + } + + return $this->runInImplicitTransaction($schema, $operation); + } + + private function applyInTransaction(): void + { + if (!($this->isActive)()) { + return; + } + + $schema = ($this->schemaProvider)(); + + if ($this->isAppliedSchemaKnown && $schema === $this->appliedSchema) { + return; + } + + parent::exec('SET LOCAL search_path TO ' . ($schema ?? 'DEFAULT')); + $this->appliedSchema = $schema; + $this->isAppliedSchemaKnown = true; + ($this->onApply)($schema ?? 'DEFAULT', true); + } + + /** + * @template T + * + * @param Closure(): T $operation + * + * @return T + */ + private function runInImplicitTransaction(string $schema, Closure $operation): mixed + { + try { + parent::exec('BEGIN; SET LOCAL search_path TO ' . $schema); + ($this->onApply)($schema, false); + $result = $operation(); + } catch (Throwable $exception) { + $this->rollBackQuietly(); + + throw $exception; + } + + parent::exec('COMMIT'); + + return $result; + } + + private function isTransactionControl(string $sql): bool + { + return preg_match(self::BEGIN_PATTERN, $sql) === 1 + || preg_match(self::END_PATTERN, $sql) === 1 + || ($this->inTransaction && preg_match(self::SAVEPOINT_PATTERN, $sql) === 1); + } + + /** + * Transaction control SQL runs as is: no SET LOCAL before it (it would fail in an aborted transaction + * and block the recovery) and no implicit transaction around it (it would end the caller's transaction). + */ + private function execTransactionControl(string $sql): int + { + $result = parent::exec($sql); + + if (preg_match(self::BEGIN_PATTERN, $sql) === 1) { + $this->startTransaction(); + } elseif (preg_match(self::END_PATTERN, $sql) === 1) { + $this->endTransaction(); + } elseif (preg_match(self::ROLLBACK_TO_PATTERN, $sql) === 1) { + // A SET LOCAL issued after the savepoint is undone with it, so re-apply before the next query. + $this->isAppliedSchemaKnown = false; + } + + return $result; + } + + private function startTransaction(): void + { + $this->inTransaction = true; + $this->appliedSchema = null; + $this->isAppliedSchemaKnown = true; + } + + private function endTransaction(): void + { + $this->inTransaction = false; + $this->appliedSchema = null; + $this->isAppliedSchemaKnown = true; + } + + private function rollBackQuietly(): void + { + try { + parent::exec('ROLLBACK'); + } catch (Throwable) { + } + } +} diff --git a/src/Doctrine/Middleware/TransactionSearchPathDriver.php b/src/Doctrine/Middleware/TransactionSearchPathDriver.php new file mode 100644 index 0000000..517483b --- /dev/null +++ b/src/Doctrine/Middleware/TransactionSearchPathDriver.php @@ -0,0 +1,46 @@ +isActive)() && $this->wrappedDriver instanceof NativePgSQLDriver) { + throw new UnsupportedDriverException('pgsql'); + } + + return new TransactionSearchPathConnection( + parent::connect($params), + $this->isActive, + $this->schemaProvider, + $this->onApply, + ); + } +} diff --git a/src/Doctrine/Middleware/TransactionSearchPathStatement.php b/src/Doctrine/Middleware/TransactionSearchPathStatement.php new file mode 100644 index 0000000..c097b20 --- /dev/null +++ b/src/Doctrine/Middleware/TransactionSearchPathStatement.php @@ -0,0 +1,24 @@ +connection->runWithSearchPath(fn (): Result => parent::execute($params)); + } +} diff --git a/src/Doctrine/SchemaConnection.php b/src/Doctrine/SchemaConnection.php index 3eac8da..a56e643 100644 --- a/src/Doctrine/SchemaConnection.php +++ b/src/Doctrine/SchemaConnection.php @@ -4,19 +4,66 @@ namespace Macpaw\PostgresSchemaBundle\Doctrine; +use Doctrine\Common\EventManager; +use Doctrine\DBAL\Configuration; use Doctrine\DBAL\Connection as DBALConnection; +use Doctrine\DBAL\Driver; +use Doctrine\DBAL\DriverManager; use Doctrine\DBAL\Platforms\PostgreSQLPlatform; +use Macpaw\PostgresSchemaBundle\Doctrine\Middleware\TransactionSearchPathDriver; use Macpaw\PostgresSchemaBundle\Exception\UnsupportedPlatformException; use Macpaw\SchemaContextBundle\Logger\DebugLogger; use Macpaw\SchemaContextBundle\Service\BaggageSchemaResolver; use Throwable; +/** + * @phpstan-import-type Params from DriverManager + */ class SchemaConnection extends DBALConnection { private static ?BaggageSchemaResolver $schemaResolver = null; private static ?DebugLogger $logger = null; + private static SearchPathMode $searchPathMode = SearchPathMode::Session; + private static ?string $serverDefaultSchema = null; private ?string $currentSchema = null; + /** + * @param array $params + * @phpstan-param Params $params + */ + public function __construct( + array $params, + Driver $driver, + ?Configuration $config = null, + ?EventManager $eventManager = null, + ) { + $driver = new TransactionSearchPathDriver( + $driver, + self::isTransactionSearchPathActive(...), + self::resolveTransactionSearchPath(...), + self::logTransactionSearchPath(...), + ); + + parent::__construct($params, $driver, $config, $eventManager); + } + + /** + * @param string|null $serverDefaultSchema null (strict, default) applies SET LOCAL to every query, ignoring any + * session search_path another client left on the pooled server + * connection. A schema name skips SET LOCAL for that schema and relies + * on the server default search_path (role/database setting). + */ + public static function setSearchPathMode(SearchPathMode $mode, ?string $serverDefaultSchema = null): void + { + self::$searchPathMode = $mode; + self::$serverDefaultSchema = $serverDefaultSchema; + } + + public static function getSearchPathMode(): SearchPathMode + { + return self::$searchPathMode; + } + public static function setSchemaResolver(BaggageSchemaResolver $resolver): void { self::$schemaResolver = $resolver; @@ -29,7 +76,9 @@ public static function setLogger(DebugLogger $logger): void public function close(): void { - self::$logger?->logSchemaResetByConnectionClose($this->currentSchema ?? ''); + if ($this->currentSchema !== null) { + self::$logger?->logSchemaResetByConnectionClose($this->currentSchema); + } parent::close(); @@ -38,7 +87,9 @@ public function close(): void public function rollBack(): bool { - self::$logger?->logSchemaResetByTransactionRollback($this->currentSchema ?? ''); + if ($this->currentSchema !== null) { + self::$logger?->logSchemaResetByTransactionRollback($this->currentSchema); + } $result = parent::rollBack(); @@ -55,6 +106,14 @@ public function connect(): bool return $isNewConnection; } + if (self::$searchPathMode === SearchPathMode::Transaction) { + if ($isNewConnection) { + $this->ensurePostgreSql(); + } + + return $isNewConnection; + } + $schema = self::$schemaResolver->getSchema(); if ($isNewConnection) { @@ -77,6 +136,31 @@ public function connect(): bool return $isNewConnection; } + private static function isTransactionSearchPathActive(): bool + { + return self::$searchPathMode === SearchPathMode::Transaction && self::$schemaResolver !== null; + } + + private static function resolveTransactionSearchPath(): ?string + { + if (self::$schemaResolver === null) { + return null; + } + + $schema = self::$schemaResolver->getSchema(); + + if (self::$serverDefaultSchema !== null && $schema === self::$serverDefaultSchema) { + return null; + } + + return '"' . str_replace('"', '""', $schema) . '"'; + } + + private static function logTransactionSearchPath(string $quotedSchema, bool $isInTransaction): void + { + self::$logger?->logApplySearchPath($quotedSchema, false, $isInTransaction); + } + private function getActualSearchPath(): ?string { if ($this->_conn === null) { diff --git a/src/Doctrine/SearchPathMode.php b/src/Doctrine/SearchPathMode.php new file mode 100644 index 0000000..376566d --- /dev/null +++ b/src/Doctrine/SearchPathMode.php @@ -0,0 +1,14 @@ +connect(); } + + public function testTransactionModeThrowsForUnsupportedPlatform(): void + { + $driverConnection = $this->createMock(DriverConnection::class); + $driverConnection->expects($this->never())->method('exec'); + $driver = $this->createMock(Driver::class); + $driver->method('connect')->willReturn($driverConnection); + + $connection = $this->getMockBuilder(SchemaConnection::class) + ->setConstructorArgs([[], $driver, new Configuration(), new EventManager()]) + ->onlyMethods(['getDatabasePlatform']) + ->getMock(); + $connection->method('getDatabasePlatform')->willReturn(new MySQLPlatform()); + + SchemaConnection::setSchemaResolver( + new BaggageSchemaResolver('public', 'development', ['development'], new DebugLogger()), + ); + SchemaConnection::setSearchPathMode(SearchPathMode::Transaction); + + try { + $this->expectException(UnsupportedPlatformException::class); + $connection->connect(); + } finally { + SchemaConnection::setSearchPathMode(SearchPathMode::Session); + } + } + + public function testTransactionModeRejectsNativePgsqlDriver(): void + { + $driver = $this->createMock(NativePgSQLDriver::class); + $driver->expects($this->never())->method('connect'); + $connection = new SchemaConnection([], $driver, new Configuration(), new EventManager()); + + SchemaConnection::setSchemaResolver( + new BaggageSchemaResolver('public', 'development', ['development'], new DebugLogger()), + ); + SchemaConnection::setSearchPathMode(SearchPathMode::Transaction); + + try { + $this->expectException(UnsupportedDriverException::class); + $connection->connect(); + } finally { + SchemaConnection::setSearchPathMode(SearchPathMode::Session); + } + } } diff --git a/tests/Integration/SearchPathPgbouncerTest.php b/tests/Integration/SearchPathPgbouncerTest.php new file mode 100644 index 0000000..7fde2e5 --- /dev/null +++ b/tests/Integration/SearchPathPgbouncerTest.php @@ -0,0 +1,364 @@ + */ + private array $connections = []; + + public static function setUpBeforeClass(): void + { + if (self::url('DIRECT_DATABASE_URL') === null || self::url('PGBOUNCER_DATABASE_URL') === null) { + return; + } + + $direct = DriverManager::getConnection(self::params('DIRECT_DATABASE_URL')); + $previewSchema = $direct->quoteIdentifier(self::PREVIEW_SCHEMA); + + $direct->executeStatement('DROP SCHEMA IF EXISTS ' . $previewSchema . ' CASCADE'); + $direct->executeStatement('DROP TABLE IF EXISTS public.marker'); + $direct->executeStatement('CREATE SCHEMA ' . $previewSchema); + $direct->executeStatement('CREATE TABLE public.marker (id serial PRIMARY KEY, v text NOT NULL)'); + $direct->executeStatement( + 'CREATE TABLE ' . $previewSchema . '.marker (id serial PRIMARY KEY, v text NOT NULL)', + ); + $direct->insert('public.marker', ['v' => self::PUBLIC_MARKER]); + $direct->insert(self::PREVIEW_SCHEMA . '.marker', ['v' => self::PREVIEW_MARKER]); + $direct->close(); + } + + protected function setUp(): void + { + if (self::url('PGBOUNCER_DATABASE_URL') === null) { + self::markTestSkipped('PGBOUNCER_DATABASE_URL is not set; run via docker compose.'); + } + + $this->resetServerSessions(); + $this->resolver = new BaggageSchemaResolver('public', 'development', ['development'], new DebugLogger()); + SchemaConnection::setSchemaResolver($this->resolver); + } + + protected function tearDown(): void + { + foreach ($this->connections as $connection) { + $connection->close(); + } + + $this->connections = []; + SchemaConnection::setSearchPathMode(SearchPathMode::Session); + SchemaConnection::setLogger(new DebugLogger()); + } + + public function testSessionModeLeaksPreviewSchemaToDefaultClient(): void + { + $default = $this->connect(); + $preview = $this->connect(); + + self::assertSame(self::PUBLIC_MARKER, $this->readMarker($default, null)); + self::assertSame(self::PREVIEW_MARKER, $this->readMarker($preview, self::PREVIEW_SCHEMA)); + + self::assertSame( + self::PREVIEW_MARKER, + $this->readMarker($default, null), + 'Session SET search_path is expected to leak behind transaction pooling.', + ); + } + + public function testSessionModeKeepsSessionSearchPathAfterSavepointRollback(): void + { + $preview = $this->connect(); + + self::assertSame(self::PREVIEW_MARKER, $this->readMarker($preview, self::PREVIEW_SCHEMA)); + $preview->beginTransaction(); + $preview->executeStatement('SAVEPOINT raw_sp'); + $preview->executeStatement('ROLLBACK TO SAVEPOINT raw_sp'); + + self::assertSame(self::PREVIEW_MARKER, $preview->fetchOne(self::MARKER_QUERY)); + $preview->commit(); + } + + public function testLenientTransactionModeKeepsDefaultClientOnServerDefaultSchema(): void + { + SchemaConnection::setSearchPathMode(SearchPathMode::Transaction, 'public'); + $default = $this->connect(); + $preview = $this->connect(); + + self::assertSame(self::PUBLIC_MARKER, $this->readMarker($default, null)); + self::assertSame(self::PREVIEW_MARKER, $this->readMarker($preview, self::PREVIEW_SCHEMA)); + self::assertSame(self::PUBLIC_MARKER, $this->readMarker($default, null)); + self::assertSame('"$user", public', $default->fetchOne('SHOW search_path')); + } + + public function testStrictTransactionModeIgnoresSearchPathLeakedByASessionClient(): void + { + $this->leakSessionSearchPath(self::PREVIEW_SCHEMA); + SchemaConnection::setSearchPathMode(SearchPathMode::Transaction); + $default = $this->connect(); + + self::assertSame(self::PUBLIC_MARKER, $this->readMarker($default, null)); + } + + public function testLenientTransactionModeInheritsSearchPathLeakedByASessionClient(): void + { + $this->leakSessionSearchPath(self::PREVIEW_SCHEMA); + SchemaConnection::setSearchPathMode(SearchPathMode::Transaction, 'public'); + $default = $this->connect(); + + self::assertSame( + self::PREVIEW_MARKER, + $this->readMarker($default, null), + 'Skipping SET LOCAL for the server default relies on no client setting a session search_path.', + ); + } + + public function testTransactionModeAppliesSchemaToPreparedStatements(): void + { + SchemaConnection::setSearchPathMode(SearchPathMode::Transaction); + $default = $this->connect(); + $preview = $this->connect(); + $this->resolver->setSchema(self::PREVIEW_SCHEMA); + + $marker = $preview->prepare(self::MARKER_QUERY . ' WHERE id = ?')->executeQuery([1])->fetchOne(); + + self::assertSame(self::PREVIEW_MARKER, $marker); + self::assertSame(self::PUBLIC_MARKER, $this->readMarker($default, null)); + } + + public function testTransactionModeWritesExplicitTransactionToResolvedSchema(): void + { + SchemaConnection::setSearchPathMode(SearchPathMode::Transaction); + $preview = $this->connect(); + $this->resolver->setSchema(self::PREVIEW_SCHEMA); + + $preview->transactional(static function (Connection $connection): void { + $connection->insert('marker', ['v' => 'tx-write']); + $connection->executeStatement('UPDATE marker SET v = v WHERE v = ?', ['tx-write']); + }); + + self::assertSame(1, $this->countDirect(self::PREVIEW_SCHEMA, 'tx-write')); + self::assertSame(0, $this->countDirect('public', 'tx-write')); + } + + public function testTransactionModeReappliesSchemaAfterSavepointRollback(): void + { + SchemaConnection::setSearchPathMode(SearchPathMode::Transaction); + $preview = $this->connect(); + $preview->setNestTransactionsWithSavepoints(true); + + $this->resolver->setSchema(null); + $preview->beginTransaction(); + $preview->beginTransaction(); + self::assertSame(self::PREVIEW_MARKER, $this->readMarker($preview, self::PREVIEW_SCHEMA)); + $preview->rollBack(); + + self::assertSame(self::PREVIEW_MARKER, $preview->fetchOne(self::MARKER_QUERY)); + $preview->commit(); + } + + public function testTransactionModeRollsBackSavepointOfAbortedTransactionAfterSchemaChange(): void + { + SchemaConnection::setSearchPathMode(SearchPathMode::Transaction); + $preview = $this->connect(); + $preview->setNestTransactionsWithSavepoints(true); + $this->resolver->setSchema(self::PREVIEW_SCHEMA); + + $preview->beginTransaction(); + $preview->beginTransaction(); + + try { + $preview->fetchOne('SELECT missing_column FROM marker'); + self::fail('Query with a missing column must fail.'); + } catch (DBALException) { + } + + $this->resolver->setSchema(null); + $preview->rollBack(); + + self::assertSame(self::PUBLIC_MARKER, $preview->fetchOne(self::MARKER_QUERY)); + $preview->commit(); + } + + public function testTransactionModeKeepsRawTransactionControlStatements(): void + { + SchemaConnection::setSearchPathMode(SearchPathMode::Transaction); + $preview = $this->connect(); + $this->resolver->setSchema(self::PREVIEW_SCHEMA); + + $preview->executeStatement('START TRANSACTION'); + $preview->insert('marker', ['v' => 'raw-tx']); + $preview->executeStatement('ROLLBACK'); + + self::assertSame(0, $this->countDirect(self::PREVIEW_SCHEMA, 'raw-tx')); + self::assertSame(self::PREVIEW_MARKER, $preview->fetchOne(self::MARKER_QUERY)); + } + + public function testTransactionModeClosesImplicitTransactionWhenApplyFails(): void + { + SchemaConnection::setSearchPathMode(SearchPathMode::Transaction); + SchemaConnection::setLogger(new DebugLogger(self::failingApplyLogger())); + $preview = $this->connect(); + $this->resolver->setSchema(self::PREVIEW_SCHEMA); + + try { + $preview->fetchOne(self::MARKER_QUERY); + self::fail('The failing logger must abort the query.'); + } catch (RuntimeException) { + } + + self::assertSame(0, $this->countOpenServerTransactions()); + } + + public function testTransactionModeClosesExplicitTransactionWhenApplyFails(): void + { + SchemaConnection::setSearchPathMode(SearchPathMode::Transaction); + $preview = $this->connect(); + $preview->fetchOne('SELECT 1'); + SchemaConnection::setLogger(new DebugLogger(self::failingApplyLogger())); + $this->resolver->setSchema(self::PREVIEW_SCHEMA); + + try { + $preview->beginTransaction(); + self::fail('The failing logger must abort beginTransaction().'); + } catch (RuntimeException) { + } + + self::assertSame(0, $this->countOpenServerTransactions()); + } + + public function testTransactionModeRecoversAfterFailedStatement(): void + { + SchemaConnection::setSearchPathMode(SearchPathMode::Transaction); + $default = $this->connect(); + $preview = $this->connect(); + $this->resolver->setSchema(self::PREVIEW_SCHEMA); + + try { + $preview->fetchOne('SELECT missing_column FROM marker'); + self::fail('Query with a missing column must fail.'); + } catch (DBALException) { + } + + self::assertSame(self::PREVIEW_MARKER, $preview->fetchOne(self::MARKER_QUERY)); + self::assertSame(self::PUBLIC_MARKER, $this->readMarker($default, null)); + } + + private static function failingApplyLogger(): AbstractLogger + { + return new class extends AbstractLogger { + /** @param array $context */ + public function log($level, string|Stringable $message, array $context = []): void + { + if ((string) $message === 'schema_context_apply_search_path') { + throw new RuntimeException('Logger failure'); + } + } + }; + } + + private function countOpenServerTransactions(): int + { + $direct = DriverManager::getConnection(self::params('DIRECT_DATABASE_URL')); + $this->connections[] = $direct; + + return (int) $direct->fetchOne( + "SELECT count(*) FROM pg_stat_activity WHERE datname = current_database() AND state LIKE 'idle in%'", + ); + } + + private function leakSessionSearchPath(string $schema): void + { + $plain = DriverManager::getConnection(self::params('PGBOUNCER_DATABASE_URL')); + $this->connections[] = $plain; + $plain->executeStatement('SET search_path TO ' . $plain->quoteIdentifier($schema)); + } + + private function resetServerSessions(): void + { + $direct = DriverManager::getConnection(self::params('DIRECT_DATABASE_URL')); + $direct->executeQuery( + 'SELECT pg_terminate_backend(pid) FROM pg_stat_activity' + . ' WHERE datname = current_database() AND pid <> pg_backend_pid() AND backend_type = ?', + ['client backend'], + ); + $direct->close(); + } + + private function countDirect(string $schema, string $value): int + { + $direct = DriverManager::getConnection(self::params('DIRECT_DATABASE_URL')); + $this->connections[] = $direct; + + return (int) $direct->fetchOne( + 'SELECT count(*) FROM ' . $direct->quoteIdentifier($schema) . '.marker WHERE v = ?', + [$value], + ); + } + + private function readMarker(Connection $connection, ?string $schema): mixed + { + $this->resolver->setSchema($schema); + + return $connection->fetchOne(self::MARKER_QUERY); + } + + private function connect(): Connection + { + $params = self::params('PGBOUNCER_DATABASE_URL'); + $params['wrapperClass'] = SchemaConnection::class; + + $connection = DriverManager::getConnection($params); + $this->connections[] = $connection; + + return $connection; + } + + private static function url(string $name): ?string + { + $url = getenv($name); + + return is_string($url) && $url !== '' ? $url : null; + } + + /** + * @return array{driver: 'pdo_pgsql', host: string, port: int, user: string, password: string, dbname: string} + */ + private static function params(string $name): array + { + $parts = parse_url((string) self::url($name)); + + return [ + 'driver' => 'pdo_pgsql', + 'host' => (string) ($parts['host'] ?? ''), + 'port' => (int) ($parts['port'] ?? 5432), + 'user' => (string) ($parts['user'] ?? ''), + 'password' => (string) ($parts['pass'] ?? ''), + 'dbname' => ltrim((string) ($parts['path'] ?? ''), '/'), + ]; + } +} From 2deb35c1b47524ebb8b6d2e49ff4a7b5029a113d Mon Sep 17 00:00:00 2001 From: Vladislav Date: Fri, 6 Nov 2026 13:26:58 +0200 Subject: [PATCH 02/14] ci(PLT-13586): skip coverage in integration job --- .github/workflows/ci.yaml | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/.github/workflows/ci.yaml b/.github/workflows/ci.yaml index e682399..c0f236b 100644 --- a/.github/workflows/ci.yaml +++ b/.github/workflows/ci.yaml @@ -165,4 +165,4 @@ jobs: run: timeout 60 sh -c 'until pg_isready -h 127.0.0.1 -p 6432 -U app; do sleep 1; done' - name: Run integration tests - run: vendor/bin/phpunit --testsuite integration --fail-on-skipped + run: vendor/bin/phpunit --testsuite integration --fail-on-skipped --no-coverage From 9128bc11c0c5771db5a5735a2a36974af05acd98 Mon Sep 17 00:00:00 2001 From: Vladislav Date: Fri, 6 Nov 2026 13:32:17 +0200 Subject: [PATCH 03/14] ci(PLT-13586): run integration tests in the matrix job --- .github/workflows/ci.yaml | 111 ++++++++++++++++---------------------- README.md | 2 +- 2 files changed, 48 insertions(+), 65 deletions(-) diff --git a/.github/workflows/ci.yaml b/.github/workflows/ci.yaml index c0f236b..4cc6a90 100644 --- a/.github/workflows/ci.yaml +++ b/.github/workflows/ci.yaml @@ -47,6 +47,47 @@ jobs: doctrine-migrations: '^3.6' doctrine-fixtures-bundle: '^3.5' coverage: xdebug + - description: 'PostgreSQL 18' + php: '8.4' + symfony-versions: '7.4.*' + doctrine-orm-versions: '^3.0' + doctrine-migrations: '3.9.*' + doctrine-fixtures-bundle: '4.0.*' + postgres: '18' + + services: + postgres: + image: postgres:${{ matrix.postgres || '14' }}-alpine + env: + POSTGRES_USER: app + POSTGRES_PASSWORD: app + POSTGRES_DB: app + POSTGRES_HOST_AUTH_METHOD: scram-sha-256 + POSTGRES_INITDB_ARGS: --auth=scram-sha-256 + ports: + - 5432:5432 + options: >- + --health-cmd "pg_isready -U app" + --health-interval 2s + --health-timeout 5s + --health-retries 20 + pgbouncer: + image: edoburu/pgbouncer:v1.24.1-p1 + env: + DB_HOST: postgres + DB_USER: app + DB_PASSWORD: app + DB_NAME: app + AUTH_TYPE: scram-sha-256 + POOL_MODE: transaction + DEFAULT_POOL_SIZE: 1 + MAX_PREPARED_STATEMENTS: 200 + ports: + - 6432:5432 + + env: + DIRECT_DATABASE_URL: pgsql://app:app@127.0.0.1:5432/app + PGBOUNCER_DATABASE_URL: pgsql://app:app@127.0.0.1:6432/app name: PHP ${{ matrix.php }} Symfony ${{ matrix.symfony-versions }} Doctrine ${{ matrix.doctrine-orm-versions }} Doctrine migrations ${{ matrix.doctrine-migrations }} Doctrine fixtures bundle ${{ matrix.doctrine-fixtures-bundle }} ${{ matrix.description }} steps: @@ -62,6 +103,7 @@ jobs: uses: shivammathur/setup-php@v2 with: php-version: ${{ matrix.php }} + extensions: pdo_pgsql coverage: xdebug - name: Add PHPUnit matcher @@ -90,12 +132,15 @@ jobs: - name: Install dependencies run: composer install + - name: Wait for PgBouncer + run: timeout 60 sh -c 'until pg_isready -h 127.0.0.1 -p 6432 -U app; do sleep 1; done' + - name: Run PHPUnit tests - run: vendor/bin/phpunit + run: vendor/bin/phpunit --fail-on-skipped if: matrix.coverage == 'none' - name: PHPUnit tests and Log Code coverage - run: vendor/bin/phpunit --coverage-clover=coverage.xml + run: vendor/bin/phpunit --fail-on-skipped --coverage-clover=coverage.xml if: matrix.coverage == 'xdebug' - name: Upload coverage reports to Codecov @@ -104,65 +149,3 @@ jobs: with: token: ${{ secrets.CODECOV_TOKEN }} - integration: - name: Integration (PostgreSQL ${{ matrix.postgres }} + PgBouncer transaction pooling) - runs-on: ubuntu-latest - strategy: - fail-fast: false - matrix: - postgres: - - '14' - - '18' - services: - postgres: - image: postgres:${{ matrix.postgres }}-alpine - env: - POSTGRES_USER: app - POSTGRES_PASSWORD: app - POSTGRES_DB: app - POSTGRES_HOST_AUTH_METHOD: scram-sha-256 - POSTGRES_INITDB_ARGS: --auth=scram-sha-256 - ports: - - 5432:5432 - options: >- - --health-cmd "pg_isready -U app" - --health-interval 2s - --health-timeout 5s - --health-retries 20 - pgbouncer: - image: edoburu/pgbouncer:v1.24.1-p1 - env: - DB_HOST: postgres - DB_USER: app - DB_PASSWORD: app - DB_NAME: app - AUTH_TYPE: scram-sha-256 - POOL_MODE: transaction - DEFAULT_POOL_SIZE: 1 - MAX_PREPARED_STATEMENTS: 200 - ports: - - 6432:5432 - - env: - DIRECT_DATABASE_URL: pgsql://app:app@127.0.0.1:5432/app - PGBOUNCER_DATABASE_URL: pgsql://app:app@127.0.0.1:6432/app - - steps: - - name: Checkout - uses: actions/checkout@v4 - - - name: Setup PHP - uses: shivammathur/setup-php@v2 - with: - php-version: '8.3' - extensions: pdo_pgsql - coverage: none - - - name: Install dependencies - run: composer install --no-interaction --no-progress - - - name: Wait for PgBouncer - run: timeout 60 sh -c 'until pg_isready -h 127.0.0.1 -p 6432 -U app; do sleep 1; done' - - - name: Run integration tests - run: vendor/bin/phpunit --testsuite integration --fail-on-skipped --no-coverage diff --git a/README.md b/README.md index ca30f9c..14c9399 100644 --- a/README.md +++ b/README.md @@ -224,7 +224,7 @@ docker compose down ``` Without `DIRECT_DATABASE_URL` and `PGBOUNCER_DATABASE_URL` the integration tests are skipped. -CI runs them in the `integration` job against PostgreSQL 14 and 18. +CI runs them in every matrix job against PostgreSQL 14, and in one job against PostgreSQL 18. ## Contributing Feel free to open issues and submit pull requests. From ed96925de442ca02bbde0945afe3400e333a79ab Mon Sep 17 00:00:00 2001 From: Vladislav Date: Fri, 6 Nov 2026 13:43:54 +0200 Subject: [PATCH 04/14] ci(PLT-13586): add code coverage check and PR comment --- .github/workflows/ci.yaml | 30 ++++++++++++++++++++++++++++++ composer.json | 3 ++- 2 files changed, 32 insertions(+), 1 deletion(-) diff --git a/.github/workflows/ci.yaml b/.github/workflows/ci.yaml index 4cc6a90..897947d 100644 --- a/.github/workflows/ci.yaml +++ b/.github/workflows/ci.yaml @@ -8,6 +8,9 @@ on: jobs: run: runs-on: ubuntu-latest + permissions: + contents: read + pull-requests: write strategy: fail-fast: false matrix: @@ -143,6 +146,33 @@ jobs: run: vendor/bin/phpunit --fail-on-skipped --coverage-clover=coverage.xml if: matrix.coverage == 'xdebug' + - name: Generate code coverage status + id: code-coverage-analyze + if: matrix.coverage == 'xdebug' + run: | + exit_code=0 + coverage_result="$(vendor/bin/coverage-check coverage.xml "${MIN_CODE_COVERAGE}")" || exit_code=$? + echo "${coverage_result}" + echo "analyze-text=${coverage_result}" >> "${GITHUB_OUTPUT}" + exit "${exit_code}" + env: + MIN_CODE_COVERAGE: 80 + + - name: Create code coverage comment + if: >- + always() + && matrix.coverage == 'xdebug' + && github.event_name == 'pull_request' + && github.event.pull_request.head.repo.full_name == github.repository + && steps.code-coverage-analyze.outputs.analyze-text + uses: mshick/add-pr-comment@ec328af66588ab8f77cdeb2c264f14aba45bbf59 # v3.12.0 + with: + message-id: code-coverage-comment + refresh-message-position: true + message: | + [postgres-schema-bundle] (PHP ${{ matrix.php }}, Symfony ${{ matrix.symfony-versions }}) ${{ steps.code-coverage-analyze.outcome == 'success' && '✅' || '❌' }} ${{ steps.code-coverage-analyze.outputs.analyze-text }} + ${{ steps.code-coverage-analyze.outcome == 'success' && 'Well done, sailor! 🚣' || 'Please add more tests to your functionality :man_shrugging:' }} + - name: Upload coverage reports to Codecov if: matrix.coverage == 'xdebug' uses: codecov/codecov-action@v4.0.1 diff --git a/composer.json b/composer.json index 391b51d..2cc421f 100644 --- a/composer.json +++ b/composer.json @@ -26,7 +26,8 @@ "phpstan/phpstan": "^1.10", "phpunit/phpunit": "^10.0", "squizlabs/php_codesniffer": "^3.13", - "dg/bypass-finals": "^1.9" + "dg/bypass-finals": "^1.9", + "rregeer/phpunit-coverage-check": "^0.3" }, "config": { "allow-plugins": { From 4fd2a6bb4645805c8ae39a42cfc58674608030e4 Mon Sep 17 00:00:00 2001 From: Vladislav Date: Fri, 6 Nov 2026 14:13:58 +0200 Subject: [PATCH 05/14] fix(PLT-13586): address Copilot review comments --- .github/workflows/ci.yaml | 62 ++++++++++++++----- .../TransactionSearchPathConnection.php | 6 +- src/Doctrine/SchemaConnection.php | 8 ++- tests/Integration/SearchPathPgbouncerTest.php | 6 +- 4 files changed, 61 insertions(+), 21 deletions(-) diff --git a/.github/workflows/ci.yaml b/.github/workflows/ci.yaml index 897947d..97c3720 100644 --- a/.github/workflows/ci.yaml +++ b/.github/workflows/ci.yaml @@ -10,7 +10,6 @@ jobs: runs-on: ubuntu-latest permissions: contents: read - pull-requests: write strategy: fail-fast: false matrix: @@ -158,20 +157,30 @@ jobs: env: MIN_CODE_COVERAGE: 80 - - name: Create code coverage comment - if: >- - always() - && matrix.coverage == 'xdebug' - && github.event_name == 'pull_request' - && github.event.pull_request.head.repo.full_name == github.repository - && steps.code-coverage-analyze.outputs.analyze-text - uses: mshick/add-pr-comment@ec328af66588ab8f77cdeb2c264f14aba45bbf59 # v3.12.0 + - name: Prepare code coverage comment + if: always() && matrix.coverage == 'xdebug' && steps.code-coverage-analyze.outputs.analyze-text + run: | + { + echo "[postgres-schema-bundle] (PHP ${MATRIX_PHP}, Symfony ${MATRIX_SYMFONY}) ${STATUS_ICON} ${ANALYZE_TEXT}" + echo "${STATUS_TEXT}" + } > coverage-comment.md + env: + MATRIX_PHP: ${{ matrix.php }} + MATRIX_SYMFONY: ${{ matrix.symfony-versions }} + ANALYZE_TEXT: ${{ steps.code-coverage-analyze.outputs.analyze-text }} + STATUS_ICON: ${{ steps.code-coverage-analyze.outcome == 'success' && '✅' || '❌' }} + STATUS_TEXT: >- + ${{ steps.code-coverage-analyze.outcome == 'success' + && 'Well done, sailor! 🚣' + || 'Please add more tests to your functionality :man_shrugging:' }} + + - name: Upload code coverage comment + if: always() && matrix.coverage == 'xdebug' && steps.code-coverage-analyze.outputs.analyze-text + uses: actions/upload-artifact@v4 with: - message-id: code-coverage-comment - refresh-message-position: true - message: | - [postgres-schema-bundle] (PHP ${{ matrix.php }}, Symfony ${{ matrix.symfony-versions }}) ${{ steps.code-coverage-analyze.outcome == 'success' && '✅' || '❌' }} ${{ steps.code-coverage-analyze.outputs.analyze-text }} - ${{ steps.code-coverage-analyze.outcome == 'success' && 'Well done, sailor! 🚣' || 'Please add more tests to your functionality :man_shrugging:' }} + name: coverage-comment + path: coverage-comment.md + retention-days: 1 - name: Upload coverage reports to Codecov if: matrix.coverage == 'xdebug' @@ -179,3 +188,28 @@ jobs: with: token: ${{ secrets.CODECOV_TOKEN }} + coverage-comment: + name: Code coverage comment + needs: run + if: >- + always() + && github.event_name == 'pull_request' + && github.event.pull_request.head.repo.full_name == github.repository + runs-on: ubuntu-latest + permissions: + pull-requests: write + steps: + - name: Download code coverage comment + id: download + continue-on-error: true + uses: actions/download-artifact@v4 + with: + name: coverage-comment + + - name: Create code coverage comment + if: steps.download.outcome == 'success' + uses: mshick/add-pr-comment@ec328af66588ab8f77cdeb2c264f14aba45bbf59 # v3.12.0 + with: + message-id: code-coverage-comment + refresh-message-position: true + message-path: coverage-comment.md diff --git a/src/Doctrine/Middleware/TransactionSearchPathConnection.php b/src/Doctrine/Middleware/TransactionSearchPathConnection.php index acd58bc..0462666 100644 --- a/src/Doctrine/Middleware/TransactionSearchPathConnection.php +++ b/src/Doctrine/Middleware/TransactionSearchPathConnection.php @@ -63,7 +63,7 @@ public function exec(string $sql): int return $this->runWithSearchPath(fn (): int => parent::exec($sql)); } - public function beginTransaction() + public function beginTransaction(): bool { $result = parent::beginTransaction(); $this->startTransaction(); @@ -84,7 +84,7 @@ public function beginTransaction() return $result; } - public function commit() + public function commit(): bool { try { return parent::commit(); @@ -93,7 +93,7 @@ public function commit() } } - public function rollBack() + public function rollBack(): bool { try { return parent::rollBack(); diff --git a/src/Doctrine/SchemaConnection.php b/src/Doctrine/SchemaConnection.php index a56e643..815a475 100644 --- a/src/Doctrine/SchemaConnection.php +++ b/src/Doctrine/SchemaConnection.php @@ -156,9 +156,13 @@ private static function resolveTransactionSearchPath(): ?string return '"' . str_replace('"', '""', $schema) . '"'; } - private static function logTransactionSearchPath(string $quotedSchema, bool $isInTransaction): void + /** + * @param bool $isCallerTransaction true when SET LOCAL runs in a transaction opened by the caller, + * false when the bundle wraps a single statement in its own transaction + */ + private static function logTransactionSearchPath(string $quotedSchema, bool $isCallerTransaction): void { - self::$logger?->logApplySearchPath($quotedSchema, false, $isInTransaction); + self::$logger?->logApplySearchPath($quotedSchema, false, $isCallerTransaction); } private function getActualSearchPath(): ?string diff --git a/tests/Integration/SearchPathPgbouncerTest.php b/tests/Integration/SearchPathPgbouncerTest.php index 7fde2e5..fde556c 100644 --- a/tests/Integration/SearchPathPgbouncerTest.php +++ b/tests/Integration/SearchPathPgbouncerTest.php @@ -55,8 +55,10 @@ public static function setUpBeforeClass(): void protected function setUp(): void { - if (self::url('PGBOUNCER_DATABASE_URL') === null) { - self::markTestSkipped('PGBOUNCER_DATABASE_URL is not set; run via docker compose.'); + if (self::url('PGBOUNCER_DATABASE_URL') === null || self::url('DIRECT_DATABASE_URL') === null) { + self::markTestSkipped( + 'PGBOUNCER_DATABASE_URL and DIRECT_DATABASE_URL must be set; run via docker compose.', + ); } $this->resetServerSessions(); From fc9fa9b389d73cbf1734665a5abfb32bd8170740 Mon Sep 17 00:00:00 2001 From: Vladislav Date: Fri, 6 Nov 2026 14:13:58 +0200 Subject: [PATCH 06/14] chore(PLT-13586): add Copilot code review runner and instructions --- .github/copilot-instructions.md | 33 +++++++++++++++++++++++ .github/workflows/copilot-code-review.yml | 25 +++++++++++++++++ 2 files changed, 58 insertions(+) create mode 100644 .github/copilot-instructions.md create mode 100644 .github/workflows/copilot-code-review.yml diff --git a/.github/copilot-instructions.md b/.github/copilot-instructions.md new file mode 100644 index 0000000..cee520b --- /dev/null +++ b/.github/copilot-instructions.md @@ -0,0 +1,33 @@ +# Copilot review instructions + +`macpaw/postgres-schema-bundle` is a public Symfony bundle. It switches the PostgreSQL `search_path` per request +through a Doctrine DBAL wrapper connection (`SchemaConnection`). Services configure it from `Kernel::boot()`. + +## Compatibility + +- PHP `>=8.3`, Doctrine DBAL `^3.4`, ORM `^2.17 || ^3.0`, Symfony `^6.4 || ^7.4 || ^8.0`. +- DBAL 3 driver interfaces declare return types in docblocks only. Native return types that narrow + them in subclasses are fine; their absence is not a fatal error. + +## Behaviour that must not change + +- `SearchPathMode::Session` is the default. With it, every statement must pass through exactly as before + the transaction mode existed. Flag any change that touches statements in Session mode. +- In `SearchPathMode::Transaction` the schema is applied only with `SET LOCAL`, never with a session `SET`. + A session `SET` leaks to other clients behind PgBouncer in transaction pooling. +- Transaction control SQL (`BEGIN`, `COMMIT`, `ROLLBACK`, savepoints) must run without a `SET LOCAL` before it. + +## Intentional decisions (do not flag) + +- `BEGIN; SET LOCAL search_path TO ...` is sent in one `exec()` to save a round trip. Only `pdo_pgsql` is + supported, and the native `pgsql` driver is rejected. +- Mode, resolver and logger are static setters, matching how the bundle has always been configured. +- Statements outside a transaction commit before the result is fetched; this relies on `pdo_pgsql` buffering. + +## Code and tests + +- `declare(strict_types=1)`, PSR-12, max line length 120, PHPStan level max. +- Unit tests in `tests/Doctrine` and `tests/Command` use mocks. Integration tests in `tests/Integration` need + PostgreSQL behind PgBouncer in transaction pooling (`docker compose`, or the CI services) and must fail, + not skip, in CI. +- Every change to `src/Doctrine/Middleware` needs an integration test that fails without it. diff --git a/.github/workflows/copilot-code-review.yml b/.github/workflows/copilot-code-review.yml new file mode 100644 index 0000000..bad1983 --- /dev/null +++ b/.github/workflows/copilot-code-review.yml @@ -0,0 +1,25 @@ +name: Copilot code review runner + +on: + workflow_dispatch: + +permissions: + contents: read + +jobs: + copilot-setup-steps: + runs-on: ubuntu-latest + timeout-minutes: 15 + steps: + - name: Checkout + uses: actions/checkout@v4 + + - name: Setup PHP + uses: shivammathur/setup-php@v2 + with: + php-version: '8.3' + extensions: pdo_pgsql + coverage: none + + - name: Install dependencies + run: composer install --no-interaction --no-progress From ae253c65ffa7ba883111605049490f3f83d8cdbc Mon Sep 17 00:00:00 2001 From: Vladislav Date: Fri, 6 Nov 2026 14:16:08 +0200 Subject: [PATCH 07/14] ci(PLT-13586): update actions/checkout to v4 --- .github/workflows/security.yaml | 2 +- .github/workflows/static-analysis.yaml | 6 +++--- 2 files changed, 4 insertions(+), 4 deletions(-) diff --git a/.github/workflows/security.yaml b/.github/workflows/security.yaml index 248d5d0..4cad68c 100644 --- a/.github/workflows/security.yaml +++ b/.github/workflows/security.yaml @@ -9,7 +9,7 @@ jobs: runs-on: ubuntu-latest steps: - name: Checkout - uses: actions/checkout@v2 + uses: actions/checkout@v4 - name: Setup PHP uses: shivammathur/setup-php@v2 diff --git a/.github/workflows/static-analysis.yaml b/.github/workflows/static-analysis.yaml index 8796ae1..209e35a 100644 --- a/.github/workflows/static-analysis.yaml +++ b/.github/workflows/static-analysis.yaml @@ -11,7 +11,7 @@ jobs: runs-on: ubuntu-latest steps: - name: Checkout - uses: actions/checkout@v2 + uses: actions/checkout@v4 - name: Setup PHP uses: shivammathur/setup-php@v2 @@ -27,7 +27,7 @@ jobs: runs-on: ubuntu-latest steps: - name: Checkout - uses: actions/checkout@v2 + uses: actions/checkout@v4 - name: Setup PHP uses: shivammathur/setup-php@v2 @@ -43,7 +43,7 @@ jobs: runs-on: ubuntu-latest steps: - name: Checkout - uses: actions/checkout@v2 + uses: actions/checkout@v4 - name: Setup PHP uses: shivammathur/setup-php@v2 From 7e5573a8bdece76c67c8989f14f264e953167892 Mon Sep 17 00:00:00 2001 From: Vladislav Date: Fri, 6 Nov 2026 14:59:34 +0200 Subject: [PATCH 08/14] fix(PLT-13586): address second Copilot review --- .github/workflows/ci.yaml | 1 + .../TransactionSearchPathConnection.php | 12 ++++++++++- tests/Integration/SearchPathPgbouncerTest.php | 20 +++++++++++++++++++ 3 files changed, 32 insertions(+), 1 deletion(-) diff --git a/.github/workflows/ci.yaml b/.github/workflows/ci.yaml index 97c3720..f182ded 100644 --- a/.github/workflows/ci.yaml +++ b/.github/workflows/ci.yaml @@ -55,6 +55,7 @@ jobs: doctrine-orm-versions: '^3.0' doctrine-migrations: '3.9.*' doctrine-fixtures-bundle: '4.0.*' + coverage: 'none' postgres: '18' services: diff --git a/src/Doctrine/Middleware/TransactionSearchPathConnection.php b/src/Doctrine/Middleware/TransactionSearchPathConnection.php index 0462666..539de91 100644 --- a/src/Doctrine/Middleware/TransactionSearchPathConnection.php +++ b/src/Doctrine/Middleware/TransactionSearchPathConnection.php @@ -56,6 +56,10 @@ public function query(string $sql): Result public function exec(string $sql): int { + if (!($this->isActive)()) { + return parent::exec($sql); + } + if ($this->isTransactionControl($sql)) { return $this->execTransactionControl($sql); } @@ -167,7 +171,13 @@ private function runInImplicitTransaction(string $schema, Closure $operation): m throw $exception; } - parent::exec('COMMIT'); + try { + parent::exec('COMMIT'); + } catch (Throwable $exception) { + $this->rollBackQuietly(); + + throw $exception; + } return $result; } diff --git a/tests/Integration/SearchPathPgbouncerTest.php b/tests/Integration/SearchPathPgbouncerTest.php index fde556c..20dca11 100644 --- a/tests/Integration/SearchPathPgbouncerTest.php +++ b/tests/Integration/SearchPathPgbouncerTest.php @@ -48,6 +48,10 @@ public static function setUpBeforeClass(): void $direct->executeStatement( 'CREATE TABLE ' . $previewSchema . '.marker (id serial PRIMARY KEY, v text NOT NULL)', ); + $direct->executeStatement( + 'CREATE TABLE ' . $previewSchema . '.deferred_unique' + . ' (v text, CONSTRAINT deferred_unique_v UNIQUE (v) DEFERRABLE INITIALLY DEFERRED)', + ); $direct->insert('public.marker', ['v' => self::PUBLIC_MARKER]); $direct->insert(self::PREVIEW_SCHEMA . '.marker', ['v' => self::PREVIEW_MARKER]); $direct->close(); @@ -253,6 +257,22 @@ public function testTransactionModeClosesExplicitTransactionWhenApplyFails(): vo self::assertSame(0, $this->countOpenServerTransactions()); } + public function testTransactionModeRecoversWhenImplicitCommitFails(): void + { + SchemaConnection::setSearchPathMode(SearchPathMode::Transaction); + $preview = $this->connect(); + $this->resolver->setSchema(self::PREVIEW_SCHEMA); + + try { + $preview->executeStatement("INSERT INTO deferred_unique (v) VALUES ('dup'), ('dup')"); + self::fail('The deferred unique constraint must fail on COMMIT.'); + } catch (DBALException) { + } + + self::assertSame(0, $this->countOpenServerTransactions()); + self::assertSame(self::PREVIEW_MARKER, $preview->fetchOne(self::MARKER_QUERY)); + } + public function testTransactionModeRecoversAfterFailedStatement(): void { SchemaConnection::setSearchPathMode(SearchPathMode::Transaction); From 96f8c8b59748f7cf3d6fc00e39fd06898981e12b Mon Sep 17 00:00:00 2001 From: Vladislav Date: Fri, 6 Nov 2026 15:05:57 +0200 Subject: [PATCH 09/14] fix(PLT-13586): harden transaction control tracking and driver check --- README.md | 7 +- .../TransactionSearchPathConnection.php | 76 +++++++++++++------ .../TransactionSearchPathDriver.php | 19 +++-- src/Exception/UnsupportedDriverException.php | 2 +- tests/Doctrine/SchemaConnectionTest.php | 16 ++-- tests/Integration/SearchPathPgbouncerTest.php | 26 +++++++ 6 files changed, 108 insertions(+), 38 deletions(-) diff --git a/README.md b/README.md index 14c9399..6274bd3 100644 --- a/README.md +++ b/README.md @@ -101,7 +101,9 @@ In transaction mode: `ROLLBACK TO SAVEPOINT`. - Plain queries, executed statements and prepared statements (Doctrine ORM) are all covered. - The schema commands below only set the schema in the resolver; they do not issue a session `SET`. -- Transaction control SQL (`BEGIN`, `COMMIT`, `ROLLBACK`, savepoints) runs as is, without `SET LOCAL` before it. +- Transaction control SQL (`BEGIN`, `COMMIT`, `ROLLBACK`, savepoints) sent with `executeStatement()` or + `executeQuery()` runs as is, without `SET LOCAL` before it. Send it as a plain statement, not as a prepared + statement with parameters. - In the default `Session` mode statements pass through unchanged. Choosing `$serverDefaultSchema`: @@ -119,6 +121,9 @@ Limitations: - Only the `pdo_pgsql` driver is supported. It prepares a statement when it is executed, inside `SET LOCAL`. The native `pgsql` driver prepares immediately, against the server default `search_path`, so it is rejected with `UnsupportedDriverException`. +- `pdo_pgsql` uses server-side prepared statements, and a statement reused outside a transaction can run on + another server connection. PgBouncer must track prepared statements: version 1.21+ with + `max_prepared_statements` greater than 0 (the default since 1.22). - A statement outside a transaction is committed before its result is fetched. This relies on `pdo_pgsql` buffering the whole result (the default); do not enable lazy (unbuffered) result fetching. - Statements that cannot run inside a transaction block (`CREATE INDEX CONCURRENTLY`, `VACUUM`, migrations with diff --git a/src/Doctrine/Middleware/TransactionSearchPathConnection.php b/src/Doctrine/Middleware/TransactionSearchPathConnection.php index 539de91..b9ec1b5 100644 --- a/src/Doctrine/Middleware/TransactionSearchPathConnection.php +++ b/src/Doctrine/Middleware/TransactionSearchPathConnection.php @@ -15,14 +15,21 @@ * Applies search_path with SET LOCAL, so it never outlives the transaction and cannot leak to other clients * through a PgBouncer server connection in transaction pooling mode. * - * While $isActive returns false the connection passes every call through unchanged. + * While $isActive returns false every statement is sent unchanged; only transaction control SQL is tracked. */ final class TransactionSearchPathConnection extends AbstractConnectionMiddleware { - private const BEGIN_PATTERN = '/^\s*(BEGIN|START\s+TRANSACTION)\b/i'; - private const END_PATTERN = '/^\s*(COMMIT|END|ABORT|ROLLBACK(?!\s+TO\b))\b/i'; - private const SAVEPOINT_PATTERN = '/^\s*(SAVEPOINT|RELEASE|ROLLBACK\s+TO)\b/i'; - private const ROLLBACK_TO_PATTERN = '/^\s*ROLLBACK\s+TO\b/i'; + /** Leading whitespace and SQL comments before the first keyword. */ + private const PREFIX = '/^(?:\s+|\/\*.*?\*\/|--[^\n]*(?:\n|$))*'; + private const BEGIN_PATTERN = self::PREFIX . '(?:BEGIN|START\s+TRANSACTION)\b/is'; + private const ROLLBACK_TO_PATTERN = self::PREFIX . 'ROLLBACK\s+(?:(?:WORK|TRANSACTION)\s+)?TO\b/is'; + private const END_PATTERN = self::PREFIX . '(?:COMMIT|END|ABORT|ROLLBACK)\b/is'; + private const SAVEPOINT_PATTERN = self::PREFIX . '(?:SAVEPOINT|RELEASE)\b/is'; + + private const CONTROL_BEGIN = 'begin'; + private const CONTROL_END = 'end'; + private const CONTROL_ROLLBACK_TO = 'rollback_to'; + private const CONTROL_SAVEPOINT = 'savepoint'; private bool $inTransaction = false; private ?string $appliedSchema = null; @@ -51,17 +58,21 @@ public function prepare(string $sql): Statement public function query(string $sql): Result { + $control = $this->classifyTransactionControl($sql); + + if ($control !== null) { + return $this->runTransactionControl($control, fn (): Result => parent::query($sql)); + } + return $this->runWithSearchPath(fn (): Result => parent::query($sql)); } public function exec(string $sql): int { - if (!($this->isActive)()) { - return parent::exec($sql); - } + $control = $this->classifyTransactionControl($sql); - if ($this->isTransactionControl($sql)) { - return $this->execTransactionControl($sql); + if ($control !== null) { + return $this->runTransactionControl($control, fn (): int => parent::exec($sql)); } return $this->runWithSearchPath(fn (): int => parent::exec($sql)); @@ -182,29 +193,44 @@ private function runInImplicitTransaction(string $schema, Closure $operation): m return $result; } - private function isTransactionControl(string $sql): bool + /** + * @return self::CONTROL_*|null + */ + private function classifyTransactionControl(string $sql): ?string { - return preg_match(self::BEGIN_PATTERN, $sql) === 1 - || preg_match(self::END_PATTERN, $sql) === 1 - || ($this->inTransaction && preg_match(self::SAVEPOINT_PATTERN, $sql) === 1); + return match (true) { + preg_match(self::BEGIN_PATTERN, $sql) === 1 => self::CONTROL_BEGIN, + preg_match(self::ROLLBACK_TO_PATTERN, $sql) === 1 => self::CONTROL_ROLLBACK_TO, + preg_match(self::END_PATTERN, $sql) === 1 => self::CONTROL_END, + preg_match(self::SAVEPOINT_PATTERN, $sql) === 1 => self::CONTROL_SAVEPOINT, + default => null, + }; } /** - * Transaction control SQL runs as is: no SET LOCAL before it (it would fail in an aborted transaction - * and block the recovery) and no implicit transaction around it (it would end the caller's transaction). + * Transaction control SQL runs as is in every mode: no SET LOCAL before it (it would fail in an aborted + * transaction and block the recovery) and no implicit transaction around it (it would end the caller's + * transaction). Its effect is tracked even while the mode is inactive, so the state stays correct + * if the mode is switched on later. + * + * @template T + * + * @param self::CONTROL_* $control + * @param Closure(): T $operation + * + * @return T */ - private function execTransactionControl(string $sql): int + private function runTransactionControl(string $control, Closure $operation): mixed { - $result = parent::exec($sql); + $result = $operation(); - if (preg_match(self::BEGIN_PATTERN, $sql) === 1) { - $this->startTransaction(); - } elseif (preg_match(self::END_PATTERN, $sql) === 1) { - $this->endTransaction(); - } elseif (preg_match(self::ROLLBACK_TO_PATTERN, $sql) === 1) { + match ($control) { + self::CONTROL_BEGIN => $this->startTransaction(), + self::CONTROL_END => $this->endTransaction(), // A SET LOCAL issued after the savepoint is undone with it, so re-apply before the next query. - $this->isAppliedSchemaKnown = false; - } + self::CONTROL_ROLLBACK_TO => $this->isAppliedSchemaKnown = false, + self::CONTROL_SAVEPOINT => null, + }; return $result; } diff --git a/src/Doctrine/Middleware/TransactionSearchPathDriver.php b/src/Doctrine/Middleware/TransactionSearchPathDriver.php index 517483b..f5eac4f 100644 --- a/src/Doctrine/Middleware/TransactionSearchPathDriver.php +++ b/src/Doctrine/Middleware/TransactionSearchPathDriver.php @@ -7,8 +7,8 @@ use Closure; use Doctrine\DBAL\Driver; use Doctrine\DBAL\Driver\Middleware\AbstractDriverMiddleware; -use Doctrine\DBAL\Driver\PgSQL\Driver as NativePgSQLDriver; use Macpaw\PostgresSchemaBundle\Exception\UnsupportedDriverException; +use PDO; use SensitiveParameter; final class TransactionSearchPathDriver extends AbstractDriverMiddleware @@ -21,7 +21,7 @@ final class TransactionSearchPathDriver extends AbstractDriverMiddleware * the transaction was opened by the caller */ public function __construct( - private readonly Driver $wrappedDriver, + Driver $wrappedDriver, private readonly Closure $isActive, private readonly Closure $schemaProvider, private readonly Closure $onApply, @@ -31,13 +31,20 @@ public function __construct( public function connect(#[SensitiveParameter] array $params): Driver\Connection { - // The native pgsql driver prepares statements immediately, before SET LOCAL is applied. - if (($this->isActive)() && $this->wrappedDriver instanceof NativePgSQLDriver) { - throw new UnsupportedDriverException('pgsql'); + $connection = parent::connect($params); + + // Only pdo_pgsql defers statement preparation to execution, inside SET LOCAL. The check looks at the + // native connection, so it also works when other driver middlewares wrap the driver. + if (($this->isActive)()) { + $native = method_exists($connection, 'getNativeConnection') ? $connection->getNativeConnection() : null; + + if (!$native instanceof PDO) { + throw new UnsupportedDriverException(get_debug_type($native)); + } } return new TransactionSearchPathConnection( - parent::connect($params), + $connection, $this->isActive, $this->schemaProvider, $this->onApply, diff --git a/src/Exception/UnsupportedDriverException.php b/src/Exception/UnsupportedDriverException.php index 2c33487..fe33d47 100644 --- a/src/Exception/UnsupportedDriverException.php +++ b/src/Exception/UnsupportedDriverException.php @@ -11,7 +11,7 @@ class UnsupportedDriverException extends RuntimeException public function __construct(string $driver) { parent::__construct( - "Unsupported database driver: {$driver}. Transaction search_path mode requires pdo_pgsql.", + "Unsupported database connection: {$driver}. Transaction search_path mode requires pdo_pgsql.", ); } } diff --git a/tests/Doctrine/SchemaConnectionTest.php b/tests/Doctrine/SchemaConnectionTest.php index c2befdc..b86c833 100644 --- a/tests/Doctrine/SchemaConnectionTest.php +++ b/tests/Doctrine/SchemaConnectionTest.php @@ -8,7 +8,7 @@ use Doctrine\DBAL\Configuration; use Doctrine\DBAL\Driver; use Doctrine\DBAL\Driver\Connection as DriverConnection; -use Doctrine\DBAL\Driver\PgSQL\Driver as NativePgSQLDriver; +use Doctrine\DBAL\Driver\Middleware\AbstractConnectionMiddleware; use Doctrine\DBAL\Platforms\PostgreSQLPlatform; use Doctrine\DBAL\Platforms\MySQLPlatform; use Macpaw\PostgresSchemaBundle\Doctrine\SchemaConnection; @@ -17,7 +17,9 @@ use Macpaw\PostgresSchemaBundle\Exception\UnsupportedPlatformException; use Macpaw\SchemaContextBundle\Logger\DebugLogger; use Macpaw\SchemaContextBundle\Service\BaggageSchemaResolver; +use PDO; use PHPUnit\Framework\TestCase; +use stdClass; class SchemaConnectionTest extends TestCase { @@ -165,7 +167,8 @@ public function testThrowsForUnsupportedPlatform(): void public function testTransactionModeThrowsForUnsupportedPlatform(): void { - $driverConnection = $this->createMock(DriverConnection::class); + $driverConnection = $this->createMock(AbstractConnectionMiddleware::class); + $driverConnection->method('getNativeConnection')->willReturn($this->createMock(PDO::class)); $driverConnection->expects($this->never())->method('exec'); $driver = $this->createMock(Driver::class); $driver->method('connect')->willReturn($driverConnection); @@ -189,10 +192,13 @@ public function testTransactionModeThrowsForUnsupportedPlatform(): void } } - public function testTransactionModeRejectsNativePgsqlDriver(): void + public function testTransactionModeRejectsNonPdoConnection(): void { - $driver = $this->createMock(NativePgSQLDriver::class); - $driver->expects($this->never())->method('connect'); + $driverConnection = $this->createMock(AbstractConnectionMiddleware::class); + $driverConnection->method('getNativeConnection')->willReturn(new stdClass()); + $driverConnection->expects($this->never())->method('exec'); + $driver = $this->createMock(Driver::class); + $driver->method('connect')->willReturn($driverConnection); $connection = new SchemaConnection([], $driver, new Configuration(), new EventManager()); SchemaConnection::setSchemaResolver( diff --git a/tests/Integration/SearchPathPgbouncerTest.php b/tests/Integration/SearchPathPgbouncerTest.php index 20dca11..af63612 100644 --- a/tests/Integration/SearchPathPgbouncerTest.php +++ b/tests/Integration/SearchPathPgbouncerTest.php @@ -224,6 +224,32 @@ public function testTransactionModeKeepsRawTransactionControlStatements(): void self::assertSame(self::PREVIEW_MARKER, $preview->fetchOne(self::MARKER_QUERY)); } + public function testTransactionModeKeepsRawTransactionControlSentThroughQueryWithComments(): void + { + SchemaConnection::setSearchPathMode(SearchPathMode::Transaction); + $preview = $this->connect(); + $this->resolver->setSchema(self::PREVIEW_SCHEMA); + + $preview->executeQuery('/* app */ BEGIN'); + $preview->insert('marker', ['v' => 'raw-query-tx']); + $preview->executeQuery("-- app\nROLLBACK"); + + self::assertSame(0, $this->countDirect(self::PREVIEW_SCHEMA, 'raw-query-tx')); + } + + public function testRawTransactionStartedBeforeTransactionModeIsSwitchedOnStaysOpen(): void + { + $preview = $this->connect(); + $preview->executeStatement('START TRANSACTION'); + SchemaConnection::setSearchPathMode(SearchPathMode::Transaction); + $this->resolver->setSchema(self::PREVIEW_SCHEMA); + + $preview->insert('marker', ['v' => 'late-mode']); + $preview->executeStatement('ROLLBACK'); + + self::assertSame(0, $this->countDirect(self::PREVIEW_SCHEMA, 'late-mode')); + } + public function testTransactionModeClosesImplicitTransactionWhenApplyFails(): void { SchemaConnection::setSearchPathMode(SearchPathMode::Transaction); From 29f0fc9d4953007ed60badfd50c4fb6c63e9e4dd Mon Sep 17 00:00:00 2001 From: Vladislav Date: Fri, 6 Nov 2026 16:05:53 +0200 Subject: [PATCH 10/14] fix(PLT-13586): handle chained, failed and batched transaction control --- README.md | 4 +- .../TransactionSearchPathConnection.php | 41 +++++++++-- .../TransactionControlBatchException.php | 18 +++++ tests/Integration/SearchPathPgbouncerTest.php | 69 +++++++++++++++++++ 4 files changed, 126 insertions(+), 6 deletions(-) create mode 100644 src/Exception/TransactionControlBatchException.php diff --git a/README.md b/README.md index 6274bd3..97634a1 100644 --- a/README.md +++ b/README.md @@ -103,7 +103,9 @@ In transaction mode: - The schema commands below only set the schema in the resolver; they do not issue a session `SET`. - Transaction control SQL (`BEGIN`, `COMMIT`, `ROLLBACK`, savepoints) sent with `executeStatement()` or `executeQuery()` runs as is, without `SET LOCAL` before it. Send it as a plain statement, not as a prepared - statement with parameters. + statement with parameters. `COMMIT AND CHAIN` and `ROLLBACK AND CHAIN` keep the transaction open. +- Transaction control batched with other statements (`BEGIN; INSERT ...; COMMIT`) is rejected with + `TransactionControlBatchException`: the statements after the first one would run without `SET LOCAL`. - In the default `Session` mode statements pass through unchanged. Choosing `$serverDefaultSchema`: diff --git a/src/Doctrine/Middleware/TransactionSearchPathConnection.php b/src/Doctrine/Middleware/TransactionSearchPathConnection.php index b9ec1b5..fa676fc 100644 --- a/src/Doctrine/Middleware/TransactionSearchPathConnection.php +++ b/src/Doctrine/Middleware/TransactionSearchPathConnection.php @@ -9,6 +9,7 @@ use Doctrine\DBAL\Driver\Middleware\AbstractConnectionMiddleware; use Doctrine\DBAL\Driver\Result; use Doctrine\DBAL\Driver\Statement; +use Macpaw\PostgresSchemaBundle\Exception\TransactionControlBatchException; use Throwable; /** @@ -19,15 +20,21 @@ */ final class TransactionSearchPathConnection extends AbstractConnectionMiddleware { + /** String literals, quoted identifiers, dollar-quoted bodies and comments, removed before looking for ';'. */ + private const NON_CODE_PATTERN = '/\'(?:\'\'|[^\'])*\'|"(?:""|[^"])*"|\$(\w*)\$.*?\$\1\$|\/\*.*?\*\/|--[^\n]*/s'; + /** Leading whitespace and SQL comments before the first keyword. */ private const PREFIX = '/^(?:\s+|\/\*.*?\*\/|--[^\n]*(?:\n|$))*'; private const BEGIN_PATTERN = self::PREFIX . '(?:BEGIN|START\s+TRANSACTION)\b/is'; private const ROLLBACK_TO_PATTERN = self::PREFIX . 'ROLLBACK\s+(?:(?:WORK|TRANSACTION)\s+)?TO\b/is'; + private const CHAIN_PATTERN = self::PREFIX + . '(?:COMMIT|END|ABORT|ROLLBACK)(?:\s+(?:WORK|TRANSACTION))?\s+AND\s+CHAIN\b/is'; private const END_PATTERN = self::PREFIX . '(?:COMMIT|END|ABORT|ROLLBACK)\b/is'; private const SAVEPOINT_PATTERN = self::PREFIX . '(?:SAVEPOINT|RELEASE)\b/is'; private const CONTROL_BEGIN = 'begin'; private const CONTROL_END = 'end'; + private const CONTROL_CHAIN = 'chain'; private const CONTROL_ROLLBACK_TO = 'rollback_to'; private const CONTROL_SAVEPOINT = 'savepoint'; @@ -61,7 +68,7 @@ public function query(string $sql): Result $control = $this->classifyTransactionControl($sql); if ($control !== null) { - return $this->runTransactionControl($control, fn (): Result => parent::query($sql)); + return $this->runTransactionControl($control, $sql, fn (): Result => parent::query($sql)); } return $this->runWithSearchPath(fn (): Result => parent::query($sql)); @@ -72,7 +79,7 @@ public function exec(string $sql): int $control = $this->classifyTransactionControl($sql); if ($control !== null) { - return $this->runTransactionControl($control, fn (): int => parent::exec($sql)); + return $this->runTransactionControl($control, $sql, fn (): int => parent::exec($sql)); } return $this->runWithSearchPath(fn (): int => parent::exec($sql)); @@ -201,6 +208,7 @@ private function classifyTransactionControl(string $sql): ?string return match (true) { preg_match(self::BEGIN_PATTERN, $sql) === 1 => self::CONTROL_BEGIN, preg_match(self::ROLLBACK_TO_PATTERN, $sql) === 1 => self::CONTROL_ROLLBACK_TO, + preg_match(self::CHAIN_PATTERN, $sql) === 1 => self::CONTROL_CHAIN, preg_match(self::END_PATTERN, $sql) === 1 => self::CONTROL_END, preg_match(self::SAVEPOINT_PATTERN, $sql) === 1 => self::CONTROL_SAVEPOINT, default => null, @@ -220,12 +228,28 @@ private function classifyTransactionControl(string $sql): ?string * * @return T */ - private function runTransactionControl(string $control, Closure $operation): mixed + private function runTransactionControl(string $control, string $sql, Closure $operation): mixed { - $result = $operation(); + if (($this->isActive)() && $this->isBatch($sql)) { + // Statements after the first one would run without SET LOCAL and outside the tracked state. + throw new TransactionControlBatchException(); + } + + try { + $result = $operation(); + } catch (Throwable $exception) { + // A failed COMMIT/ROLLBACK still ends the transaction block; the savepoint state is unknown. + if ($control === self::CONTROL_END || $control === self::CONTROL_CHAIN) { + $this->endTransaction(); + } elseif ($control === self::CONTROL_ROLLBACK_TO) { + $this->isAppliedSchemaKnown = false; + } + + throw $exception; + } match ($control) { - self::CONTROL_BEGIN => $this->startTransaction(), + self::CONTROL_BEGIN, self::CONTROL_CHAIN => $this->startTransaction(), self::CONTROL_END => $this->endTransaction(), // A SET LOCAL issued after the savepoint is undone with it, so re-apply before the next query. self::CONTROL_ROLLBACK_TO => $this->isAppliedSchemaKnown = false, @@ -235,6 +259,13 @@ private function runTransactionControl(string $control, Closure $operation): mix return $result; } + private function isBatch(string $sql): bool + { + $code = (string) preg_replace(self::NON_CODE_PATTERN, '', $sql); + + return str_contains(rtrim($code, " \t\n\r\0\x0B;"), ';'); + } + private function startTransaction(): void { $this->inTransaction = true; diff --git a/src/Exception/TransactionControlBatchException.php b/src/Exception/TransactionControlBatchException.php new file mode 100644 index 0000000..7ea4e71 --- /dev/null +++ b/src/Exception/TransactionControlBatchException.php @@ -0,0 +1,18 @@ +countDirect(self::PREVIEW_SCHEMA, 'raw-query-tx')); } + public function testTransactionModeRejectsTransactionControlBatch(): void + { + SchemaConnection::setSearchPathMode(SearchPathMode::Transaction); + $preview = $this->connect(); + $this->resolver->setSchema(self::PREVIEW_SCHEMA); + + try { + $preview->executeStatement("BEGIN; INSERT INTO marker (v) VALUES ('batch'); COMMIT"); + self::fail('A transaction control batch must be rejected.'); + } catch (TransactionControlBatchException) { + } + + self::assertSame(0, $this->countDirect(self::PREVIEW_SCHEMA, 'batch')); + self::assertSame(0, $this->countDirect('public', 'batch')); + self::assertSame(self::PREVIEW_MARKER, $preview->fetchOne(self::MARKER_QUERY)); + } + + public function testTransactionModeAppliesSchemaAfterFailedRawCommit(): void + { + SchemaConnection::setSearchPathMode(SearchPathMode::Transaction); + $preview = $this->connect(); + $this->resolver->setSchema(self::PREVIEW_SCHEMA); + + $preview->executeStatement('START TRANSACTION'); + $preview->executeStatement("INSERT INTO deferred_unique (v) VALUES ('raw-dup'), ('raw-dup')"); + + try { + $preview->executeStatement('COMMIT'); + self::fail('The deferred unique constraint must fail on COMMIT.'); + } catch (DBALException) { + } + + self::assertSame(self::PREVIEW_MARKER, $preview->fetchOne(self::MARKER_QUERY)); + self::assertSame(0, $this->countOpenServerTransactions()); + } + + public function testTransactionModeKeepsTransactionOpenAfterCommitAndChain(): void + { + SchemaConnection::setSearchPathMode(SearchPathMode::Transaction); + $preview = $this->connect(); + $this->resolver->setSchema(self::PREVIEW_SCHEMA); + + $preview->executeStatement('START TRANSACTION'); + $preview->insert('marker', ['v' => 'chain-commit-1']); + $preview->executeStatement('COMMIT AND CHAIN'); + $preview->insert('marker', ['v' => 'chain-commit-2']); + $preview->executeStatement('ROLLBACK'); + + self::assertSame(1, $this->countDirect(self::PREVIEW_SCHEMA, 'chain-commit-1')); + self::assertSame(0, $this->countDirect(self::PREVIEW_SCHEMA, 'chain-commit-2')); + } + + public function testTransactionModeKeepsTransactionOpenAfterRollbackAndChain(): void + { + SchemaConnection::setSearchPathMode(SearchPathMode::Transaction); + $preview = $this->connect(); + $this->resolver->setSchema(self::PREVIEW_SCHEMA); + + $preview->executeStatement('START TRANSACTION'); + $preview->insert('marker', ['v' => 'chain-rollback-1']); + $preview->executeStatement('ROLLBACK AND CHAIN'); + $preview->insert('marker', ['v' => 'chain-rollback-2']); + $preview->executeStatement('ROLLBACK'); + + self::assertSame(0, $this->countDirect(self::PREVIEW_SCHEMA, 'chain-rollback-1')); + self::assertSame(0, $this->countDirect(self::PREVIEW_SCHEMA, 'chain-rollback-2')); + } + public function testRawTransactionStartedBeforeTransactionModeIsSwitchedOnStaysOpen(): void { $preview = $this->connect(); From 087d32bb686f39bd1ddced993fa4602aaa5c7124 Mon Sep 17 00:00:00 2001 From: Vladislav Date: Fri, 6 Nov 2026 16:19:09 +0200 Subject: [PATCH 11/14] refactor(PLT-13586): simplify transaction search_path mode --- README.md | 14 +- .../TransactionSearchPathConnection.php | 158 ++++++++---------- .../TransactionSearchPathDriver.php | 17 +- src/Doctrine/SchemaConnection.php | 35 +++- .../TransactionControlBatchException.php | 18 -- .../TransactionSearchPathModeException.php | 26 +++ tests/Doctrine/SchemaConnectionTest.php | 16 +- tests/Integration/SearchPathPgbouncerTest.php | 124 +++++++------- 8 files changed, 217 insertions(+), 191 deletions(-) delete mode 100644 src/Exception/TransactionControlBatchException.php create mode 100644 src/Exception/TransactionSearchPathModeException.php diff --git a/README.md b/README.md index 97634a1..4d055f6 100644 --- a/README.md +++ b/README.md @@ -101,12 +101,14 @@ In transaction mode: `ROLLBACK TO SAVEPOINT`. - Plain queries, executed statements and prepared statements (Doctrine ORM) are all covered. - The schema commands below only set the schema in the resolver; they do not issue a session `SET`. -- Transaction control SQL (`BEGIN`, `COMMIT`, `ROLLBACK`, savepoints) sent with `executeStatement()` or - `executeQuery()` runs as is, without `SET LOCAL` before it. Send it as a plain statement, not as a prepared - statement with parameters. `COMMIT AND CHAIN` and `ROLLBACK AND CHAIN` keep the transaction open. -- Transaction control batched with other statements (`BEGIN; INSERT ...; COMMIT`) is rejected with - `TransactionControlBatchException`: the statements after the first one would run without `SET LOCAL`. -- In the default `Session` mode statements pass through unchanged. +- Manage transactions through the DBAL API: `beginTransaction()`, `commit()`, `rollBack()`, `transactional()`. + Raw `BEGIN`, `START TRANSACTION`, `COMMIT`, `ROLLBACK`, `END`, `ABORT` or `PREPARE TRANSACTION` SQL is rejected + with `TransactionSearchPathModeException`, as is any of it in a prepared statement or batched with other + statements. A single savepoint statement (`SAVEPOINT`, `RELEASE`, `ROLLBACK TO`), which DBAL uses for nested + transactions, runs as is. +- The mode is applied when a physical connection opens. An idle connection opened in the other mode is reopened; + changing the mode while a transaction is open throws `TransactionSearchPathModeException`. +- In the default `Session` mode the driver connection is not wrapped, and the bundle behaves exactly as before. Choosing `$serverDefaultSchema`: diff --git a/src/Doctrine/Middleware/TransactionSearchPathConnection.php b/src/Doctrine/Middleware/TransactionSearchPathConnection.php index fa676fc..694966d 100644 --- a/src/Doctrine/Middleware/TransactionSearchPathConnection.php +++ b/src/Doctrine/Middleware/TransactionSearchPathConnection.php @@ -9,41 +9,37 @@ use Doctrine\DBAL\Driver\Middleware\AbstractConnectionMiddleware; use Doctrine\DBAL\Driver\Result; use Doctrine\DBAL\Driver\Statement; -use Macpaw\PostgresSchemaBundle\Exception\TransactionControlBatchException; +use Macpaw\PostgresSchemaBundle\Exception\TransactionSearchPathModeException; use Throwable; /** * Applies search_path with SET LOCAL, so it never outlives the transaction and cannot leak to other clients * through a PgBouncer server connection in transaction pooling mode. * - * While $isActive returns false every statement is sent unchanged; only transaction control SQL is tracked. + * Transactions must be managed through the DBAL API (beginTransaction(), commit(), rollBack(), transactional()). + * Raw BEGIN/COMMIT/ROLLBACK SQL is rejected; savepoint SQL, which DBAL sends for nested transactions, is allowed + * as a single statement. Transaction control or savepoint SQL batched with other statements is rejected. */ final class TransactionSearchPathConnection extends AbstractConnectionMiddleware { - /** String literals, quoted identifiers, dollar-quoted bodies and comments, removed before looking for ';'. */ + /** String literals, quoted identifiers, dollar-quoted bodies and comments: removed before splitting on ';'. */ private const NON_CODE_PATTERN = '/\'(?:\'\'|[^\'])*\'|"(?:""|[^"])*"|\$(\w*)\$.*?\$\1\$|\/\*.*?\*\/|--[^\n]*/s'; + private const PREPARED_2PC_PATTERN = '/^(?:COMMIT|ROLLBACK)\s+PREPARED\b/i'; + private const ROLLBACK_TO_PATTERN = '/^ROLLBACK\s+(?:(?:WORK|TRANSACTION)\s+)?TO\b/i'; + private const SAVEPOINT_PATTERN = '/^(?:SAVEPOINT|RELEASE)\b/i'; + private const TRANSACTION_CONTROL_PATTERN + = '/^(?:BEGIN|START\s+TRANSACTION|COMMIT|END|ABORT|ROLLBACK|PREPARE\s+TRANSACTION)\b/i'; - /** Leading whitespace and SQL comments before the first keyword. */ - private const PREFIX = '/^(?:\s+|\/\*.*?\*\/|--[^\n]*(?:\n|$))*'; - private const BEGIN_PATTERN = self::PREFIX . '(?:BEGIN|START\s+TRANSACTION)\b/is'; - private const ROLLBACK_TO_PATTERN = self::PREFIX . 'ROLLBACK\s+(?:(?:WORK|TRANSACTION)\s+)?TO\b/is'; - private const CHAIN_PATTERN = self::PREFIX - . '(?:COMMIT|END|ABORT|ROLLBACK)(?:\s+(?:WORK|TRANSACTION))?\s+AND\s+CHAIN\b/is'; - private const END_PATTERN = self::PREFIX . '(?:COMMIT|END|ABORT|ROLLBACK)\b/is'; - private const SAVEPOINT_PATTERN = self::PREFIX . '(?:SAVEPOINT|RELEASE)\b/is'; - - private const CONTROL_BEGIN = 'begin'; - private const CONTROL_END = 'end'; - private const CONTROL_CHAIN = 'chain'; - private const CONTROL_ROLLBACK_TO = 'rollback_to'; - private const CONTROL_SAVEPOINT = 'savepoint'; + private const KIND_SAVEPOINT = 'savepoint'; + private const KIND_ROLLBACK_TO = 'rollback_to'; + private const KIND_CONTROL = 'control'; + private const KIND_PASSTHROUGH = 'passthrough'; private bool $inTransaction = false; private ?string $appliedSchema = null; private bool $isAppliedSchemaKnown = true; /** - * @param Closure(): bool $isActive whether transaction search_path mode is on * @param Closure(): ?string $schemaProvider returns the quoted schema to apply, * or null to keep the server default * @param Closure(string, bool): void $onApply called with the quoted schema and whether @@ -51,7 +47,6 @@ final class TransactionSearchPathConnection extends AbstractConnectionMiddleware */ public function __construct( Connection $wrappedConnection, - private readonly Closure $isActive, private readonly Closure $schemaProvider, private readonly Closure $onApply, ) { @@ -60,29 +55,21 @@ public function __construct( public function prepare(string $sql): Statement { + if ($this->classify($sql) !== null) { + throw TransactionSearchPathModeException::rawTransactionControl(); + } + return new TransactionSearchPathStatement(parent::prepare($sql), $this); } public function query(string $sql): Result { - $control = $this->classifyTransactionControl($sql); - - if ($control !== null) { - return $this->runTransactionControl($control, $sql, fn (): Result => parent::query($sql)); - } - - return $this->runWithSearchPath(fn (): Result => parent::query($sql)); + return $this->run($sql, fn (): Result => parent::query($sql)); } public function exec(string $sql): int { - $control = $this->classifyTransactionControl($sql); - - if ($control !== null) { - return $this->runTransactionControl($control, $sql, fn (): int => parent::exec($sql)); - } - - return $this->runWithSearchPath(fn (): int => parent::exec($sql)); + return $this->run($sql, fn (): int => parent::exec($sql)); } public function beginTransaction(): bool @@ -133,10 +120,6 @@ public function rollBack(): bool */ public function runWithSearchPath(Closure $operation): mixed { - if (!($this->isActive)()) { - return $operation(); - } - if ($this->inTransaction) { $this->applyInTransaction(); @@ -154,10 +137,6 @@ public function runWithSearchPath(Closure $operation): mixed private function applyInTransaction(): void { - if (!($this->isActive)()) { - return; - } - $schema = ($this->schemaProvider)(); if ($this->isAppliedSchemaKnown && $schema === $this->appliedSchema) { @@ -201,69 +180,80 @@ private function runInImplicitTransaction(string $schema, Closure $operation): m } /** - * @return self::CONTROL_*|null + * @template T + * + * @param Closure(): T $operation + * + * @return T */ - private function classifyTransactionControl(string $sql): ?string + private function run(string $sql, Closure $operation): mixed { - return match (true) { - preg_match(self::BEGIN_PATTERN, $sql) === 1 => self::CONTROL_BEGIN, - preg_match(self::ROLLBACK_TO_PATTERN, $sql) === 1 => self::CONTROL_ROLLBACK_TO, - preg_match(self::CHAIN_PATTERN, $sql) === 1 => self::CONTROL_CHAIN, - preg_match(self::END_PATTERN, $sql) === 1 => self::CONTROL_END, - preg_match(self::SAVEPOINT_PATTERN, $sql) === 1 => self::CONTROL_SAVEPOINT, - default => null, + return match ($this->classify($sql)) { + null => $this->runWithSearchPath($operation), + self::KIND_CONTROL => throw TransactionSearchPathModeException::rawTransactionControl(), + // No SET LOCAL before savepoint SQL: in an aborted transaction it would fail and block the recovery. + self::KIND_SAVEPOINT, self::KIND_PASSTHROUGH => $operation(), + self::KIND_ROLLBACK_TO => $this->runRollbackToSavepoint($operation), }; } /** - * Transaction control SQL runs as is in every mode: no SET LOCAL before it (it would fail in an aborted - * transaction and block the recovery) and no implicit transaction around it (it would end the caller's - * transaction). Its effect is tracked even while the mode is inactive, so the state stays correct - * if the mode is switched on later. - * * @template T * - * @param self::CONTROL_* $control - * @param Closure(): T $operation + * @param Closure(): T $operation * * @return T */ - private function runTransactionControl(string $control, string $sql, Closure $operation): mixed + private function runRollbackToSavepoint(Closure $operation): mixed { - if (($this->isActive)() && $this->isBatch($sql)) { - // Statements after the first one would run without SET LOCAL and outside the tracked state. - throw new TransactionControlBatchException(); - } - try { - $result = $operation(); - } catch (Throwable $exception) { - // A failed COMMIT/ROLLBACK still ends the transaction block; the savepoint state is unknown. - if ($control === self::CONTROL_END || $control === self::CONTROL_CHAIN) { - $this->endTransaction(); - } elseif ($control === self::CONTROL_ROLLBACK_TO) { - $this->isAppliedSchemaKnown = false; - } + return $operation(); + } finally { + // A SET LOCAL issued after the savepoint is undone with it, so re-apply before the next query. + $this->isAppliedSchemaKnown = false; + } + } - throw $exception; + /** + * Classifies transaction control in a statement or a batch. Null means ordinary SQL. + * + * @return self::KIND_*|null + */ + private function classify(string $sql): ?string + { + // Fast path for the common case: one statement without a leading comment. + if (!str_contains($sql, ';') && preg_match('/^\s*(?:--|\/\*)/', $sql) !== 1) { + return $this->classifyStatement(ltrim($sql)); } - match ($control) { - self::CONTROL_BEGIN, self::CONTROL_CHAIN => $this->startTransaction(), - self::CONTROL_END => $this->endTransaction(), - // A SET LOCAL issued after the savepoint is undone with it, so re-apply before the next query. - self::CONTROL_ROLLBACK_TO => $this->isAppliedSchemaKnown = false, - self::CONTROL_SAVEPOINT => null, - }; + $code = (string) preg_replace(self::NON_CODE_PATTERN, ' ', $sql); + $statements = array_values(array_filter( + array_map(trim(...), explode(';', $code)), + static fn (string $statement): bool => $statement !== '', + )); + $kinds = array_map($this->classifyStatement(...), $statements); + $controlKinds = array_filter($kinds, static fn (?string $kind): bool => $kind !== null); - return $result; + if ($controlKinds === []) { + return null; + } + + // Statements after transaction control in a batch would run without SET LOCAL or end the transaction. + return count($statements) === 1 ? $controlKinds[0] : self::KIND_CONTROL; } - private function isBatch(string $sql): bool + /** + * @return self::KIND_*|null + */ + private function classifyStatement(string $statement): ?string { - $code = (string) preg_replace(self::NON_CODE_PATTERN, '', $sql); - - return str_contains(rtrim($code, " \t\n\r\0\x0B;"), ';'); + return match (true) { + preg_match(self::PREPARED_2PC_PATTERN, $statement) === 1 => self::KIND_PASSTHROUGH, + preg_match(self::ROLLBACK_TO_PATTERN, $statement) === 1 => self::KIND_ROLLBACK_TO, + preg_match(self::SAVEPOINT_PATTERN, $statement) === 1 => self::KIND_SAVEPOINT, + preg_match(self::TRANSACTION_CONTROL_PATTERN, $statement) === 1 => self::KIND_CONTROL, + default => null, + }; } private function startTransaction(): void diff --git a/src/Doctrine/Middleware/TransactionSearchPathDriver.php b/src/Doctrine/Middleware/TransactionSearchPathDriver.php index f5eac4f..114e09b 100644 --- a/src/Doctrine/Middleware/TransactionSearchPathDriver.php +++ b/src/Doctrine/Middleware/TransactionSearchPathDriver.php @@ -14,7 +14,7 @@ final class TransactionSearchPathDriver extends AbstractDriverMiddleware { /** - * @param Closure(): bool $isActive whether transaction search_path mode is on + * @param Closure(): bool $isTransactionMode whether connections are opened in transaction mode * @param Closure(): ?string $schemaProvider returns the quoted schema to apply, * or null to keep the server default * @param Closure(string, bool): void $onApply called with the quoted schema and whether @@ -22,7 +22,7 @@ final class TransactionSearchPathDriver extends AbstractDriverMiddleware */ public function __construct( Driver $wrappedDriver, - private readonly Closure $isActive, + private readonly Closure $isTransactionMode, private readonly Closure $schemaProvider, private readonly Closure $onApply, ) { @@ -33,19 +33,20 @@ public function connect(#[SensitiveParameter] array $params): Driver\Connection { $connection = parent::connect($params); + if (!($this->isTransactionMode)()) { + return $connection; + } + // Only pdo_pgsql defers statement preparation to execution, inside SET LOCAL. The check looks at the // native connection, so it also works when other driver middlewares wrap the driver. - if (($this->isActive)()) { - $native = method_exists($connection, 'getNativeConnection') ? $connection->getNativeConnection() : null; + $native = method_exists($connection, 'getNativeConnection') ? $connection->getNativeConnection() : null; - if (!$native instanceof PDO) { - throw new UnsupportedDriverException(get_debug_type($native)); - } + if (!$native instanceof PDO) { + throw new UnsupportedDriverException(get_debug_type($native)); } return new TransactionSearchPathConnection( $connection, - $this->isActive, $this->schemaProvider, $this->onApply, ); diff --git a/src/Doctrine/SchemaConnection.php b/src/Doctrine/SchemaConnection.php index 815a475..b75f11a 100644 --- a/src/Doctrine/SchemaConnection.php +++ b/src/Doctrine/SchemaConnection.php @@ -11,6 +11,7 @@ use Doctrine\DBAL\DriverManager; use Doctrine\DBAL\Platforms\PostgreSQLPlatform; use Macpaw\PostgresSchemaBundle\Doctrine\Middleware\TransactionSearchPathDriver; +use Macpaw\PostgresSchemaBundle\Exception\TransactionSearchPathModeException; use Macpaw\PostgresSchemaBundle\Exception\UnsupportedPlatformException; use Macpaw\SchemaContextBundle\Logger\DebugLogger; use Macpaw\SchemaContextBundle\Service\BaggageSchemaResolver; @@ -25,6 +26,8 @@ class SchemaConnection extends DBALConnection private static ?DebugLogger $logger = null; private static SearchPathMode $searchPathMode = SearchPathMode::Session; private static ?string $serverDefaultSchema = null; + /** Whether the open physical connection was created in transaction mode; null when not connected. */ + private ?bool $isConnectedInTransactionMode = null; private ?string $currentSchema = null; /** @@ -37,9 +40,11 @@ public function __construct( ?Configuration $config = null, ?EventManager $eventManager = null, ) { + // The mode is decided per physical connection on connect: in Session mode the driver connection is + // returned unwrapped, so statements go to the database exactly as before. $driver = new TransactionSearchPathDriver( $driver, - self::isTransactionSearchPathActive(...), + self::isTransactionModeActive(...), self::resolveTransactionSearchPath(...), self::logTransactionSearchPath(...), ); @@ -48,6 +53,9 @@ public function __construct( } /** + * The mode applies to physical connections opened after the call; an idle open connection is reopened + * in the new mode, and switching while a transaction is open throws. + * * @param string|null $serverDefaultSchema null (strict, default) applies SET LOCAL to every query, ignoring any * session search_path another client left on the pooled server * connection. A schema name skips SET LOCAL for that schema and relies @@ -83,6 +91,7 @@ public function close(): void parent::close(); $this->currentSchema = null; + $this->isConnectedInTransactionMode = null; } public function rollBack(): bool @@ -100,13 +109,23 @@ public function rollBack(): bool public function connect(): bool { - $isNewConnection = parent::connect(); + $isTransactionMode = self::isTransactionModeActive(); - if (self::$schemaResolver === null) { - return $isNewConnection; + $isModeChanged = $this->isConnectedInTransactionMode !== null + && $this->isConnectedInTransactionMode !== $isTransactionMode; + + if ($isModeChanged) { + if ($this->isTransactionActive()) { + throw TransactionSearchPathModeException::modeChangedInTransaction(); + } + + $this->close(); } - if (self::$searchPathMode === SearchPathMode::Transaction) { + $isNewConnection = parent::connect(); + $this->isConnectedInTransactionMode = $isTransactionMode; + + if ($isTransactionMode) { if ($isNewConnection) { $this->ensurePostgreSql(); } @@ -114,6 +133,10 @@ public function connect(): bool return $isNewConnection; } + if (self::$schemaResolver === null) { + return $isNewConnection; + } + $schema = self::$schemaResolver->getSchema(); if ($isNewConnection) { @@ -136,7 +159,7 @@ public function connect(): bool return $isNewConnection; } - private static function isTransactionSearchPathActive(): bool + private static function isTransactionModeActive(): bool { return self::$searchPathMode === SearchPathMode::Transaction && self::$schemaResolver !== null; } diff --git a/src/Exception/TransactionControlBatchException.php b/src/Exception/TransactionControlBatchException.php deleted file mode 100644 index 7ea4e71..0000000 --- a/src/Exception/TransactionControlBatchException.php +++ /dev/null @@ -1,18 +0,0 @@ -createMock(AbstractConnectionMiddleware::class); $driverConnection->method('getNativeConnection')->willReturn($this->createMock(PDO::class)); $driverConnection->expects($this->never())->method('exec'); @@ -182,7 +183,6 @@ public function testTransactionModeThrowsForUnsupportedPlatform(): void SchemaConnection::setSchemaResolver( new BaggageSchemaResolver('public', 'development', ['development'], new DebugLogger()), ); - SchemaConnection::setSearchPathMode(SearchPathMode::Transaction); try { $this->expectException(UnsupportedPlatformException::class); @@ -194,6 +194,7 @@ public function testTransactionModeThrowsForUnsupportedPlatform(): void public function testTransactionModeRejectsNonPdoConnection(): void { + SchemaConnection::setSearchPathMode(SearchPathMode::Transaction); $driverConnection = $this->createMock(AbstractConnectionMiddleware::class); $driverConnection->method('getNativeConnection')->willReturn(new stdClass()); $driverConnection->expects($this->never())->method('exec'); @@ -204,7 +205,6 @@ public function testTransactionModeRejectsNonPdoConnection(): void SchemaConnection::setSchemaResolver( new BaggageSchemaResolver('public', 'development', ['development'], new DebugLogger()), ); - SchemaConnection::setSearchPathMode(SearchPathMode::Transaction); try { $this->expectException(UnsupportedDriverException::class); @@ -213,4 +213,16 @@ public function testTransactionModeRejectsNonPdoConnection(): void SchemaConnection::setSearchPathMode(SearchPathMode::Session); } } + + public function testSessionModeDoesNotWrapDriverConnection(): void + { + $driverConnection = $this->createMock(DriverConnection::class); + $driver = $this->createMock(Driver::class); + $driver->method('connect')->willReturn($driverConnection); + $driver->method('getDatabasePlatform')->willReturn(new PostgreSQLPlatform()); + + $connection = new SchemaConnection([], $driver, new Configuration(), new EventManager()); + + self::assertSame($driverConnection, $connection->getWrappedConnection()); + } } diff --git a/tests/Integration/SearchPathPgbouncerTest.php b/tests/Integration/SearchPathPgbouncerTest.php index aca017c..9d8a72c 100644 --- a/tests/Integration/SearchPathPgbouncerTest.php +++ b/tests/Integration/SearchPathPgbouncerTest.php @@ -7,11 +7,13 @@ use Doctrine\DBAL\Connection; use Doctrine\DBAL\DriverManager; use Doctrine\DBAL\Exception as DBALException; +use Macpaw\PostgresSchemaBundle\Doctrine\Middleware\TransactionSearchPathConnection; use Macpaw\PostgresSchemaBundle\Doctrine\SchemaConnection; use Macpaw\PostgresSchemaBundle\Doctrine\SearchPathMode; -use Macpaw\PostgresSchemaBundle\Exception\TransactionControlBatchException; +use Macpaw\PostgresSchemaBundle\Exception\TransactionSearchPathModeException; use Macpaw\SchemaContextBundle\Logger\DebugLogger; use Macpaw\SchemaContextBundle\Service\BaggageSchemaResolver; +use PHPUnit\Framework\Attributes\DataProvider; use PHPUnit\Framework\TestCase; use Psr\Log\AbstractLogger; use RuntimeException; @@ -211,112 +213,100 @@ public function testTransactionModeRollsBackSavepointOfAbortedTransactionAfterSc $preview->commit(); } - public function testTransactionModeKeepsRawTransactionControlStatements(): void + /** + * @return iterable + */ + public static function rawTransactionControlProvider(): iterable { - SchemaConnection::setSearchPathMode(SearchPathMode::Transaction); - $preview = $this->connect(); - $this->resolver->setSchema(self::PREVIEW_SCHEMA); - - $preview->executeStatement('START TRANSACTION'); - $preview->insert('marker', ['v' => 'raw-tx']); - $preview->executeStatement('ROLLBACK'); - - self::assertSame(0, $this->countDirect(self::PREVIEW_SCHEMA, 'raw-tx')); - self::assertSame(self::PREVIEW_MARKER, $preview->fetchOne(self::MARKER_QUERY)); + yield 'begin' => ['BEGIN']; + yield 'start transaction with comment' => ['/* app */ START TRANSACTION']; + yield 'commit' => ['COMMIT']; + yield 'rollback and chain' => ["-- app\nROLLBACK AND CHAIN"]; + yield 'batch' => ["BEGIN; INSERT INTO marker (v) VALUES ('batch'); COMMIT"]; } - public function testTransactionModeKeepsRawTransactionControlSentThroughQueryWithComments(): void + #[DataProvider('rawTransactionControlProvider')] + public function testTransactionModeRejectsRawTransactionControl(string $sql): void { SchemaConnection::setSearchPathMode(SearchPathMode::Transaction); $preview = $this->connect(); $this->resolver->setSchema(self::PREVIEW_SCHEMA); - $preview->executeQuery('/* app */ BEGIN'); - $preview->insert('marker', ['v' => 'raw-query-tx']); - $preview->executeQuery("-- app\nROLLBACK"); + foreach (['executeStatement', 'executeQuery'] as $method) { + try { + $preview->{$method}($sql); + self::fail(sprintf('%s() must reject raw transaction control.', $method)); + } catch (TransactionSearchPathModeException) { + } + } - self::assertSame(0, $this->countDirect(self::PREVIEW_SCHEMA, 'raw-query-tx')); + self::assertSame(0, $this->countDirect(self::PREVIEW_SCHEMA, 'batch')); + self::assertSame(self::PREVIEW_MARKER, $preview->fetchOne(self::MARKER_QUERY)); } - public function testTransactionModeRejectsTransactionControlBatch(): void + public function testIdleConnectionSwitchesToModeSetAfterItWasCreated(): void { - SchemaConnection::setSearchPathMode(SearchPathMode::Transaction); $preview = $this->connect(); - $this->resolver->setSchema(self::PREVIEW_SCHEMA); + self::assertSame(self::PREVIEW_MARKER, $this->readMarker($preview, self::PREVIEW_SCHEMA)); - try { - $preview->executeStatement("BEGIN; INSERT INTO marker (v) VALUES ('batch'); COMMIT"); - self::fail('A transaction control batch must be rejected.'); - } catch (TransactionControlBatchException) { - } + SchemaConnection::setSearchPathMode(SearchPathMode::Transaction); + $default = $this->connect(); - self::assertSame(0, $this->countDirect(self::PREVIEW_SCHEMA, 'batch')); - self::assertSame(0, $this->countDirect('public', 'batch')); self::assertSame(self::PREVIEW_MARKER, $preview->fetchOne(self::MARKER_QUERY)); + self::assertSame(self::PUBLIC_MARKER, $this->readMarker($default, null)); + self::assertInstanceOf(TransactionSearchPathConnection::class, $preview->getWrappedConnection()); } - public function testTransactionModeAppliesSchemaAfterFailedRawCommit(): void + public function testModeChangeInsideTransactionIsRejected(): void { - SchemaConnection::setSearchPathMode(SearchPathMode::Transaction); $preview = $this->connect(); - $this->resolver->setSchema(self::PREVIEW_SCHEMA); - - $preview->executeStatement('START TRANSACTION'); - $preview->executeStatement("INSERT INTO deferred_unique (v) VALUES ('raw-dup'), ('raw-dup')"); + $preview->beginTransaction(); + SchemaConnection::setSearchPathMode(SearchPathMode::Transaction); try { - $preview->executeStatement('COMMIT'); - self::fail('The deferred unique constraint must fail on COMMIT.'); - } catch (DBALException) { + $this->expectException(TransactionSearchPathModeException::class); + $preview->fetchOne('SELECT 1'); + } finally { + SchemaConnection::setSearchPathMode(SearchPathMode::Session); + $preview->rollBack(); } - - self::assertSame(self::PREVIEW_MARKER, $preview->fetchOne(self::MARKER_QUERY)); - self::assertSame(0, $this->countOpenServerTransactions()); } - public function testTransactionModeKeepsTransactionOpenAfterCommitAndChain(): void + /** + * @return iterable + */ + public static function savepointBatchProvider(): iterable { - SchemaConnection::setSearchPathMode(SearchPathMode::Transaction); - $preview = $this->connect(); - $this->resolver->setSchema(self::PREVIEW_SCHEMA); - - $preview->executeStatement('START TRANSACTION'); - $preview->insert('marker', ['v' => 'chain-commit-1']); - $preview->executeStatement('COMMIT AND CHAIN'); - $preview->insert('marker', ['v' => 'chain-commit-2']); - $preview->executeStatement('ROLLBACK'); - - self::assertSame(1, $this->countDirect(self::PREVIEW_SCHEMA, 'chain-commit-1')); - self::assertSame(0, $this->countDirect(self::PREVIEW_SCHEMA, 'chain-commit-2')); + yield 'savepoint then update' => ["SAVEPOINT s1; UPDATE marker SET v = v WHERE v = 'none'"]; + yield 'statement then commit' => ["UPDATE marker SET v = v WHERE v = 'none'; COMMIT"]; + yield 'prepare transaction' => ["PREPARE TRANSACTION 'tx1'"]; } - public function testTransactionModeKeepsTransactionOpenAfterRollbackAndChain(): void + #[DataProvider('savepointBatchProvider')] + public function testTransactionModeRejectsTransactionControlInBatchInsideTransaction(string $sql): void { SchemaConnection::setSearchPathMode(SearchPathMode::Transaction); $preview = $this->connect(); $this->resolver->setSchema(self::PREVIEW_SCHEMA); + $preview->beginTransaction(); - $preview->executeStatement('START TRANSACTION'); - $preview->insert('marker', ['v' => 'chain-rollback-1']); - $preview->executeStatement('ROLLBACK AND CHAIN'); - $preview->insert('marker', ['v' => 'chain-rollback-2']); - $preview->executeStatement('ROLLBACK'); - - self::assertSame(0, $this->countDirect(self::PREVIEW_SCHEMA, 'chain-rollback-1')); - self::assertSame(0, $this->countDirect(self::PREVIEW_SCHEMA, 'chain-rollback-2')); + try { + $this->expectException(TransactionSearchPathModeException::class); + $preview->executeStatement($sql); + } finally { + $preview->rollBack(); + } } - public function testRawTransactionStartedBeforeTransactionModeIsSwitchedOnStaysOpen(): void + public function testTransactionModeAllowsStatementsWithSemicolonsInLiterals(): void { - $preview = $this->connect(); - $preview->executeStatement('START TRANSACTION'); SchemaConnection::setSearchPathMode(SearchPathMode::Transaction); + $preview = $this->connect(); $this->resolver->setSchema(self::PREVIEW_SCHEMA); - $preview->insert('marker', ['v' => 'late-mode']); - $preview->executeStatement('ROLLBACK'); + $preview->executeStatement("INSERT INTO marker (v) VALUES ('a; COMMIT')"); - self::assertSame(0, $this->countDirect(self::PREVIEW_SCHEMA, 'late-mode')); + self::assertSame(1, $this->countDirect(self::PREVIEW_SCHEMA, 'a; COMMIT')); } public function testTransactionModeClosesImplicitTransactionWhenApplyFails(): void From 67bb8008e61f72bf9a9842ae3565e463605fc197 Mon Sep 17 00:00:00 2001 From: Vladislav Date: Fri, 6 Nov 2026 17:15:15 +0200 Subject: [PATCH 12/14] fix(PLT-13586): handle nested comments and escape strings in SQL classification --- README.md | 3 ++- .../Middleware/TransactionSearchPathConnection.php | 13 +++++++++++-- tests/Integration/SearchPathPgbouncerTest.php | 5 +++++ 3 files changed, 18 insertions(+), 3 deletions(-) diff --git a/README.md b/README.md index 4d055f6..be76b5b 100644 --- a/README.md +++ b/README.md @@ -107,7 +107,8 @@ In transaction mode: statements. A single savepoint statement (`SAVEPOINT`, `RELEASE`, `ROLLBACK TO`), which DBAL uses for nested transactions, runs as is. - The mode is applied when a physical connection opens. An idle connection opened in the other mode is reopened; - changing the mode while a transaction is open throws `TransactionSearchPathModeException`. + changing the mode while a transaction is open throws `TransactionSearchPathModeException`. Set the mode in + `Kernel::boot()` before running queries: statements prepared before the switch keep the old mode. - In the default `Session` mode the driver connection is not wrapped, and the bundle behaves exactly as before. Choosing `$serverDefaultSchema`: diff --git a/src/Doctrine/Middleware/TransactionSearchPathConnection.php b/src/Doctrine/Middleware/TransactionSearchPathConnection.php index 694966d..42098cd 100644 --- a/src/Doctrine/Middleware/TransactionSearchPathConnection.php +++ b/src/Doctrine/Middleware/TransactionSearchPathConnection.php @@ -22,8 +22,17 @@ */ final class TransactionSearchPathConnection extends AbstractConnectionMiddleware { - /** String literals, quoted identifiers, dollar-quoted bodies and comments: removed before splitting on ';'. */ - private const NON_CODE_PATTERN = '/\'(?:\'\'|[^\'])*\'|"(?:""|[^"])*"|\$(\w*)\$.*?\$\1\$|\/\*.*?\*\/|--[^\n]*/s'; + /** + * Parts of SQL that may contain ';' without ending a statement, removed before splitting on ';': + * E'' escape strings, string literals, quoted identifiers, dollar-quoted bodies, nested block comments + * and line comments. + */ + private const NON_CODE_PATTERN = '/[Ee]\'(?:\\\\.|\'\'|[^\'\\\\])*\'' + . '|\'(?:\'\'|[^\'])*\'' + . '|"(?:""|[^"])*"' + . '|\$(?\w*)\$.*?\$\k\$' + . '|(?\/\*(?:[^\/*]|\/(?!\*)|\*(?!\/)|(?&comment))*\*\/)' + . '|--[^\n]*/s'; private const PREPARED_2PC_PATTERN = '/^(?:COMMIT|ROLLBACK)\s+PREPARED\b/i'; private const ROLLBACK_TO_PATTERN = '/^ROLLBACK\s+(?:(?:WORK|TRANSACTION)\s+)?TO\b/i'; private const SAVEPOINT_PATTERN = '/^(?:SAVEPOINT|RELEASE)\b/i'; diff --git a/tests/Integration/SearchPathPgbouncerTest.php b/tests/Integration/SearchPathPgbouncerTest.php index 9d8a72c..33ae572 100644 --- a/tests/Integration/SearchPathPgbouncerTest.php +++ b/tests/Integration/SearchPathPgbouncerTest.php @@ -223,6 +223,9 @@ public static function rawTransactionControlProvider(): iterable yield 'commit' => ['COMMIT']; yield 'rollback and chain' => ["-- app\nROLLBACK AND CHAIN"]; yield 'batch' => ["BEGIN; INSERT INTO marker (v) VALUES ('batch'); COMMIT"]; + yield 'nested comment before commit' => [ + "/* outer /* inner */ tail */ COMMIT; INSERT INTO marker (v) VALUES ('batch')", + ]; } #[DataProvider('rawTransactionControlProvider')] @@ -305,8 +308,10 @@ public function testTransactionModeAllowsStatementsWithSemicolonsInLiterals(): v $this->resolver->setSchema(self::PREVIEW_SCHEMA); $preview->executeStatement("INSERT INTO marker (v) VALUES ('a; COMMIT')"); + $preview->executeStatement("INSERT INTO marker (v) VALUES (E'it\\'s; COMMIT')"); self::assertSame(1, $this->countDirect(self::PREVIEW_SCHEMA, 'a; COMMIT')); + self::assertSame(1, $this->countDirect(self::PREVIEW_SCHEMA, "it's; COMMIT")); } public function testTransactionModeClosesImplicitTransactionWhenApplyFails(): void From d99ddea353d3bd1bc18b61f3397e1befaaa6e219 Mon Sep 17 00:00:00 2001 From: Vladislav Date: Mon, 9 Nov 2026 13:26:37 +0200 Subject: [PATCH 13/14] fix(PLT-13586): fail closed on unclassifiable SQL and verify server transaction state --- README.md | 5 + .../TransactionSearchPathConnection.php | 69 ++++++- .../TransactionSearchPathDriver.php | 1 + .../TransactionSearchPathModeException.php | 8 + .../TransactionSearchPathConnectionTest.php | 178 ++++++++++++++++++ 5 files changed, 251 insertions(+), 10 deletions(-) create mode 100644 tests/Doctrine/Middleware/TransactionSearchPathConnectionTest.php diff --git a/README.md b/README.md index be76b5b..cc12b99 100644 --- a/README.md +++ b/README.md @@ -106,6 +106,11 @@ In transaction mode: with `TransactionSearchPathModeException`, as is any of it in a prepared statement or batched with other statements. A single savepoint statement (`SAVEPOINT`, `RELEASE`, `ROLLBACK TO`), which DBAL uses for nested transactions, runs as is. +- The SQL check fails closed. SQL with `;` or a leading comment that cannot be split reliably (unbalanced quotes, + comments or dollar quotes, or a PCRE limit error) is rejected; send such statements one by one. After every + statement the server transaction status (`PDO::inTransaction()`) is compared with the expected one: if transaction + control slipped past the check, the connection rolls back a transaction it left open and throws, and refuses + further statements in a DBAL transaction that the server no longer has. - The mode is applied when a physical connection opens. An idle connection opened in the other mode is reopened; changing the mode while a transaction is open throws `TransactionSearchPathModeException`. Set the mode in `Kernel::boot()` before running queries: statements prepared before the switch keep the old mode. diff --git a/src/Doctrine/Middleware/TransactionSearchPathConnection.php b/src/Doctrine/Middleware/TransactionSearchPathConnection.php index 42098cd..39bcc5a 100644 --- a/src/Doctrine/Middleware/TransactionSearchPathConnection.php +++ b/src/Doctrine/Middleware/TransactionSearchPathConnection.php @@ -19,6 +19,10 @@ * Transactions must be managed through the DBAL API (beginTransaction(), commit(), rollBack(), transactional()). * Raw BEGIN/COMMIT/ROLLBACK SQL is rejected; savepoint SQL, which DBAL sends for nested transactions, is allowed * as a single statement. Transaction control or savepoint SQL batched with other statements is rejected. + * + * The SQL classification is a guard, not a parser: SQL it cannot split reliably is rejected, and after every + * statement the server transaction status is compared with the expected one, so transaction control that slips + * past the classification fails loudly instead of running later statements without SET LOCAL. */ final class TransactionSearchPathConnection extends AbstractConnectionMiddleware { @@ -33,6 +37,8 @@ final class TransactionSearchPathConnection extends AbstractConnectionMiddleware . '|\$(?\w*)\$.*?\$\k\$' . '|(?\/\*(?:[^\/*]|\/(?!\*)|\*(?!\/)|(?&comment))*\*\/)' . '|--[^\n]*/s'; + /** Quotes, comment or dollar-quote delimiters left after removing non-code mean the SQL was not split reliably. */ + private const UNBALANCED_PATTERN = '/[\'"]|\/\*|\*\/|\$\w*\$/'; private const PREPARED_2PC_PATTERN = '/^(?:COMMIT|ROLLBACK)\s+PREPARED\b/i'; private const ROLLBACK_TO_PATTERN = '/^ROLLBACK\s+(?:(?:WORK|TRANSACTION)\s+)?TO\b/i'; private const SAVEPOINT_PATTERN = '/^(?:SAVEPOINT|RELEASE)\b/i'; @@ -49,15 +55,17 @@ final class TransactionSearchPathConnection extends AbstractConnectionMiddleware private bool $isAppliedSchemaKnown = true; /** - * @param Closure(): ?string $schemaProvider returns the quoted schema to apply, - * or null to keep the server default - * @param Closure(string, bool): void $onApply called with the quoted schema and whether - * the transaction was opened by the caller + * @param Closure(): ?string $schemaProvider returns the quoted schema to apply, + * or null to keep the server default + * @param Closure(string, bool): void $onApply called with the quoted schema and whether + * the transaction was opened by the caller + * @param Closure(): bool $isServerInTransaction whether the server session has an open transaction */ public function __construct( Connection $wrappedConnection, private readonly Closure $schemaProvider, private readonly Closure $onApply, + private readonly Closure $isServerInTransaction, ) { parent::__construct($wrappedConnection); } @@ -130,15 +138,20 @@ public function rollBack(): bool public function runWithSearchPath(Closure $operation): mixed { if ($this->inTransaction) { + // Outside a server transaction SET LOCAL is a no-op, so refuse to run after the transaction was lost. + if (!($this->isServerInTransaction)()) { + throw TransactionSearchPathModeException::rawTransactionControl(); + } + $this->applyInTransaction(); - return $operation(); + return $this->runExpectingServerTransaction(true, $operation); } $schema = ($this->schemaProvider)(); if ($schema === null) { - return $operation(); + return $this->runExpectingServerTransaction(false, $operation); } return $this->runInImplicitTransaction($schema, $operation); @@ -170,7 +183,7 @@ private function runInImplicitTransaction(string $schema, Closure $operation): m try { parent::exec('BEGIN; SET LOCAL search_path TO ' . $schema); ($this->onApply)($schema, false); - $result = $operation(); + $result = $this->runExpectingServerTransaction(true, $operation); } catch (Throwable $exception) { $this->rollBackQuietly(); @@ -201,7 +214,10 @@ private function run(string $sql, Closure $operation): mixed null => $this->runWithSearchPath($operation), self::KIND_CONTROL => throw TransactionSearchPathModeException::rawTransactionControl(), // No SET LOCAL before savepoint SQL: in an aborted transaction it would fail and block the recovery. - self::KIND_SAVEPOINT, self::KIND_PASSTHROUGH => $operation(), + self::KIND_SAVEPOINT, self::KIND_PASSTHROUGH => $this->runExpectingServerTransaction( + $this->inTransaction, + $operation, + ), self::KIND_ROLLBACK_TO => $this->runRollbackToSavepoint($operation), }; } @@ -216,13 +232,41 @@ private function run(string $sql, Closure $operation): mixed private function runRollbackToSavepoint(Closure $operation): mixed { try { - return $operation(); + return $this->runExpectingServerTransaction($this->inTransaction, $operation); } finally { // A SET LOCAL issued after the savepoint is undone with it, so re-apply before the next query. $this->isAppliedSchemaKnown = false; } } + /** + * Runs a statement and checks that it left the server transaction status as expected. A mismatch means + * transaction control that the classification did not detect: the statement already ran, but nothing else + * runs on a connection whose transaction state no longer matches DBAL. + * + * @template T + * + * @param Closure(): T $operation + * + * @return T + */ + private function runExpectingServerTransaction(bool $expected, Closure $operation): mixed + { + $result = $operation(); + + if (($this->isServerInTransaction)() === $expected) { + return $result; + } + + if ($expected) { + $this->isAppliedSchemaKnown = false; + } else { + $this->rollBackQuietly(); + } + + throw TransactionSearchPathModeException::rawTransactionControl(); + } + /** * Classifies transaction control in a statement or a batch. Null means ordinary SQL. * @@ -235,7 +279,12 @@ private function classify(string $sql): ?string return $this->classifyStatement(ltrim($sql)); } - $code = (string) preg_replace(self::NON_CODE_PATTERN, ' ', $sql); + $code = preg_replace(self::NON_CODE_PATTERN, ' ', $sql); + + // Fail closed: a PCRE limit error or leftover delimiters mean the statements cannot be split reliably. + if ($code === null || preg_match(self::UNBALANCED_PATTERN, $code) !== 0) { + throw TransactionSearchPathModeException::unclassifiableSql(); + } $statements = array_values(array_filter( array_map(trim(...), explode(';', $code)), static fn (string $statement): bool => $statement !== '', diff --git a/src/Doctrine/Middleware/TransactionSearchPathDriver.php b/src/Doctrine/Middleware/TransactionSearchPathDriver.php index 114e09b..e273a15 100644 --- a/src/Doctrine/Middleware/TransactionSearchPathDriver.php +++ b/src/Doctrine/Middleware/TransactionSearchPathDriver.php @@ -49,6 +49,7 @@ public function connect(#[SensitiveParameter] array $params): Driver\Connection $connection, $this->schemaProvider, $this->onApply, + $native->inTransaction(...), ); } } diff --git a/src/Exception/TransactionSearchPathModeException.php b/src/Exception/TransactionSearchPathModeException.php index e9ff02d..c0f7ef7 100644 --- a/src/Exception/TransactionSearchPathModeException.php +++ b/src/Exception/TransactionSearchPathModeException.php @@ -16,6 +16,14 @@ public static function rawTransactionControl(): self ); } + public static function unclassifiableSql(): self + { + return new self( + 'Transaction search_path mode cannot split this SQL into statements reliably ' + . '(unbalanced quotes, comments or dollar quotes, or a PCRE limit). Send the statements one by one.', + ); + } + public static function modeChangedInTransaction(): self { return new self( diff --git a/tests/Doctrine/Middleware/TransactionSearchPathConnectionTest.php b/tests/Doctrine/Middleware/TransactionSearchPathConnectionTest.php new file mode 100644 index 0000000..0883cb1 --- /dev/null +++ b/tests/Doctrine/Middleware/TransactionSearchPathConnectionTest.php @@ -0,0 +1,178 @@ + */ + private array $sent = []; + + private bool $isServerInTransaction = false; + + /** @var array server transaction status after each SQL */ + private array $serverTransactionAfter = []; + + public function testImplicitTransactionClosedByUndetectedControlIsRejectedWithoutCommit(): void + { + $connection = $this->connection(self::PREVIEW_SCHEMA); + $this->serverTransactionAfter[self::INSERT] = false; + + $this->expectRawTransactionControl(fn () => $connection->exec(self::INSERT)); + + self::assertSame([self::APPLY_IMPLICIT, self::INSERT, 'ROLLBACK'], $this->sent); + } + + public function testTransactionLeftOpenByUndetectedControlIsRolledBack(): void + { + $connection = $this->connection(null); + $this->serverTransactionAfter[self::INSERT] = true; + + $this->expectRawTransactionControl(fn () => $connection->exec(self::INSERT)); + + self::assertSame([self::INSERT, 'ROLLBACK'], $this->sent); + self::assertFalse($this->isServerInTransaction); + } + + public function testStatementsAfterCallerTransactionWasLostAreNotSent(): void + { + $connection = $this->connection(self::PREVIEW_SCHEMA); + $connection->beginTransaction(); + $this->serverTransactionAfter[self::INSERT] = false; + + $this->expectRawTransactionControl(fn () => $connection->exec(self::INSERT)); + $this->expectRawTransactionControl(fn () => $connection->exec('SELECT 1')); + + self::assertSame(['SET LOCAL search_path TO "preview_a"', self::INSERT], $this->sent); + } + + /** + * @return iterable + */ + public static function unclassifiableSqlProvider(): iterable + { + yield 'unterminated string' => ["SELECT 1; SELECT 'open"]; + yield 'unterminated identifier' => ['SELECT 1; SELECT "open']; + yield 'unterminated block comment' => ['SELECT 1; /* open COMMIT']; + yield 'stray comment end' => ['SELECT 1 */; COMMIT']; + yield 'unterminated dollar quote' => ['SELECT $body$ x; COMMIT']; + } + + #[DataProvider('unclassifiableSqlProvider')] + public function testUnclassifiableSqlIsRejectedBeforeSending(string $sql): void + { + $connection = $this->connection(self::PREVIEW_SCHEMA); + + foreach ([fn () => $connection->exec($sql), fn () => $connection->prepare($sql)] as $call) { + try { + $call(); + self::fail('Unclassifiable SQL must be rejected.'); + } catch (TransactionSearchPathModeException $exception) { + self::assertStringContainsString('cannot split this SQL', $exception->getMessage()); + } + } + + self::assertSame([], $this->sent); + } + + public function testPcreFailureIsRejectedBeforeSending(): void + { + $connection = $this->connection(self::PREVIEW_SCHEMA); + $backtrackLimit = ini_get('pcre.backtrack_limit'); + $jit = ini_get('pcre.jit'); + ini_set('pcre.jit', '0'); + ini_set('pcre.backtrack_limit', '1'); + + try { + $this->expectExceptionObject(TransactionSearchPathModeException::unclassifiableSql()); + $connection->exec("SELECT 1; SELECT \$x\$ long body; with semicolons \$x\$"); + } finally { + ini_set('pcre.backtrack_limit', (string) $backtrackLimit); + ini_set('pcre.jit', (string) $jit); + self::assertSame([], $this->sent); + } + } + + public function testMatchingServerTransactionStatusRunsAsBefore(): void + { + $connection = $this->connection(self::PREVIEW_SCHEMA); + + $connection->exec(self::INSERT); + $connection->beginTransaction(); + $connection->exec(self::INSERT); + $connection->commit(); + + self::assertSame( + [ + self::APPLY_IMPLICIT, + self::INSERT, + 'COMMIT', + 'SET LOCAL search_path TO "preview_a"', + self::INSERT, + ], + $this->sent, + ); + } + + private function expectRawTransactionControl(callable $call): void + { + try { + $call(); + self::fail('Undetected transaction control must be rejected.'); + } catch (TransactionSearchPathModeException $exception) { + self::assertSame( + TransactionSearchPathModeException::rawTransactionControl()->getMessage(), + $exception->getMessage(), + ); + } + } + + private function connection(?string $schema): TransactionSearchPathConnection + { + $wrapped = $this->createMock(DriverConnection::class); + $wrapped->method('exec')->willReturnCallback(function (string $sql): int { + $this->sent[] = $sql; + $this->isServerInTransaction = match (true) { + isset($this->serverTransactionAfter[$sql]) => $this->serverTransactionAfter[$sql], + str_starts_with($sql, 'BEGIN') => true, + $sql === 'COMMIT', $sql === 'ROLLBACK' => false, + default => $this->isServerInTransaction, + }; + + return 0; + }); + $wrapped->method('beginTransaction')->willReturnCallback(function (): bool { + $this->isServerInTransaction = true; + + return true; + }); + $wrapped->method('commit')->willReturnCallback(function (): bool { + $this->isServerInTransaction = false; + + return true; + }); + + return new TransactionSearchPathConnection( + $wrapped, + static fn (): ?string => $schema, + static function (): void { + }, + fn (): bool => $this->isServerInTransaction, + ); + } +} From d240bdfc974abe621f9281b67e5f927fa063a754 Mon Sep 17 00:00:00 2001 From: Vladislav Date: Mon, 9 Nov 2026 13:43:10 +0200 Subject: [PATCH 14/14] test(PLT-13586): assert rejected batches write nothing to public --- tests/Integration/SearchPathPgbouncerTest.php | 3 +++ 1 file changed, 3 insertions(+) diff --git a/tests/Integration/SearchPathPgbouncerTest.php b/tests/Integration/SearchPathPgbouncerTest.php index 33ae572..77080e9 100644 --- a/tests/Integration/SearchPathPgbouncerTest.php +++ b/tests/Integration/SearchPathPgbouncerTest.php @@ -243,7 +243,10 @@ public function testTransactionModeRejectsRawTransactionControl(string $sql): vo } } + // The safety net throws the same exception after an undetected COMMIT already ran the rest of the batch, + // so check both schemas: the batch must be rejected before anything is sent. self::assertSame(0, $this->countDirect(self::PREVIEW_SCHEMA, 'batch')); + self::assertSame(0, $this->countDirect('public', 'batch')); self::assertSame(self::PREVIEW_MARKER, $preview->fetchOne(self::MARKER_QUERY)); }