laravel-graphql icon indicating copy to clipboard operation
laravel-graphql copied to clipboard

Improvements to validation and field resolvers

Open spawnia opened this issue 6 years ago • 9 comments

  • Only generate minimal necessary rules to prevent infinite loops with circularly nested Input Objects
  • Move validation from trait to the generic field class. This makes it so that Queries and Mutations are treated the same in regards to validation.
  • Unify the generated classes for Fields, Mutations and Queries and add useful PHPDocs
  • Require Fields to implement type() via abstract method, this eliminates the need for safety checks and makes it easier on developers
  • Include default resolver in Field class to be able to apply authentication, authorization and validation even if no resolver is implemented
  • Clarify the usage of the rules() function in comparison to defining rules inline within args()
  • Add tests and documentation for all proposed changes

Closes #313 #336 #320

spawnia avatar May 16 '18 08:05 spawnia

Coverage Status

Coverage increased (+0.02%) to 70.557% when pulling 02d66fd9c653eceb1ddb69671d86602616262a74 on spawnia:recursive-input-objects-validation into 171631a17e4bf33780513f15221ea6052c67d498 on Folkloreatelier:develop.

coveralls avatar May 16 '18 08:05 coveralls

While using this in our actual project, i stumbled upon a problem.

We have deeply nested inputs with many fields. My first attempt at solving this prevented the occurence of infinite loops by limiting the nesting depth, but i still ran out of memory. I tried tuning the $validationDepth - it finally worked when i set it to 7.

The array of generated rules had a count of 153092 at this point - even though the actual values i used for the input only had one field.

Because of this, i based validation on the concrete values that are passed as inputs, and rule generation should be limited to those. This way, minimal overhead is produced and, should the need arise, any levels of nesting are possible.

spawnia avatar May 17 '18 08:05 spawnia

Convenient array validation was introduced in Laravel 5.2, so the tests for 5.1 are failing. Since 5.1 is now over two years old and is at EOL apart from security fixes, do you think we can cut the chord? @dmongeau

In general, supporting all 5.x versions of Laravel seems tough, since Laravel does not follow semantic versioning. How about we go straight to 5.5 which is the latest LTS? We could depend on PHP 7 if we do so.

spawnia avatar May 18 '18 12:05 spawnia

This is amazing! I had the out of memory issue (while not using any rules). Now it all works fine!

JustinElst avatar May 29 '18 08:05 JustinElst

If anyone else is having issues with this and noticed that this repo is dead since April, head on over to https://github.com/nuwave/lighthouse

Way more pleasant to work with and development there is pretty active, nearing v2.2 at the moment!

spawnia avatar Jul 19 '18 06:07 spawnia

@spawnia did you make an actual switch in a project, can you share some experience (effort/time required)?

The paradigm/approach to declare and write up types, resolves, etc. seems quite different (code vs. external files paired with code); even assuming my current Folklore project has 100% coverage, it seems like a lot of work switching.

It seems the documentation of lighthouse is starting to catch up, if memory serves me right when I checked it some time ago there wasn't much there.

mfn avatar Jul 19 '18 07:07 mfn

As i made the switch, i had a few hundred auto-generated types. I was able to simplify the code generation massively and quickly add new capabilities to the queries. The many classes and configuration to register them actually boiled down to just the schema definition, no actual code needed.

My use-case required that i add some substantial new features and performance improvements to Lighthouse, but once i got that out of the way, actually implementing it as part of our application was an absolute breeze. I highly recommend to check it out!

spawnia avatar Jul 19 '18 11:07 spawnia

i had a few hundred auto-generated types

👀

You mean auto-generated from models to GraphqQL types? If so, how did you do that?

I'm also having quote some non-Eloquent actual types in GraphQL which serve as intermediate representations for complex results.

just the schema definition, no actual code needed.

Hmm. I don't understand or I'm doing something wrong.

None of my GraphQL queries is a simple translation to Eloquent; it all requires a lot of custom resolving (authorization) and building custom queries (performance).

How you feel about the separation of definition of stuff vs. the parts you have to implement in code?

Right now it feels "correct" to me to define the type next to how to resolve it (which as I said, is quite complex and requires other services, etc.) which feels natural.

Thanks :-)

mfn avatar Jul 19 '18 12:07 mfn

I generated everything, including models, from an existing database schema.

While Lighthouse is centered around Eloquent, it is not limited to it. You can define custom resolvers anytime you need them. However, if you dig into the docs a little bit, you might find that you can do most common tasks by applying a directive to your schema.

Query performance is quite good, as it takes care of batching and eager loading for you. You can even do caching right from the schema definition. Lately, we have been putting a lot of work into performance.

How you feel about the separation of definition of stuff vs. the parts you have to implement in code?

Lighthouse is schema-centric, which feels like a natural fit with GraphQL. Code comes into play when defining resolvers. You can either use one of the many built-in resolvers of Lighthouse or write your own.

Feel free to hop into our Slack channel if you want to know more :)

spawnia avatar Jul 19 '18 12:07 spawnia