jolicode / automapper

:rocket: Very FAST :rocket: PHP AutoMapper with on the fly code generation
https://automapper.jolicode.com/
MIT License
154 stars 15 forks source link

AutoMapper does not handle Doctrine managed entities, leading to new instances being created for existing entities #198

Open m4n50n opened 1 month ago

m4n50n commented 1 month ago

I am using AutoMapper in a Symfony project alongside Doctrine. When mapping a DTO to an entity, AutoMapper creates new instances for related entities, even if those entities already exist in the database and should be managed by Doctrine.

For example, I am mapping a Course DTO that contains a CourseType (which is an existing entity in the database). The CourseType has a valid id, and this should indicate to Doctrine that it is an existing entity, but AutoMapper is not respecting that. Instead, it creates a new CourseType instance, which is not managed by Doctrine, leading to persistence issues (e.g., Doctrine attempts to persist the new CourseType as if it were a new entity).

The only way I found to ensure that the related entity is managed by Doctrine is to manually fetch it from the database before persisting the mapped entity, which defeats the purpose of using AutoMapper to simplify this process.

What can I do? Thanks!

AntipodTX commented 3 weeks ago

i faced the same issue, but you can do better than fetch the entity from the database. AutoMapper can map to an existing entity, so what you need is to have an Entity Proxy, if it stored in a Doctrine's IdentityMap. For example, you want to map an UserDTO to UserEntity:

$this->map($userDTO, UserEntity::class);
...
private function map(mixed $dto, string $resourceClass): object
{
    if ($dto->getId() !== null) {
        $resourse = $this
            ->entityManager
            ->getUnitOfWork()
            ->tryGetById($dto->getId() , $resourceClass);
        if ($resource !== false) {
            return $this->autoMapper->map($dto, $resource);
        }
    }
    return $this->autoMapper->map($dto, $resourceClass);
}
m4n50n commented 3 weeks ago

i faced the same issue, but you can do better than fetch the entity from the database. AutoMapper can map to an existing entity, so what you need is to have an Entity Proxy, if it stored in a Doctrine's IdentityMap. For example, you want to map an UserDTO to UserEntity:

$this->map($userDTO, UserEntity::class);
...
private function map(mixed $dto, string $resourceClass): object
{
    if ($dto->getId() !== null) {
        $resourse = $this
            ->entityManager
            ->getUnitOfWork()
            ->tryGetById($dto->getId() , $resourceClass);
        if ($resource !== false) {
            return $this->autoMapper->map($dto, $resource);
        }
    }
    return $this->autoMapper->map($dto, $resourceClass);
}

Hi @AntipodTX , thank you very much for your reply. Finally I have decided to use mark-gerarts/automapper-plus-bundle, which has not given me any problem and while I find the time to investigate more, I leave this one.

Best regards!

Korbeil commented 3 weeks ago

Hey @m4n50n and @AntipodTX , thanks for reporting your issue !

I think the best here would be to add a Doctrine provider within the AutoMapper Bundle. I'm still not sure how to make it work correctly. Ideally the provider would contain something close to your last comment @m4n50n. But I don't think we would like that every time, how do you guys we should make it trigger ?

I was thinking about a context parameter, something like ['recover_from_database' => true] ? (name is just an idea, nothing temporary). Also how should it work ? If no object found in database we throw an Exception ? we map onto a new object ? There is some stuff we should define before doing such provider in my opinion !

If you have time to answer all this and give me your feedback, I can see about implementing it later on ! Baptiste

yobud commented 3 weeks ago

Hi!

I'm just facing this issue,

In my opinion, as soon as an entity is declared as an ORM entity, should always work this way :

If it has to be otherwise, I think this is for specific use cases, and this should be handled by custom provider for these edge cases.

AntipodTX commented 3 weeks ago

@Korbeil

The code that you see above it is a simplified version of the code that i am using in my Transformer. I implemented the feature mentioned here , so i can collect all static metadata information about the Entity during the mapper class generation process and store it in a generated mapper class using my own PropertyTransformer. The thing is (for example) the Entity can have a composite identifier and, IMO, it is better to collect this information once instead of doing it all the time during the mapping (and the Provider's) call. I am not against the Provider, i am just thinking out laud about the performance.

@yobud

One small correction - If DTO has not null identifier, the Entity should be fetched from the database. if it is not fetched yet