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

[11.x] Eloquent inverse relations #51582

Merged

Conversation

samlev
Copy link
Contributor

@samlev samlev commented May 27, 2024

How many times has this happened to you?

@foreach($post->comments as $comment)
  <!-- comment content -->
  @can('promote', $comment)
    <a href="{{ route('posts.comments.promote', [$post, $comment]) }}">promote</a>
  @endcan
@endforeach

With the policy:

public function promote(User $user, Comment $comment): bool
{
    return
        $comment->approved_at !== null
        && $comment->promoted === false
        && $user->can('update', $comment->post);
}

Only to find that you're now getting a new query for every single comment to pull the post back out of the database?

Obviously you could eager-load the post back onto the model, but even then that's creating a lot of extra busywork and it might miss scopes that were added on the original query:

// Ensure that the comments pull can access trashed posts
$posts = Post::withTrashed()->with(['comments' => ['post' => fn ($q) => $q->withTrashed() ]])->paginate();

Well it's happened to me. A lot. So I decided to do something about it.

Introducing inverse()

inverse() is a modifier for Has* and Morph* relations that allows you to easily set the parent model in the correct relation on the child models:

public function comments(): HasMany
{
    return $this->hasMany(Comment::class)->inverse('post');
}

Now you can do:

$posts = Post::withTrashed()->with('comments')->paginate();

And each Comment model will have its post relation filled out with the actual instance of the parent model, with no extra queries, or missed scopes.

Note that it injects the actual instance, instead of querying and pulling another instance which could then get out of sync:

$eager = $post->comments()->with('post')->first();
$inverse = $post->comments()->inverse('post')->first();
$post->delete();
$eager->post->trashed(); // false
$inverse->post->trashed(); // true

Optionally Skipping an Inverse Relationship

If you don't want to incluide the inverse relationship for some reason, you can tack ->withoutInverse() onto the query builder that you get from the relation:

public function posts(): HasMany
{
    return $this->hasMany(Post::class)->inverse('user');
}

// ...

User::with(['posts' => fn ($q) => $q->withoutInverse()])->toArray();

Guessing the Inverse Relationship

If you don't define the inverse relationship it will attempt to guess the correct relationship based on the foreign key of the relation, the foriegn key of the parent, the name of the parent model, the name of a polymorphic relationship, or a couple of common relationship names such as owner.

class Comment extends Model
{
    public function user(): BelongsTo;
}

class PageView extends Model
{
    public function person(): BelongsTo;
}

class Post extends Model
{
    public function author(): BelongsTo;
}

class PageView extends Model
{
    public function person(): BelongsTo;
}

class Licence extends Model
{
    public function owner(): BelongsTo;
}

class Report extends Model
{
    public function reportable(): MorphTo
}

class User extends Model
{
    public function getForeignKey()
    {
        return 'person_id';
    }

    public function comments(): HasMany
    {
        // guesses user() from User::class
        return $this->hasMany(Comment::class)->inverse();
    }

    public function pageViews(): HasMany
    {
        // guesses person() from User::getForeignKey()
        return $this->hasMany(PageView::class)->inverse();
    }

    public function posts(): HasMany
    {
        // guesses author() from passing author_id
        return $this->hasMany(Post::class, 'author_id')->inverse();
    }

    public function licence(): HasOne
    {
        // guesses owner() as a fallback
        return $this->hasOne(Licence::class)->inverse();
    }

    public function reports(): MorphMany
    {
        // guesses reportable from the relationship
        return $this->morphMany(Report::class, 'reportable')->inverse();
    }
}

If the parent is the same class as the child, it will also guess parent.

class Category extends Model
{
    public function ancestor(): BelongsTo
    {
        return $this->belongsTo(Category::class, 'parent_id');
    }

    public function children(): HasMany
    {
        // guesses $category->ancestor
        return $this->hasMany(Category::class, 'parent_id')->inverse();
    }
}

Backwards Compatibility

There aren't really any backwards compatibility concerns - the new method is "opt-in", and the changes made to the existing relationships shouldn't have any affect if the new method isn't used.

Which Relationships Can Be Inverted?

