search

analogueorm / analogue

Analogue ORM : Data Mapper ORM for Laravel/PHP
MIT License
634 stars 51 forks source link

Incomplete BelongsToMany eager loads #273

Closed hexus closed 5 years ago

hexus commented 5 years ago

The BelongsToMany relationship doesn't eager load properly in 5.6.12.

I haven't yet isolated when this issue was introduced, but I've figured out where and how it happens.

  1. ResultBuilder::loadRelation() will call $relation->match() on the relation after applying an eager loading scope
  2. BelongsToMany::match(). from the above call, then starts by eagerly loading the needed entities
  3. This then reaches ResultBuilder::buildResultSet() for the related entities, which keys the results by related primary key, but this is incorrect for a many-to-many relationship

This means each related model will only be seen once amongst their parent entity, not across many parent entities like they should be.

By the time BelongsToMany::match() is building a dictionary for parent keys, it's operating on related results that are keyed by related key. Instead, every row should be present.

The only way I can see to improve this, so far, would be introducing a result builder for many-to-many relationships.

hexus commented 5 years ago

I have implemented an appropriate failing test case for this (hexus/analogue@17f6af1a), and will be working on a fix this week.

I noticed all the existing tests for the BelongsToMany relationship only test one user belonging to many groups, not many users belonging to many groups. This is likely how this regression occurred.

I will open a PR if I arrive at a fix that I'm happy with, or something that I think can serve a starting point for yourself to solve it as you see fit.

hexus commented 5 years ago

I managed to fix this in my fork in a way that I'm not happy with.

Preventing caching of eager-loaded BelongsToMany entity results, on top of only returning unkeyed results in ResultBuilder::buildResultSet() fixes loading, but breaks some tests related to storing.

hexus commented 5 years ago

I fixed the above by allowing loaded entities to be cached, but keeping the prevention of BelongsToMany loads from the cache.

adrorocker commented 5 years ago

Closing this since PR has been merged. @hexus thanks for your contribution!

hexus commented 5 years ago

Cheers! :tada: