N+1 problem with BelongsTo field on soft deleted relation

[milewski]

• Laravel Version: 8.x
• Nova Version: 3.27.0
• PHP Version: 8.0.1

Description:

Current resolve method on the \Laravel\Nova\Fields\BelongsTo class queries database again if the relationship is null (line 9-11 ) causing an extra query to be executed for each entry...

1 public function resolve($resource, $attribute = null)
2 {
3     $value = null;
4 
5     if ($resource->relationLoaded($this->attribute)) {
6        $value = $resource->getRelation($this->attribute);
7     }
8 
9     if (! $value) {
10        $value = $resource->{$this->attribute}()->withoutGlobalScopes()->getResults();
11    }
12
13    if ($value) {
14        $resource = new $this->resourceClass($value);
15
16        $this->belongsToId = Util::safeInt($value->getKey());
17
18        $this->value = $this->formatDisplayValue($resource);
19
20        $this->viewable = $this->viewable
21            && $resource->authorizedToView(request());
22    }
23 }

Correct implementation would be replace lines 5-11 with:

if ($resource->relationLoaded($this->attribute)) {
    $value = $resource->getRelation($this->attribute);
} else {
    $value = $resource->{$this->attribute}()->withoutGlobalScopes()->getResults();
}

[crynobone]

Can you provide an example with database factory where I can replicate this on our database tests. We run all our tests using Model::preventLazyLoading(true) and none of our extensive tests capture this scenario/use case.

[milewski]

Well, it's pretty easy to reproduce... all you need is something like this:

A Nova resource:

class Customer extends Resource
{
    public static string $model = CustomerModel::class;

    public static $with = [ 'phone' ];

    public function fields(Request $request): array
    {
        return [
            BelongsTo::make('Phone', 'phone'),
        ];
    }
}

A model with a relationship:

class Customer extends Model
{
    public function phone(): HasOne
    {
        return $this->hasOne(Phone::class);
    }
}

And create a bunch of customers WITHOUT any phone, it's important that they all are missing the phone.

So as you can see from this screenshot, it DOES indeed try to eager load all the phones however because they are all NULL belongsTo still perform another query on line BelongsTo.php:162

And if I apply the fix I have proposed:

[crynobone]

I assume you have a typo. Customer belongsTo Phone in resource but model definition Customer hasOne Phone.

[milewski]

I assume you have a typo. Customer belongsTo Phone in resource but model definition Customer hasOne Phone.

That was intentional, I want to use BelongsTo instead of HasOne field because I want to have the link on the index to the resource, hasOne field will hide the field on index and show a table on details ... Which is not desired

[crynobone]

In that case, I don't think we should change it. Added following tests and BelongsTo with proper usage doesn't create n+1.

    public function test_can_fetch_resources_with_nullable_with()
    {
        factory(Post::class)->times(15)->create([
            'user_id' => null,
        ]);

        PostResource::$with = ['user'];

        DB::enableQueryLog();
        DB::flushQueryLog();

        $response = $this->withExceptionHandling()
                        ->getJson('/nova-api/posts');

        $response->assertJsonCount(15, 'resources');
        $this->assertEquals(15, $response->original['total']);

        $queryLogs = ray()->pass(DB::getQueryLog());

        $this->assertCount(3, $queryLogs);
        $this->assertSame('select * from "posts" order by "posts"."id" desc limit 26 offset 0', $queryLogs[0]['query']);
        $this->assertSame('select * from "users" where 0 = 1 and "users"."deleted_at" is null', $queryLogs[1]['query']);
        $this->assertSame('select count(*) as aggregate from "posts"', $queryLogs[2]['query']);

        PostResource::$with = [];
        DB::disableQueryLog();
    }

[milewski]

But if you look at the logic on the current code:

5     if ($resource->relationLoaded($this->attribute)) {
6        $value = $resource->getRelation($this->attribute);
7     }
8 
9     if (! $value) {
10        $value = $resource->{$this->attribute}()->withoutGlobalScopes()->getResults();
11    }

1 - It looks to see if the relationship is loaded... if it is loaded it retrieves the value from it... 2 - If the value retrieved is NULL, it will attempt to query the database again just to retrieve NULL again...

This doesn't seem correct why would we want to call the database again knowing that the relationship is null and will return null...

What do you see breaking if change the logic as I proposed?

[crynobone]

What do you see breaking if change the logic as I proposed?

Honestly, I fear any unknown use case more than what I can see to make the changes on Orion release. Above tests only generate 3 queries and not 15+1+1 as expected if this really happening for BelongsTo relationship and field).

As for your use case. You should create App\Nova\Fields\BelongsToAsHasOne etc and override resolve() if you need such changes.

[milewski]

