Lazy Loading

[ashokbaruaakas]

Description

The current version of spatie/laravel-permission is raising lazy loading exceptions in Laravel. This occurs when $role->givePermissionTo("Do Something"). I didn't check for others functions.

Steps To Reproduce

1. Add Lazy Loading Prevention to AppServiceProvider.php Model::preventLazyLoading();
2. Create Permission Permission::firstOrCreate(["name" => "Can Do Something"])
3. Create Role Role::firstOrCreate(["name" => "ARole"]);
4. Get Roles $roles = Role::where("name", "ARole")->get();
5. Find first Role $role = $roles->firstWhere("name", "ARole");
6. Give Permission $role->givePermissionTo("Can Do Something");

Example Application

No response

Version of spatie/laravel-permission package:

6.10.1

Version of laravel/framework package:

11.34.2

PHP version:

8.3.14

Database engine and version:

No response

OS: Windows/Mac/Linux version:

No response

[jose123v]

Hi, Feel free to make a PR with the fix

I did run this test, and it is working as expected

/** @test */
public function it_can_be_given_a_permission_when_prevent_lazy_loading()
{
    Model::preventLazyLoading();

    $testUserRole = app(Role::class)->where('name', 'testRole')->first();

    $testUserRole->givePermissionTo('edit-articles');

    $this->assertTrue($testUserRole->hasPermissionTo('edit-articles'));
}

[ashokbaruaakas]

This test is more specific about this issue...

/** @test */
public function it_can_be_given_a_permission_when_lazy_loading_is_restricted()
{
    Model::preventLazyLoading();

    try {
        app(Permission::class)->firstOrCreate(['name' => 'edit-articles']);

        app(Role::class)->firstOrCreate(['name' => 'testUserRole']);
        app(Role::class)->firstOrCreate(['name' => 'anotherTestUserRole']);

        $roles = app(Role::class)->whereIn("name", ["testUserRole", "anotherTestUserRole"])->get();
        $testUserRole = $roles->firstWhere("name", "testUserRole");

        $testUserRole->givePermissionTo("edit-articles");

        $this->assertTrue($testUserRole->hasPermissionTo("edit-articles"));
    } catch (Exception $e) {
        $this->fail('Lazy loading detected in the givePermissionTo method: ' . $e->getMessage());
    }
}

[jose123v]

If you are going to do it that way you should use with, try it

-$roles = app(Role::class)->whereIn("name", ["testUserRole", "anotherTestUserRole"])->get();
+$roles = app(Role::class)->with(['permissions'])->whereIn("name", ["testUserRole", "anotherTestUserRole"])->get();

[drbyte]

The current version of spatie/laravel-permission is raising lazy loading exceptions in Laravel.

Yes, this is actually intentional.

What's your app-specific reason for wanting to prevent lazy loading in your app? Is it related to a specific performance problem? Is it really just for a certain set of models?

[ashokbaruaakas]

Yeah... But...
What I want to say is, for example, I have enabled Model::preventLazyLoading() and my $role is fetched without the permissions relation loaded for some reason. Now, I want to assign a permission to a role. Ideally, the code should look like:
$role->givePermissionTo('permission');

However, I have to write it like this instead:
$role->load('permissions')->givePermissionTo('permission');

Here, the call to ->load('permissions') feels unnecessary because it isn’t directly related to assigning the permission.

Maybe we could handle this by using $this->loadMissing('permissions') inside the givePermissionTo function to avoid the need for explicit loading in such cases or may be something else.

Note: I don’t have a deep understanding of the internals of spatie/laravel-permissions; I’m just speaking from my imagination. If I’ve misunderstood something, please feel free to correct me.

@jose123v @drbyte

[drbyte]

Yes, I understand your point.

I'm also aware that over the years of this package's evolution (since long before Model::preventLazyLoading() existed, and other Laravel improvements that have come over time) we've gone back and forth with explicitly loading, lazy-loading, using loadMissing(), and unsetting, etc. And every time we make the change to accommodate someone's report of the proposed change either improving performance (fewer queries) or fixing intermittent in-memory issues (due to outdated loaded relations), etc etc, it's broken other people's installations or caused ripple-effect bugs. /cc: @erikn69 thoughts?

Nevertheless, of course, as you say, if you enable Model::preventLazyLoading(), it's going to affect everything that lets it be affected. And we'll probably need to do something to address that.

That's why I asked the question: Why are you explicitly choosing to add Model::preventLazyLoading() to your app? What's your purpose behind that?

[ashokbaruaakas]

That's why I asked the question: Why are you explicitly choosing to add Model::preventLazyLoading() to your app? What's your purpose behind that?

Without restricting lazy loading, I feel like I’m knowingly allowing something potentially harmful to happen that could slow down my app in the future. By enabling Model::preventLazyLoading(), I can proactively catch and address these issues, ensuring better performance and more efficient code.

[erikn69]

Hi, at the moment you could use handleLazyLoadingViolationUsing, I haven't tried it but it should work

use Illuminate\Database\Eloquent\Model;
use Illuminate\Database\LazyLoadingViolationException;
use Spatie\Permission\Models\Permission;
use Spatie\Permission\Models\Role;

public function boot()
{
    Model::preventLazyLoading();
    Model::handleLazyLoadingViolationUsing(function (Model $model, string $relation) {
        if (in_array(get_class($model), [Permission::class, Role::class])) {
        // or //  if (in_array($relation, ['roles', 'permissions'])) {
            return;
        }

        throw new LazyLoadingViolationException($model, $relation);
    });
}

I have no experience with preventLazyLoading, I wouldn't know how to fix it

Maybe we could handle this by using $this->loadMissing('permissions') inside the givePermissionTo function to avoid the need for explicit loading in such cases

You could also try, if it works, make a PR

[erikn69]

I don't know how you get that problem, I'm testing and I don't have any code that does that, when editing the role permissions I bring the specific role and assign its permissions (/roles/{role_id})

[ashokbaruaakas]

I don't know how you get that problem, I'm testing and I don't have any code that does that, when editing the role permissions I bring the specific role and assign its permissions (/roles/{role_id})

Follow the description please...

[erikn69]

4. Get Roles $roles = Role::where("name", "ARole")->get();
5. Find first Role $role = $roles->firstWhere("name", "ARole");
6. Give Permission $role->givePermissionTo("Can Do Something");

in step 4 you must add ->with('permissions'), As I understood from the docs, preventLazyLoading allows us to find that problem, and add with, so that it does not happen

if something is added to allow a lazy load in a $roles item(first,firstWhere,filter...), would the package be prone to N+1 problems?

I mean

$roles = Role::where("name", "ARole")->get(); //->with('permissions') fix it
$roles->each/*->filter,->firstWhere,....*/(function ($role) {
    $role->givePermissionTo("Can Do Something"); //would run a select query for every role here, N+1??
});

but I did find a problem on this, but different from the one mentioned
I did add 30649f5 on #2776

@drbyte thoughts? N+1 must be allowed, or should with be used?