DefaultRoutingKeyConvention seems to use GetInterfaces.FirstOrDefault incorrectly

[SimonCropp]

@andreasohlund i think there are some problems with this code

https://github.com/Particular/NServiceBus.RabbitMQ/blob/develop/src/NServiceBus.RabbitMQ/Routing/DefaultRoutingKeyConvention.cs#L21

        var interfaces = type.GetInterfaces()
            .Where(i => !IsSystemType(i) && !IsNServiceBusMarkerInterface(i)).ToList();

        var implementedInterface = interfaces.FirstOrDefault();

Note we are calling GetInterfaces then .FirstOrDefault(); on that result

The problem is what is returned from GetInterfaces is ordered based on the source

For example this

public class MyEvent:IMyInterface1,IMyInterface2
{
}

Will return IMyInterface1 as the first from GetInterfaces

While this

public class MyEvent:IMyInterface2,IMyInterface1
{
}

Will return IMyInterface2 as the first from GetInterfaces

So from the users perspective their functionality is the same but the routing key will be different.

[andreasohlund]

@fhalim do U remember?

[fhalim]

Yes, maybe the interfaces should be sorted to make this a bit more deterministic

The problem with this algorithm are slightly bigger. For instance, it'll generate the complete key from the lowest base type before moving on to the interfaces for the next lowest one. Maybe it'd make sense to collect all the interfaces for the type and use that as a sorted list.

All this said, collapsing the type hiearchy into a string and doing prefix matching seems inherently problematic. I don't really have a good solution for it other than maybe keeping routing information out of band like we have to do on MSMQ etc.

[andreasohlund]

Can't come up with a way to fix this. Will open a improvement issue
#56

and document this as a limitation for now

[SimonCropp]

so this is still a bug in deep nested inheritance changes

https://msdn.microsoft.com/en-us/library/system.type.getinterfaces(v=vs.110).aspx

The GetInterfaces method does not return interfaces in a particular order, such as alphabetical or declaration order. Your code must not depend on the order in which interfaces are returned, because that order varies.

[adamralph]

The @Particular/rabbitmq-transport-maintainers group discussed this at some length today.

DefaultRoutingKeyConvention, as described in the opening comment, is used in DirectRoutingTopology only.

The conclusion we came to is that DirectRoutingTopology is essentially broken wrt interface based routing. Nor can we think of any way that it could be fixed, since there is no practical way to express an arbitrary number of interfaces for routing via the default exchange.

The only path this leaves open is to remove support for interface based routing in DirectRoutingTopology. We also considered whether we should drop DirectRoutingTopology entirely, leaving ConventionalRoutingTopology (the current default) as the only built-in topology. However, either path would result in a wire-level breaking change (requiring simultaneous upgrade of all endpoints).

In the meantime we believe we should document the limitation and advise against the use of message types which have interfaces in their inheritance hierarchy when using the DirectRoutingTopology, instead recommending the use of ConventionalRoutingTopology when such routing is required. We also believe it would be useful to log a warning when an interface is being used for routing with DirectRoutingTopology.

In summary, we propose the following:

• Raise an issue to be actioned ASAP, for adding an advisory in docs not to use messages types which inherit from non-system interfaces with DirectRoutingTopology, advising the use of ConventionalRoutingTopology instead.
• Raise an issue to be released in the next minor, to log a warning in DirectRoutingTopology when an interface is being used for routing.
• Withdraw "Make the direct routing topology support multiple inheritance" as something we can't fix.
• Raise an issue to discuss breaking wire compatibility by either dropping support for interface based routing in DirectRoutingTopology, or dropping DirectRoutingTopology altogether.
• Close this issue

@SimonCropp we'd appreciate any input you have on this.

[adamralph]

Raised #268, #269, #270. Closed #56.

[SimonCropp]

A couple of things

• place am obsolete message on the offending api and deploy it in a patch
• when that patch is released do a post on the google group about it

[adamralph]

Thanks @SimonCropp, those are good points. The offending API is the direct routing topology itself and the deprecation and eventual removal of that depends on the outcome of #270, so I'll reference those points from there.