Executor fetches data twice: offsetExists on ArrayAccess

[brunobg]

I'm finding a few duplicated queries on my logs and debugging. I'm using lighthouse-php but the stack trace shows they seem related to graphql-php, so I'm posting them here. I think they have different causes so I'll open multiple issues.

First chapter: I'm using Laravel with an accessor that hits the DB. The query is called twice.

More or less this:

class Stuff extends Model {
    public function getFancyAttribute() {
       return DB::select('SELECT sometable.id FROM sometable');  // some fancy query here
    }
}

I fetch this on graphql, which is like this:

type Stuff {
   id: ID!
   fancy: [Fancytype!]
}

extend type Query {
   stuff(id: String @id): Stuff @find

So when I query I see two selects, caused by calling Model::offsetExists and Model::offsetGet. Here are the relevant stack traces for both calls:

  0 => 
  array (
    'file' => '/server/vendor/laravel/framework/src/Illuminate/Database/Eloquent/Concerns/HasAttributes.php',
    'line' => 473,
    'function' => 'getFancyAttribute',
    'class' => 'App\\Models\\Stuff',
    'type' => '->',
  ),
  1 => 
  array (
    'file' => '/server/vendor/laravel/framework/src/Illuminate/Database/Eloquent/Concerns/HasAttributes.php',
    'line' => 1444,
    'function' => 'mutateAttribute',
    'class' => 'Illuminate\\Database\\Eloquent\\Model',
    'type' => '->',
  ),
  2 => 
  array (
    'file' => '/server/vendor/laravel/framework/src/Illuminate/Database/Eloquent/Concerns/HasAttributes.php',
    'line' => 386,
    'function' => 'transformModelValue',
    'class' => 'Illuminate\\Database\\Eloquent\\Model',
    'type' => '->',
  ),
  3 => 
  array (
    'file' => '/server/vendor/laravel/framework/src/Illuminate/Database/Eloquent/Concerns/HasAttributes.php',
    'line' => 365,
    'function' => 'getAttributeValue',
    'class' => 'Illuminate\\Database\\Eloquent\\Model',
    'type' => '->',
  ),
  4 => 
  array (
    'file' => '/server/vendor/laravel/framework/src/Illuminate/Database/Eloquent/Model.php',
    'line' => 1660,
    'function' => 'getAttribute',
    'class' => 'Illuminate\\Database\\Eloquent\\Model',
    'type' => '->',
  ),
  5 => 
  array (
    'file' => '/server/vendor/webonyx/graphql-php/src/Executor/Executor.php',
    'line' => 176,
    'function' => 'offsetExists',
    'class' => 'Illuminate\\Database\\Eloquent\\Model',
    'type' => '->',
  ),
  6 => 
  array (
    'file' => '/server/vendor/nuwave/lighthouse/src/Schema/Factories/FieldFactory.php',
    'line' => 166,
    'function' => 'defaultFieldResolver',
    'class' => 'GraphQL\\Executor\\Executor',
    'type' => '::',
  ),

And then:

[2020-12-28 15:59:33] local.ERROR: array (
  0 => 
  array (
    'file' => '/server/vendor/laravel/framework/src/Illuminate/Database/Eloquent/Concerns/HasAttributes.php',
    'line' => 473,
    'function' => 'getFancyAttribute',
    'class' => 'App\\Models\\Stuff',
    'type' => '->',
  ),
  1 => 
  array (
    'file' => '/server/vendor/laravel/framework/src/Illuminate/Database/Eloquent/Concerns/HasAttributes.php',
    'line' => 1444,
    'function' => 'mutateAttribute',
    'class' => 'Illuminate\\Database\\Eloquent\\Model',
    'type' => '->',
  ),
  2 => 
  array (
    'file' => '/server/vendor/laravel/framework/src/Illuminate/Database/Eloquent/Concerns/HasAttributes.php',
    'line' => 386,
    'function' => 'transformModelValue',
    'class' => 'Illuminate\\Database\\Eloquent\\Model',
    'type' => '->',
  ),
  3 => 
  array (
    'file' => '/server/vendor/laravel/framework/src/Illuminate/Database/Eloquent/Concerns/HasAttributes.php',
    'line' => 365,
    'function' => 'getAttributeValue',
    'class' => 'Illuminate\\Database\\Eloquent\\Model',
    'type' => '->',
  ),
  4 => 
  array (
    'file' => '/server/vendor/laravel/framework/src/Illuminate/Database/Eloquent/Model.php',
    'line' => 1671,
    'function' => 'getAttribute',
    'class' => 'Illuminate\\Database\\Eloquent\\Model',
    'type' => '->',
  ),
  5 => 
  array (
    'file' => '/server/vendor/webonyx/graphql-php/src/Executor/Executor.php',
    'line' => 177,
    'function' => 'offsetGet',
    'class' => 'Illuminate\\Database\\Eloquent\\Model',
    'type' => '->',
  ),
  6 => 
  array (
    'file' => '/server/vendor/nuwave/lighthouse/src/Schema/Factories/FieldFactory.php',
    'line' => 166,
    'function' => 'defaultFieldResolver',
    'class' => 'GraphQL\\Executor\\Executor',
    'type' => '::',
  ),

Now, offsetExists on laravel is really dumb:

    public function offsetExists($offset)
    {
        return ! is_null($this->getAttribute($offset));
    }

but then again this call on defaultFieldResolver assumes a lot of an ArrayAccessor:

if (isset($source[$fieldName])) {
    $property = $source[$fieldName];
}

I'm not sure what I can propose to fix this. property_exists() does not work. I think this might be only properly solvable in Laravel. A temporary workaround was implemented a smarter "offsetExists()" that replicates getAttribute() checks on the model but doesn't actually call the data fetching.

[spawnia]

What about:

$property = $source[$fieldName] ?? null;

[brunobg]

No, that gives the same result. Apparently it's just syntactic sugar and PHP calls offsetExists() by itself. Besides, PHP docs for offsetGet says This method is executed when checking if offset is empty().

So, is there anything wrong with ignoring the check? Here's what I propose:

        if (is_array($source)) {
            if (isset($source[$fieldName])) {
                $property = $source[$fieldName];
            }
        } elseif ($source instanceof ArrayAccess) {
            $property = $source[$fieldName];
        } elseif (is_object($source)) {
            if (property_exists($source, $fieldName)) {
                $property = $source->{$fieldName};
            }
        }

Rationale:

• For plain arrays there's never a problem and we maintain the same behavior.
• For ArrayAccess you try to fetch it and it's up to the implementation. By spec it should return something. A try/catch could be added too, but the spec says nothing about throwing exceptions. In the case of Eloquent\Model it returns null if the key does not exist. I tested a graphql query with a forced invalid key and it works properly -- the query is returned with a null value for the field.
• for objects, property_exists seems better than isset: As opposed with isset(), property_exists() returns true even if the property has the value null. But the behavior would still be the same in the end.

I tested this code and it works as expected, avoiding the double call. It's a bit hard to send a test case to reproduce it since it involves so much stuff, but I'm testing it like this:

• create an accessor on a Laravel Model.
• log calls to it: Log::error(var_export(xdebug_get_function_stack(), true));

[brunobg]

Ah, it seems my PR breaks some tests. Let me know if you have a better idea.

[brunobg]

Nah, it was a typo. It seems to pass everything.

[spawnia]

The crucial point is what assumptions we make about ArrayAccess::offsetGet().

The PHP documentation does not specify what should happen on nonexisting offsets. There is a code example which returns null if the value is not found, but it does not explicitly say the function should not throw. Presuming ArrayAccess should let objects function as arrays, throwing when trying to access a nonexistent offset seems reasonable.

At the moment, the default resolver simply defaults to null for every data type, as shown in #761. I think it would be confusing for the user if we were to change that for ArrayAccess only. If anything, we should make the resolver strict for every data type:

if (is_array($source) || $source instanceof ArrayAccess) {
    $property = $source[$fieldName];
} elseif (is_object($source)) {
    $property = $source->{$fieldName};
}

[brunobg]

Yes, the PHP docs are not good at all about this. But they state clearly that offsetExists calls offsetGet, which in my opinion is a very very very very dumb spec. I'd violate that spec in any implementation of mine with a clear conscience 🤪

But regarding your suggestion, there's another problem in how PHP handles plain array access. If you access an undefined index on a plain array, you get a non-catchable (with try/catch) notice. In an ArrayAccess that throws a regular exception, a try/catch would work. So I don't think array and ArrayAccess should be treated as equals, because PHP doesn't.

If you want to be safe, my suggestion is:

        if (is_array($objectValue)) {
            if (isset($objectValue[$fieldName])) {
                $property = $objectValue[$fieldName];
            }
        } elseif ($objectValue instanceof ArrayAccess) {
            try {
                $property = $objectValue[$fieldName];
            } catch (\Exception $e) {
                // pass
            }
        }

[vladar]

This is correct and expected behavior for ArrayAccess interface. Full reply: #760 (comment)

[spawnia]

they state clearly that offsetExists calls offsetGet

I just checked, actually that is not the case. See https://3v4l.org/OLqvr

[brunobg]

I'm closing this. I won't argue anymore and hopefully this will be at least fixed in Lighthouse, but you're expecting good implementations from 3rd party code -- that have been shown not to exist by at least three people -- and then you suggest a non-trivial change to fix the libraries in the user code. I understand the point of expecting decent code and not the crazy Laravel implementation, but then users end up in this "not my fault" business, and a bug that is certainly affecting a lot of people, with a report that is now a month old is still open.