Skip to content
Merged
Show file tree
Hide file tree
Changes from 9 commits
Commits
File filter

Filter by extension

Filter by extension


Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
33 changes: 33 additions & 0 deletions .github/copilot-instructions.md
Original file line number Diff line number Diff line change
@@ -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.
115 changes: 113 additions & 2 deletions .github/workflows/ci.yaml
Original file line number Diff line number Diff line change
Expand Up @@ -8,6 +8,8 @@ on:
jobs:
run:
runs-on: ubuntu-latest
permissions:
contents: read
strategy:
fail-fast: false
matrix:
Expand Down Expand Up @@ -47,6 +49,48 @@ 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.*'
coverage: 'none'
postgres: '18'
Comment thread
ganzyukv marked this conversation as resolved.
Resolved

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:
Expand All @@ -62,6 +106,7 @@ jobs:
uses: shivammathur/setup-php@v2
with:
php-version: ${{ matrix.php }}
extensions: pdo_pgsql
coverage: xdebug

- name: Add PHPUnit matcher
Expand Down Expand Up @@ -90,16 +135,82 @@ 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'
Comment thread
ganzyukv marked this conversation as resolved.
Resolved

- 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: 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:
name: coverage-comment
path: coverage-comment.md
retention-days: 1

- name: Upload coverage reports to Codecov
if: matrix.coverage == 'xdebug'
uses: codecov/codecov-action@v4.0.1
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
25 changes: 25 additions & 0 deletions .github/workflows/copilot-code-review.yml
Original file line number Diff line number Diff line change
@@ -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
2 changes: 1 addition & 1 deletion .github/workflows/security.yaml
Original file line number Diff line number Diff line change
Expand Up @@ -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
Expand Down
6 changes: 3 additions & 3 deletions .github/workflows/static-analysis.yaml
Original file line number Diff line number Diff line change
Expand Up @@ -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
Expand All @@ -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
Expand All @@ -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
Expand Down
84 changes: 81 additions & 3 deletions README.md
Original file line number Diff line number Diff line change
Expand Up @@ -68,6 +68,71 @@ 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 "<schema>"`, 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) 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`:

- 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`.
- `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
`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('<sequence>')` 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
Expand All @@ -83,7 +148,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
Expand Down Expand Up @@ -148,10 +214,22 @@ php bin/console doctrine:schema:fixtures:load <schema_name> [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 every matrix job against PostgreSQL 14, and in one job against PostgreSQL 18.
## Contributing
Feel free to open issues and submit pull requests.

Expand Down
5 changes: 3 additions & 2 deletions composer.json
Original file line number Diff line number Diff line change
Expand Up @@ -25,8 +25,9 @@
"doctrine/doctrine-fixtures-bundle": "^3.5 || ^4.0",
"phpstan/phpstan": "^1.10",
"phpunit/phpunit": "^10.0",
"squizlabs/php_codesniffer": "3.7.*",
"dg/bypass-finals": "^1.9"
"squizlabs/php_codesniffer": "^3.13",
"dg/bypass-finals": "^1.9",
"rregeer/phpunit-coverage-check": "^0.3"
},
"config": {
"allow-plugins": {
Expand Down
Loading
Loading