fix: double mapping if a property is in the constructor and also has …

…a setter (#236) * fix: double mapping if a property is in the constructor and also has a setter * fix * add tests
rekalogika · Oct 15, 2024 · ebcf851 · ebcf851
1 parent 9a134ac
commit ebcf851
Show file tree

Hide file tree

Showing 11 changed files with 401 additions and 43 deletions.
diff --git a/CHANGELOG.md b/CHANGELOG.md
@@ -6,6 +6,7 @@
 * fix: remove unalterable check in `ObjectProcessor` to allow transformers to
   handle the current target value in all cases
 * test: test lazy constructor
+* fix: double mapping if a property is in the constructor and also has a setter
 
 ## 1.11.0
 

diff --git a/src/Transformer/Model/ConstructorArguments.php b/src/Transformer/Model/ConstructorArguments.php
@@ -25,7 +25,7 @@ final class ConstructorArguments
      */
     private array $contructorArguments = [];
 
-    private bool $variadicAdded = false;
+    private ?string $variadicName = null;
 
     /**
      * @var array<int,string>
@@ -34,7 +34,7 @@ final class ConstructorArguments
 
     public function addArgument(string $name, mixed $value): void
     {
-        if ($this->variadicAdded) {
+        if ($this->variadicName !== null) {
             throw new LogicException('Cannot add argument after variadic argument');
         }
 
@@ -44,14 +44,18 @@ public function addArgument(string $name, mixed $value): void
     /**
      * @param iterable<mixed> $value
      */
-    public function addVariadicArgument(iterable $value): void
+    public function addVariadicArgument(string $name, iterable $value): void
     {
+        if ($this->variadicName !== null) {
+            throw new LogicException('Cannot add variadic argument after variadic argument');
+        }
+
+        $this->variadicName = $name;
+
         /** @var mixed $item */
         foreach ($value as $item) {
             $this->contructorArguments[] = $item;
         }
-
-        $this->variadicAdded = true;
     }
 
     public function addUnsetSourceProperty(string $name): void
@@ -79,4 +83,24 @@ public function hasArguments(): bool
     {
         return $this->contructorArguments !== [];
     }
+
+    /**
+     * @return list<string>
+     */
+    public function getArgumentNames(): array
+    {
+        $argumentNames = [];
+
+        foreach (array_keys($this->contructorArguments) as $name) {
+            if (\is_string($name)) {
+                $argumentNames[] = $name;
+            }
+        }
+
+        if ($this->variadicName !== null) {
+            $argumentNames[] = $this->variadicName;
+        }
+
+        return $argumentNames;
+    }
 }
diff --git a/src/TransformerProcessor/ObjectProcessor/ObjectProcessor.php b/src/TransformerProcessor/ObjectProcessor/ObjectProcessor.php
@@ -78,14 +78,17 @@ public function transform(
                     extraTargetValues: $extraTargetValues,
                     context: $context,
                 );
+
+                $constructorArgumentNames = [];
             } else {
-                $target = $this->instantiateRealTarget(
+                [$target, $constructorArgumentNames] = $this->instantiateRealTarget(
                     source: $source,
                     extraTargetValues: $extraTargetValues,
                     context: $context,
                 );
             }
         } else {
+            $constructorArgumentNames = [];
             $canUseTargetProxy = false;
         }
 
@@ -111,6 +114,7 @@ public function transform(
                     source: $source,
                     target: $target,
                     context: $context,
+                    exclude: $constructorArgumentNames,
                 );
             }
 
@@ -119,6 +123,7 @@ public function transform(
                 target: $target,
                 propertyMappings: $this->getPropertyMappings(),
                 extraTargetValues: $extraTargetValues,
+                exclude: $constructorArgumentNames,
                 context: $context,
             );
         }
