Remove ValidationBase

[janschmidmaier50]

There is now the FluentValidation in place, which makes things easier.

Keep on reading, the surprise is down there ;)

Place of implementation

First of all the class ValidationBase<> and the corresponding interface IValidator should be part of the Arc4u framework and not be generated for each project. Why is that ? Imho there is no need to adjust the implementation of ValidationBase<> and even if so, this can be done within the project itself. Furthermore right now there will be multiple occurrences of the exact same code within one solution, because it will be generated for every single backend service. This is senseless code duplication, which should actually be found by code inspection. Right now, when using the guidance for Visual Studio 2019 a ValidationBase (non generic) was created. So if one now creates a new business object with the guidance 2022 a Validator will be created inheriting the not existing ValidationBase<> and using the not available FluentValidation. If this would have been implemented within the Arc4u, it would have been obvious and the old ValidationBase could have been marked obsolete, giving the team time to adjust their code.

ValidationBase<> implementation

The ValidationBase<> must not keep track of the object to validate, as well as the messages. Doing so makes the instantiation of the class harder, the calling not threadsafe. Furthermore the code is not idempotent, because running Validate() one time will manipulate the message object differently then running it a couple of times. It is also not obvious that calling Validate() will change the provided (by ctor) Messages object, which violates the CleanCode principle and makes the Validate() method unpure and dishonest. So instead of

public abstract class ValidatorBase<TElement> : AbstractValidator<TElement>, IValidator where TElement : class
{
    protected ValidatorBase(Messages messages, TElement instance)
    {
        _messages = messages;
        _instance = instance;
    }

    protected ValidatorBase(Messages messages, ValidationContext<TElement> validationContext)
    {
        _messages = messages;
        _validationContext = validationContext;
    }

    private readonly Messages _messages;
    private readonly TElement _instance;
    private readonly ValidationContext<TElement> _validationContext;

    public void Validate()
    {
        var result = null != _instance ? base.Validate(_instance) : base.Validate(_validationContext);

        _messages.AddRange(result.ToMessages());
    }

    public async Task ValidateAsync()
    {
        var result = (null != _instance) ? await ValidateAsync(_instance) : await ValidateAsync(_validationContext);

        _messages.AddRange(result.ToMessages());
    }
}

it might be

// remark: ValidationContext is not being shown, but should be implemented the same way
public abstract class ValidatorBase<TElement> : AbstractValidator<TElement>, IValidator where TElement : class
{  
    public Messages Validate(in TElement toBeValidated, in Messages messages, bool ignoreWarnings)
    {
        var result = base.Validate(toBeValidated);

        var ret = new Messages(ignoreWarnings);
        ret.AddRange(messages);
        ret.AddRange(result.ToMessages());
        return ret;
    }

    public async Task<Messages> ValidateAsync(TElement toBeValidated, Messages messages, bool ignoreWarnings)
    {
        var result = await base.ValidateAsync(toBeValidated);

        var ret = new Messages(ignoreWarnings);
        ret.AddRange(messages);
        ret.AddRange(result.ToMessages());
        return ret;
    }
}

making the method pure, because there is no dependency anymore and the input objects are not modified anymore. So the provided objects can now be immutable. This also makes the validation threadsafe. Downside is now, that we need to provide the ignoreWarnings flag, because we can not read it from the provided messages, which the validation itself should not bother with. A possible improvement follows in the next chapter.

Messages vs ValidationResult

Up until now, we are always using Messages to handle results of the validation. Since we are using FluentValidation now, we might not need that anymore. We can now even separate things by their concern, so that there is a service for everything that needs to be done, like

• filtering result by severity
• log the results
• translate the results
• determine, if the results should produce an exception

So why don't we write extension methods for that ?

public static class ValidationResultExtension
{
    public static IEnumerable<ValidationFailure> FilterBy(this ValidationResult results, Severity severity)
    {
        return results.Errors.Where(failure => failure.Severity == severity);
    }

    public static void ThrowIfNeeded(this ValidationResult results, bool ignoreWarnings = true)
    {
        if (results.IsValid)
            return;
        if (ignoreWarnings && !results.FilterBy(Severity.Error).Any())
            return;
        throw new DataException(results.ToString()); //Note: Actually throwing an exception for a failed validation is maybe questionable after all
    }
}

public class ValidationResultLogging
{
    private readonly ILogger<ValidationResultLogging> _logger;

    public ValidationResultLogging(ILogger<ValidationResultLogging> logger)
    {
        _logger = logger;
    }

    public void Log(ValidationResult results)
    {
        foreach (var result in results.Errors)
        {
            CommonLoggerProperties property = DetermineLogFunction(result)(result.ErrorMessage);    
            if (!String.IsNullOrWhiteSpace(result.ErrorCode)) property.Add("Code", result.ErrorCode);
            property.Log();       
        }
    }

    private Func<string, CommonLoggerProperties> DetermineLogFunction(ValidationFailure result)
    {
        CommonMessageLogger categoryLogger = _logger.Technical();
        switch (result.Severity)
        {
            case Severity.Error: return s => categoryLogger.Error(s);
            case Severity.Warning: return s => categoryLogger.Warning(s);
            case Severity.Info: return s => categoryLogger.Information(s);
            default:
                throw new NotImplementedException($"severity {result.Severity} is not implemented");
        }
    }
}

(this is just a code snippet, on how things could be done!)

Having that, we could change the ValidationBase<> like that

public abstract class ValidatorBase<TElement> : AbstractValidator<TElement>, IValidator where TElement : class
{
    public ValidationResult Validate(in TElement toBeValidated) => base.Validate(toBeValidated);
    public async Task<ValidationResult> ValidateAsync(TElement toBeValidated) => await base.ValidateAsync(toBeValidated);
}

Conclusion

This way neither the ValidationBase<> nor the Messages (including the Message) is needed anymore - at least concerning the validation.

[vvdb-architecture]

See also https://github.com/GFlisch/Arc4u.Guidance.Doc/issues/30

[maxime-poulain]

IValidator does not expose ValidateAsync.

[vvdb-architecture]

IValidator does not expose ValidateAsync.

Please read #30

[cr-chris]

In addition, I think it could be better to use DI to get the FluentValidation.IValidator in BLs instead of giving it the responsibility to create concrete validators.

If we have specific rules based on action (insert, update, etc) or other property, we can use the When(...) feature or RuleSets.

For unit testing, when you want to test a business case from the BL, you have to provide a fully valid entity event if the case only needs some properties. You lose the advantage of AutoFixture as soon as you have to specify a valid value for several properties.

In addition, the unit test is not testing a single unit of the app anymore because it tests Business and Validation at the same time. Specially when the validation is running complexe rules which read data for example.

In addition to arguments above, the use of ValidationResult type allows us to use the extension method for unit testing (https://docs.fluentvalidation.net/en/latest/testing.html)

[vvdb-architecture]

In addition, I think it could be better to use DI to get the FluentValidation.IValidator in BLs instead of giving it the responsibility to create concrete validators.

Seconded. This has been documented and will be discussed in the "Business Layer New Model" meeting.