Azure / azure-sdk-for-net

This repository is for active development of the Azure SDK for .NET. For consumers of the SDK we recommend visiting our public developer docs at https://learn.microsoft.com/dotnet/azure/ or our versioned developer docs at https://azure.github.io/azure-sdk-for-net.
MIT License
5.45k stars 4.8k forks source link

[FEATURE REQ] Introduce abstraction similar to IMessageReceiver implemented by ServiceBusMessageActions #28431

Closed 4nandP closed 2 years ago

4nandP commented 2 years ago

Library name

Azure.Messaging.ServiceBus Microsoft.Azure.WebJobs.Extensions.ServiceBus

Please describe the feature.

Microsoft.Azure.ServiceBus had an IMessageReceiver interface that allows abandoning, completing, etc but has no equivalent in the Azure.Messaging.ServiceBus.

ServiceBusMessageActions (and one could argue other classes such as ServiceBusReceiver, ProcessMessageEventArgs, ProcessSessionMessageEventArgs) should implement an interface to allow abstraction.

Task AbandonMessageAsync(ServiceBusReceivedMessage message, IDictionary<string, object> propertiesToModify = null, CancellationToken cancellationToken = default);
Task CompleteMessageAsync(ServiceBusReceivedMessage message, CancellationToken cancellationToken = default) { throw null; }
Task DeadLetterMessageAsync(ServiceBusReceivedMessage message, IDictionary<string, object> propertiesToModify = null, CancellationToken cancellationToken = default);
Task DeadLetterMessageAsync(ServiceBusReceivedMessage message, string deadLetterReason, string deadLetterErrorDescription = null, CancellationToken cancellationToken = default);
Task DeferMessageAsync(ServiceBusReceivedMessage message, IDictionary<string, object> propertiesToModify = null, CancellationToken cancellationToken = default);
Task RenewMessageLockAsync(ServiceBusReceivedMessage message, CancellationToken cancellationToken = default);

This would essentially allow us to replace references to IMessageReceiver in Microsoft.Azure.ServiceBus with something new without having to introduce dependencies (and coupling) to Microsoft.Azure.Webjobs.Extensions.ServiceBus.

This might also help in addressing the testing concerns raised in #25375

jsquire commented 2 years ago

Thank you for your feedback. Tagging and routing to the team member best able to assist.

jsquire commented 2 years ago

Hi @4nandP. Thank you for your feedback. With respect to the lack of interfaces, the Azure SDKs have a goal of strict backwards compatibility with a very high bar for any breaking changes. With that in mind, the use of an interface makes evolving over time difficult; any change to the public API surface represented in an interface becomes a breaking change for anyone who has implemented it - including when we want to add new properties/methods.

Adding more interfaces to define new members (much like native COM interfaces do in Windows) is possible but becomes a burden of additional complexity for developers using the library. While default interface implementation is intended to make this scenario easier, it is not supported in all of the frameworks that we target. Our open classes and their virtual members can still be mocked or extended and do not have this problem. As a result, we intentionally avoid using interfaces outside of special circumstances.

Support for mocking and unit testing is one of the core goals of the new Azure SDKs, and the details of the approach that we use are discussed in the Azure SDK Design Guidelines. More discussion and detail can be found in this post from the Azure SDK blog.

I'm going to mark this as addressed, but please feel free to unresolve if you'd like to continue the discussion.

ghost commented 2 years ago

Hi @4nandP. Thank you for opening this issue and giving us the opportunity to assist. We believe that this has been addressed. If you feel that further discussion is needed, please add a comment with the text “/unresolve” to remove the “issue-addressed” label and continue the conversation.

4nandP commented 2 years ago

How would you suggest writing a piece of functionality that will call for example Abandon that can be consumed by Microsoft.Azure.Webjobs.Extensions.ServiceBus but also be consumable by something using Azure.Messaging.ServiceBus directly (i.e. something agnostic of the hosting mechanism)? I fear the initial response is too focused on an aside about testing and mocking rather than the fact that I would have to otherwise introduce coupling with the webjobs packages.

This just pushes the concern to the consumer because we'd have to define the interface in our library that originally used IMessageReceiver and then implement it as a decorator around either ServiceBusReceiver or ServiceBusMessageActions as appropriate. That seems over the top vs implementing an interface within the framework for something that doesn't seem to have changed much over the years. I'm trying to avoid coupling with the hosting layer so that the same library can be used by code running in a function app, console, docker etc, i.e. host agnostic.

/unresolve

jsquire commented 2 years ago

How would you suggest writing a piece of functionality that will call for example Abandon that can be consumed by Microsoft.Azure.Webjobs.Extensions.ServiceBus but also be consumable by something using Azure.Messaging.ServiceBus directly (i.e. something agnostic of the hosting mechanism)?

We would recommend abstracting the operations that you're using into a dedicated class/interface and delegating those calls to the appropriate type for the host environment. You've already described the general approach, so I don't believe there's a benefit to me going deeper into details here. I'm happy to do so if you feel it would be helpful.

I fear the initial response is too focused on an aside about testing and mocking rather than the fact that I would have to otherwise introduce coupling with the webjobs packages.

It is true that my initial response was framed around mocking for test purposes, as this is a core scenario for the Azure SDK libraries. Let's take a deeper look at your decoupling scenario. I think that it's important to start by acknowledging that the role that the two packages play:

The main takeaway is that we do not expect that the two API surfaces will always align; the extensions package is intentionally exposing functionality tailored to the Functions environment. Let's take a look at areas of overlap and how we could potentially represent them in a neutral way without stepping outside of each package's role or introducing a dependency on the extensions package.

My initial thought is similar to yours; bundle related functionality together behind an interface - for example, IMessageReceiver, IMessageSettler, IMessageSender and so on. Each would contain a curated set of functionality and the full-featured types would implement multiple interfaces. That introduces some noise into types and adds to a developer's cognitive load, but it's a logical breakdown. However, as mentioned above, that is a non-starter due to the use of interfaces versus the need to evolve with a strict "no breaking changes" mandate.

The next consideration would be to see if we could do something similar using abstract classes. Due to the single inheritance limitation, that would be quite difficult to achieve. It may be possible to define a minimum subset for archtypes and express that as the abstract, but likely that would be a difficult thing to maintain the purity of and adhere to as each library evolves separately. That also comes with considerations for ensuring that introducing the new base types does not cause breaking changes, which is possible but would influence patterns.

Exposing the types from the Service Bus package directly as the Functions API would eliminate most of the above concerns but also doesn't quite fit universally, as the bindings use a processor to receive messages. The Function have access to some functionality by using either the ProcessMessageEventArgs or ProcessSessionMessageArgs at the cost of having to understand which type to use for what scenario and losing the ability to curate the Functions experience. As we would like to introduce new abilities into Functions, it would require that we also introduce them as part of the processor's API, which may be an awkward fit. At this point, we would have to support this alongside the current approach which increases the cognitive cost for understanding how to use the package in the most effective way.

The most likely way that we'd support the scenario would be to introduce a companion package that understands both Azure.Messaging.ServiceBus and Microsoft.Azure.WebJobs.Extensions.ServiceBus and provides the same wrapping/delegation abstraction over the public types that we started our discussion with. While that satisfies the technical concerns, it comes with its own set of business concerns. There's a non-trivial cost associated with building and maintaining an additional package that we would need to justify for us to receive approval for doing so. Building that case typically requires there being no obvious work-around or having received a large number of requests from developers.

Unfortunately, this scenario isn't one where we've received many requests and implementing support within an application is straightforward. At this time, I don't believe that we could reasonably make a business case for it as a feature in the SDK.

4nandP commented 2 years ago

Thanks for that detailed response @jsquire , seems like that for now I'll have to go with my interface / decorator approach then, like you said, it should be straightforward enough.

jsquire commented 2 years ago

I regret that we don't have a better answer for you at this point, @4nandP. It seems that we've reached a conclusion, so I'm going to close this out. I'm happy to discuss further, if there is more that you'd like to cover. Please feel free to unresolve, if so.

ghost commented 2 years ago

Hi @4nandP. Thank you for opening this issue and giving us the opportunity to assist. We believe that this has been addressed. If you feel that further discussion is needed, please add a comment with the text “/unresolve” to remove the “issue-addressed” label and continue the conversation.