Brighter icon indicating copy to clipboard operation
Brighter copied to clipboard

Add initial implementation allowing multiple Db Contexts for TXNs

Open preardon opened this issue 2 years ago • 6 comments

@IlSocio Here is my implementation for #2104

This requires a small change to the ctor of CommandProvider / the DI stuff around it, instead of passing a IAmABoxTransactionConnectionProvider there is now a IAmABoxTransactionConnectionProviderRegistry

The BoxTransactionConnectionProvider that you pass in the ctor becomes the default and from there you can add more as follows

var boxTxnProviderRegistry =
                new BoxTransactionConnectionProviderRegistry(txnProvider1Name, txnProvider1).AddProvider(
                    txnProvider2Name, txnProvider2);

From here when you go to Deposit Post you can do the following to use the default

await _commandProcessor.DepositPostAsync(_myCommand);

or to be specific

await _commandProcessor.DepositPostAsync(_myCommand2, transactionProviderName: txnProvider2Name);

this is still a rough draft (I feel we should maybe default the default txn Provider name and there are no tests on the non async Path (however it should be identical)

@iancooper , @holytshirt , @DevJonny Please let me know your thoughts

preardon avatar May 08 '22 12:05 preardon

I don't think this is the right approach. It adds complexity for the user.

  • I don't see the need for multiple Db contexts. Brighter should work with your write side model, Darker with your read side model if you have different read and write Dbs, otherwise, why do you need multiple Db contexts?
  • Using a registry within the call to Deposit feels ugly - there is a lot of complexity being surfaced here. Our goal should be to keep the complexity to the configuration, such that handlers mean you don't really have to understand the complexity of what is happening here.
  • If we had to do this -- and I am not convinced it's not bad design in the app -- then I would suggest that you need a registry of outgoing request type to transaction provider and that we look up the transaction provider based on the type of request we are given and then ask it for the transaction.

Some others thoughts on the multiple Db contexts issue:

  • https://stackoverflow.com/questions/69413326/use-multiple-dbcontext-on-ef-core
  • https://stackoverflow.com/questions/50016509/entity-framework-core-multiple-dbcontext-single-database
  • https://entityframework.net/knowledge-base/16226360/why-multiple-dbcontext-classes- (I'd really like to understand what folks using this are trying to achieve)

iancooper avatar May 08 '22 15:05 iancooper

hello @iancooper The issue I opened was not specifically related to DbContext, but to the more generic lack of support for multiple connections.

I'll try to simplify the things with a further sample: Let's suppose I have 2 databases in my project: DB1 & DB2 In Controller1 I want to change an entity in DB1 and send a Message using the Outbox pattern. In Controller2 I want to change an entity in DB2 and send a Message using the Outbox pattern.

Currently, it seems that there is no way to support this use case.

I believe that the things became more complicated that it should be. Imho, allow this kind of flexibility is straightforward and we just need change the visibility of the overload below to public and let it be part of the interface: https://github.com/BrighterCommand/Brighter/blob/7ad1a911422cc71a8ad5a222759da92abc5c5ef6/src/Paramore.Brighter/CommandProcessor.cs#L450-L456 So, we can use this additional overload to explicitly provide the transaction, by wrapping it.

Considering that the interface is just empty, I don't see any issue to take it as dependency: https://github.com/BrighterCommand/Brighter/blob/76c3f84c72864e704a988ecf6251cb0bb42868b0/src/Paramore.Brighter/IAmABoxTransactionConnectionProvider.cs#L1-L7 In this way, we have the additional option to provide the transaction explicitly when we call brighter (like we usually can do for all the other components which are involved in the transaction) And who doesn't need to provide the transaction explicitly, can just use the current DepositPost() as today, by letting Brighter discover the transaction by using the resolver and all it's stuff.

Or maybe I'm missing something relevant here?

IlSocio avatar May 08 '22 17:05 IlSocio

If you have 2 DBs then you'll need two outboxes realistically

preardon avatar May 08 '22 17:05 preardon

Why do you have two Dbs? That seems quite unusual. If one is read and one is write i.e. CQRS then you should be using Darker for the R side, which doesn't require Outbox etc.

I'm trying to understand the actual scenario that we are trying to support here before we make things more complex.

Every method we expose on our interface is a point of coupling, so exposing the transaction provider on the deposit method carries the issue of coupling you to an implementation detail.

If this really is a valid scenario, we would like to support it simply - with any pain in the configuration section - as opposed to that leaking through too much to the handler code.

But I worry this is an edge case.

iancooper avatar May 08 '22 18:05 iancooper

I'm evaluating to use Brighter in many different projects for the outbox pattern. Some of these projects may use CQRS, most of them will not, some will use DbContext, others ADO net, others dapper. I can't know in advance all the scenarios that I'm going to face. It may happen that I'll have to support multiple different databases for writing (maybe even of different kind)

The key point for me, is flexibility, so I need to use a tool that is able to easily accommodate many different scenarios.

Brighter sounds to be a good fit because it supports many different providers, but, I'm realizing that, instead of being just a tool, it also imposes some constraints in the way we're supposed to design our projects. So, I'm trying to give my contribution to let it be more flexible and in case it will not be possible I'm going to proceed with PlanB and will create a fork.

IlSocio avatar May 08 '22 18:05 IlSocio

@IlSocio I am not trying to be difficult, but I often find that it helps us to figure out the actual requirement that we have. I'm old XP person, and the problem with theoretical requirements is they can add weight and complexity, and speculatively fixing them tends to mean we actually fix the wrong thing, or its not quite right.

So what I need to understand is what the likely ask for a project:

  • CQRS uses two Db contexts? Use Darker and Brighter.
  • App has two Dbs on the write side. What is the reason? Legacy? Polyglot persistence? In these cases the issue would be that the kind of Db might well differ, which means that it's not as simple as having one transaction provider type that maps to the relevant request, but different transaction provider types.
  • Etc.

So that is what we would need for this, a more concrete actual use case.

Of course, you are always at liberty to fork and do your own thing. As usual that comes with it becoming difficult to take our point releases. If it were me I would wait until we discovered an actual use case. We have some ideas how to solve this if we need to

iancooper avatar May 08 '22 19:05 iancooper