Only the HasOneOrMany relationships make sense for inverse(). This means that it's available for HasOne, HasMany, MorphOne, and MorphMany. It also supports the OneOfMany modifiers.

None of the BelongsTo* relations really make sense to define an inverse relationship because the inverse will probably be a collection.

Potential risks

As mentioned by @stayallive, there is a known issue with getQueueableRelations() and relationsToArray() when you have circular object references, leading to an infinite loop as it tries to dig down through the "tree" to discover relations. While this change isn't introducing that issue (it already exists), it does increase the likelihood that developers will encounter it.

I don't think that trying to fix it in this PR is wise, because that will increase the complexity of this change and, again, this change doesn't create the issue - it just makes it more likely for developers to encounter it. I've personally encountered it in my own work when I was using setRelation to manually do what inverse() does.

I do have a mitigation in mind for it, but I think that I'll reserve that for a separate PR. In short, it could be mitigated by keeping track of the objects that have been serialized, and don't keep digging down on ones that we have already seen - it shouldn't be particularly more memory hungry (it will involve temporarily holding another array of references to objects), but it does need to get added in a number of places.

Copy link

Thanks for submitting a PR!

Note that draft PR's are not reviewed. If you would like a review, please mark your pull request as ready for review in the GitHub user interface.

Pull requests that are abandoned in draft may be closed due to inactivity.

@stayallive
Copy link
Contributor

This would be great to have in the core, ever since Jonathan Reinink wrote a great blog post about optimizing circular relationships in Laravel this has been on my mind.

The package stancl/laravel-hasmanywithinverse ran with that idea and implemented it for the hasMany relation. And I have a package improves on that by implementing it for the hasOne and morphMany relations too.

The one downside is that this can cause an infinite loop in $model->getQueueableRelations() that is used when serializing models in queue jobs. This is because you create a circular dependency between the child and parent and is probably something that should be tested for or handled. Apologies if I missed a mitigation for this in your implementation but I felt like mentioning this couldn't hurt.

@samlev samlev marked this pull request as ready for review May 27, 2024 09:10
@samlev samlev marked this pull request as draft May 27, 2024 09:15
@samlev samlev marked this pull request as draft May 27, 2024 09:15
@samlev samlev marked this pull request as draft May 27, 2024 09:15
@samlev
Copy link
Contributor Author

samlev commented May 27, 2024

Apologies if I missed a mitigation for this in your implementation but I felt like mentioning this couldn't hurt.

I haven't done any mitigation for it yet, but I have encountered it before, so it's on my radar.

Partly I'm putting this up in draft right now so that I can start getting feedback before I sink much more time into it (if it's likely to get rejected, it'll probably be because it started getting complex before people could assess what it's actually about).

@bert-w
Copy link
Contributor

bert-w commented May 27, 2024

Why is it called "inverse" instead of something more sensible like ->linksTo() or ->references()? Just throwing some ideas out here.

@samlev
Copy link
Contributor Author

samlev commented May 27, 2024

Why is it called "inverse" instead of something more sensible like ->linksTo() or ->references()? Just throwing some ideas out here.

Essentially it's marking the reverse/inverse relationship, not just a model that it's linked to. They're already referred to as "inverse" relations in the documentation.

@taka-oyama
Copy link
Contributor

Wouldn't this causes an infinite loop in $model->toArray()?

@samlev
Copy link
Contributor Author

samlev commented May 28, 2024

Wouldn't this causes an infinite loop in $model->toArray()?

Potentially, yes. Well... no, this wouldn't cause it - the potential already exists when you use setRelation(), or with multiple other causes, but yes - this would potentially expose it to more people who may not have encountered it otherwise. I've mentioned it in the main blurb, and I have some ideas for mitigation, but they're not something that I want to dig into in this PR.

@samlev samlev marked this pull request as ready for review May 28, 2024 21:54
@taylorotwell
Copy link
Member

I feel like it's clunky you have to pass the name of the inverse relationship which almost always matches the model you are in at the time.

@taylorotwell taylorotwell marked this pull request as draft May 30, 2024 14:24
@samlev
Copy link
Contributor Author

samlev commented May 30, 2024

I've pushed up changes to make ->inverse() guess the relation based on a number of clues, including the foreign key, the name of the parent model, the morph type (if one exists), and a couple of "likely" strings.

