AutoMapper / AutoMapper.Collection

AutoMapper support for updating existing collections by equivalency
MIT License
245 stars 59 forks source link

Nested collections aren't mapped correctly #132

Closed DunnJM closed 2 years ago

DunnJM commented 5 years ago

I've encountered an issue where entities with privately backed collections are not mapped correctly, namely when updating existing entities.

When mapping from the DTO to the entity, instead of updating the contents of the object, the object is substituted with a new object, causing an error regarding objects with duplicate IDs being tracked to be thrown when attempting to persist these changes.

Below is a test that shows this. If you attach an ID to the object with ID ending 003 in the entity list then observe the mapping result, the entity has been updated but it is not the same object.

using AutoMapper;
using AutoMapper.EquivalencyExpression;
using System;
using System.Collections.Generic;
using System.Linq;

namespace AutoMapperCollectionTest
{
    class Program
    {
        static void Main(string[] args)
        {
            var mapperConfiguration = new MapperConfiguration((configuration) =>
            {
                configuration.AddCollectionMappers();
                configuration.CreateMap<ListItemDTO, ListItem>().EqualityComparison((dto, entity) => dto.Id == entity.Id);
                configuration.CreateMap<TestListDTO, TestList>();
            });

            var mapper = mapperConfiguration.CreateMapper();

            var dtos = new List<ListItemDTO>()
            {
                new ListItemDTO(){ Id = Guid.Parse("{00000000-0000-0000-0000-000000000001}"), Name = "Test1"},
                new ListItemDTO(){ Id = Guid.Parse("{00000000-0000-0000-0000-000000000002}"), Name = "Test2"},
                new ListItemDTO(){ Id = Guid.Parse("{00000000-0000-0000-0000-000000000003}"), Name = "SHOULD BE UPDATED"},
            };

            var items = new List<ListItem>()
            {
                new ListItem() { Id = Guid.Parse("{00000000-0000-0000-0000-000000000003}"), Name = "Test3" },
                new ListItem() { Id = Guid.Parse("{00000000-0000-0000-0000-000000000004}"), Name = "Test4" },
            };

            var testNestedDTOList = new TestListDTO() { Items = dtos };

            var testNestedList = new TestList();

            var newNestedList= mapper.Map(testNestedDTOList , testNestedList);
        }
    }

    public class TestList
    {

        private ICollection<ListItem> _items = new List<ListItem>()
        {
                new ListItem() { Id = Guid.Parse("{00000000-0000-0000-0000-000000000003}"), Name = "Test3" },
                new ListItem() { Id = Guid.Parse("{00000000-0000-0000-0000-000000000004}"), Name = "Test4" },
        };
        public IEnumerable<ListItem> Items
        {
            get => _items.AsEnumerable();
            private set => _items = (ICollection<ListItem>)value;
        }
    }

    public class TestListDTO
    {
        public IEnumerable<ListItemDTO> Items { get; set; }
    }

    public class ListItem
    {
        public Guid Id { get; set; }
        public string Name { get; set; }
    }

    public class ListItemDTO
    {
        public Guid Id { get; set; }
        public string Name { get; set; }
    }
} 
TylerCarlson1 commented 5 years ago

It looks like it's not the fact that it's a private setter, more like it's an IEnumerable and AutoMapper.Collection looks for the destination list to be an actual ICollection and not IEnumerable, so therefore it defaults back to AutoMapper logic.

DunnJM commented 5 years ago

Thanks for the reply.

You're right in that if I change this to an ICollection then AutoMapper maps this correctly, however, I want to expose this collection as unmodifiable.

I want to be able to utilise this functionality in conjunction with EF Core. I want to have a private collection which is used as a backing field/property for a publicly exposed IENumerable. I need to map my DTO collection to the private backing field of my entity.

How can I achieve this?

TylerCarlson1 commented 5 years ago

I think you can map to private fields, but you'll have to ForMember those by hand or update the AM internals to include private members. Other than that you can make an interface and have the collection have that interface and map it to that in ForMember, or update AM config to do that. I'm assuming you don't want to expose ICollection anywhere publicly, in which case you have to do these workarounds.

Other way is to make the setter of the collection take the existing list and wrap it in a read only collection and then make the getter get that value while maintaining your original. But if you are going to add through another function that might not update the list correctly, or you'll have to write a workaround for it to update correctly.

Newmski commented 5 years ago

@DunnJM did you find a work around for this?

I have just logged the same issue on StackOverflow before I stumbled upon this issue.

https://stackoverflow.com/questions/58204117/ef-core-collections-using-automapper-collection-entityframeworkcore

