Automapper: Custom Mapping Collection Produces Empty Result

Created on 19 Nov 2016 · 39Comments · Source: AutoMapper/AutoMapper

In attempting to create a custom IValueResolver that merges source and destination Collections, I'm getting an empty collection result in the destination. Here is code that demonstrates the issue;

using AutoMapper;
using System.Collections.Generic;
namespace AutoMapperTests
{
    public class AutoMapperTester
    {
        public class Source
        {
            public ICollection<string> Collection { get; set; }
        }
        public class Destination
        {
            public ICollection<string> Collection { get; set; }
        }
        public class MergeCollectionResolver : IValueResolver<Source, Destination, ICollection<string>>
        {
            public ICollection<string> Resolve(Source source, Destination destination, ICollection<string> destMember, ResolutionContext context)
            {
                foreach (string sourceItem in source.Collection)
                {
                    destMember.Add(sourceItem);
                }
                return destMember;
            }
        }
        public void Test()
        {
            Mapper.Initialize(cfg =>
            {
                cfg.CreateMap<Source, Destination>()
                    .ForMember(s => s.Collection, opt => opt.ResolveUsing(new MergeCollectionResolver()));
            });
            var source = new Source()
            {
                Collection = new List<string>() { "a", "b", "c" }
            };
            var destination = new Destination()
            {
                Collection = new List<string>() { "d", "e", "f" }
            };

            Mapper.Map<Source, Destination>(source, destination);

            // After mapping the destination collection has 0 items.
            // It should have 6.
            System.Diagnostics.Debug.Assert(destination.Collection.Count == 6);
        }
    }
}

Source

nyditot

👍1

Most helpful comment

My actual use case is a bit more complicated than the example given. What I'm really doing is mapping DTOs to Entities. The DTO contains a Collection of just the modified records as well as a Collection of key values for any records that were deleted. In the Resolver, I update the modified records and remove the deleted ones and leave all the other items in the destination's Collection member alone. It's very powerful and generic and reduces traffic to a modicum.

The method doesn't really lend itself to a TypeConverter because I'm accessing two different source members (e.g. source.SomeItems and source.SomeItems_Deleted) to resolve the destination member (e.g. destination.SomeItems). AfterMap could probably work but it would require custom logic for each of the destination's Collection members being mapped rather than a generic Resolver.

Why is it necessary to clear the destination's Collection member when said member is being reassigned? It seems kinda like setting an Int to 0 before setting it to 5 unless I'm missing something.

nyditot on 23 Nov 2016

👍3

All 39 comments

The resolved value gets mapped again and the default mechanism kicks in. You need to try something else. A type converter, after map or a custom object mapper.

lbargaoanu on 19 Nov 2016

👍1

Hmm, I don't understand why there would be a second mapping when the result is already of the desired Type but, even so, why would a second mapping discard all the items? I've been perusing the code but I'm having a hard time getting my head around the abstractions and lambdas and function building, etc. Can you direct me to where in the code the "default mechanism kicks in" for IEnumerable Types? Maybe I can graft something in there for now.

nyditot on 19 Nov 2016

The problem appears to be around CollectionMapper.cs, Line 39. If the call to Resolve returns a value, the destination member is first cleared before it is assigned the returned value, In my case the returned value IS the existing destination member value so the result actually gets cleared before it is assigned to the destination member.

I would propose that the Clear call be removed altogether since it doesn't really seem necessary OR (if I'm missing something) that a check is first made to see if the result is the same instance as the destMember and not Clear in that case.

I can get around the issue by simply creating a new instance and adding both the existing destination member items and the source member items as in the following:

public ICollection<string> Resolve(Source source, Destination destination, ICollection<string> destMember, ResolutionContext context)
{
    var result = System.Activator.CreateInstance(destMember.GetType());
    foreach (string item in destMember)
    {
        result.Add(item);
    }
    foreach (string itemin source.Collection)
    {
        result.Add(item);
    }
    return result;
}

nyditot on 20 Nov 2016

👍1

@jbogard I think mapping the result from resolvers is surprising. Maybe avoid it when possible?

lbargaoanu on 20 Nov 2016

IMO it's fine for the mapper to assign the value returned by the Resolver to the destination's member. It just needs to not mess with the original value. In my case the original and returned values were one in the same instance which is what's causing the problem. There may be other cases where the original value needs to be retained as well so I would just do the assignment and leave the original value to whatever else might be referencing it or, ultimately, the garbage collector.

nyditot on 21 Nov 2016

Is this the same behavior as 4.x? Is this a new bug introduced?

The resolver behavior was one of the original behaviors in AutoMapper. You resolve a value, then map it. Most people probably don't hit it because they'll use an assignable type.

jbogard on 23 Nov 2016

There's no bug, it just clears the collection. But, whatever you return from Resolve, it will be mapped again. This is surprising and people complain from time to time. It's true that it might break some. But if Resolve means taking over the mapping then it makes more sense. A type converter seems to me a poor workaround. AfterMap is better I think.

lbargaoanu on 23 Nov 2016

Why is it necessary to clear the destination's Collection member when said member is being reassigned? It seems kinda like setting an Int to 0 before setting it to 5 unless I'm missing something.

nyditot on 23 Nov 2016

👍3

https://github.com/TylerCarlson1/Automapper.Collection does this exact thing and is supported for 4.2.x and 5.1.x version of AutoMapper. Also helps with using Entity Keys to figure out which DTOs match Entities.

The reason the collections update as they are now is because @jbogard doesn't approve of using AutoMapper this way and suggests doing updates individually.

TylerCarlson1 on 23 Nov 2016

DISAPPROVE

On Wed, Nov 23, 2016 at 3:31 PM Tyler Carlson notifications@github.com
wrote:

https://github.com/TylerCarlson1/Automapper.Collection does this exact
thing and is supported for 4.2.x and 5.1.x version of AutoMapper. Also
helps with using Entity Keys to figure out which DTOs match Entities.

The reason the collections update as they are now is because @jbogard
https://github.com/jbogard doesn't approve of using AutoMapper this way
and suggests doing updates individually.

—
You are receiving this because you were mentioned.

Reply to this email directly, view it on GitHub
https://github.com/AutoMapper/AutoMapper/issues/1801#issuecomment-262634848,
or mute the thread
https://github.com/notifications/unsubscribe-auth/AAGYMi0kxvLKnM2VobqLVAUlTJLj3ts9ks5rBLC4gaJpZM4K3G7i
.

jbogard on 24 Nov 2016

Thanks @TylerCarlson1. This would seem to be a powerful solution to a common use case. EF reservations aside, I still don't understand why the destination's existing collection needs to be cleared before it's discarded. Is it just a coding pattern artifact or are there unintended consequences that I'm not realizing?

nyditot on 25 Nov 2016

What do you suggest is a good alternative?

On Thu, Nov 24, 2016 at 5:17 PM nyditot notifications@github.com wrote:

Thanks @TylerCarlson1 https://github.com/TylerCarlson1. This would seem
to be a powerful solution to a common use case. EF reservations aside, I
still don't understand why the destination's existing collection needs to
be cleared before it's discarded. Is it just a coding pattern artifact or
are there unintended consequences that I'm not realizing?

—
You are receiving this because you were mentioned.

Reply to this email directly, view it on GitHub
https://github.com/AutoMapper/AutoMapper/issues/1801#issuecomment-262854946,
or mute the thread
https://github.com/notifications/unsubscribe-auth/AAGYMoQp-imdmULu-0jKj4qFHTAYm_1tks5rBhsNgaJpZM4K3G7i
.

jbogard on 25 Nov 2016

My guess is it gets resolved then gets mapped over. Maybe a bug cause you resolved the list it doesn't make the property map. but still tries to clear then add the items anyways. It can't add the items because there's no property map so it doesn't execute that, but it assumes by default you would have it so it executes clear first regardless.

My best guess is it's something like that, and since it's not a valid case there's no Unit Test to check for those sort of things.

TylerCarlson1 on 25 Nov 2016

Just don't bother clearing the existing destination Collection member.

I sense there's already been some discussions on this topic but I'd like to understand your reservations about @TylerCarlson1's recommendation. This technique is saving me zillions of LOC but I want to be sure I'm not overlooking anything.

nyditot on 25 Nov 2016

@nyditot This has been discussed many times before. Search the issues.

lbargaoanu on 25 Nov 2016

