EventFlow

EventFlow copied to clipboard

Issue

I was having issues getting any command to fail when following the docs. After digging into the code and debugging I noticed that the CommandHandler<TAggregate, TIdentity, TCommand> abstract class will NEVER allow any command to fail. It disreguards the results of the command and returns back ExecutionResult.Success()

Related code

https://github.com/eventflow/EventFlow/blob/release-v0/Source/EventFlow/Commands/CommandHandler.cs#L56-L57

Workaround

To allow this to work without a fix you can implement the CommandHandler<TAggregate, TIdentity, TResult, TCommand> abstract class using IExecutionResult for the TResult and renaming ExecuteAsync to ExecuteCommandAsync.

Can you create a PR that illustrate and reproduce the problem?

On 8 Mar 2022, at 18.06, Jamie S @.***> wrote:

 Issue

I was having issues getting any command to fail when following the docs. After digging into the code and debugging I noticed that the CommandHandler<TAggregate, TIdentity, TCommand> abstract class will never allow any command to fail. It disreguards the results of the command and returns back ExecutionResult.Success()

Related code

https://github.com/eventflow/EventFlow/blob/release-v0/Source/EventFlow/Commands/CommandHandler.cs#L56-L57

Workaround

To allow this to work without a fix you can implement the CommandHandler<TAggregate, TIdentity, TResult, TCommand> abstract class using IExecutionResult for the TResult and renaming ExecuteAsync to ExecuteCommandAsync.

— Reply to this email directly, view it on GitHub, or unsubscribe. You are receiving this because you are subscribed to this thread.

Is that necessary as it's clear that this code disregards any response from a command handler and always returns ExecutionResult.Success()?

I'm pretty busy with other tasks, and it seems to be a simple control flow bug.

Sorry, if it's an obvious error, I'll have a look when I get back on my feet. Still feeling the after effects of being sick with corona the week before last.

On 8 Mar 2022, at 18.41, Rasmus Mikkelsen @.***> wrote:

 Can you create a PR that illustrate and reproduce the problem?

On 8 Mar 2022, at 18.06, Jamie S @.***> wrote:

 Issue

I was having issues getting any command to fail when following the docs. After digging into the code and debugging I noticed that the CommandHandler<TAggregate, TIdentity, TCommand> abstract class will never allow any command to fail. It disreguards the results of the command and returns back ExecutionResult.Success()

Related code

https://github.com/eventflow/EventFlow/blob/release-v0/Source/EventFlow/Commands/CommandHandler.cs#L56-L57

Workaround

To allow this to work without a fix you can implement the CommandHandler<TAggregate, TIdentity, TResult, TCommand> abstract class using IExecutionResult for the TResult and renaming ExecuteAsync to ExecuteCommandAsync.

— Reply to this email directly, view it on GitHub, or unsubscribe. You are receiving this because you are subscribed to this thread.

I don't think it's a bug. Having gone through the code myself, I understand that you will want to define your CommandHandler to inherit from IExecutionResult if you care about the result, and without IExecutionResult if you don't (i.e. relying on DomainError).

So in other words, if you need the command to fail with returned value, have your command handler from CommandHandler<TAggregate, TIdentity, TResult, TCommand>. Otherwise, use CommandHandler<TAggregate, TIdentity, TCommand> when your processing does not return any value. So in the latter case, returning ExecutionResult.Success is correct because there's no exception thrown.

@Stoom the intent is that you either go with the "old" model, which is to throw a exception, e.g. using the provided DomainError. Or you use the "new" method, which is to return a proper IExecutionResult and validate the return value.

Its slightly documented here https://docs.geteventflow.net/Commands.html#execution-results

Here's the code referenced, as you pointed out. The result succeeds. If you want the result to be passed back, you need to use the <TAggregate, TIdentity, TResult, TCommand> version. https://github.com/eventflow/EventFlow/blob/develop-v1/Source/EventFlow/Commands/CommandHandler.cs#L57

I faced the same issue today. Indeed, after changing the Command to : Command<MyAggregate, MyIdentity, **IExecutionResult**> it works as expected. But I think the way it is implemented (and documented) is not very obvious. I think this is a place where people can easily get confused..

Good point, will add it to the documentation On 9 Nov 2022, at 11.58, Ilia Tsvetkov @.***> wrote: I faced the same issue today. Indeed, after changing the Command to : Command<MyAggregate, MyIdentity, IExecutionResult> it works as expected. But I think the way it is implemented (and documented) is not very obvious. I think this is a place where people can easily get confused..

—Reply to this email directly, view it on GitHub, or unsubscribe.You are receiving this because you commented.Message ID: @.***>

Does it make more sense to not have that method return anything? If it always returns success then the only point I could see to return it is to make integrating into other methods easier, at the expense of discoverability/increase cognitive load when trying to figure things out like this issue

Hello there!

We hope you are doing well. We noticed that this issue has not seen any activity in the past 90 days. We consider this issue to be stale and will be closing it within the next seven days.

If you still require assistance with this issue, please feel free to reopen it or create a new issue.

Thank you for your understanding and cooperation.

Best regards, EventFlow

Hello there!

This issue has been closed due to inactivity for seven days. If you believe this issue still needs attention, please feel free to open a new issue or comment on this one to request its reopening.

Thank you for your contribution to this repository.

Best regards, EventFlow