Skip to content
This repository has been archived by the owner on Feb 10, 2019. It is now read-only.

Improvements to validation and field resolvers #339

Open
wants to merge 6 commits into
base: develop
Choose a base branch
from
Open

Improvements to validation and field resolvers #339

wants to merge 6 commits into from

Conversation

spawnia
Copy link

@spawnia spawnia commented May 16, 2018

  • 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

- Limit the depth of static rule generation 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
- Require Fields to implement type() via abstract method
- Include default resolver in Field class to be able to apply authentication, authorization and validation even if no resolver is implemented
- Deprecate the rules() function in favour of inline arguments defined within args()
- Add tests and documentation for all proposed changes
@coveralls
Copy link

coveralls commented May 16, 2018

Coverage Status

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

@spawnia spawnia changed the title Improvements to validation and field resolvers WIP: Improvements to validation and field resolvers May 17, 2018
@spawnia
Copy link
Author

spawnia commented May 17, 2018

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 spawnia changed the title WIP: Improvements to validation and field resolvers Improvements to validation and field resolvers May 18, 2018
@spawnia
Copy link
Author

spawnia commented May 18, 2018

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.

@JustinElst
Copy link

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

@spawnia
Copy link
Author

spawnia commented Jul 19, 2018

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!

@mfn
Copy link

mfn commented Jul 19, 2018

@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.

@spawnia
Copy link
Author

spawnia commented Jul 19, 2018

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!

@mfn
Copy link

mfn commented Jul 19, 2018

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 :-)

@spawnia
Copy link
Author

spawnia commented Jul 19, 2018

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 :)

Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Recursive input Objects do not work
4 participants