[Issue 3112] Use the configured transaction provider when no override is supplied

[dhickie]

This PR fixes issue #3112, in which an InvalidCastException was thrown when calling Post or PostAsync on the CommandProcessor if a persistent outbox was registered.

It does this by switching to a reflection based approach when calling methods on the OutboxProducerMediator, removing the need for the problematic cast. The transaction type for the generic method call is taken from the registered transaction provider during construction, and defaults to CommittableTransaction if no transaction provider is registered.

In an effort to simplify usage of the CommandProcessor, it also makes a couple of other changes:

• If a call is made to the overload of DepositPost or DepositPostAsync that doesn't take a transaction provider, it uses the one that was registered along with the persistent outbox (if any).
• If a call is made to the overload of DepositPost or DepositPostAsync that does take a transaction provider, but the transaction type for the supplied transaction provider doesn't match the type that was registered with the persistent outbox (if any), an InvalidOperationException is thrown.

[codescene-delta-analysis]

Gates Failed
Prevent hotspot decline (1 hotspot with Code Duplication)
Enforce advisory code health rules (1 file with Code Duplication)

Gates Passed
2 Quality Gates Passed

See analysis details in CodeScene

Reason for failure

Prevent hotspot decline	Violations	Code Health Impact
CommandProcessor.cs	1 rule in this hotspot	8.28 → 7.33	Suppress

Enforce advisory code health rules	Violations	Code Health Impact
CommandProcessor.cs	1 advisory rule	8.28 → 7.33	Suppress

Quality Gate Profile: Clean Code Collective
Want more control? Customize Code Health rules or catch issues early with our IDE extension and CLI tool.

[iancooper]

I would like to see if we can move away from a spy here, and use our InMemoryOutbox and possibly add an InMemoryTransaction and InMemoryTransactionProvider

[iancooper]

Could we use the InMemoryOutbox? We are trying to avoid Test Doubles where the in-memory version could substitute. If we need to query the state of the outbox, is that something that is all outboxed, or should the in-memory outbox be exposed?

[dhickie]

The issue with using the InMemoryOutbox is that it's the default implementation, and the scenario being tested here is when the user uses something other than the default, since that was what led to this issue being found in the first place (I also wouldn't have found that we should be clearing the bound MethodInfo's when we clear the outbox mediator from the CommandProcessor).

To do that without a test-only Spy implementation we would need to either have an additional type of in memory outbox in the core package (which feels redundant?), or pull in one of the other outbox implementations (and then require all the associated infrastructure in order to be able to run the core tests). This feels like the cleanest solution to me.

[iancooper]

Ah yes, that makes sense in this context. Closed.

[iancooper]

Similar to in-memory, does this imply that perhaps we need to swap from CommittableTransacton to our own InMemoryTransaction to use?

[iancooper]

Again, does this imply an InMemoryTransactionProvider?

[lillo42]

Since you are changing this, what about using Delegate rather than MethodInfo.Invoke?

private static readonly ConcurrentDictionary<string, Func<....>> s_boundDepositCallsAsync = new();

....

var del =method.CreateDelegate<Func<...>>();
s_boundDepositCallsAsync .Add(key, del);

...

var del = s_boundDepositCallsAsync[key];
del(.....);

[dhickie]

There are a few reasons not to use Delegates here. If you look at how the code would change, you'd be going from this:

deposit = depositMethod?.MakeGenericMethod(actualRequestType, transactionType)!;
return (deposit?.Invoke(this, [actualRequest, amABoxTransactionProvider, requestContext, dictionary, batchId]) as string)!;

To this:

deposit = depositMethod?.MakeGenericMethod(actualRequestType, transactionType)!;
var delegate = deposit.CreateDelegate(
    typeof(Func<IRequest, IAmABoxTransactionProvider, RequestContext, Dictionary<string, object>, string, Type, string>),
    this);
return (delegate.DynamicInvoke(actualRequest, amABoxTransactionProvider, requestContext, dictionary, batchId) as string)!;

By storing the methods as MethodInfo's, we can also store multiple methods with different signatures in the same collection, as we do with the the bound mediator methods in s_boundMediatorMethods. If they were delegates, each mediator method would need a separate member variable. So overall, using delegates makes the code more verbose without any real benefit in terms of clarity.

Lastly, using Delegate.DynamicInvoke is actually much slower than MethodInfo.Invoke, and given we're at the heart of things here we want it to be as performant as possible.

[dhickie]

I should also say that MethodInfo.CreateDelegate<T> doesn't exist in netstandard2.0, which we still support at the moment. Hence the need for Delegate.DynamicInvoke.

[codescene-delta-analysis]

Gates Passed
4 Quality Gates Passed

See analysis details in CodeScene

Quality Gate Profile: Clean Code Collective
Want more control? Customize Code Health rules or catch issues early with our IDE extension and CLI tool.

[codescene-delta-analysis]

Gates Passed
4 Quality Gates Passed

See analysis details in CodeScene

Quality Gate Profile: Clean Code Collective
Want more control? Customize Code Health rules or catch issues early with our IDE extension and CLI tool.

[iancooper]

I think we could merge this in if you want @dhickie?