[API Proposal]: Make it easier to create quality ValidateOptionsResult instances

[geeknoid]

Background and motivation

When writing implementations of the IValidateOptions.Validate, it is necessary to return a ValidateOptionsResult instance describing the result of validating input. Producing a quality instance is currently fairly clumsy, leading to 'peel the onion' validation errors. That is, validation usually terminates on the first validation error. This means as a user, you end up having to do the "edit/run/edit/run/edit/run" cycle multiple times until all the errors are addressed.

The proposal introduces a builder object that makes it simple for validation code to proceed incrementally and accumulate all the validation errors discovered.

API Proposal

namespace Microsoft.Extensions.Options;

/// <summary>
/// Builds <see cref="ValidateOptionsResult"/> with support for multiple error messages.
/// </summary>
[DebuggerDisplay("{_errors.Count} errors")]
public readonly struct ValidateOptionsResultBuilder
{
    /// <summary>
    /// Initializes a new instance of the <see cref="ValidateOptionsResultBuilder"/> struct.
    /// </summary>
    /// <param name="errors">An enumeration of error strings to initialize the builder with.</param>
    public ValidateOptionsResultBuilder(IEnumerable<string>? errors);

    /// <summary>
    /// Creates new instance of <see cref="ValidateOptionsResultBuilder"/> struct.
    /// </summary>
    /// <returns>New instance of <see cref="ValidateOptionsResultBuilder"/>.</returns>
    public static ValidateOptionsResultBuilder Create();

    /// <summary>
    /// Adds validation error.
    /// </summary>
    /// <param name="error">Content of error message.</param>
    public void AddError(string error);

    /// <summary>
    /// Adds validation error.
    /// </summary>
    /// <param name="propertyName">THe property in the option object which contains an error.</param>
    /// <param name="error">Content of error message.</param>
    public void AddError(string propertyName, string error);

    /// <summary>
    /// Adds any validation error carried by the <see cref="ValidationResult"/> instance to this instance.
    /// </summary>
    /// <param name="result">The instance to consume the errors from.</param>
    public void AddError(ValidationResult? result);

    /// <summary>
    /// Adds any validation error carried by the enumeration of <see cref="ValidationResult"/> instances to this instance.
    /// </summary>
    /// <param name="results">The enumeration to consume the errors from.</param>
    public void AddErrors(IEnumerable<ValidationResult>? results);

    /// <summary>
    /// Adds any validation errors carried by the <see cref="ValidateOptionsResult"/> instance to this instance.
    /// </summary>
    /// <param name="result">The instance to consume the errors from.</param>
    public void AddErrors(ValidateOptionsResult? result);

    /// <summary>
    /// Builds <see cref="ValidateOptionsResult"/> based on provided data.
    /// </summary>
    /// <returns>New instance of <see cref="ValidateOptionsResult"/>.</returns>
    public ValidateOptionsResult Build();
}

API Usage

internal sealed class HttpStandardResilienceOptionsCustomValidator : IValidateOptions<HttpStandardResilienceOptions>
{
    private const int CircuitBreakerTimeoutMultiplier = 2;

    public ValidateOptionsResult Validate(string name, HttpStandardResilienceOptions options)
    {
        var builder = ValidateOptionsResultBuilder.Create();

        if (options.AttemptTimeoutOptions.TimeoutInterval > options.TotalRequestTimeoutOptions.TimeoutInterval)
        {
            builder.AddError($"Total request timeout policy must have a greater timeout than the attempt timeout policy. " +
                $"Total Request Timeout: {options.TotalRequestTimeoutOptions.TimeoutInterval.TotalSeconds}s, " +
                $"Attempt Timeout: {options.AttemptTimeoutOptions.TimeoutInterval.TotalSeconds}s");
        }

        if (options.CircuitBreakerOptions.SamplingDuration < TimeSpan.FromMilliseconds(options.AttemptTimeoutOptions.TimeoutInterval.TotalMilliseconds * CircuitBreakerTimeoutMultiplier))
        {
            builder.AddError("The sampling duration of circuit breaker policy needs to be at least double of " +
                $"an attempt timeout policy’s timeout interval, in order to be effective. " +
                $"Sampling Duration: {options.CircuitBreakerOptions.SamplingDuration.TotalSeconds}s," +
                $"Attempt Timeout: {options.AttemptTimeoutOptions.TimeoutInterval.TotalSeconds}s");
        }

        if (options.RetryOptions.RetryCount != RetryPolicyOptions.InfiniteRetry)
        {
            TimeSpan retrySum = options.RetryOptions.GetRetryPolicyDelaySum();

            if (retrySum > options.TotalRequestTimeoutOptions.TimeoutInterval)
            {
                builder.AddError($"The cumulative delay of the retry policy cannot be larger than total request timeout policy interval. " +
                $"Cumulative Delay: {retrySum.TotalSeconds}s," +
                $"Total Request Timeout: {options.TotalRequestTimeoutOptions.TimeoutInterval.TotalSeconds}s");
            }
        }

        return builder.Build();
    }
}

Alternative Designs

No response

Risks

No response

[dotnet-issue-labeler]

I couldn't figure out the best area label to add to this issue. If you have write-permissions please help me learn by adding exactly one area label.

[ghost]

Tagging subscribers to this area: @dotnet/area-extensions-options
See info in area-owners.md if you want to be subscribed.

Issue Details

---

Background and motivation

When writing implementations of the IValidateOptions.Validate, it is necessary to return a ValidateOptionsResult instance describing the result of validating input. Producing a quality instance is currently fairly clumsy, leading to 'peel the onion' validation errors. That is, validation usually terminates on the first validation error. This means as a user, you end up having to do the "edit/run/edit/run/edit/run" cycle multiple times until all the errors are addressed.

The proposal introduces a builder object that makes it simple for validation code to proceed incrementally and accumulate all the validation errors discovered.

API Proposal

namespace Microsoft.Extensions.Options;

/// <summary>
/// Builds <see cref="ValidateOptionsResult"/> with support for multiple error messages.
/// </summary>
[DebuggerDisplay("{_errors.Count} errors")]
public readonly struct ValidateOptionsResultBuilder
{
    /// <summary>
    /// Initializes a new instance of the <see cref="ValidateOptionsResultBuilder"/> struct.
    /// </summary>
    /// <param name="errors">An enumeration of error strings to initialize the builder with.</param>
    public ValidateOptionsResultBuilder(IEnumerable<string>? errors);

    /// <summary>
    /// Creates new instance of <see cref="ValidateOptionsResultBuilder"/> struct.
    /// </summary>
    /// <returns>New instance of <see cref="ValidateOptionsResultBuilder"/>.</returns>
    public static ValidateOptionsResultBuilder Create();

    /// <summary>
    /// Adds validation error.
    /// </summary>
    /// <param name="error">Content of error message.</param>
    public void AddError(string error);

    /// <summary>
    /// Adds validation error.
    /// </summary>
    /// <param name="propertyName">THe property in the option object which contains an error.</param>
    /// <param name="error">Content of error message.</param>
    public void AddError(string propertyName, string error);

    /// <summary>
    /// Adds any validation error carried by the <see cref="ValidationResult"/> instance to this instance.
    /// </summary>
    /// <param name="result">The instance to consume the errors from.</param>
    public void AddError(ValidationResult? result);

    /// <summary>
    /// Adds any validation error carried by the enumeration of <see cref="ValidationResult"/> instances to this instance.
    /// </summary>
    /// <param name="results">The enumeration to consume the errors from.</param>
    public void AddErrors(IEnumerable<ValidationResult>? results);

    /// <summary>
    /// Adds any validation errors carried by the <see cref="ValidateOptionsResult"/> instance to this instance.
    /// </summary>
    /// <param name="result">The instance to consume the errors from.</param>
    public void AddErrors(ValidateOptionsResult? result);

    /// <summary>
    /// Builds <see cref="ValidateOptionsResult"/> based on provided data.
    /// </summary>
    /// <returns>New instance of <see cref="ValidateOptionsResult"/>.</returns>
    public ValidateOptionsResult Build();
}

API Usage

internal sealed class HttpStandardResilienceOptionsCustomValidator : IValidateOptions<HttpStandardResilienceOptions>
{
    private const int CircuitBreakerTimeoutMultiplier = 2;

