Eventuous / eventuous

Event Sourcing library for .NET
https://eventuous.dev
Apache License 2.0
442 stars 70 forks source link

Ability to return a custom return type for a command API reponse #270

Closed LockTar closed 10 months ago

LockTar commented 11 months ago

Is your feature request related to a problem? Please describe. The state property of the Result class is a string but in the swagger.json there is no type attribute. We're generating SDK's from our swagger files and the state is now ignored because of the missing type attribute. But even if that was in place, it's still an object so you're missing al the properties.

Describe the solution you'd like We want the ability to have:

  1. A custom return type other that the Result class.
  2. A State property from the Result class that is not of the type object?.

Describe alternatives you've considered Changing the CommandHttpApiBase to this:

/// <summary>
/// Base class for exposing commands via Web API using a controller.
/// </summary>
/// <typeparam name="TAggregate">Aggregate type</typeparam>
/// <typeparam name="TResult">Result type</typeparam>
[PublicAPI]
public abstract class CommandHttpApiBase<TAggregate, TResult> : ControllerBase
    where TAggregate : Aggregate
    where TResult : class, new()
{
    private readonly ICommandService<TAggregate> service;
    private readonly MessageMap? commandMap;

    public CommandHttpApiBase(ICommandService<TAggregate> service, MessageMap? commandMap = null)
    {
        this.service = service;
        this.commandMap = commandMap;
    }

    /// <summary>
    /// Call this method from your HTTP endpoints to handle commands and wrap the result properly.
    /// </summary>
    /// <param name="command">Command instance</param>
    /// <param name="cancellationToken">Request cancellation token</param>
    /// <typeparam name="TCommand">Command type</typeparam>
    /// <returns></returns>
    protected async Task<ActionResult<TResult>> Handle<TCommand>(TCommand command, CancellationToken cancellationToken)
        where TCommand : class
    {
        var result = await service.Handle(command, cancellationToken);

        return AsActionResult<TAggregate>(result);
    }

    /// <summary>
    /// Call this method from your HTTP endpoints to handle commands where there is a mapping between
    /// HTTP contract and the domain command, and wrap the result properly.
    /// </summary>
    /// <param name="command">HTTP command</param>
    /// <param name="cancellationToken">Cancellation token</param>
    /// <typeparam name="TContract">HTTP command type</typeparam>
    /// <typeparam name="TCommand">Domain command type</typeparam>
    /// <returns></returns>
    /// <exception cref="InvalidOperationException">Throws if the command map hasn't been configured</exception>
    protected async Task<ActionResult<TResult>> Handle<TContract, TCommand>(TContract command, CancellationToken cancellationToken)
        where TContract : class where TCommand : class
    {
        if (commandMap == null) throw new InvalidOperationException("Command map is not configured");

        var cmd = commandMap.Convert<TContract, TCommand>(command);
        var result = await service.Handle(cmd, cancellationToken);

        return AsActionResult<TAggregate>(result);
    }

    static ActionResult<TResult> AsActionResult<T>(Result result) where T : Aggregate
        => result is ErrorResult error
            ? error.Exception switch
            {
                OptimisticConcurrencyException<T> => new ConflictObjectResult(error),
                AggregateNotFoundException<T> => new NotFoundObjectResult(error),
                _ => new BadRequestObjectResult(error)
            }
            : new OkObjectResult(result);
}

Sample return class

public record UserResult : EventuousResult
{
    public new UserState? State { get; set; }
}

public record EventuousResult // The name is wrong but this should be `Result` record of Eventuous
{
    // Ignore. Only to support custom return type.
    internal EventuousResult() { }

    public EventuousResult(object? state, bool success, IEnumerable<Change>? changes = null)
    {
        State = state;
        Success = success;
        Changes = changes;
    }

    public object? State { get; set; }

    public bool Success { get; set; }

    public IEnumerable<Change>? Changes { get; set; }
}

Sample command

public class UsersCommandApi : CommandHttpApiBase<User, UserResult>
{
    public UsersCommandApi(ICommandService<User> service) : base(service) { }

    [HttpPost]
    [Route("add")]
    public Task<ActionResult<UserResult>> AddUser(
        [FromBody] AddUser cmd, CancellationToken cancellationToken
    )
        => Handle(cmd, cancellationToken);
}

Additional context Current: image

New: image

alexeyzimarev commented 6 months ago

I am now wondering if we should replace both Result and custom result by Result<TState>? Things become much simpler if we do.

LockTar commented 6 months ago

To be honest, we haven't updated to the latest version yet so we are still using our own implementation but can be swapped instantly with the latest version. They should end up with the same result.

I like the Result that Eventuous gives out of the box. The custom result is even better. But is that what everybody needs? We are thinking of a different setup ourselves.

Maybe we should first set the requirements? What do you think about these:

alexeyzimarev commented 5 months ago

