lighthouse icon indicating copy to clipboard operation
lighthouse copied to clipboard

Add testConditionOnQueriedField for WhereHasConditionsDirective

Open Jofairden opened this issue 5 years ago • 7 comments

  • [x] Added or updated tests
  • [ ] Documented user facing changes
  • [ ] Updated CHANGELOG.md

See #1470

Changes

Added a test for this issue

Breaking changes None

Details I cannot seem to run any tests at all, I just keep getting a bunch of errors. Care to tell me how the tests can be setup locally?

Jofairden avatar Jul 14 '20 09:07 Jofairden

I cannot seem to run any tests at all, I just keep getting a bunch of errors. Care to tell me how the tests can be setup locally?

https://github.com/nuwave/lighthouse/blob/master/CONTRIBUTING.md#how-to-contribute-to-lighthouse

spawnia avatar Jul 14 '20 09:07 spawnia

Ok well I seem to have identified where the issue comes from, also explaining why there's no issue right now in Lighthouse after adding the test.

We are coupling players to teams with an linking table called player_team, the resulting query when using the whereCondition looks like:

select count(*) as aggregate 
from `teams` 
where EXISTS (
	select * from `players` 
	inner join `player_team` 
	on `player_team`.`player_id` = `players`.`id` 
	where `teams`.`id` = `player_team`.`team_id` 
	AND (`ID`  = 120)
)

Since it's literally just taking the enum value as the column for the condition we get an ambiguous error, because what it should be checking is AND (players.id = 120). I tried prefixing the column but the engine doesn't allow that, it does seem that this should be supported by default though. If I prefix the final AND clause myself and run it in my SQL editor it works fine.

Jofairden avatar Jul 14 '20 10:07 Jofairden

@Jofairden so you are not using joins instead of plain Eloquent relations? Automatically prefixing column names when joins are used is a long standing issue within Laravel.

I got my hands dirty and tried to fix this before, but found that the rabbit hole goes quite deep: https://github.com/spawnia/laravel-framework/tree/prefer-original-table-columns-on-join

spawnia avatar Jul 15 '20 07:07 spawnia

@Jofairden so you are not using joins instead of plain Eloquent relations? Automatically prefixing column names when joins are used is a long standing issue within Laravel.

I got my hands dirty and tried to fix this before, but found that the rabbit hole goes quite deep: https://github.com/spawnia/laravel-framework/tree/prefer-original-table-columns-on-join

The relationship that's being queried here is a hasManyThrough relationship, specifically if we look at players on team: (PlayerTeam = pivot)

    public function players()
    {
        return $this->hasManyThrough(
            Player::class,
            PlayerTeam::class,
            "team_id",
            "id",
            "id",
            "player_id");
    }

Anyways, so are you saying this is more or less an issue in Laravel and less so in Lighthouse / our app ? That'd be quite an issue for us

Jofairden avatar Jul 15 '20 08:07 Jofairden

@Jofairden you can try and replicate this issue without Lighthouse using the query builder yourself. I believe that this would produce a similar error:

$team->players()->where('id', 123)

spawnia avatar Jul 16 '20 17:07 spawnia

$team->players()->where('id', 123)

Yup I can confirm this now:

=> Illuminate\Database\Eloquent\Relations\HasManyThrough {#4223}
>>> Team::query()->find(1)->players()->where('id', 1)->first()
Illuminate/Database/QueryException with message 'SQLSTATE[23000]: Integrity constraint violation: 1052 Column 'id' in where clause is ambiguous (SQL: select `players`.*, `player_team`.`team_id` as `laravel_through_key` from `players` inner join `player_team` on `player_team`.`player_id` = `players`.`id` where `player_team`.`team_id` = 1 and `id` = 1 limit 1)'

Jofairden avatar Jul 17 '20 10:07 Jofairden

Having the same for a polymorphic relation:

    public function players()
    {
        return $this->morphedByMany(Player::class,
            'participant',
            'participants',
            'fixture_id')
            ->withPivot('location');
    }

When I prefix the first one it works, but the filters aren't doing this: Team::query()->find(1)->players()->where(players.id', 1)->first()

Any ideas on that?

Jofairden avatar Jul 17 '20 10:07 Jofairden