    public ValidateOptionsResult Validate(string name, HttpStandardResilienceOptions options)
    {
        var builder = ValidateOptionsResultBuilder.Create();

        if (options.AttemptTimeoutOptions.TimeoutInterval > options.TotalRequestTimeoutOptions.TimeoutInterval)
        {
            builder.AddError($"Total request timeout policy must have a greater timeout than the attempt timeout policy. " +
                $"Total Request Timeout: {options.TotalRequestTimeoutOptions.TimeoutInterval.TotalSeconds}s, " +
                $"Attempt Timeout: {options.AttemptTimeoutOptions.TimeoutInterval.TotalSeconds}s");
        }

        if (options.CircuitBreakerOptions.SamplingDuration < TimeSpan.FromMilliseconds(options.AttemptTimeoutOptions.TimeoutInterval.TotalMilliseconds * CircuitBreakerTimeoutMultiplier))
        {
            builder.AddError("The sampling duration of circuit breaker policy needs to be at least double of " +
                $"an attempt timeout policy’s timeout interval, in order to be effective. " +
                $"Sampling Duration: {options.CircuitBreakerOptions.SamplingDuration.TotalSeconds}s," +
                $"Attempt Timeout: {options.AttemptTimeoutOptions.TimeoutInterval.TotalSeconds}s");
        }

        if (options.RetryOptions.RetryCount != RetryPolicyOptions.InfiniteRetry)
        {
            TimeSpan retrySum = options.RetryOptions.GetRetryPolicyDelaySum();

            if (retrySum > options.TotalRequestTimeoutOptions.TimeoutInterval)
            {
                builder.AddError($"The cumulative delay of the retry policy cannot be larger than total request timeout policy interval. " +
                $"Cumulative Delay: {retrySum.TotalSeconds}s," +
                $"Total Request Timeout: {options.TotalRequestTimeoutOptions.TimeoutInterval.TotalSeconds}s");
            }
        }

        return builder.Build();
    }
}

Alternative Designs

No response

Risks

No response

Author:	geeknoid
Assignees:	-
Labels:	api-suggestion, untriaged, area-Extensions-Options
Milestone:	8.0.0

[tarekgh]

@geeknoid just wondering, can't use List<string> to carry all errors all the way and then call ValidateOptionsResult.Fail at the end of the validation?

[ericstj]

Looks like this is effectively a helper class for the source-generator #85475 likely to avoid the source-generator emitting boilerplate "framework-like" code into the user assembly.

[tarekgh]

Could be. I am still wondering how much we are saving here. I mean instead of calling ValidateOptionsResultBuilder.AddError would just call LiSt<string>.Add(...). And instead of calling ValidateOptionsResultBuilder.Build will call ValidateOptionsResult.Fail. I may be missing something but wondering what we are gaining here more than just exposing a dedicated helper for this scenario.

[geeknoid]

The value in the abstraction is that it leads to higher quality diagnostics by standardizing some of the behavior. If you look at the overloads, you'll see it's more than merely a list. In particular, the overload that takes a property name is important. Otherwise, you get messages like "invalid value" as opposed to a more useful "property XXX has an invalid value". The other overloads provide other forms of composition which are handled in a canonical way.

Here is the implementation we've been using for the proposed API.

public readonly struct ValidateOptionsResultBuilder
{
    internal const string SeparatorString = "; ";

    private readonly List<string> _errors = new();

    public ValidateOptionsResultBuilder(IEnumerable<string>? errors)
    {
        if (errors != null)
        {
            _errors.AddRange(errors);
        }
    }

    public static ValidateOptionsResultBuilder Create() => new(null);

    public void AddError(string error) => _errors.Add(Throws.IfNullOrWhitespace(error));

    public void AddError(string propertyName, string error)
    {
        _ = Throws.IfNullOrWhitespace(propertyName);
        _ = Throws.IfNullOrWhitespace(error);

        _errors.Add($"Property {propertyName}: {error}");
    }

    public void AddError(ValidationResult? result)
    {
        if (result?.ErrorMessage != null)
        {
            _errors.Add(result.ErrorMessage);
        }
    }

    public void AddErrors(IEnumerable<ValidationResult>? results)
    {
        if (results != null)
        {
            foreach (var result in results)
            {
                AddError(result);
            }
        }
    }

    public void AddErrors(ValidateOptionsResult? result)
    {
        if (result != null && result.Failed)
        {
#if NETCOREAPP3_1_OR_GREATER
            foreach (var failure in result.Failures)
            {
                _errors.Add(failure);
            }
#else
            _errors.Add(result.FailureMessage);
#endif
        }
    }

    public ValidateOptionsResult Build()
    {
        if (_errors.Count == 0)
        {
            return ValidateOptionsResult.Success;
        }

#if NETCOREAPP3_1_OR_GREATER
        return ValidateOptionsResult.Fail(_errors);
#else
        return ValidateOptionsResult.Fail(string.Join(SeparatorString, _errors));
#endif
    }
}

[tarekgh]

I see the goodness of abstraction. I am wondering if we can add AddErrors(...) to ValidateOptionsResult instead of introducing a builder. @davidfowl any idea if changing ValidateOptionsResult to be mutable cause any problem? I know we have Skip and Success static properties which need to return Immutable objects, but we can handle that internally. My idea is just having the users deal with one simple type instead of having a builder and result objects.

[geeknoid]

IMO the shape of ValidateOptiosnResult isn't great for making it mutable. It's designed as an immutable object and the fact static instances are used somewhat reinforces that.

[tarekgh]

@geeknoid collecting some feedback and discussing this proposal, it is suggested to better have the builder for the ValidationResult instead of ValidateOptionsResult because ValidationResult is used in wider scenarios. Also, we suggest having a factory method to create a ValidateOptionsResult object from ValidationResult. This will be good enough to address building ValidateOptionsResult too. Please let me know what you think.

namespace System.ComponentModel.DataAnnotations
{
    /// <summary>
    /// A helper class to build an ValidationResult object which allows multiple error messages.
    /// The error messages in the resulted ValidationResult object will be concatenated with the `;` separator
    /// </summary>
    public readonly struct ValidationResultBuilder
    {
        /// <summary>
        /// Add the list of error messages to the builder.
        /// </summary>
        /// <param name="errors">The list of the error messages</param>
        public ValidationResultBuilder(IEnumerable<string>? errors) { }

        /// <summary>
        /// Create empty ValidationResultBuilder object
        /// </summary>
        /// <returns>The created ValidationResultBuilder object</returns>
        public static ValidationResultBuilder Create() => new(null);

        /// <summary>
        /// Add error message to the builder
        /// </summary>
        /// <param name="error">The error message to add to the build</param>
        public void AddError(string error) { }

        /// <summary>
        /// Add a new error message to the builder formated with the property name $"property {propertyName}: {error}"
        /// </summary>
        /// <param name="propertyName">The property name associated with the error message</param>
        /// <param name="error">The error message to add to the builder</param>
        public void AddError(string propertyName, string error) { }

        /// <summary>
        /// Add the error message and property names from the input ValidationResult object to the builder
        /// </summary>
        /// <remarks>
        /// If the input ValidationResult has property names, will concatenate the property names separated by ',' and then concatenate the result with the error message.
        /// </remarks>
        /// <param name="result">ValidationResult object to add its message and property names to the builder</param>
        public void AddError(ValidationResult? result) { }
        public void AddErrors(IEnumerable<ValidationResult>? results) { }
        public ValidationResult Build() { }
    }
}

namespace Microsoft.Extensions.Options
{
    public static class ValidateOptionsResultExtensions
    {
        public static ValidateOptionsResult ToValidateOptionsResult(this ValidationResult result);
    }
}

@jeffhandley @eerhardt if you have feedback on the proposal.

[geeknoid]

Could FromValidationResult be an extension method?

public static ValidateOptionsResult ToValidationResult(this ValidationResult result) 

Seems more natural.

[geeknoid]

Overall, this looks like it would work fine. I would prefer if the builder were a readonly struct just for the sake of reducing allocations in the happy path. When you make it a struct, you can eliminate most (maybe all, I don't remember our code anymore) allocations that occur when validating a bunch of models.

[tarekgh]

Thanks @geeknoid. I updated my proposal #77404 (comment) addressing both issues you suggested. If this version is good, I'll move it up to the issue description.

[jeffhandley]

I'm generally supportive of this. I worry a bit about baking English language semantics into the logic as that will make localization even more difficult than it is today. But modeling the ; separator, the $"property {propertyName}: {error}" format, and the , separator through APIs would make this much less usable.

Perhaps the only other approach to consider is one that allows the equivalent of a SelectMany type behavior for a validator to produce 0-N results (I've long regretted that we didn't incorporate that capability).

[tarekgh]

I think the current proposal can work with the localization too. The only piece that is not localized is the word property in the string $"property {propertyName}: {error}". We may omit this work and keep it as $"{propertyName}: {error}". But in general, I expect propertyName will be in English anyway, so having property word before it will not make a big problem. for ; and , we can provide these as the default separator and allow customizing it in the builder constructor.

[eerhardt]

But in general, I expect propertyName will be in English anyway

Why wouldn't it support

runtime/src/libraries/System.ComponentModel.Annotations/src/System/ComponentModel/DataAnnotations/DisplayAttribute.cs

Lines 9 to 17 in 252018c

	/// <summary>
	/// DisplayAttribute is a general-purpose attribute to specify user-visible globalizable strings for types and members.
	/// The string properties of this class can be used either as literals or as resource identifiers into a specified
	/// <see cref="ResourceType" />
	/// </summary>
	[AttributeUsage(
	AttributeTargets.Property | AttributeTargets.Field | AttributeTargets.Parameter | AttributeTargets.Method | AttributeTargets.Class,
	AllowMultiple = false)]
	public sealed class DisplayAttribute : Attribute

[tarekgh]

Talked offline with @eerhardt and after some discussion, we'll go back to the original proposal which is suggested by @geeknoid. The reason is ValidationResult seems designed to carry one failure through one error message. Having a builder for ValidationResult will start creating a notion carrying multiple errors which can cause confusion for users who are currently carrying multiple ValidationResult to represent multiple errors.

By that, we'll proceed with the original proposal.

[jeffhandley]

Sounds good, @tarekgh. I'd like to separately explore having validators that can product multiple results.

[bartonjs]

• As the whole purpose of this type is to be mutated, it should be a class, not a struct.
• Since it's a class, Create() can move to .ctor().
• Pedantic: The doc comments could use some improved wording and grammar.
• AddError(string error) and AddError(string propertyName, string error) are not overloaded correctly (the string in args[0] changes meaning). We rewrote this to AddError(string error, string? propertyName = null)
• There was a discussion of Build() vs ToValidateOptionsResult(), and Build() won as that's the more prevalent pattern in Microsoft.Extensions.*
• The constructor that accepts an IEnumerable<string> was removed. If there's a demonstrated need later it may reappear as an AddErrors method.
• We left AddError(string) as AddError, renamed the rest to AddResult(s)
• We added a Clear() for good measure
• The nullability of public void AddResults(ValidateOptionsResult result) was updated to indiciate that null ValidationOptionsResult values are not expected.

namespace Microsoft.Extensions.Options;

/// <summary>
/// Builds <see cref="ValidateOptionsResult"/> with support for multiple error messages.
/// </summary>
[DebuggerDisplay("{_errors.Count} errors")]
public class ValidateOptionsResultBuilder
{
    /// <summary>
    /// Creates new instance of the <see cref="ValidateOptionsResultBuilder"/> class.
    /// </summary>
    public ValidateOptionsResultBuilder();

    /// <summary>
    /// Adds validation error.
    /// </summary>
    /// <param name="error">Content of error message.</param>
    /// <param name="propertyName">THe property in the option object which contains an error.</param>
    public void AddError(string error, string? propertyName = null);

    /// <summary>
    /// Adds any validation error carried by the <see cref="ValidationResult"/> instance to this instance.
    /// </summary>
    /// <param name="result">The instance to consume the errors from.</param>
    public void AddResult(ValidationResult? result);

    /// <summary>
    /// Adds any validation error carried by the enumeration of <see cref="ValidationResult"/> instances to this instance.
    /// </summary>
    /// <param name="results">The enumeration to consume the errors from.</param>
    public void AddResults(IEnumerable<ValidationResult?>? results);

    /// <summary>
    /// Adds any validation errors carried by the <see cref="ValidateOptionsResult"/> instance to this instance.
    /// </summary>
    /// <param name="result">The instance to consume the errors from.</param>
    public void AddResults(ValidateOptionsResult result);

    /// <summary>
    /// Builds <see cref="ValidateOptionsResult"/> based on provided data.
    /// </summary>
    /// <returns>New instance of <see cref="ValidateOptionsResult"/>.</returns>
    public ValidateOptionsResult Build();

    public void Clear();
}