api-platform / core

The server component of API Platform: hypermedia and GraphQL APIs in minutes
https://api-platform.com
MIT License
2.45k stars 878 forks source link

No identifier defined "App\DTO\ActionRequest". You should add #[\ApiPlatform\Core\Annotation\ApiProperty(identifier: true)]" on the property identifying the resource." #3975

Closed sidz closed 3 years ago

sidz commented 3 years ago

API Platform version(s) affected: 2.6.0

Description
have a simple DTO with custom opreation and custom controller. After update from 2.5.9 to 2.6.0 I've got an error No identifier defined "App\DTO\ActionRequest". You should add #[\ApiPlatform\Core\Annotation\ApiProperty(identifier: true)]" on the property identifying the resource." I am not sure whether I need to add ApiProperty annotation, where and why.

How to reproduce

<?php

declare(strict_types=1);

namespace App\DTO;

use ApiPlatform\Core\Annotation\ApiResource;
use Symfony\Component\Validator\Constraints as Assert;

/**
 * @ApiResource(
 *     itemOperations={},
 *     collectionOperations={
 *         "action"={
 *             "method"="POST",
 *             "path"="/action",
 *             "controller"=Action::class,
 *             "openapi_context"={"tags"={"Action"}}
 *         }
 *     }
 * )
 */
final class ActionRequest
{
    /**
     * @Assert\NotBlank()
     */
    public string $prop1;
    /**
     * @Assert\NotBlank()
     */
    public string $prop2;
}

Additional Context Code above works fine with Api Platform 2.5.9

maks-rafalko commented 3 years ago

Related to https://github.com/api-platform/core/pull/3871

It would be nice to have some docs / upgrade instructions explaining why id is required and for what cases. For such cases as above, where we don't have item operations and use DTO just for the custom controller in POST operation, IMO id is not needed.

soyuka commented 3 years ago

identifiers are only needed for classes (DTO, POPO) marked with #[ApiResource]. App\DTO\Action is not the class shown above. @maks-rafalko please open a new issue.

maks-rafalko commented 3 years ago

@sidz could you please make the following change in your description:

- No identifier defined "App\DTO\Action"
+ No identifier defined "App\DTO\ActionRequest"

I guess this is what really happens and this is why @soyuka confused

soyuka commented 3 years ago

@maks-rafalko the class above is marked as ApiResource therefore it must have an ID as a Resource is identifiable (by an IRI) in REST terminology. If not it causes issues and should not be marked as a Resource. TLDR: There's a confusion between DTO and Resource. This use case is not what API Platform should be used for, a simple custom symfony controller is sufficient. If you want an OpenAPI support on it you can add it's path by extending the documentation as documented here.

andrewmy commented 3 years ago

There's a confusion between DTO and Resource. This use case is not what API Platform should be used for, a simple custom symfony controller is sufficient.

But https://api-platform.com/docs/core/controllers/ says:

Note: using custom controllers with API Platform is discouraged. [link to a page which encourages DTOs]

This point should be cleared up in the docs.

soyuka commented 3 years ago

Definitely. But the example from above does use a custom controller which is discouraged. What is your use case @andrewmy? Do you have an issue with identifiers? Note that DTOs do not require the ApiResource annotation, nor an identifier.

robertfausk commented 3 years ago

@soyuka I am using api-platform with a DDD and CQRS driven approach (only GET and POST routes - where the POST routes have custom validations/asserts and are executing the commands). According to https://api-platform.com/docs/core/messenger/#dispatching-a-resource-through-the-message-bus (docs of current v2.6) I can use

<?php
// api/src/Entity/ResetPasswordRequest.php

namespace App\Entity;

use ApiPlatform\Core\Annotation\ApiResource;
use Symfony\Component\Validator\Constraints as Assert;

/**
 * @ApiResource(
 *     messenger=true,
 *     collectionOperations={
 *         "post"={"status"=202}
 *     },
 *     itemOperations={},
 *     output=false
 * )
 */
final class ResetPasswordRequest
{
    /**
     * @var string
     *
     * @Assert\NotBlank
     */
    public $username;
}

