altmann / FluentResults

A generalised Result object implementation for .NET/C#
MIT License
2.07k stars 115 forks source link

Using FluentResults inside a MediatR validation pipeline #54

Closed PlexRipper closed 4 years ago

PlexRipper commented 4 years ago

Hi there,

First off, this is a great library and I'm a big fan of it!

I initially followed this article but instead of reinventing the wheel I started using FluentResults in my Fluent Validation pipeline. Basically all responses coming from my CQRS Queries are wrapped in the Result object, this avoids having to work with exceptions as a method of error handeling.

However, I can't get my pipeline to play nice:

    public class ValidationPipeline<TRequest, TResponse>
        : IPipelineBehavior<TRequest, TResponse>
        where TResponse : class
        where TRequest : IRequest<TResponse>
    {
        private readonly IValidator<TRequest> _compositeValidator;

        public ValidationPipeline(IValidator<TRequest> compositeValidator)
        {
            _compositeValidator = compositeValidator;
        }

        public async Task<TResponse> Handle(TRequest request, CancellationToken cancellationToken, RequestHandlerDelegate<TResponse> next)
        {
            var result = await _compositeValidator.ValidateAsync(request, cancellationToken);

            if (!result.IsValid)
            {
                Error error = new Error();
                var responseType = typeof(TResponse);

                foreach (var validationFailure in result.Errors)
                {
                    Log.Warning($"{responseType} - {validationFailure.ErrorMessage}");
                    error.Reasons.Add(new Error(validationFailure.ErrorMessage));
                }
                // This always returns null instead of a Result with errors in it. 
                var f = Result.Fail(error) as TResponse;
                return f;

                // This causes an exception saying it cannot find the constructor.
                var invalidResponse =
                        Activator.CreateInstance(invalidResponseType, null) as TResponse;

            }

            return await next();
        }
    }

I also have to somehow convert the Result object back to TResponse, where TResponse is always a Result

Any suggestions are greatly appreciated!

Edit: Could the Result constructor be made public instead of internal?

altmann commented 4 years ago

Hi,

thanks for the feedback! I also used Mediatr in a couple of projects - some with and some without the result pattern. I would prefer the following way:

 public class ValidationPipeline<TRequest, Result<TResponse>>
        : IPipelineBehavior<TRequest, TResponse>
        where TResponse : class
        where TRequest : IRequest<Result<TResponse>>
    {
        private readonly IValidator<TRequest> _compositeValidator;

        public ValidationPipeline(IValidator<TRequest> compositeValidator)
        {
            _compositeValidator = compositeValidator;
        }

        public async Task<TResponse> Handle(TRequest request, CancellationToken cancellationToken, RequestHandlerDelegate<TResponse> next)
        {
            var result = await _compositeValidator.ValidateAsync(request, cancellationToken);

            if (!result.IsValid)
            {
                Error error = new Error();
                var responseType = typeof(TResponse);

                foreach (var validationFailure in result.Errors)
                {
                    Log.Warning($"{responseType} - {validationFailure.ErrorMessage}");
                    error.Reasons.Add(new Error(validationFailure.ErrorMessage));
                }

                return Result.Fail<TResponse>(error);
            }

            return await next();
        }
    }

I'am not sure if its 100% buildable - but I think you will get the point. In this solution it is explicit that you use the Result object, its typesafe and you don't need Reflection magic. What do you think? Is it helpful?

mwhelan commented 4 years ago

This is how I am using FluentResults with FluentValidation and Mediatr on my current project:

public class RequestValidationBehaviour<TRequest, TResponse> : IPipelineBehavior<TRequest, TResponse>
    where TRequest : CommandBase
    where TResponse : Result
{
    private readonly ValidationService _validationService;
    private readonly ILogger<TRequest> _logger;

    public RequestValidationBehaviour(IEnumerable<IValidator<TRequest>> validators, ILogger<TRequest> logger)
    {
        _validationService = new ValidationService(validators);
        _logger = logger;
    }

    public async Task<TResponse> Handle(TRequest command, CancellationToken cancellationToken, RequestHandlerDelegate<TResponse> next)
    {
        var validationResult = _validationService.ValidateCommand(command);

        if (validationResult.IsFailed)
        {
            return validationResult as TResponse;
        }

        var handlerResult = await next();

        var result = _validationService.EvaluateResults(command.IgnoreWarnings, validationResult, handlerResult);

        return result as TResponse;
    }
}

public class ValidationService
{
    public IEnumerable<IValidator> FluentValidators { get; }

    public ValidationService(IEnumerable<IValidator> fluentValidators)
    {
        FluentValidators = fluentValidators;
    }

    public Result ValidateCommand<TCommand>(TCommand command) where TCommand : CommandBase
    {
        var result = Result.Ok();

        if (!FluentValidators.Any())
        {
            return result;
        }

        var context = new ValidationContext<TCommand>(command);

        var failures = FluentValidators
            .Select(v => v.Validate(context))
            .SelectMany(validationResult => validationResult.Errors)
            .Where(f => f != null)
            .ToList();

        if (failures.Count == 0)
        {
            return result;
        }

        Log.Warning("One or more validation failures have occurred.: {Name} {@ValidationErrors} {@Request}",
            command.GetType().Name, failures, command);

        if (failures.Any(x => x.Severity == Severity.Error))
        {
            return Result.Fail("Validation failures")
                .WithValidationResults(failures);
        }

        if (failures.Any(x => x.Severity == Severity.Warning))
        {
            if (command.IgnoreWarnings)
            {
                return Result.Ok()
                    .WithValidationResults(failures);
            }
            else
            {
                return Result.Fail("Validation failures")
                    .WithValidationResults(failures);
            }
        }

        return result;
    }

    public Result EvaluateResults(bool ignoreWarnings, params Result[] results)
    {
        var result = Result.Merge(results);

        if (result.HasErrors())
        {
            return result;
        }

        if (result.HasWarnings() && ignoreWarnings == false)
        {
            return result.WithReason(new TreatWarningsAsErrors());
        }

        return result;
    }
}

Before the handler runs, I validate the command. After the handler completes, I process any Results that came back from the domain. I have extended Results to include Warnings. As I'm only validating commands, I am not returning any data and so don't use the Result response. That being said, I do return the new ID in the Create command, but I do that with a Success Reason:

public class RecordsCreatedSuccess : Success
{
    public List<int> NewIds { get; }

    public RecordsCreatedSuccess(int id) 
        : base("Record created")
    {
        NewIds = new List<int> {id};
    }

    public RecordsCreatedSuccess(List<int> newIds)
        : base("Records created")
    {
        NewIds = newIds;
    }
}
victormarante commented 4 years ago

@mwhelan nice approach to validation. Is it possible to see the full source code? Specifically thinking about the extensions you've made to Results and such.

mwhelan commented 4 years ago

Sure, you can see the full thing in a sample I use for one of my open source projects. It's a bit experimental, so I welcome any feedback on how it might be improved.

If you want to understand the end-to-end, have a look at the ToDoController. You can also see how I map fluent results that come back from Mediatr handlers to ASP.Net Core action results using extension methods.

victormarante commented 4 years ago

@mwhelan Thanks for the reply and sample code. Your project was quite advanced for my level of C#, but I got a good idea of how to use the Result type, so thanks for that.

JasonLandbridge commented 4 years ago

Thank you very much @altmann and @mwhelan!

@altmann, I tried your solution but I couldn't get it to work but after some trial and error I found the reason it didn't work which I have corrected in my version below. I'm also very happy I didn't have to resort to the reflection nonsense and the end-result is very clean. :)

@mwhelan, I would not have figured out the above solution without your great example!

Incidentally, it was both the code samples you provided that allowed me fix it. There were 2 errors in the @altmann sample which I have fixed below:

ValidationPipeline.cs

// "ValidationPipeline<TRequest, Result<TResponse>" should be "ValidationPipeline<TRequest, TResponse>"
// This was causing a build error: Type parameter 'Result' hides class 'Result'
public class ValidationPipeline<TRequest, TResponse>
    : IPipelineBehavior<TRequest, TResponse>
    where TResponse : Result
    where TRequest : IRequest<Result>
{
    private readonly IValidator<TRequest> _compositeValidator;

    public ValidationPipeline(IValidator<TRequest> compositeValidator)
    {
        _compositeValidator = compositeValidator;
    }

    public async Task<TResponse> Handle(TRequest request, CancellationToken cancellationToken, RequestHandlerDelegate<TResponse> next)
    {

        var result = await _compositeValidator.ValidateAsync(request, cancellationToken);

        if (!result.IsValid)
        {
            Error error = new Error();
            var responseType = typeof(TResponse);

            foreach (var validationFailure in result.Errors)
            {
                Log.Warning($"{responseType} - {validationFailure.ErrorMessage}");
                error.Reasons.Add(new Error(validationFailure.ErrorMessage));
            }
            // "Result.Fail<TResponse>(error);" should be "Result.Fail(error) as TResponse;"
            // This caused a build error as it could not implicitly cast this to TResponse,
            // I tried to solve this by adding "as TResponse" but this in turn caused the return value to always be null;
            // The solution was to remove the "<TResponse>" and then it worked!
            return Result.Fail(error) as TResponse;
        }

        return await next();
    }
}

CreatePlexAccountCommand.cs

public class CreatePlexAccountCommand : IRequest<Result<PlexAccount>>
{
    public PlexAccount PlexAccount { get; }

    public CreatePlexAccountCommand(PlexAccount plexAccount)
    {
        PlexAccount = plexAccount;
    }
}

public class CreatePlexAccountCommandValidator : AbstractValidator<CreatePlexAccountCommand>
{
    public CreatePlexAccountCommandValidator()
    {
        RuleFor(x => x.PlexAccount.Id).Equal(0).WithMessage("The Id should be 0 when creating a new PlexAccount");
        RuleFor(x => x.PlexAccount.Username).NotEmpty().MinimumLength(5);
        RuleFor(x => x.PlexAccount.Password).NotEmpty().MinimumLength(5);
        RuleFor(x => x.PlexAccount.DisplayName).NotEmpty();
    }
}

public class CreateAccountHandler : BaseHandler, IRequestHandler<CreatePlexAccountCommand, Result<PlexAccount>>
{
    private readonly IPlexRipperDbContext _dbContext;

    public CreateAccountHandler(IPlexRipperDbContext dbContext)
    {
        _dbContext = dbContext;
    }

    public async Task<Result<PlexAccount>> Handle(CreatePlexAccountCommand command, CancellationToken cancellationToken)
    {
        Log.Debug("Creating a new Account in DB");

        await _dbContext.PlexAccounts.AddAsync(command.PlexAccount);
        await _dbContext.SaveChangesAsync(cancellationToken);
        await _dbContext.Entry(command.PlexAccount).GetDatabaseValuesAsync();

        return ReturnResult(command.PlexAccount, command.PlexAccount.Id);
    }
}

I tested my Queries and Commands and everything seems to be working!

Thanks again for the help!

mwhelan commented 4 years ago

thanks @JasonLandbridge , I'm glad it helped.

altmann commented 4 years ago

@JasonLandbridge @mwhelan Thanks for all the responses. I think this is a very interesting and great sample of using FluentResult in combination with MediatR. I spend today some time to create a solution for some common usecases with MediatR.

I tried @JasonLandbridge solution but it don't work in usecase Query with Result in which the ValidationPipeline is not executed at all. In usecase Command with Result your solution works. Is is really working? What did you do that it works?

I made a new branch features/MediatRSample with all the code which is necessary to test the MediatR ValidationPipeline. My solution which is based from all your answers works in all success cases in all above usecases but don't work in the fail case of usecase Query with Result. The problem is that validationResult as TResponse returns null but should return a failed result of type Result<T>.

public class ValidationPipelineBehavior<TRequest, TResponse>
        : IPipelineBehavior<TRequest, TResponse>
        where TResponse : ResultBase
    {
        public async Task<TResponse> Handle(TRequest request, CancellationToken cancellationToken, RequestHandlerDelegate<TResponse> next)
        {
            var validationResult = await ValidateAsync(request);
            if (validationResult.IsFailed)
            {
                return validationResult as TResponse;
            }

            return await next();
        }

        private async Task<Result> ValidateAsync(TRequest request)
        {
            // do here some validation, for example with fluentvalidation
            return Result.Fail("Validation failed");
        }
    }

I added a unit test which also face the problem that result as TResponse returns null. I think the problem is something with casting generic types and that the generic type is not known at compile time. If you have any hints please let me know.

        [TestMethod]
        public void ImplicitCastOperator_ReturnFailedGenericValueResult()
        {
            var result = Result.Fail("First error message");

            // Act
            var convertedResult = ConvertTo<Result<SampleResponse>>(result);

            // Assert
            convertedResult.IsFailed.Should().BeTrue();
        }

        public TResponse ConvertTo<TResponse>(Result result) where TResponse : ResultBase
        {
            return result as TResponse;
        }

Another story: Our solution works only with Autoface, Unity, ... but not with the asp.net core build in Microsoft dependency injection package because the Microsoft dependency injection package doesn't consider constraints and so the ValidationPipeline is injected in all usecases which is not correct. @jbogard already created an issue in the Microsoft dependency injection repository.

JasonLandbridge commented 4 years ago

@altmann, the result as TResponse; returning a null was the exact problem I was stuck on. I managed to solve it my case, and I will take a look at your branch and see if i can fix it.

Also, I'm probably the biggest fan of FluentResults ever because I'm making a Typescript version of FluentResults as we speak, with the same syntax so it should work 1 on 1. It's called FluentTypeResults and I have just finished converting everything to Typescript, and now I will test it out in my own project.

The idea behind is to have my .NET Core WebApi always return a Fluent Result. And then in my Vue.js/Typescript front-end cast the API response to a Typescript Result and from there check if there are any errors which I would like to show to the user :D

Let me know is this is good or bad practice because I couldn't find any articles or resources on this.

Edit: wrong account..

altmann commented 4 years ago

@altmann, the result as TResponse; returning a null was the exact problem I was stuck on. I managed to solve it my case, and I will take a look at your branch and see if i can fix it.

This would be great! Let me know if you know the a solution.

Also, I'm probably the biggest fan of FluentResults ever because I'm making a Typescript version of FluentResults as we speak, with the same syntax so it should work 1 on 1. It's called FluentTypeResults and I have just finished converting everything to Typescript, and now I will test it out in my own project.

The idea behind is to have my .NET Core WebApi always return a Fluent Result. And then in my Vue.js/Typescript front-end cast the API response to a Typescript Result and from there check if there are any errors which I would like to show to the user :D

Let me know is this is good or bad practice because I couldn't find any articles or resources on this.

I created an issue #56 so that we can discuss this topic separately.

JasonLandbridge commented 4 years ago

@altmann, I have been trying to find a solution for the past several hours but I can't get it to work. I'm stuck on the validation pipeline not being called whatever I do or change. I can however confirm its working in my own project, on queries and commands with results.

I will make an repository now with a working sample of how I have everything setup and then it might be easier to figure out where the problem lies.

JasonLandbridge commented 4 years ago

@altmann, okay scratch that.. In my project it's also not working, the pipeline is never called and instead it was validating against the web API layer in which I also have Fluent Validation integrated. Which caused me to wrongly assume that it is working, I tried it with my queries and commands directly and none of it is working :(

Apologies for the misleading, I am now trying to find a solution here: FluentResultMediatr

I'm now at the same point where either the validation pipeline is not called, or it is called if I just have a typeless Result in my query but then I can't convert the Result to TResponse since I end up with null.

I think I should first solve the pipeline not being called and once that is working I should be able to convert Result correctly to TResponse.

JasonLandbridge commented 4 years ago

I managed to resolve the TResponse to null issue, but it does require a public parameterless constructor in Result:

public class ValidationPipeline<TRequest, TResponse> : IPipelineBehavior<TRequest, TResponse>
        where TResponse : ResultBase, new()
        // We need a common parent between Result and Result<T>,
        // and we need the new() constraint because we're going to instantiate the TResponse
        // The new() constraint does require a parameterless constructor in ResultBase,
        // otherwise the pipeline is not called
    {
        private readonly IValidator<TRequest> _compositeValidator;

        public ValidationPipeline(IValidator<TRequest> compositeValidator)
        {
            _compositeValidator = compositeValidator;
        }

        public async Task<TResponse> Handle(TRequest request, CancellationToken cancellationToken, RequestHandlerDelegate<TResponse> next)
        {

            var result = await _compositeValidator.ValidateAsync(request, cancellationToken);

            if (!result.IsValid)
            {
                Error error = new Error();
                var responseType = typeof(TResponse);
                Type[] typeParameters = responseType.GetGenericArguments();

                foreach (var validationFailure in result.Errors)
                {
                    error.Reasons.Add(new Error(validationFailure.ErrorMessage));
                }

                // Instantiate the TResponse
                var f = new TResponse();
                // Add error, this is a bit hacky as I didn't have an easy way to add an error to ResultBase
                f.WithReason(error);
                // Returns non-null and valid result of type TResponse
                return f;
            }

            return await next();
        }
    }

I imported FluentResults into the project directly and made the changes to the Result constructor, everything can be found here: FluentResultMediatr

Edit: I should mention that I made an Web API with swagger to test everything, start the project and then go to: http://localhost:5000/swagger/index.html#/ and try 0 as an invalid value.

altmann commented 4 years ago

Thanks @JasonLandbridge for your time and for your solution. I think its no problem to make the constructor public so that the combination of MediatR and FluentResults works without reflection.

I will have a look at your solution today or the next days and will respond here.

JasonLandbridge commented 4 years ago

@altmann, Awesome, thanks!

altmann commented 4 years ago

@JasonLandbridge I have a look yesterday evening and your solution works in all usecases in success and failed state. Congratulations. The problem is that you also add this method to ResultBase which return ResultBase which is not expected as consumer. Consumer use Result and Result and know nothing about ResultBase. I will have a look at this the next days.

        public ResultBase WithReason(Reason reason)
        {
            Reasons.Add(reason);
            return this;
        }
JasonLandbridge commented 4 years ago

Nice! Yeah me adding the WithReason to ResultBasewas more to showcase the fix to get the Errors from FluentValidation into the Result.Fail(), but now I realize I'm returning a ResultBase and not a Result.

That is the tricky issue, the ResultBase is the only common parent between Result and Result<T> which is why I added the WithReason to ResultBase because I could otherwise not add errors to the TResponse and have the ValidationPipeline be called at the same time.

If I change to, as a constraint, where TResponse : Result, new() then the ValidationPipeline does not get called anymore in the case of Result<T>, and if change it to where TResponse : Result<object>, new() then I am faced with an Autofac exception which I don't really understand. I figured we could maybe have two validation pipelines, one for Result and another for Result<T>, but so far Result<T> is only called in the case of where TResponse : ResultBase, new(), and then we have gone full circle again.

It might be noted that that the ValidationPipeline always returns a Result.Fail() if the validation fails, regardless of if the TResponse is Result or Result<T> and thus doesn't have to be typed like Result<T> for it to work. If the validation is successful then it will call next(), which is a callback and continues on from there. Which means the ValidationPipeline isn't concerned with the T in Result<T> as in the case of failure the Result.Value is always default(T) and therefore always returns a Result.Fail().

This might help a bit in finding a solution but this was what I found out so far. No rush and I'm already grateful that you're willing to take a look at this 👍

altmann commented 4 years ago

@JasonLandbridge Based on your idea to create an TResponse with new TResponse() the solution is now the following:

if (validationResult.IsFailed)
{
   var result = new TResponse();

   foreach (var reason in validationResult.Reasons)
      result.Reasons.Add(reason);

   return result;
}

Mediatr Sample

I only have to make the constructors of Result and Result<T> public which is okay for me. A new method WithReason(...) is not necessary because the Property Reasons is from type List<Reason> so its possible to easily add new reasons over the property. For me it was a suprise that the property Reasons is from type List<Reason> and not from type ReadOnlyList<Reason>. With ReadOnlyList the list would be protected from the outside and the list could only be manipulated over the Result public interface like WithReason(...), WithError(...) etc. I would like that but I will not change that in the future to prevent breaking changes.

Next steps:

Thanks for all!

JasonLandbridge commented 4 years ago

Awesome @altmann, thank you very much!

altmann commented 4 years ago

New package is available via nuget which support mediatr usecase. See https://www.nuget.org/packages/FluentResults/2.1.0 The full functional sample code can be found here: https://github.com/altmann/FluentResults/tree/master/src/FluentResults.Samples.MediatR/MediatRLogic

Thanks for all the input and support! Issue closed

coderdnewbie commented 3 years ago

Good news update. In link https://github.com/altmann/FluentResults/issues/54#issuecomment-667670664 it states:

‘Another story: Our solution works only with Autoface, Unity, ... but not with the asp.net core build in Microsoft dependency injection package because the Microsoft dependency injection package doesn't consider constraints and so the ValidationPipeline is injected in all usecases which is not correct. @jbogard already created an issue in the Microsoft dependency injection repository.’

This is now supported in Microsoft Dependency Injection in .Net 5.0, as indicated by @jbogard in this article:

https://jimmybogard.com/constrained-open-generics-support-merged-in-net-core-di-container/

altmann commented 3 years ago

@coderdnewbie Thanks for sharing the good news.

majormartintibor commented 1 year ago

Here is my implementation of using MediatR together with FluentResults and FluentValidation

`internal sealed class ValidationPipelineBehaviour<TRequest, TResponse> : IPipelineBehavior<TRequest, TResponse> where TRequest : IRequest where TResponse : ResultBase, new() { private readonly IEnumerable<IValidator> _validators;

public ValidationPipelineBehaviour(IEnumerable<IValidator<TRequest>> validators)
    => _validators = validators;

public async Task<TResponse> Handle(
    TRequest request, 
    RequestHandlerDelegate<TResponse> next, 
    CancellationToken cancellationToken)
{        
    if(! _validators.Any())
    {
        return await next();
    }

    var errors = _validators
        .Select(validator => validator.Validate(request))
        .SelectMany(validationResult => validationResult.Errors)
        .Where(validationFailure => validationFailure is not null)
        .Select(failure => new Error(failure.ErrorMessage))
        .Distinct()
        .ToArray();

    if (errors.Any())
    {
        var result = new TResponse();

        foreach (var error in errors)
            result.Reasons.Add(error);

        return result;
    }

    return await next();
}

}`

I can add Validations like this for example:

internal class GetReviewQueryValidator : AbstractValidator<GetReviewQuery> { public GetReviewQueryValidator() { RuleFor(x => x.Id) .NotEmpty() .GreaterThanOrEqualTo(1); } }

I can have multiple Validators and in the end all validation failures will be put into 1 Result with all failures gathered in the reasons. This is still a work in progress. I don1t use the dogmatic clean architecture, I use hexagonal architecture and this logic is found in the "Core" project (kinda merges the Domain and Application into one). In my API layer I try to map the Result into something more readable:

`public abstract class ApiController : ControllerBase { //TODO: figure out how we want to map errors to appropriate staus codes, like 404 Not Found //currently all failures will be treated as 400 BadRequests protected IActionResult HandleFailure(Result result) => result switch { { IsSuccess: true } => throw new InvalidOperationException(), _ => BadRequest( CreateProblemDetails( "Validation Error", StatusCodes.Status400BadRequest, result.Reasons)) };

private static ProblemDetails CreateProblemDetails(
    string title,
    int status,
    List<IReason> reasons) =>
    new()
    {
        Title = title,
        Status = status,
        Type = string.Empty,
        Detail = string.Empty,
        Extensions = { { nameof(reasons), reasons } }
    };

}`

It ends up looking like this: { "type": "", "title": "Validation Error", "status": 400, "detail": "", "reasons": [ { "message": "'Id' must not be empty.", "metadata": {} }, { "message": "'Id' must be greater than or equal to '1'.", "metadata": {} } ] }

Now I need to figure out how to elegantly map results to better status codes. Like having 404 not found.