Make it possible to modify eager-loading queries by extending SelectFields

[turgutsaricam]

Summary

This project is a tremendous help. However, when it comes to eagerly loading relations, I always end up in examining SelectFields class to figure out how to implement the functionality I want. Most of the time, I simply need to modify the eager-loading query, created in SelectFields class, by considering the requested fields. I probably spent more than 100 hours in total trying to hack the package to implement what I need. The reason I spent this much time is because SelectFields class is not extendible. For example, #510 talks about eagerly loading counts, which is not possible currently. The reason why I created this pull request is that I need to eagerly load counts as well. There are also other issues related to SelectFields in this repository. For example, #875 mentions a need for a custom behavior. #806 wants to modify the behavior of eager-loading function. #799 was a nice attempt, which made it possible to extend SelectFields and provide a custom SelectFields class to be used by the package, but it is not enough for many use-cases. The class is quite complex, as also stated here. The complexity of the class partially comes from using static methods too. This pull request partially addresses these issues.

Here is a summary of the changes made in this pull request:

• Use instance methods instead of static methods. By this way, we do not need to pass certain variables as parameters to all the methods.
• Store constructor parameters in instance variables. Their getters are added as well. With this, a child class can retrieve the constructor parameters from any method, when overriding the methods. Also, we do not need to pass $queryArgs and $ctx variables. The methods that need them simply retrieve them via the getters. This reduces the complexity of the class.
• Make it possible to modify the eager-loading queries. This is the main contribution of this pull request. This will make it possible to customize the behavior of eager-loading.
• Do not override the columns added to the select statement of the queries by the custom queries. This lets the custom queries define custom columns.

There are many eager-loading methods in Laravel. We might also need to customize the eager-loading behavior to use them as well. With the changes in this pull request, everybody can implement their own behavior. Below is an example of extending SelectFields to eagerly load counts:

<?php

namespace App\GraphQL\Helpers\SelectFields;

use App\GraphQL\Fields\Pagination\CustomPaginationType;
use GraphQL\Type\Definition\Type as GraphqlType;
use Illuminate\Database\Eloquent\Relations\Relation;
use Illuminate\Support\Str;
use Rebing\GraphQL\Support\SelectFields;

class AppSelectFields extends SelectFields {

    // Override the onAfterModifyQuery method to modify the query after select fields and relations 
    // are added to it. Also, by overriding onBeforeModifyQuery, it is possible to modify $with and
    // $select variables that will be added to the query in SelectFields.
    protected function onAfterModifyQuery($query, GraphqlType $parentType, array $with, array $select,
                                          array $requestedFields): void {
        parent::onAfterModifyQuery($query, $parentType, $with, $select, $requestedFields);

        if ($parentType instanceof CustomPaginationType) {
            $type = $parentType->getUnderlyingType();
            $actualRequestedFields = data_get($requestedFields, 'fields.items.fields') ?? [];
            $this->eagerlyLoadCounts($query, array_keys($actualRequestedFields), $type);
        }
    }

    /*
     *
     */

    /**
     * @param Relation    $query
     * @param string[]    $requestedFields
     * @param GraphqlType $type
     * @return void
     * @noinspection PhpMissingParamTypeInspection
     */
    protected function eagerlyLoadCounts($query, array $requestedFields, GraphqlType $type) {
        foreach($requestedFields as $fieldName) {
            if (!str_ends_with($fieldName, '_count')) continue;

            $exploded = explode('.', $fieldName);
            $this->eagerlyLoadCount($query, $exploded[count($exploded) - 1], $type);
        }
    }

    /**
     * @param Relation    $query
     * @param string      $fieldName
     * @param GraphqlType $type
     * @return void
     */
    protected function eagerlyLoadCount($query, string $fieldName, GraphqlType $type): void {
        $field = $type->findField($fieldName);
        if (!$field) return;

        $config = $field->config;
        // 'count' is a custom config that I added to customize the behavior
        if (($config['count'] ?? true) !== true) return;

        // 'relation' is a custom config that I added to customize the behavior
        $relationName = $config['relation'] ?? Str::replace('_count', '', $fieldName);
        $query->withCount($relationName);
    }

}

Switching to instance methods from static methods opens up the possibility of adding more hooks like onBeforeModifyQuery and onAfterModifyQuery. In the future, I expect other pull requests that add other hooks when developers come across other use-cases.

The changes I made are breaking changes for the applications that extend SelectFields. Other than that, this is non-breaking. I did not add any tests. The existing tests all pass. If this pull request is intended to be merged, I can add the tests with your directions. I did not update README.md because I did not see any guidance on how to extend SelectFields.

---

Type of change:

• Bug fix (non-breaking change which fixes an issue)
• New feature (non-breaking change which adds functionality)
• Breaking change (fix or feature that would cause existing functionality to not work as expected)
• This change requires a documentation update
• Misc. change (internal, infrastructure, maintenance, etc.)

