-
Notifications
You must be signed in to change notification settings - Fork 4.7k
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]: Expand data validation attributes #77402
Comments
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. |
Tagging subscribers to this area: @dotnet/area-system-componentmodel Issue DetailsBackground and motivationOur project features hundreds of different option types against which we perform data validation operations. We've introduced a number of new data validation attributes which capture essential scenarios in our code. As these are general-purpose in nature, it would be great to add those to the core set of supported attributes. API Proposalnamespace System.ComponentModel.Annotations;
/// <summary>
/// Marks a property to be validated as <see href="https://en.wikipedia.org/wiki/Base64">Base64</see> string.
/// </summary>
[AttributeUsage(AttributeTargets.Property | AttributeTargets.Field | AttributeTargets.Parameter, AllowMultiple = false)]
public sealed class Base64StringAttribute : ValidationAttribute
{
}
/// <summary>
/// Provides exclusive boundary validation for <see cref="long"/> or <see cref="double"/> values.
/// </summary>
[AttributeUsage(AttributeTargets.Property | AttributeTargets.Field | AttributeTargets.Parameter, AllowMultiple = false)]
public sealed class ExclusiveRangeAttribute : ValidationAttribute
{
/// <summary>
/// Gets the minimum value for the range.
/// </summary>
public object Minimum { get; }
/// <summary>
/// Gets the maximum value for the range.
/// </summary>
public object Maximum { get; }
/// <summary>
/// Initializes a new instance of the <see cref="ExclusiveRangeAttribute"/> class.
/// </summary>
/// <param name="minimum">The minimum value, exclusive.</param>
/// <param name="maximum">The maximum value, exclusive.</param>
public ExclusiveRangeAttribute(int minimum, int maximum);
/// <summary>
/// Initializes a new instance of the <see cref="ExclusiveRangeAttribute"/> class.
/// </summary>
/// <param name="minimum">The minimum value, exclusive.</param>
/// <param name="maximum">The maximum value, exclusive.</param>
public ExclusiveRangeAttribute(double minimum, double maximum);
}
/// <summary>
/// Specifies the minimum length of any <see cref="IEnumerable"/> or <see cref="string"/> objects.
/// </summary>
/// <remarks>
/// The standard <see cref="MinLengthAttribute" /> supports only non generic <see cref="Array"/> or <see cref="string"/> typed objects
/// on .Net Framework, while <see cref="ICollection{T}"/> type is supported only on .Net Core.
/// See issue here <see href="https://github.com/dotnet/runtime/issues/23288"/>.
/// This attribute aims to allow validation of all these objects in a consistent manner across target frameworks.
/// </remarks>
[AttributeUsage(AttributeTargets.Property | AttributeTargets.Field | AttributeTargets.Parameter, AllowMultiple = false)]
public sealed class LengthAttribute : ValidationAttribute
{
/// <summary>
/// Gets the minimum allowed length of the collection or string.
/// </summary>
public int MinimumLength { get; }
/// <summary>
/// Gets the maximum allowed length of the collection or string.
/// </summary>
public int? MaximumLength { get; }
/// <summary>
/// Gets or sets a value indicating whether the length validation should exclude the <see cref="MinimumLength"/> and <see cref="MaximumLength"/> values.
/// </summary>
/// <remarks>
/// By default the property is set to <c>false</c>.
/// </remarks>
public bool Exclusive { get; set; }
/// <summary>
/// Initializes a new instance of the <see cref="LengthAttribute"/> class.
/// </summary>
/// <param name="minimumLength">
/// The minimum allowable length of array/string data.
/// Value must be greater than or equal to zero.
/// </param>
public LengthAttribute(int minimumLength);
/// <summary>
/// Initializes a new instance of the <see cref="LengthAttribute"/> class.
/// </summary>
/// <param name="minimumLength">
/// The minimum allowable length of array/string data.
/// Value must be greater than or equal to zero.
/// </param>
/// <param name="maximumLength">
/// The maximum allowable length of array/string data.
/// Value must be greater than or equal to zero.
/// </param>
public LengthAttribute(int minimumLength, int maximumLength);
}
/// <summary>
/// Provides a way of requiring a value that is not default(T) for a property of value type.
/// </summary>
/// <remarks>Only to be used with value type properties.</remarks>
[AttributeUsage(AttributeTargets.Property | AttributeTargets.Field, AllowMultiple = false)]
public sealed class NonDefaultValueRequiredAttribute : ValidationAttribute
{
}
/// <summary>
/// Provides a way of not accepting specific values for a property/field/parameter.
/// </summary>
[AttributeUsage(AttributeTargets.Property | AttributeTargets.Field | AttributeTargets.Parameter, AllowMultiple = false)]
public sealed class RejectValuesAttribute : ValidationAttribute
{
/// <summary>
/// Gets the values which are not accepted.
/// </summary>
public ICollection<object> RejectedValues { get; }
/// <summary>
/// Initializes a new instance of the <see cref="RejectValuesAttribute"/> class.
/// </summary>
/// <param name="rejectedValues">Values which should not be accepted.</param>
public RejectValuesAttribute(params object[] rejectedValues);
}
/// <summary>
/// Provides boundary validation for <see cref="TimeSpan"/>.
/// </summary>
[AttributeUsage(AttributeTargets.Property | AttributeTargets.Field | AttributeTargets.Parameter, AllowMultiple = false)]
public sealed class TimeSpanAttribute : ValidationAttribute
{
/// <summary>
/// Gets the lower bound for time span.
/// </summary>
public TimeSpan Minimum { get; }
/// <summary>
/// Gets the upper bound for time span.
/// </summary>
public TimeSpan? Maximum { get; }
/// <summary>
/// Gets or sets a value indicating whether the time span validation should exclude the minimum and maximum values.
/// </summary>
/// <remarks>
/// By default the property is set to <c>false</c>.
/// </remarks>
public bool Exclusive { get; set; }
/// <summary>
/// Initializes a new instance of the <see cref="TimeSpanAttribute"/> class.
/// </summary>
/// <param name="minMs">Minimum in milliseconds.</param>
public TimeSpanAttribute(int minMs);
/// <summary>
/// Initializes a new instance of the <see cref="TimeSpanAttribute"/> class.
/// </summary>
/// <param name="minMs">Minimum in milliseconds.</param>
/// <param name="maxMs">Maximum in milliseconds.</param>
public TimeSpanAttribute(int minMs, int maxMs);
/// <summary>
/// Initializes a new instance of the <see cref="TimeSpanAttribute"/> class.
/// </summary>
/// <param name="min">Minimum represented as time span string.</param>
public TimeSpanAttribute(string min);
/// <summary>
/// Initializes a new instance of the <see cref="TimeSpanAttribute"/> class.
/// </summary>
/// <param name="min">Minimum represented as time span string.</param>
/// <param name="max">Maximum represented as time span string.</param>
public TimeSpanAttribute(string min, string max);
} API Usageclass Options
{
// require a valid base-64 string
[Base64String]
public string EncodedBinaryData { get; set; } = string.Empty;
// a value between specific exclusive boundaries
[ExclusiveRange(0, 1.0)]
public double BetweenZeroAndOne { get; set; } = .5;
// 1 to 5 items
[Length(1, 5)]
public int[] ArrayOfStuff { get; set; }
// anything but these
[RejectValues("Red")]
public string Color { get; set; }
// 10ms to 100ms
[TimeSpan(10, 100)]
public TimeSpan Timeout { get; set; }
} Alternative DesignsNo response RisksNo response
|
Tagging subscribers to this area: @ajcvickers, @bricelam, @roji Issue DetailsBackground and motivationOur project features hundreds of different option types against which we perform data validation operations. We've introduced a number of new data validation attributes which capture essential scenarios in our code. As these are general-purpose in nature, it would be great to add those to the core set of supported attributes. API Proposalnamespace System.ComponentModel.Annotations;
/// <summary>
/// Marks a property to be validated as <see href="https://en.wikipedia.org/wiki/Base64">Base64</see> string.
/// </summary>
[AttributeUsage(AttributeTargets.Property | AttributeTargets.Field | AttributeTargets.Parameter, AllowMultiple = false)]
public sealed class Base64StringAttribute : ValidationAttribute
{
}
/// <summary>
/// Provides exclusive boundary validation for <see cref="long"/> or <see cref="double"/> values.
/// </summary>
[AttributeUsage(AttributeTargets.Property | AttributeTargets.Field | AttributeTargets.Parameter, AllowMultiple = false)]
public sealed class ExclusiveRangeAttribute : ValidationAttribute
{
/// <summary>
/// Gets the minimum value for the range.
/// </summary>
public object Minimum { get; }
/// <summary>
/// Gets the maximum value for the range.
/// </summary>
public object Maximum { get; }
/// <summary>
/// Initializes a new instance of the <see cref="ExclusiveRangeAttribute"/> class.
/// </summary>
/// <param name="minimum">The minimum value, exclusive.</param>
/// <param name="maximum">The maximum value, exclusive.</param>
public ExclusiveRangeAttribute(int minimum, int maximum);
/// <summary>
/// Initializes a new instance of the <see cref="ExclusiveRangeAttribute"/> class.
/// </summary>
/// <param name="minimum">The minimum value, exclusive.</param>
/// <param name="maximum">The maximum value, exclusive.</param>
public ExclusiveRangeAttribute(double minimum, double maximum);
}
/// <summary>
/// Specifies the minimum length of any <see cref="IEnumerable"/> or <see cref="string"/> objects.
/// </summary>
/// <remarks>
/// The standard <see cref="MinLengthAttribute" /> supports only non generic <see cref="Array"/> or <see cref="string"/> typed objects
/// on .Net Framework, while <see cref="ICollection{T}"/> type is supported only on .Net Core.
/// See issue here <see href="https://github.com/dotnet/runtime/issues/23288"/>.
/// This attribute aims to allow validation of all these objects in a consistent manner across target frameworks.
/// </remarks>
[AttributeUsage(AttributeTargets.Property | AttributeTargets.Field | AttributeTargets.Parameter, AllowMultiple = false)]
public sealed class LengthAttribute : ValidationAttribute
{
/// <summary>
/// Gets the minimum allowed length of the collection or string.
/// </summary>
public int MinimumLength { get; }
/// <summary>
/// Gets the maximum allowed length of the collection or string.
/// </summary>
public int? MaximumLength { get; }
/// <summary>
/// Gets or sets a value indicating whether the length validation should exclude the <see cref="MinimumLength"/> and <see cref="MaximumLength"/> values.
/// </summary>
/// <remarks>
/// By default the property is set to <c>false</c>.
/// </remarks>
public bool Exclusive { get; set; }
/// <summary>
/// Initializes a new instance of the <see cref="LengthAttribute"/> class.
/// </summary>
/// <param name="minimumLength">
/// The minimum allowable length of array/string data.
/// Value must be greater than or equal to zero.
/// </param>
public LengthAttribute(int minimumLength);
/// <summary>
/// Initializes a new instance of the <see cref="LengthAttribute"/> class.
/// </summary>
/// <param name="minimumLength">
/// The minimum allowable length of array/string data.
/// Value must be greater than or equal to zero.
/// </param>
/// <param name="maximumLength">
/// The maximum allowable length of array/string data.
/// Value must be greater than or equal to zero.
/// </param>
public LengthAttribute(int minimumLength, int maximumLength);
}
/// <summary>
/// Provides a way of requiring a value that is not default(T) for a property of value type.
/// </summary>
/// <remarks>Only to be used with value type properties.</remarks>
[AttributeUsage(AttributeTargets.Property | AttributeTargets.Field, AllowMultiple = false)]
public sealed class NonDefaultValueRequiredAttribute : ValidationAttribute
{
}
/// <summary>
/// Provides a way of not accepting specific values for a property/field/parameter.
/// </summary>
[AttributeUsage(AttributeTargets.Property | AttributeTargets.Field | AttributeTargets.Parameter, AllowMultiple = false)]
public sealed class RejectValuesAttribute : ValidationAttribute
{
/// <summary>
/// Gets the values which are not accepted.
/// </summary>
public ICollection<object> RejectedValues { get; }
/// <summary>
/// Initializes a new instance of the <see cref="RejectValuesAttribute"/> class.
/// </summary>
/// <param name="rejectedValues">Values which should not be accepted.</param>
public RejectValuesAttribute(params object[] rejectedValues);
}
/// <summary>
/// Provides boundary validation for <see cref="TimeSpan"/>.
/// </summary>
[AttributeUsage(AttributeTargets.Property | AttributeTargets.Field | AttributeTargets.Parameter, AllowMultiple = false)]
public sealed class TimeSpanAttribute : ValidationAttribute
{
/// <summary>
/// Gets the lower bound for time span.
/// </summary>
public TimeSpan Minimum { get; }
/// <summary>
/// Gets the upper bound for time span.
/// </summary>
public TimeSpan? Maximum { get; }
/// <summary>
/// Gets or sets a value indicating whether the time span validation should exclude the minimum and maximum values.
/// </summary>
/// <remarks>
/// By default the property is set to <c>false</c>.
/// </remarks>
public bool Exclusive { get; set; }
/// <summary>
/// Initializes a new instance of the <see cref="TimeSpanAttribute"/> class.
/// </summary>
/// <param name="minMs">Minimum in milliseconds.</param>
public TimeSpanAttribute(int minMs);
/// <summary>
/// Initializes a new instance of the <see cref="TimeSpanAttribute"/> class.
/// </summary>
/// <param name="minMs">Minimum in milliseconds.</param>
/// <param name="maxMs">Maximum in milliseconds.</param>
public TimeSpanAttribute(int minMs, int maxMs);
/// <summary>
/// Initializes a new instance of the <see cref="TimeSpanAttribute"/> class.
/// </summary>
/// <param name="min">Minimum represented as time span string.</param>
public TimeSpanAttribute(string min);
/// <summary>
/// Initializes a new instance of the <see cref="TimeSpanAttribute"/> class.
/// </summary>
/// <param name="min">Minimum represented as time span string.</param>
/// <param name="max">Maximum represented as time span string.</param>
public TimeSpanAttribute(string min, string max);
} API Usageclass Options
{
// require a valid base-64 string
[Base64String]
public string EncodedBinaryData { get; set; } = string.Empty;
// a value between specific exclusive boundaries
[ExclusiveRange(0, 1.0)]
public double BetweenZeroAndOne { get; set; } = .5;
// 1 to 5 items
[Length(1, 5)]
public int[] ArrayOfStuff { get; set; }
// anything but these
[RejectValues("Red")]
public string Color { get; set; }
// 10ms to 100ms
[TimeSpan(10, 100)]
public TimeSpan Timeout { get; set; }
} Alternative DesignsNo response RisksNo response
|
@geeknoid can you include the code samples that demonstrate where validation would happen? |
I see, it looks like that calls back into the validation infrastructure in System.ComponentModel.Annotations - so this isn't proposing any new consumers/entry-points for this validation - just some new attributes that use the existing mechanisms. |
Validation attributes are not something we plan to invest any time in going forward. It is unlikely we will accept any changes in this area. |
@ajcvickers can you say more -- how did we end up with that perspective, was it resources, usefulness, or lack of adoption? |
Also, can you say more about the cost of additions like these? What are the costs beyond the attribute itself (including the validation method)? |
@danmoseley Attributes in
Use in validation has a few problems:
The problems are worse when the attributes are consumed directly:
It would be great for .NET to have a modern validation system that does not suffer from these issues. However, that has never been high enough priority to be funded, and last time I checked nobody is really interested in taking on and owning such a thing, since experience has shown the existing system to be such as can of worms. Beyond that, it doesn't seem wise to further add to the current system, beyond maintaining behavior that already exists. |
Btw, the only reason my team owns this area is because we added some new attributes for legacy EF over 10 years ago. We learned our lesson and never did that again! :-) |
Thanks for writing all that up - I think it will be useful in future. @geeknoid I assume functionally everything works fine if you keep these attributes where you have them today. |
I know the libraries folks are currently adding README.md to each library indicating the kinds of changes they take. This one could probably benefit from one. @jeffhandley could you maybe point @ajcvickers at an example in the recommended format? |
@danmoseley Unfortunately, no this is somewhat of a deal-breaker for the project we're on. As we want to move many of our components into the core libraries, these components depend on all these attributes. If the attributes don't make it to the core of the platform, then we would need to rewrite our components to not depend on the attributes, which would be a lot of busy work leading to likely lower quality experience of our customers. |
@ajcvickers So if I'm reading your response correctly, you're basically saying that the current attribute-based validation model is broken in many ways, and you don't want to invest in expanding its feature set? For our project, we have a lot of option types, and they are validated systematically using a code generator option validator, which leverages all the validation attributes in a reliable and predictable way. It has worked very well for us, and leads to high-quality state coming into our code, and high-quality errors being presented to the user. |
Yes.
That's great for a system used in a well-defined way by a private group of consumers. It's a different thing to add new attributes to the BCL where they will be available to all consumers of .NET who will have many different expectations and come up with many different ways of consuming the code. And especially since there is history around how this code should be consumed that goes beyond what you are doing. |
Yes, we've been adding them to lots of libraries the last couple weeks. The XML libraries is a good example: #77401 |
IMO the validation namespace is already generic / non-UI specific and I wouldn't see any need to impose a scope limit to just UI now. I use data validation attributes in all kinds of applications (console, web desktop and mobile, services). Data validation isn't UI specific.
IMO I wouldn't restrict the scope of potential validators purely based on what they logically do, but rather judge them on a case by case basis on their effort required to introduce and their level of impact to customers. |
I'm marking the issue as |
It’s marked as blocking. For tomorrow, we were planning to review interop; is this urgent? If so, I’ll update the agenda and reserve 10-15 min for it. Otherwise it’s Tuesday next week. |
The sooner the better, but if we can't get to it for another week, that's ok. |
Then I suggest we push it to next week so we don’t have to time box too much. My gut feel is that interop stuff will take a while, probably even two rounds of reviews. |
namespace System.ComponentModel.Annotations;
public partial class RangeAttribute : ValidationAttribute
{
public bool MinimumIsExclusive { get; set; } = false;
public bool MaximumIsExclusive { get; set; } = false;
}
public partial class RequiredAttribute : ValidationAttribute
{
// Fail validation for structs that equal the default value for the type
public bool DisallowAllDefaultValues { get; set; } = false;
}
// Validates that the specified string uses Base64 encoding
[AttributeUsage(AttributeTargets.Property | AttributeTargets.Field | AttributeTargets.Parameter, AllowMultiple = false)]
public class Base64StringAttribute : ValidationAttribute
{
}
// Specifies length ranges for string/IEnumerable
[AttributeUsage(AttributeTargets.Property | AttributeTargets.Field | AttributeTargets.Parameter, AllowMultiple = false)]
public class LengthAttribute : ValidationAttribute
{
public LengthAttribute(long minimumLength, long maximumLength);
public long MinimumLength { get; }
public long MaximumLength { get; }
}
// Validation using allow and deny lists
[AttributeUsage(AttributeTargets.Property | AttributeTargets.Field | AttributeTargets.Parameter, AllowMultiple = false)]
public class AllowedValuesAttribute : ValidationAttribute
{
public AllowedValuesAttribute(params object[] values);
public object[] Values { get; }
}
[AttributeUsage(AttributeTargets.Property | AttributeTargets.Field | AttributeTargets.Parameter, AllowMultiple = false)]
public class DeniedValuesAttribute : ValidationAttribute
{
public DeniedValuesAttribute(params object[] values);
public object[] Values { get; }
} |
Per @eiriktsarpalis this is the better place to make the case for replacing the use of the word "Exclusive" with "Included" for the range attributes, thus finally addressing the remaining problems with the original and current names. This isnt a knock against whomever chose those names because naming can be hard. Also English is an overly complicated language with lots of odd and inconsistent rules. I am aware that c# is not English but our programs that do use English words should be borrowing ones with unambiguous meanings. "Exclusive" is more commonly used to describe "something special, limited, or unique that anyone can get, but only from one source". Like "exclusive content" that a media company might generate, or an "exclusive story" that a journalist may write about (but that everyone can access). In this attribute you intend to mean some strict mathematical usage that is something like the opposite of that. By using "Exclusive" there is an easy point of confusion with an everyday English word. If you dont feel there is any confusion with using any form of "Exclude" then you must recognize that it qualifies as a negative name, which does not follow long standing convention for booleans. The new names actually go farther away from those conventions than the previous choices due to the placement of the "be" word. The intellisense argument is not logical and just satisfying someone's need to have things arranged the way they like them (I've been guilty of this in the past too). Switching the order of the "be" word to the middle makes it harder to select the correct property in intellisense, and putting four properties all starting with the same letter and being close in spelling means more keystrokes are required to select the property that I want to use when its not the first choice in the list. Instead of having two I'm not going to be comfortable with using this attribute with its current naming scheme. I will just avoid using it (and require any usage of it in PR's that I have to approve to follow suit) because I know these names will be problematic for someone else later and create bugs. Copied and summarized from a few posts already made |
The terms "exclusive" and "inclusive" are fairly standard when it comes to upper bounds and ranges. A quick web search on "exclusive range" should illuminate this. The reason why we went with "exclusive" over "inclusive" is that inclusive ranges are the default for |
@eiriktsarpalis - Keep in mind that the person saying this is someone who has studied mathematics extensively (at some very prestigious universities) and for whom the notions of Inclusive/Exclusive are probably something understood as naturally as breathing. There's nothing wrong with that perspective, but it is rather unique and rare. Many in our profession have a degree in a different field of study (Phd Biology, MS Communication, etc), some have taken no college classes, some didn't complete high school. We read "exclusive" and have to cognate for a few moments or stop and look it up. If the word were "Included", that impediment to reading the code would not happen.
There are also 71 Million web search results for "exclusive range is confusing" XD Do most English speakers recognize the word "bounds" as in "Leaps and bounds" or "out of bounds"? I dont know, but it could be either. Getting rid of the word "bounds" from the name was a good move. "Exclusive" needs the same treatment. Even changing it to "Excluded" would be better than "Exclusive". EDIT: "Inclusive" isn't really any better than "Exclusive" . When there is a more common, more easily understood, or less ambiguous naming alterative, that should be what is used. |
While you're right that mathematical jargon should not necessarily always transfer to programming APIs, my point is that this is fairly established terminology from the perspective of programming APIs. A quick web search should highlight this, there are examples of this in C#, Rust, Scala, Ruby, Java and the list goes on. |
I read that and these phrases were ringing in my head...
if you do the same thing as everyone else, it's not improving or adding value. That's like repeating a pattern from existing code for the reason of consistency when you know a better pattern. You have a chance to set a better precedent, don't waste it |
I don't actually. We're talking about terminology here, so what is "better" here is really a matter of established convention. I've shown beyond reasonable doubt that inclusive/exclusive is the established terminology, both in the context of mathematics and in the context of programming. |
Keyword there is "like" - I'm giving a similar situation where you would likely choose the opposite. "Better" in this case has nothing to do with convention, and everything to do with readability and less defects. If you dont want to use a more common, more easily understood, and less ambiguous naming alterative, thats up to you. |
Background and motivation
Our project features hundreds of different option types against which we perform data validation operations. We've introduced a number of new data validation attributes which capture essential scenarios in our code. As these are general-purpose in nature, it would be great to add those to the core set of supported attributes.
API Proposal
Original API proposal
API Usage
Alternative Designs
No response
Risks
No response
The text was updated successfully, but these errors were encountered: