Graphql-laravel: Infinite loop while input object pointing at each other

[Artem-Schander]

https://github.com/Artem-Schander/graphql-infinite-loop

[crissi]

Does this fix your problem https://github.com/rebing/graphql-laravel/pull/511?

[Artem-Schander]

Hi @crissi
Unfortunately it did not fix my issue.
Maybe it would be enough to check inside the loop if the $prefix aready contains the $name?

public function getInputTypeRules(InputObjectType $input, string $prefix, array $resolutionArguments): array
{
    $rules = [];

    $usedNames = explode('.', $prefix);

    foreach ($input->getFields() as $name => $field) {
        if (in_array($name, $usedNames)) {
            continue;
        }

        $key = "{$prefix}.{$name}";

        // get any explicitly set rules
        if (isset($field->rules)) {
            $rules[$key] = $this->resolveRules($field->rules, $resolutionArguments);
        }

        // then recursively call the parent method to see if this is an
        // input object, passing in the new prefix
        if ($field->type instanceof InputObjectType) {
            // in case the field is a self reference we must not do
            // a recursive call as it will never stop
            if ($field->type->toString() == $input->toString()) {
                continue;
            }
        }
        $rules = array_merge($rules, $this->inferRulesFromType($field->type, $key, $resolutionArguments));
    }

    return $rules;
}

I realize that this is not an optimal solution, but maybe it's the right direction.

[Artem-Schander]

BTW. I've added a few scenarios to the RecursionTest
Now there are also examples for indirect recursion
Publisher -> Authors -> Books -> Publisher ...

[mfn]

Sorry for the delay and thanks for the succinct example repo 👍

I'm unfamiliar with that part of the code.

In \Rebing\GraphQL\Support\Field::getInputTypeRules there seems to be a primitive short-circuit for recursions:

            // then recursively call the parent method to see if this is an
            // input object, passing in the new prefix
            if ($field->type instanceof InputObjectType) {
                // in case the field is a self reference we must not do
                // a recursive call as it will never stop
                if ($field->type->toString() == $input->toString()) {
                    continue;
                }
            }

This only works if there's a direct recursion.

However thinking about this piece of code, there already seems to be a problem: yes, it stops the recursion but what about actual validation if _there is_ nested data? It would not properly validate such date (if I read the code correctly).

The parameter $resolutionArguments is always the full arguments as passed to the GraphQL query.

I'm wondering: why recurse further if at the given $key/$prefix we could check _into_ the $resolutionArguments if there is data at level present and if not, simply stop recursing.

I.e. only build the $rules array for data being present. But that could maybe lead to another problem, i.e. what if such rules would _require_ a field being present but we stopped adding it because there's no data present 🤔

[Artem-Schander]

I think you're right by thinking about the necessity to validate further than the given payload structure.
Therefore I thought It would be enough to skip all input types that already have been processed in terms of validation. But maybe it's a naive idea.

Another approach would be to validate only by the rules in the root input object and being as precise as necessary by declaring them. At least I think it would be the most logical way and user friendly also.

F.A.
In my projects are always different mutation classes for creating and updating which are extending the same save mutation. So the save mutation does not have any rules in most cases and the other two have very different ones.

<?php

namespace App\GraphQL\Base\Mutations;

use Illuminate\Validation\Rule;

class CreateUser extends SaveUser {

    protected $attributes = [
        'name' => 'CreateUser'
    ];

    // TODO
    // test validation
    protected function rules(array $args = []): array
    {
        $rules = parent::rules();

        $rules += [
            'user.email' => ['required', 'email'], // TODO: unique
            'user.password' => ['required', 'string'],
            'user.gender' => ['string', Rule::in(['M', 'F', 'X'])],
            'user.first_name' => ['required', 'string'],
            'user.last_name' => ['required', 'string'],
            'user.addresses' => ['required', 'array', 'min:1', 'selected_billing_address', 'selected_shipping_address'],
            'user.addresses.*.street' => ['required', 'string'],
            'user.addresses.*.zipcode' => ['required', 'string'],
            'user.addresses.*.city' => ['required', 'string'],
            'user.addresses.*.country' => ['required', 'string'],
        ];

        return $rules;
    }
}

<?php

namespace App\GraphQL\Base\Mutations;

use Illuminate\Validation\Rule;

class UpdateUser extends SaveUser {

    protected $attributes = [
        'name' => 'UpdateUser'
    ];

    // TODO
    // test validation
    protected function rules(array $args = []): array
    {
        $rules = parent::rules();

        $rules += [
            'user.id' => ['required'],
            'user.email' => ['filled', 'email'], // TODO: unique
            'user.email' => ['filled', 'string'],
            'user.gender' => ['filled', 'string', Rule::in(['M', 'F', 'X'])],
            'user.first_name' => ['filled', 'string'],
            'user.last_name' => ['filled', 'string'],
            'user.addresses' => ['array', 'selected_billing_address', 'selected_shipping_address'],
            'user.addresses.*.street' => ['filled', 'string'],
            'user.addresses.*.zipcode' => ['filled', 'string'],
            'user.addresses.*.city' => ['filled', 'string'],
            'user.addresses.*.country' => ['filled', 'string'],
        ];

        return $rules;
    }
}

As you can see I'm deciding on the first level which data is needed to perform the requested mutation.

[mfn]

As you can see I'm deciding on the first level which data is needed to perform the requested mutation.

I thought about this too but couldn't make up my mind yet. Especially with _optional recursive_ stuff it gets problematic. But then, input recursion is ... something of its own kind altogether. At least to me :)

Another idea I just had while reading your feedback: would it be possible to to iteratively walk through the nested data structure and, level by level, apply the validations?

Probably not easy but it would not require to declare upfront (which your sample does and what the automatic code in the library tries to do).

But maybe this is thinking too far.

What I initially wrote:

I.e. only build the $rules array for data being present. But that could maybe lead to another problem, i.e. what if such rules would require a field being present but we stopped adding it because there's no data present

Somewhat it doesn't really make sense: what is a _require recursive type_ anyway? That definition doesn't make sense because it would express a required indefinite recursion as a input argument 🤪

As of now, I think a solution could be to simply introspect the payload cautiously while the rules array is being built and stop that if no matching data is present in the input payload.

[Artem-Schander]

Is there any progress on this issue?

[mfn]

I'm not ware of anyone actively working on it, contributions are very welcome!

It might also make sense to check other GraphQL PHP implementations how to prevent such problems (if they provide such customized validations, that is).

[crissi]

can you add your failing example as a test?

Will make it easier for the person who picks up the task

[mfn]

Infinite loop while…

can you add your failing example as a test?

Well, I just hope it doesn't let the test suit run forever 😏

[Artem-Schander]

@crissi
There are already some tests

@mfn
I think with a small memory_limit setting you will not wait that long :)

[mfn]

@Artem-Schander @crissi did hard work on this one, there's no release yet but maybe you can test with dev-master?

[Artem-Schander]

Nice. Many thanks. I'll test it and let you know about the outcome.

[Artem-Schander]

@crissi Thanks again. It works perfectly so far :)

[mfn]

Nice, great work @crissi 🙇

[crissi]

Thanks guys:blush: