schmittjoh / serializer

Library for (de-)serializing data of any complexity (supports JSON, and XML)
http://jmsyst.com/libs/serializer
MIT License
2.32k stars 587 forks source link

[3.20.0] The provided class "MyEnum" is an enum, and cannot be instantiated #1457

Closed ruudk closed 1 year ago

ruudk commented 1 year ago
Q A
Bug report? yes
Feature request? no
BC Break report? no
RFC? no

I'm super grateful for the work on enums in https://github.com/schmittjoh/serializer/pull/1448. I'm just testing it out with jms/serializer v3.20.0 together with jms/serializer-bundle dev-enable-enum and noticed something wrong.

Steps required to reproduce the problem

enum SocialPlatform : string
{
    case Spotify = 'spotify';
    case Instagram = 'instagram';
}

class MyEvent {
    public function __construct(
        private int $userId,
        private SocialPlatform $socialPlatform,
    ) {
    }
}

$object = new MyEvent(1, SocialPlatform::instagram);

// This is in a functional test, using Symfony Bundle, with `enum_support` enabled.
$this->serializer = self::getContainer()->get('jms_serializer');

$this->context = SerializationContext::create()
    ->enableMaxDepthChecks()
    ->setSerializeNull(true);

$serialized = $this->serializer->serialize($object, 'json', $this->context);
// serializes to: {"user_id":1,"social_platform":"instagram"}

$deserialized = $this->serializer->deserialize($serialized, MyEvent::class, 'json');
// fails Doctrine\Instantiator\Exception\InvalidArgumentException : The provided class "SocialPlatform" is an enum, and cannot be instantiated

Expected Result

No error, deserialized back to an object.

Actual Result

Doctrine\Instantiator\Exception\InvalidArgumentException : The provided class "SocialPlatform" is an enum, and cannot be instantiated
 /Volumes/CS/www/app/vendor/jms/serializer/src/Construction/UnserializeObjectConstructor.php:22
 /Volumes/CS/www/app/vendor/jms/serializer/src/Construction/DoctrineObjectConstructor.php:72
 /Volumes/CS/www/app/vendor/jms/serializer/src/GraphNavigator/DeserializationGraphNavigator.php:189
 /Volumes/CS/www/app/vendor/jms/serializer/src/JsonDeserializationVisitor.php:188
 /Volumes/CS/www/app/vendor/jms/serializer/src/GraphNavigator/DeserializationGraphNavigator.php:214
 /Volumes/CS/www/app/vendor/jms/serializer/src/JsonDeserializationVisitor.php:112
 /Volumes/CS/www/app/vendor/jms/serializer/src/GraphNavigator/DeserializationGraphNavigator.php:140
 /Volumes/CS/www/app/vendor/jms/serializer/src/JsonDeserializationVisitor.php:188
 /Volumes/CS/www/app/vendor/jms/serializer/src/GraphNavigator/DeserializationGraphNavigator.php:214
 /Volumes/CS/www/app/vendor/jms/serializer/src/Serializer.php:252
 /Volumes/CS/www/app/vendor/jms/serializer/src/Serializer.php:180
goetas commented 1 year ago

https://github.com/schmittjoh/JMSSerializerBundle/pull/915#issuecomment-1375348012 related

ruudk commented 1 year ago

@goetas Now that that PR is merged, is it fixed? I didn't test yet.

goetas commented 1 year ago

it should be. can you verify? you need this release of the bundle https://github.com/schmittjoh/JMSSerializerBundle/releases/tag/5.2.0

ruudk commented 1 year ago

I have the latest version of both packages. The problem is still there.

I think the issue is related to DoctrineObjectConstructor.php:72 not supporting enums yet.

ruudk commented 1 year ago

I verified that \JMS\Serializer\Handler\EnumHandler::serializeEnum is called upon serialization. But in deserialization, it never calls \JMS\Serializer\Handler\EnumHandler::deserializeEnum

