Skip to content

Make role model sync atomic - #2968

Open
amrachraf6699 wants to merge 1 commit into
spatie:mainfrom
amrachraf6699:fix/atomic-role-model-sync
Open

Make role model sync atomic#2968
amrachraf6699 wants to merge 1 commit into
spatie:mainfrom
amrachraf6699:fix/atomic-role-model-sync

Conversation

@amrachraf6699

Copy link
Copy Markdown

Summary

Makes Role::syncModels() atomic so a failed attachment cannot remove existing role-to-model assignments.

Problem

syncModels() deleted all existing pivot records before attaching the replacement models.

When an unsaved model was supplied, the pivot insert failed because the model key was null. The exception was thrown, but the prior assignments had already been deleted.

Fix

Wrap the delete-and-attach sequence in a database transaction.

The original QueryException still propagates, but the database rolls back the pivot deletion when attachment fails.

Test

Added a regression test covering an unsaved target model:

expect(fn () => $role->syncModels($unsavedUser))
    ->toThrow(QueryException::class);

expect($assignedUser->fresh()->hasRole($role))
    ->toBeTrue();
vendor/bin/pest tests/Traits/HasAssignedModelsTest.php --filter="does not detach models when syncing an unsaved model"

Passed.

@drbyte
drbyte marked this pull request as draft August 14, 2026 05:09
@amrachraf6699
amrachraf6699 marked this pull request as ready for review August 23, 2026 21:43
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant