BusBuilder prototype first-step

[martincostello]

Add new BusBuilder class as a prototype of a replacement for CreateMeABus.

[slang25]

Would the entry point here be static? Or is the idea that the builder is resolved from DI with some dependencies injected?

[slang25]

I assume this would also look to solve the lack of async and deferred execution of the current builder, in which case we no longer require a logger as the first thing to be configured.

[martincostello]

My idea would be that there's one somewhere and you get it however you want (your own static, DI, whatever) and then chain from there.

Basically I want to get rid of all the static stuff as it's the source of 90% of the pain in the integration tests with the shared static for mocking out the factory to do things.

This is just the barest skeleton of the sort of shape things could take with an arbitrary name I've picked, then if we like where it's going I can start actually trying to plumb it in and cast off the statics as much as possible (but after #426 gets merged).

[martincostello]

Indeed, with the builder pattern we could make things very lazy/configurable over time, and then it would be up to the integrator to either use it once and use that forever, or use the builder as part of DI registration and get things as they are needed.

[slang25]

Cool, I agree on all points. It'd be nasty if there were some dependencies that were baked into a static factory method for example.

I was thinking that potentially we'd just be building up a data structure with the fluent api, and the starting point would be blank, so it's kind of just a style thing whether it's static or not.

If we are going to allow cross cutting things (let's say ILogger, perfect example) to be injected via DI, then I could see it being beneficial to go down the instance method entrypoint route.

[martincostello]

Any thoughts on this from anyone else (@AnthonySteele, @brainmurphy) before I start thinking about making this PR actually do something working?

[martincostello]

FYI I've started working on a working prototype for this. I'm doing it as an alternate implementation for now, rather than replacing anything already in the library.

[martincostello]

So I've got a basic skeleton of a API surface we could look towards doing. An example of it is below, which is just a simple unit test.

The general gist is:

1. You create a builder;
2. You configure everything you need to on it (config, handlers, publisher, subscribers, regions etc.);
3. You call Build() when you're done configuring;
4. You get back an IMessagingBus;
5. The host calls Start(...) on the bus.

The MessagingBusBuilder acts as the root, and you could pass it around to split up configuration/registration into chunks.

Then once everything is done, you just get the simple IMessagingBus instance which only lets you start and stop processing messages, which the host application would use at the appropriate point (e.g. the end of void Configure(IApplicationBuilder app) in a ASP.NET Core application).

One (of the many things) missing so far is a need to hook DI in to the builder for use during Build(), though this is partially helped by the Func<T> overloads for various things on the builder.

[Fact]
public static void Can_Create_Messaging_Bus_Fluently()
{
    // Arrange
    var builder = new MessagingBusBuilder()
        .Client()
            .WithBasicCredentials("accessKey", "secretKey")
        .And().Parent
        .Messaging()
            .WithRegions("eu-west-1", "eu-central-1")
            .And()
            .WithActiveRegion("eu-west-1")
        .And().Parent
        .WithLoggerFactory(new LoggerFactory());

    // Assert
    IMessagingBus bus = builder.Build();

    bus.Start(new CancellationToken(canceled: true));
}

[slang25]

I love the DI extension method and how tidy this looks 👏

It's a taste thing, but I'm not sure about the .And().Parent(), wouldn't having blocks like .Messaging() be just as discoverable if they took a delegate that had its own nested builder?
i.e.

.Messaging(x => x
            .WithRegions("eu-west-1", "eu-central-1")
            .WithActiveRegion("eu-west-1"))

Another thing to add to the wish list, the existing implementation allows you to to build a publisher and subscriber all on one go, which struck me a wierd. So Build() would give you something that could publish, and start listening, regardless of what I can actually do. What are your (and others) thoughts on this?

Finally, (as mentioned in another issue) we need a BuildAsync instead of Build, which means making all of the side-effecting stuff in the builder lazy, and queue it up for later execution. We might be able to tackle it in a separate PR, but it would be good to keep it in mind with this PR.

[adammorr]

This looks good and a lot nicer than the current API...adios CreateMeABus

One thought I had (which sort of leans towards what Stu had mentioned) was to configure the bus with common configuration and then branch off of that into the more specific publisher / subscriber logic....maybe something like this;

var configuredBus = new MessagingBus()
    .Messaging(cfg => {
        // Regions / Serialisation / Naming conventions
    })
    .Monitoring(cfg => {
        // Message monitor / logging / ...?
    })

IMessagePublisher publisher = await configuredBus.ConfigurePublishers()
    .AddTopicPublisher<T>()
    .AddQueuePublisher<T>()
    .BuildAsync();

IMessageSubscriber subscriber = await configuredBus.ConfigureSubscribers()
    .AddTopicSubscriber<TMessage, THandler>(cfg => {})
    .AddQueueSubscriber<TMessage, THandler>(cfg => {})
    .BuildAsync();

Just another idea but feel free to 🔥 it!

[martincostello]

I like both of those suggestions. There's already a lot of plumbing going on for this, so I don't think the possible overhead of a few extra types for the nested builders is a bad shout.

The And().Parent was just a first stab at trying to "get back" from a more specific API configuration back to the root while preserving a single builder chain.

It probably means I can ditch the need for the base class for the builders, which seemed like a good idea at the start, but as nothing's really being subclassed within a particular builder type (and I've sealed'd them), it's just extra fluff now. I did a similar pattern for Selenium IWebDriver, where there actually was a lot of sub-classing (Chrome, Firefox, IE etc.), so that pattern actually solved a lot of duplication.

I'll mull on this and look at doing some more hacking around tomorrow to work towards the suggested nested builders.

[martincostello]

I've made a first pass at the suggested approach with the nesting, and also made some tweaks here and there and fixed a few bugs along the way (I'll cherry-pick those into separate PRs later today) so that the two tests both pass again (locally at least) by pointing at goaws.

I'll come back to looking at separating out subscribing and messaging later on.

The updated sample from the test is now (I've left And() in for now):

[Fact]
public void Can_Create_Messaging_Bus_Fluently()
{
    // Arrange
    var services = new ServiceCollection()
        .AddLogging((p) => p.AddXUnit(OutputHelper))
        .AddJustSaying(
            (builder) =>
            {
                builder.Client(
                            (options) => options.WithBasicCredentials("accessKey", "secretKey"))
                       .Messaging(
                            (options) => options.WithRegions("eu-west-1", "eu-central-1")
                                                .And()
                                                .WithActiveRegion("eu-west-1"))
                       .Subscriptions(
                            (options) => options.WithHandler<MyMessage>());
            })
        .AddJustSayingHandler<MyMessage, MyHandler>();

    IServiceProvider serviceProvider = services.BuildServiceProvider();

    // Assert
    var bus = serviceProvider.GetRequiredService<IMessagingBus>();
    bus.Start(new CancellationToken(canceled: true));
}

[slang25]

It's looking really nice 👍 I'll have a deeper dive later today and feedback

[slang25]

Just for reference, @adammorr did a prototype a while ago of the fluent api (see here)

Something that I quite liked in that prototype was making it clear if you were dealing with topics or just queues in subscriptions and publishing, with methods like AddTopicPublisher and AddTopicSubscriber, vs AddQueuePublisher and AddQueueSubscriber.

One of the things that I find slightly annoying with the way JustSaying currently works is that you might expect those 4 apis to be relatively symmetrical, but in the common case of AddTopicSubscriber it does more for you, it checks if the topic exists, creates it if not, and same for the subscription to the queue. I'm not suggesting that we change that here, but again it's something to keep in mind while giving the apis an overhaul.

While you're in the guts of this, what's your thoughts?

[martincostello]

I haven't gotten through all of the internals yet, but from what you're describing it might be worth "forking" things in the configuration to differentiate between the two models so that only relevant options are presented for what's being configured at the time.

[martincostello]

Having a quick look at @adammorr's PR, it does look like there's a lot of common ideas going on between that and this.

I think the only thing I disagree with from there is the use of interfaces. I think concrete types are much better for this scenario as it makes them easy to extend with minimal fuss and/or breaking chances, rather than the explosion of interfaces that exists in JustSayingFluentlyInterfaces.cs.

[martincostello]

Have cherry-picked some of the changes here into #455 and #456. Will rebase this once those are merged.

[martincostello]

Rebased.

[shaynevanasperen]

@martincostello This is nice work so far, so please don't take this the wrong way. I very much like @adammorr's suggestion of splitting out the publish and subscribe parts into two branches.

AddQueuePublisher doesn't make sense because you don't publish to queues (you publish to topics). Likewise, AddTopicSubscriber doesn't make sense because you don't subscribe to topics (you subscribe to queues). I'd like to see something that makes it more clear what is actually happening behind the scenes:

var configuredBus = new MessagingBus()
    .Messaging(cfg => {
        // Regions / Serialisation / Naming conventions
    })
    .Monitoring(cfg => {
        // Message monitor / logging / ...?
    })

IMessagePublisher publisher = await configuredBus.ConfigurePublishers()
    .EnsureTopicExists<T1>() // for when we want to publish to a topic which will allow fan-out to many queues
    .BuildAsync();

IMessageSender sender = await configuredBus.ConfigureSenders()
    .EnsureQueueExists<T2>() // for when we want point-to-point queues with no topic
    .BuildAsync();

IMessageSubscriber subscriber = await configuredBus.ConfigureSubscribers()
    .EnsureTopicExists<T1>(topic =>
        topic.EnsureQueueExists<THandler1>(cfg => {})) // here we are adding a queue for a specific topic (we could continue to chain more EnsureQueueExists calls here to fan-out to many queues)
    .EnsureQueueExists<T2, THandler2>(cfg => {}) // here we are adding a queue without a topic
    .BuildAsync();

EnsureTopicExists and EnsureQueueExists make it clear that it's creating a new topic or queue (only if they don't already exist)

Usage would be:

sender.SendAsync(...)
publisher.PublishAsync(...)
subscriber.StartListeningAsync(...)

[martincostello]

I'll look at splitting things up more into publish/subscribe/listen when more of this PoC is wired-up.

I still need to implement any publishing at all, and I only got subscriptions working last night. Publishers and listeners aren't yet wired up into the DI or available directly on the builder.

Once I've got the basics of JustSaying working end-to-end with a goaws integration that passes, I can start refactoring things to fit the suggestions more.

Writing this PR is also very much a learning process for me of what JustSaying actually does under the covers.

[slang25]

That's a fair point @shaynevanasperen, it was done like that because JustSaying didn't have separate semantics for message sends, we should take the time here to get it right.

[adammorr]

@shaynevanasperen just on your point about AddQueuePublisher doesn't make sense because you don't publish to queues you can publish directly to queues (point to point in current lingo)

[slang25]

I was thinking about the same thing @shaynevanasperen with regards to explicitly asking for topic/queue/subscription creation, it would need to be something that wouldn't be easy to miss for existing consumers relying on that behaviour.

@adammorr You are right, I think what Shayne means is that the existing JustSaying lingo is wrong, cause you send to queues, and publish to topics. So if we introduced a Send method, then we'd change the fluent api to match.

[AnthonySteele]

This Looks Good To Me, and I am very happy with the general approach. But I am not the expert on the fluent interface (it's the part of JustSaying that I am least familiar / concerned with) so someone else will have to say if the details fit with the various consumers.

[martincostello]

I'd say that if @adammorr seems OK with it, we can get this merged so we can start fleshing it out more and starting to switch over/remove the old code over the quiet period between now and January.

[adammorr]

I’ll take a look when I get back from holiday tomorrow 👍

[adammorr]

This looks good to me 👍 the only question I have (which may have already been considered or will come as part of fleshing this out once merged) is around .AddJustSayingHandler<TMessage, THandler> being outside of the configuration within Subscriptions(...).

As subscriber configuration and its handler are closely related, would we want to either;

• add safeguards to notify the user if they have configured a queue but not added a handler?
• should defining the handler be part of SubscriptionsBuilder<T>?
• something else?

[martincostello]

The AddJustSayingHandler<,> stuff is an MS DI extension more than part of the builder, so it's separate. Otherwise the builder (probably) will get more complicated as it'll need more abstract-DI-awareness baked into it.

Being able to do warnings would also mean needing introspecting the DI and/or maybe prematurely activating services, so while that might be a nice-to-have, it might get complicated.

[adammorr]

OK cool - it's probably not worth incurring any additional complexity for this given the existing fluent API doesn't give you any kind of warning either!

[martincostello]

Regarding the above, actually, maybe we could put in some lightweight stuff into the default IHandlerResolver implementation at a later point in the fleshing-out phase if it seems easy to do.

[martincostello]

Right I think we're at the point now this can be merged and we can start some Christmas-period-lull hax0r activities to start cutting over and expanding the new API.

Just needs a ✔️ now...

[slang25]

Great stuff @martincostello

[martincostello]

💥

[martincostello]

Later today I'll look at making some issues (i.e. tasks) that can be doled out amongst the interested parties to start the switch-over.

[slang25]

[martincostello]

First batch of issues linked above.