kirschbaum-development / eloquent-power-joins

The Laravel magic you know, now applied to joins.
MIT License
1.41k stars 89 forks source link

`joinRelationship` can't handle the One-to-Many Polymorphic relationship (`morphTo`) #147

Closed vHeemstra closed 1 year ago

vHeemstra commented 1 year ago

Related: #129

Context

Intro

OrderItems are line items of a placed order. It can be (linked to) either a Product or a Course (maybe others later). So this is a One-to-Many polymorphic relationship, where the OrderItem has one related model and Product/Course has many related OrderItems.

OrderItem

Model

use SoftDeletes, PowerJoins;

public function orderable()
{
  return $this->morphTo();
}

DB table order_items

Product / Course

Model

use SoftDeletes, PowerJoins;

public function order_items()
{
  return $this->morphMany(OrderItem::class, 'orderable');
}

DB table products / courses

Problem

OrderItem::joinRelationship('orderable')->toSql() produces a very strange SQL with an INNER JOIN to itself using an unknown/empty column (like it was joining the other way around):

select `order_items`.*
from `order_items`
inner join `order_items`
on `order_items`.`orderable_id` = `order_items`.``
and `order_items`.`deleted_at` is null
where `order_items`.`deleted_at` is null

Suggested solution

For these inverse cases, the query builder needs to know the other table name of course. Following Laravels way to handle this with whereHasMorph, I would suggest something like:

Which would produce:

select `order_items`.*, `products`.*
from `order_items`
inner join `products`
on `order_items`.`orderable_id` = `products`.`id`
and `order_items`.`orderable_type` = 'App\\Models\\Product'
and `products`.`deleted_at` is null
where `order_items`.`deleted_at` is null

(Maybe not feasible) To join multiple types, something like:

Would produce:

select `order_items`.*,
  # Include columns for each type
  `products`.*,
  `courses`.*
from `order_items`
  # Left join for each type:
  left join `products`
    on `order_items`.`orderable_id` = `products`.`id`
    and `order_items`.`orderable_type` = 'App\\Models\\Product'
    and `products`.`deleted_at` is null
  left join `courses`
    on `order_items`.`orderable_id` = `courses`.`id`
    and `order_items`.`orderable_type` = 'App\\Models\\Course'
    and `courses`.`deleted_at` is null

where
  `order_items`.`deleted_at` is null

  # Ensure only rows with an orderable item:
  and (
    `products`.`id` is not null
    or `courses`.`id` is not null
  )
luisdalmolin commented 1 year ago

This was implemented in the last release (3.3.0). Thanks for the report.

API is:

Image::joinRelationship('imageable', morphable: Post::class);
vHeemstra commented 1 year ago

Awesome!

Is the multi-variant also implemented?

Like so:

OrderItem::joinRelationship('orderable', [Product::class, Course::class])

EDIT: Nevermind, just read this:

https://github.com/kirschbaum-development/eloquent-power-joins/blob/9dd336b17cca5c82315365c5323895297fc35037/README.md?plain=1#L96

luisdalmolin commented 1 year ago

Yeah, querying both with joins is actually interesting. Can you explain your use case here? Each order item will have only a product or a course, right? You want to grab the data independently of which one it is?

vHeemstra commented 1 year ago

Right, so as I said in my initial description, since it is a morphable relationship, it is open to any other model type. So when you want to get all OrderItems that only belong certain 'orderable' models and load/get those orderables too, you could use a statement like:

OrderItem::joinRelationship('orderable', [Product::class, Course::class])

This would only load these types of model (in addition to the OrderItem).

And maybe it is even possible to do something like:

OrderItem::powerJoinHas('orderable', [Product::class, Course::class])

?

Anyway, the main point of this issue was to be able to use MySQL's JOIN with the morphTo relationship on models to (hopefully) have a better performance running one query instead of multiple. Although this might not be the case in the end with the MySQL inner workings, computation and memory performance.

Some testing should be done to see if this actually speeds things up or just creates way too big of a workload to the MySQL server.

luisdalmolin commented 1 year ago

So when you want to get all OrderItems that only belong certain 'orderable' models and load/get those orderables too, you could use a statement like

You would have to use left joins in this case, but that's reasonable. Regular inner joins would try to basically fetch an OrderItem where it has a Product AND a Course, which will never be true in this case

What you may be looking for seems actually the powerJoinHas instead, indeed, at least from your description. I need to find some time, but will try to add this.

vHeemstra commented 1 year ago

Yes, my search was for multiple functionalities that your package has, but it just would not work (yet) with the 'reverse' morphTo relationship. So that's the reason to start this issue.

I tried going through the code to add it myself and send in a PR, but I got a bit stuck in the code 😄 I will have another look

luisdalmolin commented 1 year ago

I just added the ability to use powerJoinHas using the morphable attribute in the latest release.