Apply can directive action when the field resolves to a promise - #2786
Open
dxiiren wants to merge 1 commit into
Open
Apply can directive action when the field resolves to a promise#2786dxiiren wants to merge 1 commit into
dxiiren wants to merge 1 commit into
Conversation
BaseCanDirective handles an authorization failure according to the action argument, but only catches what is thrown synchronously. A batch loaded relation resolves to a SyncPromise, so CanResolvedDirective authorizes inside its then() callback, which runs after the try-catch has been left. The failure escaped, so RETURN_VALUE and EXCEPTION_NOT_AUTHORIZED were silently ignored whenever batchload_relations was enabled. Chain the same handling onto the promise rejection so the action applies regardless of whether the field resolves eagerly or through a batch loader.
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
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.
Resolves #2758
Changes
BaseCanDirectivedecides what to do with an authorization failure based on theactionargument, but it only sees failures thrown synchronously:
CanResolvedDirectiveauthorizes throughResolved::handle(), which chains onto a promisewhen it gets one:
With
batchload_relationsenabled a relation resolves to aSyncPromise, so$handleruns when that promise is fulfilled, which is after the try-catch above has been left. The
AuthorizationExceptionescapes it, andactionis ignored:RETURN_VALUEnever returnsits value and
EXCEPTION_NOT_AUTHORIZEDnever rewrites the message. Turningbatchload_relationsoff makes the same schema behave as documented, which is what makesthis surprising.
The
catchbody moves intohandleAuthorizationFailure()and is also chained onto thepromise as a rejection handler, so the
actionapplies whether the field resolves eagerlyor through a batch loader.
The added test covers a batch loaded
@hasManyguarded by@canResolved(action: RETURN_VALUE, returnValue: null). On currentmasterthe fieldcomes back
nullbut anThis action is unauthorized.error is reported alongside it,which is exactly the ignored
action. With this change the field isnulland no error isreported.
Verified on PHP 8.3.33 / Laravel 13.23.0 against MySQL and Redis, using the repository's
own Docker setup. All 90 tests across
CanDirective,CanFindDirective,CanModelDirective,CanQueryDirective,CanResolvedDirectiveandCanRootDirectivepass.Breaking changes
None for schemas that behaved correctly already. Fields that previously leaked an
authorization error despite
action: RETURN_VALUEnow return the configured value, whichis the documented behavior and the point of the fix.