Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

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

Open
eiriktsarpalis opened this issue Feb 23, 2023 · 8 comments
Assignees
Labels
api-needs-work API needs work before it is approved, it is NOT ready for implementation area-System.ComponentModel.DataAnnotations partner-impact This issue impacts a partner who needs to be kept updated
Milestone

Comments

@eiriktsarpalis
Copy link
Member

eiriktsarpalis commented Feb 23, 2023

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

  • 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

@eiriktsarpalis eiriktsarpalis added the api-suggestion Early API idea and discussion, it is NOT ready for implementation label Feb 23, 2023
@ghost ghost added the untriaged New issue has not been triaged by the area owner label Feb 23, 2023
@eiriktsarpalis eiriktsarpalis removed the untriaged New issue has not been triaged by the area owner label Feb 23, 2023
@eiriktsarpalis eiriktsarpalis added this to the Future milestone Feb 23, 2023
@ghost
Copy link

ghost commented Feb 24, 2023

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 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);
+   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:

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
Copy link

ghost commented Feb 24, 2023

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 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);
+   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:

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
Copy link
Member

@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 eiriktsarpalis added api-ready-for-review API is ready for review, it is NOT ready for implementation and removed api-suggestion Early API idea and discussion, it is NOT ready for implementation labels Mar 27, 2023
@eiriktsarpalis
Copy link
Member Author

Sure, marking as ready for review.

@eiriktsarpalis eiriktsarpalis modified the milestones: Future, 8.0.0 Mar 27, 2023
@eiriktsarpalis eiriktsarpalis self-assigned this Mar 27, 2023
@geeknoid
Copy link
Member

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

@tarekgh tarekgh added the partner-impact This issue impacts a partner who needs to be kept updated label Mar 27, 2023
@eiriktsarpalis eiriktsarpalis added the blocking Marks issues that we want to fast track in order to unblock other important work label Apr 24, 2023
@terrajobst
Copy link
Member

  • We suggest to provide a RangeAttribute<T> that is abstract and constrains the parameters to have the same type.
  • We should consider a design where RangeAttribute<T> doesn't extend RangeAttribute such that we can constrain T to IComparable<T> rather than IComparable.
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))
//     {        
//     }
// }

@terrajobst terrajobst added api-needs-work API needs work before it is approved, it is NOT ready for implementation and removed api-ready-for-review API is ready for review, it is NOT ready for implementation labels Apr 25, 2023
@jeffhandley
Copy link
Member

We will not pursue that revised design in .NET 8. Moving to Future and removing the blocking Marks issues that we want to fast track in order to unblock other important work label.

@jeffhandley jeffhandley modified the milestones: 8.0.0, Future May 12, 2023
@jeffhandley jeffhandley removed the blocking Marks issues that we want to fast track in order to unblock other important work label May 12, 2023
@eerhardt
Copy link
Member

eerhardt commented Oct 30, 2023

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; }
}

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
api-needs-work API needs work before it is approved, it is NOT ready for implementation area-System.ComponentModel.DataAnnotations partner-impact This issue impacts a partner who needs to be kept updated
Projects
None yet
Development

No branches or pull requests

7 participants