AutoMapper / AutoMapper.Collection.EFCore

EFCore support for AutoMapper.Collections
MIT License
144 stars 21 forks source link

New entities with default primary key get overridden (last wins)- default primary key values should be ignored for entity identification #52

Closed marc-mueller closed 7 months ago

marc-mueller commented 3 years ago

We currently face an issue with mapping an entity with related entities (i.e. groups --> users) where the child entity collection contains new entities (to be added) and existing entities (to be updated). The mapper identifies the the objects according to their primary keys (according to the DBContext definition). The problem is that the mapper treats the default value for a primary key the same way as a real primary key. We generate the identities on SQL server so in this example a new entity always has the id property set to 0. When mapping the collection, the mapper identifies the two new entities with id 0 as two times the primary key 0 and inserts the last object two times.

Before mapping: image

After mapping: image

The behavior is correct if a real primary key is set. For default values of the primary key this behavior should not be run and two entities with id = 0 should remain in the collection.

TylerCarlson1 commented 3 years ago

Can you provide a gist or code example that can replicate the problem for me to test?

Their are a few ideas but I don't fully understand what you are doing without seeing example code.

marc-mueller commented 3 years ago

Hi Tyler

Thanks for your answer. I'm working to get some sample code. The actual code is not that easy to extract since it is buried deeply in our framework. For my current understanding it should be reproducible with a simple implementation.

We use currently map two times when something arrives through the rest API: DTO (type: DTO) --> Detached Entity (type: Entity) --> Entity (type: Entity). The reason for this is that the business logic only knows the entity type and before storing it to the database we need to adjust a couple of properties (i.e. ignoring properties). The problem happens when we map the detached entity to the attached entity.

Pseudo Code:

var id = detachedEntity.Id;
var attachedEntity = dbcontext.GetById(id);
mapper.map(detachedEntity, attachedEntity);
return attachedEntity;

In our scenario, the entity we are updating is a group entity which has a list of users. In the database, there is currently one user associated with the group (group.Users.Count() is 1) and the entity being sent to the REST API contains 3 users in this collection, where 1 is the existing and two are new. So the two new users have Id = 0 since they will be created. We do the mapping before calling Attach or SaveChanges so the Ids are still 0 by the time the mapper is doing its work. After the mapping, the entities will be attached / added to the context.

It looks like the mapper is doing its thing as all the entities would already have been attached and therefore would have a unique id (or temporary unique id). But we use the mapper before we attach the new objects and therefore we end up with a couple of entities having 0 (default value of primary key type). It looks like that every occurrence of id = 0 will be replaced with the last one.

Without knowing the internal details of the implementation (if you can quickly send me a link to the code where this happens, that would be great), a simple check for the default value of the primary key property could help.

Determine if it is the same object or not: instead of: src.PrimaryKey == dst.PrimaryKey we could have something like: src.PrimaryKey != default(KeyType) && src.PrimaryKey == dst.PrimaryKey

Are you interested in a fully working example or are specific code path enough for further investigation?

Thanks for your help!

TylerCarlson1 commented 3 years ago

When you map the detached entity to the attached entity the source and destination types are the same. This isn't a supported use case for AutoMapper in general.

When you map to same types it doesn't like it, and you need to explicitly CreateMap for each type and subtype. Also when mapping to same mapping type, the PK check might not even be happening. I don't know this for sure.

In EFCore vs EF6 it seems better at handling child lists. If you take off the PK check based on DBContex you might still be able to save over items and EF Core will handle the detached/reattached.

Also when removing the PK checker check and see if detached to attached has same issue or gets resolved. That will point to PK being the issue vs somehow it defaults to AM's implementation and has issue when same types.

marc-mueller commented 3 years ago

Thanks for your feedback! I'm a little bit surprised that our use case is not supported by AutoMapper. It worked quite good so far except that the PK mapping was too eager and also mapped default PKs. We create a mapping configuration for EntityType <--> EntityType and it worked.

So how to continue on this? If you tell me that our use case is not supported, then we cannot expect any fix for our problem? Thanks for a quick clarification.

TylerCarlson1 commented 3 years ago

If creating mapping to and from Entity Types works then theirs your solution. It's just not the intention of Automapper to do such a thing. I would recommend going with one mapping from DTO to entity if at all possible. If not you can continue just create maps as needed to make it work.