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

ConventionalRoutingTopology pub/sub allows the use of nested types without distinguishing them from un-nested types #134

Closed
2 tasks
bording opened this issue Mar 24, 2016 · 4 comments

Comments

@bording
Copy link
Member

bording commented Mar 24, 2016

The default ConventionalRoutingTopology has a problem where it can treat different classes in the same namespace as the same for pub/sub, meaning that subscribing to one of the classes will result in receiving messages for all of the related classes.

This can happen when there are nested classes involved. For example, given the following:

namespace Foo
{
    public class ClassA : IEvent
    {

    }

    public class ClassB
    {
        public class ClassA : IEvent
        {
        }
    }

    public class ClassC
    {
        public class ClassA : IEvent
        {

        }
    }
}

If an endpoint subscribes to any of the ClassA events, it will receive a message when any of the ClassAs are published.

The reason this happens is that the exchange name that represents the type is created by using just the namespace and type name

This means that the nested aspect of the class is lost, and all three will use the same exchange, Foo:ClassA.

Instead, it should use the FullName of the type, which would preserve the nested aspect and create three separate exchanges.

However, making this change would break backwards compat, so it seems like we can't do that at this point.

Given that this sort of class setup is likely to be rare, perhaps we just document this as something to avoid instead?

Plan of attack

  • Throw an exception when a message type being published is nested, with an opt-out config option
  • Update docs, including upgrade guide
@bording bording added the bug label Mar 24, 2016
@adamralph
Copy link
Member

Discussed this in the @Particular/rabbitmq-transport-maintainers sync today. This is something we can fix, but it would break wire compatibility.

One option put forward to is to create a completely new (opt-in) topology. In practice that would require a simultaneous switch to the new topology for all affected endpoints, plus a clear out of the exchanges from the old topology.

@adamralph adamralph added this to the 5.0.0 milestone May 23, 2017
@adamralph adamralph changed the title ConventionalRoutingTopology pub/sub can't distinguish between multiple nested classes in the same namespace ConventionalRoutingTopology pub/sub allows the use of nested types without distinguishing them from un-nested types May 23, 2017
@adamralph
Copy link
Member

@Particular/rabbitmq-transport-maintainers discussed this today. We will throw an exception when a message type is nested. The fix for an endpoint will be to un-nest the message type. This will not break wire compatibility.

@bording bording mentioned this issue Jul 24, 2017
@adamralph
Copy link
Member

Added a PoA.

@adamralph
Copy link
Member

adamralph commented Sep 1, 2017

Closing, since we will not be fixing this bug. Rather, we will remove the conventional routing topology at some point. It will be deprecated in 5.0.0.

@adamralph adamralph removed this from the 5.0.0 milestone Sep 1, 2017
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

No branches or pull requests

2 participants