rebus-org / Rebus.AzureServiceBus

:bus: Azure Service Bus transport for Rebus
https://mookid.dk/category/rebus
Other
33 stars 20 forks source link

Add '/' back as a valid topicname character in v6 #28

Closed jr01 closed 5 years ago

jr01 commented 5 years ago

Hi!

Can we have the '/' back as a valid character in v6 ToValidAzureServiceBusTopicName and in a v6.0.7? v6.0.4 removed the '/' as valid, but it has been valid for a long time and also is valid in v7-a07.

The reason for asking is that we have many topics with '/' and have several microservices and want to migrate them gradually from v3 to v6 (.net core/ Microsoft.AzureServicebus).

v6.0.3 seems not usable for us, because of the bugfix in v6.0.5.

I can spend the time to make a PR. Please let me know if you'd like that!

Thanks in advance!

mookid8000 commented 5 years ago

Why not upgrade to v7?

jr01 commented 5 years ago

TBH I got scared by the changelog yesterday:

BREAKING CHANGE .... If you update to 7, you must update ALL of your endpoints, otherwise pub/sub will not work!

But now I look deeper into the code I notice the UseLegacyNaming setting.

I'll give it a try today and report back.

jr01 commented 5 years ago

Hi again!

I did some tests and have 2 issues with 7.0.0-a07 with UseLegacyNaming:

  1. a '.' is replaced with '_' in queue name while v3, v6.0.3, and v6.0.6 don't replace it.
  2. the subscription name includes everything before the '/' while in v3 and v6.0.3 that was all stripped.
    string GetSubscriptionName()
        {
            var idx = Address.LastIndexOf("/", StringComparison.Ordinal) + 1;
            return Address.Substring(idx).ToValidAzureServiceBusEntityName();
        }

Maybe it's an idea to have a plug-gable naming strategy in v7, so we can implement it ourselves? That should also allow some additional use cases like grouping queues/topics (prefixing all queue's and topics with value, e.g. in my testing I just used v6.0.3/somequeue and v7/somequeue). I could spend the time on it?

jr01 commented 5 years ago

@mookid8000 to elaborate on my idea, here is some initial coding to a pluggable naming strategy. (at the moment I haven't tested it at all...) https://github.com/jr01/Rebus.AzureServiceBus/commit/5d219b01174ec35fb85bf71cd0e89e6ffbf946dd

Before I continue with it I'd like the feedback on the general idea.

mookid8000 commented 5 years ago

Actually I was thinking that v7 without legacy naming would be what you were after... or did I not understand what you were trying to do?

jr01 commented 5 years ago

Let me share some testcases,

With Rebus v3 (our current version) we have names like:

This can be reproduced withthe following v3 test code:

[TestFixture, Category(TestCategory.Azure)]
public class TestAsbTopicsPubSub : FixtureBase
{
    [Test]
    public async Task PubSubSeemsToWork()
    {
        var activator = Using(new BuiltinHandlerActivator());
        Configure.With(activator)
            .Transport(t => t.UseAzureServiceBus(StandardAzureServiceBusTransportFactory.ConnectionString, "group/some.inputqueue"))
            .Start();

        var gotString1 = new ManualResetEvent(false);

        activator.Handle<object>(async str => gotString1.Set());

        await activator.Bus.Advanced.Topics.Subscribe("group/some.interesting topic");

        await Task.Delay(500);

        await activator.Bus.Advanced.Topics.Publish("group/some.interesting topic", new object{ });

        gotString1.WaitOrDie(TimeSpan.FromSeconds(2));
    }
}

Notice the '/', '.' and ' ' in the topic name. This is on purpose.

With Rebus v6.0.3 the test has the exact same result. So all is good in 6.0.3 (except for the missing 'context.OnAborted(async () => renewalTask.Dispose());' issue ).

In Rebus v6.0.6 the result is:

With BAD I mean different and not compatible with the current names we have in the ASB.

In Rebus v7 and Rebus v7+UseLegacyNaming the above test code actually throws an exception:

Message: System.ArgumentException : Could not create topic 'group/some.interesting topic' ----> Microsoft.Azure.ServiceBus.ServiceBusException

because the topic isn't normalized in the code at all...

Instead if I normalize/remove the space in the call, like so, .Subscribe("group/some.interesting_topic")

Then in v7 without UseLegacyNaming()

But in v7 with .UseLegacyNaming()

I think my idea could be a nice solution where different naming strategies ( Legacy_v603, Legacy_v604, Default (v7) ) could be delivered out of the box, and at the same time it allows to have completely customized naming.

mookid8000 commented 5 years ago

fixed by #29 (out in Rebus.AzureServiceBus 7.0.0-a09)