Support customization of attachment names in blob storage

[CasperWSchmidt]

Is your feature request related to a problem? Please describe.
We have a a lot of events going through Azure Service Bus. Trying to find a specific one is quite hard.

Describe the solution you'd like
We would like to have some structure in the naming, fx. by having a Func<Message, string> in the configuration to decide the blob name. In our case something like date/topicName/messageId would be the optimal name.

I'm happy to create a PR with this if needed.

[SeanFeldman]

It sounds like a legitimate ask. If you'd like to give it a try, absolutely.
There are a few things to consider.

1. Container and the rest of the blob path need to be controlled separately.
2. Can this be implemented as an extension rather than a configuration class change, given it's working for most users so it would need to be an extension

Thinking quickly here, I'd rather not modify the configuration (AzureStorageAttachmentConfiguration) and instead create an extension that achieves what you need. Similar to AzureStorageAttachmentConfigurationExtensions.

var config = AzureStorageAttachmentConfiguration(...)
config.CustomAttachmentName(Func<Message, string>)

[CasperWSchmidt]

@SeanFeldman I get that you would prefer to not make breaking changes, but I don't see how the configuration can store the new Func without adding a property to the configuration (AzureStorageAttachmentConfiguration) since C# does not have extension properties (yet). If the configuration needs a new property, I don't see why this can't be added to the constructor as an optional parameter (after Func<Message, bool>? messageMaxSizeReachedCriteria = default). This way it will be non-breaking and existing users of the plugin won't notice the change. Also, AzureStorageAttachment.cs Line 59 would default to the existing behaviour if the new Func is null or returns an empty string.

[SeanFeldman]

You're correct about adding another overload to the constructor. I'm just not sure how frequent this feature is to make it part of an overload.

Let's see how it plays out on the PR. Perhaps I'm overthinking it 🙂

[SeanFeldman]

@CasperWSchmidt thank you for the PR.
Please have a look at what I've done based on your PR: #180.

[CasperWSchmidt]

@SeanFeldman I don't see why the extension version is so much better than mine - the configuration is still changed (internal property added). With an optional parameter the api is not changed in a way that affects people updating. The only issue could be unit tests using mocks where even optional parameters must be specified (that's how Moq works).

That said, it is your project and your solution adds the feature I need so please merge it 😊

[SeanFeldman]

@CasperWSchmidt by no means was my intent to even hint that it's "better". As a matter of fact, the alternative is entirely based on your suggestion and initial PR. The reason I'd rather go with an extension method is to isolate the intent and make it very explicit rather than add to an already overwhelming list of optional parameters.

Regarding releasing - I was thinking to add Managed Identity support (#181), but that might be a bit more time-intensive than I have on my hands. If you have spare time, by all means :)

[CasperWSchmidt]

@SeanFeldman okay "better" might not be the best word, but I didn't quite get why you were so keen on the extension when the configuration had to be changed. I do now and you're right. Too many (optional) parameters is a code-smell :)