but this gives me No identifier defined "App\Entity\ResetPasswordRequest". You should add #[\ApiPlatform\Core\Annotation\ApiProperty(identifier: true)]" on the property identifying the resource." in . (which is being imported from "/srv/api/config/routes/api_platform.yaml"). Make sure there is a loader supporting the "api_platform" type..

So I am unable to upgrade to latest api-platfrom/core v2.6 in my projects. Maybe it is just a missing "forgot to upgrade the docs" issue or a missing migration guide issue.

PS: Setting * @ApiProperty(identifier=true) to an attribute - e.g. on $username on given ResetPasswordRequest example - does not feel right to me.

andrewmy commented 3 years ago

@soyuka I got basically the same use case as @robertfausk

sidz commented 3 years ago

same for us

soyuka commented 3 years ago

The example is wrong in the documentation and I'll fix this as soon as possible. There's a huge confusion between DTOs and Resources in API Platform since we introduced messenger. Here's how things are supposed to work.

You should have a Resource, User, or Car that can be represented by an IRI (/users/1 or /cars/1). This IRI must exists even if the resource link gives a 404. On every operation, API Platform allows you to specify a PHP class (also called DTO or POPO) that represents the Input or the Output of that resource when you Write or when you Read.

That said, it's wrong to specify an ApiResource on something that is a DTO as a DTO has no representational state. In other words, behind your ActionRequest there should be a User resource.

Here's an example, using a Car as Resource. It is not accessible on the API and I disabled the /cars/1 operation on purpose. The creation operation (POST /cars) will be handled via an input DTO named SomeInput that goes through messenger.

<?php

namespace App\Entity;

use ApiPlatform\Core\Annotation\ApiResource;
use App\Repository\CarRepository;
use Doctrine\ORM\Mapping as ORM;
use App\Dto\SomeInput;
use ApiPlatform\Core\Action\NotFoundAction;

/**
 * @ORM\Entity()
 */
 #[ApiResource(collectionOperations: [
        "post" => ["status" => 202, "messenger" => "input", "input" => SomeInput::class, "output" => false]
    ], itemOperations: [
        "get" => ["controller" => NotFoundAction::class, "read" => false, "status" => 404]
    ]
)]
class Car
{
    /**
     * @ORM\Id()
     * @ORM\GeneratedValue()
     * @ORM\Column(type="integer")
     */
    public $id;

    /**
     * @ORM\Column(type="string", length=255)
     */
    public $name;
}

The DTO:

<?php

namespace App\Dto;

class SomeInput
{
    public string $foo;
}

Note that this class has no ApiResource, no identifiers etc. It's just a plain PHP class.

The example is up and running on : https://github.com/soyuka/api-platform-alice-sf5/tree/messenger-example

I must say that we often don't use this input or output and that a DataPersister is sufficient in most cases. It's perfectly fine to do something like this (CQRS-driven approach):

namespace App\DataPersister;

use ApiPlatform\Core\DataPersister\ContextAwareDataPersisterInterface;
use App\Entity\Car;
use App\Dto\Input;

final class CarDataPersister implements ContextAwareDataPersisterInterface
{
    public function __construct(private SerializerInterface $serializer, private RequestStack $requestStack) { }
    public function supports($data, array $context = []): bool
    {
        return $data instanceof Car && $context['collection_operation_name'] === 'post';
    }

    public function persist($data, array $context = [])
    {
        $input = $this->serializer->deserialize($requestStack->getCurrentRequest()->getContent(), Input::class, 'json'); 
        // do something with input
    }

}

The Query part of CQRS is the DataProvider in API Platform, the Command matches the DataPersister concept.

andrewmy commented 3 years ago

@soyuka thanks for the example, it reads a lot like https://api-platform.com/docs/core/dto/ though.

Does it mean then that e.g. ResetPasswordRequest: 1) should be handled by a custom controller not involving any ApiResources? 2) or should be tied to a post operation on a User resource?

If (2), how do we distinguish the post operation from one which creates a User from e.g. admin?

soyuka commented 3 years ago