If I change the Bars collection from IEnumerable to ICollection it works however I too would like to prevent the collection being modified.

DunnJM commented 5 years ago

Hi! Sorry for the late reply.

Yes I did find a solution/workaround.

Here's the test code:

namespace AutoMapperCollectionTest
{
    class Program
    {
        static void Main(string[] args)
        {
            var mapperConfiguration = new MapperConfiguration((configuration) =>
            {
                configuration.AddCollectionMappers();
                configuration.CreateMap<ListItemDTO, ListItem>().EqualityComparison((dto, entity) => dto.Id == entity.Id);
                configuration.CreateMap<TestListDTO, TestList>()
                    .ForMember(TestList.Mapper.Items, opt => opt.MapFrom(dto => dto.Items))
                    .ForMember(e => e.Items, opt => opt.Ignore());
            });

            var mapper = mapperConfiguration.CreateMapper();

            var dtos = new List<ListItemDTO>()
            {
                new ListItemDTO(){ Id = Guid.Parse("{00000000-0000-0000-0000-000000000001}"), Name = "Test1"},
                new ListItemDTO(){ Id = Guid.Parse("{00000000-0000-0000-0000-000000000002}"), Name = "Test2"},
                new ListItemDTO(){ Id = Guid.Parse("{00000000-0000-0000-0000-000000000003}"), Name = "SHOULD BE UPDATED"},
            };

            var items = new List<ListItem>()
            {
                new ListItem() { Id = Guid.Parse("{00000000-0000-0000-0000-000000000003}"), Name = "Test3" },
                new ListItem() { Id = Guid.Parse("{00000000-0000-0000-0000-000000000004}"), Name = "Test4" },
            };

            var testNestedDTOList = new TestListDTO() { Items = dtos };

            var testNestedList = new TestList();

            var newNestedList = mapper.Map(testNestedDTOList, testNestedList);

        }
    }

    public class TestList
    {

        private List<ListItem> _items = new List<ListItem>()
        {
                new ListItem() { Id = Guid.Parse("{00000000-0000-0000-0000-000000000003}"), Name = "Test3" },
                new ListItem() { Id = Guid.Parse("{00000000-0000-0000-0000-000000000004}"), Name = "Test4" },
        };
        public IEnumerable<ListItem> Items => _items.AsReadOnly();

        public static class Mapper
        {
            public static readonly Expression<Func<TestList, ICollection<ListItem>>> Items = (tl) => tl._items;
        }
    }

    public class TestListDTO
    {
        public IEnumerable<ListItemDTO> Items { get; set; }
    }

    public class ListItem
    {
        public Guid Id { get; set; }
        public string Name { get; set; }
    }

    public class ListItemDTO
    {
        public Guid Id { get; set; }
        public string Name { get; set; }
    }
}

I created a static class in my entity that maps from my public Items collection to my private Items List and exposes this expression.

Then, in the mapping configuration, just provide this expression to AutoMapper.

I seem to remember EF complained when I implemented this and I also had to construct the entity as such:

    public class TestList
    {

        private List<ListItem> _items;
        {
             _items =  = new List<ListItem>()
        };
        public IEnumerable<ListItem> Items => _items.AsReadOnly();

        public static class Mapper
        {
            public static readonly Expression<Func<TestList, ICollection<ListItem>>> Items = (tl) => tl._items;
        }
    }

It's not the prettiest solution, but it works.

Hope this helps!

jayparmar commented 2 years ago

If we use v10, we face this issue where new objects are created for destination types instead of relacing properties. Here is sample code with Automapper v10 and Autommapper.Collection v7.0.1


using AutoMapper;
using AutoMapper.EquivalencyExpression;

var sourceList = new List<Source>()
{
    new Source
    {
        Id = 1,
        val1 = 1111
    }
};

var destList = new List<Destination>()
{
    new Destination
    {
        Id = 1,
        val2 = 2222
    }
};
var configuration = new MapperConfiguration(cfg => 
{
    cfg.AddCollectionMappers();
    cfg.CreateMap<Source, Destination>()
        .EqualityComparison((dest, opt) => dest.Id == opt.Id);
});

configuration.CompileMappings();

var mapper = configuration.CreateMapper();

mapper.Map(sourceList, destList);

public class Source
{
    public int Id { get; set; }
    public int val1 { get; set; }
    public int val2 { get; set; }
}

public class Destination
{
    public int Id { get; set; }
    public int val1 { get; set; }
    public int val2 { get; set; }
}
lbargaoanu commented 11 months ago

A PR is welcome.