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

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

Closed
illambo opened this issue Feb 7, 2023 · 5 comments · Fixed by #953

Comments

@illambo
Copy link
Contributor

illambo commented Feb 7, 2023

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

@illambo illambo added the bug label Feb 7, 2023
@mfn
Copy link
Collaborator

mfn commented Feb 7, 2023

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.

@illambo
Copy link
Contributor Author

illambo commented Feb 8, 2023

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

@crissi
Copy link
Contributor

crissi commented Feb 8, 2023

Does the https://github.com/rebing/graphql-laravel#eager-loading-relationships always-field mentioned here, fixes it for you?

or the query param?
https://github.com/rebing/graphql-laravel#type-relationship-query

@illambo
Copy link
Contributor Author

illambo commented Feb 8, 2023

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
Copy link
Collaborator

mfn commented Feb 8, 2023

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

@mfn mfn linked a pull request Mar 5, 2023 that will close this issue
12 tasks
@mfn mfn closed this as completed in #953 Mar 5, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging a pull request may close this issue.

3 participants