dotnet / runtime

.NET is a cross-platform runtime for cloud, mobile, desktop, and IoT apps.
https://docs.microsoft.com/dotnet/core/
MIT License
15.27k stars 4.73k forks source link

[API Proposal]: Add a RangeAttribute constructor supporting arbitrary IComparable ranges. #82526

Open eiriktsarpalis opened 1 year ago

eiriktsarpalis commented 1 year ago

Background and motivation

The RangeAttribute currently supports int and double ranges out of the box using dedicated constructor overloads, however any other range necessitates using this overload requiring the operand type as well as string formatted representations of the lower and upper bounds. This only works for types supported by the TypeConverter class expressing limits in strings forces concerns around culture-sensitive formatting:

public class TimeSpanRangeAttribute : RangeAttribute
{
    public TimeSpanRangeAttribute(int minMilliseconds, int maxMilliseconds)
        : base(type: typeof(TimeSpan),
               minimum: TimeSpan.FromMilliseconds(minMilliseconds).ToString("c"),
               maximum: TimeSpan.FromMilliseconds(maxMilliseconds).ToString("c"))
    {
        ParseLimitsInInvariantCulture = true;
    }
}

I've been working on a prototype that adds a protected constructor which accepts arbitrary IComparable bounds directly.

API Proposal

namespace System.ComponentModel.DataAnnotations;

public partial class RangeAttribute
{
    public RangeAttribute(int minimum, int maximum);
    public RangeAttribute(double minimum, double maximum);
+   public RangeAttribute(IComparable minimum, IComparable maximum);

    [RequiresUnreferencedCode("Generic TypeConverters may require the generic types to be annotated.")]
    public RangeAttribute(Type type, string minimum, string maximum);
}

API Usage

The above example is now rendered as follows:

public class TimeSpanMillisecondRangeAttribute : RangeAttribute
{
    public TimeSpanMillisecondRangeAttribute(int minimumMs, int maximumMs)
        : base(TimeSpan.FromMilliseconds(minimumMs), TimeSpan.FromMilliseconds(maximumMs))
    { }
}

Alternative Designs

Risks

No response

cc @geeknoid @jeffhandley

ghost commented 1 year ago

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

Issue Details
### Background and motivation The `RangeAttribute` currently supports `int` and `double` ranges out of the box using dedicated constructor overloads, however any other range necessitates using [this overload](https://learn.microsoft.com/en-us/dotnet/api/system.componentmodel.dataannotations.rangeattribute.-ctor?view=net-7.0#system-componentmodel-dataannotations-rangeattribute-ctor(system-type-system-string-system-string)) requiring the operand type as well as string formatted representations of the lower and upper bounds. This only works for types supported by the `TypeConverter` class expressing limits in strings forces concerns around culture-sensitive formatting: ```C# public class TimeSpanRangeAttribute : RangeAttribute { public TimeSpanRangeAttribute(int minMilliseconds, int maxMilliseconds) : base(type: typeof(TimeSpan), minimum: TimeSpan.FromMilliseconds(minMilliseconds).ToString("c"), maximum: TimeSpan.FromMilliseconds(maxMilliseconds).ToString("c")) { ParseLimitsInInvariantCulture = true; } } ``` I've been working on a [prototype](https://github.com/eiriktsarpalis/runtime/commit/646b388fd80b418fb96d4ffad63361a1051f3b76) that adds a protected constructor which accepts arbitrary `IComparable` bounds directly. ### API Proposal ```diff namespace System.ComponentModel.DataAnnotations; public partial class RangeAttribute { public RangeAttribute(int minimum, int maximum); public RangeAttribute(double minimum, double maximum); + protected RangeAttribute(IComparable minimum, IComparable maximum); [RequiresUnreferencedCode("Generic TypeConverters may require the generic types to be annotated.")] public RangeAttribute(Type type, string minimum, string maximum); } ``` ### API Usage The above example is now rendered as follows: ```csharp public class TimeSpanMillisecondRangeAttribute : RangeAttribute { public TimeSpanMillisecondRangeAttribute(int minimumMs, int maximumMs) : base(TimeSpan.FromMilliseconds(minimumMs), TimeSpan.FromMilliseconds(maximumMs)) { } } ``` ### Alternative Designs * I marked the proposed constructor as `protected`, since marking it public would add OOTB support for things like string or long ranges using their built-in IComparable implementation. Marking it `protected` emphasizes its use as an extensibility point for user-defined derived validation attributes. If people thinks it is useful, we could mark as `public` instead. * Proposal leaves out `IComparer` support since the validator implementation is oriented around handling of `IComparable` values. ### Risks _No response_ cc @geeknoid @jeffhandley
Author: eiriktsarpalis
Assignees: -
Labels: `api-suggestion`, `area-System.Runtime`
Milestone: Future
ghost commented 1 year ago

Tagging subscribers to this area: @ajcvickers, @bricelam, @roji See info in area-owners.md if you want to be subscribed.

