Avoid calling model accessor twice through default resolver

[canatufkansu]

Describe the bug

When I call a model accessor from GraphQL query Lighthouse calls it twice. When I call it from outside of Lighthouse it is being called only once.

Expected behavior/Solution

When I call the accessor it should only be called once.

Steps to reproduce

1. Create a simple model and add accessor into the model, I have added dump() to understand how many time this accessor is being called.

namespace App;

use Illuminate\Database\Eloquent\Model;

class Brand extends Model
{
    public function getTestAttribute() {
        dump('TEST');
        return 'test';
    }
}

2. Define Brand type in GraphQL

type Brand {
    id: ID!
    franchise_id: Int
    country_id: Int
    name: String!
    website: String
    created_at: DateTime
    updated_at: DateTime
    test: String
}

3. Add GraphQL query to the schema

brand(id: ID @eq): Brand @guard(with: ["api"]) @find(model: "App\\Brand")

4. Call it the query by adding test attribute

query($id:ID) {
    brand (id: $id) {
        id
        test
    }
}

5. Examine the result, in the query response I see result of the dump('TEST') two times and json response itself.

"TEST"
"TEST"
{"data":{"brand":{"id":"5","test":"test"}}}

6. Change the model accessor to return null rather than a string

namespace App;

use Illuminate\Database\Eloquent\Model;

class Brand extends Model
{
    public function getTestAttribute() {
        dump('TEST');
        return null;
    }
}

7. Examine this time result of the dump('TEST') only one and json response itself

"TEST"
{"data":{"brand":{"id":"5","test":null}}}

8. This time I will try to access the accessor outside of Lighthouse, to do that first restore model accessor to return string again

namespace App;

use Illuminate\Database\Eloquent\Model;

class Brand extends Model
{
    public function getTestAttribute() {
        dump('TEST');
        return 'test';
    }
}

9. Try to access model's accessor trough Laravel's web.php, to do that add the following code at the top of the web.php

Route::get('/', function () {
    return \App\Brand::first()->test;
});

10. Try it out by going to localhost from the browser, I can see from the response that this time dump('TEST') only called once

"TEST"
test

Lighthouse Version
v4.18.0

[spawnia]

That behaviour comes from the webonyx/graphql-php default field resolver. webonyx/graphql-php#760 attempts to improve this.

[brunobg]

@canatufkansu while that fix is not accepted into graphql-php, here's a dirty workaround. Add to your model class:

    /**
     * Determine if the given attribute exists.
     * TEMPORARY FIX FOR https://github.com/webonyx/graphql-php/issues/759
     *
     * @param  mixed  $key
     * @return bool
     */
    public function offsetExists($key)
    {
        if (!$key) {
            return false;
        }

        // If the attribute exists in the attribute array or has a "get" mutator we will
        // get the attribute's value. Otherwise, we will proceed as if the developers
        // are asking for a relationship's value. This covers both types of values.
        if (array_key_exists($key, $this->attributes) ||
            array_key_exists($key, $this->casts) ||
            $this->hasGetMutator($key) ||
            $this->isClassCastable($key)) {
            return true;
        }

        // Here we will determine if the model base class itself contains this given key
        // since we don't want to treat any of those methods as relationships because
        // they are all intended as helper methods and none of these are relations.
        if (method_exists(self::class, $key)) {
            return false;
        }
        return true;
    }

[canatufkansu]

Thank you very much, I was trying to figure out the issue for a while. 👌

[spawnia]

Let's keep this open, depending on the outcome of the other PR we might change something in Lighthouse.

[spawnia]

Given that webonyx/graphql-php most likely will not change the default resolver implementation (see webonyx/graphql-php#760 (comment)), and I don't even think it should, we can look into providing a custom default resolver for Lighthouse.

I am actually delighted we got this far with the default. However, the most commonly used data structure in Lighthouse are probably Laravel Models, so we can optimize our implementation to fit them better.

[brunobg]

Since both graphql-php and laravel think that it's a feature and not a bug, thanks for wanting to fix this here.

[spawnia]

I think the stance we took in graphql-php was the right call, and Laravel is slow and heavy to implement change. I strongly prefer fixing issues upstream, but sometimes a workaround can be acceptable if the benefit is great enough.

[brunobg]

I understand the position in graphql-php of expecting a sane implementation of ArrayAccess, which is why I didn't argue it further. But the reality is that people often don't write sane implementations and this is the reason for this bug. I think Laravel is wrong, but I also think that graphql-php should play it safe -- oddly I think lighthouse is the least guilty, which is why I didn't post the issue here in the first place...

Is there anything I can do to help this to be fixed in lighthouse ASAP?

[spawnia]

You can add a PR with another default field resolver that checks for instanceof Model and does a more efficient lookup then. We can make it opt-in at first, then turn it on by default in a later PR.

[brunobg]

Let me see if I get this right:

• the current one is lighthouse/src/Schema/ResolverProvider.php, right?
• so I'd check in provideResolver() if $fieldValue->parent instanceof Model and return a new resolver class for that

Is that it?

[spawnia]

We currently use Executor::getDefaultFieldResolver(). We can replace that with a custom function, which then contains the check, using the resolver argument $root.

[HaguilarSertec]

Following the example, posible workarounds I have found for deal with this:

type Brand {
    id: ID!
    franchise_id: Int
    country_id: Int
    name: String!
    website: String
    created_at: DateTime
    updated_at: DateTime
    test: String @field(resolver: "File\\With\\Logic@function")
}

or

type Brand {
    id: ID!
    franchise_id: Int
    country_id: Int
    name: String!
    website: String
    created_at: DateTime
    updated_at: DateTime
    test: String @cache(private: true, maxAge: 172800)
}