Skip to content

#[Map...] is a mess (rfc) #58662

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

Open
klkvsk opened this issue Oct 25, 2024 · 9 comments
Open

#[Map...] is a mess (rfc) #58662

klkvsk opened this issue Oct 25, 2024 · 9 comments
Labels
HttpKernel RFC RFC = Request For Comments (proposals about features that you want to be discussed) Stalled

Comments

@klkvsk
Copy link

klkvsk commented Oct 25, 2024

Description

Mapping attributes for controller arguments are a mix of different concepts.
#[MapEntity], #[MapDateTime] - are "map to what".
#[MapQueryString], #[MapQueryParameter], #[MapRequestPayload], #[MapUploadedFile] - are "map from where".

Moreover, they can not be combined, as such you can not do

#[MapEntity]
#[MapQueryParameter]
Post $post

Providing a value resolver in most cases would not work either:

#[MapQueryParameter(resolver: EntityValueResolver::class)]
Post $post

The problem is ValueResolvers are hardcoded to check only a single input bag (Entity- and DateTimeValueResolver check only request attributes).

Proposed solution:

  1. Introduce the \Symfony\Component\HttpKernel\Attribute\ValueConsumer base attribute
  2. Introduce a set of ValueConsumer attributes:
    • #[RouteParam], #[QueryParam], #[QueryString], #[BodyParam], #[BodyPayload], #[HeaderParam]
    • can be combined in order to support cases when the value can be sent either in query or post data.
  3. Add optional third param in ValueResolverInterface::resolve:
interface ValueResolverInterface
{
    public function resolve(Request $request, ArgumentMetadata $argument, mixed $value = null): iterable;
}
  1. in \Symfony\Component\HttpKernel\Controller\ArgumentResolver::getArguments() before calling resolvers, run through ValueConsumer attributes defined for argument until one returning a value, then pass this value to resolve().
  2. Update all ValueResolvers to use $value if provided. If not, fallback to their original logic.

Example

#[Route('/posts/cat/{category}/{date?}')]
public function posts(
    #[RouteParam]
    PostCategory $category,

    #[RouteParam('date')]
    #[QueryParam]
    ?\DateTimeInterface $dateFrom,

    #[RouteParam('date')]
    #[QueryParam]
    ?\DateTimeInterface $dateTo,
) {
    // /posts/cat/1/2024-10-25, same as:
    // /posts/cat/1?dateFrom=2024-10-25&dateTo=2024-10-25
}
@derrabus derrabus added RFC RFC = Request For Comments (proposals about features that you want to be discussed) HttpKernel labels Oct 25, 2024
@stof
Copy link
Member

stof commented Oct 25, 2024

All those attribute are actually configuring how the parameter should be mapped. the "to what" info comes from the parameter type.

and combining those attributes will be hard. For instance, the EntityValueResolver has several ways of looking for the entity based on the request. Forcing it to use a value coming from the caller would not fit this (and similar issue will happen for many other resolvers).

@klkvsk
Copy link
Author

klkvsk commented Oct 25, 2024

@stof good point about EntityValueResolver, let's look into it.

There are 3 ways for entity lookup:

  1. by ID
  2. by mapping (criteria)
  3. by expression

So first two are about the input we use to search. Last one can be about input, as @ro0NL showed, but mostly about the method we use to search.

How we can adapt this resolver to ValueConsumers:
The only thing EntityValueResolver needs from $request is attributes bag. We just need to replace it with another bag if specified.
So,

  • if $value is passed and is scalar, $inputBag = new ParameterBag([ $argument->getName() => $value ]);
  • if $value is passed and is an array, $inputBag = new ParamererBag($value);
  • if $value is null, $inputBag = $request->attributes;

This implies we also want some ValueConsumers to consume a set of fields, like:

// "?y=2024&m=10" -> $yearAndMonth == [ "year" => "2024", "month" => "10" ]
#[QueryParam(["y" => "year", "m" => "month"] 
array $yearAndMonth

Now, this consumed value(s) can be used as defined in MapEntity id and mapping params:

For complex identifiers:

#[QueryParam(["y" => "year", "m" => "month"])]
#[MapEntity(id: [ "year", "month" ])]
MonthlyReport $report

Same for mapping (criteria):

#[QueryParam(["y" => "year", "m" => "month"])]
#[MapEntity(mapping: [ "year", "month" ])]
MonthlyReport $report

OR, you can pass all query/payload params and let the mapping decide, well, how to map them, the old way.

#[QueryString]
#[MapEntity(mapping: [ "y" => "year", "m" => "month" ])]
MonthlyReport $report

--
Lastly, support for MapEntity expr. We should add value to the list of variables for the evaluation:

#[QueryParam("email")]
#[MapEntity(expr: "repository.findByEmail(value)")]
Client $client

@stof
Copy link
Member

stof commented Oct 25, 2024

@klkvsk your proposal misses the fact that in the existing implementation, you don't need any attribute for the EntityValueResolver to produce a result for most common cases.
For instance, over ~1200 controllers in my work project (which leads to more arguments as many of them have several resolved arguments), I have exactly 8 usages of a MapEntity attributes (corresponding to cases where I use an expression to use a custom repository method, as a way to load some associated collections in a way avoiding N+1 queries).

Forcing to add 2 attributes on each controller argument would be a massive regression of DX.

Lastly, support for MapEntity expr. We should add value to the list of variables for the evaluation:

We cannot add support for value as it would conflict with a route attribute named value, as the existing API passes route attributes as variables in the expression, alongside request and repository, which means that any addition of a new predefined variable is a BC break (that was one of the reasons why the subject expression of the IsGranted attribute has been implemented by exposing a args variable instead of providing all arguments as their own variables btw).

@klkvsk
Copy link
Author

klkvsk commented Oct 25, 2024

@stof

Forcing to add 2 attributes on each controller argument would be a massive regression of DX.

As I said, if no ValueConsumer attributes specified, then the $value passed to resolve() call is null, meaning ValueResolvers should fall back to original logic, i.e. use $request->attributes as before.
This proposal is intended to be fully back-compatible.

We cannot add support for value as it would conflict with a route attribute named value

It won't conflict, again, due to the fact that we don't use attributes anymore, if ValueConsumer is defined.
But I'd like to correct myself, it'll be better to add variables to evaluator by expandind $inputBag->all(), like it is not expanding $request->attributes->all(). It will make DX better not having two ways of writing expressions.

Examples:

#[Route('/posts/{slug}')]
public function getPost(
    #[MapEntity(expr: 'repository.getPostBySlug(slug)')]
    Post $post,
)

This will work as usual. No #[QueryParam] or #[BodyParam] or #[RouteParam], therefore $request->attributes are used. One can argue, why do we need #[RouteParam], if not having it the same as having it, unless you specify different name for param. I think it will be better to have it for consistency and code readability reasons.


#[Route('/posts/{slug}')]
public function getPost(
    #[RouteParam('slug')] // or just #[RouteParam]
    #[MapEntity(expr: 'repository.getPostBySlug(slug)')]
    Post $post,
)

This will work the same, but internally, EntityValueResolver will work with ParameterBag([ 'slug' => $value ]) instead of $request->attributes


Case with conflict:

// GET /posts/<categoryId>/?id=<postId>
#[Route('/posts/{id}')]
public function getPost(
    // #[RouteParam('id')] -- optional, EntityResolver will use category ID anyway
    Category $category,
    
    #[QueryParam('id')]
    #[MapEntity(expr: 'repository.find(id)')] 
    Post $post,
)

Here we explicitly define that we intend to use post ID from query for $post, we do not care about category ID.

@klkvsk
Copy link
Author

klkvsk commented Oct 25, 2024

Simplified, changes to EntityValueResolver will look like that:

final class EntityValueResolver implements ValueResolverInterface
{
    public function resolve(Request $request, ArgumentMetadata $argument, mixed $value = null): array
    {
        if (is_scalar($value)) {
            $inputBag = new ParameterBag([$argument->getName() => $value]);
        } elseif (is_array($value)) {
            $inputBag = new ParameterBag($inputBag);
        } else {
            $inputBag = $request->attributes;
        }

        // next every access to $request->attributes is replaced with $inputBag
        // everything else stays the same

As for expressions:

        // was: $variables = array_merge($request->attributes->all(), [
        $variables = array_merge($inputBag->all(), [
            'repository' => $repository,
            'request' => $request,
        ]);

Another method would be to create ParameterBag before resolve() is called, and pass to it:

public function resolve(Request $request, ArgumentMetadata $argument, ParameterBag $inputBag = null): array
{
    $inputBag ??= $request->attributes;
   ...

Again, $inputBag will be non-null only (!) if ValueConsumer attributes are defined.

@bobvandevijver
Copy link
Contributor

@klkvsk When you want to map parameters from your path to an entity it is recommended to use the same name in the path as for the method argument, which ensure that a conflict not happens. So your conflict example becomes:

// GET /posts/<categoryId>/?id=<postId>
#[Route('/posts/{category}')]
public function getPost(
    Category $category,
    
    #[QueryParam('id')]
    #[MapEntity(expr: 'repository.find(id)')] 
    Post $post,
)

@klkvsk
Copy link
Author

klkvsk commented Oct 29, 2024

I opened PR #58709 to work on this.

@carsonbot
Copy link

Thank you for this suggestion.
There has not been a lot of activity here for a while. Would you still like to see this feature?
Every feature is developed by the community.
Perhaps someone would like to try?
You can read how to contribute to get started.

@carsonbot
Copy link

Hello? This issue is about to be closed if nobody replies.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
HttpKernel RFC RFC = Request For Comments (proposals about features that you want to be discussed) Stalled
Projects
None yet
Development

No branches or pull requests

6 participants
@stof @klkvsk @derrabus @bobvandevijver @carsonbot and others