spectreconsole / spectre.console

A .NET library that makes it easier to create beautiful console applications.
https://spectreconsole.net
MIT License
9.19k stars 474 forks source link

When using custom command setting validation attributes, validation error message overrides ValidationResult error message. #498

Open MizMahem opened 3 years ago

MizMahem commented 3 years ago

Issue When using custom validation attributes derived from ParameterValidationAttribute, the validation error message provided in the attribute constructor overrides any ValidationResult error message. Using the ValidationResult messages returned by Validate is probably preferable because in this context the CommandParameterContext variable can be used to give more detailed feedback on any errors.

Example

namespace example
{
    using Spectre.Console;
    using Spectre.Console.Cli;
    using System;
    using System.IO;

    /// <summary>Validates that a attributed string points to a file that is readable.</summary>
    [AttributeUsage(AttributeTargets.Property, AllowMultiple = true)]
    public sealed class FileExistsValidatorAttribute : ParameterValidationAttribute
    {
        /// <summary>Initializes a new instance of the <see cref="FileExistsValidatorAttribute"/> class.</summary>
        /// <param name="errorMessage">The error message.</param>
        public FileExistsValidatorAttribute() : base("Path must point to a file that exist and be readable.") { }

        /// <summary>Validates that the attributed string points to a file that is readable.</summary>
        /// <param name="context">The attribute context.</param>
        /// <returns>A ValidationResult.</returns>
        public override ValidationResult Validate(CommandParameterContext context) => context.Value switch
        {
            string fileName when File.Exists(fileName) => ValidationResult.Success(),
            string fileName when !File.Exists(fileName) => ValidationResult.Error($"File \"{fileName}\" does not exist, or is unreadable. ({context.Parameter.PropertyName})."),
            _ => throw new InvalidOperationException($"Parameter is not a string ({context.Parameter.PropertyName})."),
        };
    }
}

Please upvote :+1: this issue if you are interested in it.

MizMahem commented 3 years ago

Digging into the code I found that giving the constructor a blank string or null causes the Validate error message to be used. This isn't very intuitive and isn't documented anywhere. And in a nullable context, the errorMessage parameter in the constructor can't even be null. Perhaps it should be allowed to be null for this purpose, or a constructor not taking an errorMessage should be added?

MaxAtoms commented 2 years ago

Looked into it today as I thought it would be a good issue to get me started on Hacktoberfest.

It really seems that there is no problem in making the ErrorMessage property in the ParameterValidationAttribute.cs nullable, like @MizMahem mentioned.

The CommandValidator already checks whether the ErrorMessage is null or whitespace:

https://github.com/spectreconsole/spectre.console/blob/d34012cad044cc7395e3b6a7e08624d7ba583d0e/src/Spectre.Console/Cli/Internal/CommandValidator.cs#L35-L39

The XmlDocCommand adds it as a nullable attribute.

There seems to be no test coverage, however. Maybe I could look into that and create a PR with test(s), @patriksvensson?