The fixes for #1630, #1632 and #1645 seems to be what is causing my issue. Would a check that the value returned by the Resolver is not equal to the existing value before calling Clear make everybody happy?

nyditot on 25 Nov 2016

That's just arbitrary. You map it again or you don't.

lbargaoanu on 26 Nov 2016

If the AutoMapper.Collections package works, can we close this?

jbogard on 29 Nov 2016

I feel that it is a bug to clear the destination's Collection before replacing it. I may have reasons for wanting to keep the original data (e.g. undo/redo). If you're keeping the destination's Collection instance and repopulating it with items from the resolved Collection and then throwing the resolved collection away, then I guess you have to clear it first but I definitely find that to be surprising given that the Resolver is supposedly taking full charge of the mapping for the property value.

I do have a workaround though.

nyditot on 29 Nov 2016

No - resolver isn't supposed to be taking full charge of mapping to a
property. That's a type converter. Resolver is merely taking charge of
RESOLVING the value. It then gets mapped.

People get confused both ways. The only way I can see around this is
additional configuration (or your workaround).

fyi this is why I don't use these more advanced things if I can, like
mapping to existing objects. It just creates more opacity and confusion if
the behavior isn't exactly how you expect.

On Mon, Nov 28, 2016 at 8:05 PM, nyditot notifications@github.com wrote:

I feel that it is a bug to clear the destination's Collection before
replacing it. I may have reasons for wanting to keep the original data
(e.g. undo/redo). If you're keeping the destination's Collection instance
and repopulating it with items from the resolved Collection and then
throwing the resolved collection away, then I guess you have to clear it
first but I definitely find that to be surprising given that the Resolver
is supposedly taking full charge of the mapping for the property value.

I do have a workaround though.

—
You are receiving this because you were mentioned.
Reply to this email directly, view it on GitHub
https://github.com/AutoMapper/AutoMapper/issues/1801#issuecomment-263455206,
or mute the thread
https://github.com/notifications/unsubscribe-auth/AAGYMn0JXc9iD04s6u2oX5wQRbsJKqPfks5rC4iFgaJpZM4K3G7i
.

jbogard on 29 Nov 2016

Ok I think I finally get it. I was taking "resolving the value" to mean "providing the destination's final property value" when, in fact, it means "providing a value that is then mapped to the destination's final property value". In the simple examples in the documentation, either interpretation produces that same result so I guess that's where I went wrong.

I think what @TylerCarlson1, et. al. are doing is extraordinarily powerful in that it allows database updates to be performed with a modicum of data transfer. It's a big deal for me and I would love to see this technique become part of the AM core. In the meantime, I would agree that the code is performing as expected and that this issue can be closed.

nyditot on 29 Nov 2016

AM.Collections was originally a PR for AM, about 2+ years old now. I just gave up on it getting into AM and decided to make my own NuGet for it. I believe this has been one of the more often requested features in the past.

I have no control on what eventually gets merged in AM, so I just plug it whenever I can. :)

TylerCarlson1 on 29 Nov 2016

Ha! I don't mind it as a second package. There's just already enough code in AutoMapper that I don't use and therefore is more difficult to support. For big new features, unless I intend to support it forever, I don't want to add in. My reverse maps are maybe....5% of what I have? So I just don't have the use cases to make sure it works in the general sense.

jbogard on 30 Nov 2016

I feel your pain brother and I'm humbled by and extremely grateful for what you've done thus far but I think you may be undervaluing the usefulness of this feature.

nyditot on 30 Nov 2016

Maybe have the main markdown page or the wiki have extension section saying this is available, with full disclosure that you don't approve but some people think it's important. Also you could include AutoMapper.EF6, AutoMapper.Data (if it ever gets fixed that is), AutoMapper.DependencyInjection, ect in there as well.

TylerCarlson1 on 30 Nov 2016

I'm not discounting that other people find value. Really I just want Tyler
to support it since he actually uses it. Different repo is the easiest way
to have that happen.

Maybe I can also add info on the website?

On Wed, Nov 30, 2016 at 4:26 PM nyditot notifications@github.com wrote:

I feel your pain brother and I'm humbled by and extremely grateful for
what you've done thus far but I think you may be undervaluing the
usefulness of this feature.

