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

Add testConditionOnQueriedField for WhereHasConditionsDirective #1472

Draft
wants to merge 2 commits into
base: master
Choose a base branch
from

Conversation

Jofairden
Copy link

  • 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?

@spawnia
Copy link
Collaborator

spawnia commented Jul 14, 2020

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

@Jofairden
Copy link
Author

Jofairden commented Jul 14, 2020

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.

@spawnia
Copy link
Collaborator

spawnia commented Jul 15, 2020

@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

@Jofairden
Copy link
Author

Jofairden commented Jul 15, 2020

@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

@spawnia
Copy link
Collaborator

spawnia commented Jul 16, 2020

@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 spawnia marked this pull request as draft July 17, 2020 09:35
@Jofairden
Copy link
Author

$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
Copy link
Author

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?

@spawnia spawnia added the bug An error within Lighthouse label Nov 27, 2020
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug An error within Lighthouse
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants