镜像站点 · 本页由第三方 GitHub 只读镜像提供,非 GitHub 官方站点,不接受任何登录或凭据输入。前往 github.com
Skip to content

Entity inheritance normalises based on parent class #2931

Closed

Description

@jmwri

Tested against 2.4.4 and 2.4.5.

It seems that the normalisation process will only use the properties defined the the parent class if you use inheritance within entities.

Example entities:

Contact.php

/**
 * Class Contact
 * @ApiResource()
 */
abstract class Contact
{
    /**
     * @ApiProperty(
     *     description="ID of contact",
     *     identifier=true,
     * )
     * @Assert\Type(type="string")
     * @Assert\Length(min="36", max="36")
     * @var ?string
     */
    protected $id;

    /**
     * @ApiProperty(
     *     description="Type of contact"
     * )
     * @Assert\NotBlank()
     * @var string
     */
    protected $type;

    /**
     * Contact constructor.
     * @param string $type
     * @param string|null $id
     */
    public function __construct(
        string $type,
        ?string $id = null
    ) {
        $this->setType($type);
        $this->setId($id);
    }

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

    /**
     * @param string $type
     * @return Contact
     */
    protected function setType(string $type): Contact
    {
        $this->type = $type;
        return $this;
    }

    /**
     * @return mixed
     */
    public function getId(): ?string
    {
        return $this->id;
    }

    /**
     * @param string $id
     * @return Contact
     */
    public function setId($id): Contact
    {
        $this->id = $id;
        return $this;
    }
}

ContactA.php

/**
 * Class ContactA
 */
class ContactA extends Contact
{
    /**
     * @ApiProperty(
     *     description="Property only existing on contact type A"
     * )
     * @var ?string
     */
    protected $propertyA;

    /**
     * ImplementationContact constructor.
     * @param string|null $id
     * @param string|null $propertyA
     */
    public function __construct(
        ?string $id = null,
        ?string $propertyA = null
    ) {
        parent::__construct('a', $id);
        $this->setPropertyA($propertyA);
    }

    /**
     * @return string|null
     */
    public function getPropertyA(): ?string
    {
        return $this->propertyA;
    }

    /**
     * @param string $propertyA
     * @return ContactA
     */
    public function setPropertyA(?string $propertyA): ContactA
    {
        $this->propertyA = $propertyA;
        return $this;
    }
}

ContactB.php

/**
 * Class ContactB
 */
class ContactB extends Contact
{
    /**
     * @ApiProperty(
     *     description="Property only existing on contact type B"
     * )
     * @var ?string
     */
    protected $propertyB;

    /**
     * ImplementationContact constructor.
     * @param string|null $id
     * @param string|null $propertyB
     */
    public function __construct(
        ?string $id = null,
        ?string $propertyB = null
    ) {
        parent::__construct('b', $id);
        $this->setPropertyA($propertyB);
    }

    /**
     * @return string|null
     */
    public function getPropertyB(): ?string
    {
        return $this->propertyB;
    }

    /**
     * @param string $propertyB
     * @return ContactB
     */
    public function setPropertyA(?string $propertyB): ContactB
    {
        $this->propertyB = $propertyB;
        return $this;
    }
}

ContactProvider.php

class ContactProvider implements
    CollectionDataProviderInterface,
    RestrictedDataProviderInterface
{
    public function supports(string $resourceClass, string $operationName = null, array $context = []): bool
    {
        return Contact::class === $resourceClass;
    }

    public function getCollection(string $resourceClass, string $operationName = null, array $context = []): array
    {
        return [
            new ContactA('c80e5f8c-005b-415d-ac95-3f493ea492ff', 'propertyA value'),
            new ContactB('0e806713-5a4c-4f76-8f37-7667e23c2d59', 'propertyB value'),
        ];
    }
}

The output of the above would be:

[
    {
        "id": "c80e5f8c-005b-415d-ac95-3f493ea492ff",
        "type": "a"
    },
    {
        "id": "0e806713-5a4c-4f76-8f37-7667e23c2d59",
        "type": "b"
    }
]

Whereas I would expect:

