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

Fix ambiguous column id when using Relation instead of Builder #283

Merged
merged 6 commits into from
May 19, 2021
Merged

Fix ambiguous column id when using Relation instead of Builder #283

merged 6 commits into from
May 19, 2021

Conversation

fabio-ivona
Copy link
Contributor

@rappasoft I noticed a slight issue when using bulk actions with a many to many Relation to generate data:

if the pivot table has an id field, the query will generate an integrity constraint violation when using the id column to filter data:

SQLSTATE[23000]: Integrity constraint violation: 1052 Column 'id' in field list is ambiguous (SQL: select `id` from `students` inner join `subscriptions` on `students`.`id` = `subscriptions`.`student_id` where `subscriptions`.`edition_id` = 3 and `students`.`id` in (3) order by `students`.`last_name` asc)

this happens when executing the WithBulkActions::selectedRowsQuery() and WithBulkActions::selectedKeys() methods and can be avoided by letting laravel to qualify the primaryKey column:

$this->primaryKey //id

would become:

$query->qualifyColumn($this->primaryKey) //students.id

this is a very rare case, but could happen

@fabio-ivona fabio-ivona marked this pull request as draft May 14, 2021 14:22
Took 13 minutes
@fabio-ivona
Copy link
Contributor Author

@rappasoft seems that automatic tests are cancealed by some github policy

@@ -65,7 +65,7 @@ public function resetBulk(): void
public function selectedRowsQuery()
{
return (clone $this->rowsQuery())
->unless($this->selectAll, fn ($query) => $query->whereIn($query->qualifyColumn($this->primaryKey), $this->selected));
Copy link
Owner

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Was there a reason you removed this one? Also, is there qualifyColumn documentation?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

damn, I forgot to undo it, it was a try because I noticed that the test I wrote was succeeding with the old code. so I put the PR in draft while was writing a better test

just fixing it

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

anyway, qualifyColumn is not documented in official Laravel documentation, it was released with laravel 5.5.29:

https://laravel-news.com/laravel-5-5-29-released

but it simply does this:

public function qualifyColumn($column)
{
  return $this->model->qualifyColumn($column);
}

and Model::qualifyColumn() adds the table name before the column name:

public function qualifyColumn($column)
{
    if (Str::contains($column, '.')) {
        return $column;
    }

    return $this->getTable().'.'.$column;
}

@fabio-ivona
Copy link
Contributor Author

@rappasoft I just rewrote my test table and RelationshipDatatableComponentTest in order to have an example that gives an error without this PR and it is fixed by adding qualifyColumn()

@fabio-ivona fabio-ivona marked this pull request as ready for review May 14, 2021 21:16
Took 5 minutes
@rappasoft rappasoft added the Awaiting Next Release Currently merged into development awaiting a release to master label May 15, 2021
@rappasoft rappasoft mentioned this pull request May 19, 2021
@rappasoft rappasoft merged commit c65ebb7 into rappasoft:master May 19, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Awaiting Next Release Currently merged into development awaiting a release to master
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants