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

[5.3.26] BelongsToMany withCount selects incorrect id. #16645

Closed
cabalopolis opened this issue Dec 3, 2016 · 17 comments
Closed

[5.3.26] BelongsToMany withCount selects incorrect id. #16645

cabalopolis opened this issue Dec 3, 2016 · 17 comments

Comments

@cabalopolis
Copy link

  • Laravel Version: 5.3.26
  • PHP Version: 7.0.13
  • Database Driver & Version: MariaDB-10.1.19

Description:

With a belongsToMany => belongsToMany, using the Builder::withCount() causes the incorrect id to be selected, it appears the id from the pivot table is being selected rather than the table itself.

Here are the models:

  • User (users)
class User extends Model {
   ...
   public function roles() {
      return $this->belongsToMany(Role::class);
   }
   ...
}

users

id name email
9 Some Name [email protected]
  • Role (roles)(role_user)(permission_role)
class Role extends Model {
   ...
   public function permissions() {
      return $this->belongsToMany(Permission::class);
   }
   ...
}

roles

id name description
2 Administrator Some description here

role_user

id role_id user_id
4 2 9

permission_role

id permission_id role_id
12 19 2
13 20 2
14 17 2
15 21 2
  • Permission (permissions)
class Permission extends Model {
   ...
}

Steps To Reproduce:

  1. Grab the roles for the user with the permission count.
$roles = $user->roles()->withCount('permissions')->get()
  1. Check the first role id.
$role_id = $roles->first()->id;
  1. The givenid is 4 (from the role_user table), when it should be 2 (from the roles table).
array(
   "id" => 4, // this should be 2.
   "name" => "Administrator",
   "description" => "Some description here",
   "role_id" => 2,
   "user_id" => 9,
   "permissions_count" => 4,
)

Additionally: the role_id and the user_id should be in a pivot key.

@KKSzymanowski
Copy link
Contributor

KKSzymanowski commented Dec 4, 2016

You are correct.

The generated query looks like this:

SELECT *,

  (SELECT count(*)
   FROM `permissions`
   INNER JOIN `permission_role` ON `permissions`.`id` = `permission_role`.`permission_id`
   WHERE `permission_role`.`role_id` = `roles`.`id`) AS `permissions_count`
FROM `roles`
INNER JOIN `role_user` ON `roles`.`id` = `role_user`.`role_id`
WHERE `role_user`.`user_id` = 9

And it's result is:

id name description created_at updated_at id role_id user_id permissions_count
2 Administrator Some description here 2016-12-04 17:17:01 2016-12-04 17:17:01 4 2 9 4

First 5 columns are from the roles table, next 3 are from the INNER JOINed role_user and the last one is produced by a subquery fetching count of permissions assigned to the role.

As you can see, the id is here twice. Because PHP arrays don't allow for duplicate keys, the id field will first hold 2 and then it'll be overwritten by 4.

Additionally: the role_id and the user_id should be in a pivot key.

They are. Try this:

$user = \App\User::first();

$pivot = $user->roles()
              ->withCount('permissions')
              ->get()
              ->first()
              ->pivot;

dd($pivot->toArray());

For me, its output is:

array:2 [
  "user_id" => 9
  "role_id" => 2
]

Therefore you should probably use this value to get the role id.

Nevertheless, this is a bug in my opinion.

@themsaid
Copy link
Member

themsaid commented Dec 5, 2016

@KKSzymanowski do you think you can open a PR with a fix?

@KKSzymanowski
Copy link
Contributor

I'm not really sure what to do with this.
I suppose one way to go about it is to prefix columns from the pivot table to avoid name conflicts.

I would need to take a look how it's done in other parts of the Eloquent engine, because I'm not that familiar with relationship logic. Give me a day or two, maybe you'll come up with something in the meantime. If so, please share any ideas.

@srmklive
Copy link
Contributor

srmklive commented Dec 5, 2016

@cabalopolis have you tried it like this as well:

public function roles() {
   return $this->belongsToMany(Role::class, 'role_user', 'role_id');
}
public function permissions() {
   return $this->belongsToMany(Permission::class, 'permission_role',  'permission_id');
}

@KKSzymanowski
Copy link
Contributor

I haven't tested but it but I wouldn't expect it to work because these are the default values for belongsToMany relationship.

@srmklive
Copy link
Contributor

srmklive commented Dec 6, 2016

@cabalopolis @KKSzymanowski What you guys have described here, i haven't been able to replicate. My view is that you guys haven't done your due diligence in this regard. I have exactly the table structure as well described by @cabalopolis. This just doesn't happen. I am attaching screenshots of what i did through php artisan tinker:

screenshot from 2016-12-06 12 14 21
screenshot from 2016-12-06 12 20 11
screenshot from 2016-12-06 12 21 40

@cabalopolis
Copy link
Author

@srmklive I'm not 100% sure you have understood the issue / performed that steps correctly. I have set up a repository with the issue outlined, feel free to clone it and observe the issue: https://github.com/cabalopolis/with-count-bug.

@srmklive
Copy link
Contributor

srmklive commented Dec 6, 2016

@cabalopolis I have done it correctly. You in my view are not doing it right 😆. I have cloned your repo, performed the exact steps you mentioned and couldn't replicate the issue you are supposedly facing. All i did was setup model factory config for Role & Permission model. Please find below screenshots of steps i did on your repo:

screenshot from 2016-12-06 15 00 20
screenshot from 2016-12-06 15 05 11

@srmklive
Copy link
Contributor

srmklive commented Dec 6, 2016

Furthermore if such a issue was happening, i am sure the issue section would have flooded with this, and fix would have been issued instantly.

@cabalopolis
Copy link
Author

@srmklive you cannot see the issue as it is being masked by the fact that the pivot table entry has an ID of 1 and the roles table as and ID of 1. If the entry in the pivot table were to have an ID of 2 then you would see the issue.

@srmklive
Copy link
Contributor

srmklive commented Dec 6, 2016

@cabalopolis here you go then 😉

screenshot from 2016-12-06 15 17 53

@cabalopolis
Copy link
Author

@srmklive Run the migrations + seeds in the repo and perform the same as I have done in tinker.
screen shot 2016-12-06 at 11 24 23 pm
The role ID is definitely not correct.

@fernandobandeira
Copy link
Contributor

fernandobandeira commented Dec 6, 2016

@cabalopolis

I can reproduce it too, with your repo... 😸

reproduce

Reproduced with sqlite...

@KKSzymanowski
Copy link
Contributor

@srmklive You didn't see the bug because you created a role with an id of 1 and the pivot table had the id of 1. Then you created a second role(id = 2) and when you attached to the user the pivot table once again had the same id.

@srmklive
Copy link
Contributor

srmklive commented Dec 6, 2016

@KKSzymanowski It doesn't matter which id i attach it in the pivot table, i always got the data with correct id. What @cabalopolis is referring to an issue with a single entity i guess.

@cabalopolis
Copy link
Author

@stmklive the issue is not limited to any single entry. @KKSzymanowski is correct, the behaviour of php and the generated query is what is causing the bug. It appears that the pivot table and the roles table are being merged, which is why the role_id and the user_id is appearing twice (once in the array and once in the pivot key of the array).

@srmklive
Copy link
Contributor

srmklive commented Dec 6, 2016

@cabalopolis The issue which @KKSzymanowski alluded to was that eloquent shouldn't select the pivot table id column, I agree with that. The reason I went back & forth with you was the issue with data representation you listed, which I couldn't replicate.

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

5 participants