8000 AbstractObjectNormalizer ignores the property types of discriminated classes · Issue #27607 · symfony/symfony · GitHub
[go: up one dir, main page]
Skip to content
8000

AbstractObjectNormalizer ignores the property types of discriminated classes #27607

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
timostamm opened this issue Jun 14, 2018 · 5 comments
Closed

AbstractObjectNormalizer ignores the property types of discriminated classes #27607

timostamm opened this issue Jun 14, 2018 · 5 comments
Labels
Bug Serializer Status: Needs Review

Comments

@timostamm
Copy link

Symfony version(s) affected: 4.0.1

Description

When deserializing a discriminated (abstract) type, the AbstractObjectNormalizer correctly instantiates the mapped class.

It also correctly deserializes the properties declared on the abstract type. But for properties declared on the mapped class, it ignores the property type.

Example:

/**
 * @DiscriminatorMap(
 *   typeProperty="discr",
 *   mapping={ "concrete" = ConcreteSubject::class }
 * )
 */
abstract class AbstractSubject {
    public function setA(Value $v) … // Value is correctly deserialized
}

class ConcreteSubject extends AbstractSubject {
    public function setB(Value $v) … // Value is NOT denormalized, serializer tries to set an array
}

How to reproduce

https://github.com/timostamm/symfony-serializer-discriminator-repro/blob/master/tests/DeserializerDiscriminatorTest.php

  • testDeserializePropertyOnAbstract is okay
  • testDeserializePropertyOnConcrete fails with InvalidArgumentException: Expected argument of type "App\Tests\DummySubject", "array" given.

Possible Solution

denormalize() currently calls validateAndDenormalize() with the unresolved abstract class.
validateAndDenormalize() then immediately exits because it cannot detect the type of the property.

A possible solution would be to pass the resolved classname to validateAndDenormalize() instead of the abstract classname.

See Patched_AbstractObjectNormalizer.php

Maybe the normalizer should just resolve the class at the start of denormalization. This means that instantiateObject() would already receive the resolved class and the override of AbstractNormalizer::intantiateObject() could be dropped.

See second commit

Additional context
@voodoo-dn
Copy link

Hi. The same problem. On deserialization only abstract class properties filled by desirializer, concrete properties of objects are empty.
I use Symfony 4.1 + API Platform. Pass descriminator and some properties by api and receive not fully constructed concrete object.

App\Domain\Entity\Prize\AbstractPrize:
    attributes:
        id:
            groups: ['admin_read']
        place:
            groups: ['admin_read', 'admin_write']
    discriminator_map:
        type_property: 'discriminator'
        mapping:
            money: 'App\Domain\Entity\Prize\Money\MoneyPrize'
            points: 'App\Domain\Entity\Prize\PointPrize'

App\Domain\Entity\Prize\Money\MoneyPrize:
    attributes:
        wager:
            groups: ['admin_read', 'admin_write']
        amounts:
            groups: ['admin_read', 'admin_write']

App\Domain\Entity\Prize\Money\Amount:
    attributes:
        amount:
            groups: ['admin_read', 'admin_write']
        currency:
            groups: ['admin_read', 'admin_write']

App\Domain\Entity\Prize\PointPrize:
    attributes:
        count:
            groups: ['admin_read', 'admin_write']
8000

@timostamm
Copy link
Author

@voodoo-dn could you test if my patch fixes the issue for you?

Copy Patched_ObjectNormalizer.php and Patched_AbstractObjectNormalizer.php into /src/Tests/ and add this to your service.yml:

# patch AbstractObjectNormalizer, see https://github.com/symfony/symfony/issues/27607
App\Tests\Patched_ObjectNormalizer:
  arguments:
    - '@serializer.mapping.class_metadata_factory'
    - ~
    - '@serializer.property_accessor'
    - '@property_info'
    - '@serializer.mapping.class_discriminator_resolver'
  tags:
    - {name: serializer.normalizer, priority: -999}

@seritools
Copy link

Even in 4.2 and master, this is still unresolved and makes the discriminatorMap pretty much unusable.

@Simperfit
Copy link
Contributor

@timostamm could you provide a pull request about this bug ? so we can check the provided fix directly.

Before doing could you check again against master since some work has been done on the serializer lately.

@sandergo90
Copy link
Contributor

Hi all,

I'm having the same issue with the DiscriminatorMap. If I may propose a fix, why can't we just fetch the class from the instantiated object? The $this->instantiateObject will already give you an object with the correct class. So if there would just be a $resolvedClass = \get_class($object) beneath the instantiate, that fixes the issue for me.

@sandergo90 sandergo90 mentioned this issue Jul 5, 2019
Merged
@fabpot fabpot closed this as completed Jul 8, 2019
fabpot added a commit that referenced this issue Jul 8, 2019
@fabpot
bug #32383 [Serializer] AbstractObjectNormalizer ignores the property…
6b8d4aa 
… types of discriminated classes (sandergo90)

This PR was merged into the 4.2 branch.

Discussion
----------

[Serializer] AbstractObjectNormalizer ignores the property types of discriminated classes

| Q             | A
| ------------- | ---
| Branch?       | 4.2
| Bug fix?      | yes
| New feature?  | no
| BC breaks?    | no
| Deprecations? | no
| Tests pass?   | yes
| Fixed tickets | #27607
| License       | MIT
| Doc PR        |

As discusses in ticket #27607, when using a discriminator map and the symfony serializer, things go wrong when having two classes in the discriminator map having the same property name. When the function ```validateAndDenormalize``` is called, the function ```getTypes``` would be called with the wrong class name.

If you take a look at the ```getTypes``` function below, I'm not even sure if we still need the part of the discriminator at this place because we already passed the class name of the discriminated class.

```
    /**
     * @return Type[]|null
     */
    private function getTypes(string $currentClass, string $attribute)
    {
        if (null === $this->propertyTypeExtractor) {
            return null;
        }

        if (null !== $types = $this->propertyTypeExtractor->getTypes($currentClass, $attribute)) {
            return $types;
        }

        if (null !== $this->classDiscriminatorResolver && null !== $discriminatorMapping = $this->classDiscriminatorResolver->getMappingForClass($currentClass)) {
            if ($discriminatorMapping->getTypeProperty() === $attribute) {
                return [
                    new Type(Type::BUILTIN_TYPE_STRING),
                ];
            }

            foreach ($discriminatorMapping->getTypesMapping() as $mappedClass) {
                if (null !== $types = $this->propertyTypeExtractor->getTypes($mappedClass, $attribute)) {
                    return $types;
                }
            }
        }

        return null;
    }
```

Commits
-------

9fc56c7 [Serializer]: AbstractObjectNormalizer ignores the property types of discriminated classes
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Assignees
No one assigned
Labels
Bug Serializer Status: Needs Review
Projects
None yet
Milestone
No milestone
Development
37CC

No branches or pull requests
8 participants
@fabpot @sandergo90 @xabbuh @Simperfit @timostamm @voodoo-dn @seritools @carsonbot
0