Skip to content

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

Merged
taylorotwell merged 1 commit into
laravel:8.xfrom
dbakan:with-aggreate-qualify-column
Nov 2, 2020
Merged

[8.x] Fix qualified column in withAggregate#35061
taylorotwell merged 1 commit into
laravel:8.xfrom
dbakan:with-aggreate-qualify-column

Conversation

@dbakan

@dbakan dbakan commented Nov 2, 2020

Copy link
Copy Markdown
Contributor

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

khalilst commented Nov 2, 2020

Copy link
Copy Markdown
Contributor

@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

dbakan commented Nov 2, 2020

Copy link
Copy Markdown
Contributor Author

@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

khalilst commented Nov 2, 2020

Copy link
Copy Markdown
Contributor

@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