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

Incomplete BelongsToMany eager loads #273

Closed
hexus opened this issue Sep 24, 2018 · 5 comments
Closed

Incomplete BelongsToMany eager loads #273

hexus opened this issue Sep 24, 2018 · 5 comments

Comments

@hexus
Copy link
Contributor

hexus commented Sep 24, 2018

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
Copy link
Contributor Author

hexus commented Oct 1, 2018

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
Copy link
Contributor Author

hexus commented Oct 5, 2018

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
Copy link
Contributor Author

hexus commented Oct 5, 2018

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

adrorocker added a commit that referenced this issue Mar 12, 2019
Fixed incomplete BelongsToMany eager loads (#273)
@adrorocker
Copy link
Member

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

@hexus
Copy link
Contributor Author

hexus commented Mar 13, 2019

Cheers! 🎉

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

No branches or pull requests

2 participants