[Bug] PostAsync throws an invalidCast exception with DynamoDB outbox

[romtur]

Describe the bug

An InvalidCastException is thrown when calling PostAsync with a DynamoDB outbox. However, it works correctly when calling DepositPostAsync while providing an IAmADynamoDbTransactionProvider

To Reproduce

Setup a DynamoDB outbox:

brighterBuilder.UseExternalBus((configure) =>
      { 
          configure.ProducerRegistry = new KafkaProducerRegistryFactory(kafkaConfiguration, publications).Create();
          configure.Outbox = new DynamoDbOutbox(dynamoDBClient, new DynamoDbConfiguration());
          configure.TransactionProvider = typeof(DynamoDbUnitOfWork);
          configure.ConnectionProvider = typeof(DynamoDbUnitOfWork);
      });

Call PostAsync method

The failure occurs in CommandProcessor:

Link to original discussion: #3091

Exceptions (if any)

System.InvalidCastException: Unable to cast object of type 'Paramore.Brighter.ExternalBusServices2[Paramore.Brighter.Message,Amazon.DynamoDBv2.Model.TransactWriteItemsRequest]' to type 'Paramore.Brighter.ExternalBusServices2[Paramore.Brighter.Message,System.Transactions.CommittableTransaction]'

Further technical details

• Brighter version: 1.0.0-preview.3
• The OS: Windows
• Kafka

[romtur]

Just to let you know, this issue also affects the MySqlOutbox

[dhickie]

I've spent some time digging into this. When looking at the DepositPost(Async) method, there are two overloads - one that accepts an implementation of IAmABoxTransactionProvider<TTransaction> as an argument, and one that doesn't. Post(Async) meanwhile, only has a single method, which does not accept a transaction provider argument.

When calling either the DepositPost(Async) method without a transaction provider argument, or Post(Async), it assumes that TTransaction takes the type CommittableTransaction, and completely ignores any transaction types which have been registered in the IoC container through UseExternalBus. If the user has registered a transaction provider that uses a transaction other than ComittableTransaction, then the cast fails and an exception is thrown.

One fix would be to add an overload to Post(Async) to allow users to provide a transaction provider, but I'm wondering why we should require the user to pass in a transaction provider at all. In v9, the overload of DepositPost(Async) that didn't take a transaction provider argument would take the default provider from the IoC container, but that has changed now that OutboxProducerMediator is a generic class. I'll have a think about how we could just use the transaction provider (and therefore transaction type) from the IoC container by default.

[iancooper]

So, in order to try and simplify configuration, we tried to move away from some of the repetition in the V9 interface. There was also a lot of complexity in the builder. When we built the samples to be switchable between transports and providers, some of the complexity of the V9 interface became particularly painful.

To fix that we needed to make OutboxProducerMediator take some generic types. Arguably, TMessage is redundant, because it is always our Message type. I think I tried to drop it before, but hit a bumpy road. But TTransaction lets us understand what type of transaction you might be using with our Outbox.

CommittableTransaction is a hack where we don't have a transaction, because you use the default in-memory box. Now, in principle, you could use CommittableTransaction to try and ensure a transaction with the In-Memory Outbox, but right now we have not really built that out.

Post is our simple method for an asynchronous command or event. It really says "I am not using an Outbox". Of course, you are, because you use are in-memory one, but that is an implementation detail for us. So what Post was intending to do was always use the InMemory Outbox. In principle you can also use our DepositPost and ClearOutbox explicitly without a transaction provider, again the assumption being that you want to use in-memory.

Now, I suspect what we have not thought about is: what happens if you set up an Outbox, but then use Post. In that design, our option that Post should always force you to an In-Memory Outbox, and passing a null transaction provider to that.

The problem is that this line

var mediator = ((IAmAnOutboxProducerMediator<Message, TTransaction>)s_mediator);

is going to try to pick up the parameterized type and try to cast the mediator, but you are using the in-memory, not the persisted store version, and as the mediator is a singleton, its going to fail.

In that case, I guess the best option would be to force your Post onto the registered Outbox, which would mean add a version of Post that takes an IAmABoxTransactionProvider<TTransaction>? transactionProvider so you could be explicit about that. In principle, you could provide a cast null, so that you don't have to use one, but we still know that you are aligned with the registered outbox.

In that case, I suspect we need to ensure that if you call Post without a transactionProvider, we ensure that you don't have a different Outbox type configured, and throw a ConfigurationException if you do, so as to prevent you from trying to operate both alongside each other.

[dhickie]

Thanks Ian, that clarifies the difference between Post and DepositPost for me a lot. I think it's reasonable for users to expect to be able to use Post when they have an outbox registered and just want a simple method for publishing something, but with the benefits that a persistent outbox provides.

Given that Post doesn't make any meaningful use of transactions, for me it would feel needlessly complex to expect the user to provide a (possibly null) transaction provider, even if they don't make any use of transactions elsewhere, simply because they have a persistent outbox registered - if you're not familiar with the internals of Brighter it's not at all clear what it's doing or why it's needed.

Instead, I feel the user journey for Post and DepositPost should be:

1. If the user calls DepositPost without an implementation of IAmABoxTransactionProvider, it automatically grabs the provider that was registered in the call to UseExternalBus and passes it on to the mediator.
2. If the user calls DepositPost with an implementation of IAmABoxTransactionProvider, but the transaction type doesn't match the type that was configured in UseExternalBus, then throw an exception.
3. If the user calls Post, it passes a null transaction provider to the mediator (and hence doesn't make use of transaction) but automatically passes the correct type for the outbox that was registered in UseExternalBus.

I think this is achievable by:

• Updating CommandProcessor.InitExtServiceBus to also:
  • Store a reference to the transaction provider that was registered in UseExternalBus
  • Identify and store the transaction type for the mediator that's being initialised
• When we need to make calls to methods on the generic version of IAmAnOutboxProducerMediator, such as AddToOutbox, we use reflection to identify the method on the mediator and invoke it like we do in CallDepositPostAsync for bulk deposits, removing the need to perform a cast

[iancooper]

Hi @dhickie. That makes sense as an algorithm. And I agree we can use the CallDepositPostAsync trick to avoid the cast.

Look forward to the PR