Hm, but the last option isn't available now anyway, and there's no way to implement it using custom result too. Custom result is just a type that is given to the response type. The actual result always contain the list of changes, the commit position, and the updated state. Result<TState> just removes the illusion of having something "custom" and makes things explicit.

If we want to implement custom results that can contain an arbitrary payload, there must be a way to convert the default result, which is the Result<TState> to a custom result. That has never been discussed, and I am not sure how practical it is.

If we want to support custom result types, there must be overloads for command to HTTP mappers to accept the conversion function. Still, how would it work for controllers? Or, maybe, for controllers it's actually simpler as they call the service, it returns the Result<TState> instance, which can then be converted to whatever you want. It doesn't require having a generic constraint for the custom result type on the controller base class signature.

LockTar commented 5 months ago

I think that Eventuous should always return Result<TState>. That will give the result of the above screenshots (right?).

I then think there should be an option/hook to convert that Result<TState> to a custom result of your choice. This could be a virtual method or something.

See sample below how we are doing it right now on Eventuous version 0.15.0-beta.7.

/// <summary>
/// Base class for overriding the default behavior for result mapping.
/// </summary>
/// <typeparam name="TAggregate">Aggregate type</typeparam>
/// <typeparam name="TResult">Result type</typeparam>
public abstract class CustomCommandHttpApiBase<TAggregate, TResult> : CommandHttpApiBase<TAggregate, TResult>
    where TAggregate : Aggregate 
    where TResult : Result
{
    public CustomCommandHttpApiBase(ICommandService<TAggregate> service, MessageMap? commandMap = null) :
        base(service, commandMap)
    {
    }

    protected override ActionResult AsActionResult(Result result)
    {
        return result is ErrorResult error
            ? error.Exception switch
            {
                ValidationException => MapValidationExceptionAsValidationProblemDetails(error),
                _ => base.AsActionResult(result)
            }
            : base.AsActionResult(result);
    }

    private BadRequestObjectResult MapValidationExceptionAsValidationProblemDetails(ErrorResult error)
    {
        if (error is null || error.Exception is null || error.Exception is not ValidationException)
        {
            throw new ArgumentNullException(nameof(error), "Exception in result is not of the type `ValidationException`. Unable to map validation result.");
        }

        ValidationException exception = (ValidationException)error.Exception;

        var problemDetails = new ValidationProblemDetails()
        {
            Status = StatusCodes.Status400BadRequest,
            Detail = "Please refer to the errors property for additional details."
        };

        var groupFailures = exception.Errors.GroupBy(v => v.PropertyName);
        foreach (var groupFailure in groupFailures)
        {
            problemDetails.Errors.Add(groupFailure.Key, groupFailure.Select(s => s.ErrorMessage).ToArray());
        }

        return new BadRequestObjectResult(problemDetails);
    }
}

The controller is then like this:

public record UserResult : Result
{
    public new UserState? State { get; init; }
}

[Authorize]
[Route("/users")]
public class UsersCommandApi : CustomCommandHttpApiBase<User, UserResult>
{
    private readonly IIdentityService _identityService;

    public UsersCommandApi(IIdentityService identityService, ICommandService<User> service) : base(service)
    {
        _identityService = identityService;
    }

    [RequiredScopeOrAppPermission(
        new string[] { },
        new[] { PBRoles.User.Administrator,
                PBRoles.User.ServicedeskAdministrator,
                PBRoles.User.ServicedeskUser })]
    [HttpPost]
    [Route("add")]
    [SwaggerResponse((int)HttpStatusCode.OK, "OK", typeof(UserResult))]
    [SwaggerResponse((int)HttpStatusCode.BadRequest, "Bad Request", typeof(ValidationProblemDetailsException))]
    public Task<ActionResult<UserResult>> AddUser(
        [FromBody] AddUserCommand cmd, CancellationToken cancellationToken
    )
    {
        cmd.AddedBy = GetUserObjectId();
        cmd.AddedAt = DateTimeOffset.UtcNow;
        return Handle(cmd, cancellationToken);
    }
}

I think we have covered everthing with this setup except a 200 a complete custom body that doesn't inherit Result at all..

alexeyzimarev commented 5 months ago

I now changed the controller base signature to this:

public abstract class CommandHttpApiBase<TState>(IStateCommandService<TState> service, MessageMap? commandMap = null) : ControllerBase
    where TState : State<TState>, new() {

It allows using both aggregate and functional command services since both of them implement the IStateCommandService interface.

The interface itself looks like this:

public interface IStateCommandService<TState> where TState : State<TState>, new() {
    Task<Result<TState>> Handle<TCommand>(TCommand command, CancellationToken cancellationToken) where TCommand : class;
}

I think it will work for your code, the AsActionResult function remains virtual. If people don't want to use custom result, the controller can return Result<TState> directly, and the OpenAPI will be correct without any additional code.