framework: Wrong Eloquent model ID after query with JOIN

I have to say, I agree with @brenelz silently overwriting a column value is unexpected behaviour. An ambiguous column name exception should be thrown in this case. Or the joined to table has priority and doesn’t loose its values to overwriting.

Doing neither seems counter intuitive.

Should this not at least throw an ambiguous column error? Its kind of weird it just uses the wrong id and keeps going.

It’s because of your join the contacts.id is overwriting the documents.id because they have the same column name. Use a select clause.

Or use ->select(‘documents.*’)

solution is ->get(‘documents.*’) in fetch, thanks!

There is a simlpe fix for this:

User::select('joined_table.*', 'users.*')

This way, users fields will overwrite joined table fields

@vedmant I did find that option eventually, however I still believe this is a bug and should be fixed. When you don’t make the call to select, it’s overwriting a model’s value in the code such that it does not match the real value that is stored in the database.

As a developer, I expect it to leave my primary model’s values as representative of what’s in the database when I don’t include a call to the select method. Why is it defaulting to the joined table’s ID column value when you’re selecting from a completely different model (i.e. defaulting to roles.id when you’re querying the User model)? It’s an unexpected behavior that makes no sense and should not be occurring.

The fact that there is ‘an easy fix’ does not solve the problem. If it only caused errors that would be one thing but this returns bad data!!! The last thing you need when working with data is have it be wrong.

“Eloquent” is anything but.

A great explanation why this issue occurs:

From: https://stackoverflow.com/a/19144466/3553367

---

If you run this query in phpMyAdmin you should probably be able to see that the result contains multiple columns by the name “id”. When PHP parses the query result to a associative array or object, keys must be unique! If keys are colliding, the last column will be used!

Example:

SQL result:

id    user_id    name    id    name    company_id
1     2          Camp1   2     Pelle   1

PHP result:

array (size=1)
  0 => 
    object(stdClass)[131]
      public 'id' => string '2' (length=1)
      public 'user_id' => string '2' (length=1)
      public 'name' => string 'Pelle' (length=5)
      public 'company_id' => string '1' (length=1)

To solve this you could add a select clause to only select the campaign columns:

Campaign::select('campaigns.*')
    ->join('users', 'users.id', '=', 'campaigns.user_id')
    ->where('users.company_id', '=', Auth::user()->company->id)
    ->where('campaigns.id', '=', Input::get('id'))
    ->first();

今天也遇到了，通过 selec t指定字段解决了

@gileneusz Experienced developers consider this unexpected behavior, too. They’ve made it clear they won’t fix it, though, which is why I avoid Laravel like the plague. If you want a better maintained framework, look toward Symfony or, if you really like Laravel, consider swapping Eloquent out for Doctrine in your application build.

@architech99 It actually doesn’t add id column to select query unless you specifically add it with select(). By default it selects all columns:

select * from `users` where `id` = '123'

Also for joins it basically doing nothing, this is how my query with join looks:

select * from `hotels` inner join `hotel_translations` as `t` on `hotels`.`id` = `t`.`hotel_id` and `t`.`locale` = 'en' order by `id` asc limit 10 offset 0

This approach allows maximum flexibility, and there is no way Eloquent can know that it got id field from hotel_translations table instead of hotels.

This should really be fixed in my opinion.

It is very easy to forget the ->select() call or overlook that a colleague developer forgot.

Yes, in an ideal world everything is TDD’d. But every 9 till 5 labour developer knows that this is not the case in reality.

But even if you would have a test, a (less experienced?) developer will more then likely start with something like:

$group = factory(Group::class)->create();
factory(User::class)->create(['group_id' => $group]);

$user = User::join('groups', 'groups.id', '=', 'users.group_id')->first();
$this->assertEquals('/users/1', route('users', $user));

Which will pass, since both id’s are 1.

An experienced developer might see this, and will first create a dummy user or group, so there are different id’s. But that same developer will probably also see the danger of not having a select added to the join.

In the last 5 years this might happen to me 3 or 4 times, and yes the fix is easy to implement.

But the discussions with clients when this happens are huge, since it is just hard to explain why a customer sees data from another customer because of this unexpected behaviour. The damage has already been done.

Please don’t let this go by the argument of :

• just add a select
• this is how mysql works
• it is not a brute force or exploitable weakness of the framework

Maybe even consider to - at least - make sure primary keys from the main select are never overwritten by data from joined columns.