ruudk commented 1 year ago

On serialization, it calls \JMS\SerializerBundle\Debug\TraceableHandlerRegistry::getHandler with $typeName = 'enum'.

On deserialization, it calls the getHandler again but this time, the $typeName = 'FQCN\ToTheEnum'

goetas commented 1 year ago

Did you maybe explicitly specify the type in the Metadata? It would be great if you could reproduce the issue? The doctrine constructor should never be used for enums

ruudk commented 1 year ago

I don't use #[Type] attributes, it's all inferred from typed properties. Maybe that's the issue?

enum SocialPlatform : string
{
    case Spotify = 'spotify';
    case Instagram = 'instagram';
}

class MyEvent {
    public function __construct(
        private int $userId,
        private SocialPlatform $socialPlatform,
    ) {
    }
}

I now understand why the typeName = 'enum' on serialization, because of https://github.com/schmittjoh/serializer/blob/master/src/EventDispatcher/Subscriber/EnumSubscriber.php Shouldn't the same be done for deserialization?

ruudk commented 1 year ago

When I manually type it does work properly:

class MyEvent {
    public function __construct(
        private int $userId,
        #[Type("enum<'SocialPlatform', 'value'>")]
        private SocialPlatform $socialPlatform,
    ) {
    }
}
ruudk commented 1 year ago

I think that the $typeName of an enum should always become enum with additionally the fqcn. Then deserialization becomes easier, as it's not confused with normal objects.

goetas commented 1 year ago

You are right, I totally forgot about the de serializeation listener 😭😭

ruudk commented 1 year ago

It can happen. Do you have time to add it? Or do you want me to try it?

goetas commented 1 year ago

I will not have time this morning, probably in the evening. If you want to try it will be certainly helpful

ruudk commented 1 year ago

I'm having a hard time understanding where the problem is. Because when I check the JsonSerializationTest I see that it already provides a lot of tests for enums. And that BaseSerializationTest::testEnum also tests deserialization.

That just passes fine.

So I suspect there is something wrong with JMSSerializerBundle. As that is what I'm using.

ruudk commented 1 year ago

Maybe the problem is that the Subscriber only changes the typeName to enum on serializer.pre_serialize. But that this type should be stored in the metadata (and cached). Because when I set a breakpoint in TraceableMetadataFactory::getMetadataForClass I see that when it loads MyEvent, the $socialPlatform has PropertyMetadata with $type set to [name: 'SocialPlatform', params: []].

ruudk commented 1 year ago

The problem is this: https://github.com/schmittjoh/serializer/blob/0fa5e4f332fdce524aef5e5d4572d98d30168437/src/Metadata/Driver/EnumPropertiesDriver.php#L42-L45

It seems that the inner driver already provides the type ([name: 'SocialPlatform', params: []]).

The driver that causes this is the TypedPropertiesDriver.

ruudk commented 1 year ago

Caused by https://github.com/schmittjoh/serializer/blob/0fa5e4f332fdce524aef5e5d4572d98d30168437/src/Metadata/Driver/TypedPropertiesDriver.php#L120-L121 because class_exists(SocialPlatform) returns true 😊

ruudk commented 1 year ago

Fixed in:

goetas commented 1 year ago

@ruudk the fix is available in https://github.com/schmittjoh/JMSSerializerBundle/pull/919 could you please test it? cc @simPod

goetas commented 1 year ago

@scyzoryck I think it addresses https://github.com/schmittjoh/serializer/pull/1463 as well, what do you think?

ruudk commented 1 year ago

Just tested it, still not working. https://github.com/schmittjoh/JMSSerializerBundle/pull/919#issuecomment-1415216916

ruudk commented 1 year ago

Don't understand why this is closed as I tested the PR and said it was not working.

goetas commented 1 year ago

I'm sorry, I missed that, 😞

ruudk commented 1 year ago

Tried it again today, and it seems to be working fine. Thank you! πŸ’™