Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Custom InputType Validator for #[Input] Types #435

Merged
merged 12 commits into from
Mar 20, 2022

Conversation

oojacoboo
Copy link
Collaborator

Since the introduction of the #[Input] attribute as an additional way to create InputTypes, there isn't solid validation support. Previously, with the factories, you could easily call validation on every factory. Now, by automating the creation of InputTypes via the #[Input] attribute, there isn't a way to validate user input until the query or mutation controller.

Now, you could call your validator as the first line of every single query/mutation controller method. But, this is error prone and frankly, pretty nasty.

Instead, this PR provides an interface and setter on the SchemaFactory that allows for you to create your own validator, called after an InputType has been hydrated with user input data. For more details, please see the website/docs/validation.mdx doc.

@codecov-commenter
Copy link

codecov-commenter commented Jan 14, 2022

Codecov Report

Merging #435 (c9fe254) into master (c73dff8) will increase coverage by 0.00%.
The diff coverage is 100.00%.

Impacted file tree graph

@@            Coverage Diff            @@
##             master     #435   +/-   ##
=========================================
  Coverage     98.26%   98.27%           
- Complexity     1586     1590    +4     
=========================================
  Files           147      147           
  Lines          4267     4281   +14     
=========================================
+ Hits           4193     4207   +14     
  Misses           74       74           
Impacted Files Coverage Δ
src/FactoryContext.php 100.00% <100.00%> (ø)
src/InputTypeGenerator.php 100.00% <100.00%> (ø)
src/SchemaFactory.php 100.00% <100.00%> (ø)
src/Types/InputType.php 98.38% <100.00%> (+0.08%) ⬆️

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update c73dff8...c9fe254. Read the comment docs.

GraphQLite also supports a fully custom validation implementation for all input types defined with an `@Input` annotation or PHP8 `#[Input]` attribute. This offers a way to validate input types before they're available as a method parameter of your query and mutation controllers. This way, when you're using your query or mutation controllers, you can feel confident that your input type objects have already been validated.

<div class="alert alert--warning">
<p>It's important to note that this validation implementation does not validate input types created with a factory. If you are creating an input type with a factory, or using primitive parameters in your query/mutation controllers, you should be sure to validate these independently. This is strictly for input type objects.</p>
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Does the Input Fields attribute bypass the constructor and directly populates fields using reflection?

I do the validation either inside the domain object's constructor or within the factory if it requires access to other services?

I personally have favored the Factory approach so far for input types because it is less magic and keeps things simpler without coupling my domain with graphql.

I think the main reason for creating Input type objects is to use them for updating optional fields easily but the warning here makes it seem that creating input types via a factory is somehow not good practice which isn't true imo.

Copy link
Collaborator Author

@oojacoboo oojacoboo Jan 14, 2022

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@aszenz Yes it does, please see the following PR: #269

Some of the documentation talks about putting your validation close to your domain objects. I'm guessing this is @moufmouf's opinions. I personally disagree for many reasons. Validation should be tied to your API and your API consumer's input, first and foremost. If you'd like to have validation on your domain models, fine, but that's a different thing. Your validation on user input won't always match up 1:1 with your domain model. And for this reason, our InputType objects are essentially DTOs. The decoupling of your domain model from your API user input offers a lot more flexibility. It may require some additional boilerplate. That's where the #[Input] attribute shines though. You're able to eliminate the factories and also, with PHP 8, create some really clean DTO objects. I'd encourage you to give it a try. This is something that @devmaslov and myself happened to converge on around the same time. It's like he was reading my mind, and I'm sure many other GraphQLite users.

I think you're mis-understanding what's being said and the warning as well. It's pointing out that this validator does not apply to InputTypes created with a factory. This is so that there isn't any confusion around that. There isn't any need for this validator to apply to InputTypes created with a factory. It's perfectly acceptable that those concerns be handled, as desired, within the factory.

I agree that there is some auto-magic going on here, but it's really nothing more than removing all the boilerplate for factories. At least in our case, 90% of InputTypes are created with a factory that's taking in arguments and passing them straight through. It became so repetitive that we wrote helper methods to just take the arguments and create the object. The factory was nothing more than repetitive code, at this point. The next natural progression is the #[Input] attribute.

The issue, currently, is that you don't have a clean way to validate it. This PR addresses that.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I agree that validation on user input can be different from domain validation.

By applying domain validation inside the domain objects every kind of user input is validated for free (be it command, graphql or other user input sources) since the domain object is the single source of truth in the application.
It actually reduces the boilerplate of having to validate inputs from different sources when the domain model / factory can be trusted to do proper validation.

It's true that in 90% of the cases the factory appears as repetitive code but it is important to realize that it enables the domain / api layer to develop independently without coupling them together.

The reason i wanted to mention the docs is that we have seen bad coupling happen with Symfony forms and entities in our app and I think this Input type can lead to the same problems.

Imagine removing/renaming one setter in the entity and suddenly the api layer no longer works just because of some annotation.

So imo it's good to highlight the cons of using input annotations over factories even if they are very handy in some cases.

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yea, if your domain and API are nearly 1:1, maybe you can skip the DTO approach and validate on your models. Personally, I think this is a bad approach and you should treat your API as an API and your model as a model. The two will likely have some overlap (more or less depending on your design), but should be fully decoupled.

You can also have your commands hit your GraphQL API if you wanted to reuse your API and validation layers there.

At any rate, your concern around the input annotation being used on models is valid. The input annotation should really be used with DTOs. I've updated the documentation to add some notes on that.

@oojacoboo oojacoboo mentioned this pull request Jan 20, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants