fix(metadata): fix generating discriminator dependencies (#115)

This fix a bug where when registering a class that have discriminator mapping it's sub classes would not be registered and created during warmup
jolicode · May 3, 2024 · 41b95c6 · 41b95c6
2 parents aa27772 + 88ecc0c
commit 41b95c6
Show file tree

Hide file tree

Showing 13 changed files with 66 additions and 64 deletions.
diff --git a/src/AutoMapper.php b/src/AutoMapper.php
@@ -156,10 +156,13 @@ public static function create(
         $customTransformerRegistry = new PropertyTransformerRegistry($propertyTransformers);
         $metadataRegistry = new MetadataRegistry($configuration);
         $providerRegistry = new ProviderRegistry($providers);
+        $classDiscriminatorResolver = new ClassDiscriminatorResolver($classDiscriminatorFromClassMetadata);
+
         $metadataFactory = MetadataFactory::create(
             $configuration,
             $customTransformerRegistry,
             $metadataRegistry,
+            $classDiscriminatorResolver,
             $transformerFactories,
             $classMetadataFactory,
             $nameConverter,
@@ -168,7 +171,7 @@ public static function create(
         );
 
         $mapperGenerator = new MapperGenerator(
-            new ClassDiscriminatorResolver($classDiscriminatorFromClassMetadata),
+            $classDiscriminatorResolver,
             $configuration,
             $expressionLanguage,
         );

diff --git a/src/Generator/InjectMapperMethodStatementsGenerator.php b/src/Generator/InjectMapperMethodStatementsGenerator.php
@@ -4,7 +4,6 @@
 
 namespace AutoMapper\Generator;
 
-use AutoMapper\Generator\Shared\DiscriminatorStatementsGenerator;
 use AutoMapper\Metadata\Dependency;
 use AutoMapper\Metadata\GeneratorMetadata;
 use PhpParser\Node\Arg;
@@ -29,10 +28,8 @@
  */
 final readonly class InjectMapperMethodStatementsGenerator
 {
-    public function __construct(
-        private DiscriminatorStatementsGenerator $discriminatorStatementsGeneratorSource,
-        private DiscriminatorStatementsGenerator $discriminatorStatementsGeneratorTarget,
-    ) {
+    public function __construct()
+    {
     }
 
     /**
@@ -66,8 +63,6 @@ public function getStatements(Expr\Variable $automapperRegistryVariable, Generat
 
         return [
             ...$injectMapperStatements,
-            ...$this->discriminatorStatementsGeneratorSource->injectMapperStatements($metadata),
-            ...$this->discriminatorStatementsGeneratorTarget->injectMapperStatements($metadata),
         ];
     }
 }
diff --git a/src/Generator/MapperGenerator.php b/src/Generator/MapperGenerator.php
@@ -44,17 +44,14 @@ public function __construct(
         );
 
         $this->mapMethodStatementsGenerator = new MapMethodStatementsGenerator(
-            $discriminatorStatementsGeneratorSource = new DiscriminatorStatementsGenerator($classDiscriminatorResolver, true),
-            $discriminatorStatementsGeneratorTarget = new DiscriminatorStatementsGenerator($classDiscriminatorResolver, false),
+            new DiscriminatorStatementsGenerator($classDiscriminatorResolver, true),
+            new DiscriminatorStatementsGenerator($classDiscriminatorResolver, false),
             $cachedReflectionStatementsGenerator,
             $expressionLanguage,
             $configuration->allowReadOnlyTargetToPopulate,
         );
 
-        $this->injectMapperMethodStatementsGenerator = new InjectMapperMethodStatementsGenerator(
-            $discriminatorStatementsGeneratorSource,
-            $discriminatorStatementsGeneratorTarget
-        );
+        $this->injectMapperMethodStatementsGenerator = new InjectMapperMethodStatementsGenerator();
 
         $this->disableGeneratedMapper = !$configuration->autoRegister;
     }

diff --git a/src/Generator/Shared/ClassDiscriminatorResolver.php b/src/Generator/Shared/ClassDiscriminatorResolver.php
@@ -50,7 +50,7 @@ public function getDiscriminatorPropertyMetadata(GeneratorMetadata $metadata, bo
     }
 
     /**
-     * @return array<string, string>
+     * @return array<class-string<object>, string>
      */
     public function discriminatorMapperNames(GeneratorMetadata $metadata, bool $fromSource): array
     {

diff --git a/src/Generator/Shared/DiscriminatorStatementsGenerator.php b/src/Generator/Shared/DiscriminatorStatementsGenerator.php
@@ -27,46 +27,6 @@ public function __construct(
     ) {
     }
 
-    /**
-     * @return list<Stmt>
-     */
-    public function injectMapperStatements(GeneratorMetadata $metadata): array
-    {
-        if (!$this->supports($metadata)) {
-            return [];
-        }
-
-        $discriminatorMapperNames = $this->classDiscriminatorResolver->discriminatorMapperNames($metadata, $this->fromSource);
-
-        $injectMapperStatements = [];
-
-        foreach ($discriminatorMapperNames as $typeTarget => $discriminatorMapperName) {
-            /*
-             * We inject dependencies for all the discriminator variant
-             *
-             * ```php
-             *  $this->mappers['Discriminator_Mapper_VariantA'] = $autoMapperRegistry->getMapper($source, VariantA::class);
-             *  $this->mappers['Discriminator_Mapper_VariantB'] = $autoMapperRegistry->getMapper($source, VariantB::class);
-             *  ...
-             * ```
-             */
-            $injectMapperStatements[] = new Stmt\Expression(
-                new Expr\Assign(
-                    new Expr\ArrayDimFetch(
-                        new Expr\PropertyFetch(new Expr\Variable('this'), 'mappers'),
-                        new Scalar\String_($discriminatorMapperName)
-                    ),
-                    new Expr\MethodCall(new Expr\Variable('autoMapperRegistry'), 'getMapper', [
-                        new Arg(new Scalar\String_($this->fromSource ? $typeTarget : $metadata->mapperMetadata->source)),
-                        new Arg(new Scalar\String_($this->fromSource ? $metadata->mapperMetadata->target : $typeTarget)),
-                    ])
-                )
-            );
-        }
-
-        return $injectMapperStatements;
-    }
-
     /**
      * @return list<Stmt>
      *

diff --git a/src/Metadata/MetadataFactory.php b/src/Metadata/MetadataFactory.php
@@ -22,6 +22,7 @@
 use AutoMapper\Extractor\ReadWriteTypeExtractor;
 use AutoMapper\Extractor\SourceTargetMappingExtractor;
 use AutoMapper\Extractor\WriteMutator;
+use AutoMapper\Generator\Shared\ClassDiscriminatorResolver;
 use AutoMapper\Transformer\AllowNullValueTransformerInterface;
 use AutoMapper\Transformer\ArrayTransformerFactory;
 use AutoMapper\Transformer\BuiltinTransformerFactory;
@@ -30,6 +31,7 @@
 use AutoMapper\Transformer\DateTimeTransformerFactory;
 use AutoMapper\Transformer\DependentTransformerInterface;
 use AutoMapper\Transformer\EnumTransformerFactory;
+use AutoMapper\Transformer\MapperDependency;
 use AutoMapper\Transformer\MultipleTransformerFactory;
 use AutoMapper\Transformer\NullableTransformerFactory;
 use AutoMapper\Transformer\ObjectTransformerFactory;
@@ -65,6 +67,7 @@ public function __construct(
         private readonly TransformerFactoryInterface $transformerFactory,
         private readonly EventDispatcherInterface $eventDispatcher,
         public readonly MetadataRegistry $metadataRegistry,
+        private readonly ClassDiscriminatorResolver $classDiscriminatorResolver,
     ) {
     }
 
@@ -80,7 +83,7 @@ public function getGeneratorMetadata(string $source, string $target): GeneratorM
             $metadata = $this->createGeneratorMetadata($this->metadataRegistry->get($source, $target));
             $this->generatorMetadata[$source][$target] = $metadata;
 
-            // Add dependencies to the mapper
+            // Add dependencies from transformer to the mapper
             foreach ($metadata->propertiesMetadata as $propertyMapping) {
                 if ($propertyMapping->transformer instanceof DependentTransformerInterface) {
                     foreach ($propertyMapping->transformer->getDependencies() as $mapperDependency) {
@@ -90,6 +93,25 @@ public function getGeneratorMetadata(string $source, string $target): GeneratorM
                     }
                 }
             }
+
+            // Add dependencies from discriminator to the mapper
+            if ($this->classDiscriminatorResolver->hasClassDiscriminator($metadata, true)) {
+                foreach ($this->classDiscriminatorResolver->discriminatorMapperNames($metadata, true) as $newSourceType => $mapperDependencyName) {
+                    $dependencyMetadata = $this->getGeneratorMetadata($newSourceType, $metadata->mapperMetadata->target);
+                    $mapperDependency = new MapperDependency($mapperDependencyName, $newSourceType, $metadata->mapperMetadata->target);
+
+                    $metadata->addDependency(new Dependency($mapperDependency, $dependencyMetadata));
+                }
+            }
+
+            if ($this->classDiscriminatorResolver->hasClassDiscriminator($metadata, false)) {
+                foreach ($this->classDiscriminatorResolver->discriminatorMapperNames($metadata, false) as $newTargetType => $mapperDependencyName) {
+                    $dependencyMetadata = $this->getGeneratorMetadata($metadata->mapperMetadata->source, $newTargetType);
+                    $mapperDependency = new MapperDependency($mapperDependencyName, $metadata->mapperMetadata->source, $newTargetType);
+
+                    $metadata->addDependency(new Dependency($mapperDependency, $dependencyMetadata));
+                }
+            }
         }
 
         return $this->generatorMetadata[$source][$target];
@@ -117,6 +139,19 @@ public function resolveAllMetadata(MetadataRegistry $metadataRegistry): void
                     }
                 }
             }
+
+            // Add dependencies from discriminator to the mapper
+            if ($this->classDiscriminatorResolver->hasClassDiscriminator($generatorMetadata, true)) {
+                foreach ($this->classDiscriminatorResolver->discriminatorMapperNames($generatorMetadata, true) as $newSourceType => $mapperDependencyName) {
+                    $remainingMetadata[] = $metadataRegistry->get($newSourceType, $generatorMetadata->mapperMetadata->target);
+                }
+            }
+
+            if ($this->classDiscriminatorResolver->hasClassDiscriminator($generatorMetadata, false)) {
+                foreach ($this->classDiscriminatorResolver->discriminatorMapperNames($generatorMetadata, false) as $newTargetType => $mapperDependencyName) {
+                    $remainingMetadata[] = $metadataRegistry->get($generatorMetadata->mapperMetadata->source, $newTargetType);
+                }
+            }
         }
     }
 
@@ -286,6 +321,7 @@ public static function create(
         Configuration $configuration,
         PropertyTransformerRegistry $customTransformerRegistry,
         MetadataRegistry $metadataRegistry,
+        ClassDiscriminatorResolver $classDiscriminatorResolver,
         array $transformerFactories = [],
         ClassMetadataFactory $classMetadataFactory = null,
         AdvancedNameConverterInterface $nameConverter = null,
@@ -376,6 +412,7 @@ public static function create(
             $transformerFactory,
             $eventDispatcher,
             $metadataRegistry,
+            $classDiscriminatorResolver,
         );
     }
 }
diff --git a/src/Symfony/Bundle/Resources/config/metadata.php b/src/Symfony/Bundle/Resources/config/metadata.php
@@ -8,6 +8,7 @@
 use AutoMapper\Extractor\FromSourceMappingExtractor;
 use AutoMapper\Extractor\FromTargetMappingExtractor;
 use AutoMapper\Extractor\SourceTargetMappingExtractor;
+use AutoMapper\Generator\Shared\ClassDiscriminatorResolver;
 use AutoMapper\Metadata\MetadataFactory;
 use AutoMapper\Transformer\TransformerFactoryInterface;
 use Symfony\Component\EventDispatcher\EventDispatcherInterface;
@@ -47,6 +48,7 @@
                 service(TransformerFactoryInterface::class),
                 service(EventDispatcherInterface::class),
                 service('automapper.config_mapping_registry'),
+                service(ClassDiscriminatorResolver::class),
             ])
     ;
 };
diff --git a/src/Transformer/MapperDependency.php b/src/Transformer/MapperDependency.php
@@ -14,8 +14,8 @@
 final readonly class MapperDependency
 {
     /**
-     * @param class-string $source
-     * @param class-string $target
+     * @param class-string<object>|'array' $source
+     * @param class-string<object>|'array' $target
      */
     public function __construct(
         public string $name,

diff --git a/src/Transformer/ObjectTransformer.php b/src/Transformer/ObjectTransformer.php
@@ -95,7 +95,7 @@ private function getDependencyName(): string
     }
 
     /**
-     * @return class-string<mixed>|'array'
+     * @return class-string<object>|'array'
      */
     private function getSource(): string
     {
@@ -105,7 +105,7 @@ private function getSource(): string
             /**
              * Cannot be null since we check the source type is an Object.
              *
-             * @var class-string<mixed> $sourceTypeName
+             * @var class-string<object> $sourceTypeName
              */
             $sourceTypeName = $this->sourceType->getClassName();
         }
@@ -114,7 +114,7 @@ private function getSource(): string
     }
 
     /**
-     * @return class-string<mixed>|'array'
+     * @return class-string<object>|'array'
      */
     private function getTarget(): string
     {
@@ -124,7 +124,7 @@ private function getTarget(): string
             /**
              * Cannot be null since we check the target type is an Object.
              *
-             * @var class-string<mixed> $targetTypeName
+             * @var class-string<object> $targetTypeName
              */
             $targetTypeName = $this->targetType->getClassName();
         }

diff --git a/tests/Bundle/Resources/App/Api/Processor/BookProcessor.php b/tests/Bundle/Resources/App/Api/Processor/BookProcessor.php
@@ -14,11 +14,13 @@
     public function process(mixed $data, Operation $operation, array $uriVariables = [], array $context = [])
     {
         if (!$data instanceof Book) {
-            return;
+            return null;
         }
 
         if ($operation instanceof HttpOperation && $operation->getMethod() === 'POST') {
             $data->id = random_int(1, 1000);
         }
+
+        return $data;
     }
 }
diff --git a/tests/Bundle/Resources/config/packages/automapper.yaml b/tests/Bundle/Resources/config/packages/automapper.yaml
@@ -13,6 +13,7 @@ automapper:
     mappers:
       - { source: 'AutoMapper\Tests\Bundle\Resources\App\Entity\NestedObject', target: 'array' }
       - { source: 'AutoMapper\Tests\Bundle\Resources\App\Entity\AddressDTO', target: 'array', reverse: true }
+      - { source: 'AutoMapper\Tests\Bundle\Resources\App\Entity\Pet', target: 'array', reverse: true }
       - { source: 'AutoMapper\Tests\Bundle\Resources\App\Entity\UserDTO', target: 'array', reverse: false }
       - { source: 'array', target: 'AutoMapper\Tests\Bundle\Resources\App\Entity\Order', reverse: false }
       - { source: 'AutoMapper\Tests\Bundle\Resources\App\Api\Entity\Book', target: 'array', reverse: true }
diff --git a/tests/Bundle/ServiceInstantiationTest.php b/tests/Bundle/ServiceInstantiationTest.php
@@ -44,6 +44,9 @@ public function testWarmup(): void
         self::assertFileExists(__DIR__ . '/Resources/var/cache/test/automapper/Symfony_Mapper_AutoMapper_Tests_Bundle_Resources_App_Entity_NestedObject_array.php');
         self::assertFileExists(__DIR__ . '/Resources/var/cache/test/automapper/Symfony_Mapper_AutoMapper_Tests_Bundle_Resources_App_Entity_User_array.php');
         self::assertFileExists(__DIR__ . '/Resources/var/cache/test/automapper/Symfony_Mapper_AutoMapper_Tests_Bundle_Resources_App_Entity_AddressDTO_array.php');
+        self::assertFileExists(__DIR__ . '/Resources/var/cache/test/automapper/Symfony_Mapper_AutoMapper_Tests_Bundle_Resources_App_Entity_Pet_array.php');
+        self::assertFileExists(__DIR__ . '/Resources/var/cache/test/automapper/Symfony_Mapper_AutoMapper_Tests_Bundle_Resources_App_Entity_Dog_array.php');
+        self::assertFileExists(__DIR__ . '/Resources/var/cache/test/automapper/Symfony_Mapper_AutoMapper_Tests_Bundle_Resources_App_Entity_Cat_array.php');
     }
 
     public function testAutoMapper(): void

diff --git a/tests/Metadata/MetadataFactoryTest.php b/tests/Metadata/MetadataFactoryTest.php
@@ -8,6 +8,7 @@
 use AutoMapper\Extractor\FromSourceMappingExtractor;
 use AutoMapper\Extractor\FromTargetMappingExtractor;
 use AutoMapper\Extractor\SourceTargetMappingExtractor;
+use AutoMapper\Generator\Shared\ClassDiscriminatorResolver;
 use AutoMapper\Metadata\GeneratorMetadata;
 use AutoMapper\Metadata\MetadataFactory;
 use AutoMapper\Metadata\MetadataRegistry;
@@ -88,7 +89,8 @@ protected function setUp(): void
             $fromTargetMappingExtractor,
             $transformerFactory,
             new EventDispatcher(),
-            new MetadataRegistry($configuration)
+            new MetadataRegistry($configuration),
+            new ClassDiscriminatorResolver()
         );
     }