should be handled by a custom controller not involving any ApiResources?

Prefer the use of a data persister or messenger, but indeed a custom controller also works.

@andrewmy please read my edit, and indeed if you want it to be linked to a Resource, in my opinion it should be tied to an operation. You can of course use a custom operation and keep the behavior of our normal resources:

#ApiResource(
  collectionOperations: ["post", "delete",
    "custom" => [ do whatever ]
  ]
) 
soyuka commented 3 years ago

I've reviewed the documentation! Sorry for the hassle, I hope that I made things clearer for everyone. Note that from the moment you declare a Resource, there must be an identifier because there must be an IRI identifying this resource. This is also why API Platform requires at least one item operation which I also fixed at https://api-platform.com/docs/core/operations/#enabling-and-disabling-operations.

Thanks everyone for the support and the bug report this definitely helps us improve our Framework a lot!

andrewmy commented 3 years ago

Thanks for the update @soyuka, much clearer now :)

comp64 commented 3 years ago

I don't understand why is there a need for an artificial exception that breaks the workflow for many of us. It did work in 2.5.9. Rather annoying breaking change unless it serves some real purpose.

maks-rafalko commented 3 years ago

@comp64 agree, but it was reconsidered. See https://github.com/api-platform/core/pull/4052

Should work again with the next patch release

gonssal commented 3 years ago

I have this same problem and I just have a question. Should I expect BC breakages like this between point releases in Api Platform? I tried to update to 2.6 and all my non-ORM ApiResources that had no identifier stopped working. If I add an artificial identifier to fix the issue, then I get a 404 "Not Found" error on them. The doc discourages using Controllers and that's why I implemented it this way, but now you recommend to use Controllers. This is really bad DX honestly.

smilesrg commented 3 years ago

I faced the same issue, but using MongoDB support, and removed doctrine bundles completely.

UPD: Solved by manually removing cache directory rm -rf /var/cache/*

No identifier defined in "App\Document\Hotel". You should add #[\ApiPlatform\Core\Annotation\ApiProperty(identifier: true)]" on the property identifying the resource." in . (which is being imported from "/extranet/config/routes/api_platform.yaml"). Make sure there is a loader supporting the "api_platform" type.

<?php

namespace App\Document;

use ApiPlatform\Core\Annotation\ApiProperty;
use ApiPlatform\Core\Annotation\ApiResource;
use Doctrine\ODM\MongoDB\Mapping\Annotations as ODM;

/**
 * @ODM\Document
 */
#[ApiResource(
    collectionOperations: ['get'],
    itemOperations: ['get'],
)]
class Hotel
{
    /**
     * @ODM\Id(strategy="INCREMENT", type="int")
     */
    #[ApiProperty(identifier: true)]
    public $id;

    /**
     * @ODM\Field
     */
    protected string $name;

    public function getId()
    {
        return $this->id;
    }

    /**
     * @param mixed $id
     * @return Hotel
     */
    public function setId($id)
    {
        $this->id = $id;

        return $this;
    }

    public function getName(): string
    {
        return $this->name;
    }

    public function setName(string $name): Hotel
    {
        $this->name = $name;

        return $this;
    }
}
jdevinemt commented 1 year ago

[...] Note that from the moment you declare a Resource, there must be an identifier because there must be an IRI identifying this resource. This is also why API Platform requires at least one item operation [...]

@soyuka I am trying to implement an endpoint for a report, which calculates gross sales for a date range based on several criteria. The data returned is organized by year, month, source. To implement this endpoint without using a custom controller, which is discouraged, it seems I would need to create a GrossSalesReportYearResult resource, with year as the identifier. I could then use a collection operation to get the GrossSalesReportYearResult results based on my search criteria. But it seems pointless to have a GET item operation for GrossSalesReportYearResult, since I can't retrieve one of these items by IRI alone, it relies on the search criteria.

Other than just using a custom controller, am I missing another solution?

I guess I'm trying create something like a singleton resource, where an identifier is not required and the IRI would just be /api/resource (no identifier). It would only implement a GET item operation and no collection operations.