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

Add validation for GraphQL mutations #1604

Merged
merged 5 commits into from Dec 26, 2017

Conversation

dunglas
Copy link
Member

@dunglas dunglas commented Dec 25, 2017

Q A
Bug fix? no
New feature? yes
BC breaks? no
Deprecations? no
Tests pass? yes
Fixed tickets n/a
License MIT
Doc PR n/a

Validate data during GraphQL mutations. Also includes a refactoring of the validation system.

TODO:

  • Add unit tests

* Validates an item.
*
* @param object $data
* @param array $contexy
Copy link
Member

Choose a reason for hiding this comment

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

$context

* Validates an item.
*
* @param object $data
* @param array $context
Copy link
Member

Choose a reason for hiding this comment

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

Missing @throws?

private function validate($item, ResolveInfo $info, ResourceMetadata $resourceMetadata, string $operationName = null)
{
if (null === $this->validator) {
return;
Copy link
Member

Choose a reason for hiding this comment

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

We don't want to warn the user if there is no validator?

Copy link
Member Author

Choose a reason for hiding this comment

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

No (we would just make this property not nullable otherwise), it allows to create micro-frameworks on top of API Platform and it mimics the behavior of the ValidateListener (which is optional).

/**
* @expectedException \ApiPlatform\Core\Bridge\Symfony\Validator\Exception\ValidationException
*/
public function testInvalid()
Copy link
Member

Choose a reason for hiding this comment

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

Use a common method?

Copy link
Member Author

Choose a reason for hiding this comment

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

We wouldn't be able to share much code.

return;
}

$data = $event->getControllerResult();
Copy link

Choose a reason for hiding this comment

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

@dunglas consider validating controller argument data instead of controller result. We cann't put some logic inside controller now because request data is not validated yet.

Copy link
Member Author

Choose a reason for hiding this comment

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

I'm not sure to understand, the validation occurs after the execution of the controller.

Copy link
Member Author

Choose a reason for hiding this comment

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

If you need to validate the data before executing your logic, just inject ValidatorInterface in your controller and call it.

Copy link
Member

Choose a reason for hiding this comment

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

I think it's related to this: #1590

Copy link

Choose a reason for hiding this comment

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

yes, @alanpoulain is right. IMHO validating input data before controller execution is much logical.

Copy link
Member Author

@dunglas dunglas Dec 26, 2017

Choose a reason for hiding this comment

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

Changing this would be a big BC break and will provide less flexibility. IMO the data must be valid before being persisted, but after the custom logic has been executed.

@theofidry
Copy link
Contributor

👏

@dunglas dunglas merged commit 52fb7f3 into api-platform:master Dec 26, 2017
@dunglas dunglas deleted the graphql-validation branch December 26, 2017 16:13
hoangnd25 pushed a commit to hoangnd25/core that referenced this pull request Feb 23, 2018
* Add validation for GraphQL mutations
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.

None yet

4 participants