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] Only select related columns by default in CanBeOneOfMany::ofMany #39307

Merged
merged 2 commits into from
Oct 22, 2021
Merged

[8.x] Only select related columns by default in CanBeOneOfMany::ofMany #39307

merged 2 commits into from
Oct 22, 2021

Conversation

bastien-phi
Copy link
Contributor

@bastien-phi bastien-phi commented Oct 22, 2021

#39187 introduced an issue with selected columns of related :

dump($user->latestLogin->getAttributes());

/*
array:4 [
  "id" => "2"
  "user_id" => "1"
  "deleted_at" => null
  "id_aggregate" => "2"
]
*/

The id_aggregate column should not be selected and can cause issues.

The problem comes from the sql query :

select * 
from "logins" 
inner join (
    select MAX("id") as "id_aggregate", "logins"."user_id" 
    from "logins" 
    --- more sql
) as "latestOfMany"
on "latestOfMany"."id_aggregate" = "logins"."id" 
--- more sql

As the query selects *, id_aggregate is also selected.

Forcing the query to select "logins".* solves the problem.

select "logins".* 
from "logins" 
inner join (
    select MAX("id") as "id_aggregate", "logins"."user_id" 
    from "logins" 
    --- more sql
) as "latestOfMany"
on "latestOfMany"."id_aggregate" = "logins"."id" 
--- more sql
dump($user->latestLogin->getAttributes());

/*
array:3 [
  "id" => "2"
  "user_id" => "1"
  "deleted_at" => null
]
*/

@bastien-phi
Copy link
Contributor Author

fixes #39306

@taylorotwell
Copy link
Member

Can @cbl give some feedback here. I feel like I'm shooting in the dark merging all these PRs that all seem to introduce additional problems.

@taylorotwell taylorotwell merged commit a6ef21a into laravel:8.x Oct 22, 2021
@cbl
Copy link
Contributor

cbl commented Oct 22, 2021

@taylorotwell I think tests on complicated features like this are everything.

I think this feature is pretty well tested and ~99% of all use cases work when the tests pass, however, issues occur from time to time due to different behavior of sql drivers. So we should consider mapping all tests over other common sql drivers such as mysql and PostgreSQL as well.

Otherwise we would just rely on people saying that this works... 😅

If wanted I will create a pr for mapping the tests over other sql drivers.

@driesvints
Copy link
Member

If wanted I will create a pr for mapping the tests over other sql drivers.

We can do that for unit tests for now. For integration tests, I still have it on my list to set up different builds for PostgreSQL, MSSQL & MySQL.

@bastien-phi
Copy link
Contributor Author

Thanks @taylorotwell @driesvints and @cbl for your hard work on that.

I feel really sorry for the trouble I caused. I hope there won't be any new issue...

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.

None yet

4 participants