AutoMapper catches and ignores NullReferenceException #122

Closed
MatteS75 opened this Issue Sep 8, 2011 · 16 comments

Comments

Projects
None yet
5 participants

MatteS75 commented Sep 8, 2011

As written in:

http://stackoverflow.com/questions/7332426/automapper-catches-and-ignores-nullreferenceexception

Maybe this is by design, but we initially did not expect automapper to catch and ignore all NullReferenceExceptions in our mappings. We mostly use the MapFrom and create sometimes complex expressions. We want these mappings to fail if there's any exception, even a NullReferenceException, but we cant get AutoMapper to do that. Is there any way to make automapper not ignore all these exceptions without having to write a custom value resolver for every case? This would mean alot of extra code for us, so much in fact that it probably would be less code without using automapper in the first place.

These are the test that we would expect to all pass:

[TestFixture]
public class Tests
{
[SetUp]
public void Setup() { Mapper.Reset(); }

[Test]
public void ShouldThrowMappingExceptionUsingMapFromExpression()
{
    Mapper.CreateMap<Source, Destination>()
        .ForMember(d => d.DestinationMember, o => o.MapFrom(s => s.SourceMember.SourceProperty))
        ;

    Assert.Throws<AutoMapperMappingException>(() => Mapper.Map<Source, Destination>(new Source()));
}

[Test]
public void ShouldThrowMappingExceptionUsingResolveUsingExpression()
{
    Mapper.CreateMap<Source, Destination>()
        .ForMember(d => d.DestinationMember, o => o.ResolveUsing(s => s.SourceMember.SourceProperty))
        ;

    Assert.Throws<AutoMapperMappingException>(() => Mapper.Map<Source, Destination>(new Source()));
}

[Test]
public void ShouldThrowMappingExceptionUsingResolverInstance()
{
    Mapper.CreateMap<Source, Destination>()
        .ForMember(d => d.DestinationMember, o => o.ResolveUsing(new TestValueResolver()).FromMember(s => s.SourceMember))
        ;

    Assert.Throws<AutoMapperMappingException>(() => Mapper.Map<Source, Destination>(new Source()));
}

[Test]
public void ShouldThrowMappingExceptionUsingResolverType()
{
    Mapper.CreateMap<Source, Destination>()
        .ForMember(d => d.DestinationMember, o => o.ResolveUsing<TestValueResolver>().FromMember(s => s.SourceMember))
        ;

    Assert.Throws<AutoMapperMappingException>(() => Mapper.Map<Source, Destination>(new Source()));
}

}

public class Destination
{
public string DestinationMember { get; set; }
}

public class Source
{
public SourceChild SourceMember { get; set; }
}

public class SourceChild
{
public string SourceProperty { get; set; }
}

public class TestValueResolver : ValueResolver<SourceChild, string>
{
protected override string ResolveCore(SourceChild source)
{
return source.SourceProperty;
}
}

Owner

jbogard commented Nov 7, 2011

Could be addressed by allowing custom DelegateBasedResolver

Yes, is that possible today with 2.0? We had an idea to find a way using alot of reflection magic, but havnt gotten around to it yet.

Owner

jbogard commented Nov 10, 2011

I wonder - I could do this only with ResolveUsing and not in MapFrom. Since MapFrom is expression-based, there's a little bit more expectation on not allowing NREs through, but ResolveUsing you're on your own.

Would that work?

@jbogard jbogard closed this Jan 30, 2013

DanTup commented Aug 29, 2013

FWIW; I've just removed AutoMapper from a project because of this issue. I spent some time wondering why my mapping code was not working correctly. It was a NRE that caused a chunk of mapping to be skipped over, and was not me deliberately wanting to to a.b.c.d without null checks, but a genuine oversight in my code.

I don't believe swallowing exceptions (especially NREs) by default is ever an acceptable thing to do. It masks real issues and makes them really difficult to debug (since the consumer of your library probably does not expect this behaviour by default). This should be opt-in as a minimum, but since you can't ever tell which NREs are "convenience" and which are real bugs with mappings, I really don't think you should even have this "feature".

It made me chuckle that I found this blog post of yours while Googling to figure out what the problem was:

http://lostechies.com/jimmybogard/2007/04/30/swallowing-exceptions-is-hazardous-to-your-health/

I liked AutoMapper; but I just can't bring myself to use software that swallows exceptions without needing to opt-in. I'm not pleading for you to change this; it might be useful behaviour to more people than there are that dislike it; but I thought it was worth you having my thoughts.

Yikes.

DanTup commented Aug 29, 2013

:-/

I didn't mean for my comment to seem like a rant; I just wanted to document my thoughts/reasoning (constructively, hopefully) should @jbogard wish to consider them for future versions. It would be nice if this functionality was opt-in.

It's hard enough working on a huge legacy codebase (with lots of dev and lots of refactoring) without having to sink time into debugging issues because of silent exceptions :(

Owner

jbogard commented Aug 29, 2013

I guess what I'm most interested in is your specific scenario. What your
mapping looks like to understand when this behavior isn't appropriate.

On Thursday, August 29, 2013, Danny Tuppeny wrote:

:-/

I didn't mean for my comment to seem like a rant; I just wanted to
document my thoughts/reasoning (constructively, hopefully) should @jbogardhttps://github.com/jbogardwish to consider them for future versions. It would be nice if this
functionality was opt-in.

It's hard enough working on a huge legacy codebase (with lots of dev and
lots of refactoring) without having to sink time into debugging issues
because of silent exceptions :(


Reply to this email directly or view it on GitHubhttps://github.com/AutoMapper/AutoMapper/issues/122#issuecomment-23492692
.

DanTup commented Aug 29, 2013

I was new'ing up an object in the map with a few properties. One threw NRE which aborts the entire block, so the other properties failed. I don't have the code to hand and I'm on a tablet, but it was something similar to:

MapFrom/ResolveUsing(o => new Something(o.a.b.c, o.b.c.d));

If a is null, it doesn't pass null into the constructor (obviously), so the whole Something ends up null, silently. Without knowing what's going on, it was a very strange issue to understand.

I might be using this wrong, but with no XML comments explaining this behaviour, it seems way too easy to fall into this trap and lose time scratching your head.

Owner

jbogard commented Aug 29, 2013

I think then maybe the resolver is the best way to handle this? Not do NRE
swallowing for that?

MapFrom is a bit different in that it's for redirection, without complex
logic. It is supposed to mimic flattening, which does ignore nulls.

On Thursday, August 29, 2013, Danny Tuppeny wrote:

I was new'ing up an object in the map with a few properties. One threw NRE
which aborts the entire block, so the other properties failed. I don't have
the code to hand and I'm on a tablet, but it was something similar to:

MapFrom/ResolveUsing(o => new Something(o.a.b.c, o.b.c.d));

If a is null, it doesn't pass null into the constructor (obviously), so
the whole Something ends up null, silently. Without knowing what's going
on, it was a very strange issue to understand.

I might be using this wrong, but with no XML comments explaining this
behaviour, it seems way too easy to fall into this trap and lose time
scratching your head.


Reply to this email directly or view it on GitHubhttps://github.com/AutoMapper/AutoMapper/issues/122#issuecomment-23501301
.

@jbogard jbogard reopened this Aug 29, 2013

Owner

jbogard commented Aug 29, 2013

MapFrom should inspect for null, ResolveUsing should not.

DanTup commented Aug 29, 2013

Whatever you do, please a) add XML comments to the methods making it clear where exceptions will be discarded and b) document this somewhere. The reason this is so frustrating is that it's both undocumented (in XML docs and the wiki!) and unexpected :-(

Owner

jbogard commented Aug 29, 2013

Wiki is editable by anyone, and I welcome pull requests :) Valid point tho

Member

ChrisMissal commented Aug 29, 2013

MapFrom should inspect for null, ResolveUsing should not.

Is this not the current behavior? I'm pretty sure it is.

DanTup commented Aug 29, 2013

I don't believe it is. The following code has the NullReferenceException swallowed, whether you use MapFrom or ResolveUsing. I'm running this with the latest stable version from NuGet.

class Program
{
    static void Main(string[] args)
    {
        Mapper.AddProfile<MyMap>();

        try
        {
            var a = Mapper.Map<MyClass1>("");
        }
        catch (NullReferenceException)
        {
            Console.WriteLine("NullReferenceException was correctly raised.");
            return;
        }
        Console.WriteLine("NullReferenceException was silently swallowed.");
    }
}

class MyMap : Profile
{
    protected override void Configure()
    {
        This will throw a NullReferenceException (but be swallowed)
        CreateMap<string, MyClass1>()
            .ForMember(c1 => c1.Name, o => o.ResolveUsing(s => ((string)null).Length));
    }
}

class MyClass1
{
    public string Name { get; set; }
}
Member

ChrisMissal commented Aug 29, 2013

I just tested this on 2.2.1 and .ResolveUsing() doesn't throw the exception either, so I don't know what I was thinking. I was checking to see if there was a difference between 2.2.1 and 3.0.

DanTup commented Aug 29, 2013

I think I hit another more subtle issue while doing this too; ResolveUsing(/MapFrom?) expected a function that returned object; and if the returned value doesn't "fit" in the property being mapped, it also seemed to be silently thrown away. IMO that's terrible for the same reason... Unexpected and tricky to track down.

@jbogard jbogard closed this in b5e5718 Oct 28, 2013

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment