Skip to content

[6.2] [doctrine-bridge] EntityValueResolver priority is too high and there is no way to be skipped by a custom ParamCoverter #48433

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.

Sign up for GitHub

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

Jump to bottom
Closed
marforon opened this issue Dec 1, 2022 · 7 comments · Fixed by #48489
Closed

[6.2] [doctrine-bridge] EntityValueResolver priority is too high and there is no way to be skipped by a custom ParamCoverter #48433

marforon opened this issue Dec 1, 2022 · 7 comments · Fixed by #48489
Labels
Bug DoctrineBridge Status: Needs Review

Comments

@marforon
Copy link

marforon commented Dec 1, 2022 •
edited
Loading

Symfony version(s) affected

6.2

Description

EntityValueResolver introduced in Symfony 6.2 priority (110) is higher than RequestAttributeValueResolver priority (100) which makes ParamConverter unusable in some cases. Only UserValueResolver has a higher priority.

How to reproduce

Consider this controller action:

    #[Route('/asset-file/{assetFile}/distribute', name: 'distribute', methods: [Request::METHOD_POST])]
    #[ParamConverter('jwDistribution', converter: SerializerParamConverter::class)]
    public function distribute(AssetFile $assetFile, JwDistribution $jwDistribution): JsonResponse
    {
           return new JsonResponse();
    }

In this case, AssetFile and JwDistribution represent Doctrine's entities, but JwDistribution doesn't exist in repository and should be converted by a custom converter (serializer). Instead, EntityValueResolver is used also for JwDistribution and it obviously fails to convert (because it's entity object without identifier). That must be at least some kind of regression. Could we do something about it?

Possible Solution

At least
RequestAttributeValueResolver should has a higher priority than EntityValueResolver.

Additional Context

No response
@marforon marforon added the Bug label Dec 1, 2022
@marforon marforon changed the title [6.2] [doctrine-bridge] EntityValueResolver priority is too high and it's almost each time used [6.2] [doctrine-bridge] EntityValueResolver priority is too high and there is no way to use a custom ParamCoverter Dec 1, 2022
@marforon marforon changed the title [6.2] [doctrine-bridge] EntityValueResolver priority is too high and there is no way to use a custom ParamCoverter [6.2] [doctrine-bridge] EntityValueResolver priority is too high and there is no way to be skipped by a custom ParamCoverter Dec 1, 2022
@nicolas-grekas nicolas-grekas mentioned this issue Dec 5, 2022
Merged
@nicolas-grekas
Copy link
Member

Thanks for the report. Can you please confirm that #48489 would fix the issue?

@marforon
Copy link
Author

marforon commented Dec 6, 2022 •
edited
Loading

@nicolas-grekas thanks for the response. Unfortunately, it won't help. The problem is it will continue to execute a find method in EntityManager

symfony/src/Symfony/Bridge/Doctrine/ArgumentResolver/EntityValueResolver.php

Line 59 in 6780b02

} elseif (false === $object = $this->find($manager, $request, $options, $argument->getName())) {
and in my case, the identifier from API is null (new entity). It will throw this exception https://github.com/doctrine/orm/blob/2.13.x/lib/Doctrine/ORM/EntityManager.php#L452

It would help if the EntityValueResolver would be skipped if the identifier is null.

EDIT: getIdentifier() method returns JwDistribution object from the request, because ParamConverter already set the attribute in SerializerParamConverter, so this doesn't help

symfony/src/Symfony/Bridge/Doctrine/ArgumentResolver/EntityValueResolver.php

Line 104 in 6780b02

if (false === $id || null === $id) {
...

@nicolas-grekas
Copy link
Member

OK thanks. Can you provide a small reproducing app that I could play with locally to work on this?

@stof
Copy link
Member

stof commented Dec 6, 2022

having higher priority than RequestAttributeValueResolver is the intended purpose, as this supports essential features when using only the value resolver system.

But we should indeed add a condition to skip this find() when the attribute is already an object, to improve support for the case of mixing those value resolvers with the legacy ParamConverter system of SensioFrameworkExtraBundle.
Side note: you should convert your SerializerParamConverter to an argument resolver instead.

@marforon
Copy link
Author

marforon commented Dec 6, 2022 •
edited
Loading

OK thanks. Can you provide a small reproducing app that I could play with locally to work on this?

I will do so later today or tomorrow.

having higher priority than RequestAttributeValueResolver is the intended purpose, as this supports essential features when using only the value resolver system.

But we should indeed add a condition to skip this find() when the attribute is already an object, to improve support for the case of mixing those value resolvers with the legacy ParamConverter system of SensioFrameworkExtraBundle. Side note: you should convert your SerializerParamConverter to an argument resolver instead.

Skipping the EntityValueResolver if the attribute is already an object would help. We are going to migrate to an argument resolver sooner or later, but to be honest I miss some option to skip this visitor pattern which iterates the whole stack of arguments resolvers (it's also not the best thing to do for performance reasons). It would be cool to add some attribute for parameters which would allow to define a specific argument resolver right away and skip the whole process with priority.

@nicolas-grekas
Copy link
Member

But we should indeed add a condition to skip this find() when the attribute is already an object

I updated #48489 to follow this idea, does that work for you?

@marforon
Copy link
Author

marforon commented Dec 7, 2022

But we should indeed add a condition to skip this find() when the attribute is already an object

I updated #48489 to follow this idea, does that work for you?

Thank you! I tested it and it works for me.

@fabpot fabpot closed this as completed Dec 8, 2022
fabpot added a commit that referenced this issue Dec 8, 2022
@fabpot
bug #48489 [DoctrineBridge] Skip resolving entities when the correspo…
0d95f41 
…nding request attribute is already an object (nicolas-grekas)

This PR was merged into the 6.2 branch.

Discussion
----------

[DoctrineBridge] Skip resolving entities when the corresponding request attribute is already an object

| Q             | A
| ------------- | ---
| Branch?       | 6.2
| Bug fix?      | yes
| New feature?  | no
| Deprecations? | no
| Tickets       | Fix #48433
| License       | MIT
| Doc PR        | -

Commits
-------

9b0d717 [DoctrineBridge] Skip resolving entities when the corresponding request attribute is already an object
@mbabker mbabker mentioned this issue Dec 12, 2022
Open
@marforon marforon mentioned this issue Jan 9, 2023
Closed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Assignees
No one assigned
Labels
Bug DoctrineBridge Status: Needs Review
Projects
None yet
Milestone
No milestone
6 participants
@fabpot @nicolas-grekas @stof @xabbuh @carsonbot @marforon