Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Cannot save the user model twice in the same request #2638

Open
Moundlen opened this issue Mar 17, 2024 · 5 comments · May be fixed by #2658
Open

Cannot save the user model twice in the same request #2638

Moundlen opened this issue Mar 17, 2024 · 5 comments · May be fixed by #2658

Comments

@Moundlen
Copy link

Moundlen commented Mar 17, 2024

Describe the bug
Hi,
Sorry if this has already been asked but I'm using Laravel fortify to register new users, once user is registered I automatically log him in but I have a listener that listens for Login and automatically edits the user IP and last login and saves the user again. When the user is saved again I get an error Duplicate entry '7-153-User' for key 'model_has_roles.PRIMARY'.
I fixed the issue by saving quietly but is this expected behavior ?
Now everytime I save a user in the same lifetime cycle it just crashes.
It seems to come from the use of attach instead of sync in the HasRoles trait line 168. see #2420
Did not have this issue before because I migrated from v4 to v6.

Versions

  • spatie/laravel-permission package version: 6.4.0
  • laravel/framework package: 10.48.3

PHP version: 8.2.16

Database version: mysql 8.0.32

To Reproduce

  • Create a new User
  • Sync or Assign role to the user
  • Save the User
  • Save the User again

My listener:

public function login(Login $event): void
    {
        try {
            /** @var User $user */
            $user = $event->user;
            activity('Auth')->by($user)
                ->withProperty('IP', request()->getClientIp())
                ->log('Login');

            $user->last_login = Carbon::now();
            $user->last_ip = request()->getClientIp();
            $user->saveQuietly();
        } catch (\Exception $exception) {
            report($exception);
        }
    }

Expected behavior
I would expect the save to not throw an error if I save the User twice.

Environment (please complete the following information, because it helps us investigate better):

  • OS: ubuntu
  • Version: 22.04
@Moundlen Moundlen changed the title Duplicate entry '7-153-User' for key 'model_has_roles.PRIMARY' Cannot save the user model twice in the same request Apr 3, 2024
@drbyte
Copy link
Collaborator

drbyte commented Apr 19, 2024

  1. How are you registering roles to the model? It would be helpful to see what your code is for that, since it should have already finished persisting things and letting go of what was specified to be attached. Seeing your code would help.
  2. In your listener, perhaps you can call $user->fresh() to have a clean unique instance before updating its properties and saving?

@Moundlen
Copy link
Author

Moundlen commented Apr 19, 2024

Hi, here is a simple code to reproduce created a new empty laravel 11 project.

<?php

use Illuminate\Support\Facades\Route;
use Spatie\Permission\Models\Role;
use Spatie\Permission\Models\Permission;

Route::get('/', function () {
    $user = new \App\Models\User([
        'name' => \Illuminate\Support\Str::random(),
        'password' => \Illuminate\Support\Str::random(),
        'email' => \Illuminate\Support\Str::random() . '@example.com',
    ]);
    if (Role::where('name', 'writer')->doesntExist()) {
        $role = Role::create(['name' => 'writer']);
        $permission = Permission::create(['name' => 'edit articles']);
        $role->givePermissionTo($permission);
    }
    $user->assignRole('writer');

    $user->save();
    $user = $user->fresh();
    $user->save();
});

@drbyte
Copy link
Collaborator

drbyte commented Apr 19, 2024

Thanks for the code sample.

Yes, if you're creating a User with new, then it is not yet persisted into the database, and attached pivots are getting duplicated when calling save().

However, if you first ->save() that unpersisted User before calling assignRole()/etc, subsequent saves will not be problematic.

Also, if you use User::create() instead of new User, none of these problems occur.

@drbyte
Copy link
Collaborator

drbyte commented Apr 19, 2024

@erikn69 I'm interested in your thoughts about how to avoid these "Duplicate entry" errors when attaching to an un-persisted model.

Ref: #2419 #2420 #2574
Ref: https://github.com/laravel/framework/blob/master/src/Illuminate/Database/Eloquent/Relations/Concerns/InteractsWithPivotTable.php

@erikn69
Copy link
Contributor

erikn69 commented Apr 19, 2024

I think it is a problem that has always been present.
A validation could be added,
I would have to do tests, I think that adding wasRecentlyCreated would solve it

$class::saved(
function ($object) use ($permissions, $model, $teamPivot) {
if ($model->getKey() != $object->getKey()) {
return;
}
$model->permissions()->attach($permissions, $teamPivot);
$model->unsetRelation('permissions');
}
);
}

@erikn69 erikn69 linked a pull request Apr 19, 2024 that will close this issue
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 a pull request may close this issue.

3 participants