5.7.20 Breaking Change to Relationships

[JeremyJass]

• Laravel Version: 5.7.20
• PHP Version: 7.2.8
• Database Driver & Version: MySQL 5.6.8

Description:

The 5.7.20 update included PR#26992 which introduced a breaking change to our Laravel application. In an attempt to eliminate unnecessary queries, it creates eager loading situations where a query that should (or at very least historically has) run is inhibited.

Since this is a breaking change on a point release, recommend rolling back these changes and if still desired include them as a breaking change in the next major release. I've also included a possible fix, but verifying this fixes our issue without creating any further problems seems more dangerous then just rolling back.

Possible Fix:

In the modified relationship files, instead of assuming the query is unnecessary just because $this->child->{$this->foreignKey} evaluates to null, additionally check if there are any attributes at all. The absence of any attributes indicates the Model has not yet been inflated, and the requested query may still be valid.

\Illuminate\Database\Eloquent\Relations\BelongsTo.php : 78

if (count($this->child->getAttributes()) != 0 && is_null($this->child->{$this->foreignKey})) {
    return $this->getDefaultFor($this->parent);
}

Steps To Reproduce:

A bit lengthy, but was able to minimally replicate our issue using the following 3 Model classes, and a migration listed below.

To keep things simple this is a bit contrived, but Users subscribe to Organizations, and Organizations belong to a Location. What we are trying to do is establish a conditional relationship on the Organizations class that allows us to retrieve all subscribed Users that meet some alertable criteria that is dependent on the Location.

Changed in 5.7.20, when the alertable method in the Organization class is called, $timezone = $this->location->timezone_id; throws a Trying to get property 'timezone_id' of non-object exception. This is because, $this->child->{$this->foreignKey} from BelongsTo.php (line 78) evaluates to null.

The assumption is that if $this->child->{$this->foreignKey} (in BelongsTo) is null, no related Model exists and the query can be avoided. However in our code since we are eager loading and the Model isn't inflated yet, this check fails even though we need the query to run.

Although our problem was only with the BelongsTo relationship, similar changes were made to the other relationship files (PR#26992) and they are likely impacted as well.

Target Action (put in a Command, Controller, or tinker)

// Works in 5.7.19, broken in 5.7.20
$orgs_with_alertable_users = \App\Organization::whereHas('alertable')->with('alertable')->get();

User.php

<?php
namespace App;
use Illuminate\Database\Eloquent\Model;

class User extends Model
{
    protected $table = 'test_users';
    protected $fillable = [
        'organization_id'
    ];
}

Location.php

<?php
namespace App;
use Illuminate\Database\Eloquent\Model;

class Location extends Model
{
    protected $table = 'test_locations';
    protected $fillable = [
        'timezone_id'
    ];
}

Organization.php

<?php
namespace App;
use App\User;
use App\Location;
use Illuminate\Database\Eloquent\Model;

class Organization extends Model
{
    protected $table = 'test_organizations';
    protected $fillable = [
        'location_id'
    ];

    /*
     * This Organization belongs to a Location.
     * @return \Illuminate\Database\Eloquent\Relations\BelongsTo
     */
    public function location()
    {
        return $this->belongsTo(Location::class, 'location_id', 'id');
    }

    /*
     * Users subscribed to this Organization.
     * @return \Illuminate\Database\Eloquent\Relations\HasMany
     */
    public function subscribers()
    {
        return $this->hasMany(User::class, 'organization_id', 'id');
    }

    /*
     * Users that meet some criteria that is dependent on their Location.
     * @return \Illuminate\Database\Eloquent\Relations\HasMany
     */
    public function alertable()
    {
        // Worked in 5.7.19, broken in 5.7.20
        $timezone = $this->location->timezone_id;

        // Alternative approach that works in 5.7.20, but doesn't leverage query caching
        // $timezone = $this->location()->first()->timezone_id;

        // construct Location sensitive parameters
        $start_of_shift = $this->shiftStart(now($timezone));
        $end_of_shift = $start_of_shift->copy()->addHours(8);

        return $this->subscribers()
            ->whereBetween('created_at', [$start_of_shift, $end_of_shift]);
    }

    /*
     * Shift Calcuation Helper
     * @param  \Carbon\Carbon $date
     * @return \Carbon\Carbon
     */
    public static function shiftStart($date)
    {
        // Placeholder dumb logic.
        return $date->copy()->subHours(2);
    }
}

Migration File (includes seeding of dummy records)

<?php
use Illuminate\Support\Facades\Schema;
use Illuminate\Database\Schema\Blueprint;
use Illuminate\Database\Migrations\Migration;

class CreateTestTables extends Migration
{
    public function up()
    {
        Schema::create('test_users', function (Blueprint $table) {
            $table->increments('id');
            $table->timestamps();
            $table->integer('organization_id');
        });

        Schema::create('test_locations', function (Blueprint $table) {
            $table->increments('id');
            $table->timestamps();
            $table->string('timezone_id');
        });

        Schema::create('test_organizations', function (Blueprint $table) {
            $table->increments('id');
            $table->timestamps();
            $table->integer('location_id');
        });

        // Seed dummy records
        $location = \App\Location::create([
            'timezone_id' => 'America/New_York'
        ]);
        $org = \App\Organization::create([
            'location_id' => $location->id
        ]);
        \App\User::create([
            'organization_id' => $org->id
        ]);
    }

    public function down()
    {
        Schema::dropIfExists('test_organizations');
        Schema::dropIfExists('test_locations');
        Schema::dropIfExists('test_users');
    }
}

[staudenmeir]

I don't understand Organization::alertable(): What's the idea behind $this->location?

$this->location always fetches the first row in the locations table, it is not related to the specific organization. The executed query in 5.7.19:

select * from `locations` limit 1

[JeremyJass]

$this->location was an attempt to reference the Organization::location() relationship defined in the class. In this example we would be trying to select Users of an Organization by some criteria that was at least partially dependent on the Location of the Organization. It would appear this isn't what was actually happening.

Wanted to get home so I could fire up a downgraded Vagrant and test thinking it might be something with there only being 1 record in the test data, but even adding a second record, and assigning the Organization to the second record the same query was being executed.

Is referring to a relationship from within the context of a relationship what's causing the problem? If it is, the problem is likely our approach here and the 5.7.20 just shone a light on an issue that was silent before.

Thanks for taking the time to copy over the code and helping investigate!

[staudenmeir]

Yes, this is a fundamental issue. When calling a relationship using has() or with(), $this is always an empty dummy model. Here, $this is just a new Organization instance without any attributes.

[Shkeats]

@JeremyJass Could you share what actually happened when you run the code? I've been getting a segfault on query builders with eager loads that worked in prior versions and I wan't to work out if the issue I'm having is related to yours.

It definitely relates to the same PR, as when I port just these changes backwards to 5.7.19 (which works) I get the same problems.

[JeremyJass]

My issue stemmed from trying to reference $this within the context of a relationship method. When a relationship is called from a has() or with() statement ($myModel->with('relationship')->...), I've now learned from staudenmeir that $this is always just an empty dummy.

Since we were trying to use $this in a relationship context ($this->checkMyProperties() to scope the relationship results) we got a Trying to get property of non-object exception. Probably not related to your issue, but good luck!

[Shkeats]

@JeremyJass Appreciate your reply, it was helpful. Looks like I have a faulty relation like yours in play here too. One that references a $this->attribute inside the model it's defined on and therefore can't be eager loaded. Seems like the change @staudenmeir made here just exposes it in a different way, it was never working as intended in my application.

[MannikJ]

Upgrading to v5.17.20 also revealed some flaws in our app, because we are also sometimes using $this within relation to filter the results, which is apparently not the best idea.

What I don't understand is why the results seemed to be correct with the previous versions. What does the query do, if you try to load relations from a dummy model?
I made a small example to create the strange behavior I am talking about by using '$model->load()'.
I did not investigate what happens when I load the relation by calling 'with()' on the builder instance though.

/** @test */
public function relation()
{
    $model = MyModel::create();
    $related = RelatedModel::create(['my_model_id' => $model->id]);
    $this->assertInstanceOf(RelatedModel::class, $model->relation); // correctly loaded
    $model->load('relation');
    $this->assertNull($model->relation);
}

public function relation()
    {
        $relation = $this->hasOne(RelatedModel::class);

        // Will be null if called from 'load',
        // because the null check in 'getResults'
        // No query executed
        $this->otherRelation;

        // Executes a query
        // Returns a result set if called
        // from 'load' that looks correct in my case, 
        // although '$this' is only a dummy with empty attributes
        // Why? 
        // 'getResults' is not called
        $this->otherRelation()->get();

        return $relation;
    }

public function otherRelation()
{
    return $this->hasMany(OtherRelatedModel::class);
}

In previous versions $this->otherRelation would return the same as $this->otherRelation()->get(), because of the missing null check.

[MannikJ]

Ok what happens here when calling $this->otherRelation()->get() is that if $this is a dummy model and the foreign keys etc. are null, the relation constraints are apparently not applied at all.

The query executed is something like:

select *  
from "other_related_models"

instead of:

select * from "other_related_models"
where "other_related_models"."related_model_id" = ?
and "other_related_models"."related_model_id" is not null

So depending on the type of relation you get either all records of the table or a too large set if some constraints are still applied.
Shouldn't this be considered a bug?

[staudenmeir]

When I query a HasMany relationship on an empty dummy model, I get this query:

select *
from "other_related_models"  
where "other_related_models"."related_model_id" is null
and "other_related_models"."related_model_id" is not null