rebing/graphql-laravel

Using getRelations of SelectFields involves only selecting the requested fields for relationships

illambo opened this issue · 5 comments

Versions:

  • graphql-laravel Version: 8.5.0
  • Laravel Version: 9.48.0
  • PHP Version: 8.1.14

Description:

Taking as an example (taken from the docs):

public function resolve($root, array $args, $context, ResolveInfo $info, Closure $getSelectFields)
{
     /** @var SelectFields $fields */
    $fields = $getSelectFields();
    $select = $fields->getSelect();
    $with = $fields->getRelations();

    $users = User::select($select)->with($with);

    return $users->get();
}

Using $select = $fields->getSelect(); the developer can filter or not only for the fields requested by the query but this logic it is not applicable at the relationship level, $with = $fields->getRelations(); this filter is forced (you do not have the option to select "*").

I apologize if I may not have expressed the problem well.

When nested relations are extracted within the query, only the fields requested for the relation in these are extracted. This leads to problems if there are calculated fields that rely on missing fields from the nested relationship, which are not retrieved by the query and therefore are not available during field processing, generating an incorrect result.

We would like to have the possibility to be able to select the entire recordset for the required relations, maybe with an extra parameters in the getRelations method ?
Having to "play" with the always attribute it doesn't seem like the correct solution to us because involves redundancy of the eloquent model logic inside graphql.

What do you think ? (We are aware of the related discussion)

Thank you

mfn commented

Sorry, same answer as in #979 . I try to keep SelectFields alive as best as possible, but TBH I've a hard time maintaining and (and also no fun doing so) and I also never truly used it, dataloaders are preferred.

Ok, as soon as I can I elaborate tests and pr.

Hi @crissi, I played with both ways (always field and type relationship query) to get around the problem but in the end it all turned out to be too complicated and logically incorrect (IMO the graphql layer must not be aware of the fields needed for any computed field logics on the eloquent side), so I chose another solution, extend SelectFields class and overwrite the method handleFields.

I disabled this check, so instead of this:

// If parent type is an union or interface we select all fields
// because we don't know which other fields are required
if (is_a($parentType, UnionType::class) || is_a($parentType, \GraphQL\Type\Definition\InterfaceType::class)) {
$select = ['*'];
}

I just left this:

$select = [$parentTable.'.*'];

Doing so I "solved" the problem for me, I hope I was clear and helpful.

mfn commented

I just remembered this other PR #277, talking about something different but targeting the same area and logic (wasn't merged in the end).