Issue Details
### Background and motivation The `RangeAttribute` currently supports `int` and `double` ranges out of the box using dedicated constructor overloads, however any other range necessitates using [this overload](https://learn.microsoft.com/en-us/dotnet/api/system.componentmodel.dataannotations.rangeattribute.-ctor?view=net-7.0#system-componentmodel-dataannotations-rangeattribute-ctor(system-type-system-string-system-string)) requiring the operand type as well as string formatted representations of the lower and upper bounds. This only works for types supported by the `TypeConverter` class expressing limits in strings forces concerns around culture-sensitive formatting: ```C# public class TimeSpanRangeAttribute : RangeAttribute { public TimeSpanRangeAttribute(int minMilliseconds, int maxMilliseconds) : base(type: typeof(TimeSpan), minimum: TimeSpan.FromMilliseconds(minMilliseconds).ToString("c"), maximum: TimeSpan.FromMilliseconds(maxMilliseconds).ToString("c")) { ParseLimitsInInvariantCulture = true; } } ``` I've been working on a [prototype](https://github.com/eiriktsarpalis/runtime/commit/646b388fd80b418fb96d4ffad63361a1051f3b76) that adds a protected constructor which accepts arbitrary `IComparable` bounds directly. ### API Proposal ```diff namespace System.ComponentModel.DataAnnotations; public partial class RangeAttribute { public RangeAttribute(int minimum, int maximum); public RangeAttribute(double minimum, double maximum); + protected RangeAttribute(IComparable minimum, IComparable maximum); [RequiresUnreferencedCode("Generic TypeConverters may require the generic types to be annotated.")] public RangeAttribute(Type type, string minimum, string maximum); } ``` ### API Usage The above example is now rendered as follows: ```csharp public class TimeSpanMillisecondRangeAttribute : RangeAttribute { public TimeSpanMillisecondRangeAttribute(int minimumMs, int maximumMs) : base(TimeSpan.FromMilliseconds(minimumMs), TimeSpan.FromMilliseconds(maximumMs)) { } } ``` ### Alternative Designs * I marked the proposed constructor as `protected`, since marking it public would add OOTB support for things like string or long ranges using their built-in IComparable implementation. Marking it `protected` emphasizes its use as an extensibility point for user-defined derived validation attributes. If people thinks it is useful, we could mark as `public` instead. * Proposal leaves out `IComparer` support since the validator implementation is oriented around handling of `IComparable` values. ### Risks _No response_ cc @geeknoid @jeffhandley
Author: eiriktsarpalis
Assignees: -
Labels: `api-suggestion`, `area-System.ComponentModel.DataAnnotations`, `area-System.Runtime`
Milestone: Future
jeffhandley commented 1 year ago

@eiriktsarpalis I'm inclined to pull this into .NET 8 to group it with the other DataAnnotations enhancements we're making during the release. @geeknoid Would you be able to use this if it was in .NET 8?

/cc @dotnet/area-system-componentmodel-dataannotations

eiriktsarpalis commented 1 year ago

Sure, marking as ready for review.

geeknoid commented 1 year ago

@jeffhandley Yes, I think we could use this in .NET 8

terrajobst commented 1 year ago
namespace System.ComponentModel.DataAnnotations;

public partial class RangeAttribute
{
    // Existing:
    // public RangeAttribute(int minimum, int maximum);
    // public RangeAttribute(double minimum, double maximum);
    private protected RangeAttribute(Type type, IComparable minimum, IComparable maximum);
}

public abstract class RangeAttribute<T> : RangeAttribute
    where T: IComparable
{
    protected RangeAttribute(T minimum, T maximum);
}

// Example usage:
// 
// public class TimeSpanMillisecondRangeAttribute : RangeAttribute<TimeSpan>
// {
//     public TimeSpanMillisecondRangeAttribute(int minimumMs, int maximumMs)
//         : base(TimeSpan.FromMilliseconds(minimumMs),
//                TimeSpan.FromMilliseconds(maximumMs))
//     {        
//     }
// }
jeffhandley commented 1 year ago

We will not pursue that revised design in .NET 8. Moving to Future and removing the https://github.com/dotnet/runtime/labels/blocking label.

eerhardt commented 1 year ago

Could we also use the IParsable interface and create:

public sealed class ParsedRangeAttribute<T> : RangeAttribute<T> // this name could use some work
    where T : IComparable, IParsable<T>
{
    public ParsedRangeAttribute(string minimum, string maximum);
}

Then users wouldn't need to create their own derived attribute classes to use types that are IParsable, like TimeSpan.

Regarding naming, I think it would make more sense to use the Range<T> attribute name for the sealed class - the one that would be used in all the callsites. So maybe it would make sense to say:

public abstract class ComparableRangeAttribute<T> : RangeAttribute
    where T : IComparable
{
    protected ComparableRangeAttribute(T minimum, T maximum);
}

public sealed class RangeAttribute<T> : ComparableRangeAttribute<T>
    where T : IComparable, IParsable<T>
{
    public RangeAttribute(string minimum, string maximum);
}

And then usages would look like:

public class ResilienceStrategyOptions
{
    [Range<TimeSpan>("00:00:00", "1.00:00:00")]
    public TimeSpan? MaxDelay { get; set; }
}