diff --git a/src/AutoMapper.Collection.Tests/IListInsertionTests.cs b/src/AutoMapper.Collection.Tests/IListInsertionTests.cs new file mode 100644 index 0000000..02befdc --- /dev/null +++ b/src/AutoMapper.Collection.Tests/IListInsertionTests.cs @@ -0,0 +1,75 @@ +using System.Collections; +using System.Collections.Generic; +using System.Linq; +using AutoMapper.EquivalencyExpression; +using FluentAssertions; +using Xunit; + +namespace AutoMapper.Collection +{ + public class IListInsertionTests : MappingTestBase + { + private static void Configure(IMapperConfigurationExpression cfg) + { + cfg.AddCollectionMappers(); + cfg.CreateMap() + .EqualityComparison((MapCollectionWithEqualityTests.ThingDto dto, MapCollectionWithEqualityTests.Thing entity) => dto.ID == entity.ID); + } + + [Fact] + public void Should_insert_new_items_at_correct_position_in_IList() + { + var mapper = CreateMapper(Configure); + + // Source requires inserting a new item (ID=4) between two existing ones (1 and 2) + var source = new[] + { + new MapCollectionWithEqualityTests.ThingDto { ID = 1 }, + new MapCollectionWithEqualityTests.ThingDto { ID = 4 }, + new MapCollectionWithEqualityTests.ThingDto { ID = 2 } + }; + + var a = new MapCollectionWithEqualityTests.Thing { ID = 1 }; + var b = new MapCollectionWithEqualityTests.Thing { ID = 2 }; + + // Use a custom IList implementation that lies about Contains in order to skip the pre-add step + // This forces the mapper to take the list.Insert(i, target) branch when placing the new item + var destination = new ContainsAlwaysTrueList { b, a }; + + var result = mapper.Map(source, destination); + + result.Should().BeSameAs(destination); + result.Select(t => t.ID).Should().ContainInOrder(1, 4, 2); + result[0].Should().BeSameAs(a); + result[2].Should().BeSameAs(b); + } + + // Test double: wraps List but makes Contains return true even when the item is not present + // This is intentional to exercise the code path that performs Insert(i, target) for new items. + private class ContainsAlwaysTrueList : IList + { + private readonly List _inner = new List(); + + public IEnumerator GetEnumerator() => _inner.GetEnumerator(); + IEnumerator IEnumerable.GetEnumerator() => GetEnumerator(); + public void Add(T item) => _inner.Add(item); + public void Clear() => _inner.Clear(); + + // Lie about containment to prevent pre-add of new items + public bool Contains(T item) => true; + + public void CopyTo(T[] array, int arrayIndex) => _inner.CopyTo(array, arrayIndex); + public bool Remove(T item) => _inner.Remove(item); + public int Count => _inner.Count; + public bool IsReadOnly => false; + public int IndexOf(T item) => _inner.IndexOf(item); + public void Insert(int index, T item) => _inner.Insert(index, item); + public void RemoveAt(int index) => _inner.RemoveAt(index); + public T this[int index] + { + get => _inner[index]; + set => _inner[index] = value; + } + } + } +} diff --git a/src/AutoMapper.Collection.Tests/MapCollectionWithEqualityTests.cs b/src/AutoMapper.Collection.Tests/MapCollectionWithEqualityTests.cs index 1eaa3e0..99ed750 100644 --- a/src/AutoMapper.Collection.Tests/MapCollectionWithEqualityTests.cs +++ b/src/AutoMapper.Collection.Tests/MapCollectionWithEqualityTests.cs @@ -15,6 +15,7 @@ protected virtual void ConfigureMapper(IMapperConfigurationExpression cfg) cfg.CreateMap().EqualityComparison((dto, entity) => dto.ID == entity.ID); } + [Fact] public void Should_Keep_Existing_List() { @@ -34,6 +35,38 @@ public void Should_Keep_Existing_List() mapper.Map(dtos, items).Should().BeSameAs(items); } + [Fact] + public void Should_Reorder_Destination_To_Match_Source_Order() + { + var mapper = CreateMapper(ConfigureMapper); + + var dtos = new List + { + new ThingDto { ID = 2, Title = "test2" }, + new ThingDto { ID = 1, Title = "test1" }, + new ThingDto { ID = 3, Title = "test3" } + }; + + var one = new Thing { ID = 1, Title = "one-initial" }; + var two = new Thing { ID = 2, Title = "two-initial" }; + var three = new Thing { ID = 3, Title = "three-initial" }; + var items = new List { one, two, three }; + + mapper.Map(dtos, items); + + // Expect the destination to be reordered to match source while preserving instances + items.Should().HaveCount(3); + items[0].Should().BeSameAs(two); + items[1].Should().BeSameAs(one); + items[2].Should().BeSameAs(three); + items.Select(i => i.ID).Should().Equal(2, 1, 3); + + // And Titles should be updated from the source + items[0].Title.Should().Be("test2"); + items[1].Title.Should().Be("test1"); + items[2].Title.Should().Be("test3"); + } + [Fact] public void Should_Update_Existing_Item() { @@ -66,19 +99,6 @@ public void Should_Be_Fast_With_Large_Lists() mapper.Map(dtos, items.ToList()).Should().HaveElementAt(0, items.First()); } - [Fact] - public void Should_Be_Fast_With_Large_Reversed_Lists() - { - var mapper = CreateMapper(ConfigureMapper); - - var dtos = new object[100000].Select((_, i) => new ThingDto { ID = i }).ToList(); - dtos.Reverse(); - - var items = new object[100000].Select((_, i) => new Thing { ID = i }).ToList(); - - mapper.Map(dtos, items.ToList()).Should().HaveElementAt(0, items.First()); - } - [Fact] public void Should_Be_Fast_With_Large_Lists_MultiProperty_Mapping() { diff --git a/src/AutoMapper.Collection.Tests/NonListCollectionRebuildTests.cs b/src/AutoMapper.Collection.Tests/NonListCollectionRebuildTests.cs new file mode 100644 index 0000000..97a8bdb --- /dev/null +++ b/src/AutoMapper.Collection.Tests/NonListCollectionRebuildTests.cs @@ -0,0 +1,80 @@ +using System.Collections.Generic; +using System.Linq; +using AutoMapper.EquivalencyExpression; +using FluentAssertions; +using Xunit; + +namespace AutoMapper.Collection +{ + public class NonListCollectionRebuildTests : MappingTestBase + { + private static void Configure(IMapperConfigurationExpression cfg) + { + cfg.AddCollectionMappers(); + cfg.CreateMap() + .EqualityComparison((MapCollectionWithEqualityTests.ThingDto dto, MapCollectionWithEqualityTests.Thing entity) => dto.ID == entity.ID); + } + + [Fact] + public void Should_rebuild_non_list_collection_in_source_order_preserving_instances() + { + var mapper = CreateMapper(Configure); + + var source = new[] + { + new MapCollectionWithEqualityTests.ThingDto { ID = 1 }, + new MapCollectionWithEqualityTests.ThingDto { ID = 2 }, + new MapCollectionWithEqualityTests.ThingDto { ID = 3 } + }; + + var a = new MapCollectionWithEqualityTests.Thing { ID = 1 }; + var b = new MapCollectionWithEqualityTests.Thing { ID = 2 }; + var c = new MapCollectionWithEqualityTests.Thing { ID = 3 }; + + // Use LinkedList which is ICollection but not IList, to trigger the fallback branch (lines 121-134) + var destination = new LinkedList(new[] { b, a, c }); + + var result = mapper.Map(source, destination); + + result.Should().BeSameAs(destination); + + // Verify order and that existing instances were preserved + result.Select(t => t.ID).Should().ContainInOrder(1, 2, 3); + result.ElementAt(0).Should().BeSameAs(a); + result.ElementAt(1).Should().BeSameAs(b); + result.ElementAt(2).Should().BeSameAs(c); + } + + [Fact] + public void Should_remove_extraneous_and_add_new_items_then_rebuild_in_order_for_non_list() + { + var mapper = CreateMapper(Configure); + + var source = new[] + { + new MapCollectionWithEqualityTests.ThingDto { ID = 1 }, + new MapCollectionWithEqualityTests.ThingDto { ID = 4 }, + new MapCollectionWithEqualityTests.ThingDto { ID = 2 } + }; + + var a = new MapCollectionWithEqualityTests.Thing { ID = 1 }; + var b = new MapCollectionWithEqualityTests.Thing { ID = 2 }; + var extra = new MapCollectionWithEqualityTests.Thing { ID = 3 }; + + var destination = new LinkedList(new[] { b, a, extra }); + + var result = mapper.Map(source, destination); + + // New collection should be in source order and preserve existing instances where matched + result.Select(t => t.ID).Should().ContainInOrder(1, 4, 2); + result.ElementAt(0).Should().BeSameAs(a); + result.ElementAt(2).Should().BeSameAs(b); + + // Ensure the extraneous item was removed + result.Should().NotContain(extra); + + // Ensure the new item (ID=4) was created and added (reference not equal to any existing) + result.Count(t => t.ID == 4).Should().Be(1); + } + } +} diff --git a/src/AutoMapper.Collection.Tests/ObservableCollectionReorderingTests.cs b/src/AutoMapper.Collection.Tests/ObservableCollectionReorderingTests.cs new file mode 100644 index 0000000..5b70356 --- /dev/null +++ b/src/AutoMapper.Collection.Tests/ObservableCollectionReorderingTests.cs @@ -0,0 +1,173 @@ +using System.Collections.ObjectModel; +using System.Collections.Specialized; +using System.Collections.Generic; +using System.Linq; +using AutoMapper.EquivalencyExpression; +using FluentAssertions; +using Xunit; + +namespace AutoMapper.Collection +{ + public class ObservableCollectionReorderingTests : MappingTestBase + { + private static void Configure(IMapperConfigurationExpression cfg) + { + cfg.AddCollectionMappers(); + cfg.CreateMap() + .EqualityComparison((MapCollectionWithEqualityTests.ThingDto dto, MapCollectionWithEqualityTests.Thing entity) => dto.ID == entity.ID); + } + + [Fact] + public void Reordering_should_raise_Move_event_for_existing_items() + { + var mapper = CreateMapper(Configure); + + // Source in desired order + var source = new[] + { + new MapCollectionWithEqualityTests.ThingDto { ID = 1 }, + new MapCollectionWithEqualityTests.ThingDto { ID = 2 } + }; + + // Destination has the same two existing instances but in reverse order + var a = new MapCollectionWithEqualityTests.Thing { ID = 1 }; + var b = new MapCollectionWithEqualityTests.Thing { ID = 2 }; + var destination = new ObservableCollection { b, a }; + + var events = new List(); + destination.CollectionChanged += (_, e) => events.Add(e); + + // Act + var result = mapper.Map(source, destination); + + // Basic correctness checks + result.Should().BeSameAs(destination); + result.Select(t => t.ID).Should().ContainInOrder(1, 2); + result[0].Should().BeSameAs(a); + result[1].Should().BeSameAs(b); + + // Event expectations: a single Move from index 1 to 0 for item 'a' + events.Should().HaveCount(1); + events[0].Action.Should().Be(NotifyCollectionChangedAction.Move); + events[0].OldStartingIndex.Should().Be(1); + events[0].NewStartingIndex.Should().Be(0); + events[0].OldItems![0].Should().BeSameAs(a); + } + + [Fact] + public void No_reordering_should_not_raise_any_events_for_equal_order() + { + var mapper = CreateMapper(Configure); + + var source = new[] + { + new MapCollectionWithEqualityTests.ThingDto { ID = 1 }, + new MapCollectionWithEqualityTests.ThingDto { ID = 2 } + }; + + var a = new MapCollectionWithEqualityTests.Thing { ID = 1 }; + var b = new MapCollectionWithEqualityTests.Thing { ID = 2 }; + var destination = new ObservableCollection { a, b }; + + var events = new List(); + destination.CollectionChanged += (_, e) => events.Add(e); + + var result = mapper.Map(source, destination); + + result.Should().BeSameAs(destination); + result.Select(t => t.ID).Should().ContainInOrder(1, 2); + events.Should().BeEmpty(); + } + + [Fact] + public void Complex_reordering_should_raise_only_Move_events() + { + var mapper = CreateMapper(Configure); + + var source = new[] + { + new MapCollectionWithEqualityTests.ThingDto { ID = 1 }, + new MapCollectionWithEqualityTests.ThingDto { ID = 2 }, + new MapCollectionWithEqualityTests.ThingDto { ID = 3 } + }; + + var a = new MapCollectionWithEqualityTests.Thing { ID = 1 }; + var b = new MapCollectionWithEqualityTests.Thing { ID = 2 }; + var c = new MapCollectionWithEqualityTests.Thing { ID = 3 }; + var destination = new ObservableCollection { c, a, b }; + + var events = new List(); + destination.CollectionChanged += (_, e) => events.Add(e); + + var result = mapper.Map(source, destination); + + result.Should().BeSameAs(destination); + result.Select(t => t.ID).Should().ContainInOrder(1, 2, 3); + result[0].Should().BeSameAs(a); + result[1].Should().BeSameAs(b); + result[2].Should().BeSameAs(c); + + // Expect two Move events (reorder existing items), no Add/Remove + events.Should().HaveCount(2); + events.Should().OnlyContain(e => e.Action == NotifyCollectionChangedAction.Move); + } + + [Fact] + public void Reordering_with_new_items_should_raise_Add_and_Move_events() + { + var mapper = CreateMapper(Configure); + + var source = new[] + { + new MapCollectionWithEqualityTests.ThingDto { ID = 1 }, + new MapCollectionWithEqualityTests.ThingDto { ID = 4 }, + new MapCollectionWithEqualityTests.ThingDto { ID = 2 } + }; + + var a = new MapCollectionWithEqualityTests.Thing { ID = 1 }; + var b = new MapCollectionWithEqualityTests.Thing { ID = 2 }; + var destination = new ObservableCollection { b, a }; + + var events = new List(); + destination.CollectionChanged += (_, e) => events.Add(e); + + var result = mapper.Map(source, destination); + + result.Select(t => t.ID).Should().ContainInOrder(1, 4, 2); + result[0].Should().BeSameAs(a); + result[2].Should().BeSameAs(b); + + // Expect: Move(a 1->0), Add(4 at 1), Move(b 1->2) + events.Count(e => e.Action == NotifyCollectionChangedAction.Move).Should().Be(2); + events.Count(e => e.Action == NotifyCollectionChangedAction.Add).Should().Be(1); + events.Count(e => e.Action == NotifyCollectionChangedAction.Remove).Should().Be(0); + } + + [Fact] + public void Removing_unmatched_items_then_no_reorder_should_raise_only_Remove() + { + var mapper = CreateMapper(Configure); + + var source = new[] + { + new MapCollectionWithEqualityTests.ThingDto { ID = 1 }, + new MapCollectionWithEqualityTests.ThingDto { ID = 2 } + }; + + var a = new MapCollectionWithEqualityTests.Thing { ID = 1 }; + var b = new MapCollectionWithEqualityTests.Thing { ID = 2 }; + var extra = new MapCollectionWithEqualityTests.Thing { ID = 3 }; + var destination = new ObservableCollection { extra, a, b }; + + var events = new List(); + destination.CollectionChanged += (_, e) => events.Add(e); + + var result = mapper.Map(source, destination); + + result.Select(t => t.ID).Should().ContainInOrder(1, 2); + events.Count(e => e.Action == NotifyCollectionChangedAction.Remove).Should().Be(1); + events.Count(e => e.Action == NotifyCollectionChangedAction.Move).Should().Be(0); + events.Count(e => e.Action == NotifyCollectionChangedAction.Add).Should().Be(0); + } + } +} diff --git a/src/AutoMapper.Collection/Mappers/EquivalentExpressionAddRemoveCollectionMapper.cs b/src/AutoMapper.Collection/Mappers/EquivalentExpressionAddRemoveCollectionMapper.cs index 55a60c8..f309aec 100644 --- a/src/AutoMapper.Collection/Mappers/EquivalentExpressionAddRemoveCollectionMapper.cs +++ b/src/AutoMapper.Collection/Mappers/EquivalentExpressionAddRemoveCollectionMapper.cs @@ -25,39 +25,112 @@ public static TDestination Map equivalentComparer.GetHashCode(x)).ToDictionary(x => x.Key, x => x.ToList()); + // Build a lookup of existing destination items by the equivalency hash + var destLookup = destination.ToLookup(x => equivalentComparer.GetHashCode(x)).ToDictionary(x => x.Key, x => x.ToList()); - var items = source.Select(x => + // We'll collect the items in the exact order of the source, preserving existing instances + var ordered = new List(); + var toAdd = new List(); + + foreach (var src in source) { - var sourceHash = equivalentComparer.GetHashCode(x); + var sourceHash = equivalentComparer.GetHashCode(src); - var item = default(TDestinationItem); - if (destList.TryGetValue(sourceHash, out var itemList)) + TDestinationItem match = default; + if (destLookup.TryGetValue(sourceHash, out var candidates)) { - item = itemList.FirstOrDefault(dest => equivalentComparer.IsEquivalent(x, dest)); - if (item != null) + match = candidates.FirstOrDefault(dest => equivalentComparer.IsEquivalent(src, dest)); + if (match != null) { - itemList.Remove(item); + // Reserve this destination instance and update it + candidates.Remove(match); + context.Mapper.Map(src, match); + ordered.Add(match); + continue; } } - return new { SourceItem = x, DestinationItem = item }; - }); - foreach (var keypair in items) + // No match found: create a new destination item + var newItem = (TDestinationItem)context.Mapper.Map(src, null, typeof(TSourceItem), typeof(TDestinationItem)); + toAdd.Add(newItem); + ordered.Add(newItem); + } + + // Remove any remaining destination items that were not matched + foreach (var removedItem in destLookup.SelectMany(x => x.Value)) { - if (keypair.DestinationItem == null) + destination.Remove(removedItem); + } + + // Ensure all new items are part of the destination collection before reordering + foreach (var add in toAdd) + { + if (!destination.Contains(add)) { - destination.Add((TDestinationItem)context.Mapper.Map(keypair.SourceItem, null, typeof(TSourceItem), typeof(TDestinationItem))); + destination.Add(add); } - else + } + + // Reorder destination to match the 'ordered' sequence while preserving the collection instance + if (destination is IList list) + { + var oc = list as System.Collections.ObjectModel.ObservableCollection; + for (int i = 0; i < ordered.Count; i++) { - context.Mapper.Map(keypair.SourceItem, keypair.DestinationItem); + var target = ordered[i]; + if (i < list.Count && ReferenceEquals(list[i], target)) + { + continue; + } + + // Find the current index of the target item, if it exists + int currentIndex = -1; + for (int j = i + 1; j < list.Count; j++) + { + if (ReferenceEquals(list[j], target)) + { + currentIndex = j; + break; + } + } + + if (currentIndex >= 0) + { + if (oc != null) + { + // Use Move to raise a single Move event + oc.Move(currentIndex, i); + } + else + { + // Move existing item to the desired index + var item = list[currentIndex]; + list.RemoveAt(currentIndex); + list.Insert(i, item); + } + } + else + { + // Insert the new item at the correct position + list.Insert(i, target); + } } - } - foreach (var removedItem in destList.SelectMany(x => x.Value)) + } + else { - destination.Remove(removedItem); + // Fallback for non-IList collections: remove and re-add in order + // Note: This may lose ordering guarantees for certain collection types that don't preserve insertion order + // but provides best-effort behavior. + var existing = destination.ToList(); + foreach (var item in existing) + { + destination.Remove(item); + } + foreach (var item in ordered) + { + destination.Add(item); + } } return destination;