@samlev samlev force-pushed the feature/11.x_eloquent_inverse_relation branch from 2cf444f to eb42cbf Compare May 30, 2024 18:34
@samlev samlev marked this pull request as ready for review May 30, 2024 18:35
@samlev samlev force-pushed the feature/11.x_eloquent_inverse_relation branch from 3e7ae71 to 58a9f01 Compare May 31, 2024 04:27
@samlev samlev force-pushed the feature/11.x_eloquent_inverse_relation branch from 58a9f01 to cc25c9d Compare May 31, 2024 04:44
@bert-w
Copy link
Contributor

bert-w commented May 31, 2024

Why is it called "inverse" instead of something more sensible like ->linksTo() or ->references()? Just throwing some ideas out here.

Essentially it's marking the reverse/inverse relationship, not just a model that it's linked to. They're already referred to as "inverse" relations in the documentation.

I disagree; the word "inverse" in the docs is used as an indication that hasMany has an inverse, being belongsTo. It gives me the impression that I get a different relation type back. However, this PR modifies some relation by essentially saying "whenever this relation is fetched, please assign the parent model to each of the child instances". Therefore it sounds more logical to use something like ->references()/->referenced()/->reference().

@samlev
Copy link
Contributor Author

samlev commented May 31, 2024

I disagree; the word "inverse" in the docs is used as an indication that hasMany has an inverse, being belongsTo. It gives me the impression that I get a different relation type back. However, this PR modifies some relation by essentially saying "whenever this relation is fetched, please assign the parent model to each of the child instances". Therefore it sounds more logical to use something like ->references()/->referenced()/->reference().

No, it's setting the "parent" model as the inverse relationship. $user->hasMany(Post::class)->inverse() will try to find the relationship that links the post back to the user - i.e.$post->belongsTo(User::class).

That's why this isn't on any belongs* relations, because there is no sensible inverse there - the inverse relationship to $post->belongsTo(User::class) is $user->hasMany(Post::class), but that doesn't make sense because the post model isn't many posts. It's one of many posts so you can't grab the user and set the single post as the collection of posts.

->references() isn't an accurate description of what's going on, because all relationships are references. This is explicitly for filling the inverse relationship back to the model that just pulled another model into existence - not just for randomly assigning that model to "something" on the child. Whatever term is used must be a relationship that the child model has. It's up to developers if they want to be weird about it and set it to a non-inverse relationship, but the intent is that this sets the parent model to the relevant inverse relationship on the child.

I could do $user->posts()->with('user');, but that involves extra queries and extra memory.

@bert-w
Copy link
Contributor

bert-w commented May 31, 2024

No, it's setting the "parent" model as the inverse relationship. $user->hasMany(Post::class)->inverse() will try to find the relationship that links the post back to the user - i.e.$post->belongsTo(User::class).

That's why this isn't on any belongs* relations, because there is no sensible inverse there - the inverse relationship to $post->belongsTo(User::class) is $user->hasMany(Post::class), but that doesn't make sense because the post model isn't many posts. It's one of many posts so you can't grab the user and set the single post as the collection of posts.

Yes you are right, the code indeed sets the inverse relationship to the "parent" exactly as you say, but the word ->inverse() does not convey this meaning to me. One of the important features of this PR is that the model that you set on the relation-result is a direct reference, which is a key difference with the current Eloquent setup.

This PR is a good idea anyway regardless of which word is used :)

@samlev samlev force-pushed the feature/11.x_eloquent_inverse_relation branch from ff6ca30 to bea61e6 Compare June 1, 2024 01:49
@Tofandel
Copy link
Contributor

Tofandel commented Jul 8, 2024

Love the idea

Wouldn't this causes an infinite loop in $model->toArray()?

It would and maybe the toArray() method needs a fix? The mitigation is to use attributesToArray() which doesn't include any relations, if you need relations it becomes a bit tweakier

@samlev samlev marked this pull request as draft July 22, 2024 13:21
@driesvints
Copy link
Member

@samlev what's the status of this PR? Might be best to resend it another time when you have time to finish it?

@samlev
Copy link
Contributor Author

