folkloreinc / laravel-graphql

Facebook GraphQL for Laravel 5. It supports Relay, eloquent models, validation and GraphiQL.
1.76k stars 234 forks source link

Recursive input Objects do not work #313

Open Artem-Schander opened 6 years ago

Artem-Schander commented 6 years ago

I'm facing a strange behaviour. As soon as I define an input object to have a relation to itself (or to another input object that references the first one) I'm not able to execute my tests.

Correct me if I'm wrong but it should be possible to point a relation of an input object to one of its own kind. F.a. to save a nested navigation or hierarchical categories?!

ralrom commented 6 years ago

Did you take a look at this documentation?

If you have a recursive / circular type, you need to wrap your fields in a function for it to work

Artem-Schander commented 6 years ago

Thank you for the hint. I had no idea about this approach. Mainly because it kind of works in an older project, without that function wrap. Anyway. Closing the issue then.

Artem-Schander commented 6 years ago

Sorry, but I'm getting the same result. Here is a simplified version of my code:

<?php

namespace App\GraphQL\Types;

use GraphQL;
use GraphQL\Type\Definition\Type;
use Folklore\GraphQL\Support\Type as BaseType;

class CategoryInput extends BaseType
{
    protected $inputObject = true;

    public function fields()
    {
        return [
            'id' => [
                'type' => Type::int(),
            ],
            'name' => [
                'type' => Type::string(),
            ],
            'children' => [
                'type' => Type::listOf(MyTypes::category()),
            ],
        ];
    }
}

class MyTypes
{
    private static $category;

    public static function category()
    {
        return self::$category ?: (self::$category = GraphQl::type('CategoryInput'));
    }
}

It would be nice if someone could point out what I'm making wrong. Thanks

Artem-Schander commented 6 years ago

Is there anybody out there?

spawnia commented 6 years ago

I am currently experiencing an issue with circular references in InputObjects. I have defined rules with my InputObjects, which causes inferRulesFromType and getInputTypeRules to run into an infinite loop.

I believe this issue can be resolved by looking at the $resulutionArguments, which do contain the actual input variables. At this point, the variables where already validated against the schema definition, right? So we can be sure the arguments structure matches with the fields of the defined InputObject. How about we only get the rules that correspond to an actual given value and stop looping once we iterated through the whole input args?

If no one is working on something like this currently, i might give it a shot and submit a PR. What do you think?

spawnia commented 6 years ago

While working on this, i found that we actually have two seperate issues going on here. I have opened another issue to detect rule definitions for circular referenced Input Objects that are impossible to resolve: #336

The second issue is one that lies within the way Laravel validations are used here. The current approach goes through the tree of InputObjects and infers an array of nested validation rules. Then, those rules and the complete input args are passed through the Laravel validator all at once.

One approach i could see would be to look at the depth of the provided input arguments and only infer the rules until that point. The other option would be to loop through the input arguments and run the validations on each iteration level, thus eliminating the need to infer the complete rules upfront.

What approach seems better in your opinion, any Pros/Cons?

bytebrain commented 6 years ago

I used this:

CategoryType.php

<?php
namespace App\GraphQL\Type;

use Folklore\GraphQL\Support\Facades\GraphQL;
use GraphQL\Type\Definition\Type;
use Folklore\GraphQL\Support\Type as GraphQLType;

use App\Models\Category;

class CategoryType extends GraphQLType
{
    protected $attributes = [
        'name' => 'Category',
        'description' => 'A category',
        'model' => Category::class,
    ];

    public function fields()
    {
        return [
            'id' => [
                'type' => Type::nonNull(Type::int()),
                'description' => 'The id of a category'
            ],
            'name' => [
                'type' => Type::string(),
                'description' => 'The title of a category'
            ],
            'parent_id' => [
                'type' => Type::int(),
                'description' => 'The parent category id of a category'
            ],
            // field relation to model user_profiles
            'children' => [
                'type' => Type::listOf(GraphQL::type('Category')),
                'description' => 'The child categories of a category',
                'always' => ['id', 'name'],
            ]
        ];
    }
}

My model: Category.php

namespace App\Models;

use Illuminate\Database\Eloquent\Model;

class Category extends Model
{
    protected $table = 'categories';
    protected $fillable = ['parent_id', 'name'];
    public $timestamps = false;

    public function children()
    {
        return $this->hasMany('App\Models\Category', 'parent_id', 'id');
    }
}
spawnia commented 6 years ago

Since my fix for this is not getting merged, and this Repo is apparently unmaintained i jumped ship. I can warmly recommend https://lighthouse-php.com/, we are actively developing it and it is a breeze to develop with.

And yeah, it does support recursive nested validation. You can even insert nested Mutations spanning multiple models and their relations, all without writing any PHP.

KVSocial commented 6 years ago

Since my fix for this is not getting merged, and this Repo is apparently unmaintained i jumped ship. I can warmly recommend https://lighthouse-php.netlify.com/, we are actively developing it and it is a breeze to develop with.

And yeah, it does support recursive nested validation. You can even insert nested Mutations spanning multiple models and their relations, all without writing any PHP.

Appreciate your effort on lighthouse; question though how much work is switching from this laravel graphQL package to yours? because to me it looks like a shit ton of work to switch on bigger project.

Artem-Schander commented 6 years ago

@spawnia lighthouse looks really promising.
But like KVSocial said, it's for sure not that easy to migrate.
However, for future projects I'll definitely give it a try.

spawnia commented 6 years ago

It depends on how specialized your Queries are. If you do mostly CRUD and a lot of Eloquent, it is a breeze. If not, you will have to dig in a little deeper, but after a while i think it will be way easier to add new features and to maintain the project.

You can use the built in printer of webonyx/graphql-php to get your schema as SDL, which will be a great starting point. http://webonyx.github.io/graphql-php/reference/#graphqllanguageprinter

You can try it for a small part of your schema and see if you like it :)

mfn commented 6 years ago

because to me it looks like a shit ton of work to switch on bigger project

It's already ton of work a small project as I'm having.

It depends on how specialized your Queries are. If you do mostly CRUD and a lot of Eloquent, it is a breeze. If not, you will have to dig in a little deeper

This. I tried to make a quick win here (max 2 hours to see how far I get) but I mostly do not use CRUD at all; it's all specialised, custom resolvers all the way and also the customization I needed for this library (error reporting isn't very satisfying, took some time to get it straight) I have to go over all again.

But, eventually: I will have to do it. Folklore is pretty much dead

spawnia commented 6 years ago

@mfn Yeah, error handling is quite a bit different in GraphQL. You can not really abort on the first error, since you might have another field that worked and should return data.

I did a major rewrite on how Lighthouse does Error handling, you might want to check out the newest version. I am quite interested in how you did it as well!

mfn commented 6 years ago

@spawnia my biggest grief was that all exceptions where reported in the GraphQL result payload but they were not logged anywhere => completely useless:

That was a major pain issue because exception not being logged means it does not go through your regular exception handler channels which means they are not properly e.g. reported to 3rd party Services (like Sentry).

I solved it with a custom error handler, a re-implementation of folklores own way how to dispatch errors and a fallback in Exception\Handler::render(). A sh.. ton of work :-) Not really "much" work but took ages to figure it out, get it right, etc.

Will give my feedback about lighthouse when I finally can make time for the switch :-)

spawnia commented 6 years ago

webonyx/graphql-php does a good job of collecting the errors. I made it so our error handler sends all Exceptions through a pipeline that works just like middleware. You can register any number of handlers that may rethrow, log, reformat or do whatever with the Errors.