diff --git a/.claude/plans/tier4-performance/001-findoneby-limit-one.md b/.claude/plans/tier4-performance/001-findoneby-limit-one.md index a16fcc03..858aafcf 100644 --- a/.claude/plans/tier4-performance/001-findoneby-limit-one.md +++ b/.claude/plans/tier4-performance/001-findoneby-limit-one.md @@ -1,6 +1,6 @@ # Task 001: F1 — Add `LIMIT 1` to `findOneBy` -**Status**: pending +**Status**: complete **Depends on**: [none] **Retry count**: 0 @@ -16,14 +16,15 @@ - Build the WHERE clause with the same property->column mapping `findBy` uses (`metadata->getPropertyToColumnMap()`, criteria joined with ` AND `, positional bindings); append `LIMIT 1` to the SQL. Result-equivalence: because `findBy` issues no `ORDER BY`, the row returned under `LIMIT 1` is the same arbitrary "first" row `findBy()->first()` returned — do NOT introduce an `ORDER BY` (that would change which row is returned). When zero rows match, return `null`. - Hydrate `$rows[0]` and eager-load it via the SAME private `eagerLoadRelationships([$entity])` call `find()`/`findBy` use, which reads `$this->pendingRelationships`. The `with(...)->findOneBy(...)` chain must still populate relationships on the single result — verify the clone's `pendingRelationships` flow through the new direct path (do not bypass it by short-circuiting before `eagerLoadRelationships`). - Query-count assertions via inline anonymous `ConnectionInterface` stub with `public array &$queries` (mirroring `RepositoryCrudTest`): assert exactly one `query()` call whose SQL ends with/contains `LIMIT 1`, and assert the returned entity equals what the old `findBy()->first()` path produced. + - **The stub MUST implement `driverName(): string`** (Tier 2 added this to `ConnectionInterface`, line 62). An anonymous `implements ConnectionInterface` class that omits it is abstract-incomplete and fatals at instantiation. The reference stub in `RepositoryCrudTest` (line 159) returns `'sqlite'`; do the same. ## Requirements (Test Descriptions) -- [ ] `it returns the matching entity for findOneBy when a row exists` -- [ ] `it returns null from findOneBy when no row matches` -- [ ] `it appends LIMIT 1 to the findOneBy query` -- [ ] `it issues exactly one query for findOneBy even when multiple rows would match` -- [ ] `it maps entity property names to column names in the findOneBy WHERE clause` -- [ ] `it eager-loads relationships on the single entity returned by findOneBy` +- [x] `it returns the matching entity for findOneBy when a row exists` +- [x] `it returns null from findOneBy when no row matches` +- [x] `it appends LIMIT 1 to the findOneBy query` +- [x] `it issues exactly one query for findOneBy even when multiple rows would match` +- [x] `it maps entity property names to column names in the findOneBy WHERE clause` +- [x] `it eager-loads relationships on the single entity returned by findOneBy` ## Acceptance Criteria - All requirements have passing tests @@ -31,4 +32,8 @@ - No decrease in test coverage ## Implementation Notes -(Left blank - filled in by programmer during implementation) +- `findOneBy` now issues a direct `SELECT * FROM {table} WHERE {conditions} LIMIT 1` query instead of delegating to `findBy()->first()` +- Property-to-column mapping uses the same `metadata->getPropertyToColumnMap()` logic as `findBy` +- Hydration and eager-loading flow through the same private `eagerLoadRelationships([$entity])` call as `find()` and `findBy()`, preserving the `with(...)->findOneBy(...)` chain +- No `ORDER BY` introduced; behavior is deliberately equivalent to the arbitrary "first" row `findBy()->first()` returned +- Test stub connection implements `driverName(): string` returning `'sqlite'` as required by Tier 2's `ConnectionInterface` diff --git a/.claude/plans/tier4-performance/002-exists-family-select-one-limit.md b/.claude/plans/tier4-performance/002-exists-family-select-one-limit.md index 37ad0b08..64ccd68b 100644 --- a/.claude/plans/tier4-performance/002-exists-family-select-one-limit.md +++ b/.claude/plans/tier4-performance/002-exists-family-select-one-limit.md @@ -1,6 +1,6 @@ # Task 002: F1 — `SELECT 1 ... LIMIT 1` for `exists` / `existsBy` / `isColumnUnique` -**Status**: pending +**Status**: complete **Depends on**: [001] **Retry count**: 0 @@ -8,30 +8,31 @@ The existence-check family does far more work than needed: `exists($id)` calls `find()` which hydrates a full entity and eager-loads relationships; `existsBy` calls `findOneBy` (full fetch); `isColumnUnique` runs `SELECT *` with no `LIMIT`. All three only need to know whether a row exists. Replace each with a boolean probe — `SELECT 1 FROM WHERE ... LIMIT 1` — returning early without hydration or eager loading. ## Description (scope detail) -This depends on 001 because both edit the same `Repository.php` lookup region; sequencing avoids merge churn. `exists`/`existsBy` may keep delegating only if the delegate is itself a `LIMIT 1` probe — prefer dedicated `SELECT 1 ... LIMIT 1` SQL so no entity is ever hydrated for a bool. +This depends on 001 because both edit the same `Repository.php` lookup region; sequencing avoids merge churn. **In the CURRENT code `exists($id)` delegates to `find()` (line 544) and `existsBy()` delegates to `findOneBy()` (line 555).** After Task 001, `findOneBy` returns a fully hydrated, eager-loaded entity capped at `LIMIT 1` — so if `existsBy`/`exists` keep delegating, they STILL hydrate an entity for a bool, violating this task's "the probe must never construct an entity" requirement. Therefore replace BOTH bodies with dedicated `SELECT 1 ... LIMIT 1` probes; do NOT keep delegating to `find`/`findOneBy`. ## Context - Related files: - - `packages/database/src/Repository/Repository.php` (`exists` ~517-521, `existsBy` ~528-532, `isColumnUnique` ~542-557) - - `packages/database/tests/Feature/RepositoryCrudTest.php` (anonymous `ConnectionInterface` stub query-recording pattern) + - `packages/database/src/Repository/Repository.php` (`exists` ~541-545, `existsBy` ~552-556, `isColumnUnique` ~561-582 — note `isColumnUnique` is `protected`) + - `packages/database/tests/Feature/RepositoryCrudTest.php` (anonymous `ConnectionInterface` stub query-recording pattern; stub at line 49 includes a `driverName()` method at line 159 — new stubs MUST include it too) - Patterns to follow: - `exists($id)`: `SELECT 1 FROM
WHERE = ? LIMIT 1`, return `count($rows) > 0`. - `existsBy(array $criteria)`: build WHERE from the same property->column map as `findBy` (multiple criteria joined with ` AND `), `SELECT 1 ... LIMIT 1`, return `count($rows) > 0`. Preserve the existing multi-criteria behavior of `findBy` exactly — same property->column resolution, same positional binding order. - `isColumnUnique($column, $value, $excludeId)`: `SELECT 1 FROM
WHERE = ?` plus optional `AND != ?`, append `LIMIT 1`, return `count($rows) === 0`. With `LIMIT 1` the probe returns at most one row, so `=== 0` correctly means "no other row holds the value" — identical semantics to the prior unbounded `SELECT *`. - - No `hydrate()`, no `eagerLoadRelationships()` on any of these paths — the probe must never construct an entity. + - No `hydrate()`, no `eagerLoadRelationships()`, and no delegation to `find`/`findOneBy` on any of these paths — the probe must never construct an entity. Verify by asserting the recorded SQL is `SELECT 1 ...` (not `SELECT *`) and that no eager-load query is emitted. - Equivalence guard: each test must assert the boolean result matches the pre-change behavior for both the present and absent cases, not merely that `LIMIT 1`/`SELECT 1` appears in the SQL. + - Any new inline anonymous `ConnectionInterface` stub MUST implement `driverName(): string` (Tier 2 interface addition) or it fatals at instantiation. ## Requirements (Test Descriptions) -- [ ] `it returns true from exists when a row with the id is present` -- [ ] `it returns false from exists when no row has the id` -- [ ] `it probes exists with SELECT 1 and LIMIT 1 and does not hydrate an entity` -- [ ] `it returns true from existsBy when criteria match a row` -- [ ] `it returns false from existsBy when criteria match nothing` -- [ ] `it probes existsBy with SELECT 1 and LIMIT 1` -- [ ] `it returns true from isColumnUnique when no other row holds the value` -- [ ] `it returns false from isColumnUnique when another row holds the value` -- [ ] `it excludes the given id from the isColumnUnique uniqueness check` -- [ ] `it probes isColumnUnique with SELECT 1 and LIMIT 1 instead of SELECT star` +- [x] `it returns true from exists when a row with the id is present` +- [x] `it returns false from exists when no row has the id` +- [x] `it probes exists with SELECT 1 and LIMIT 1 and does not hydrate an entity` +- [x] `it returns true from existsBy when criteria match a row` +- [x] `it returns false from existsBy when criteria match nothing` +- [x] `it probes existsBy with SELECT 1 and LIMIT 1` +- [x] `it returns true from isColumnUnique when no other row holds the value` +- [x] `it returns false from isColumnUnique when another row holds the value` +- [x] `it excludes the given id from the isColumnUnique uniqueness check` +- [x] `it probes isColumnUnique with SELECT 1 and LIMIT 1 instead of SELECT star` ## Acceptance Criteria - All requirements have passing tests @@ -39,4 +40,9 @@ This depends on 001 because both edit the same `Repository.php` lookup region; s - No decrease in test coverage ## Implementation Notes -(Left blank - filled in by programmer during implementation) +- Replaced `exists()` body with `SELECT 1 FROM
WHERE = ? LIMIT 1`, returns `count($rows) > 0` +- Replaced `existsBy()` body with same property→column mapping as `findBy`, `SELECT 1 ... LIMIT 1`, returns `count($rows) > 0` +- Replaced `isColumnUnique()` body with `SELECT 1 FROM
WHERE = ? [AND != ?] LIMIT 1`, returns `count($rows) === 0` +- None of the three methods delegate to `find`/`findOneBy` — no hydration, no eager-loading on any probe path +- New test file: `packages/database/tests/Feature/RepositoryExistsTest.php` with 10 tests covering boolean correctness and SQL shape assertions +- Anonymous `ConnectionInterface` stub includes `driverName(): string` as required by Tier 2 interface diff --git a/.claude/plans/tier4-performance/003-media-findmany-batch.md b/.claude/plans/tier4-performance/003-media-findmany-batch.md index 6c88e52f..a68a4140 100644 --- a/.claude/plans/tier4-performance/003-media-findmany-batch.md +++ b/.claude/plans/tier4-performance/003-media-findmany-batch.md @@ -1,6 +1,6 @@ # Task 003: F2 — Batch media hydration in `AttachmentManager::findByAttachable` -**Status**: pending +**Status**: complete **Depends on**: [none] **Retry count**: 0 @@ -13,24 +13,24 @@ ## Context - Related files: - `packages/media/src/Service/AttachmentManager.php` (`findByAttachable` ~46-63 — the per-id loop; `readonly class`) - - `packages/media/src/Contracts/MediaRepositoryInterface.php` (only has `save`/`delete`/`find(int): ?Media` — add `findMany`) - - `packages/media/src/Contracts/MediaAttachmentRepositoryInterface.php` (`findByAttachable` returns `array` ids) - - `packages/media/tests/Service/AttachmentManagerTest.php` (`makeMediaRepository` anonymous-class mock implementing `MediaRepositoryInterface` — MUST add `findMany`) - - `packages/media/tests/Service/MediaManagerTest.php` (~line 228, second anonymous-class mock implementing `MediaRepositoryInterface` — MUST add `findMany`) + - `packages/media/src/Contracts/MediaRepositoryInterface.php` (only has `save`/`delete`/`find(int $id): ?Media` — add `findMany`; ids are `int`) + - `packages/media/src/Contracts/MediaAttachmentRepositoryInterface.php` (`findByAttachable` returns `array` ids — these `int` ids are what `findMany` receives) + - `packages/media/tests/Service/AttachmentManagerTest.php` (`makeMediaRepository` anonymous-class mock at line 76 implementing `MediaRepositoryInterface` — MUST add `findMany`) + - `packages/media/tests/Service/MediaManagerTest.php` (`makeRepository` at line 244, second anonymous-class mock implementing `MediaRepositoryInterface` — MUST add `findMany`) - Patterns to follow: - - Add `MediaRepositoryInterface::findMany(array $ids): array` returning `array`, documented: empty input -> empty array with no query; result keyed/ordered is the caller's concern (the contract returns a flat `array` of the matched rows, order unspecified). + - Add `MediaRepositoryInterface::findMany(array $ids): array` (`@param array $ids`) returning `array`, documented: empty input -> empty array with no query; the contract returns a flat `array` of the matched rows in unspecified order (the caller — `AttachmentManager` — is responsible for any ordering). Confirmed: there is NO concrete `MediaRepository` class in-package and exactly two in-repo implementers (the two test mocks above) — both must add the method or the `marko/media` suite fatals. - The mock `findMany` returns the stored media for each requested id that exists, skipping misses (mirrors `find()` returning null). - `AttachmentManager::findByAttachable` calls `findMany($mediaIds)` once, then re-orders the returned media to match the attachment id list and skips ids with no matching media (preserve current null-skip + ordering behavior) — the re-ordering/skip logic lives in `AttachmentManager`, not the repository. - Query-count assertions: the mock records how many times `findMany` (vs `find`) is invoked; assert `find` is never called and `findMany` is called exactly once regardless of attachment count. ## Requirements (Test Descriptions) -- [ ] `it returns the media entities for all attached ids` -- [ ] `it returns media in the order of the attachment id list` -- [ ] `it skips ids that have no matching media row` -- [ ] `it returns an empty array when there are no attachments` -- [ ] `it resolves all attachments with a single findMany call and never calls find per id` -- [ ] `it defines findMany on MediaRepositoryInterface returning an array of Media` -- [ ] `it does not invoke findMany when there are no attachment ids` +- [x] `it returns the media entities for all attached ids` +- [x] `it returns media in the order of the attachment id list` +- [x] `it skips ids that have no matching media row` +- [x] `it returns an empty array when there are no attachments` +- [x] `it resolves all attachments with a single findMany call and never calls find per id` +- [x] `it defines findMany on MediaRepositoryInterface returning an array of Media` +- [x] `it does not invoke findMany when there are no attachment ids` ## Acceptance Criteria - All requirements have passing tests @@ -38,4 +38,9 @@ - No decrease in test coverage ## Implementation Notes -(Left blank - filled in by programmer during implementation) +- Added `findMany(array $ids): array` to `MediaRepositoryInterface` with documented contract (empty input → empty array no query; caller responsible for ordering; consumer implements single `WHERE id IN (...)` query) +- Rewired `AttachmentManager::findByAttachable` to call `findMany` once, then re-order by attachment id list and skip missing ids in PHP +- Added `findMany` to both in-package test mocks: `makeMediaRepository` in `AttachmentManagerTest.php` and `makeRepository` in `MediaManagerTest.php` +- Added `makeTrackingMediaRepository` mock with `findCallCount`/`findManyCallCount` properties to verify N+1 elimination +- Added `MediaRepositoryInterfaceTest.php` for the interface definition requirement +- Pre-existing full-suite crash in `notification`/`notification-database` packages is unrelated to this task diff --git a/.claude/plans/tier4-performance/004-admin-auth-permissions-batch.md b/.claude/plans/tier4-performance/004-admin-auth-permissions-batch.md index 402db341..1834c7e2 100644 --- a/.claude/plans/tier4-performance/004-admin-auth-permissions-batch.md +++ b/.claude/plans/tier4-performance/004-admin-auth-permissions-batch.md @@ -1,36 +1,37 @@ # Task 004: F3 — Batch permission loading for all roles in one query -**Status**: pending +**Status**: complete **Depends on**: [none] **Retry count**: 0 ## Description -`AdminUserProvider::loadRolesAndPermissions` runs on every authenticated admin request (via `retrieveById`, `retrieveByCredentials`, `retrieveByToken`) and loops `roleRepository->getPermissionsForRole($role->id)` once per role — N permission queries for a user with N roles. Add a batch method that fetches the permission set for all role ids in a single `WHERE rp.role_id IN (...)` query, and rewire the provider to use it. +`AdminUserProvider::loadRolesAndPermissions` runs on every authenticated admin request (via `retrieveById`, `retrieveByCredentials`, and `retrieveByRememberToken` — NOT `retrieveByToken`, that method does not exist) and loops `roleRepository->getPermissionsForRole($role->id)` once per role — N permission queries for a user with N roles. Add a batch method that fetches the permission set for all role ids in a single `WHERE rp.role_id IN (...)` query, and rewire the provider to use it. ## Context - Related files: - `packages/admin-auth/src/AdminUserProvider.php` (`loadRolesAndPermissions` ~109-132 — the per-role loop; `readonly class`) - `packages/admin-auth/src/Repository/RoleRepositoryInterface.php` (`getPermissionsForRole(int): array` — add `getPermissionsForRoles`) - - `packages/admin-auth/src/Repository/RoleRepository.php` (`getPermissionsForRole` ~100-119 — INNER JOIN `role_permissions rp` ... `WHERE rp.role_id = ?`; mirror its hydration via `metadataFactory->parse(Permission::class)`) - - `packages/admin-auth/tests/Unit/AdminUserProviderTest.php` (`createMockRoleRepo` ~315 is a `readonly class implements RoleRepositoryInterface` — MUST add `getPermissionsForRoles` or the suite fatals; it takes a `$permissionsMap` keyed by role id, so the batch version should return the union across the requested ids) - - `packages/admin-auth/tests/Unit/Repository/RoleRepositoryTest.php` (`createRoleMockConnectionWithHistory` ~191 records `['sql','bindings']` per `query()`/`execute()` — use this to assert one permissions query) - - `packages/admin-auth/tests/Unit/Repository/RoleRepositoryInterfaceTest.php` (~line 19 reflection method-presence checks; extend `$expectedMethods` with `getPermissionsForRoles`) + - `packages/admin-auth/src/Repository/RoleRepository.php` (`getPermissionsForRole` ~100-119 — INNER JOIN `role_permissions rp` ... `WHERE rp.role_id = ?`; mirror its hydration via `metadataFactory->parse(Permission::class)`. Note it declares `@throws EntityException` because `hydrator->hydrate()` throws it.) + - `packages/admin-auth/tests/Unit/AdminUserProviderTest.php` (`createMockRoleRepo` ~315 is a `readonly class implements RoleRepositoryInterface` — MUST add `getPermissionsForRoles` or the suite fatals, and the class must remain `readonly`; it takes a `$permissionsMap` keyed by role id, so the batch version returns the deduplicated union across the requested ids. This is the only in-repo anonymous implementer of the interface besides `RoleRepository`.) + - `packages/admin-auth/tests/Unit/Repository/RoleRepositoryTest.php` (`createRoleMockConnectionWithHistory` ~191 records `['sql','bindings']` per `query()`/`execute()`, already implements `driverName()` at line 256, and returns the SAME canned `$queryResult` for EVERY `query()` call — construct the permission rows accordingly when asserting the one-query / dedup behavior) + - `packages/admin-auth/tests/Unit/Repository/RoleRepositoryInterfaceTest.php` (~line 19 reflection method-presence checks; extend `$expectedMethods` with `getPermissionsForRoles` — also update the test title at line 12 if it enumerates method names) - Patterns to follow: - - Add `RoleRepositoryInterface::getPermissionsForRoles(array $roleIds): array` returning `array` (deduplicated across roles), implemented with one `SELECT DISTINCT p.* FROM permissions p INNER JOIN role_permissions rp ON p.id = rp.permission_id WHERE rp.role_id IN (?, ?, ...)` where the placeholder count matches the role-id count. + - Add `RoleRepositoryInterface::getPermissionsForRoles(array $roleIds): array` (`@param array $roleIds`, `@throws EntityException`) returning `array` (deduplicated across roles), implemented with one `SELECT DISTINCT p.* FROM permissions p INNER JOIN role_permissions rp ON p.id = rp.permission_id WHERE rp.role_id IN (?, ?, ...)` where the placeholder count matches the role-id count. + - Because `getPermissionsForRoles` hydrates Permissions, it throws `EntityException`. That propagates through `loadRolesAndPermissions` into `AdminUserProvider::retrieveById`/`retrieveByCredentials`/`retrieveByRememberToken` — add `@throws EntityException` to `getPermissionsForRoles` (interface + impl), `loadRolesAndPermissions`, and those three provider methods per code-standards rule 9. Any NEW inline anonymous `ConnectionInterface` stub must implement `driverName(): string`. - **Empty `$roleIds` MUST short-circuit to `[]` with no query** — never emit `WHERE rp.role_id IN ()`, which is a SQL syntax error. Build the IN placeholder list from the (non-empty) role-id array and bind each id positionally. - `loadRolesAndPermissions` collects non-null role ids, calls `getPermissionsForRoles` once, and passes `array_unique` permission keys to `setRoles` exactly as before. Behavior must be identical for a user with zero roles (no permissions query) and for roles sharing permissions (deduped key set). - **Update every in-repo implementer of `RoleRepositoryInterface`** (`RoleRepository` + the `createMockRoleRepo` anonymous class) to satisfy the new method before running the suite. - Query-count assertions: `createRoleMockConnectionWithHistory` records each `query()` call; assert exactly one permissions query for N roles, that its SQL contains `IN (` with N placeholders, and that the resulting permission-key set is identical to the per-role loop. ## Requirements (Test Descriptions) -- [ ] `it loads the same permission set as the per-role implementation` -- [ ] `it deduplicates permission keys shared across roles` -- [ ] `it issues exactly one permissions query for a user with multiple roles` -- [ ] `it issues no permissions query when the user has no roles` -- [ ] `it returns an empty array from getPermissionsForRoles without querying when given no role ids` -- [ ] `it queries permissions with a WHERE role_id IN clause whose placeholder count matches the role ids` -- [ ] `it sets roles and unique permission keys on the authenticated user` -- [ ] `it defines getPermissionsForRoles on RoleRepositoryInterface` +- [x] `it loads the same permission set as the per-role implementation` +- [x] `it deduplicates permission keys shared across roles` +- [x] `it issues exactly one permissions query for a user with multiple roles` +- [x] `it issues no permissions query when the user has no roles` +- [x] `it returns an empty array from getPermissionsForRoles without querying when given no role ids` +- [x] `it queries permissions with a WHERE role_id IN clause whose placeholder count matches the role ids` +- [x] `it sets roles and unique permission keys on the authenticated user` +- [x] `it defines getPermissionsForRoles on RoleRepositoryInterface` ## Acceptance Criteria - All requirements have passing tests @@ -38,4 +39,11 @@ - No decrease in test coverage ## Implementation Notes -(Left blank - filled in by programmer during implementation) +- Added `getPermissionsForRoles(array $roleIds): array` to `RoleRepositoryInterface` with `@throws EntityException` and `@param array $roleIds` +- Implemented in `RoleRepository` with `SELECT DISTINCT p.* FROM permissions p INNER JOIN role_permissions rp ON p.id = rp.permission_id WHERE rp.role_id IN (?, ...)` — placeholders built from count of role ids +- Empty `$roleIds` short-circuits to `[]` with no query issued (SQL syntax error protection) +- Rewired `AdminUserProvider::loadRolesAndPermissions` to collect non-null role ids and call `getPermissionsForRoles` once; `array_unique` on permission keys preserved +- Added `@throws EntityException` to `loadRolesAndPermissions`, `retrieveById`, `retrieveByCredentials`, `retrieveByRememberToken` +- Added `getPermissionsForRoles` to `createMockRoleRepo` anonymous readonly class in `AdminUserProviderTest` — returns deduplicated union across the requested ids +- Added `TrackingRoleRepo` named class in `AdminUserProviderTest` with `getPermissionsForRole` throwing (to assert it is never called) and `batchCallCount`/`batchRoleIdsReceived` tracking +- Updated `$expectedMethods` list in `RoleRepositoryInterfaceTest` to include `getPermissionsForRoles`; test title left unchanged (it does not enumerate all method names) diff --git a/.claude/plans/tier4-performance/005-admin-auth-sync-transactional-batch.md b/.claude/plans/tier4-performance/005-admin-auth-sync-transactional-batch.md index abdbc535..59299ab8 100644 --- a/.claude/plans/tier4-performance/005-admin-auth-sync-transactional-batch.md +++ b/.claude/plans/tier4-performance/005-admin-auth-sync-transactional-batch.md @@ -1,6 +1,6 @@ # Task 005: F3 (optional) — Transactional, batched `syncPermissions` / `syncRoles` -**Status**: pending +**Status**: complete **Depends on**: [004] **Retry count**: 0 @@ -16,17 +16,18 @@ Cheap correctness+perf improvement folded into the F3 cluster; depends on 004 to - `packages/database/src/Connection/TransactionInterface.php` and `Repository::insertBatch` (conditional-transaction guard: only begin/commit when the connection implements `TransactionInterface` and is not already in a transaction — mirror this) - `packages/admin-auth/tests/` (RoleRepository tests) - Patterns to follow: - - Begin a transaction only when supported (same guard `insertBatch` uses); on success commit, on `Throwable` rollback and rethrow. - - Replace the per-id INSERT loop with a single multi-row `INSERT INTO role_permissions (role_id, permission_id) VALUES (?,?),(?,?)...` (chunk if the id count is large). + - Begin a transaction only when supported (same guard `insertBatch` uses at lines 334-338: `$this->connection instanceof TransactionInterface && !$this->connection->inTransaction()`); on success commit, on `Throwable` rollback and rethrow. + - Replace the per-id INSERT loop with a single multi-row `INSERT INTO role_permissions (role_id, permission_id) VALUES (?,?),(?,?)...` (chunk if the id count is large; use a typed rows-per-chunk constant kept well within the pgsql 65535-parameter limit). - Empty permission-id list still clears existing rows (DELETE runs) and issues no INSERT. + - The "supports transactions" test needs a stub implementing BOTH `ConnectionInterface` AND `TransactionInterface` (with `beginTransaction`/`commit`/`rollback`/`inTransaction`); the "does not support transactions" test uses a plain `ConnectionInterface` stub. EITHER stub MUST implement `driverName(): string` (Tier 2 interface addition) or it fatals at instantiation. ## Requirements (Test Descriptions) -- [ ] `it replaces a role's permissions with the new set` -- [ ] `it clears all permissions when given an empty permission id list` -- [ ] `it inserts all new permissions in a single multi-row insert` -- [ ] `it wraps the delete and insert in one transaction when the connection supports transactions` -- [ ] `it rolls back and leaves permissions unchanged when an insert fails mid-sync` -- [ ] `it still syncs when the connection does not support transactions` +- [x] `it replaces a role's permissions with the new set` +- [x] `it clears all permissions when given an empty permission id list` +- [x] `it inserts all new permissions in a single multi-row insert` +- [x] `it wraps the delete and insert in one transaction when the connection supports transactions` +- [x] `it rolls back and leaves permissions unchanged when an insert fails mid-sync` +- [x] `it still syncs when the connection does not support transactions` ## Acceptance Criteria - All requirements have passing tests @@ -34,4 +35,7 @@ Cheap correctness+perf improvement folded into the F3 cluster; depends on 004 to - No decrease in test coverage ## Implementation Notes -(Left blank - filled in by programmer during implementation) +- Replaced per-row INSERT loop in `syncPermissions` with a single multi-row batched INSERT, chunked at `SYNC_ROWS_PER_CHUNK = 500` rows per chunk (well within pgsql 65535-parameter limit) +- Added conditional transaction guard mirroring `Repository::insertBatch`: opens a transaction only when the connection implements `TransactionInterface` and is not already in a transaction; commits on success, rolls back and rethrows on `Throwable` +- Replaced the old `'syncs permissions for a role via syncPermissions'` test (which expected per-row INSERTs) with six new tests covering all requirements +- Added `createRoleTransactionalConnectionWithHistory` helper implementing both `ConnectionInterface` and `TransactionInterface` with `failOnInsert` flag for rollback testing; `inTransaction()` derives state from the log so no external flag is needed diff --git a/.claude/plans/tier4-performance/006-redis-multikey-batch.md b/.claude/plans/tier4-performance/006-redis-multikey-batch.md index 50132eac..abf93576 100644 --- a/.claude/plans/tier4-performance/006-redis-multikey-batch.md +++ b/.claude/plans/tier4-performance/006-redis-multikey-batch.md @@ -1,35 +1,51 @@ # Task 006: F4 — Batch Redis multi-key ops (MGET / pipeline / variadic DEL) -**Status**: pending +**Status**: complete **Depends on**: [none] **Retry count**: 0 ## Description -`RedisCacheDriver::getMultiple`, `setMultiple`, and `deleteMultiple` each loop the single-key `get`/`set`/`delete` methods, producing N Redis round-trips for N keys. Predis supports MGET for batched reads, command pipelining for batched TTL writes, and a variadic DEL. Replace the loops with one MGET, one pipeline, and one variadic DEL respectively, preserving all current multi-key semantics. +`RedisCacheDriver::getMultiple`, `setMultiple`, and `deleteMultiple` each loop the single-key `get`/`set`/`delete` methods, producing N Redis round-trips for N keys. Predis supports MGET for batched reads, command pipelining for batched TTL writes, and a variadic DEL. Replace the loops with one MGET, one pipeline, and one variadic DEL respectively, preserving all current multi-key semantics — **including Tier 1's HMAC value signing**. + +## Description (CRITICAL — Tier 1 HMAC signer must be preserved) +The CURRENT driver wraps every stored value through `CacheValueSigner` (Tier 1, security feature): +- `set()` stores `$this->cacheValueSigner->wrap(serialize($value))` (envelope format `<64-hex-hmac>.`). +- `get()` returns `unserialize($this->cacheValueSigner->verifyAndUnwrap($data))`. + +The batch paths MUST go through the SAME signer — NOT raw `serialize`/`unserialize`: +- `setMultiple` MUST `$this->cacheValueSigner->wrap(serialize($value))` for each pair before queueing it in the pipeline. +- `getMultiple` MUST `unserialize($this->cacheValueSigner->verifyAndUnwrap($data))` for each non-null MGET result, applying `$default` for nulls. + +If you bypass the signer: (1) the EXISTING tests `sets multiple keys` and `gets multiple keys` FAIL, because a `setMultiple(...)` followed by `get('key1')` round-trips through `get()` → `verifyAndUnwrap()` and throws `TamperedCacheValueException` on an unsigned value; and (2) you silently strip tamper protection from the multi-key path. `increment()` (Tier 1, lines 172-188) is NOT routed through the signer — do NOT touch it and do NOT route batch values around the signer "to match increment". ## Context - Related files: - - `packages/cache-redis/src/Driver/RedisCacheDriver.php` (`getMultiple`/`setMultiple`/`deleteMultiple` ~132-160; single-key `get`/`set`/`delete` for serialization, prefixing, TTL, and `validateKey` reference) - - `packages/cache-redis/tests/Unit/...RedisCacheDriverTest.php` (`MockRedisClient extends Predis\Client` with public `$storage`/`$ttls`; behavioral expectations for getMultiple/setMultiple/deleteMultiple already exist — keep them green and add batching assertions) + - `packages/cache-redis/src/Driver/RedisCacheDriver.php` (`getMultiple` ~129-140, `setMultiple` ~145-154, `deleteMultiple` ~159-167; single-key `get`/`set` for the `CacheValueSigner` wrap/verify, prefixing, TTL, and `validateKey` reference) + - `packages/cache-redis/src/Signer/CacheValueSigner.php` (`wrap(string): string` / `verifyAndUnwrap(string): string`) + - `packages/cache-redis/tests/Unit/RedisCacheDriverTest.php` (`MockRedisClient extends Predis\Client` with public `$storage`/`$ttls` at line 19; behavioral expectations for getMultiple/setMultiple/deleteMultiple at lines 351-398 already exist — keep them green and add batching assertions) - Patterns to follow: - - `getMultiple`: prefix + `validateKey` all keys, one `mget(...$prefixedKeys)`, then map back in input-key order — unserialize hits, use `$default` for nulls. - - `setMultiple`: open a Predis `pipeline`, queue `setex` (TTL>0, default TTL applies when `$ttl` is null) or `set` (TTL 0) per pair, execute once; return `true`. - - `deleteMultiple`: one `del(...$prefixedKeys)`; return `true`. - - Extend `MockRedisClient` to implement `mget`, a `pipeline` callback collecting queued commands, and a variadic `del`, recording invocation counts so tests can assert a single batched call. + - `getMultiple`: `validateKey` + prefix all keys, one `mget(...$prefixedKeys)`, then map back in input-key order — for each non-null hit `unserialize($this->cacheValueSigner->verifyAndUnwrap($value))`, use `$default` for nulls. + - `setMultiple`: open a Predis `pipeline`, queue `setex($prefixedKey, $ttl, $this->cacheValueSigner->wrap(serialize($value)))` (TTL>0, default TTL applies when `$ttl` is null) or `set($prefixedKey, $envelope)` (TTL 0) per pair, execute once; return `true`. `validateKey` every key first. + - `deleteMultiple`: one `del(...$prefixedKeys)`; return `true`. (The existing `MockRedisClient::del` at lines 71-93 is ALREADY variadic and flattens array args — do not change its signature; just call it variadically from the driver.) + - Extend `MockRedisClient` to add `mget(...$keys): array` (values in argument order, null for misses, reading `$storage`) and a `pipeline(callable $callback): array` that invokes the callback with a recorder collecting queued `setex`/`set` commands and applies them to `$storage`/`$ttls`. Record invocation counts so tests can assert exactly one MGET / one pipeline execution / one DEL. + - `@throws` on `getMultiple`/`setMultiple` must include `TamperedCacheValueException` (matching `get`/`set`); `InvalidKeyException` on all three. - Independent of any Tier 1 `CacheInterface::increment`; do not touch it. ## Requirements (Test Descriptions) -- [ ] `it returns values for all requested keys via getMultiple` -- [ ] `it returns the default for missing keys in getMultiple` -- [ ] `it preserves input key order in the getMultiple result` -- [ ] `it issues a single MGET for getMultiple instead of one get per key` -- [ ] `it stores all pairs via setMultiple with the given TTL` -- [ ] `it applies the default TTL in setMultiple when none is given` -- [ ] `it stores persistent pairs without a TTL when setMultiple TTL is zero` -- [ ] `it issues the setMultiple writes in a single pipeline` -- [ ] `it deletes all given keys via deleteMultiple` -- [ ] `it issues a single variadic DEL for deleteMultiple` -- [ ] `it validates every key in the multi-key operations` +- [x] `it returns values for all requested keys via getMultiple` +- [x] `it returns the default for missing keys in getMultiple` +- [x] `it preserves input key order in the getMultiple result` +- [x] `it issues a single MGET for getMultiple instead of one get per key` +- [x] `it verifies the HMAC envelope on each value read by getMultiple` +- [x] `it stores all pairs via setMultiple with the given TTL` +- [x] `it applies the default TTL in setMultiple when none is given` +- [x] `it stores persistent pairs without a TTL when setMultiple TTL is zero` +- [x] `it issues the setMultiple writes in a single pipeline` +- [x] `it stores each setMultiple value as a signed HMAC envelope` +- [x] `it round-trips values written by setMultiple back through getMultiple` +- [x] `it deletes all given keys via deleteMultiple` +- [x] `it issues a single variadic DEL for deleteMultiple` +- [x] `it validates every key in the multi-key operations` ## Acceptance Criteria - All requirements have passing tests @@ -37,4 +53,9 @@ - No decrease in test coverage ## Implementation Notes -(Left blank - filled in by programmer during implementation) +- `getMultiple`: validates all keys first, then calls `mget(...$prefixedKeys)` for a single round-trip; maps results back by input-key index, applying `verifyAndUnwrap` + `unserialize` on hits and `$default` for nulls. +- `setMultiple`: validates all keys first, resolves TTL (null → default), then calls `pipeline(callable)` once; the callback queues `setex` (TTL>0) or `set` (TTL 0) per pair with `wrap(serialize($value))` envelopes. +- `deleteMultiple`: validates all keys first, then calls `del(...$prefixedKeys)` as a single variadic call. +- `MockRedisClient` extended with `mget`, `pipeline` (via `MockPipelineRecorder`), and invocation counters (`mgetCount`, `pipelineCount`, `delCount`). `del` counter also added. +- `MockPipelineRecorder` added as a named class at file top — applies `setex`/`set` commands directly to the mock client's `storage`/`ttls`, keeping the HMAC round-trip intact. +- All HMAC signing paths preserved: `setMultiple` wraps via signer, `getMultiple` verifies via signer. TamperedCacheValueException propagates correctly. diff --git a/.claude/plans/tier4-performance/007-notification-batch-fanout.md b/.claude/plans/tier4-performance/007-notification-batch-fanout.md index 1dbda35a..982d1491 100644 --- a/.claude/plans/tier4-performance/007-notification-batch-fanout.md +++ b/.claude/plans/tier4-performance/007-notification-batch-fanout.md @@ -1,6 +1,6 @@ # Task 007: F5 — Batched database-channel notification fan-out -**Status**: pending +**Status**: complete **Depends on**: [none] **Retry count**: 0 @@ -22,23 +22,23 @@ The sender resolves channels **per recipient** (the current code calls `$notific - `packages/notification/src/Channel/DatabaseChannel.php` (`send` — single raw INSERT with id/type/notifiable_type/notifiable_id/data/read_at/created_at + `generateUuid`) - `packages/notification/src/Contracts/ChannelInterface.php` (UNCHANGED — do not edit) - new `packages/notification/src/Contracts/BatchChannelInterface.php` (add this; `sendMany(array, NotificationInterface): void`, `@throws ChannelException`) - - `packages/notification/tests/Unit/NotificationSenderTest.php` and `tests/Unit/Channel/DatabaseChannelTest.php` (existing per-recipient send + mock-`ConnectionInterface` patterns to extend) + - `packages/notification/tests/Unit/NotificationSenderTest.php` (existing tests use PHPUnit `createMock(ChannelInterface::class)` — the existing `database`-channel mocks are plain `ChannelInterface`, NOT `BatchChannelInterface`, so they correctly fall back to per-recipient `send()` and stay green; register a real `DatabaseChannel` or a `BatchChannelInterface` mock for the new batch test) and `tests/Unit/Channel/DatabaseChannelTest.php` (uses PHPUnit `createMock(ConnectionInterface::class)`; the new batch query-count test needs a hand-written anonymous stub instead, which must implement `driverName()`) - `packages/database/src/Repository/Repository.php` `insertBatch` (~314-323 placeholder/multi-row VALUES construction shape only — NOT the PK loop) - Patterns to follow: - - `DatabaseChannel::sendMany`: build one multi-row `INSERT INTO notifications (id, type, notifiable_type, notifiable_id, data, read_at, created_at) VALUES (?,?,?,?,?,?,?),(...)` over all recipients, chunked so the placeholder count stays within driver limits; each row carries the same column data the single-send path writes (fresh UUID per row via `generateUuid`, notification class name, `json_encode` of `toDatabase($notifiable)` per recipient, null `read_at`, current timestamp). A failed `execute()` is wrapped in `ChannelException::deliveryFailed('database', ...)` exactly as `send()` does. - - The sender groups recipients by their per-recipient-resolved channel name before dispatch; non-`BatchChannelInterface` channels and single-recipient groups use the existing `send()` path (preserving `ChannelException`/`NotificationException` wrapping). - - Query-count assertions: anonymous `ConnectionInterface` stub recording each `execute()`; assert the number of INSERT statements equals the chunk count (one when N fits a single chunk), that N rows' worth of bindings are present, and each row's UUID is distinct. + - `DatabaseChannel::sendMany`: build one multi-row `INSERT INTO notifications (id, type, notifiable_type, notifiable_id, data, read_at, created_at) VALUES (?,?,?,?,?,?,?),(...)` over all recipients, chunked so the placeholder count stays within driver limits (use a typed rows-per-chunk constant); each row carries the same column data the single-send path writes (fresh UUID per row via `generateUuid`, `$notification::class`, `(string) $notifiable->getNotifiableId()`, `$notifiable->getNotifiableType()`, `json_encode($notification->toDatabase($notifiable), JSON_THROW_ON_ERROR)` per recipient, null `read_at`, `date('Y-m-d H:i:s')`). `generateUuid()` calls `random_bytes()` (`RandomException`) and the json_encode throws `JsonException` — wrap the ENTIRE per-chunk build+execute in `try { ... } catch (Throwable $e) { throw ChannelException::deliveryFailed('database', $e->getMessage()); }` exactly as `send()` does, so UUID/JSON failures are wrapped too. Declare `@throws ChannelException` on `sendMany`. + - The sender groups recipients by their per-recipient-resolved channel name (resolved via `$this->manager->channel($channelName)`, which throws `NotificationException::unknownChannel` — keep that resolution so the existing unknown-channel test stays green). For each channel: if the resolved channel `instanceof BatchChannelInterface` and the group has >1 recipient, call `sendMany($group, $notification)`, ELSE call `send()` per recipient. BOTH the `sendMany` call and the per-recipient `send` calls MUST be wrapped in the SAME try/catch the current `send` loop uses (lines 40-47): `catch (ChannelException) { rethrow }` / `catch (Throwable) { throw NotificationException::sendFailed($channelName, ...) }`. Do not let the batch path skip this wrapping. + - Query-count assertions: a NEW anonymous `ConnectionInterface` stub recording each `execute()` (it MUST implement `driverName(): string` — Tier 2 interface addition — or it fatals; the existing `DatabaseChannelTest` uses PHPUnit `createMock` which auto-stubs `driverName`, but a hand-written stub does not). Assert the number of INSERT statements equals the chunk count (one when N fits a single chunk), that N rows' worth of bindings are present, and each row's UUID is distinct. ## Requirements (Test Descriptions) -- [ ] `it persists a notification for every recipient on the database channel` -- [ ] `it issues a single multi-row insert when all recipients fit one chunk` -- [ ] `it issues one insert per chunk when recipients exceed the chunk size` -- [ ] `it writes the same column data per row as the single-recipient send` -- [ ] `it generates a distinct id for each persisted notification row` -- [ ] `it falls back to per-recipient send for channels without batch support` -- [ ] `it routes each recipient only to the channels that recipient declared` -- [ ] `it wraps a batch insert failure in a channel exception` -- [ ] `it leaves MailChannel and other ChannelInterface implementations unchanged` +- [x] `it persists a notification for every recipient on the database channel` +- [x] `it issues a single multi-row insert when all recipients fit one chunk` +- [x] `it issues one insert per chunk when recipients exceed the chunk size` +- [x] `it writes the same column data per row as the single-recipient send` +- [x] `it generates a distinct id for each persisted notification row` +- [x] `it falls back to per-recipient send for channels without batch support` +- [x] `it routes each recipient only to the channels that recipient declared` +- [x] `it wraps a batch insert failure in a channel exception` +- [x] `it leaves MailChannel and other ChannelInterface implementations unchanged` ## Acceptance Criteria - All requirements have passing tests @@ -46,4 +46,8 @@ The sender resolves channels **per recipient** (the current code calls `$notific - No decrease in test coverage ## Implementation Notes -(Left blank - filled in by programmer during implementation) +- Added `BatchChannelInterface` at `src/Contracts/BatchChannelInterface.php` with `sendMany(array $notifiables, NotificationInterface): void` +- `DatabaseChannel` now implements both `ChannelInterface` and `BatchChannelInterface`; `sendMany` builds chunked multi-row INSERTs with `ROWS_PER_CHUNK = 500` (7 cols × 500 = 3500 placeholders, within driver limits) +- `NotificationSender::send` refactored to build a channel-name → recipients map first, then for each channel: calls `sendMany` when the channel implements `BatchChannelInterface` and >1 recipient; falls back to per-recipient `send` otherwise +- `ChannelInterface` left completely unchanged +- Test coverage: `DatabaseChannelBatchTest.php` uses a hand-written `BatchTestConnection` stub (implements `driverName()`) to count `execute()` calls and inspect bindings directly diff --git a/.claude/plans/tier4-performance/_devils_advocate.md b/.claude/plans/tier4-performance/_devils_advocate.md new file mode 100644 index 00000000..ba565062 --- /dev/null +++ b/.claude/plans/tier4-performance/_devils_advocate.md @@ -0,0 +1,68 @@ +# Devil's Advocate Review: tier4-performance + +Reviewed against the CURRENT tree on `feature/tier4-performance` (Tier 1 + 2 + 3 merged). All cited classes/methods were cross-checked against source. + +## Critical (Must fix before building) + +### C1 — Task 006 (F4) must preserve Tier 1's HMAC value signer, or it breaks existing tests AND silently disables tamper protection +`packages/cache-redis/src/Driver/RedisCacheDriver.php` wraps every stored value through `CacheValueSigner`: +- `set()`: `$this->cacheValueSigner->wrap(serialize($value))` +- `get()`: `unserialize($this->cacheValueSigner->verifyAndUnwrap($data))` +- envelope format is `<64-hex-hmac>.` (verified by `RedisCacheDriverTest` lines 428-490). + +Task 006's description says `getMultiple` should "unserialize hits" and `setMultiple` should "queue setex/set per pair" — it never mentions the signer. If a worker takes that literally and writes raw `serialize()`/`unserialize()` in the batch paths: +1. The EXISTING behavioral tests `sets multiple keys` (line 373) and `gets multiple keys` (line 351) will FAIL, because `setMultiple(...)` then `$this->driver->get('key1')` round-trips through `get()`, which calls `verifyAndUnwrap()` on a value that was stored WITHOUT an envelope → throws `TamperedCacheValueException`. +2. Even if the batch read also bypassed the signer, it would silently strip tamper protection from the multi-key path — a production-safety regression of a security feature Tier 1 deliberately added. + +Fix: `setMultiple` must wrap each value `$this->cacheValueSigner->wrap(serialize($value))` before queueing `setex`/`set` in the pipeline; `getMultiple` must `unserialize($this->cacheValueSigner->verifyAndUnwrap($data))` for each non-null MGET result (and apply `$default` for nulls). `@throws` must include `TamperedCacheValueException` (matching `get()`/`set()`). `increment()` (Tier 1, lines 172-188) is NOT routed through the signer and MUST NOT be touched. + +### C2 — Task 006 (F4): the test `MockRedisClient` has no `mget` and no `pipeline`; the existing `del` IS already variadic +`MockRedisClient extends Predis\Client` (`tests/Unit/RedisCacheDriverTest.php` line 19) currently stubs only `get/set/setex/exists/del/keys/ttl/incr/expire`. The task must add `mget(...$keys): array` (returning values in argument order, null for misses) and a `pipeline(callable): array` that invokes the callback with a recorder collecting queued `setex`/`set` calls and applies them to `$storage`/`$ttls`, plus call-count tracking so a single batched invocation can be asserted. Note `del(...$keys)` (lines 71-93) is ALREADY variadic and already flattens array args — `deleteMultiple` can call `del(...$prefixedKeys)` and the mock handles it; do not "add" a variadic del, it exists. The Predis pipeline contract is `pipeline(function ($pipe) {...})` returning an array of results; the mock's recorder must expose `setex`/`set` with the same signatures the driver calls. + +## Important (Should fix before building) + +### I1 — Tasks 001, 002, 004, 005, 007: every NEW inline anonymous `ConnectionInterface` stub MUST implement `driverName(): string` or the suite fatals +Tier 2 added `ConnectionInterface::driverName(): string` (`packages/database/src/Connection/ConnectionInterface.php` line 62). Any anonymous class `implements ConnectionInterface` that omits it is abstract-incomplete and fatals at instantiation. The existing reference stubs already include it (`RepositoryCrudTest` line 159, `createRoleMockConnectionWithHistory` line 256) returning `'sqlite'`, but the task files don't tell a worker writing a fresh stub to include it. PHPUnit `createMock(ConnectionInterface::class)` auto-stubs it, so only hand-written anonymous stubs are affected (tasks 001/002/004/005/007's query-count stubs). Add an explicit note to each affected task. + +### I2 — Tasks 001 & 002: line references are stale; `exists`/`existsBy` currently DELEGATE, which interacts with task ordering +Current line numbers in `Repository.php`: `findOneBy` is 232-236 (task 001 correct), but `exists` is 541-545 (task 002 says ~517-521), `existsBy` is 552-556 (task says ~528-532), `isColumnUnique` is 561-582 (task says ~542-557). More important than the drift: `exists()` delegates to `find()` and `existsBy()` delegates to `findOneBy()` (lines 544, 555). Task 001 changes `findOneBy` to a `LIMIT 1` path; if Task 002's worker leaves `existsBy` delegating to the new `findOneBy`, it still hydrates an entity (the LIMIT-1 path returns a full hydrated+eager-loaded entity) — violating Task 002's "the probe must never construct an entity" requirement. Task 002 must replace both `exists` and `existsBy` bodies with dedicated `SELECT 1 ... LIMIT 1` probes, NOT keep delegating. Update the line refs and make the no-delegation requirement explicit. + +### I3 — Task 004 (F3): `getPermissionsForRoles` hydrates Permissions, so it `@throws EntityException`; the description omits this +`getPermissionsForRole` (RoleRepository line 98) declares `@throws EntityException` because `hydrator->hydrate()` throws it. The new `getPermissionsForRoles` mirrors that hydration and must carry the same `@throws EntityException`, which then propagates up through `loadRolesAndPermissions` and `AdminUserProvider::retrieveById`/`retrieveByCredentials`/`retrieveByRememberToken`. The interface method and the provider call chain must declare `@throws EntityException` per code-standards rule 9. (The provider's `retrieveBy*` methods currently declare no `@throws`; adding the propagation is required for lint.) + +### I4 — Task 004 (F3): the provider entry point is `retrieveByRememberToken`, not `retrieveByToken` +Both `_plan.md` and Task 004 say `loadRolesAndPermissions` is reached via "`retrieveByToken`". The actual method is `retrieveByRememberToken` (AdminUserProvider line 73). Minor naming, but a worker grepping for `retrieveByToken` will not find it. Correct the references so the worker traces the right call sites (`retrieveById` line 22, `retrieveByCredentials` line 40, `retrieveByRememberToken` line 73). + +### I5 — Task 004 (F3): `createMockRoleRepo` is a `readonly class` and already implements `existsBy`; the new method must follow the same shape +The mock at `AdminUserProviderTest.php` line 318 is `new readonly class (...) implements RoleRepositoryInterface` and its `permissionsMap` is keyed by role id (line 324). Task 004 already calls this out, but note the mock also implements `existsBy` (line 356) which is NOT on `RoleRepositoryInterface` directly — it's inherited from `RepositoryInterface`. The worker must add `getPermissionsForRoles(array $roleIds): array` returning the union across the requested ids from `permissionsMap`, deduplicated, and keep the class `readonly`. Confirmed: this is the only in-repo anonymous implementer of the interface besides `RoleRepository` itself. + +### I6 — Task 007 (F5): the sender's batch (`sendMany`) dispatch needs the SAME error wrapping the per-recipient path has +`NotificationSender::send` (lines 40-47) wraps each `channel->send()` in `try { } catch (ChannelException) { rethrow } catch (Throwable) { throw NotificationException::sendFailed(...) }`. When F5 routes a group to `channel->sendMany($group, $notification)`, that call must be wrapped in the identical try/catch (ChannelException passes through, other Throwable → `NotificationException::sendFailed($channelName, ...)`). The task says the fallback path preserves wrapping but is silent on the batch path. Also: `$this->manager->channel($channelName)` throws `NotificationException::unknownChannel` — the grouping/dispatch must resolve channels through the manager exactly as today so the existing test `throws NotificationException when notification declares unknown channel` (sender test line 81) stays green. + +### I7 — Task 007 (F5): `sendMany` must declare `@throws ChannelException` AND handle `RandomException`/`JsonException` it raises internally +`DatabaseChannel::send` calls `random_bytes()` (→ `RandomException`) via `generateUuid()` and `json_encode(..., JSON_THROW_ON_ERROR)` (→ `JsonException`), both swallowed by its `catch (Throwable)` → `ChannelException::deliveryFailed('database', ...)`. `sendMany` builds N UUIDs and N json_encodes, so it must wrap the whole multi-row build+execute in the same `try/catch (Throwable) { throw ChannelException::deliveryFailed('database', ...) }` and declare `@throws ChannelException`. The task mentions wrapping a failed `execute()` but not the per-row `generateUuid`/`json_encode` failures — make the whole-operation wrapping explicit. + +### I8 — Task 003 (F2): `MediaRepositoryInterface::find` takes `int`, so `findMany(array $ids)` ids are `int`; the `MediaManagerTest` mock is at line 244, not ~228 +`MediaRepositoryInterface::find(int $id)` (line 19) — ids are `int`. `MediaAttachmentRepositoryInterface::findByAttachable` returns `array` (line 24). So `findMany(array $ids): array` takes `array`. The second in-repo implementer is `MediaManagerTest::makeRepository` at line 244-278 (task says "~line 228" — stale). Both that mock and `AttachmentManagerTest::makeMediaRepository` (line 76) must add `findMany`. Confirmed there are exactly two in-repo implementers and NO concrete `MediaRepository` class — the task's premise holds. Note: `find()` (and therefore the mock's `findMany`) returns entities keyed only by id presence; the contract should document `findMany` returns a flat `array` of matched rows in unspecified order (AttachmentManager reorders). + +## Minor (Nice to address) + +### M1 — Task 004: consider adding a `getPermissionsForRoles` signature test to `RoleRepositoryInterfaceTest` +That file has per-method signature tests (e.g. `getPermissionsForRole method signature requires int and returns array`, lines 58-69) in addition to the `$expectedMethods` presence list. Adding a parallel signature test for `getPermissionsForRoles` (param `roleIds`, type `array`, returns `array`) would match the existing convention, though only the `$expectedMethods` addition is strictly required to avoid a fatal. + +### M2 — Task 006: the test file is `packages/cache-redis/tests/Unit/RedisCacheDriverTest.php` +Task 006 references `tests/Unit/...RedisCacheDriverTest.php` with an ellipsis; the actual path has no extra subdirectory. Minor, but worth pinning so the worker edits the right file. + +### M3 — F5 grouping order vs. existing test expectations +The existing sender tests assert `send` is called the right number of times but not the order across channels. F5's channel-grouping changes iteration from per-notifiable-then-channel to per-channel-then-group. Current tests don't assert cross-channel ordering, so they stay green, but the new heterogeneous-channels test should pin per-recipient routing (already in task requirement `it routes each recipient only to the channels that recipient declared`). + +## Questions for the Team + +### Q1 — Should the empty-`$values` case of `setMultiple` (F4) still open a pipeline? +An empty `setMultiple([])` opening an empty Predis pipeline is a wasted round-trip. Worth a no-op guard (`if ($values === []) return true;`)? Same question for `getMultiple([])` (skip MGET) and `deleteMultiple([])` (skip DEL). Low stakes, but consistent with the F3 empty-`IN ()` guard philosophy. + +### Q2 — Does `findMany` (F2) need a deterministic order in the interface contract, or is PHP-side reordering the permanent answer? +The plan documents `findMany` returns unspecified order and `AttachmentManager` reorders. That's fine, but if other consumers call `findMany` directly they'll get arbitrary order. Acceptable for now; flagging in case a future caller assumes input-order output. + +### Q3 — Chunk size constant for F5 multi-row INSERT and F3 batched INSERT +Both Task 005 and Task 007 say "chunk so placeholder count stays within driver limits" without naming a value. Worth fixing a shared, typed constant (e.g. rows-per-chunk) so the two batch paths are consistent and the limit is documented? (pgsql max params 65535; a conservative rows-per-chunk avoids it.) diff --git a/.claude/plans/tier4-performance/_plan.md b/.claude/plans/tier4-performance/_plan.md index d7891bf8..786f601b 100644 --- a/.claude/plans/tier4-performance/_plan.md +++ b/.claude/plans/tier4-performance/_plan.md @@ -4,7 +4,7 @@ 2026-06-10 ## Status -ready +completed ## Objective Eliminate N+1 query loops and missing `LIMIT 1` optimizations on hot lookup, attachment, admin-auth, Redis multi-key, and notification fan-out paths so each operation issues a bounded, batched number of queries/round-trips instead of one per row/key/recipient. @@ -18,7 +18,7 @@ none - `database-package` owns `Repository.php` (F1). F5 does NOT reuse `Repository::insertBatch()` (notifications have no Entity/auto-increment PK — see below); it only mirrors the multi-row VALUES placeholder shape. This plan only changes the `findOneBy`/`exists`/`existsBy`/`isColumnUnique` SQL and adds no new public surface there beyond `LIMIT 1`. - `repository-lifecycle-events` adds the `EntityCreating/Created` dispatch in `insertBatch()`; F5 does not touch `insertBatch` so the event contract is unaffected. - `orm-relationships` owns `eagerLoadRelationships()` — F1 must keep eager-loading semantics identical for `findOneBy` (it still returns a fully hydrated, eager-loaded entity via `$this->pendingRelationships`, just capped at one row; no `ORDER BY` introduced so the chosen row is unchanged). -- `cache-redis` owns `RedisCacheDriver.php` (F4). Tier 1 may have added `CacheInterface::increment`; F4 is independent of it (multi-key get/set/delete only) but the implementer must rebase cleanly if `increment` landed. +- `cache-redis` owns `RedisCacheDriver.php` (F4). **Tier 1 added a `CacheValueSigner` HMAC envelope — `set()` stores `cacheValueSigner->wrap(serialize($value))` and `get()` returns `unserialize(cacheValueSigner->verifyAndUnwrap($data))`. F4's batch paths MUST route through the SAME signer (wrap on `setMultiple`, verify+unwrap on `getMultiple`) — not raw serialize/unserialize — or the existing round-trip tests fail with `TamperedCacheValueException` and tamper protection is silently stripped from the multi-key path.** Tier 1 also added an atomic `increment()` that is NOT signed; F4 does not touch it and rebases cleanly. - `auth-package` / `admin-system` own `AdminUserProvider` and `RoleRepository` (F3). F3 adds one batch method to `RoleRepositoryInterface` + `RoleRepository` and rewires `loadRolesAndPermissions`. **Adding to the interface forces updating the `createMockRoleRepo` anonymous-class implementer in `AdminUserProviderTest.php` (and the `getPermissionsForRole` reflection list in `RoleRepositoryInterfaceTest.php`) in the same task, or the suite fatals.** - `notification` owns `NotificationSender` + `DatabaseChannel` (F5). F5 adds a NEW `BatchChannelInterface` (opt-in) rather than modifying `ChannelInterface` — modifying the shared `ChannelInterface` would break `MailChannel`, the docs `SmsChannel`, and all third-party channels. - `media` ships only `MediaRepositoryInterface` — **there is no concrete `MediaRepository` class** in the package. F2 adds `findMany` to the interface, rewires `AttachmentManager`, and updates the two in-repo test mocks that implement the interface (`AttachmentManagerTest`, `MediaManagerTest`); the single-query SQL implementation is the consumer's responsibility, documented in the contract. @@ -28,7 +28,7 @@ none - rabbitmq failed-job scan — Tier 2 F4. Reference only. - Per-request discovery scan / discovery cache — separate `discovery-cache` plan. Reference only. -**How query-count is tested.** This codebase has no dedicated counting/fake `ConnectionInterface` in `marko/testing`. The established pattern (see `packages/database/tests/Feature/RepositoryCrudTest.php` ~lines 47-130) is an inline anonymous class implementing `ConnectionInterface` with a `public array &$queries` reference property that appends `['sql' => $sql, 'bindings' => $bindings, 'type' => 'query'|'execute']` on every `query()`/`execute()` call. Tests then: +**How query-count is tested.** This codebase has no dedicated counting/fake `ConnectionInterface` in `marko/testing`. The established pattern (see `packages/database/tests/Feature/RepositoryCrudTest.php` ~lines 47-163) is an inline anonymous class implementing `ConnectionInterface` with a `public array &$queries` reference property that appends `['sql' => $sql, 'bindings' => $bindings, 'type' => 'query'|'execute']` on every `query()`/`execute()` call. **Tier 2 added `ConnectionInterface::driverName(): string` — every hand-written anonymous `ConnectionInterface` stub MUST implement it (returning e.g. `'sqlite'`) or it fatals at instantiation as abstract-incomplete. The reference stubs already do (RepositoryCrudTest line 159, RoleRepositoryTest line 256).** Tests then: - assert `count($queries)` (e.g. exactly one SELECT regardless of N ids/roles), - assert SQL substrings (`str_contains($sql, 'LIMIT 1')`, `WHERE id IN (?, ?, ?)`, a single multi-row `INSERT ... VALUES (?,?),(?,?)`), - and return canned rows so the observable result (entity set / bool / permission set) can be asserted identical to the pre-change behavior. @@ -66,31 +66,33 @@ Each task writes the behavioral (correct-result) assertions FIRST, then the quer ## Task Overview | Task | Description | Depends On | Status | |------|-------------|------------|--------| -| 001 | F1: `LIMIT 1` for `findOneBy` | - | pending | -| 002 | F1: `SELECT 1 ... LIMIT 1` for `exists`/`existsBy`/`isColumnUnique` | 001 | pending | -| 003 | F2: add `findMany` to `MediaRepositoryInterface` (+ test mocks) + single-call `findByAttachable` | - | pending | -| 004 | F3: `getPermissionsForRoles` batch query (+ interface mocks) + rewire `loadRolesAndPermissions` | - | pending | -| 005 | F3 (optional): transactional + batched `syncPermissions` (no `syncRoles`) | 004 | pending | -| 006 | F4: batched Redis multi-key ops (MGET / pipeline / variadic DEL) | - | pending | -| 007 | F5: new `BatchChannelInterface` + `DatabaseChannel::sendMany` + sender grouping | - | pending | +| 001 | F1: `LIMIT 1` for `findOneBy` | - | completed | +| 002 | F1: `SELECT 1 ... LIMIT 1` for `exists`/`existsBy`/`isColumnUnique` | 001 | completed | +| 003 | F2: add `findMany` to `MediaRepositoryInterface` (+ test mocks) + single-call `findByAttachable` | - | completed | +| 004 | F3: `getPermissionsForRoles` batch query (+ interface mocks) + rewire `loadRolesAndPermissions` | - | completed | +| 005 | F3 (optional): transactional + batched `syncPermissions` (no `syncRoles`) | 004 | completed | +| 006 | F4: batched Redis multi-key ops (MGET / pipeline / variadic DEL) | - | completed | +| 007 | F5: new `BatchChannelInterface` + `DatabaseChannel::sendMany` + sender grouping | - | completed | ## Architecture Notes - **F1 — `findOneBy`** currently delegates to `findBy($criteria)->first()`, fetching ALL matching rows + hydrating + eager-loading every one. Add a query path that appends `LIMIT 1` so the DB returns at most one row; the single returned entity must still be eager-loaded (preserve `orm-relationships` behavior). `findBy` itself is unchanged. -- **F1 — exists family** (`exists`, `existsBy`, `isColumnUnique`) currently hydrate full entities or run `SELECT *`. Replace with a boolean probe: `SELECT 1 FROM
WHERE ... LIMIT 1` via `connection->query()`, returning `count($rows) > 0` (or `=== 0` for `isColumnUnique`). No hydration, no eager-load. `exists($id)` keys on the primary-key column; `existsBy` builds the same property->column criteria mapping `findBy` uses; `isColumnUnique` keeps its `excludeId` AND-clause. +- **F1 — exists family** (`exists`, `existsBy`, `isColumnUnique`) currently delegate (`exists`→`find()`, `existsBy`→`findOneBy()`) or run `SELECT *`. **After Task 001 `findOneBy` still returns a hydrated, eager-loaded entity (capped at LIMIT 1), so leaving `existsBy` delegating would STILL hydrate for a bool — Task 002 must replace both bodies with dedicated probes, not keep delegating.** Replace with a boolean probe: `SELECT 1 FROM
WHERE ... LIMIT 1` via `connection->query()`, returning `count($rows) > 0` (or `=== 0` for `isColumnUnique`). No hydration, no eager-load. `exists($id)` keys on the primary-key column; `existsBy` builds the same property->column criteria mapping `findBy` uses; `isColumnUnique` (which is `protected`) keeps its `excludeId` AND-clause. - **F2** — `AttachmentManager::findByAttachable` loops `mediaRepository->find($id)` once per id. Add `MediaRepositoryInterface::findMany(array $ids): array` (returns `array`; empty input -> empty array, no query) to the interface only — `marko/media` ships no concrete repository, so the single `WHERE id IN (...)` query is a documented consumer responsibility. `AttachmentManager` calls `findMany($mediaIds)` once, then re-orders the returned media to the attachment id list and skips ids with no matching media (preserves the current null-skip + ordering). Both in-package test mocks implementing `MediaRepositoryInterface` must add `findMany` or the suite fatals. -- **F3** — `loadRolesAndPermissions` loops `roleRepository->getPermissionsForRole($role->id)` per role inside `retrieveById()` (every authenticated admin request). Add `RoleRepositoryInterface::getPermissionsForRoles(array $roleIds): array` returning the deduplicated permission set across all roles via one `SELECT DISTINCT p.* ... INNER JOIN role_permissions rp ON ... WHERE rp.role_id IN (?, ?, ...)` with placeholder count == role-id count. Rewire `loadRolesAndPermissions` to collect non-null role ids, call once, dedupe permission keys. **Empty role-id list short-circuits to `[]` with no query — never emit `IN ()` (SQL syntax error).** The `createMockRoleRepo` implementer in `AdminUserProviderTest.php` must add the method in the same task. +- **F3** — `loadRolesAndPermissions` loops `roleRepository->getPermissionsForRole($role->id)` per role inside `retrieveById()`, `retrieveByCredentials()`, and `retrieveByRememberToken()` (NOT `retrieveByToken`, which does not exist) — every authenticated admin request. Add `RoleRepositoryInterface::getPermissionsForRoles(array $roleIds): array` returning the deduplicated permission set across all roles via one `SELECT DISTINCT p.* ... INNER JOIN role_permissions rp ON ... WHERE rp.role_id IN (?, ?, ...)` with placeholder count == role-id count. Because it hydrates Permissions it `@throws EntityException` (like `getPermissionsForRole`); propagate that tag through `loadRolesAndPermissions` and the three `retrieveBy*` provider methods. Rewire `loadRolesAndPermissions` to collect non-null role ids, call once, dedupe permission keys. **Empty role-id list short-circuits to `[]` with no query — never emit `IN ()` (SQL syntax error).** The `createMockRoleRepo` implementer (a `readonly class`) in `AdminUserProviderTest.php` must add the method (keeping the class `readonly`) in the same task. - **F3 optional (Task 005)** — `syncPermissions` does `DELETE` then one `INSERT` per permission id with no transaction. Wrap the DELETE + a single batched insert (or chunked multi-row INSERT) in a transaction so a mid-loop failure cannot leave a role half-synced. Apply the same to `syncRoles` if it exists with the same shape. -- **F4** — `RedisCacheDriver` multi-key methods loop single-key `get`/`set`/`delete`. Replace with: `getMultiple` -> one `mget(...prefixedKeys)` then map results (deserialize hits, default for nulls), preserving input-key order and missing-key default. `setMultiple` -> a Predis pipeline issuing `setex` (TTL>0) or `set` (TTL 0) per pair in one round-trip; default TTL still applies. `deleteMultiple` -> one variadic `del(...prefixedKeys)`. All keys validated via existing `validateKey`. Return values unchanged (`bool`/iterable). -- **F5** — `NotificationSender::send` resolves channels per recipient (`$notification->channels($notifiable)`, which can differ per recipient) and calls `channel->send($notifiable, $notification)` once each; `DatabaseChannel::send` runs one raw INSERT. Add a NEW opt-in `BatchChannelInterface` (`sendMany(array $notifiables, NotificationInterface): void`) implemented by `DatabaseChannel` — do NOT add to `ChannelInterface` (breaks `MailChannel`/`SmsChannel`/third-party channels). The sender builds a channel-name -> recipients map from the per-recipient resolution, then for each `BatchChannelInterface` channel with >1 recipient calls `sendMany` once; otherwise falls back to per-recipient `send`. `DatabaseChannel::sendMany` builds one chunked multi-row `INSERT INTO notifications (...) VALUES (...),(...)` directly (UUID PK per row via `generateUuid`, same column data as single-send: id/type/notifiable_type/notifiable_id/data/read_at/created_at), wrapping failures in `ChannelException`. **No dependency on Tier 2 F9** — notifications use explicit UUID PKs, not auto-increment, so the `insertBatch`/`lastInsertId` pgsql bug cannot apply. Chunk size keeps placeholder count within driver limits. +- **F4** — `RedisCacheDriver` multi-key methods loop single-key `get`/`set`/`delete`. Replace with: `getMultiple` -> one `mget(...prefixedKeys)` then map results (`unserialize(cacheValueSigner->verifyAndUnwrap($value))` for each hit, default for nulls), preserving input-key order and missing-key default. `setMultiple` -> a Predis pipeline issuing `setex` (TTL>0) or `set` (TTL 0) per pair in one round-trip, each value `cacheValueSigner->wrap(serialize($value))`; default TTL still applies. `deleteMultiple` -> one variadic `del(...prefixedKeys)` (the mock's `del` is already variadic). All keys validated via existing `validateKey`. **The HMAC signer MUST be preserved on both batch paths** (see Discovery Notes) — `@throws` includes `TamperedCacheValueException`. Return values unchanged (`bool`/iterable). `increment()` is untouched. +- **F5** — `NotificationSender::send` resolves channels per recipient (`$notification->channels($notifiable)`, which can differ per recipient) and calls `channel->send($notifiable, $notification)` once each; `DatabaseChannel::send` runs one raw INSERT. Add a NEW opt-in `BatchChannelInterface` (`sendMany(array $notifiables, NotificationInterface): void`) implemented by `DatabaseChannel` — do NOT add to `ChannelInterface` (breaks `MailChannel`/`SmsChannel`/third-party channels). The sender builds a channel-name -> recipients map from the per-recipient resolution (resolving each channel via `manager->channel($name)`, which throws `NotificationException::unknownChannel`), then for each `BatchChannelInterface` channel with >1 recipient calls `sendMany` once; otherwise falls back to per-recipient `send`. **Both the `sendMany` call and the fallback `send` calls must be wrapped in the SAME try/catch the current loop uses (ChannelException rethrown, other Throwable -> `NotificationException::sendFailed`).** `DatabaseChannel::sendMany` builds one chunked multi-row `INSERT INTO notifications (...) VALUES (...),(...)` directly (UUID PK per row via `generateUuid`, same column data as single-send: id/type/notifiable_type/notifiable_id/data/read_at/created_at), wrapping the WHOLE per-chunk build+execute in `try/catch (Throwable) -> ChannelException::deliveryFailed('database', ...)` so `random_bytes()` (`RandomException`) and `json_encode(JSON_THROW_ON_ERROR)` (`JsonException`) failures are wrapped too; `@throws ChannelException`. **No dependency on Tier 2 F9** — notifications use explicit UUID PKs, not auto-increment, so the `insertBatch`/`lastInsertId` pgsql bug cannot apply. Chunk size (a typed constant) keeps placeholder count within driver limits. ## Risks & Mitigations - **Eager-load regression (F1):** capping `findOneBy` at one row must not drop eager loading. Mitigation: test asserts a `with()`-relationship is still populated on the single result. - **No shared counting fake:** query-count assertions rely on inline anonymous `ConnectionInterface` stubs. Mitigation: each task's test description specifies the stub records `['sql','bindings','type']` and asserts on `count()` + SQL substrings, mirroring `RepositoryCrudTest`. -- **Predis pipeline/MGET mock fidelity (F4):** the existing `MockRedisClient` only stubs `get/set/setex/del/exists/keys/ttl`. Mitigation: extend the mock to implement `mget` and `pipeline` (collecting queued commands) and a variadic `del`, recording call counts; tests assert both behavior and that the batched method was invoked once. +- **Predis pipeline/MGET mock fidelity (F4):** the existing `MockRedisClient` only stubs `get/set/setex/del/exists/keys/ttl/incr/expire` — it has NO `mget` and NO `pipeline` (but `del` is ALREADY variadic and flattens arrays). Mitigation: extend the mock to implement `mget` and `pipeline` (collecting queued commands), recording call counts; tests assert both behavior and that the batched method was invoked once. Do not re-add a variadic `del`. +- **HMAC signer preservation (F4) [CRITICAL]:** Tier 1 wraps stored values in a `CacheValueSigner` envelope. If the batch paths use raw serialize/unserialize, the existing `setMultiple`+`get` round-trip tests fail with `TamperedCacheValueException` and tamper protection is lost. Mitigation: `setMultiple` wraps each value via `cacheValueSigner->wrap(serialize(...))`; `getMultiple` reads each via `unserialize(cacheValueSigner->verifyAndUnwrap(...))`; `@throws TamperedCacheValueException` on both. `increment()` stays unsigned and untouched. +- **`driverName()` on hand-written ConnectionInterface stubs (F1/F3/F5):** Tier 2 added `ConnectionInterface::driverName(): string`. Any new inline anonymous stub that omits it is abstract-incomplete and fatals. Mitigation: each task that writes a fresh stub must implement `driverName()` (returning e.g. `'sqlite'`); PHPUnit `createMock` auto-stubs it, so only hand-written stubs are affected. - **Ordering guarantees (F2):** `findMany` returns matched media in unspecified order. Mitigation: `AttachmentManager` reorders results in PHP by the attachment id list and skips misses, and the test asserts the returned order. - **Interface-change suite breakage (F2/F3):** adding `findMany`/`getPermissionsForRoles` to an interface fatals every implementer that lacks it. Mitigation: each task enumerates the in-repo implementers (media test mocks; `createMockRoleRepo`) and updates them in the same TDD cycle. - **ChannelInterface is shared (F5):** adding a batch method to `ChannelInterface` breaks `MailChannel`/`SmsChannel`/third-party channels. Mitigation: introduce a separate opt-in `BatchChannelInterface`; sender uses `instanceof` to route. - **Per-recipient channel resolution (F5):** recipients can declare different channels. Mitigation: sender groups by per-recipient-resolved channel name (not a single shared set); test covers heterogeneous channels. - **Empty `IN ()` (F3):** an empty role-id list must short-circuit, never building `WHERE rp.role_id IN ()`. Mitigation: explicit guard + test. -- **Cross-plan rebase (F4 vs Tier 1 increment, F1 vs database plan):** Mitigation: tasks declare the file-cluster owners in Context; implementer rebases on latest before starting and re-runs the owning package's full suite. F5 has NO dependency on Tier 2 F9 (UUID PKs). +- **Cross-plan rebase (Tier 1+2+3 already merged):** F4 must preserve Tier 1's `CacheValueSigner` (above) and not touch Tier 1's `increment()`. F1 touches `findOneBy`/`exists`/`existsBy`/`isColumnUnique` in the SAME `Repository.php` Tier 2 modified for `insertBatch` RETURNING (lines 265-399) — F1's methods are at 232-236 and 541-582, disjoint from insertBatch, so they rebase cleanly. New stubs need Tier 2's `driverName()` (above). Mitigation: tasks declare the file-cluster owners in Context; implementer rebases on latest before starting and re-runs the owning package's full suite. F5 has NO dependency on Tier 2 F9 (UUID PKs). - **Transaction support (F5/Task 005):** `insertBatch` opens a transaction only when the connection implements `TransactionInterface`. Mitigation: Task 005's batch path follows the same conditional-transaction guard so non-transactional drivers still work. F5's `DatabaseChannel::sendMany` is a single multi-row INSERT and does not require its own transaction. diff --git a/packages/admin-auth/src/AdminUserProvider.php b/packages/admin-auth/src/AdminUserProvider.php index 2f02929f..670114f9 100644 --- a/packages/admin-auth/src/AdminUserProvider.php +++ b/packages/admin-auth/src/AdminUserProvider.php @@ -10,6 +10,7 @@ use Marko\Authentication\AuthenticatableInterface; use Marko\Authentication\Contracts\PasswordHasherInterface; use Marko\Authentication\Contracts\UserProviderInterface; +use Marko\Database\Exceptions\EntityException; readonly class AdminUserProvider implements UserProviderInterface { @@ -19,6 +20,9 @@ public function __construct( private PasswordHasherInterface $passwordHasher, ) {} + /** + * @throws EntityException + */ public function retrieveById( int|string $identifier, ): ?AuthenticatableInterface { @@ -37,6 +41,9 @@ public function retrieveById( return $user; } + /** + * @throws EntityException + */ public function retrieveByCredentials( array $credentials, ): ?AuthenticatableInterface { @@ -70,6 +77,9 @@ public function validateCredentials( return $this->passwordHasher->verify($password, $user->getAuthPassword()); } + /** + * @throws EntityException + */ public function retrieveByRememberToken( int|string $identifier, string $token, @@ -106,28 +116,28 @@ public function updateRememberToken( $this->userRepository->save($user); } + /** + * @throws EntityException + */ private function loadRolesAndPermissions( AdminUser $user, ): void { $roles = $this->userRepository->getRolesForUser($user->id); - $permissionKeys = []; - - foreach ($roles as $role) { - if ($role->id === null) { - continue; - } + $roleIds = array_filter( + array_map(fn (mixed $role): ?int => $role->id, $roles), + fn (?int $id): bool => $id !== null, + ); - $permissions = $this->roleRepository->getPermissionsForRole($role->id); + $permissions = $this->roleRepository->getPermissionsForRoles(array_values($roleIds)); - foreach ($permissions as $permission) { - $permissionKeys[] = $permission->key; - } - } + $permissionKeys = array_unique( + array_map(fn (mixed $permission): string => $permission->key, $permissions), + ); $user->setRoles( roles: $roles, - permissionKeys: array_unique($permissionKeys), + permissionKeys: $permissionKeys, ); } } diff --git a/packages/admin-auth/src/Repository/RoleRepository.php b/packages/admin-auth/src/Repository/RoleRepository.php index 71810f57..121a4a16 100644 --- a/packages/admin-auth/src/Repository/RoleRepository.php +++ b/packages/admin-auth/src/Repository/RoleRepository.php @@ -9,10 +9,12 @@ use Marko\AdminAuth\Events\RoleCreated; use Marko\AdminAuth\Events\RoleDeleted; use Marko\AdminAuth\Events\RoleUpdated; +use Marko\Database\Connection\TransactionInterface; use Marko\Database\Entity\Entity; use Marko\Database\Exceptions\EntityException; use Marko\Database\Exceptions\RepositoryException; use Marko\Database\Repository\Repository; +use Throwable; /** * @extends Repository @@ -21,6 +23,8 @@ class RoleRepository extends Repository implements RoleRepositoryInterface { protected const string ENTITY_CLASS = Role::class; + private const int SYNC_ROWS_PER_CHUNK = 500; + /** * Save a role, dispatching appropriate events. * @@ -118,23 +122,90 @@ public function getPermissionsForRole( ); } + /** + * Get the deduplicated permission set across all given role ids in a single query. + * + * @param array $roleIds + * @return array + * @throws EntityException + */ + public function getPermissionsForRoles( + array $roleIds, + ): array { + if ($roleIds === []) { + return []; + } + + $placeholders = implode(', ', array_fill(0, count($roleIds), '?')); + $sql = "SELECT DISTINCT p.* FROM permissions p + INNER JOIN role_permissions rp ON p.id = rp.permission_id + WHERE rp.role_id IN ($placeholders)"; + + $rows = $this->connection->query($sql, $roleIds); + + $permissionMetadata = $this->metadataFactory->parse(Permission::class); + + return array_map( + fn (array $row): Permission => $this->hydrator->hydrate( + Permission::class, + $row, + $permissionMetadata, + ), + $rows, + ); + } + /** * Sync permissions for a role, replacing all existing. * + * Wraps the DELETE and batched INSERT in a transaction when the connection + * supports it, so a mid-sync failure cannot leave a role half-synced. + * * @param array $permissionIds + * @throws Throwable */ public function syncPermissions( int $roleId, array $permissionIds, ): void { - // Remove all existing permissions for this role - $sql = 'DELETE FROM role_permissions WHERE role_id = ?'; - $this->connection->execute($sql, [$roleId]); - - // Attach the new permissions - foreach ($permissionIds as $permissionId) { - $sql = 'INSERT INTO role_permissions (role_id, permission_id) VALUES (?, ?)'; - $this->connection->execute($sql, [$roleId, $permissionId]); + $ownsTransaction = $this->connection instanceof TransactionInterface + && !$this->connection->inTransaction(); + + if ($ownsTransaction) { + $this->connection->beginTransaction(); + } + + try { + $this->connection->execute( + 'DELETE FROM role_permissions WHERE role_id = ?', + [$roleId], + ); + + foreach (array_chunk($permissionIds, self::SYNC_ROWS_PER_CHUNK) as $chunk) { + $placeholders = implode( + ', ', + array_fill(0, count($chunk), '(?, ?)'), + ); + $bindings = []; + foreach ($chunk as $permissionId) { + $bindings[] = $roleId; + $bindings[] = $permissionId; + } + $this->connection->execute( + "INSERT INTO role_permissions (role_id, permission_id) VALUES $placeholders", + $bindings, + ); + } + + if ($ownsTransaction) { + $this->connection->commit(); + } + } catch (Throwable $e) { + if ($ownsTransaction) { + $this->connection->rollback(); + } + + throw $e; } } diff --git a/packages/admin-auth/src/Repository/RoleRepositoryInterface.php b/packages/admin-auth/src/Repository/RoleRepositoryInterface.php index 18da40ed..25f39c5a 100644 --- a/packages/admin-auth/src/Repository/RoleRepositoryInterface.php +++ b/packages/admin-auth/src/Repository/RoleRepositoryInterface.php @@ -6,7 +6,9 @@ use Marko\AdminAuth\Entity\Permission; use Marko\AdminAuth\Entity\Role; +use Marko\Database\Exceptions\EntityException; use Marko\Database\Repository\RepositoryInterface; +use Throwable; /** * Interface for Role entity repository. @@ -24,15 +26,29 @@ public function findBySlug( * Get all permissions for a role. * * @return array + * @throws EntityException */ public function getPermissionsForRole( int $roleId, ): array; + /** + * Get the deduplicated permission set across all given role ids in a single query. + * Returns an empty array without querying when $roleIds is empty. + * + * @param array $roleIds + * @return array + * @throws EntityException + */ + public function getPermissionsForRoles( + array $roleIds, + ): array; + /** * Sync permissions for a role, replacing all existing. * * @param array $permissionIds + * @throws Throwable */ public function syncPermissions( int $roleId, diff --git a/packages/admin-auth/tests/Unit/AdminUserProviderTest.php b/packages/admin-auth/tests/Unit/AdminUserProviderTest.php index 3479ec9d..a5d5549d 100644 --- a/packages/admin-auth/tests/Unit/AdminUserProviderTest.php +++ b/packages/admin-auth/tests/Unit/AdminUserProviderTest.php @@ -17,6 +17,107 @@ use ReflectionClass; use RuntimeException; +/** + * Tracking role repo: records which batch methods were called. + * getPermissionsForRole throws to confirm it is never called after rewiring. + */ +class TrackingRoleRepo implements RoleRepositoryInterface +{ + /** @var array */ + public array $batchRoleIdsReceived = []; + + public int $batchCallCount = 0; + + /** + * @param array> $permissionsMap + */ + public function __construct( + private readonly array $permissionsMap = [], + ) {} + + public function find(int|string $id): ?Role + { + return null; + } + + public function findOrFail(int|string $id): Role + { + throw new RuntimeException('Not found'); + } + + public function findAll(): EntityCollection + { + return new EntityCollection(); + } + + public function findBy(array $criteria): EntityCollection + { + return new EntityCollection(); + } + + public function findOneBy(array $criteria): ?Role + { + return null; + } + + public function existsBy(array $criteria): bool + { + return false; + } + + public function findBySlug(string $slug): ?Role + { + return null; + } + + public function getPermissionsForRole(int $roleId): array + { + throw new RuntimeException('getPermissionsForRole must not be called after rewiring to batch'); + } + + public function getPermissionsForRoles(array $roleIds): array + { + $this->batchCallCount++; + $this->batchRoleIdsReceived = $roleIds; + + if ($roleIds === []) { + return []; + } + + $permissions = []; + $seen = []; + + foreach ($roleIds as $rid) { + foreach ($this->permissionsMap[$rid] ?? [] as $permission) { + if (!in_array($permission->key, $seen, true)) { + $seen[] = $permission->key; + $permissions[] = $permission; + } + } + } + + return $permissions; + } + + public function syncPermissions( + int $roleId, + array $permissionIds, + ): void {} + + public function isSlugUnique( + string $slug, + ?int $excludeId = null, + ): bool { + return true; + } + + public function save(Entity $entity): void {} + + public function delete(Entity $entity): void {} + + public function insertBatch(array $entities): void {} +} + it('implements UserProviderInterface', function (): void { $reflection = new ReflectionClass(AdminUserProvider::class); @@ -207,6 +308,123 @@ ->and($userRepo->lastSavedUser)->toBe($user); }); +it('loads the same permission set as the per-role implementation', function (): void { + $user = createTestAdminUser(); + + $editorRole = new Role(); + $editorRole->id = 1; + $editorRole->name = 'Editor'; + $editorRole->slug = 'editor'; + $editorRole->isSuperAdmin = '0'; + + $permission1 = new Permission(); + $permission1->id = 1; + $permission1->key = 'posts.create'; + $permission1->label = 'Create Posts'; + $permission1->group = 'posts'; + + $permission2 = new Permission(); + $permission2->id = 2; + $permission2->key = 'posts.edit'; + $permission2->label = 'Edit Posts'; + $permission2->group = 'posts'; + + $roleRepo = new TrackingRoleRepo(permissionsMap: [ + 1 => [$permission1, $permission2], + ]); + $userRepo = createMockUserRepo(findReturn: $user, rolesReturn: [$editorRole]); + $hasher = createMockHasher(); + + $provider = new AdminUserProvider($userRepo, $roleRepo, $hasher); + + $result = $provider->retrieveById(1); + + expect($result)->toBeInstanceOf(AdminUser::class) + ->and($result->hasPermission('posts.create'))->toBeTrue() + ->and($result->hasPermission('posts.edit'))->toBeTrue() + ->and($roleRepo->batchCallCount)->toBe(1); +}); + +it('deduplicates permission keys shared across roles', function (): void { + $user = createTestAdminUser(); + + $editorRole = new Role(); + $editorRole->id = 1; + $editorRole->name = 'Editor'; + $editorRole->slug = 'editor'; + $editorRole->isSuperAdmin = '0'; + + $moderatorRole = new Role(); + $moderatorRole->id = 2; + $moderatorRole->name = 'Moderator'; + $moderatorRole->slug = 'moderator'; + $moderatorRole->isSuperAdmin = '0'; + + $sharedPermission = new Permission(); + $sharedPermission->id = 1; + $sharedPermission->key = 'posts.create'; + $sharedPermission->label = 'Create Posts'; + $sharedPermission->group = 'posts'; + + $uniquePermission = new Permission(); + $uniquePermission->id = 2; + $uniquePermission->key = 'comments.moderate'; + $uniquePermission->label = 'Moderate Comments'; + $uniquePermission->group = 'comments'; + + $roleRepo = new TrackingRoleRepo(permissionsMap: [ + 1 => [$sharedPermission], + 2 => [$sharedPermission, $uniquePermission], + ]); + $userRepo = createMockUserRepo(findReturn: $user, rolesReturn: [$editorRole, $moderatorRole]); + $hasher = createMockHasher(); + + $provider = new AdminUserProvider($userRepo, $roleRepo, $hasher); + + $result = $provider->retrieveById(1); + + $roles = $result->getRoles(); + + expect($result)->toBeInstanceOf(AdminUser::class) + ->and($result->hasPermission('posts.create'))->toBeTrue() + ->and($result->hasPermission('comments.moderate'))->toBeTrue() + ->and($roles)->toHaveCount(2) + ->and($roleRepo->batchCallCount)->toBe(1); +}); + +it('sets roles and unique permission keys on the authenticated user', function (): void { + $user = createTestAdminUser(); + + $editorRole = new Role(); + $editorRole->id = 1; + $editorRole->name = 'Editor'; + $editorRole->slug = 'editor'; + $editorRole->isSuperAdmin = '0'; + + $permission = new Permission(); + $permission->id = 1; + $permission->key = 'posts.create'; + $permission->label = 'Create Posts'; + $permission->group = 'posts'; + + $roleRepo = new TrackingRoleRepo(permissionsMap: [ + 1 => [$permission], + ]); + $userRepo = createMockUserRepo(findReturn: $user, rolesReturn: [$editorRole]); + $hasher = createMockHasher(); + + $provider = new AdminUserProvider($userRepo, $roleRepo, $hasher); + + $result = $provider->retrieveById(1); + + expect($result)->toBeInstanceOf(AdminUser::class) + ->and($result->getRoles())->toHaveCount(1) + ->and($result->getRoles()[0]->getSlug())->toBe('editor') + ->and($result->hasPermission('posts.create'))->toBeTrue() + ->and($roleRepo->batchCallCount)->toBe(1) + ->and($roleRepo->batchRoleIdsReceived)->toBe([1]); +}); + // Helper functions function createTestAdminUser( @@ -371,6 +589,28 @@ public function getPermissionsForRole( return $this->permissionsMap[$roleId] ?? []; } + public function getPermissionsForRoles( + array $roleIds, + ): array { + if ($roleIds === []) { + return []; + } + + $permissions = []; + $seen = []; + + foreach ($roleIds as $roleId) { + foreach ($this->permissionsMap[$roleId] ?? [] as $permission) { + if (!in_array($permission->key, $seen, true)) { + $seen[] = $permission->key; + $permissions[] = $permission; + } + } + } + + return $permissions; + } + public function syncPermissions( int $roleId, array $permissionIds, diff --git a/packages/admin-auth/tests/Unit/Repository/RoleRepositoryInterfaceTest.php b/packages/admin-auth/tests/Unit/Repository/RoleRepositoryInterfaceTest.php index add3989e..e561a9cf 100644 --- a/packages/admin-auth/tests/Unit/Repository/RoleRepositoryInterfaceTest.php +++ b/packages/admin-auth/tests/Unit/Repository/RoleRepositoryInterfaceTest.php @@ -9,7 +9,7 @@ use Marko\Database\Repository\RepositoryInterface; use ReflectionClass; -it('creates RoleRepositoryInterface with find, findBySlug, all, save, delete methods', function (): void { +it('defines RoleRepositoryInterface extending RepositoryInterface with role methods', function (): void { $reflection = new ReflectionClass(RoleRepositoryInterface::class); expect($reflection->isInterface())->toBeTrue() @@ -19,6 +19,7 @@ $expectedMethods = [ 'findBySlug', 'getPermissionsForRole', + 'getPermissionsForRoles', 'syncPermissions', 'isSlugUnique', ]; @@ -98,3 +99,18 @@ $returnType = $method->getReturnType(); expect($returnType->getName())->toBe('bool'); }); + +it('defines getPermissionsForRoles on RoleRepositoryInterface', function (): void { + $reflection = new ReflectionClass(RoleRepositoryInterface::class); + + expect($reflection->hasMethod('getPermissionsForRoles'))->toBeTrue(); + + $method = $reflection->getMethod('getPermissionsForRoles'); + $parameters = $method->getParameters(); + + expect($method->isPublic())->toBeTrue() + ->and($parameters)->toHaveCount(1) + ->and($parameters[0]->getName())->toBe('roleIds') + ->and($parameters[0]->getType()->getName())->toBe('array') + ->and($method->getReturnType()->getName())->toBe('array'); +}); diff --git a/packages/admin-auth/tests/Unit/Repository/RoleRepositoryTest.php b/packages/admin-auth/tests/Unit/Repository/RoleRepositoryTest.php index 74b16f7d..321b6d94 100644 --- a/packages/admin-auth/tests/Unit/Repository/RoleRepositoryTest.php +++ b/packages/admin-auth/tests/Unit/Repository/RoleRepositoryTest.php @@ -10,6 +10,7 @@ use Marko\AdminAuth\Repository\RoleRepositoryInterface; use Marko\Database\Connection\ConnectionInterface; use Marko\Database\Connection\StatementInterface; +use Marko\Database\Connection\TransactionInterface; use Marko\Database\Entity\EntityHydrator; use Marko\Database\Entity\EntityMetadataFactory; use Marko\Database\Repository\Repository; @@ -151,7 +152,7 @@ ->and($queryHistory[0]['bindings'])->toBe([1]); }); -it('syncs permissions for a role via syncPermissions', function (): void { +it('replaces a role\'s permissions with the new set', function (): void { $queryHistory = []; $connection = createRoleMockConnectionWithHistory( [], @@ -164,20 +165,304 @@ $repository->syncPermissions(1, [10, 20, 30]); - // First query should be "DELETE" of existing permissions expect($queryHistory[0]['sql'])->toContain('DELETE FROM role_permissions') ->and($queryHistory[0]['sql'])->toContain('role_id = ?') - ->and($queryHistory[0]['bindings'])->toBe([1]) - ->and($queryHistory[1]['sql'])->toContain('INSERT INTO role_permissions') - ->and($queryHistory[1]['bindings'])->toBe([1, 10]) - ->and($queryHistory[2]['bindings'])->toBe([1, 20]) - ->and($queryHistory[3]['bindings'])->toBe([1, 30]); + ->and($queryHistory[0]['bindings'])->toBe([1]); +}); + +it('clears all permissions when given an empty permission id list', function (): void { + $queryHistory = []; + $connection = createRoleMockConnectionWithHistory( + [], + $queryHistory, + ); + $metadataFactory = new EntityMetadataFactory(); + $hydrator = new EntityHydrator(); + + $repository = new RoleRepository($connection, $metadataFactory, $hydrator); + + $repository->syncPermissions(1, []); + + expect($queryHistory)->toHaveCount(1) + ->and($queryHistory[0]['sql'])->toContain('DELETE FROM role_permissions') + ->and($queryHistory[0]['bindings'])->toBe([1]); +}); + +it('inserts all new permissions in a single multi-row insert', function (): void { + $queryHistory = []; + $connection = createRoleMockConnectionWithHistory( + [], + $queryHistory, + ); + $metadataFactory = new EntityMetadataFactory(); + $hydrator = new EntityHydrator(); + + $repository = new RoleRepository($connection, $metadataFactory, $hydrator); + + $repository->syncPermissions(1, [10, 20, 30]); + + $insertEntries = array_values(array_filter( + $queryHistory, + fn (array $e): bool => str_contains($e['sql'], 'INSERT'), + )); + + expect($insertEntries)->toHaveCount(1) + ->and($insertEntries[0]['sql'])->toContain('INSERT INTO role_permissions') + ->and($insertEntries[0]['bindings'])->toBe([1, 10, 1, 20, 1, 30]); +}); + +it('wraps the delete and insert in one transaction when the connection supports transactions', function (): void { + $txLog = []; + $connection = createRoleTransactionalConnectionWithHistory([], $txLog); + $metadataFactory = new EntityMetadataFactory(); + $hydrator = new EntityHydrator(); + + $repository = new RoleRepository($connection, $metadataFactory, $hydrator); + + $repository->syncPermissions(1, [10, 20]); + + $ops = array_column($txLog, 'op'); + + expect($ops[0])->toBe('beginTransaction') + ->and(in_array('execute', $ops, true))->toBeTrue() + ->and($ops[count($ops) - 1])->toBe('commit'); +}); + +it('rolls back and leaves permissions unchanged when an insert fails mid-sync', function (): void { + $txLog = []; + $connection = createRoleTransactionalConnectionWithHistory([], $txLog, failOnInsert: true); + $metadataFactory = new EntityMetadataFactory(); + $hydrator = new EntityHydrator(); - // Next 3 queries should be INSERTs + $repository = new RoleRepository($connection, $metadataFactory, $hydrator); + + expect(fn () => $repository->syncPermissions(1, [10, 20])) + ->toThrow(RuntimeException::class); + + $ops = array_column($txLog, 'op'); + + expect(in_array('beginTransaction', $ops, true))->toBeTrue() + ->and(in_array('rollback', $ops, true))->toBeTrue() + ->and(in_array('commit', $ops, true))->toBeFalse(); +}); + +it('still syncs when the connection does not support transactions', function (): void { + $queryHistory = []; + $connection = createRoleMockConnectionWithHistory([], $queryHistory); + $metadataFactory = new EntityMetadataFactory(); + $hydrator = new EntityHydrator(); + + $repository = new RoleRepository($connection, $metadataFactory, $hydrator); + + $repository->syncPermissions(1, [10, 20]); + + $insertEntries = array_values(array_filter( + $queryHistory, + fn (array $e): bool => str_contains($e['sql'], 'INSERT'), + )); + + expect($insertEntries)->toHaveCount(1) + ->and($insertEntries[0]['bindings'])->toBe([1, 10, 1, 20]); +}); + +it('returns an empty array from getPermissionsForRoles without querying when given no role ids', function (): void { + $queryHistory = []; + $connection = createRoleMockConnectionWithHistory([], $queryHistory); + $metadataFactory = new EntityMetadataFactory(); + $hydrator = new EntityHydrator(); + + $repository = new RoleRepository($connection, $metadataFactory, $hydrator); + + $permissions = $repository->getPermissionsForRoles([]); + + expect($permissions)->toBeEmpty() + ->and($queryHistory)->toBeEmpty(); +}); + +it( + 'queries permissions with a WHERE role_id IN clause whose placeholder count matches the role ids', + function (): void { + $queryHistory = []; + $connection = createRoleMockConnectionWithHistory( + [ + [ + 'id' => 1, + 'key' => 'posts.create', + 'label' => 'Create Posts', + 'group' => 'posts', + 'created_at' => '2024-01-01 00:00:00', + ], + [ + 'id' => 2, + 'key' => 'posts.edit', + 'label' => 'Edit Posts', + 'group' => 'posts', + 'created_at' => '2024-01-01 00:00:00', + ], + ], + $queryHistory, + ); + $metadataFactory = new EntityMetadataFactory(); + $hydrator = new EntityHydrator(); + + $repository = new RoleRepository($connection, $metadataFactory, $hydrator); + + $permissions = $repository->getPermissionsForRoles([1, 2, 3]); + + expect($permissions)->toHaveCount(2) + ->and($queryHistory)->toHaveCount(1) + ->and($queryHistory[0]['sql'])->toContain('IN (') + ->and($queryHistory[0]['sql'])->toContain('role_permissions') + ->and($queryHistory[0]['bindings'])->toBe([1, 2, 3]); + }, +); + +it('issues exactly one permissions query for a user with multiple roles', function (): void { + $queryHistory = []; + $connection = createRoleMockConnectionWithHistory( + [ + [ + 'id' => 1, + 'key' => 'posts.create', + 'label' => 'Create Posts', + 'group' => 'posts', + 'created_at' => '2024-01-01 00:00:00', + ], + ], + $queryHistory, + ); + $metadataFactory = new EntityMetadataFactory(); + $hydrator = new EntityHydrator(); + + $repository = new RoleRepository($connection, $metadataFactory, $hydrator); + + $repository->getPermissionsForRoles([1, 2, 3]); + + $permissionQueries = array_filter( + $queryHistory, + fn (array $entry): bool => str_contains($entry['sql'], 'role_permissions') && str_contains( + $entry['sql'], + 'IN (', + ), + ); + + expect($permissionQueries)->toHaveCount(1); +}); + +it('issues no permissions query when the user has no roles', function (): void { + $queryHistory = []; + $connection = createRoleMockConnectionWithHistory([], $queryHistory); + $metadataFactory = new EntityMetadataFactory(); + $hydrator = new EntityHydrator(); + + $repository = new RoleRepository($connection, $metadataFactory, $hydrator); + + $permissions = $repository->getPermissionsForRoles([]); + + expect($permissions)->toBeEmpty() + ->and($queryHistory)->toBeEmpty(); }); // Helper functions +/** + * @param array> $queryResult + * @param array $txLog + */ +function createRoleTransactionalConnectionWithHistory( + array $queryResult = [], + ?array &$txLog = null, + bool $failOnInsert = false, +): ConnectionInterface&TransactionInterface { + $txLog ??= []; + + return new class ($queryResult, $txLog, $failOnInsert) implements ConnectionInterface, TransactionInterface + { + /** + * @param array> $queryResult + * @param array $txLog + */ + public function __construct( + private readonly array $queryResult, + /** @noinspection PhpPropertyOnlyWrittenInspection - Reference property modifies external variable */ + private array &$txLog, + private readonly bool $failOnInsert, + ) {} + + public function connect(): void {} + + public function disconnect(): void {} + + public function isConnected(): bool + { + return true; + } + + public function query( + string $sql, + array $bindings = [], + ): array { + $this->txLog[] = ['op' => 'execute', 'sql' => $sql, 'bindings' => $bindings]; + + return $this->queryResult; + } + + public function execute( + string $sql, + array $bindings = [], + ): int { + if ($this->failOnInsert && str_contains($sql, 'INSERT')) { + throw new RuntimeException('Simulated insert failure'); + } + + $this->txLog[] = ['op' => 'execute', 'sql' => $sql, 'bindings' => $bindings]; + + return 1; + } + + public function prepare(string $sql): StatementInterface + { + throw new RuntimeException('Not implemented'); + } + + public function lastInsertId(): int + { + return 1; + } + + public function driverName(): string + { + return 'sqlite'; + } + + public function beginTransaction(): void + { + $this->txLog[] = ['op' => 'beginTransaction']; + } + + public function commit(): void + { + $this->txLog[] = ['op' => 'commit']; + } + + public function rollback(): void + { + $this->txLog[] = ['op' => 'rollback']; + } + + public function inTransaction(): bool + { + return array_any($this->txLog, fn (array $e): bool => $e['op'] === 'beginTransaction') + && !array_any($this->txLog, fn (array $e): bool => $e['op'] === 'commit' || $e['op'] === 'rollback'); + } + + public function transaction(callable $callback): null + { + return null; + } + }; +} + function createRoleMockConnection( array $queryResult = [], ): ConnectionInterface { @@ -202,6 +487,7 @@ function createRoleMockConnectionWithHistory( */ public function __construct( private readonly array $queryResult, + /** @noinspection PhpPropertyOnlyWrittenInspection - Reference property modifies external variable */ private array &$queryHistory, ) {} diff --git a/packages/cache-redis/src/Driver/RedisCacheDriver.php b/packages/cache-redis/src/Driver/RedisCacheDriver.php index 99ca6c4f..53afb003 100644 --- a/packages/cache-redis/src/Driver/RedisCacheDriver.php +++ b/packages/cache-redis/src/Driver/RedisCacheDriver.php @@ -130,10 +130,23 @@ public function getMultiple( array $keys, mixed $default = null, ): iterable { + foreach ($keys as $key) { + $this->validateKey($key); + } + + $prefixedKeys = array_map(fn ($k) => $this->prefixKey($k), $keys); + $raw = $this->connection->client()->mget(...$prefixedKeys); + $result = []; - foreach ($keys as $key) { - $result[$key] = $this->get($key, $default); + foreach ($keys as $i => $key) { + $value = $raw[$i] ?? null; + + if ($value === null) { + $result[$key] = $default; + } else { + $result[$key] = unserialize($this->cacheValueSigner->verifyAndUnwrap($value)); + } } return $result; @@ -147,9 +160,25 @@ public function setMultiple( ?int $ttl = null, ): bool { foreach ($values as $key => $value) { - $this->set($key, $value, $ttl); + $this->validateKey($key); } + $ttl ??= $this->config->defaultTtl(); + $client = $this->connection->client(); + + $client->pipeline(function ($pipe) use ($values, $ttl): void { + foreach ($values as $key => $value) { + $prefixedKey = $this->prefixKey($key); + $envelope = $this->cacheValueSigner->wrap(serialize($value)); + + if ($ttl > 0) { + $pipe->setex($prefixedKey, $ttl, $envelope); + } else { + $pipe->set($prefixedKey, $envelope); + } + } + }); + return true; } @@ -160,9 +189,13 @@ public function deleteMultiple( array $keys, ): bool { foreach ($keys as $key) { - $this->delete($key); + $this->validateKey($key); } + $prefixedKeys = array_map(fn ($k) => $this->prefixKey($k), $keys); + + $this->connection->client()->del(...$prefixedKeys); + return true; } diff --git a/packages/cache-redis/tests/Unit/RedisCacheDriverTest.php b/packages/cache-redis/tests/Unit/RedisCacheDriverTest.php index d2ba4abc..78ceac8c 100644 --- a/packages/cache-redis/tests/Unit/RedisCacheDriverTest.php +++ b/packages/cache-redis/tests/Unit/RedisCacheDriverTest.php @@ -15,6 +15,29 @@ use Predis\Client; use Predis\ClientInterface; +readonly class MockPipelineRecorder +{ + public function __construct( + private MockRedisClient $client, + ) {} + + public function setex( + string $key, + int $seconds, + string $value, + ): void { + $this->client->storage[$key] = $value; + $this->client->ttls[$key] = $seconds; + } + + public function set( + string $key, + string $value, + ): void { + $this->client->storage[$key] = $value; + } +} + /** @noinspection PhpMissingParentConstructorInspection - Test stub intentionally skips parent */ class MockRedisClient extends Client { @@ -24,6 +47,12 @@ class MockRedisClient extends Client /** @var array */ public array $ttls = []; + public int $mgetCount = 0; + + public int $pipelineCount = 0; + + public int $delCount = 0; + /** @noinspection PhpMissingParentConstructorInspection */ public function __construct() {} @@ -71,6 +100,7 @@ public function exists( public function del( ...$keys, ): int { + $this->delCount++; $count = 0; $flatKeys = []; @@ -92,6 +122,38 @@ public function del( return $count; } + public function mget( + ...$keys, + ): array { + $this->mgetCount++; + $flatKeys = []; + + foreach ($keys as $key) { + if (is_array($key)) { + $flatKeys = array_merge($flatKeys, $key); + } else { + $flatKeys[] = $key; + } + } + + return array_map(fn ($k) => $this->storage[$k] ?? null, $flatKeys); + } + + public function pipeline( + ...$arguments, + ): mixed { + $this->pipelineCount++; + + $callback = $arguments[0] ?? null; + + if (is_callable($callback)) { + $recorder = new MockPipelineRecorder($this); + $callback($recorder); + } + + return []; + } + public function keys( $pattern, ): array { @@ -507,4 +569,167 @@ protected function createClient(): ClientInterface expect($driver->get('complex'))->toBe($value); }); + + it('returns values for all requested keys via getMultiple', function (): void { + $mockClient = createMockClient(); + $driver = createDriver($mockClient); + + $driver->set('key1', 'value1'); + $driver->set('key2', 'value2'); + + $result = $driver->getMultiple(['key1', 'key2']); + + expect($result)->toBe(['key1' => 'value1', 'key2' => 'value2']); + }); + + it('returns the default for missing keys in getMultiple', function (): void { + $mockClient = createMockClient(); + $driver = createDriver($mockClient); + + $result = $driver->getMultiple(['missing1', 'missing2'], 'fallback'); + + expect($result)->toBe(['missing1' => 'fallback', 'missing2' => 'fallback']); + }); + + it('preserves input key order in the getMultiple result', function (): void { + $mockClient = createMockClient(); + $driver = createDriver($mockClient); + + $driver->set('z', 'z-val'); + $driver->set('a', 'a-val'); + + $result = $driver->getMultiple(['z', 'a']); + + expect(array_keys($result))->toBe(['z', 'a']); + }); + + it('issues a single MGET for getMultiple instead of one get per key', function (): void { + $mockClient = createMockClient(); + $driver = createDriver($mockClient); + + $driver->set('key1', 'v1'); + $driver->set('key2', 'v2'); + + $mockClient->mgetCount = 0; + + $driver->getMultiple(['key1', 'key2']); + + expect($mockClient->mgetCount)->toBe(1); + }); + + it('verifies the HMAC envelope on each value read by getMultiple', function (): void { + $mockClient = createMockClient(); + $driver = createDriver($mockClient); + + $mockClient->storage['marko:cache:tampered'] = str_repeat('a', 64) . '.' . serialize('bad'); + + expect(fn () => $driver->getMultiple(['tampered'])) + ->toThrow(TamperedCacheValueException::class); + }); + + it('stores all pairs via setMultiple with the given TTL', function (): void { + $mockClient = createMockClient(); + $driver = createDriver($mockClient); + + $driver->setMultiple(['key1' => 'v1', 'key2' => 'v2'], 120); + + expect($driver->get('key1'))->toBe('v1') + ->and($driver->get('key2'))->toBe('v2') + ->and($mockClient->ttls['marko:cache:key1'])->toBe(120) + ->and($mockClient->ttls['marko:cache:key2'])->toBe(120); + }); + + it('applies the default TTL in setMultiple when none is given', function (): void { + $mockClient = createMockClient(); + $driver = createDriver($mockClient, defaultTtl: 3600); + + $driver->setMultiple(['key1' => 'v1']); + + expect($mockClient->ttls['marko:cache:key1'])->toBe(3600); + }); + + it('stores persistent pairs without a TTL when setMultiple TTL is zero', function (): void { + $mockClient = createMockClient(); + $driver = createDriver($mockClient); + + $driver->setMultiple(['key1' => 'v1'], 0); + + expect(isset($mockClient->storage['marko:cache:key1']))->toBeTrue() + ->and(isset($mockClient->ttls['marko:cache:key1']))->toBeFalse(); + }); + + it('issues the setMultiple writes in a single pipeline', function (): void { + $mockClient = createMockClient(); + $driver = createDriver($mockClient); + + $mockClient->pipelineCount = 0; + + $driver->setMultiple(['key1' => 'v1', 'key2' => 'v2']); + + expect($mockClient->pipelineCount)->toBe(1); + }); + + it('stores each setMultiple value as a signed HMAC envelope', function (): void { + $mockClient = createMockClient(); + $driver = createDriver($mockClient); + + $driver->setMultiple(['key1' => 'v1']); + + $stored = $mockClient->storage['marko:cache:key1']; + + expect(strlen($stored))->toBeGreaterThan(65) + ->and(substr($stored, 64, 1))->toBe('.'); + }); + + it('round-trips values written by setMultiple back through getMultiple', function (): void { + $mockClient = createMockClient(); + $driver = createDriver($mockClient); + + $driver->setMultiple(['alpha' => 'hello', 'beta' => [1, 2, 3]]); + + $result = $driver->getMultiple(['alpha', 'beta']); + + expect($result)->toBe(['alpha' => 'hello', 'beta' => [1, 2, 3]]); + }); + + it('deletes all given keys via deleteMultiple', function (): void { + $mockClient = createMockClient(); + $driver = createDriver($mockClient); + + $driver->set('key1', 'v1'); + $driver->set('key2', 'v2'); + $driver->set('key3', 'v3'); + + $driver->deleteMultiple(['key1', 'key2']); + + expect($driver->has('key1'))->toBeFalse() + ->and($driver->has('key2'))->toBeFalse() + ->and($driver->has('key3'))->toBeTrue(); + }); + + it('issues a single variadic DEL for deleteMultiple', function (): void { + $mockClient = createMockClient(); + $driver = createDriver($mockClient); + + $driver->set('key1', 'v1'); + $driver->set('key2', 'v2'); + + $mockClient->delCount = 0; + + $driver->deleteMultiple(['key1', 'key2']); + + expect($mockClient->delCount)->toBe(1); + }); + + it('validates every key in the multi-key operations', function (): void { + $mockClient = createMockClient(); + $driver = createDriver($mockClient); + + expect(fn () => $driver->getMultiple(['valid', 'invalid/key'])) + ->toThrow(InvalidKeyException::class) + ->and(fn () => $driver->setMultiple(['valid' => 'v', 'invalid/key' => 'v'])) + ->toThrow(InvalidKeyException::class) + ->and(fn () => $driver->deleteMultiple(['valid', 'invalid/key'])) + ->toThrow(InvalidKeyException::class); + }); }); diff --git a/packages/database/src/Repository/Repository.php b/packages/database/src/Repository/Repository.php index 16535c42..d50daa82 100644 --- a/packages/database/src/Repository/Repository.php +++ b/packages/database/src/Repository/Repository.php @@ -227,12 +227,46 @@ public function findBy( /** * Find a single entity matching the given criteria. * + * Issues a LIMIT 1 query so the database returns at most one row. + * The returned entity is fully hydrated and eager-loaded, identical to findBy()->first() + * but without fetching all matching rows first. + * * @return TEntity|null */ public function findOneBy( array $criteria, ): ?Entity { - return $this->findBy($criteria)->first(); + $propertyToColumn = $this->metadata->getPropertyToColumnMap(); + $conditions = []; + $bindings = []; + + foreach ($criteria as $property => $value) { + $column = $propertyToColumn[$property] ?? $property; + $conditions[] = "$column = ?"; + $bindings[] = $value; + } + + $sql = sprintf( + 'SELECT * FROM %s WHERE %s LIMIT 1', + $this->metadata->tableName, + implode(' AND ', $conditions), + ); + + $rows = $this->connection->query($sql, $bindings); + + if (count($rows) === 0) { + return null; + } + + $entity = $this->hydrator->hydrate( + static::ENTITY_CLASS, + $rows[0], + $this->metadata, + ); + + $this->eagerLoadRelationships([$entity]); + + return $entity; } /** @@ -537,26 +571,63 @@ public function count(): int /** * Check if an entity with the given ID exists. + * + * Issues a `SELECT 1 FROM
WHERE = ? LIMIT 1` probe — + * no hydration, no eager-loading. */ public function exists( int|string $id, ): bool { - return $this->find(id: $id) !== null; + $columnName = $this->metadata->getPrimaryKeyProperty()->columnName; + + $sql = sprintf( + 'SELECT 1 FROM %s WHERE %s = ? LIMIT 1', + $this->metadata->tableName, + $columnName, + ); + + $rows = $this->connection->query($sql, [$id]); + + return count($rows) > 0; } /** * Check if any entity matches the given criteria. * + * Issues a `SELECT 1 FROM
WHERE ... LIMIT 1` probe — + * no hydration, no eager-loading. + * * @param array $criteria Column-value pairs to match */ public function existsBy( array $criteria, ): bool { - return $this->findOneBy(criteria: $criteria) !== null; + $propertyToColumn = $this->metadata->getPropertyToColumnMap(); + $conditions = []; + $bindings = []; + + foreach ($criteria as $property => $value) { + $column = $propertyToColumn[$property] ?? $property; + $conditions[] = "$column = ?"; + $bindings[] = $value; + } + + $sql = sprintf( + 'SELECT 1 FROM %s WHERE %s LIMIT 1', + $this->metadata->tableName, + implode(' AND ', $conditions), + ); + + $rows = $this->connection->query($sql, $bindings); + + return count($rows) > 0; } /** * Check if a column value is unique, optionally excluding an entity by ID. + * + * Issues a `SELECT 1 FROM
WHERE = ? [AND != ?] LIMIT 1` + * probe — no hydration, no eager-loading. */ protected function isColumnUnique( string $column, @@ -564,7 +635,7 @@ protected function isColumnUnique( int|string|null $excludeId = null, ): bool { $sql = sprintf( - 'SELECT * FROM %s WHERE %s = ?', + 'SELECT 1 FROM %s WHERE %s = ?', $this->metadata->tableName, $column, ); @@ -576,6 +647,8 @@ protected function isColumnUnique( $bindings[] = $excludeId; } + $sql .= ' LIMIT 1'; + $rows = $this->connection->query($sql, $bindings); return count($rows) === 0; diff --git a/packages/database/tests/Feature/RepositoryExistsTest.php b/packages/database/tests/Feature/RepositoryExistsTest.php new file mode 100644 index 00000000..0fa741d0 --- /dev/null +++ b/packages/database/tests/Feature/RepositoryExistsTest.php @@ -0,0 +1,203 @@ +isColumnUnique('email', $email, $excludeId); + } +} + +/** + * Build a stub ConnectionInterface that records all query() calls and returns + * the given rows when the SQL starts with SELECT 1. + * + * @param array> $rows Rows to return for SELECT 1 probes + * @param list}> $log Reference to capture queries + */ +function makeExistsConnection(array $rows, array &$log): ConnectionInterface +{ + return new class ($rows, $log) implements ConnectionInterface + { + public function __construct( + private readonly array $rows, + /** @noinspection PhpPropertyOnlyWrittenInspection - Reference property modifies external variable */ + private array &$log, + ) {} + + public function connect(): void {} + + public function disconnect(): void {} + + public function isConnected(): bool + { + return true; + } + + public function query( + string $sql, + array $bindings = [], + ): array { + $this->log[] = ['sql' => $sql, 'bindings' => $bindings]; + + return $this->rows; + } + + public function execute( + string $sql, + array $bindings = [], + ): int { + return 0; + } + + public function prepare(string $sql): StatementInterface + { + throw new RuntimeException('Not implemented'); + } + + public function lastInsertId(): int + { + return 0; + } + + public function driverName(): string + { + return 'sqlite'; + } + }; +} + +describe('Repository exists family', function (): void { + it('returns true from exists when a row with the id is present', function (): void { + $log = []; + $connection = makeExistsConnection([['1' => 1]], $log); + $repository = new ExistUserRepository($connection, new EntityMetadataFactory(), new EntityHydrator()); + + expect($repository->exists(1))->toBeTrue(); + }); + + it('returns false from exists when no row has the id', function (): void { + $log = []; + $connection = makeExistsConnection([], $log); + $repository = new ExistUserRepository($connection, new EntityMetadataFactory(), new EntityHydrator()); + + expect($repository->exists(999))->toBeFalse(); + }); + + it('probes exists with SELECT 1 and LIMIT 1 and does not hydrate an entity', function (): void { + $log = []; + $connection = makeExistsConnection([['1' => 1]], $log); + $repository = new ExistUserRepository($connection, new EntityMetadataFactory(), new EntityHydrator()); + + $repository->exists(42); + + expect($log)->toHaveCount(1) + ->and($log[0]['sql'])->toStartWith('SELECT 1 FROM') + ->and($log[0]['sql'])->toContain('LIMIT 1') + ->and($log[0]['sql'])->not->toContain('SELECT *'); + }); + + it('returns true from existsBy when criteria match a row', function (): void { + $log = []; + $connection = makeExistsConnection([['1' => 1]], $log); + $repository = new ExistUserRepository($connection, new EntityMetadataFactory(), new EntityHydrator()); + + expect($repository->existsBy(['email' => 'alice@example.com']))->toBeTrue(); + }); + + it('returns false from existsBy when criteria match nothing', function (): void { + $log = []; + $connection = makeExistsConnection([], $log); + $repository = new ExistUserRepository($connection, new EntityMetadataFactory(), new EntityHydrator()); + + expect($repository->existsBy(['email' => 'nobody@example.com']))->toBeFalse(); + }); + + it('probes existsBy with SELECT 1 and LIMIT 1', function (): void { + $log = []; + $connection = makeExistsConnection([], $log); + $repository = new ExistUserRepository($connection, new EntityMetadataFactory(), new EntityHydrator()); + + $repository->existsBy(['email' => 'alice@example.com']); + + expect($log)->toHaveCount(1) + ->and($log[0]['sql'])->toStartWith('SELECT 1 FROM') + ->and($log[0]['sql'])->toContain('LIMIT 1') + ->and($log[0]['sql'])->not->toContain('SELECT *'); + }); + + it('returns true from isColumnUnique when no other row holds the value', function (): void { + $log = []; + $connection = makeExistsConnection([], $log); + $repository = new ExistUserRepository($connection, new EntityMetadataFactory(), new EntityHydrator()); + + expect($repository->isEmailUnique('unique@example.com'))->toBeTrue(); + }); + + it('returns false from isColumnUnique when another row holds the value', function (): void { + $log = []; + $connection = makeExistsConnection([['1' => 1]], $log); + $repository = new ExistUserRepository($connection, new EntityMetadataFactory(), new EntityHydrator()); + + expect($repository->isEmailUnique('taken@example.com'))->toBeFalse(); + }); + + it('excludes the given id from the isColumnUnique uniqueness check', function (): void { + $log = []; + $connection = makeExistsConnection([], $log); + $repository = new ExistUserRepository($connection, new EntityMetadataFactory(), new EntityHydrator()); + + $repository->isEmailUnique('alice@example.com', 5); + + expect($log)->toHaveCount(1) + ->and($log[0]['sql'])->toContain('id != ?') + ->and($log[0]['bindings'])->toBe(['alice@example.com', 5]); + }); + + it('probes isColumnUnique with SELECT 1 and LIMIT 1 instead of SELECT star', function (): void { + $log = []; + $connection = makeExistsConnection([], $log); + $repository = new ExistUserRepository($connection, new EntityMetadataFactory(), new EntityHydrator()); + + $repository->isEmailUnique('alice@example.com'); + + expect($log)->toHaveCount(1) + ->and($log[0]['sql'])->toStartWith('SELECT 1 FROM') + ->and($log[0]['sql'])->toContain('LIMIT 1') + ->and($log[0]['sql'])->not->toContain('SELECT *'); + }); +}); diff --git a/packages/database/tests/Repository/FindOneByLimitOneTest.php b/packages/database/tests/Repository/FindOneByLimitOneTest.php new file mode 100644 index 00000000..904191ff --- /dev/null +++ b/packages/database/tests/Repository/FindOneByLimitOneTest.php @@ -0,0 +1,512 @@ +> $rows + */ +function makeFobConnection(array $rows = []): ConnectionInterface +{ + return new readonly class ($rows) implements ConnectionInterface + { + public function __construct( + private array $rows, + ) {} + + public function connect(): void {} + + public function disconnect(): void {} + + public function isConnected(): bool + { + return true; + } + + public function query( + string $sql, + array $bindings = [], + ): array { + return $this->rows; + } + + public function execute( + string $sql, + array $bindings = [], + ): int { + return 0; + } + + public function prepare(string $sql): StatementInterface + { + throw new RuntimeException('Not implemented'); + } + + public function lastInsertId(): int + { + return 0; + } + + public function driverName(): string + { + return 'sqlite'; + } + }; +} + +/** + * Build a query-recording connection that stores every query() call. + * + * @param array> $rows Rows to return for every query() call + * @param array}> $queries Reference to the external capture array + */ +function makeFobRecordingConnection(array $rows, array &$queries): ConnectionInterface +{ + return new class ($rows, $queries) implements ConnectionInterface + { + public function __construct( + private readonly array $rows, + /** @noinspection PhpPropertyOnlyWrittenInspection - Reference property modifies external variable */ + private array &$queries, + ) {} + + public function connect(): void {} + + public function disconnect(): void {} + + public function isConnected(): bool + { + return true; + } + + public function query( + string $sql, + array $bindings = [], + ): array { + $this->queries[] = ['sql' => $sql, 'bindings' => $bindings]; + + return $this->rows; + } + + public function execute( + string $sql, + array $bindings = [], + ): int { + return 0; + } + + public function prepare(string $sql): StatementInterface + { + throw new RuntimeException('Not implemented'); + } + + public function lastInsertId(): int + { + return 0; + } + + public function driverName(): string + { + return 'sqlite'; + } + }; +} + +function makeFobQbFactory(array $relatedRows = []): QueryBuilderFactoryInterface +{ + $stub = new readonly class ($relatedRows) implements QueryBuilderInterface + { + public function __construct(private array $rows) {} + + public function table(string $table): static + { + return $this; + } + + public function select(string ...$columns): static + { + return $this; + } + + public function where( + string $column, + string $operator, + mixed $value, + ): static { + return $this; + } + + public function whereIn( + string $column, + array $values, + ): static { + return $this; + } + + public function whereNull(string $column): static + { + return $this; + } + + public function whereNotNull(string $column): static + { + return $this; + } + + public function whereJsonContains( + string $path, + mixed $value, + ): static { + return $this; + } + + public function whereJsonExists(string $path): static + { + return $this; + } + + public function whereJsonMissing(string $path): static + { + return $this; + } + + public function orWhere( + string $column, + string $operator, + mixed $value, + ): static { + return $this; + } + + public function join( + string $table, + string $first, + string $operator, + string $second, + ): static { + return $this; + } + + public function leftJoin( + string $table, + string $first, + string $operator, + string $second, + ): static { + return $this; + } + + public function rightJoin( + string $table, + string $first, + string $operator, + string $second, + ): static { + return $this; + } + + public function orderBy( + string $column, + string $direction = 'ASC', + ): static { + return $this; + } + + public function orderByRaw( + string $expression, + string $direction = 'ASC', + ): static { + return $this; + } + + public function selectRaw( + string $expression, + array $bindings = [], + ): static { + return $this; + } + + public function whereRaw( + string $expression, + array $bindings = [], + ): static { + return $this; + } + + public function limit(int $limit): static + { + return $this; + } + + public function offset(int $offset): static + { + return $this; + } + + public function distinct(): static + { + return $this; + } + + public function union(QueryBuilderInterface $other): static + { + return $this; + } + + public function unionAll(QueryBuilderInterface $other): static + { + return $this; + } + + public function getColumnCount(): int + { + return 1; + } + + public function compileSubquery(array &$bindings): string + { + return ''; + } + + public function get(): array + { + return $this->rows; + } + + public function first(): ?array + { + return $this->rows[0] ?? null; + } + + public function insert( + array $data, + ?string $primaryKey = null, + ): int { + return 0; + } + + public function update(array $data): int + { + return 0; + } + + public function delete(): int + { + return 0; + } + + public function count(?string $column = null): int + { + return count($this->rows); + } + + public function raw( + string $sql, + array $bindings = [], + ): array { + return []; + } + + public function groupBy(string ...$columns): static + { + return $this; + } + + public function having( + string $expression, + array $bindings = [], + ): static { + return $this; + } + + public function min(string $column): int|float|null + { + return null; + } + + public function max(string $column): int|float|null + { + return null; + } + + public function sum(string $column): int|float|null + { + return null; + } + + public function avg(string $column): int|float|null + { + return null; + } + }; + + return new readonly class ($stub) implements QueryBuilderFactoryInterface + { + public function __construct(private QueryBuilderInterface $builder) {} + + public function create(): QueryBuilderInterface + { + return $this->builder; + } + }; +} + +function makeFobLoader(array $relatedRows = []): RelationshipLoader +{ + return new RelationshipLoader( + new EntityMetadataFactory(), + new EntityHydrator(), + makeFobQbFactory($relatedRows), + ); +} + +// ── Tests ────────────────────────────────────────────────────────────────────── + +describe('findOneBy LIMIT 1', function (): void { + it('returns the matching entity for findOneBy when a row exists', function (): void { + $row = ['id' => 7, 'email' => 'alice@example.com', 'status' => 'active']; + $connection = makeFobConnection([$row]); + $repo = new FobUserRepository($connection, new EntityMetadataFactory(), new EntityHydrator()); + + /** @var FobUser $entity */ + $entity = $repo->findOneBy(['email' => 'alice@example.com']); + + expect($entity)->toBeInstanceOf(FobUser::class) + ->and($entity->id)->toBe(7) + ->and($entity->email)->toBe('alice@example.com'); + }); + + it('returns null from findOneBy when no row matches', function (): void { + $connection = makeFobConnection([]); + $repo = new FobUserRepository($connection, new EntityMetadataFactory(), new EntityHydrator()); + + $entity = $repo->findOneBy(['email' => 'ghost@example.com']); + + expect($entity)->toBeNull(); + }); + + it('appends LIMIT 1 to the findOneBy query', function (): void { + $queries = []; + $row = ['id' => 1, 'email' => 'x@example.com', 'status' => 'active']; + $connection = makeFobRecordingConnection([$row], $queries); + $repo = new FobUserRepository($connection, new EntityMetadataFactory(), new EntityHydrator()); + + $repo->findOneBy(['email' => 'x@example.com']); + + expect($queries)->toHaveCount(1) + ->and($queries[0]['sql'])->toContain('LIMIT 1'); + }); + + it('issues exactly one query for findOneBy even when multiple rows would match', function (): void { + $queries = []; + $rows = [ + ['id' => 1, 'email' => 'a@example.com', 'status' => 'active'], + ['id' => 2, 'email' => 'b@example.com', 'status' => 'active'], + ['id' => 3, 'email' => 'c@example.com', 'status' => 'active'], + ]; + // Connection returns all 3 rows (simulating DB returning multiple), but the + // SQL itself contains LIMIT 1 — one query is issued. + $connection = makeFobRecordingConnection($rows, $queries); + $repo = new FobUserRepository($connection, new EntityMetadataFactory(), new EntityHydrator()); + + $repo->findOneBy(['status' => 'active']); + + expect($queries)->toHaveCount(1); + }); + + it('maps entity property names to column names in the findOneBy WHERE clause', function (): void { + $queries = []; + $row = ['id' => 5, 'email' => 'mapped@example.com', 'status' => 'pending']; + $connection = makeFobRecordingConnection([$row], $queries); + $repo = new FobUserRepository($connection, new EntityMetadataFactory(), new EntityHydrator()); + + // 'status' maps to column 'status'; verify the WHERE clause uses the column name + $repo->findOneBy(['status' => 'pending']); + + expect($queries[0]['sql'])->toContain('status = ?') + ->and($queries[0]['bindings'])->toBe(['pending']); + }); + + it('eager-loads relationships on the single entity returned by findOneBy', function (): void { + $userRow = ['id' => 3, 'email' => 'rel@example.com', 'status' => 'active']; + $profileRow = ['id' => 30, 'user_id' => 3]; + + $factory = makeFobQbFactory([$profileRow]); + $loader = new RelationshipLoader(new EntityMetadataFactory(), new EntityHydrator(), $factory); + + $repo = new FobUserRepository( + connection: makeFobConnection([$userRow]), + metadataFactory: new EntityMetadataFactory(), + hydrator: new EntityHydrator(), + queryBuilderFactory: $factory, + eventDispatcher: null, + relationshipLoader: $loader, + ); + + /** @var FobUser $entity */ + $entity = $repo->with('profile')->findOneBy(['email' => 'rel@example.com']); + + expect($entity)->toBeInstanceOf(FobUser::class) + ->and($entity->profile)->toBeInstanceOf(FobProfile::class) + ->and($entity->profile->id)->toBe(30); + }); +}); diff --git a/packages/docs-markdown/docs/packages/admin-auth.md b/packages/docs-markdown/docs/packages/admin-auth.md index 3fce3742..7c3246fe 100644 --- a/packages/docs-markdown/docs/packages/admin-auth.md +++ b/packages/docs-markdown/docs/packages/admin-auth.md @@ -210,10 +210,14 @@ interface RoleRepositoryInterface extends RepositoryInterface { public function findBySlug(string $slug): ?Role; public function getPermissionsForRole(int $roleId): array; + /** @return array */ + public function getPermissionsForRoles(array $roleIds): array; public function syncPermissions(int $roleId, array $permissionIds): void; public function isSlugUnique(string $slug, ?int $excludeId = null): bool; } +`getPermissionsForRoles()` returns the deduplicated permission set across all given role IDs in a single query. Empty input returns an empty array without issuing a query. + interface PermissionRepositoryInterface extends RepositoryInterface { public function findByKey(string $key): ?Permission; diff --git a/packages/docs-markdown/docs/packages/media.md b/packages/docs-markdown/docs/packages/media.md index bdf635a9..19f175d4 100644 --- a/packages/docs-markdown/docs/packages/media.md +++ b/packages/docs-markdown/docs/packages/media.md @@ -428,8 +428,15 @@ use Marko\Media\Entity\Media; public function save(Media $media): Media; public function delete(int $id): void; public function find(int $id): ?Media; + +/** @return array */ +public function findMany(array $ids): array; ``` +`findMany()` accepts an `array` of IDs and returns all matching `Media` entities in a single query. Empty input returns an empty array without issuing a query. IDs with no matching row are silently skipped. The returned order is unspecified --- callers are responsible for reordering when a specific order is required. + +Consumers implementing `MediaRepositoryInterface` must add this method. + ### MediaAttachmentRepositoryInterface ```php diff --git a/packages/docs-markdown/docs/packages/notification.md b/packages/docs-markdown/docs/packages/notification.md index 1b6511ae..9bc39d3f 100644 --- a/packages/docs-markdown/docs/packages/notification.md +++ b/packages/docs-markdown/docs/packages/notification.md @@ -236,6 +236,48 @@ interface ChannelInterface } ``` +### BatchChannelInterface + +Channels may opt into batch delivery by implementing `BatchChannelInterface` in addition to `ChannelInterface`. When `NotificationSender` fans out a notification to more than one recipient on the same channel and that channel implements `BatchChannelInterface`, it calls `sendMany()` instead of per-recipient `send()` calls. For non-batch channels the fallback per-recipient path is always used. + +```php +use Marko\Notification\Contracts\BatchChannelInterface; +use Marko\Notification\Contracts\ChannelInterface; +use Marko\Notification\Contracts\NotifiableInterface; +use Marko\Notification\Contracts\NotificationInterface; + +class SmsChannel implements ChannelInterface, BatchChannelInterface +{ + public function send( + NotifiableInterface $notifiable, + NotificationInterface $notification, + ): void { + // single-recipient delivery + } + + public function sendMany( + array $notifiables, + NotificationInterface $notification, + ): void { + // bulk delivery in one operation + } +} +``` + +The built-in `DatabaseChannel` implements `BatchChannelInterface` and persists multiple recipients via a single chunked multi-row INSERT. + +```php +use Marko\Notification\Contracts\BatchChannelInterface; +use Marko\Notification\Contracts\NotifiableInterface; +use Marko\Notification\Contracts\NotificationInterface; + +interface BatchChannelInterface +{ + /** @param array $notifiables */ + public function sendMany(array $notifiables, NotificationInterface $notification): void; +} +``` + ### Exceptions | Exception | Description | diff --git a/packages/media/src/Contracts/MediaRepositoryInterface.php b/packages/media/src/Contracts/MediaRepositoryInterface.php index 7f461f9d..1a08a39a 100644 --- a/packages/media/src/Contracts/MediaRepositoryInterface.php +++ b/packages/media/src/Contracts/MediaRepositoryInterface.php @@ -19,4 +19,22 @@ public function delete( public function find( int $id, ): ?Media; + + /** + * Return all Media entities whose ids are in the given list. + * + * Empty input MUST return an empty array without issuing a query. + * Ids that have no matching row are silently skipped. + * The returned array order is unspecified; callers are responsible + * for reordering if a specific order is required. + * + * A single-query implementation (e.g. WHERE id IN (...)) is the + * recommended approach for consumer implementations. + * + * @param array $ids + * @return array + */ + public function findMany( + array $ids, + ): array; } diff --git a/packages/media/src/Service/AttachmentManager.php b/packages/media/src/Service/AttachmentManager.php index 94bcac59..3481e2f0 100644 --- a/packages/media/src/Service/AttachmentManager.php +++ b/packages/media/src/Service/AttachmentManager.php @@ -52,11 +52,19 @@ public function findByAttachable( $attachableId, ); + if ($mediaIds === []) { + return []; + } + + $mediaById = []; + foreach ($this->mediaRepository->findMany($mediaIds) as $media) { + $mediaById[(int) $media->id] = $media; + } + $result = []; foreach ($mediaIds as $id) { - $media = $this->mediaRepository->find($id); - if ($media !== null) { - $result[] = $media; + if (isset($mediaById[$id])) { + $result[] = $mediaById[$id]; } } diff --git a/packages/media/tests/Contracts/MediaRepositoryInterfaceTest.php b/packages/media/tests/Contracts/MediaRepositoryInterfaceTest.php new file mode 100644 index 00000000..4556f49b --- /dev/null +++ b/packages/media/tests/Contracts/MediaRepositoryInterfaceTest.php @@ -0,0 +1,25 @@ +isInterface())->toBeTrue() + ->and($reflection->hasMethod('findMany'))->toBeTrue(); + + $method = $reflection->getMethod('findMany'); + $params = $method->getParameters(); + $returnType = $method->getReturnType(); + + expect($params)->toHaveCount(1) + ->and($params[0]->getName())->toBe('ids') + ->and($returnType)->toBeInstanceOf(ReflectionNamedType::class) + ->and($returnType->getName())->toBe('array'); +}); diff --git a/packages/media/tests/Service/AttachmentManagerTest.php b/packages/media/tests/Service/AttachmentManagerTest.php index c42dbb5e..df69e879 100644 --- a/packages/media/tests/Service/AttachmentManagerTest.php +++ b/packages/media/tests/Service/AttachmentManagerTest.php @@ -110,9 +110,198 @@ public function find( ): ?Media { return $this->storage[$id] ?? null; } + + /** + * @param array $ids + * @return array + */ + public function findMany( + array $ids, + ): array { + return array_values(array_filter( + array_map(fn (int $id) => $this->storage[$id] ?? null, $ids), + fn ($m) => $m !== null, + )); + } }; } +function makeTrackingMediaRepository( + Media ...$media, +): MediaRepositoryInterface { + return new class ($media) implements MediaRepositoryInterface + { + /** @var array */ + private array $storage; + + public int $findCallCount = 0; + + public int $findManyCallCount = 0; + + /** @param array $media */ + public function __construct( + array $media, + ) { + $this->storage = []; + foreach ($media as $m) { + $this->storage[(int) $m->id] = $m; + } + } + + public function save( + Media $media, + ): Media { + $this->storage[(int) $media->id] = $media; + + return $media; + } + + public function delete( + int $id, + ): void { + unset($this->storage[$id]); + } + + public function find( + int $id, + ): ?Media { + ++$this->findCallCount; + + return $this->storage[$id] ?? null; + } + + /** + * @param array $ids + * @return array + */ + public function findMany( + array $ids, + ): array { + ++$this->findManyCallCount; + + return array_values(array_filter( + array_map(fn (int $id) => $this->storage[$id] ?? null, $ids), + fn ($m) => $m !== null, + )); + } + }; +} + +it('returns the media entities for all attached ids', function (): void { + $media1 = makeAttachMedia(id: 1, path: '2024/01/img1.jpg'); + $media2 = makeAttachMedia(id: 2, path: '2024/01/img2.jpg'); + $attachmentRepo = makeAttachmentRepository(); + $mediaRepo = makeTrackingMediaRepository($media1, $media2); + + $manager = new AttachmentManager( + attachmentRepository: $attachmentRepo, + mediaRepository: $mediaRepo, + ); + + $manager->attach($media1, 'App\Post', 10); + $manager->attach($media2, 'App\Post', 10); + + $found = $manager->findByAttachable('App\Post', 10); + + expect($found)->toHaveCount(2) + ->and($found[0])->toBeInstanceOf(Media::class) + ->and($found[1])->toBeInstanceOf(Media::class); +}); + +it('returns media in the order of the attachment id list', function (): void { + $media1 = makeAttachMedia(id: 1, path: '2024/01/img1.jpg'); + $media2 = makeAttachMedia(id: 2, path: '2024/01/img2.jpg'); + $media3 = makeAttachMedia(id: 3, path: '2024/01/img3.jpg'); + $attachmentRepo = makeAttachmentRepository(); + $mediaRepo = makeTrackingMediaRepository($media1, $media2, $media3); + + $manager = new AttachmentManager( + attachmentRepository: $attachmentRepo, + mediaRepository: $mediaRepo, + ); + + $manager->attach($media3, 'App\Post', 10); + $manager->attach($media1, 'App\Post', 10); + $manager->attach($media2, 'App\Post', 10); + + $found = $manager->findByAttachable('App\Post', 10); + + expect($found)->toHaveCount(3) + ->and($found[0]->id)->toBe(3) + ->and($found[1]->id)->toBe(1) + ->and($found[2]->id)->toBe(2); +}); + +it('skips ids that have no matching media row', function (): void { + $media1 = makeAttachMedia(id: 1, path: '2024/01/img1.jpg'); + $attachmentRepo = makeAttachmentRepository(); + $mediaRepo = makeTrackingMediaRepository($media1); + + $manager = new AttachmentManager( + attachmentRepository: $attachmentRepo, + mediaRepository: $mediaRepo, + ); + + $manager->attach($media1, 'App\Post', 10); + // Manually inject a dangling attachment id (99) that has no media row + $attachmentRepo->attachments[] = ['mediaId' => 99, 'attachableType' => 'App\Post', 'attachableId' => 10]; + + $found = $manager->findByAttachable('App\Post', 10); + + expect($found)->toHaveCount(1) + ->and($found[0]->id)->toBe(1); +}); + +it('returns an empty array when there are no attachments', function (): void { + $attachmentRepo = makeAttachmentRepository(); + $mediaRepo = makeTrackingMediaRepository(); + + $manager = new AttachmentManager( + attachmentRepository: $attachmentRepo, + mediaRepository: $mediaRepo, + ); + + $found = $manager->findByAttachable('App\Post', 10); + + expect($found)->toBeEmpty(); +}); + +it('resolves all attachments with a single findMany call and never calls find per id', function (): void { + $media1 = makeAttachMedia(id: 1); + $media2 = makeAttachMedia(id: 2); + $media3 = makeAttachMedia(id: 3); + $attachmentRepo = makeAttachmentRepository(); + $mediaRepo = makeTrackingMediaRepository($media1, $media2, $media3); + + $manager = new AttachmentManager( + attachmentRepository: $attachmentRepo, + mediaRepository: $mediaRepo, + ); + + $manager->attach($media1, 'App\Post', 10); + $manager->attach($media2, 'App\Post', 10); + $manager->attach($media3, 'App\Post', 10); + + $manager->findByAttachable('App\Post', 10); + + expect($mediaRepo->findCallCount)->toBe(0) + ->and($mediaRepo->findManyCallCount)->toBe(1); +}); + +it('does not invoke findMany when there are no attachment ids', function (): void { + $attachmentRepo = makeAttachmentRepository(); + $mediaRepo = makeTrackingMediaRepository(); + + $manager = new AttachmentManager( + attachmentRepository: $attachmentRepo, + mediaRepository: $mediaRepo, + ); + + $manager->findByAttachable('App\Post', 10); + + expect($mediaRepo->findManyCallCount)->toBe(0); +}); + it('retrieves all media attached to a given entity', function (): void { $media1 = makeAttachMedia(id: 1, path: '2024/01/img1.jpg'); $media2 = makeAttachMedia(id: 2, path: '2024/01/img2.jpg'); diff --git a/packages/media/tests/Service/MediaManagerTest.php b/packages/media/tests/Service/MediaManagerTest.php index daf7ca0b..d87ff424 100644 --- a/packages/media/tests/Service/MediaManagerTest.php +++ b/packages/media/tests/Service/MediaManagerTest.php @@ -274,6 +274,19 @@ public function find( ): ?Media { return $this->saved[$id] ?? null; } + + /** + * @param array $ids + * @return array + */ + public function findMany( + array $ids, + ): array { + return array_values(array_filter( + array_map(fn (int $id) => $this->saved[$id] ?? null, $ids), + fn ($m) => $m !== null, + )); + } }; } diff --git a/packages/notification/src/Channel/DatabaseChannel.php b/packages/notification/src/Channel/DatabaseChannel.php index fc09ecdd..ad02aefb 100644 --- a/packages/notification/src/Channel/DatabaseChannel.php +++ b/packages/notification/src/Channel/DatabaseChannel.php @@ -5,14 +5,22 @@ namespace Marko\Notification\Channel; use Marko\Database\Connection\ConnectionInterface; +use Marko\Notification\Contracts\BatchChannelInterface; use Marko\Notification\Contracts\ChannelInterface; use Marko\Notification\Contracts\NotifiableInterface; use Marko\Notification\Contracts\NotificationInterface; use Marko\Notification\Exceptions\ChannelException; +use Random\RandomException; use Throwable; -class DatabaseChannel implements ChannelInterface +class DatabaseChannel implements ChannelInterface, BatchChannelInterface { + /** + * Maximum rows per INSERT statement to keep placeholder count within driver limits. + * 7 columns × 500 rows = 3500 placeholders (safely under the 65535 MySQL/PDO limit). + */ + private const int ROWS_PER_CHUNK = 500; + public function __construct( private ConnectionInterface $connection, ) {} @@ -46,8 +54,48 @@ public function send( } } + /** + * Persist notifications for multiple notifiables using chunked multi-row INSERTs. + * + * @param array $notifiables + * @throws ChannelException On batch insert failure + */ + public function sendMany( + array $notifiables, + NotificationInterface $notification, + ): void { + $chunks = array_chunk($notifiables, self::ROWS_PER_CHUNK); + + foreach ($chunks as $chunk) { + try { + $placeholderRow = '(?, ?, ?, ?, ?, ?, ?)'; + $placeholders = implode(', ', array_fill(0, count($chunk), $placeholderRow)); + + $sql = 'INSERT INTO notifications (id, type, notifiable_type, notifiable_id, data, read_at, created_at) VALUES ' . $placeholders; + + $bindings = []; + foreach ($chunk as $notifiable) { + $data = $notification->toDatabase($notifiable); + $bindings[] = $this->generateUuid(); + $bindings[] = $notification::class; + $bindings[] = $notifiable->getNotifiableType(); + $bindings[] = (string) $notifiable->getNotifiableId(); + $bindings[] = json_encode($data, JSON_THROW_ON_ERROR); + $bindings[] = null; + $bindings[] = date('Y-m-d H:i:s'); + } + + $this->connection->execute($sql, $bindings); + } catch (Throwable $e) { + throw ChannelException::deliveryFailed('database', $e->getMessage()); + } + } + } + /** * Generate a UUID v4 string. + * + * @throws RandomException */ private function generateUuid(): string { diff --git a/packages/notification/src/Contracts/BatchChannelInterface.php b/packages/notification/src/Contracts/BatchChannelInterface.php new file mode 100644 index 00000000..6f53e2a4 --- /dev/null +++ b/packages/notification/src/Contracts/BatchChannelInterface.php @@ -0,0 +1,21 @@ + $notifiables + * @throws ChannelException On batch delivery failure + */ + public function sendMany( + array $notifiables, + NotificationInterface $notification, + ): void; +} diff --git a/packages/notification/src/NotificationSender.php b/packages/notification/src/NotificationSender.php index 24f8d6c7..5ca0a06a 100644 --- a/packages/notification/src/NotificationSender.php +++ b/packages/notification/src/NotificationSender.php @@ -4,6 +4,7 @@ namespace Marko\Notification; +use Marko\Notification\Contracts\BatchChannelInterface; use Marko\Notification\Contracts\NotifiableInterface; use Marko\Notification\Contracts\NotificationInterface; use Marko\Notification\Exceptions\ChannelException; @@ -31,19 +32,40 @@ public function send( ): void { $notifiables = is_array($notifiables) ? $notifiables : [$notifiables]; + // Build a map of channel-name => list of recipients that declared that channel. + /** @var array> $channelRecipients */ + $channelRecipients = []; + foreach ($notifiables as $notifiable) { $channels = $notification->channels($notifiable); foreach ($channels as $channelName) { - $channel = $this->manager->channel($channelName); + $channelRecipients[$channelName][] = $notifiable; + } + } + // For each channel, use batch path when supported and multiple recipients; otherwise per-recipient. + foreach ($channelRecipients as $channelName => $recipients) { + $channel = $this->manager->channel($channelName); + + if ($channel instanceof BatchChannelInterface && count($recipients) > 1) { try { - $channel->send($notifiable, $notification); + $channel->sendMany($recipients, $notification); } catch (ChannelException $e) { throw $e; } catch (Throwable $e) { throw NotificationException::sendFailed($channelName, $e->getMessage()); } + } else { + foreach ($recipients as $notifiable) { + try { + $channel->send($notifiable, $notification); + } catch (ChannelException $e) { + throw $e; + } catch (Throwable $e) { + throw NotificationException::sendFailed($channelName, $e->getMessage()); + } + } } } } diff --git a/packages/notification/tests/Unit/Channel/DatabaseChannelBatchTest.php b/packages/notification/tests/Unit/Channel/DatabaseChannelBatchTest.php new file mode 100644 index 00000000..66fda0d9 --- /dev/null +++ b/packages/notification/tests/Unit/Channel/DatabaseChannelBatchTest.php @@ -0,0 +1,245 @@ +}> */ + public array $executeCalls = []; + + public function connect(): void {} + + public function disconnect(): void {} + + public function isConnected(): bool + { + return true; + } + + /** + * @return array> + */ + public function query( + string $sql, + array $bindings = [], + ): array { + return []; + } + + public function execute( + string $sql, + array $bindings = [], + ): int { + $this->executeCalls[] = ['sql' => $sql, 'bindings' => $bindings]; + + return count($bindings) / 7; + } + + public function prepare(string $sql): StatementInterface + { + throw new RuntimeException('Not implemented in test stub'); + } + + public function lastInsertId(): int + { + return 0; + } + + public function driverName(): string + { + return 'mysql'; + } +} + +function makeBatchNotifiable(string $type = 'App\\Entity\\User', int|string $id = 1): NotifiableInterface +{ + return new readonly class ($type, $id) implements NotifiableInterface + { + public function __construct( + private string $type, + private int|string $id, + ) {} + + public function routeNotificationFor(string $channel): mixed + { + return null; + } + + public function getNotifiableType(): string + { + return $this->type; + } + + public function getNotifiableId(): int|string + { + return $this->id; + } + }; +} + +function makeNotification(array $data = ['message' => 'Hello']): NotificationInterface +{ + return new readonly class ($data) implements NotificationInterface + { + public function __construct( + private array $data, + ) {} + + /** @return array */ + public function channels(NotifiableInterface $notifiable): array + { + return ['database']; + } + + public function toMail(NotifiableInterface $notifiable): Message + { + return new Message(); + } + + /** @return array */ + public function toDatabase(NotifiableInterface $notifiable): array + { + return $this->data; + } + }; +} + +test('it persists a notification for every recipient on the database channel', function (): void { + $connection = new BatchTestConnection(); + $channel = new DatabaseChannel($connection); + $notification = makeNotification(); + + $notifiables = [ + makeBatchNotifiable(id: 1), + makeBatchNotifiable(id: 2), + makeBatchNotifiable(id: 3), + ]; + + $channel->sendMany($notifiables, $notification); + + // All 3 recipients should be persisted + $totalBindings = array_merge(...array_column($connection->executeCalls, 'bindings')); + $totalRows = count($totalBindings) / 7; + + expect($totalRows)->toBe(3); +}); + +test('it issues a single multi-row insert when all recipients fit one chunk', function (): void { + $connection = new BatchTestConnection(); + $channel = new DatabaseChannel($connection); + $notification = makeNotification(); + + $notifiables = [ + makeBatchNotifiable(id: 1), + makeBatchNotifiable(id: 2), + makeBatchNotifiable(id: 3), + ]; + + $channel->sendMany($notifiables, $notification); + + expect($connection->executeCalls)->toHaveCount(1); +}); + +test('it issues one insert per chunk when recipients exceed the chunk size', function (): void { + $connection = new BatchTestConnection(); + $channel = new DatabaseChannel($connection); + $notification = makeNotification(); + + // Create enough notifiables to exceed one chunk + // Chunk size is based on placeholder limit; use reflection to get it + $reflection = new ReflectionClass(DatabaseChannel::class); + $chunkSizeConst = $reflection->getConstant('ROWS_PER_CHUNK'); + + $notifiables = []; + for ($i = 1; $i <= $chunkSizeConst + 1; $i++) { + $notifiables[] = makeBatchNotifiable(id: $i); + } + + $channel->sendMany($notifiables, $notification); + + expect($connection->executeCalls)->toHaveCount(2); +}); + +test('it writes the same column data per row as the single-recipient send', function (): void { + $connection = new BatchTestConnection(); + $channel = new DatabaseChannel($connection); + $notification = makeNotification(['key' => 'value', 'order_id' => 42]); + + $notifiable = makeBatchNotifiable(type: 'App\\Entity\\User', id: 99); + + $channel->sendMany([$notifiable, makeBatchNotifiable(id: 2)], $notification); + + $bindings = $connection->executeCalls[0]['bindings']; + // First row bindings: id, type, notifiable_type, notifiable_id, data, read_at, created_at + $firstRowBindings = array_slice($bindings, 0, 7); + + expect($firstRowBindings[1])->toBeString() // type = notification class name + ->and($firstRowBindings[2])->toBe('App\\Entity\\User') // notifiable_type + ->and($firstRowBindings[3])->toBe('99') // notifiable_id as string + ->and(json_decode($firstRowBindings[4], true))->toBe(['key' => 'value', 'order_id' => 42]) // data JSON + ->and($firstRowBindings[5])->toBeNull() // read_at + ->and($firstRowBindings[6])->toBeString(); // created_at +}); + +test('it generates a distinct id for each persisted notification row', function (): void { + $connection = new BatchTestConnection(); + $channel = new DatabaseChannel($connection); + $notification = makeNotification(); + + $notifiables = [ + makeBatchNotifiable(id: 1), + makeBatchNotifiable(id: 2), + makeBatchNotifiable(id: 3), + ]; + + $channel->sendMany($notifiables, $notification); + + $bindings = $connection->executeCalls[0]['bindings']; + + // Extract UUIDs (index 0, 7, 14 for 3 rows of 7 columns) + $uuids = []; + for ($i = 0; $i < 3; $i++) { + $uuids[] = $bindings[$i * 7]; + } + + expect(array_unique($uuids))->toHaveCount(3); +}); + +test('it wraps a batch insert failure in a channel exception', function (): void { + $connection = new class () extends BatchTestConnection + { + public function execute( + string $sql, + array $bindings = [], + ): int { + throw new RuntimeException('DB connection lost'); + } + }; + + $channel = new DatabaseChannel($connection); + $notification = makeNotification(); + + $notifiables = [makeBatchNotifiable(id: 1), makeBatchNotifiable(id: 2)]; + + expect(fn () => $channel->sendMany($notifiables, $notification)) + ->toThrow(ChannelException::class, "Failed to deliver notification via 'database' channel."); +}); + +test('it leaves MailChannel and other ChannelInterface implementations unchanged', function (): void { + $reflection = new ReflectionClass(MailChannel::class); + + expect($reflection->implementsInterface(BatchChannelInterface::class))->toBeFalse() + ->and($reflection->implementsInterface(ChannelInterface::class))->toBeTrue(); +}); diff --git a/packages/notification/tests/Unit/NotificationSenderTest.php b/packages/notification/tests/Unit/NotificationSenderTest.php index 3749e00a..d248c81d 100644 --- a/packages/notification/tests/Unit/NotificationSenderTest.php +++ b/packages/notification/tests/Unit/NotificationSenderTest.php @@ -116,6 +116,24 @@ $sender->queue($notifiable, $notification); }); +test('it falls back to per-recipient send for channels without batch support', function (): void { + $notifiableA = $this->createMock(NotifiableInterface::class); + $notifiableB = $this->createMock(NotifiableInterface::class); + + // Plain ChannelInterface (not BatchChannelInterface) — expects 2 individual send() calls + $channel = $this->createMock(ChannelInterface::class); + $channel->expects($this->exactly(2))->method('send'); + + $manager = new NotificationManager(); + $manager->register('database', $channel); + + $notification = $this->createMock(NotificationInterface::class); + $notification->method('channels')->willReturn(['database']); + + $sender = new NotificationSender($manager); + $sender->send([$notifiableA, $notifiableB], $notification); +}); + test('it wraps channel delivery failures in ChannelException', function (): void { $notifiable = $this->createMock(NotifiableInterface::class);