—
You are receiving this because you were mentioned.

Reply to this email directly, view it on GitHub
https://github.com/AutoMapper/AutoMapper/issues/1801#issuecomment-264000270,
or mute the thread
https://github.com/notifications/unsubscribe-auth/AAGYMl-7nRClxK6ENa4Ow9_vI3aDc3sqks5rDenugaJpZM4K3G7i
.

jbogard on 1 Dec 2016

Fair enough.

nyditot on 1 Dec 2016

That's fine with me. I maintain it now anyways.

So if it's not going into code base maybe it's just getting it more exposure, so that people who might want it can find it without getting lucky with search or reply in issue request. Like adding links to the github page in various places.

TylerCarlson1 on 1 Dec 2016

Should it go under the AutoMapper org? I'm happy to do that. Then I can
have it use the appveyor CI and all that junk. I'd then add you as a
maintainer to that repo.

On Wed, Nov 30, 2016 at 8:13 PM, Tyler Carlson notifications@github.com
wrote:

That's fine with me. I maintain it now anyways.

So if it's not going into code base maybe it's just getting it more
exposure, so that people who might want it can find it without getting
lucky with search or reply in issue request. Like adding links to the
github page in various places.

—
You are receiving this because you were mentioned.
Reply to this email directly, view it on GitHub
https://github.com/AutoMapper/AutoMapper/issues/1801#issuecomment-264058805,
or mute the thread
https://github.com/notifications/unsubscribe-auth/AAGYMtNiVaTvh1LRtPjBAirr7CJLBTJOks5rDi0vgaJpZM4K3G7i
.

jbogard on 1 Dec 2016

That would be great. I wouldn't have to manually build and all that extra hassle.

I'm happy to support it as an official/semi-official extension of AutoMapper.

TylerCarlson1 on 1 Dec 2016

Added you as an org member, so you should be able to transfer:

https://help.github.com/articles/transferring-a-repository-owned-by-your-personal-account/

On Thu, Dec 1, 2016 at 10:28 AM Tyler Carlson notifications@github.com
wrote:

That would be great. I wouldn't have to manually build and all that extra
hassle.

I'm happy to support it as an official/semi-official extension of
AutoMapper.

—
You are receiving this because you were mentioned.

Reply to this email directly, view it on GitHub
https://github.com/AutoMapper/AutoMapper/issues/1801#issuecomment-264202906,
or mute the thread
https://github.com/notifications/unsubscribe-auth/AAGYMrkPRO-Om4la5kLMEqqX03bSrKCeks5rDueCgaJpZM4K3G7i
.

jbogard on 2 Dec 2016

Done. Kind of surreal right now.

TylerCarlson1 on 2 Dec 2016

It would be awesome if an article was added to the wiki for this, just to raise the profile even further. It took a couple of days for me to figure out how to migrate my generic entity collection mapper from my use of AM 3.2.1. @TylerCarlson1, thanks for your efforts on this!

rocknet on 3 Mar 2017

Add it to the wiki! Anyone can edit.

On Fri, Mar 3, 2017 at 11:24 AM, Chris Rock notifications@github.com
wrote:

It would be awesome if an article was added to the wiki for this, just to
raise the profile even further. It took a couple of days for me to figure
out how to migrate my generic entity collection mapper from my use of AM
3.2.1. @TylerCarlson1 https://github.com/TylerCarlson1, thanks for your
efforts on this!

—
You are receiving this because you were mentioned.
Reply to this email directly, view it on GitHub
https://github.com/AutoMapper/AutoMapper/issues/1801#issuecomment-284015860,
or mute the thread
https://github.com/notifications/unsubscribe-auth/AAGYMklBc03ZNoR8qkatUQsriymN3AUpks5riEzSgaJpZM4K3G7i
.

jbogard on 3 Mar 2017

I feel that it is a bug to clear the destination's Collection before replacing it. I may have reasons for wanting to keep the original data (e.g. undo/redo). If you're keeping the destination's Collection instance and repopulating it with items from the resolved Collection and then throwing the resolved collection away, then I guess you have to clear it first but I definitely find that to be surprising given that the Resolver is supposedly taking full charge of the mapping for the property value.

I do have a workaround though.

What was your workaround? How do you handle this for EF?