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

[8.x] Fix qualified column in withAggregate #35061

Merged
merged 1 commit into from
Nov 2, 2020
Merged

[8.x] Fix qualified column in withAggregate #35061

merged 1 commit into from
Nov 2, 2020

Conversation

dbakan
Copy link
Contributor

@dbakan dbakan commented Nov 2, 2020

This PR fixes a QueryException when using the new Builder features withAggregate, withMin, withSum, etc.

When using User::withMin('roles', 'id') on a belongsToMany relation the current implementation creates a query like

select users.*, (select min(`id`) from roles inner join role_user on ...) as ...

which can cause an SQL error:

SQLSTATE[23000]: Integrity constraint violation: 1052 Column 'id' in field list is ambiguous

This PR passes the $column through $relation->getRelated()->qualifyColumn($column) to ensure fully qualified names:

select users.*, (select min(`roles`.`id`) from roles inner join role_user on ...) as ...

@khalilst
Copy link
Contributor

khalilst commented Nov 2, 2020

@dbakan
I have test your idea and there was not SQL error:

>>> Product::withMin('comments', 'id')->get()
=> Illuminate\Database\Eloquent\Collection {#3241
     all: [
       App\Models\Product {#4119
         id: 1,
         name: "fSbKRoIpBl",
         created_at: "2020-11-02 09:48:39",
         updated_at: "2020-11-02 09:48:39",
         comments_min_id: 1,
       },
       App\Models\Product {#3249
         id: 2,
         name: "TBpEYufkJI",
         created_at: "2020-11-02 09:48:39",
         updated_at: "2020-11-02 09:48:39",
         comments_min_id: 3,
       },
       App\Models\Product {#4185
         id: 3,
         name: "iBlkWmyfU3",
         created_at: "2020-11-02 09:48:39",
         updated_at: "2020-11-02 09:48:39",
         comments_min_id: 7,
       },
       App\Models\Product {#3557
         id: 4,
         name: "c0o1InKesx",
         created_at: "2020-11-02 09:48:47",
         updated_at: "2020-11-02 09:48:47",
         comments_min_id: 5,
       },
     ],
   }

This is the raw SQL:

>>> Product::withMin('comments', 'id')->toSql()
=> "select `products`.*, (select min(`id`) from `comments` where `products`.`id` = `comments`.`commentable_id` and `comments`.`commentable_type` = ? limit 1) as `comments_min_id` from `products`"

@dbakan
Copy link
Contributor Author

dbakan commented Nov 2, 2020

@khalilst There is no join happening in your SQL.

// migration to create the role_user table:
Schema::create('role_user', function (Blueprint $table) {
    $table->id(); // this will be the ambiguous column
    $table->foreignId('user_id');
    $table->foreignId('role_id');

    $table->unique(['user_id', 'role_id']);
});

// relation on User model
public function roles()
{
    return $this->belongsToMany('App\Models\Role');
}
>>> User::withMin('roles', 'id')->toSql();
select `users`.*, (select min(`id`) from `roles` inner join `role_user` on `roles`.`id` = `role_user`.`role_id` where `users`.`id` = `role_user`.`user_id` limit 1) as `roles_min_id` from `users`

I know that you might drop the column role_user.id in this example, but of cause it also happens with other columns in the pivot table like created_at.

@khalilst
Copy link
Contributor

khalilst commented Nov 2, 2020

@dbakan Right, thanks for clarifying.

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

Successfully merging this pull request may close these issues.

3 participants