I think this will solve most of problems related to this issue, without having a lot of backwards compatibility.

I hope that since the results of leaving out the select method on an Eloquent join query can be very tricky, you might take a look one more time on this issue.

I know most active devs here on the git repos, are more then average experienced and talented and you all 100% TDD your projects and breathe Eloquent.

In the real life this is just not always the case and leaving this unexpected outcome as a developer problem, while it can be fixed, is a missed chance.

@architech99 That’s not Eloquent problem at all, Mysql rewrites id of one table form id of joined table without throwing any errors. To make Eloquent pull exactly id from the table it creates Model it will require to append something like select *, users.id as user_model_id which is quite controversial solution, any other solution will require exact table columns mapping, you can use Doctrine2 for this as it generates queries with exact mappings:

 SELECT t0.id AS id_1, t0.full_name AS full_name_2, t0.username AS username_3, t0.email AS email_4, t0.password AS password_5, t0.roles AS roles_6 FROM symfony_demo_user t0 WHERE t0.id = ?

@vedmant I understand how it’s pulling data together, but it’s still overwriting a value unexpectedly. And that’s the problem. When you’re hydrating the model, it shouldn’t overwrite a value on a model that’s already there just because it found another one with the same name. But it is. And that’s the bug.

I’m not sure how changing someone’s data results in unexpected ways is being “flexible”.

@architech99 I think it’s done this way to make Eloquent more flexible, Eloquent doesn’t store full list of columns (unlike Doctrine or Kohana ORM) so it can’t map DB columns to appropriate Model attributes.

I’m not sure if Mysql strict mode can throw errors in the case of join columns overlap, but there is certainly no way Eloquent can detect this overlaps without having exact data about all table columns.

@nerdo To answer your questions about L5.4, it has not been fixed. I’m upgrading a client to L5.4 currently and just ran into this. I ran into it L4.2 and it’s been an on-going issue for quite some time.

IMHO #4962 (comment) still applies. I think https://laravel.com/docs/master/queries#joins could use a warning there about this.

Why does laravel need extra meta information? It already has the table name of the model (otherwise this wouldn’t work in the first place), so the default select should be changed to table_name.*.

Or when there are joins and the select method is not used, make sure the main model table_name is selected last, so those values have precedence, instead of the joined table as it is now.

->select('joined_table_name.*', 'main_table_name.*')

I think this gives the best backwards compatibility and a sensible default when joining.

Solution from @Grey2k worked when I put it in as array.

solution is ->get([‘documents.*’])

I agree with Samyoul and the others who upvoted his post.

I ran into this issue when using a belongsToMany relation. The pivot table had it’s own id that was overwriting the model’s silently. Doing ->select(‘models.*’) works, but, at least in the case of the relation, it seems like it should do this automatically. I’m using L5.3 btw. Not sure if it’s changed in 5.4

Old issue, but this is still a problem. If you’re doing regular Query Builder stuff, sure. However, I ran into this inside of a Scope. In Laravel Nova, I’m creating a boolean filter that allows someone to select multiple locations, which are pivot tables.

function scopeLocations($query) { $query->join(‘pivot_table’, ‘table1.id’, ‘=’, ‘pivot_table.record_id’) …

Overwrites all of the Nova resource IDs when you use the filter. So clicking “preview” or “edit” gives you 404 because it is the wrong resource number. I’ve fixed it with:

function scopeLocations($query) { $query->select('my_main_resource.*)->join(‘pivot_table’, ‘table1.id’, ‘=’, ‘pivot_table.record_id’) …

Does work, but, not sure if this is the right approach here…

It is also a matter of how the code reads and what it implies.

Take this example from the docs:

$users = DB::table('users')
    ->leftJoin('posts', 'users.id', '=', 'posts.user_id')
    ->get();

foreach($users as $user) {
    dd($user->id);
}

Please thumb down if you really would expect that $user->id will return the posts.id 😉

I implemented a potential fix for this: https://github.com/laravel/framework/pull/29519

@danabrey good luck getting the community to implement anything meaningful related to this. I moved on to Symfony and have slowly been moving all my clients to it and using doctrine. Projects are moving along much faster without finding this random issues.

Is merging values from one type of entity onto another in the event of a clash ever going to be helpful behaviour? Could there be a check in place that throws an exception if a clash of column names exists, so that there’s more clarity as to what is happening in this clearly confusing situation?