samlev commented Jul 26, 2024

@driesvints it had been ready to go for a while, except for a concern about relationsToArray() which I have thoughts about, but isn't appropriate for this PR. Essentially this would potentially expose more people to an existing issue with circular references in relationsToArray(), but wouldn't cause any issues on any existing code.

I just moved this PR back to draft this week because it had been sitting open for a while with no action, so I assumed that the relationsToArray() issue was the holdup. If that's not the case I can re-open it, otherwise I'll try to get a PR up to fix that issue next week.

@driesvints
Copy link
Member

I don't think you need to move it to draft if Taylor hasn't. He'll review your PR once he has the chance 👍

@samlev samlev marked this pull request as ready for review July 26, 2024 10:36
@taylorotwell
Copy link
Member

@samlev can you elaborate on what would cause the infinite loop issue you reference? Maybe provide a set of example models and relations that would cause that?

@samlev
Copy link
Contributor Author

samlev commented Aug 1, 2024

@taylorotwell the basic issue is if you have circular references in relationships, then $model->relationsToArray() (and by extension $model->toArray()) will just keep recursively trying to re-serialise the same objects.

A toy example:

$user->posts->each->setRelation('author', $user);

$user->toArray(); // causes infinite recursion on $user->posts[0]->author->posts[0]->author->posts[0]...

This is already an issue in the existing code for models and I have a half-complete PR to resolve it, but it's not something that I think is appropriate for this PR. I'm happy to put this PR on hold until I get the other ready.

@samlev
Copy link
Contributor Author

samlev commented Aug 13, 2024

Found some time, eventually. The recursive/circular relations issue should be resolved/resolvable with #52461

@taylorotwell taylorotwell merged commit b11c240 into laravel:11.x Aug 22, 2024
29 checks passed
@taylorotwell
Copy link
Member

Thanks!

@christophrumpel
Copy link
Contributor

@samlev Hey, thanks for the feature. What was the reason to keep chaperon and inverse?

@PerryvanderMeer
Copy link
Contributor

@samlev Hey, thanks for the feature. What was the reason to keep chaperon and inverse?

@christophrumpel The alias was added by @taylorotwell here: 98400bf

@christophrumpel
Copy link
Contributor

Ah I see, so I guess "chaperone" is now the official name? 😅 thx @PerryvanderMeer

@samlev
Copy link
Contributor Author

samlev commented Sep 9, 2024

@samlev Hey, thanks for the feature. What was the reason to keep chaperon and inverse?

That's a question for @taylorotwell - inverse() was the name that I used in the PR and seemingly there's a few people for whom it makes sense.

But chaperone() is the official name now

@Kaleemullah007
Copy link

great addition @samlev

@moisish
Copy link
Contributor

moisish commented Oct 15, 2024

Great addition @samlev

Should this be expanded for belongsTo relationships?

On the same example of the mentioned blog /products/{category}/{product} the Product belongs to a Category.

public function show(Category $category, Product $product)
{
    $product->setRelation('category', $category);
}

@samlev
Copy link
Contributor Author

samlev commented Oct 15, 2024

@moisish The problem with BelongsTo relationships is that the inverse relation is usually a HasMany, and in that case there's not a good story because you have a single model and the inverse relationship is a collection of many models.

Maybe you could inject it into EloquentCollection, but the mechanism would be different, and there's still no efficient way to validate that ant particular collection is all of the relationship, or should have even been loaded.

Has* style relationships work because the parent is a single model that you've already got access to:

class Post extends Model
{
    public function comments(): HasMany
    {
        // Each comment has exactly one post 
        // so we can set this post as the post on
        // each comment.
        return $this->hasMany(Comment::class)->chaperone();
    }

    public function user(): BelongsTo
    {
        // The user has many posts, but we only have
        // this one post - if we set the inverse relationship
        // on the user, then it would be missing every other
        // post that the user has.
        return $this->belongsTo(User::class)->chaperone();
    }
}

In your example the category has many products, and that's the relationship where you would add chaperone():

class Category extends Model
{
    public function products(): HasMany
    {
        return $this->hasMany(Product::class)->chaperone();
    }
}

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

Successfully merging this pull request may close these issues.