[
    {
        "id": "c80e5f8c-005b-415d-ac95-3f493ea492ff",
        "type": "a",
        "propertyA": "propertyA value"
    },
    {
        "id": "0e806713-5a4c-4f76-8f37-7667e23c2d59",
        "type": "b",
        "propertyB": "propertyB value"
    }
]

I've narrowed the issue down to AbstractItemNormalizer.php#L325.

Should this method actually get the property names of $classOrObject rather that $context['resource_class']?

I made a quick modification locally to see if I could get it working as expected and the following works, and have created #2932.

protected function getAllowedAttributes($classOrObject, array $context, $attributesAsString = false)
    {
        $options = $this->getFactoryOptions($context);
        $class = is_object($classOrObject) ? get_class($classOrObject) : $classOrObject;
        $propertyNames = $this->propertyNameCollectionFactory->create($class, $options);
//        $propertyNames = $this->propertyNameCollectionFactory->create($context['resource_class'], $options);

        $allowedAttributes = [];
        foreach ($propertyNames as $propertyName) {
            $propertyMetadata = $this->propertyMetadataFactory->create($class, $propertyName, $options);
//            $propertyMetadata = $this->propertyMetadataFactory->create($context['resource_class'], $propertyName, $options);

            if (
                $this->isAllowedAttribute($classOrObject, $propertyName, null, $context) &&
                (
                    isset($context['api_normalize']) && $propertyMetadata->isReadable() ||
                    isset($context['api_denormalize']) && ($propertyMetadata->isWritable() || !\is_object($classOrObject) && $propertyMetadata->isInitializable())
                )
            ) {
                $allowedAttributes[] = $propertyName;
            }
        }

        return $allowedAttributes;
    }

Activity

  1. teohhanhui commented on Jul 18, 2019

    @teohhanhui
    Contributor

    If you want information from ContactA / ContactB, they must be declared as @ApiResource too.

  2. self-assigned this
    on Jul 18, 2019
  3. jmwri commented on Jul 18, 2019

    @jmwri
    Author

    AFAIK it's not possible to define multiple types for a resource. How does IX-API currently support polymorphic endpoints? I don't think it's unreasonable to expect polymorphism to be supported as it's a well established pattern.

    In a previous version this worked, and is a perfectly valid use case. What has changed?
    I believe this is supported by Doctrine's @Discriminator decorator, does this behaviour break that?

  4. teohhanhui commented on Jul 18, 2019

    @teohhanhui
    Contributor

    @jmwri You can declare ContactA and ContactB as API resources. When you do GET /contacts, for example, you'll get the items with the most specific resource type.

    Have you tried that? 😄

    The previous behaviour was a bug that has been fixed.

  5. jmwri commented on Jul 18, 2019

    @jmwri
    Author

    Unless I misunderstand, I would need to define /contactas and /contactbs that's not polymorphic, but 2 different entities.

    I want to have a single entity with varying types.
    This is supported by OpenAPI: https://swagger.io/docs/specification/data-models/inheritance-and-polymorphism/

    Is my understanding correct?

  6. teohhanhui commented on Jul 18, 2019

    @teohhanhui
    Contributor

    @jmwri Try with GET /contacts, it should give you items with "@type": "ContactA" / "@type": "ContactB", so polymorphism works.

    But what's also important is that we do not leak information from what's not an API resource. In the case of parent classes / interfaces, it's correct to expose those properties, as the declared API resource class inherits from them. But to expose properties from any child class is wrong.

  7. jmwri commented on Jul 19, 2019

    @jmwri
    Author

    I agree that we shouldn't just show all properties of child classes. But they are defined as ApiProperty's. Is there really no way to support this?

  8. teohhanhui commented on Jul 19, 2019

    @teohhanhui
    Contributor

    We can't declare that the type is Contact but expose properties from ContactA and ContactB. That makes no sense. It'd be confusing for the client as well, to have unexpected properties that's not defined by the type, which are sometimes there and sometimes not, depending on a hidden underlying type that's not communicated to the client. The way to support this is to declare ContactA and ContactB as API resource classes, as they should be.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Metadata

Metadata

Assignees

Labels

No labels
No labels

Type

No type

Projects

No projects

    Milestone

    No milestone

    Relationships

    None yet

    Development

    No branches or pull requests

    Issue actions