api-platform / core

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

Identifier named $id is inappropriately created #5405

Open NotionCommotion opened 1 year ago

NotionCommotion commented 1 year ago

API Platform version(s) affected: 3.1.2

Description
If an identifier named something different that $id is used, a second identifier named $id is automatically created.

How to reproduce

When using 3.0, the path to a NaicsCode resource was /naics_codes/123, but with 3.1 it is naics_codes/code=123;id=123

<?php

declare(strict_types=1);

namespace App\Entity\Specification;

use ApiPlatform\Metadata\ApiResource;
use ApiPlatform\Metadata\Get;
use ApiPlatform\Metadata\GetCollection;
use Doctrine\Common\Collections\ArrayCollection;
use Doctrine\Common\Collections\Collection;
use Doctrine\ORM\Mapping as ORM;

#[ApiResource(operations: [new Get(), new GetCollection()])]
#[ORM\Entity(readOnly: true)]
class NaicsCode
{
    #[ORM\Id]
    #[ORM\Column(type: 'integer')]
    #[ORM\GeneratedValue(strategy: 'NONE')]
    private string $code;

    #[ORM\Column(type: 'string', length: 255)]
    private string $title;

    #[ORM\ManyToOne(targetEntity: self::class, inversedBy: 'children')]
    #[ORM\JoinColumn(referencedColumnName: 'code')]
    private $parent;

    #[ORM\OneToMany(mappedBy: 'parent', targetEntity: self::class)]
    private $children;

    #[ORM\Column(type: 'string', length: 31)]
    private $type;

    public function __construct()
    {
        $this->children = new ArrayCollection();
    }

    public function getCode(): string
    {
        return $this->code;
    }

    public function getId(): string
    {
        return $this->code;
    }

    public function setCode(string $code): self
    {
        $this->code = $code;

        return $this;
    }

    public function getTitle(): string
    {
        return $this->title;
    }

    public function setTitle(string $title): self
    {
        $this->title = $title;

        return $this;
    }

    public function getParent(): ?self
    {
        return $this->parent;
    }

    public function setParent(?self $parent): self
    {
        $this->parent = $parent;

        return $this;
    }

    public function debug(int $follow=0, bool $verbose = false, array $hide=[]):array
    {
        return ['code'=>$this->code, 'title'=>$this->title, 'class'=>get_class($this)];
    }

    /**
     * @return Collection<int, self>
     */
    public function getChildren(): Collection
    {
        return $this->children;
    }

    public function addChild(self $child): self
    {
        if (!$this->children->contains($child)) {
            $this->children[] = $child;
            $child->setParent($this);
        }

        return $this;
    }

    public function removeChild(self $child): self
    {
        // set the owning side to null (unless already changed)
        if ($this->children->removeElement($child) && $child->getParent() === $this) {
            $child->setParent(null);
        }

        return $this;
    }

    public function getType(): ?string
    {
        return $this->type;
    }

    public function setType(string $type): self
    {
        $this->type = $type;

        return $this;
    }
}

Possible Solution

Not a good long term solution, but adding this appears to work.

    #[ApiProperty(identifier: false)]
    private ?int $id = null;

Additional Context

mrossard commented 1 year ago

Not a great solution either, but you might get the right behavior by explicitly specifying uriTemplates for your operations, without having to add a dummy property to your class...?

NotionCommotion commented 1 year ago

@mrossard Appreciate your suggestion. I expect it will work, however, I feel it is on par with my dummy property solution and bothered changing, and my opinion is that the right solution is not for ApiPlatform to require an ID called "id".

mrossard commented 1 year ago

I definitely agree, that just seemed "not as bad" of a solution while waiting for a real fix.

In fact, just adding #[ApiProperty(identifier: true)] to your $code might be (still not perfect but) enough...?

soyuka commented 1 year ago

is it possible that https://github.com/api-platform/core/commit/1170c384636bbad999f037cedcdb4190a8028360 fixes that issue?

NotionCommotion commented 1 year ago

@soyuka Nope

soyuka commented 1 year ago

Mhh okay I'll investigate. id is automatically detected as identifier but if this breaks it's probably not good.

Also related to this kind of work on APIs I'd suggest to split the resource from the entity (https://github.com/api-platform/core/pull/5409#discussion_r1119900564) I need to document this better though.

broncha commented 1 year ago

I debugged this, and in case where id field does not exist, the $identifiers variable is populated like this

 array:1 [
  "id" => "name=Cost of Goods Sold;id=Cost of Goods Sold"
]

I even have the operations url templates defined properly

#[ApiResource(
    operations: [
        new GetCollection(
            uriTemplate: "/accounts.{_format}",
            requirements: ['name', '.*'],
            security: "is_granted('ROLE_ACCOUNT_LIST')"
        ),
        new Get(
            uriTemplate: "/accounts/{name}.{_format}",
            requirements: ['name' => '.*'],
            security: "is_granted('ROLE_ACCOUNT_LIST')"
        ),
        new Post(
            uriTemplate: "/accounts.{_format}",
            security: "is_granted('ROLE_ACCOUNT_CREATE')"
        ),
        new Delete(
            uriTemplate: "/accounts/{name}.{_format}",
            security: "is_granted('ROLE_ACCOUNT_DELETE')"
        ),
        new Put(
            uriTemplate: "/accounts/{name}.{_format}",
            security: "is_granted('ROLE_ACCOUNT_EDIT')"
        ),
        new Patch(
            uriTemplate: "/accounts/{name}.{_format}",
            security: "is_granted('ROLE_ACCOUNT_EDIT')"
        )
    ], normalizationContext: ['groups' => ['chart']])]

adding this solves the issue as @NotionCommotion said but I really dont need that id field

#[ApiProperty(identifier: false)]
    private ?int $id = null;