Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

MessageMapper.Initialize NullReferenceException due to incorrect treatment of generic types #1737

Closed
marinkobabic opened this issue Nov 6, 2013 · 4 comments
Labels
Milestone

Comments

@marinkobabic
Copy link
Contributor

If in the domain the following abstract class is defined, the bus will during the startup cause a NullReferenceException

public abstract class TaskDataCommandBase<T> : ICommand where T : BaseTaskData
{
    public T TaskData { get; set; }

    protected TaskDataCommandBase()
    {
    }

    protected TaskDataCommandBase(T taskData)
    {
        this.TaskData = taskData;
    }
}

The problem here is when the method GetTypeName(Type t) gets the property T TaskData as input, the t.FullName is then null. ContainsKey using null causes the exception. Here the stacktrace

[ArgumentNullException: Value cannot be null.
Parameter name: key]
   System.Collections.Generic.Dictionary`2.FindEntry(TKey key) +11064477
   System.Collections.Generic.Dictionary`2.ContainsKey(TKey key) +4
   NServiceBus.MessageInterfaces.MessageMapper.Reflection.MessageMapper.InitType(Type t, ModuleBuilder moduleBuilder) in :0
   NServiceBus.MessageInterfaces.MessageMapper.Reflection.MessageMapper.InitType(Type t, ModuleBuilder moduleBuilder) in :0
   NServiceBus.MessageInterfaces.MessageMapper.Reflection.MessageMapper.Initialize(IEnumerable`1 types) in :0
   NServiceBus.Serializers.XML.Config.MessageTypesInitializer.Run() in :0
   NServiceBus.Configure.<Initialize>b__14(IWantToRunWhenConfigurationIsComplete o) in :0
   System.Collections.Generic.List`1.ForEach(Action`1 action) +10996297
   NServiceBus.Configure.Initialize() in :0
   NServiceBus.Configure.CreateBus() in :0

Just to make it clear, I can't actually go on with the development, because the bus is not able to start.

The minimum I expect is to check for abstract classes, because it makes no sense to map them. This should be an easy fix.

@marinkobabic
Copy link
Contributor Author

To avoid the issue I removed the generic base class and implemented a generic interface. The hope was, that the bus is not trying to create an instance of an interface. Nope, the MessageMapper is not tested well so I have now another exception

[TypeLoadException: Type 'PubliWeb.EwrDomain.Commands.ITaskDataCommand`1__impl' from assembly 'NServiceBus.Scheduling.Messages__impl, Version=0.0.0.0, Culture=neutral, PublicKeyToken=null' has a field of an illegal type.]
   System.Reflection.Emit.TypeBuilder.TermCreateClass(RuntimeModule module, Int32 tk, ObjectHandleOnStack type) +0
   System.Reflection.Emit.TypeBuilder.CreateTypeNoLock() +1059
   System.Reflection.Emit.TypeBuilder.CreateType() +82
   NServiceBus.MessageInterfaces.MessageMapper.Reflection.MessageMapper.CreateTypeFrom(Type t, ModuleBuilder moduleBuilder) in :0
   NServiceBus.MessageInterfaces.MessageMapper.Reflection.MessageMapper.GenerateImplementationFor(Type interfaceType, ModuleBuilder moduleBuilder) in :0
   NServiceBus.MessageInterfaces.MessageMapper.Reflection.MessageMapper.InitType(Type t, ModuleBuilder moduleBuilder) in :0
   NServiceBus.MessageInterfaces.MessageMapper.Reflection.MessageMapper.Initialize(IEnumerable`1 types) in :0
   NServiceBus.Serializers.XML.Config.MessageTypesInitializer.Run() in :0
   NServiceBus.Configure.<Initialize>b__14(IWantToRunWhenConfigurationIsComplete o) in :0
   System.Collections.Generic.List`1.ForEach(Action`1 action) +10996297
   NServiceBus.Configure.Initialize() in :0
   NServiceBus.Configure.CreateBus() in :0
   PubliWeb.Infrastructure.Host.Bus.BusExtensions.StartNsbBus(BootstrapperConfigBus bootstrapperConfig, Boolean doInstall) +86
   PubliWeb.EwrDomainServer.WebApiApplication.Application_Start() in c:\Dev\PubliWebDomain\EwrDomainServer\Main\Source\Global.asax.cs:47

@johnsimons
Copy link
Member

@MBabi are you able to provide us with a failing unit test?
And maybe a fix 😄

@marinkobabic
Copy link
Contributor Author

Hi John

I think the fix is simple. All you need to change is to add the similar condition like in the ConfigUnicastBus.IsMessageHandler method. Here the corrected version of IsMessageType

/// <summary>
        /// Returns true if the given type is a message type.
        /// </summary>
        public static bool IsMessageType(Type t)
        {
            try
            {
                if (t.IsGenericTypeDefinition)
                {
                    return false;
                }

                return MessagesConventionCache.ApplyConvention(t,
                                                               type => IsMessageTypeAction(type) ||
                                                                       IsCommandTypeAction(type) ||
                                                                       IsEventTypeAction(type) ||
                                                                       IsInSystemConventionList(type));
            }
            catch (Exception ex)
            {
                throw new MessageConventionException("Failed to evaluate Message convention. See inner exception for details.", ex);           
            }
        }

The other options is to fix it directory in the MessageMapper.cs

old new 
... ... @@ -53,7 +53,7 @@ public void InitType(Type t, ModuleBuilder moduleBuilder)
53  53                   return;
54  54               }
55  55   
56      -            if (t.IsSimpleType())
    56  +            if (t.IsSimpleType() || t.IsGenericTypeDefinition)
57  57               {
58  58                   return;
59  59               }
... ... @@ -339,7 +339,7 @@ public Type GetMappedTypeFor(Type t)
339 339              {
340 340                  Type result;
341 341                  concreteToInterfaceTypeMapping.TryGetValue(t, out result);
342     -                if (result != null)
    342 +                if (result != null || t.IsGenericTypeDefinition)
343 343                  {
344 344                      return result;
345 345                  }

Here some tests to reproduce the issue:

namespace NServiceBus.MessageInterfaces.Tests
{
    using NUnit.Framework;

    [TestFixture]
    public class When_mapping_usinggenerics
    {
        IMessageMapper mapper;

        [SetUp]
        public void SetUp()
        {
            mapper = new MessageMapper.Reflection.MessageMapper();
        }

        [Test]
        public void Class_abstract_generic_with_only_properties_generic_should_not_be_mapped()
        {
            var genericClassType = typeof(GenericAbstractCommand<>);
            mapper.Initialize(new[] { genericClassType });
            Assert.Null(mapper.GetMappedTypeFor(genericClassType));
        }

        [Test]
        public void Class_generic_with_only_properties_generic_should_not_be_mapped()
        {
            var abstractClassType = typeof(GenericCommand<>);
            mapper.Initialize(new[] { abstractClassType });
            Assert.Null(mapper.GetMappedTypeFor(abstractClassType));
        }

        [Test]
        public void Class_derived_from_generic_with_only_properties_generic_should_be_mapped()
        {
            var abstractClassType = typeof(DerivedGenericCommand);
            mapper.Initialize(new[] { abstractClassType });
            Assert.NotNull(mapper.GetMappedTypeFor(abstractClassType));
        }

        [Test]
        public void Class_concrete_generic_with_only_properties_generic_should_be_mapped()
        {
            var abstractClassType = typeof(GenericCommand<Data>);
            mapper.Initialize(new[] { abstractClassType });
            Assert.NotNull(mapper.GetMappedTypeFor(abstractClassType));
        }

        [Test]
        public void Class_abstract_with_only_properties_should_be_mapped()
        {
            var abstractClassType = typeof(SimpleAbstractClass);
            mapper.Initialize(new[] { abstractClassType });
            Assert.NotNull(mapper.GetMappedTypeFor(abstractClassType));
        }

        [Test]
        public void Class_abstract_with_methods_should_not_be_mapped()
        {
            var abstractClassType = typeof(SimpleAbstractClassWithMethods);
            mapper.Initialize(new[] { abstractClassType });
            Assert.Null(mapper.GetMappedTypeFor(abstractClassType));
        }

        [Test]
        public void Interfaces_generic_with_only_properties_should_not_be_mapped()
        {
            var genericInterfaceType = typeof(InterfaceGenericWithProperties<>);
            mapper.Initialize(new[] { genericInterfaceType });
            Assert.Null(mapper.GetMappedTypeFor(genericInterfaceType));
        }

        [Test]
        public void Interfaces_generic_with_methods_should_not_be_mapped()
        {
            var genericInterfaceType = typeof(InterfaceGenericWithMethods<>);
            mapper.Initialize(new[] { genericInterfaceType });
            Assert.Null(mapper.GetMappedTypeFor(genericInterfaceType));
        }

        [Test]
        public void Interfaces_with_only_properties_should_be_mapped()
        {
            var simpleInterfaceType = typeof(InterfaceWithProperties);
            mapper.Initialize(new[] { simpleInterfaceType });
            Assert.NotNull(mapper.GetMappedTypeFor(simpleInterfaceType));
        }

    }

    public abstract class Data
    {

    }

    public interface InterfaceGenericWithProperties<T> : IMessage where T : Data
    {
        string SomeProperty { get; set; }
        T MessageProperty { get; set; }
    }

    public interface InterfaceGenericWithMethods<in T> : IMessage where T : Data
    {
        string SomeProperty { get; set; }
        void MethodOnInterface(T myMessage);
    }

    public abstract class GenericAbstractCommand<T> : IMessage where T : Data
    {
        public T DataToTransfer { get; set; }
    }

    public class GenericCommand<T> : IMessage where T : Data
    {
        public T DataToTransfer { get; set; }
    }

    public class DerivedGenericCommand : GenericAbstractCommand<Data>
    {

    }

    public interface InterfaceSimplWithProperties : IMessage
    {
        string SomeProperty { get; set; }
    }

    public abstract class SimpleAbstractClass : IMessage
    {
        string SomeProperty { get; set; }
    }

    public abstract class SimpleAbstractClassWithMethods : IMessage
    {
        string SomeProperty { get; set; }
        protected abstract void DoTest();
    }
}

@johnsimons
Copy link
Member

Can you please send us a PR instead?

On Saturday, November 9, 2013, Marinko wrote:

Hi John

I think the fix is simple. All you need to change is to add the similar
condition like in the ConfigUnicastBus.IsMessageHandler method. Here the
corrected version of IsMessageType

///

    /// Returns true if the given type is a message type.
    /// </summary>


    public static bool IsMessageType(Type t)
    {
        try
        {
            if (t.IsGenericTypeDefinition)
            {
                return false;
            }
            return MessagesConventionCache.ApplyConvention(t,
                                                           type => IsMessageTypeAction(type) ||
                                                                   IsCommandTypeAction(type) ||
                                                                   IsEventTypeAction(type) ||
                                                                   IsInSystemConventionList(type));
        }
        catch (Exception ex)
        {
            throw new MessageConventionException("Failed to evaluate Message convention. See inner exception for details.", ex);
        }
    }

The other options is to fix it directory in the MessageMapper.cs

old new ... ... @@ -53,7 +53,7 @@ public void InitType(Type t, ModuleBuilder moduleBuilder)53 53 return;54 54 }55 55 56 - if (t.IsSimpleType())
56 + if (t.IsSimpleType() || t.IsGenericTypeDefinition)57 57 {58 58 return;59 59 }... ... @@ -339,7 +339,7 @@ public Type GetMappedTypeFor(Type t)339 339 {340 340 Type result;341 341 concreteToInterfaceTypeMapping.TryGetValue(t, out result);342 - if (result != null)
342 + if (result != null || t.IsGenericTypeDefinition)343 343 {344 344 return result;345 345 }

Here some tests to reproduce the issue:

namespace NServiceBus.MessageInterfaces.Tests{
using NUnit.Framework;
[TestFixture]
public class When_mapping_usinggenerics
{
IMessageMapper mapper;
[SetUp]
public void SetUp()
{
mapper = new MessageMapper.Reflection.MessageMapper();
}
[Test]
public void Class_abstract_generic_with_only_properties_generic_should_not_be_mapped()
{
var genericClassType = typeof(GenericAbstractCommand<>);
mapper.Initialize(new[] { genericClassType });
Assert.Null(mapper.GetMappedTypeFor(genericClassType));
}
[Test]
public void Class_generic_with_only_properties_generic_should_not_be_mapped()
{
var abstractClassType = typeof(GenericCommand<>);
mapper.Initialize(new[] { abstractClassType });
Assert.Null(mapper.GetMappedTypeFor(abstractClassType));
}
[Test]
public void Class_derived_from_generic_with_only_properties_generic_should_be_mapped()
{
var abstractClassType = typeof(DerivedGenericCommand);
mapper.Initialize(new[] { abstractClassType });
Assert.NotNull(mapper.GetMappedTypeFor(abstractClassType));
}
[Test]
public void Class_concrete_generic_with_only_properties_generic_should_be_mapped()
{
var abstractClassType = typeof(GenericCommand);
mapper.Initialize(new[] { abstractClassType });
Assert.NotNull(mapper.GetMappedTypeFor(abstractClassType));
}
[Test]
public void Class_abstract_with_only_properties_should_be_mapped()
{
var abstractClassType = typeof(SimpleAbstractClass);
mapper.Initialize(new[] { abstractClassType });
Assert.NotNull(mapper.GetMappedTypeFor(abstractClassType));
}
[Test]
public void Class_abstract_with_methods_should_not_be_mapped()
{
var abstractClassType = typeof(SimpleAbstractClassWithMethods);
mapper.Initialize(new[] { abstractClassType });
Assert.Null(mapper.GetMappedTypeFor(abstractClassType));
}
[Test]
public void Interfaces_generic_with_only_properties_should_not_be_mapped()
{
var genericInterfaceType = typeof(InterfaceGenericWithProperties<>);
mapper.Initialize(new[] { genericInterfaceType });
Assert.Null(mapper.GetMappedTypeFor(genericInterfaceType));
}
[Test]
public void Interfaces_generic_with_methods_should_not_be_mapped()
{
var genericInterfaceType = typeof(InterfaceGenericWithMethods<>);
mapper.Initialize(new[] { genericInterfaceType });
Assert.Null(mapper.GetMappedTypeFor(genericInterfaceType));
}
[Test]
public void Interfaces_with_only_properties_should_be_mapped()
{
var simpleInterfaceType = typeof(InterfaceWithProperties);
mapper.Initialize(new[] { simpleInterfaceType });
Assert.NotNull(mapper.GetMappedTypeFor(simpleInterfaceType));
}

}

public abstract class Data
{

}

public interface InterfaceGenericWithProperties<T> : IMessage where T : Data
{
    string SomeProperty { get; set; }
    T MessageProperty { get; set; }
}

public interface InterfaceGenericWithMethods<in T> : IMessage where T : Data
{
    string SomeProperty { get; set; }
    void MethodOnInterface(T myMessage);
}

public abstract class GenericAbstractCommand<T> : IMessage where T : Data
{
    public T DataToTransfer { get; set; }
}

public class GenericCommand<T> : IMessage where T : Data
{
    public T DataToTransfer { get; set; }
}

public class DerivedGenericCommand : GenericAbstractCommand<Data>
{

}

public interface InterfaceSimplWithProperties : IMessage
{
    string SomeProperty { get; set; }
}

public abstract class SimpleAbstractClass : IMessage
{
    string SomeProperty { get; set; }
}

public abstract class SimpleAbstractClassWithMethods : IMessage
{
    string SomeProperty { get; set; }
    protected abstract void DoTest();
}}


Reply to this email directly or view it on GitHubhttps://github.com//issues/1737#issuecomment-28064379
.

Regards
John Simons
NServiceBus

marinkobabic added a commit to marinkobabic/NServiceBus that referenced this issue Nov 11, 2013
GenericTypeDefintions can't be mapped except we use constraints
SimonCropp added a commit that referenced this issue Nov 17, 2013
issue #1737 NullReferenceException MessageMapper
@SimonCropp SimonCropp added the Bug label Feb 11, 2014
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

No branches or pull requests

3 participants