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

Unnecessary queries when using HasOneOrMany when primary key is null #51266

Closed
Yinci opened this issue May 2, 2024 · 7 comments
Closed

Unnecessary queries when using HasOneOrMany when primary key is null #51266

Yinci opened this issue May 2, 2024 · 7 comments

Comments

@Yinci
Copy link

Yinci commented May 2, 2024

Laravel Version

11.6.0

PHP Version

8.2.5

Database Driver & Version

MySQL 8.0.32

Description

When using HasMany or MorphMany relations, if the model's primary key is null, it creates an impossible query:

select * from `media` where `media`.`model_type` = 'App\Models\Employer' and `media`.`model_id` is null and `media`.`model_id` is not null limit 1
select count(*) as aggregate from `employers` where `employers`.`parent_id` is null and `employers`.`parent_id` is not null and `employers`.`deleted_at` is null

In the above scenario, the employer model utilized has been fetched using a select statement which transforms the primary key (select('id as value')). However, when attempting to fetch relations on this model, it will still call the database.

Seemingly, this should have been fixed with #26992 but I suppose when it is a direct relation query it doesn't use getResults and therefore doesn't bail.

Steps To Reproduce

  1. Create a model with a parent_id:
Schema::create('employers', function (Blueprint $table) {
    $table->unsignedBigInteger('parent_id')->nullable();
});
  1. Seed a parent and child:
$employer = \App\Models\Employer::create();
\App\Models\Employer::create(['parent_id' => $employer->id]);
  1. Define a relation:
public function children(): HasMany
{
    return $this->hasMany(Employer::class, 'parent_id', 'id');
}
  1. Fetch the Employer using a select and call the relation as query:
$employer = Employer::select('id as value')->first()->children()->count();
@driesvints
Copy link
Member

I'm sorry but I'm afraid I don't understand this use case or see a reason why you'd ever want to use a query like this? How would the relation ever be fetched if the primary key of the parent is null?

@driesvints
Copy link
Member

Closing this issue because it's inactive, already solved, old or not relevant anymore. Feel to open up a new issue if you're still experiencing this.

@Yinci
Copy link
Author

Yinci commented May 13, 2024

@driesvints I apologize for not replying sooner; it's not that you want a model with a primary key that's null, it's just an effect when it happens to be. In my case due to a select statement used to populate a select2 with backend data, and the employer model had a total_children attribute set in the $appends. When the result was returned to the frontend it automatically converts it to json, appending the attribute, resulting in an impossible query.

@driesvints
Copy link
Member

I'm sorry. I just don't understand why you want to do select('id as value'). A primary identifier is key for mapping relations. It can't just be aliased like that. If you feel there's something that can be improved you're always free to send in a PR.

@Yinci
Copy link
Author

Yinci commented May 14, 2024

@driesvints Let me give the full example, so you hopefully get a better understanding:

Simple breakdown of the Employer model:

<?php 

namespace App\Models;

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

class Employer extends Model
{
    protected $appends = [‘total_children’];

    public function getTotalChildrenAttribute(): int
    {
        return (int) $this->children()->count();
    }

    public function children(): HasMany
    {
        return $this->hasMany(Employer::class, 'parent_id', 'id');
    }
}

Simple snippet representing the code that gets the filter:

public function getFilters(): array
{
    return [
        ‘employers’ => Employer::select(‘id as value’, ‘company_name as text’)->get()->toArray(),
    ];
}

This simple scenario creates an impossible query, simply because using the select removes the expected primary key, and using toArray triggers the appends, calling the hasMany relation.

In any normal scenario, there wouldn’t be an issue, but this scenario requires the filter value to be labeled value. And, I suppose this would also be the case if you happened to call toArray on a replicated model (Employer::first()->replicate()->toArray()).

Yes, I want to note that performance wise this $appends is not the smartest and has since then been optimized, however it remains the fact that this can cause unnecessary queries, definitely for people who are not yet known with Laravel’s best practices or gotcha’s.

While I would open a PR if I could, I simply do not have enough knowledge of the Query Builder to know at which point it would be best to implement a bail if it creates an impossible query.

@driesvints
Copy link
Member

I'm sorry but I just don't think we can ever support that scenario. A primary identifier shouldn't be aliased like that in a query.

@Yinci
Copy link
Author

Yinci commented May 17, 2024

@driesvints That's a shame. I just tested and confirmed that it also happens on a replicated instance (which makes sense). I suppose I'll try and see if I can make a PR. Either way, I guess for performance it's important to keep this in mind.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

No branches or pull requests

2 participants