@@ -188,12 +193,13 @@ class: $this->metadata->getTargetClass(),
 
     /**
      * @param array<string,mixed> $extraTargetValues
+     * @return array{object,list<string>} the object & the list of constructor arguments
      */
     private function instantiateRealTarget(
         object $source,
         array $extraTargetValues,
         Context $context,
-    ): object {
+    ): array {
         $targetClass = $this->metadata->getTargetClass();
 
         // check if class is valid & instantiable
@@ -211,8 +217,10 @@ private function instantiateRealTarget(
         try {
             $reflectionClass = new \ReflectionClass($targetClass);
 
-            return $reflectionClass
+            $target = $reflectionClass
                 ->newInstanceArgs($constructorArguments->getArguments());
+
+            return [$target, $constructorArguments->getArgumentNames()];
         } catch (\TypeError | \ReflectionException $e) {
             throw new InstantiationFailureException(
                 source: $source,
@@ -244,15 +252,18 @@ private function instantiateTargetProxy(
         // create proxy initializer. this initializer will be executed when the
         // proxy is first accessed
 
+        $constructorArgumentNames = [];
+
         $initializer = function (object $target) use (
             $source,
             $context,
             $extraTargetValues,
+            &$constructorArgumentNames,
         ): void {
             // if the constructor is lazy, run it here
 
             if (!$this->metadata->constructorIsEager()) {
-                $target = $this->runConstructorManually(
+                $constructorArgumentNames = $this->runConstructorManually(
                     source: $source,
                     target: $target,
                     extraTargetValues: $extraTargetValues,
@@ -267,6 +278,7 @@ private function instantiateTargetProxy(
                 target: $target,
                 propertyMappings: $this->getLazyPropertyMappings(),
                 extraTargetValues: $extraTargetValues,
+                exclude: $constructorArgumentNames,
                 context: $context,
             );
         };
@@ -282,7 +294,7 @@ class: $targetClass,
         // if the constructor is eager, run it here
 
         if ($this->metadata->constructorIsEager()) {
-            $target = $this->runConstructorManually(
+            $constructorArgumentNames = $this->runConstructorManually(
                 source: $source,
                 target: $target,
                 extraTargetValues: $extraTargetValues,
@@ -297,6 +309,7 @@ class: $targetClass,
             target: $target,
             propertyMappings: $this->getEagerPropertyMappings(),
             extraTargetValues: $extraTargetValues,
+            exclude: $constructorArgumentNames,
             context: $context,
         );
 
@@ -305,15 +318,16 @@ class: $targetClass,
 
     /**
      * @param array<string,mixed> $extraTargetValues
+     * @return list<string> the list of the constructor arguments
      */
     private function runConstructorManually(
         object $source,
         object $target,
         array $extraTargetValues,
         Context $context,
-    ): object {
+    ): array {
         if (!method_exists($target, '__construct')) {
-            return $target;
+            return [];
         }
 
         $constructorArguments = $this->generateConstructorArguments(
@@ -341,7 +355,7 @@ private function runConstructorManually(
             );
         }
 
-        return $target;
+        return $constructorArguments->getArgumentNames();
     }
 
     /**
@@ -378,7 +392,10 @@ private function generateConstructorArguments(
                         $targetPropertyValue = [$targetPropertyValue];
                     }
 
-                    $constructorArguments->addVariadicArgument($targetPropertyValue);
+                    $constructorArguments->addVariadicArgument(
+                        $propertyMapping->getTargetProperty(),
+                        $targetPropertyValue,
+                    );
                 } else {
                     $constructorArguments->addArgument(
                         $propertyMapping->getTargetProperty(),
@@ -417,15 +434,23 @@ private function generateConstructorArguments(
     /**
      * @param array<string,PropertyMappingMetadata> $propertyMappings
      * @param array<string,mixed> $extraTargetValues
+     * @param list<string> $exclude
      */
     private function readSourceAndWriteTarget(
         object $source,
         object $target,
         array $propertyMappings,
         array $extraTargetValues,
+        array $exclude,
         Context $context,
     ): object {
         foreach ($propertyMappings as $propertyMapping) {
+            $argumentName = $propertyMapping->getTargetProperty();
+
+            if (\in_array($argumentName, $exclude, true)) {
+                continue;
+            }
+
             $target = $this->propertyProcessorFactory
                 ->getPropertyProcessor($propertyMapping)
                 ->readSourcePropertyAndWriteTargetProperty(
@@ -443,6 +468,10 @@ private function readSourceAndWriteTarget(
                 continue;
             }
 
+            if (\in_array($property, $exclude, true)) {
+                continue;
+            }
+
             $propertyMapping = $propertyMappings[$property];
 
             $target = $this->propertyProcessorFactory
@@ -458,48 +487,58 @@ private function readSourceAndWriteTarget(
         return $target;
     }
 
+    /**
+     * @param list<string> $exclude
+     */
     private function mapDynamicProperties(
         object $source,
         object $target,
+        array $exclude,
         Context $context,
     ): void {
         $sourceProperties = $this->metadata->getSourceProperties();
 
         /** @var mixed $sourcePropertyValue */
         foreach (get_object_vars($source) as $sourceProperty => $sourcePropertyValue) {
-            if (!\in_array($sourceProperty, $sourceProperties, true)) {
-                try {
-                    if (isset($target->{$sourceProperty})) {
-                        /** @psalm-suppress MixedAssignment */
-                        $currentTargetPropertyValue = $target->{$sourceProperty};
-                    } else {
-                        $currentTargetPropertyValue = null;
-                    }
+            if (\in_array($sourceProperty, $sourceProperties, true)) {
+                continue;
+            }
 
+            if (\in_array($sourceProperty, $exclude, true)) {
+                continue;
+            }
 
-                    if (
-                        $currentTargetPropertyValue === null
-                        || \is_scalar($currentTargetPropertyValue)
-                    ) {
-                        /** @psalm-suppress MixedAssignment */
-                        $targetPropertyValue = $sourcePropertyValue;
-                    } else {
-                        /** @var mixed */
-                        $targetPropertyValue = $this->mainTransformer->transform(
-                            source: $sourcePropertyValue,
-                            target: $currentTargetPropertyValue,
-                            sourceType: null,
-                            targetTypes: [],
-                            context: $context,
-                            path: $sourceProperty,
-                        );
-                    }
-                } catch (\Throwable) {
-                    $targetPropertyValue = null;
+            try {
+                if (isset($target->{$sourceProperty})) {
+                    /** @psalm-suppress MixedAssignment */
+                    $currentTargetPropertyValue = $target->{$sourceProperty};
+                } else {
+                    $currentTargetPropertyValue = null;
                 }
 
-                $target->{$sourceProperty} = $targetPropertyValue;
+
+                if (
+                    $currentTargetPropertyValue === null
+                    || \is_scalar($currentTargetPropertyValue)
+                ) {
+                    /** @psalm-suppress MixedAssignment */
+                    $targetPropertyValue = $sourcePropertyValue;
+                } else {
+                    /** @var mixed */
+                    $targetPropertyValue = $this->mainTransformer->transform(
+                        source: $sourcePropertyValue,
+                        target: $currentTargetPropertyValue,
+                        sourceType: null,
+                        targetTypes: [],
+                        context: $context,
+                        path: $sourceProperty,
+                    );
+                }
+            } catch (\Throwable) {
+                $targetPropertyValue = null;
             }
+
+            $target->{$sourceProperty} = $targetPropertyValue;
         }
     }
 }
diff --git a/tests/config/rekalogika-mapper/generated-mappings.php b/tests/config/rekalogika-mapper/generated-mappings.php
@@ -291,6 +291,30 @@
         target: \Rekalogika\Mapper\Tests\Fixtures\Basic\PersonDto::class
     );
 
+    $mappingCollection->addObjectMapping(
+        // tests/src/IntegrationTest/ConstructorAndPropertyTest.php on line 77
+        source: \Rekalogika\Mapper\Tests\Fixtures\ConstructorAndProperty\SourceObject::class,
+        target: \Rekalogika\Mapper\Tests\Fixtures\ConstructorAndProperty\ObjectWithConstructorAndSetter::class
+    );
+
+    $mappingCollection->addObjectMapping(
+        // tests/src/IntegrationTest/ConstructorAndPropertyTest.php on line 32
+        source: \Rekalogika\Mapper\Tests\Fixtures\ConstructorAndProperty\SourceObject::class,
+        target: \Rekalogika\Mapper\Tests\Fixtures\ConstructorAndProperty\ObjectWithConstructorArgumentsAndGetters::class
+    );
+
+    $mappingCollection->addObjectMapping(
+        // tests/src/IntegrationTest/ConstructorAndPropertyTest.php on line 64
+        source: \Rekalogika\Mapper\Tests\Fixtures\ConstructorAndProperty\SourceObject::class,
+        target: \Rekalogika\Mapper\Tests\Fixtures\ConstructorAndProperty\ObjectWithConstructorArgumentsAndPublicProperties::class
+    );
+
+    $mappingCollection->addObjectMapping(
+        // tests/src/IntegrationTest/ConstructorAndPropertyTest.php on line 48
+        source: \Rekalogika\Mapper\Tests\Fixtures\ConstructorAndProperty\SourceObject::class,
+        target: \Rekalogika\Mapper\Tests\Fixtures\ConstructorAndProperty\ObjectWithIdOnlyOnConstructor::class
+    );
+
     $mappingCollection->addObjectMapping(
         // tests/src/IntegrationTest/DateTimeMappingTest.php on line 195
         source: \Rekalogika\Mapper\Tests\Fixtures\DateTime\DateTimeTestObjectInterface::class,