Checklist:

• Existing tests have been adapted and/or new tests have been added
• Add a CHANGELOG.md entry
• Update the README.md
• Code style has been fixed via composer fix-style

[mfn]

I've spoken out multiple times against the use of SelectFields due to all the problems it has and you touched quite nicely upon this topic and probably more.

I exclusively use the dataloader approach and never use(d) SelectFields and never will. I think it's an battle/approach which cannot really be won and/or requires too much time to get right and maintain, which I'm not able to provide.

Your approach might be better, make things correct, better for the future but I just need to look at a few lines of the PR and realize it's major breaking change and would require bump of a new major version; like right after I realized 8.0.0 we would need to jump to 9.0.0 alone due to this PR 😅

---

I'm trying to navigate the muddy waters of this part of the library and find a safe harbor and I bet you invested already more time then I ever did into it.

I wonder if SelectFields can be made independent in a way it would be decoupled from this library without too much effort while still being backwards compatible but also be replaceable with any other implementation.

[crissi]

I use the selectfields approach for 95% of use cases and it makes very fast and simple to write most queries.

I do agree it is a bit coupled.

Crreating it own package is not a bad idea. More like a plugin is good idea.

One way of decoupling/organizing it could be add all select options under one key, so instead of alias, always, is_relation being it own "root" key, we would add it to selectable, maybe also make it possible to be set as an DTO (so we validate the params are correct), or maybe transfed to DTO. So you can't set wrong values, this could of course be done with all params.

'posts' => [
      'type'          => Type::listOf(GraphQL::type('Post')),
      'description'   => 'A list of posts written by the user',
      // Now this will simply request the "posts" column, and it won't
      // query for all the underlying columns in the "post" object
      // The value defaults to true
     'selectable' => [
             'is_relation' => false,
             'always' => ['key']
    ],
  ]

Then we could start deprecating the keys in the root.
I would be up for making a pr for this!

[turgutsaricam]

@mfn Sorry that I did not reply earlier. After you pointed out the data loader approach, I started checking it out and implementing it into my project. I see how it is a better approach, given that it was originated at Facebook. I actually started to move away from SelectFields after reading your comment. Actually, I could have implemented the data loader pattern earlier, if this repository encouraged the usage of that pattern instead of SelectFields. This repository is too intertwined with SelectFields. Upon reading README.md, anybody who starts to use this repository will be incentivized to use SelectFields. There is not enough guidance to using the data loader pattern. Moreover, one should do quite a bit of research to understand that pattern and use it in their projects. For example, overblog/dataloader-php does not work with webonyx/graphql-php currently. I tried to use it, but ended up with fatal errors. After that, I decided to write my own data loader implementation that works nicely with webonyx/graphql-php. While doing that, I did a lot more research to understand how to implement the pattern. My point is, there is not a straightforward way to use the data loader pattern with this repository. Moreover, the README.md is full of SelectField references. Many methods come with a parameter that is used to retrieve a SelectFields instance. As @crissi said, I use SelectFields approach in about 95% of the cases as well. This is because this repository encourages the use of it. Hence, if SelectFields is not the right way, it should be discouraged. If it is not discouraged, new users of this library will use SelectFields instead of the data loader pattern, which will make it much harder to move away from SelectFields. In my opinion, if SelectFields is not the right way, its usage should be discouraged, and more guidance on how to use the data loader pattern should be in README.md.

Apart from these, if you agree, I can make another PR that does not break backwards compatibility yet allows to override the following eager-loading function:

  return function ($query) use ($with, $select, $customQuery, $requestedFields, $ctx): void {
      if ($customQuery) {
          $query = $customQuery($requestedFields['args'], $query, $ctx) ?? $query;
      }

      $query->select($select);
      $query->with($with);
  };

Basically, moving the creation of this function to its own protected static method will do the job for me and probably many others. I am currently overriding the entire SelectFields::getSelectableFieldsAndRelations() method just to override the eager-loading function returned from that method. Overriding just the returned function would make it more maintainable.

[Artem-Schander]

Hi everyone,
It is nice to see that there is movement in this regard. Some time ago I also started to tackle the mentioned issues in SelectFields. Unfortunately I was withdrawn from this project (other stuff is more important atm).

Is there currently any progress on this? I would love to see this in the next release :)

[mfn]

Is there currently any progress on this?

Not from my side currently. Working on anything SelectFields related is not something which motivates me (except maybe: moving it out so it can be independently worked on).

[turgutsaricam]

I completely moved away from SelectFields to the data loader pattern. So, I do not work on SelectFields anymore.

[Artem-Schander]

Hm.. too bad. I thought I could avoid to refactor our project.

[mfn]

I completely moved away from SelectFields to the data loader pattern. So, I do not work on SelectFields anymore.

Closing this PR therefore; it can always by picked up by someone motivated 🤞🏼