-
-
Notifications
You must be signed in to change notification settings - Fork 840
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
Validation from DTO before resolver #6370
Comments
See #6354 for my bug report about this |
I'm not sure about what should be done here, @NicoHaase does #6363 solves your issue? I've no strong opinion about this but I would like to keep the same behavior as in 3.2, this is an untested behavior so we didn't really planned anything. @codedge you're also resolving a mutation? Can we try to compare the use cases? |
Here are my details: I have a custom resolver, that is filled with a custom DTO. Inside the DTO a run a validation of a property ( Entity <?php
declare(strict_types=1);
namespace App\Entity;
#[
Api\ApiResource(
operations: [],
graphQlOperations: [
new GraphQl\Mutation(
resolver: SampleResolver::class,
input: MySampleDto::class,
name: 'create',
),
]
)
]
class SampleRequest implements CustomInterface
{
} DTO <?php
declare(strict_types=1);
namespace App\Dto;
use Symfony\Component\Validator\Constraints as Assert;
final class MySampleDto
{
#[Assert\GreaterThan(value: 0)]
public int $amount;
} Resolver final readonly class CreateBurnRequestResolver implements MutationResolverInterface
{
public function __construct(
private ManagerRegistry $managerRegistry,
) {}
public function __invoke(?object $item, array $context): ?object
{
if (!$item instanceof MySampleDto) {
return null;
}
// Not being executed if amount is 0
;
} |
This is wrong something must be missing to set About this issue I don't understand how on one case validation ran, on the other it did not. The only difference I see is that there's an input in this case? We all agree that the resolver shouldn't be hit if validation fails? |
@soyuka sorry for confusing: the issue "validation on delete" happened on another entity type, and I'll try to craft a test case for it |
I'd like to fix this tomorrow would be awesome if I could have a test case tyvm! |
3.3...NicoHaase:core:fix-delete-validation contains a first way of testing this. |
yes |
thanks, this makes more thanks sorry for the trouble, will be patched today. |
After looking into it it looks like in 3.2 the mutation resolver runs before validation: core/src/GraphQl/Resolver/Factory/ItemMutationResolverFactory.php Lines 82 to 90 in 9a8adff
Validation only runs later at:
I'm confused I must have failed reproducing that behavior at afb739b when the backward compatibility is disabled. I think that the best would be to add a note to the changelog and to the migration guides about this when EVENT_LISTENERS_BACKWARD_COMPATIBILITY_LAYER is on/off. IMO validation should be done on the user input. I'll revert #6363 and fix the delete mutation not needing validation. |
Please keep in mind that reverting #6363 will also re-open #6354, as the behaviour reported there (which was fixed with #6363) is then again broken. My problem with deleting entities (where the validation was run, but probably shouldn't) is not related to the mutation problem I've reported initially |
@NicoHaase the problem with valiadating a delete mutation is fixed right? |
Yes, that problem is fixed after updating to 3.3.5. Thank you for asking 🙌 |
API Platform version(s) affected: 3.3.3
Description
With the latest 3.3.3 release the validation within a DTO is not run before the data reaches the resolver. The change is, I guess this one:
What is the reasoning behind or how should this be handled now? I usually validate data before reaching the resolver.
The text was updated successfully, but these errors were encountered: