Improve fluent API with builder pattern when registering Azure Service Bus message pump
Closed this issue · 15 comments
Is your feature request related to a problem? Please describe.
Improve fluent API with builder pattern so that we control what the following methods are that can be called.
services.AddServiceBusQueueMessagePump(secretName, options => options.JobId = jobId)
.WithServiceBusMessageHandler<OrdersAzureServiceBusMessageHandler, Order>();
services.WithAutoRestartServiceBusQueueMessagePumpOnRotatedCredentials(...);This is to make it more clear and less verbose since every extension we have today is returning IServiceCollection.
This also helps new people to explore what the other options are for configuring the runtime.
Describe the solution you'd like
Return a IServiceBusMessagePumpBuilder which enforces you to call WithServiceBusMessageHandler, WithAutoRestartOnRotatedCredentials and/or ... after AddServiceBusQueueMessagePump
services.AddServiceBusQueueMessagePump(secretName, options => options.JobId = jobId)
+++ .WithAutoRestartOnRotatedCredentials(...)
.WithServiceBusMessageHandler<OrdersAzureServiceBusMessageHandler, Order>();
--- services.WithServiceBusMessageHandler<OrdersAzureServiceBusMessageHandler, Order>(); <-- No longer possible on services directly
--- services.WithAutoRestartServiceBusQueueMessagePumpOnRotatedCredentials(...); <-- No longer possible on services directlyWhat I would not do, however, is have a Build method at the end.
Describe alternatives you've considered
Nothing specifically, but open to suggestions.
Additional context
Add any other context or screenshots about the feature request here.
Thoughts @fgheysels ?
Yes, was something I thought about too, but didn't go through with it because it's actually some kind of breaking change, but since we're almost on the next major version we can indeed thing about this.
It's an approach that's also more in line with how the other .NET services are registered.
What I would not do, however, is have a Build method at the end.
What exactly do you mean ? You'll need to have something that actually finishes the setup of your builder, and leads to building the messagepump ? Or do you mean that this should be hidden ?
What I would not do, however, is have a Build method at the end.
What exactly do you mean ? You'll need to have something that actually finishes the setup of your builder, and leads to building the messagepump ? Or do you mean that this should be hidden ?
The actual build up will be when internally the .BuildServiceProvider will be called. This fluent API is about registration, not creation.
@fgheysels Ideally I would just have to do this:
services.AddServiceBusQueueMessagePump(secretName, options => options.JobId = jobId)
.WithAutoRestartOnRotatedCredentials(...)
.WithServiceBusMessageHandler<OrdersAzureServiceBusMessageHandler, Order>();
Which doesn't require a Build at the end, but if it's required then that's fine - Although a better name should be used then.
However, it could lead to cases where people didn't call it, nothing is happening and they are confused. Docs could help with that, but that wouldn't be enough.
If we do this (separate services), singletons defined could be created more than once, though. Just a remark.
Isn't that something we can/should manage?
Isn't that something we can/should manage?
'Can' in 'We can circumvent that?'
'Should' as in 'We should circumvent that?'
I think we should be able to manage that, no?
I think we should be able to manage that, no?
The problem I'm referring is that with separate service collections, they don't know from each other if a singleton has already been created. But that's only the case if the service descriptors don't hold the state (as in "is instance already created?") but the service provider does. So, it could be that the problem I think there is, doesn't exists. 😄 If that makes sense.
Something we can write a test for, of course, when we take up this issue.
I think we should be able to manage that, no?
The problem I'm referring is that with separate service collections, they don't know from each other if a singleton has already been created. But that's only the case if the service descriptors don't hold the state (as in "is instance already created?") but the service provider does. So, it could be that the problem I think there is, doesn't exists. 😄 If that makes sense.
Something we can write a test for, of course, when we take up this issue.
Just tested this, it seems like the state is indeed being held by the IServiceProvider and not the ServiceDescriptor which means that singletons could be created more than once (1 / service collection).
Or.... we don't work with seperate service collections at all and just make it more fluently.
I'll leave it up to your judgement to be honest. But what do you mean with more fluently? Can you add a sample?
I'll leave it up to your judgement to be honest. But what do you mean with more fluently? Can you add a sample?
Ok, I'll wait till #170 is finished, I'll safe us some merge conflicts. Oh, and with 'more fluently' I only mean this issue. That we don't look for separate service collections (for each message pump) but that we only update the message handling extensions with better discovery (like proposed here).
If we don't use separate service collections, we can do this, though:
services.AddServiceBusQueueMessagePump(secretName)
.WithServiceBusMessageHandler<OrdersAzureServiceBusMessageHandler, Order>(); // <-- available for message pump below
services.AddServiceBusTopicMessagePump(secretName);But maybe that's ok. It's possible today. Maybe we should just look first for making this more better discoverable (as the initial request was in this issue).
That was indeed what I was looking for, users should just call the AddServiceBusQueueMessagePump every time they want to add a new pump. So we are good to go then?
That was indeed what I was looking for, users should just call the AddServiceBusQueueMessagePump every time they want to add a new pump. So we are good to go then?
Yes, all good. I'll first finish with this one #141 as it will also help with Azure Functions later on.