Well, could you have a discussion internally with your team as well about this? I really don't see how someone would benefit from relying on this bug, my use case is not that abstract, if I remember correctly I have even seen Taylor himself saying that he exchange belongsTo/HasOne sometimes on a Laracast Nova video tutorial, my guess is that most users who use this haven't even realized there is this issue, and there is not much room to have a completely wild use case that would benefit specifically on this extra query being made to the database...

[crynobone]

Well, could you have a discussion internally with your team as well about this? I really don't see how someone would benefit from relying on this bug, my use case is not that abstract,

I'm fine to change the requirement if it's affecting BelongsTo usage but it doesn't

<?php

namespace App\Nova;

class Post extends Resource
{
    public static $with = ['user'];

    public function fields(Request $request)
    {
        return [
            ID::make('ID', 'id')->asBigInt()->sortable(),

            BelongsTo::make('User', 'user')->display('name')->sortable(),

            // ...
        ];
    }
}

<?php

namespace App\Models;

use Illuminate\Database\Eloquent\Model;

class Post extends Model
{
    public function user()
    {
        return $this->belongsTo(User::class);
    }
}

[HeJiaNong]

In that case, I don't think we should change it. Added following tests and BelongsTo with proper usage doesn't create n+1.

    public function test_can_fetch_resources_with_nullable_with()
    {
        factory(Post::class)->times(15)->create([
            'user_id' => null,
        ]);

        PostResource::$with = ['user'];

        DB::enableQueryLog();
        DB::flushQueryLog();

        $response = $this->withExceptionHandling()
                        ->getJson('/nova-api/posts');

        $response->assertJsonCount(15, 'resources');
        $this->assertEquals(15, $response->original['total']);

        $queryLogs = ray()->pass(DB::getQueryLog());

        $this->assertCount(3, $queryLogs);
        $this->assertSame('select * from "posts" order by "posts"."id" desc limit 26 offset 0', $queryLogs[0]['query']);
        $this->assertSame('select * from "users" where 0 = 1 and "users"."deleted_at" is null', $queryLogs[1]['query']);
        $this->assertSame('select count(*) as aggregate from "posts"', $queryLogs[2]['query']);

        PostResource::$with = [];
        DB::disableQueryLog();
    }

When the associated field uses a foreign key value that does not exist, it will trigger an N+1 problem. like this:

public function test_n_plus_1_problem_will_occur_when_the_association_does_not_exist(): void
{
    $user = UserFactory::new()->create([ 'id' => 'user_id' ]);

    $this->actingAs($user);

    $state = new Sequence(
        [ 'user_id' => 'Undefined_1' ],
        [ 'user_id' => 'Undefined_2' ],
        [ 'user_id' => 'Undefined_3' ],
    );

    PostFactory::new()->count(15)->state($state)->create();

    PostResource::$with = [ 'user' ];

    DB::enableQueryLog();
    DB::flushQueryLog();

    $response = $this->withExceptionHandling()->getJson($this->domain . '/nova-api/post-resources');

    $response->assertJsonCount(15, 'resources');
    $this->assertEquals(15, $response->original[ 'total' ]);

    $queries = collect(DB::getQueryLog())->pluck('query');

    $this->assertCount(20, $queries);

    // Other middleware queries
    $this->assertSame('pragma table_info("permissions")', $queries->shift());
    $this->assertSame('select * from "permissions"', $queries->shift());

    // posts
    $this->assertSame('select * from "posts" order by "posts"."id" desc limit 26 offset 0', $queries->shift());
    $this->assertSame('select * from "users" where "users"."id" in (?, ?, ?) and "users"."deleted_at" is null', $queries->shift());
    $this->assertSame('select count(*) as aggregate from "posts"', $queries->shift());

    // N+1 problems arise
    $this->assertCount(15, $queries);
    $queries->each(fn(string $query) => $this->assertSame('select * from "users" where "users"."id" = ? limit 1', $query));

    PostResource::$with = [];
    DB::disableQueryLog();
}

PostResource

<?php

namespace App\Nova;

use Illuminate\Http\Request;
use Laravel\Nova\Fields\BelongsTo;
use Laravel\Nova\Fields\ID;
use App\Models\Post;

class PostResource extends Resource
{
    public static $model = Post::class;

    public static $with = [ 'user' ];

    public function fields(Request $request): array
    {
        return [
            ID::make(__('ID'), 'id')->sortable(),

            BelongsTo::make('User', 'user', User::class),
        ];
    }
}

PostModel

<?php

namespace App\Models;

use Illuminate\Database\Eloquent\Factories\HasFactory;
use Illuminate\Database\Eloquent\Model;
use Illuminate\Database\Eloquent\Relations\BelongsTo;

class Post extends Model
{
    use HasFactory;

    public function user(): BelongsTo
    {
        return $this->belongsTo(User::class);
    }
}

[crynobone]

This has been fixed with v3.29.0. https://nova.laravel.com/releases/3.29.0

[github-actions[bot]]

This thread has been automatically locked since there has not been any recent activity after it was closed. Please open a new issue for related bugs.