Loading a BelongsTo relationship when the foreign key is null causes N+1

[AnthonyEdmonds]

I have a strange issue where calling $model->load('relationship'); is running a query which will never return a value when that relationship's key is null.

It seems that Laravel does not check whether a foreign key value is null prior to attempting an eager load, as per this issue on lazy loading: laravel/ideas#748

I am running beyondcode/laravel-query-detector, and it flags it up as an N+1 issue every time.

This feels like a bug, however I am uncertain whether this is something that is known and has a workaround.

Recreation

My setup for this is relatively simple, I have a model which has several optional relationships to another model.

Here is an abstracted example:

Post

<?php

namespace App\Models;

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

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

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

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

Test

$post = new Post([
    'poster_id' => 1,
    'contributor_id' => null,
    'editor_id' => null,
]);

$post->load([
    'poster_id',
    'contributor_id',
    'editor_id',
]);

Laravel runs select * from users where users.id in ('') and users.deleted_at is null for each null relationship, which not only gets flagged as an N+1 query but will never return anything.

In my mind Laravel should not be attempting to load a null relationship.

Any thoughts?

[Sophist-UK]

According to laravel/ideas#748 this was resolved in Laravel 5.7 with #26992 .

[AnthonyEdmonds]

Yes, for lazy loading. Eager loading via ->load() on the other hand still seems to have the issue, unless I have overlooked something.

[AnthonyEdmonds]

Looking at #748, their fix was to add a is_null check to the getResults() method on the BelongsTo relationship.

Modifying that relationship, it doesn't seem that ->load() even calls it.

    public function getResults()
    {
        dd(1); // Does not fire, load executes normally

        if (is_null($this->child->{$this->foreignKey})) {
            return $this->getDefaultFor($this->parent);
        }

        return $this->query->first() ?: $this->getDefaultFor($this->parent);
    }

[AnthonyEdmonds]

If I am reading this correctly, ->load() calls eagerLoadRelation() and getRelation() in Illuminate\Database\Eloquent\Builder, line 730 - 780.

Certainly doesn't seem to be a null check in there.

I guess there would have to be a null check, as with #748, followed by a call to getDefault, which somehow ties into the other models being loaded.

[AnthonyEdmonds]

Looking further into it, the difference seems to be in Database\Eloquent\Relations.

A normal query uses the getResults() method, where eager loading uses getEager().

/**
     * Get the results of the relationship.
     *
     * @return mixed
     */
    abstract public function getResults();

    /**
     * Get the relationship for eager loading.
     *
     * @return \Illuminate\Database\Eloquent\Collection
     */
    public function getEager()
    {
        return $this->get();
    }

Naturally, BelongsTo has the null check on getResults(), but getEager() is just a generic get().

Could be as simple as returning an empty collection on null...

[AnthonyEdmonds]

Looking at this from the perspective of the BelongsTo relationship, I would have expected $this->parent or $this->child to be the model that ->load() was called on. This is not the case, it is a blank model without any of the attributes of the caller.

[AnthonyEdmonds]

Illuminate\Database\Eloquent\Builder method getRelation() on line 765 purposefully returns a relationship using a new instance of the model, rather than the original. Not sure why, though I'm sure there's a reason.

[AnthonyEdmonds]

I have a fix for the purposes of demonstrating the concept:

Illuminate\Database\Eloquent\Builder, line 756

    public function getRelation($name)
    {
        $relation = Relation::noConstraints(function () use ($name) {
            try {
                // Removed call to newInstance() to pass loading model to Relation correctly
                return $this->getModel()->$name();
            } catch (BadMethodCallException $e) {
                throw RelationNotFoundException::make($this->getModel(), $name);
            }
            ...

Illuminate\Database\Eloquent\Relations\Relation, line 156

    public function getEager()
    {
        // Taken from BelongsTo for the sake of testing.
        // If null, return new Collection() to be passed into matching
        if (is_null($this->child->{$this->foreignKey})) {
            return new Collection();
        }

        return $this->get();
    }

Naturally this isn't an actual fix, as the null check would need to be per relationship type.

That's about all the time I have for this tonight. If anyone has any thoughts or insights based on the above, I'd be glad to hear it.