Repository navigation
Improve Permission authorization and cache performance - #54
Merged
Merged
Conversation
Bring the Permission command, guard, provider and integration tests in
line with spatie/laravel-permission main at 6615eefac655 (8.x): upstream
case names, assertions, comments and file placement, with Gate and
CustomGate tests moved under Integration/. Stronger Hypervel coverage is
kept, including the zero team id, guard named "0" and non-HasRoles model
cases. The test base no longer forces the teams, cache and model
settings, so method-level environment attributes apply, and CACHE_STORE
selects the cache store like upstream's CACHE_DRIVER. A database store
migrates its cache tables first, and lock pruning is disabled because it
adds a random query to counted tests.
Fixes found along the way:
- set{Permission,Role,Team}Class() now write the config like upstream.
initializeCache() re-reads it, so reinitializing reverted the class
while the container binding kept the new one.
- The registrar resolves the cache manager when initializing the cache
instead of keeping the one it was built with, so a rebound cache
manager takes effect (upstream #2973).
- Required settings that always ship in the config lose their code
fallbacks; the optional cache settings keep one documented default,
with a constant for the expiration.
- The about command drops an always-present "Denied Permissions" entry
that made the "Default" label unreachable.
- permission:setup-teams uses now(), and two Closure::fromCallable()
static-analysis workarounds become direct calls.
Hypervel adaptations: an undefined permission cache store throws instead
of silently falling back to the array store; setup-teams returns failure
when the migration cannot be written; upstream's chmod-based failure
case uses an unwritable destination because CI runs as root; Octane
listener tests are not ported because team ids and loaded catalogs are
coroutine-local.
Validation: the Permission suite passes on the array store, these files
pass on the database store, the Postgres Permission tests pass, and
formatting and static analysis are clean.
The model cache coordinator rechecked the cache after acquiring its fill lock with a plain get(). Through a memoized repository, that read returned the miss remembered before the lock, so a fill finished by another request in the meantime was loaded and published again. Later fills in the same coroutine also locked and read the store again, because memoized writes forget their key. The recheck now uses getAuthoritativeRaw() when the repository supports it, matching Repository::flexible(). An envelope found by the recheck, or one published through the fill repository, replaces the coroutine's memo entry for a plain MemoizedStore. Failed writes, lost leases and lazy writer repositories are not remembered. Tagged keys never reach that memo. On the database cache store, a cold Permission catalog fill is now 9 statements and a cold authorization 24; the roles re-read after filling is gone. Validation: tests/Cache, tests/Auth, tests/Integration/Auth (with Redis), tests/Sanctum, tests/Permission on the array and database stores (only known later-slice failures), cs-fixer and composer analyse.
…upstream Bring the middleware, model and reverse-assignment tests in line with spatie/laravel-permission main at 6615eefac655 (8.x): upstream case names, order and assertions, including every client case. The separate Passport client middleware test is folded into the upstream client cases, keeping its disabled-credentials case and its same-permission precheck in the via-role case. Stronger Hypervel assertions stay inside the upstream-named cases: a non-matching wildcard permission is denied, and an admin-guard user is denied web-guard roles and permissions. Hypervel-only cases for JSON responses, users without Authorizable, the empty guard and integer-backed enums are kept. Wildcard and model tests move config to defineEnvironment() and seeding into the test coroutine, the teams assigned-model case uses a method-level environment, and its duplicate in the team variant is removed. Deprecated expectExceptionMessage() calls become expectExceptionMessageIsOrContains(). Source cleanups: - RoleMiddleware, RoleOrPermissionMiddleware and WildcardPermission call hasAnyRole() and getAllPermissions() directly like upstream instead of through Closure::fromCallable() static-analysis workarounds. - The middleware and the provider's route macros convert listed enum names with array_map(enum_value(...)) instead of untyped wrappers; the route macros keep mixed input and declare their Route return type. The middleware keep Hypervel's Authorizable check and per-name can() loop because the Authorizable contract does not include canAny(). Validation: these files pass on the array and database cache stores; the Permission suite shows only known later-slice failures; formatting and static analysis are clean.
…validation Bring HasPermissionsTest in line with spatie/laravel-permission main at 6615eefac655 (8.x): every upstream case under its upstream name and order, the restored third user and comment, and the ported detach-event case for syncPermissions(). The custom-pivot case changes the auth provider model at runtime, so it calls Guard::flushState() after the change because Guard caches provider models for the worker lifetime. Exception-only catch blocks drop their no-op assertTrue(true), and deprecated expectExceptionMessage() becomes expectExceptionMessageIsOrContains(). The custom-models variant renames its skipped integer-scope override to match. Count cases add the database cache store's statements through a new TestCase::usesDatabaseCacheStore() helper. PermissionRegistrarTest's final missing-name cases expect their exceptions directly. Source fix: saving a model with queued assignments invalidated both its role and direct-permission caches for every queued context, even when only permissions were queued. On the database cache store that cost three extra statements per save (8 instead of 5). The flush now invalidates the role cache only for queued role contexts and the permission cache through the existing per-context permission path. Reverse role assignment had the same waste for each affected model and now invalidates only role caches. The combined registrar invalidation methods lost their callers and are removed. A root CacheTest case covers the reverse-assignment path keeping the warm direct-permission memo. Validation: HasPermissionsTest and PermissionRegistrarTest pass on the array and database cache stores; the Permission suite passes on the array store, with only known later-slice failures on the database store; formatting and static analysis are clean.
Bring HasPermissionsWithCustomModelsTest and TeamHasPermissionsTest in line with spatie/laravel-permission main at 6615eefac655 (8.x). The custom-models variant follows upstream's case order and restores its skip comment for the integer-scope override. TeamHasPermissionsTest gains upstream's five custom-pivot team cases and their fixtures, and three cases take their upstream names. Several upstream cases change the auth provider model at runtime. The new TestCase::useAuthUserModel() sets it and flushes Guard's provider model cache, which lasts for the worker lifetime; the HasPermissionsTest custom-pivot case now uses it too. The custom-models variant's database-store failures came from an always-zero query-count offset. The counts now name the database cache store's real statements: soft deletes invalidate the permission catalog when deleting and when deleted, force deletes invalidate it once in the delete transaction and write a new assignment token, and the team warm reuse case pays for two assignment cache fills. Validation: HasPermissionsTest, HasPermissionsWithCustomModelsTest and TeamHasPermissionsTest pass on the array and database cache stores; the Permission suite passes on the array store, with only known later-slice failures on the database store; formatting is clean.
Bring HasRolesTest in line with spatie/laravel-permission main at 6615eefac655 (8.x): every upstream case under its upstream name and order, upstream's pipe-conversion and custom-pivot cases, restored comments, the string scope's second user role, the object scope's statement order and Admin::all() in the guard withoutscope case. The pipe-conversion case replaces Hypervel's malformed-quote case, which covered one of its inputs. The cross-guard sync catch drops its no-op assertTrue(true), and deprecated expectExceptionMessage() becomes expectExceptionMessageIsOrContains(). Upstream's teams branch in the unnecessary-SQL case is dropped: the sync reads the current team's pivot rows directly instead of reloading the relation. TeamHasRolesTest therefore inherits the base case, and its duplicate override is removed. Count cases add the database cache store's statements. Test setup fix: setUpBaseTestPermissions() created the test user and admin, so setUpCustomModels() added a second pair. The custom-models variant's Admin::all() then returned two models, which enables lazy-load prevention for the admin's touched relations on delete; the base case had worked around it with an eager load. The user and admin are now created once after the fixture tables, like upstream's setUpDatabase(). Validation: HasRolesTest and TeamHasRolesTest pass on the array and database cache stores; the custom-model variants, DeniedPermissionTest and RoleWithNestingTest pass on the array store; the Permission suite passes on the array store, with only known later-slice failures on the database store; formatting is clean.
… upstream Bring HasRolesWithCustomModelsTest, TeamHasRolesTest and TeamScopeTest in line with spatie/laravel-permission main at 6615eefac655 (8.x). HasRolesWithCustomModelsTest takes upstream's case names and order. Its always-zero query-count offset is replaced by the database cache store's real statements: soft deletes invalidate the role catalog when deleting and when deleted, and force deletes invalidate it once in the delete transaction and write a new assignment token. TeamHasRolesTest runs the team pivot-deletion body under the base name and the base body as ...FromHasRolesTest, as upstream names them. It gains upstream's five custom-pivot team cases and their fixtures, restores the multi-team case's comments, takes the upstream sync-or- remove name and drops a no-op assertTrue(true). TeamScopeTest restores upstream's two multi-expectation exception cases from five split cases and the introspection case's full name. The team class config in defineEnvironment() is removed because setTeamClass() in setup already sets it; permission.teams stays there because the migration adds team columns only when teams are enabled. Deprecated expectExceptionMessage() becomes expectExceptionMessageIsOrContains(). TestCase types getPackageProviders()'s $app like its parent and imports the package role and permission models for its property types. Validation: the three files pass on the array and database cache stores; the Permission suite passes on the array store, with only known later-slice failures on the database store; formatting is clean.
…ions Bring WildcardHasPermissionsTest in line with spatie/laravel-permission main at 6615eefac655 (8.x): all 19 upstream cases under their upstream names, order, variables and comments, plus upstream's two wildcard index cases for assigning and removing a role. Wildcards are enabled in defineEnvironment() instead of setUp(), which mutated config and flushed the cache outside the test coroutine and failed on Redis. A Hypervel case covers syncModels() rotating the assignment token that every wildcard index key includes. The Hypervel-only WildcardPermissionTest is removed: five of its cases duplicated upstream cases, and its token case built its state with a raw pivot delete and a direct token rotation. UnitEnumTest now covers the signatures Hypervel widens from upstream's BackedEnum: findOrCreate() and findByName() on roles and permissions, and the three middleware using() methods. Catalog models were cloned from a new model prototype, as upstream does, so their connection name stayed null while database-loaded models carry the resolved name. Model::is() compares connection names, so $user->roles->contains(Role::findByName(...)) and findById()->is(find()) returned false for the same rows. The registrar now names the prototype like Eloquent hydration's Connection::getWritableName(): the model's connection, or the model resolver's default when null or empty, with a read alias reduced to its base name and a write alias kept. It parses the name instead of resolving a connection, which would take a pooled connection on warm checks. The wildcard docs said a permission must exist before it can be checked; only the assigned wildcard permission needs a record. GuardTest gains its data provider's title docblock. Validation: the changed test files pass on the array, database and Redis cache stores; the Permission suite passes on the array store, with only known later-slice failures on the database store; the Postgres Permission tests pass; formatting and static analysis are clean.
With wildcard permissions enabled, denies matched only the exact checked name: a denied posts.* did not block an allowed posts.create, and a denied articles.edit did not block articles.edit.123 granted by articles.*. Denies are a Hypervel addition to spatie/laravel-permission (main at 6615eefac655, 8.x). The Wildcard contract adds getDeniedIndex() beside getIndex(). WildcardPermission indexes getAllPermissions() and the new public getDeniedPermissions() through one shared loop. getDeniedPermissions() reads the same cached direct and via-role collections and joins them with concat(): a loaded permissions relation or custom pivot supplies an Eloquent collection, whose merge() would replace a direct deny with a role allow of the same key. HasPermissions::hasPermissionTo() and Role::hasPermissionTo() now share hasWildcardPermission(), as upstream's wildcard branches do. It normalizes the checked value once, keeps the partition check for permission objects, returns false when the configured wildcard class matches the denied index, then checks the allowed index. hasDeniedPermission() and hasDeniedPermissionViaRoles() stay exact. The registrar stores both indexes in the existing context entry, so every wildcard index invalidation clears them together. WildcardPermission::buildIndex() also built each segment without subparts twice, in a separate branch and again in the subpart loop, so work doubled with each segment (31 calls instead of 5 for a four-segment name). Upstream has the same code. Every segment now goes through the loop once; its blank-subpart check covers the removed blank-segment check. The README records the contract addition and getDeniedPermissions(); the user guide documents getDeniedPermissions(), wildcard deny matching and custom wildcard classes. Validation: the changed test files pass on the array, database and Redis cache stores; the Permission suite passes on the array store, with only known later-slice failures on the database store; the Postgres Permission tests pass; formatting and static analysis are clean.
Assess the configuration, contracts, exceptions, helpers, Guard, migration and stubs, events and commands against spatie/laravel-permission main at 6615eefac655. - permission:show rendered a role's denied permissions as allowed, because it plucked permission ids only (upstream has no denies). It now reads each eager-loaded pivot's is_denied flag and shows allowed, denied and unassigned cells, with no extra queries. The user guide explains the symbols. - The show-by-teams test uses non-sequential team ids, so upstream's positional team headers would fail it. - UnauthorizedException::missingTraitHasRoles() takes the user types the middleware pass instead of object. - DefaultTeamResolver is no longer final, and its flushState() and subscriber call are removed: the subscriber's CoroutineContext flush already clears the team. - SchemaConfigTest's omitted-pivot-key case uses the array store for the permission cache. It switches the default connection to a fresh database, which a database cache store followed when the migration cleared the role cache. - Remove PublicApiTest and SchemaConfigTest cases covered elsewhere or that only asserted removed names; BladeTest covers the hasexactroles directive. PackageMetadataTest compares every external requirement with the root package. - Method title docblocks, restored upstream comments (the AssignRole one now names what it checks), the teams config comment, and README and user-guide corrections. Validation: Permission suite on the array store; database-store run with only the known cache and partition query-count failures; affected files on Redis; composer lint and composer analyse.
Assess the registrar's cache and catalog machinery against spatie/laravel-permission main at 6615eefac655 (upstream has no equivalent; its CacheTest was reconciled earlier). - Package relations built a Permission model and resolved a connection for every hydrated pivot, because newPivot() asks for the pivot connection per pivot. The connection is memoized per relation, so package relation loads cost the same as plain Eloquent. - Catalog hydration clones one prepared pivot instead of building one per role-permission link, and builds its indexes in one pass. - Catalog and via-role pivots no longer point back at the models that hold them. Those references made every request's catalog cyclic, so only the garbage collector could free it, about half the per-request cost. - The cached payload stores each permission's role keys and denied role keys instead of pivot rows (129 to 41 KB at 1,000 links). - Per-request catalog lookup after a cache hit drops from about 18 to 3 ms at 1,000 links and from 186 to 26 ms at 10,000. - Remove the unused forgetModel*Cache* invalidators. They invalidated immediately, so a concurrent request could refill from pre-commit rows inside a transaction; forgetCachedPermissions() is the documented reset. - The Permission TestCase isolates a Redis permission cache per ParaTest worker before the migrations clear it, and redis.yml runs tests/Permission with the Redis store. - CacheTest covers denied role keys in the payload, a shared-store hit from a new coroutine, freeing catalog models without the cycle collector, and the database store's forget() result. The Postgres create-race test covers Role::create() too. Validation: Permission suite under ParaTest on the array, file and Redis stores; database store with only the known partition query-count failures; Postgres integration tests; composer lint and composer analyse.
Assess the HasPermissions and HasRoles internals against spatie/laravel-permission main at 6615eefac655 (upstream has no equivalent for the queued assignments, provenance or deny machinery; its event cases were reconciled earlier). - Cached direct-permission pivots took the subject's connection, so saving or deleting one wrote through the subject database while live relation pivots use permission storage. They now take the permission's connection name, and they no longer point back at their permission, which made the memoized collection cyclic garbage. - hasRole() looked up integer- and UUID-valued enums as role keys, so a user holding the role with key 7 passed a check for an enum valued 7. Enums compare with role names, as upstream's enum branch does, using a strict string comparison. hasAllRoles() no longer converts a single enum before passing it to hasRole(), which sent a UUID value to the key lookup. - The permission queue is keyed by context and permission like the role queue, so a later queued effect replaces an earlier one; the collapse pass, five helpers and the flush-time team recheck go. - syncPermissions() calls syncPermissionEffects(); assignment cache invalidation and the assignment-context builders each have one helper instead of several copies. - hasDirectPermission() and Role::hasPermissionTo() look up the single assignment for a permission (the pivot primary keys allow one row per permission and subject), and Role follows upstream's check order. - Remove requireDeletionModelKey() (Eloquent rejects a keyless delete before any query), value checks the native types already enforce, and instanceof checks that only narrowed types; two of them silently skipped partition checks. - Type assignment, scope and role inputs with their documented unions; the checks upstream tests call with null, objects or arrays keep mixed. - Tests: regressions for the pivot connection, enum role names and direct-permission lifetime; remove cases for impossible duplicate effects, Eloquent's key check and upstream-covered events; assert single dispatches and complete sync results. Validation: Permission suite under ParaTest on the array, file and Redis stores; database store with only the known partition query-count failures; Postgres integration tests; php-cs-fixer and composer analyse.
…base store Assess the partition machinery and its tests against spatie/laravel-permission main at 6615eefac655 (upstream has no partition support or tests). - Remove the provider's PermissionRegistrar singleton closure; the class is autowirable and auto-singletoned. flushState() no longer forgets the container instance: the test subscriber has already replaced the container, so the lookup only created an empty one. - resolvePartition() no longer wraps a non-scalar resolver result in an UnexpectedValueException; PermissionPartition's native types reject it. partitionFromRecord() no longer rejects an empty stored partition, which only raw SQL can write. - Partition relations marked eager-loaded collections in both initRelation() and match(). Builder::eagerLoadRelation() always passes the first's result to the second, so match() alone marks every model, including those without results. - Replace type-only instanceof checks missed in the previous slice with null checks in the registrar's settlement tokens, coroutine memos and key index, and in hasDeniedPermissionViaRoles(). - Tests: a sync's detached event payloads exclude the subject's assignments in another partition; an empty eager-loaded relation is marked current; exact role-assignment and removal query counts. Remove absence checks, cases built from states only raw SQL creates, cases reading back the tests' own schema, constructor reflection and duplicates. Use non-deprecated exception message expectations. - The partition query-count cases use the array permission cache, since a database store logs its statements between the counted queries. CI runs the Permission suite with the database cache store. Validation: Permission suite under ParaTest on the array, file, database and Redis stores; Postgres integration tests; the partition database test on SQLite and Postgres; php-cs-fixer and composer analyse.
Compare every spatie/laravel-permission docs page at main 6615eefac655 with the user guide, checking each claim against current source. - Port the applicable upstream coverage: authorizable user models and reserved trait names, default guards and guard resolution, permission-side role methods, ID and enum lookups, super-admin options, the can and package middleware with aliases, guards, pipes, priority and controller middleware, Blade directives, command arguments, team middleware and team roles, wildcard syntax, model extension, pivot timestamps, seeding, test seeding with the Seeder attribute, best practices and a policy example that always returns. - Correct stale or wrong guidance: sync query counts, UUID migration changes, global role uniqueness, reloading relations after switching teams, separate-connection limits and the default_model fallback. - Not ported: upstream's example app, PhpStorm, UI and upgrade pages, MySQL key-length notes and an attribute example with a static call, which is invalid PHP. The guide's differences list moves to the README, which gains the cache store's lock requirement and the worker-wide cache with partitioning for tenants. - Bulk allow/deny updates skipped updated_at on permission relations using withTimestamps(); they now set it like updateExistingPivot(). - The create migration and the teams stub forgot only the catalog key, while upstream's single key is its whole cache. Per-model assignment caches therefore survived recreated tables on shared stores: a model whose key was reused read the old role keys, and after enabling teams a check without a team read pre-teams assignments. Both also forget the assignment token now, so the guide's opening seeder reset goes. - The sync note maps the catalog and assignment caches, the relation builders and the diff-based syncs. A " #" in it had started a YAML comment that cut off the rest of the note. Validation: Permission suite under ParaTest on the array, database and Redis stores; Postgres integration tests; the migration regressions fail on the database and Redis stores without the fix; php-cs-fixer and composer analyse.
Set spatie/laravel-permission main 6615eefac655 as the checked-through revision after the full-package reconciliation of its tests, source and documentation. The assessment examined no pull requests, so its last reviewed PR stays unset.
The teams stub read permission.teams and permission.table_names through typed getters before its "config not loaded" check, so missing config failed with a generic typed-getter error, and the check itself only caught an empty array. Upstream's stub and our create migration show the message that tells users to clear the config cache. The stub now checks table_names first, like the create migration. Validation: PermissionServiceProviderTest runs the missing-config case for both migrations (the stub case failed before the fix); SchemaConfig, Cache (array and database stores) and command tests; php-cs-fixer.
|
Important Review skippedAuto reviews are disabled on this repository. Please check the settings in the CodeRabbit UI or the ⚙️ Run configuration
You can disable this status message by setting the Use the checkbox below for a quick retry:
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
PR Summary by QodoReconcile Permission with Spatie and fix authorization and cache defects
AI Description
Diagram
High-Level Assessment
Files changed (92)
|
 |
![greptile-apps[bot]](https://avatars.githubusercontent.com/in/867647?s=60&v=4){kind=small}
Comment on lines
1942
to
+1945
| $roles = $this->getHydratedPermissionRoleCollection( | ||
| (array) $item['roles'], | ||
| $permission, | ||
| (array) $item['roles'], | ||
| (array) $item['denied_roles'], |
There was a problem hiding this comment.
If a deployment keeps entries in a persistent cache store, an existing catalog still has pivot-row arrays in roles and no denied_roles field. This code reads it under the unchanged cache key as though roles contained IDs. Permission checks can then warn and lose role grants until the entry expires or is cleared. Version the catalog key or rebuild entries in the old format.
Knowledge Base Used: Cache, filesystem, and Redis storage
Member
Author
There was a problem hiding this comment.
Hypervel 0.4 hasn't been released, and we don't keep compatibility with earlier 0.4 builds, so no persistent store holds a catalog in the old format. Versioning the key or rebuilding old entries would add code that only serves pre-release builds. No change here.
There was a problem hiding this comment.
That’s fair. Given Hypervel 0.4 is unreleased and compatibility with earlier 0.4 builds is explicitly out of scope, a persistent cache cannot contain the old catalog format under the supported upgrade path. I’m withdrawing this finding; no key versioning or legacy rebuild is needed.
Code Review by Qodo
1. Permission checks repeatedly rebuild caches
|
![qodo-free-for-open-source-projects[bot]](https://avatars.githubusercontent.com/in/1068000?s=60&v=4){kind=small}
Comment on lines
+1883
to
+1887
| // Role keys instead of pivot rows keep the payload small; hydration rebuilds the pivots. | ||
| return [ | ||
| 'attributes' => Arr::except($permission->getAttributes(), $except), | ||
| 'roles' => $roles, | ||
| 'roles' => $roleKeys, | ||
| 'denied_roles' => $deniedRoleKeys, |
There was a problem hiding this comment.
2. Permission checks break on catalogs cached before the upgrade 🐞 Bug ☼ Reliability
getSerializedPermissionsForCache() now stores scalar role keys and a denied_roles field under the unchanged, unversioned catalog cache key, but getHydratedPermissionCollection() assumes every cached entry has that new shape. A pre-upgrade entry can remain in the shared cache for up to 24 hours and cause warnings or lost role-granted permissions when a new worker reads it; during a rolling deploy, an old worker reading a new entry can instead throw a TypeError.
Agent Prompt
## Issue description
The catalog payload changed from pivot-bearing role arrays to scalar role keys plus `denied_roles`, but it remains under the same unversioned shared cache key. Pre-upgrade entries and entries written by old workers during a rolling deploy can therefore reach an incompatible hydrator, causing warnings, TypeErrors, or lost role-granted permissions until the entry expires or is cleared.
## Fix Focus Areas
- src/permission/src/PermissionRegistrar.php[1323-1331]
- src/permission/src/PermissionRegistrar.php[1869-1888]
- src/permission/src/PermissionRegistrar.php[1939-1950]
## Recommended Fix
Version the serialized catalog format, for example with a `version` field or a format suffix such as `:v2` on the catalog key in `getCacheKey()`. Before hydration in `permissionCatalog()`, treat an incompatible version or shape—including missing `denied_roles` or non-scalar role entries—as a cache miss and rebuild and overwrite the entry using `getSerializedPermissionsForCache()`; alternatively, detect and convert old-format role links. Add a regression test that reads an old-format cached payload and an upgrade note to the Permission docs.
ⓘ Copy this prompt and use it to remediate the issue with your preferred AI generation tools
Member
Author
There was a problem hiding this comment.
Hypervel 0.4 hasn't been released, and we don't keep compatibility with earlier 0.4 builds, so there's no older cached format or older worker to handle during a deploy. No change here.
| { | ||
| $this->mergeConfigFrom(__DIR__ . '/../config/permission.php', 'permission'); | ||
|
|
||
| $this->app->singleton(PermissionRegistrar::class, fn ($app) => new PermissionRegistrar( |
There was a problem hiding this comment.
1. Permission checks repeatedly rebuild caches 🐞 Bug ➹ Performance
Removing the singleton binding makes each resolution of PermissionRegistrar construct a new instance, whose initializeCache() clears all permission runtime state. Permission operations resolve the registrar independently, so checks in the same request repeatedly discard their hydrated catalog and wildcard indexes and reload them.
Agent Prompt
## Issue description
Removing the registrar singleton causes each independent resolution to initialize a new registrar and clear coroutine-local permission caches.
## Fix Focus Areas
- src/permission/src/PermissionServiceProvider.php[37-40]
- src/permission/src/PermissionRegistrar.php[132-142]
## Recommended Fix
Restore a singleton binding for PermissionRegistrar using its current constructor dependencies. Keep cache-manager resolution inside initializeCache() so reinitialization can pick up a rebound manager.
ⓘ Copy this prompt and use it to remediate the issue with your preferred AI generation tools
Member
Author
There was a problem hiding this comment.
Hypervel's container shares unbound concrete classes for the worker's lifetime: the first make() caches the instance, and later resolutions return it. PermissionRegistrar is still built once per worker, so the closure binding only repeated what the container already does. No change here.
binaryfire
changed the title
The teams config comment said to enable teams before migrating or to run permission:setup-teams, which read as if the command works without enabling teams. It refuses to run until teams are enabled, so the comment now says to enable teams first in both cases. The partitioning guide explains how to reset every partition but not when that matters. The create migration clears only the unpartitioned cache keys, so a custom migration that recreates partitioned tables must reset each affected partition. Otherwise cached assignments from the old tables apply to new records that reuse their keys. A per-partition reset rotates that partition's assignment token, so it clears those entries. The enum example now imports the Role and Permission models it uses.
testItFiresDetachEventWhenSyncingPermissions faked events before the initial grant, so the grant's own attached event satisfied the attached assertion, which checks the model's permissions only when it runs. The test passed even if the sync dispatched no attached event. The grant now runs before Event::fake(), so only the sync's event can match. Spatie's test has the same order; a comment keeps the change from being undone. PackageMetadataTest compared every non-Hypervel requirement with the root package, but dropping composer-runtime-api, nesbot/carbon or symfony/http-kernel from the Permission package would still have passed. It now asserts those three requirements are present. PolicyTest's comments said "view" where the assertions check "update". Validation: HasPermissionsTest and its two inherited variants (custom models, teams), PackageMetadataTest and PolicyTest; php-cs-fixer.
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
This fixes authorization and caching bugs in the Permission package and makes reading permissions from the cache several times cheaper.
The main fixes: with wildcard permissions enabled, a denied pattern like
posts.*did not block an allowedposts.create.hasRole()matched integer- and UUID-backed enums against role keys instead of role names. And after a migration recreated the permission tables, assignments cached on a shared store could apply to new models that reused old keys. Reading the role catalog from the cache now takes about 3 ms instead of 18 ms at 1,000 role-permission links.One shared class changes too: the model cache coordinator, which Auth, Sanctum and Permission use, now rechecks the real cache store after taking its fill lock. Auth and Sanctum behave as before.
Denied permissions and wildcards
posts.*did not block an allowedposts.create, and a deniedarticles.editdid not blockarticles.edit.123granted byarticles.*. Denies now go through the same wildcard matching as allows. TheWildcardcontract gainsgetDeniedIndex()besidegetIndex(), and models and roles gaingetDeniedPermissions().WildcardPermission::buildIndex()built each segment without subparts twice, so the work doubled with every segment: 31 calls instead of 5 for a four-segment name. Spatie has the same code. Each segment is now built once.permission:showshowed a role's denied permissions as allowed. It now shows allowed, denied and unassigned cells, with no extra queries.withTimestamps()did not setupdated_at. It now does, likeupdateExistingPivot().Role and permission checks
hasRole()treated integer- and UUID-backed enums as role keys, so a user holding the role with key 7 passed a check for an enum whose value was 7. Enums now compare with role names, as in Spatie, using a strict comparison.hasAnyRole()andhasAllRoles()follow.Model::is()compares connection names, so$user->roles->contains(Role::findByName('admin'))andRole::findById($id)->is(Role::find($id))returned false for the same row. Spatie has the same problem. Catalog models now carry the connection name Eloquent would give them, without taking a database connection.setPermissionClass(),setRoleClass()andsetTeamClass()now write the config like Spatie's. Reinitializing the cache had reverted the class while the container binding kept the new one.aboutcommand always listed denied permissions as an enabled feature, so it never showedDefault. It now uses Spatie's feature labels.Caching and performance
Repository::flexible()does, and its result replaces the request's memoized entry. Auth and Sanctum pass plain repositories, so nothing changes for them.Permissionmodel and resolved a database connection for every loaded pivot. The connection is now resolved once per relation, so loading package relations costs the same as plain Eloquent.forgetModel*Cache*methods are removed. They cleared the cache immediately, even inside a transaction, so a concurrent request could refill it from rows that were not committed yet.forgetCachedPermissions()is still the reset for raw writes.Partitioning
Row partitioning works as before, with less code around it. Eager-loaded partition relations are marked current once, in
match(), instead of twice. A resolver that returns a non-scalar partition fails with PHP's ownTypeErrorfromPermissionPartitioninstead of a wrapped exception. New tests check that a sync's detached events leave out the user's assignments in other partitions, and that an eager-loaded relation with no results is still marked current. The guide now says to reset each affected partition's cache when a custom migration recreates partitioned tables.Simplification and API consistency
null, objects or arrays keepmixed, so they still throw Spatie's exceptions.PermissionRegistraris no longer bound with a closure, since the container already shares it.DefaultTeamResolveris no longerfinal, so it can be extended as in Spatie.UnauthorizedException::missingTraitHasRoles()takes the user types the middleware actually pass instead ofobject.Tests
Spatie's current test suite now runs as part of the Permission tests, converted from Pest to PHPUnit, and most of the bugs above came up while merging it in. Spatie's tests keep their upstream names, order, datasets and file placement. Duplicate Hypervel cases are folded into them, keeping the stronger Hypervel assertions, and Hypervel-only coverage for denies, partitions, coroutine isolation and query counts stays. Cases that only tested Eloquent itself, states that only raw SQL can create, or the test schema are removed. Spatie's Octane listener tests are not ported, because the current team and the loaded catalog are already per coroutine.
The suite picks its cache store from
CACHE_STORE, like Spatie'sCACHE_DRIVER, and gives each parallel worker its own Redis database. Bugs where one request reads another's cached data only show up on a shared store, so CI now also runs the Permission suite with the database cache store intests.ymland with Redis inredis.yml.Intentional differences
The package now follows the current
spatie/laravel-permissionexcept where noted here. The package README lists the remaining differences from Spatie and why: denied permissions and the wildcard contract'sgetDeniedIndex(), unit enum inputs, row partitioning, the cache configuration and its lock requirement, the worker-wide cache, and the missing Octane listener. Two smaller adaptations are also kept. An undefined cache store throws instead of quietly falling back to the array store, andpermission:setup-teamsfails when it cannot write the migration.Documentation
The Permission guide now covers Spatie's documentation where it applies: guards, direct and role permissions, super-admins, enums, middleware, Blade directives, commands, teams, wildcards, custom models, seeding and testing. Stale guidance is corrected, including sync query counts, UUID migrations, global roles with teams and separate database connections.
docs/upstream-sync/sync.yamlrecords the checked Spatie revision and how Hypervel's cache and relation code maps onto Spatie's.Verification
The Permission suite passes with the array, database and Redis cache stores; it runs on SQLite. The Postgres Permission tests pass, and the partition database test passes on SQLite and Postgres. PHPStan, formatting and the full parallel suite pass.
Summary by cubic
Aligns Hypervel's Permission package with
spatie/laravel-permissionat 6615eefac655 (8.x), merging Spatie's Pest test suite into the existing PHPUnit coverage with regression tests for every defect exposed.posts.*blocks an allowedposts.create.WildcardPermission::buildIndex()builds each segment once; permission checks use a single shared wildcard path.hasRole()compares enums with role names instead of treating integer- and UUID-backed enum values as role keys.setPermissionClass(),setRoleClass()andsetTeamClass()write the config like Spatie's, and the registrar resolves the cache manager each time it initializes the cache.permission:showdisplays allowed, denied and unassigned cells without extra queries, and theaboutcommand uses Spatie's feature labels.Caching and performance
forgetModel*Cache*methods are removed.Partitioning and simplification
match()instead of twice, and a non-scalar partition fails with PHP's ownTypeError.PermissionRegistraris no longer bound with a closure, andDefaultTeamResolveris no longerfinal.CI now runs the Permission suite with the database and Redis cache stores to surface shared-store cache bugs. The Permission guide covers Spatie's applicable docs, including the teams setup comment and per-partition cache resets, and the README lists the remaining intentional differences.
Tests
Event::fake()after the initial grant so only the sync's event satisfies the assertion.PackageMetadataTestasserts the three required non-Hypervel dependencies, andPolicyTestcomments read correctly.Written for commit 25cac72. Summary will update on new commits.
Note
Improve permission authorization and cache performance with denied-permission and wildcard-deny support
PermissionRegistrarand theHasPermissions/HasRolestraits to store role keys plus denied-role key lists in the cached catalog, instead of full pivot rows. Hydrated pivots are cloned from a shared prototype and use the permission model's connection.WildcardPermission::getDeniedIndex()is checked before the allowed index, so denied wildcard permissions suppress matching direct and role-derived permissions. A newgetDeniedPermissions()accessor returns direct and role-derived denies, de-duplicated.fillnow locks on the underlyingStore, re-reads authoritative storage past a memoized miss, and only memoizes successful primary-cache writes.enum_valuecallable, and permission/role APIs gain native union types.PermissionRegistrarremoves the public model-assignment cache invalidation methods, andDefaultTeamResolver::flushState()andTEAM_ID_CONTEXT_KEYare no longer public — external callers of these APIs must update.Macroscope summarized 25cac72.