Runtime: Enable MinLengthAttribute and MaxLengthAttribute in more collection types.

[weitzhandler]

Please implement a ValidationAttribute that validates that a collection-typed property has any elements.
Maybe expand the RangeAttribute functionality to work with collection?

public class Contact
{
  [Range(3, 10)]
  public ICollection<Phone> Phones { get; set; }
}

[lajones]

Note: MaxLengthAttribute and MinLengthAttribute can already be applied to ICollections.

[weitzhandler]

But not on ICollection<T>'s or IEnumerables 😳

Repro:

```c#
class Program
{
static void Main(string[] args)
{
var p = new Program();
var result = Validator.TryValidateObject(p, new ValidationContext(p), null, true);
Debug.Assert(!result);
}

  [MinLength(2)]
  public IEnumerable<string> Names { get; } = new List<string>(); //works: new string[] { };
}

StackTrace:

at System.ComponentModel.DataAnnotations.MinLengthAttribute.IsValid(Object value)
at System.ComponentModel.DataAnnotations.ValidationAttribute.IsValid(Object value, ValidationContext validationContext)
at System.ComponentModel.DataAnnotations.ValidationAttribute.GetValidationResult(Object value, ValidationContext validationContext)
at System.ComponentModel.DataAnnotations.Validator.TryValidate(Object value, ValidationContext validationContext, ValidationAttribute attribute, ValidationError& validationError)
at System.ComponentModel.DataAnnotations.Validator.GetValidationErrors(Object value, ValidationContext validationContext, IEnumerable1 attributes, Boolean breakOnFirstError) at System.ComponentModel.DataAnnotations.Validator.GetObjectPropertyValidationErrors(Object instance, ValidationContext validationContext, Boolean validateAllProperties, Boolean breakOnFirstError) at System.ComponentModel.DataAnnotations.Validator.GetObjectValidationErrors(Object instance, ValidationContext validationContext, Boolean validateAllProperties, Boolean breakOnFirstError) at System.ComponentModel.DataAnnotations.Validator.TryValidateObject(Object instance, ValidationContext validationContext, ICollection1 validationResults, Boolean validateAllProperties)
at ConsoleApp1.Program.Main(String[] args) in C:\Users\Shimmy\AppData\Local\Temporary Projects\ConsoleApp1\Program.cs:line 16
```

The problem is that the attribute only deals with ICollection, not ICollection<T> (which doesn't inherit from ICollection).

Apparently this has been discussed before: dotnet/runtime#21101.

[EDIT] Fixed C# syntax highlighting by @karelz

[lajones]

Note to self: see also PR20934.

[weitzhandler]

Issue reopened.

ICollection<T> should also be addressed too (since it doesn't inherit ICollection), and IEnumerable as well, would be glad to see something like the following added here:

```c#
else if (value is IEnumerable enumerable)
{
var enumerator = enumerable.GetEnumerator();
while (length < Length && enumerator.MoveNext())
{
length++;
}
}

And the following added [here](https://github.com/dotnet/corefx/blob/master/src/System.ComponentModel.Annotations/src/System/ComponentModel/DataAnnotations/MaxLengthAttribute.cs#L85):

```c#
else if (value is IEnumerable enumerable)
{
  var enumerator = enumerable.GetEnumerator();
  while (length <= Length && enumerator.MoveNext())
  {
    length++;
  }
}

[EDIT] Fixed C# syntax highlighting by @karelz

[weitzhandler]

@karelz I chose flat highlighting to be able to use the <b> tags to emphasize the operators 😉

[karelz]

Oh, I missed that - fixed it (remove the bold).
It is all code diff, it's not clear top me what the bold would mean. Do you want to call it out in text?

[weitzhandler]

I wanted to emphasize the difference between the top snippet of the bottom snippet (notice the less-than vs the less-than or equal operators), so I wanted to bold it out in the code, I just don't know how to set language in <pre><code> in GitHub markdown, so to enable syntax coloring as well.

<pre>while (<b>length < Length</b> && enumerator.MoveNext())</pre> is rendered as

while (length < Length && enumerator.MoveNext())

Notice the bold code.

My code sample doesn't really matter anyway, was just pointing an example implementation.

[lajones]

@weitzhandler Are you creating .NET Core or .NET Framework for your results above? And what version?

[weitzhandler]

It doesn't matter, .NET Framework only supports Arrays, while .NET Core only supports ICollections.

ICollection<T> and IEnumerable<T> are not supported in either, hence this suggestion.

[divega]

DataAnnotations Triage: We think that enabling [MinLength] and [MaxLength] on ICollection<T> is a reasonable request and we would consider a PR for it.

Note that the code necessary to invoke the Count property in an ICollection<T> for an unknown T can be tricky. We think it would be reasonable to fall back to using reflection and invoking a Count property getter.

Also, we don't think it is a good idea to open up this for IEnumerable<T> as not all IEnumerable<T> instances are countable.

[weitzhandler]

@divega
I'm willing to implement this.
Did you see my suggestion on IEnumerable (hence IEnumerable<T>)?
In IEnumerables, I'd like iterate the collection to the necessary point to determine if validation succeeded/failed.
To sum up:

else if (value is IEnumerable enumerable)
{
  var enumerator = enumerable.GetEnumerator();
  while (length <= Length && enumerator.MoveNext())
  {
    length++;
  }
}
else 
  throw new InvalidCastException(...);

[divega]

@weitzhandler we saw the proposal for IEnumerable, but perhaps we didn't discuss it enough.

You should use a using statement for the enumerator, but besides that, I don't see a problem with it.

@ajcvickers @lajones do you see a problem with this?

[weitzhandler]

@divega I'm using the non-generic enumerator, which does not implement IDisposable. I would obviously wrap it in a using if I were to use the generic one.

[divega]

@weitzhandler use IEnumerable<object> instead.

[weitzhandler]

@divega
I will, but can you please link a source on why IEnumerable<object> over IEnumerable?
Is this a general practice in .NET? Asking just so I can expand my knowledge.
And bear in mind that that won't work for primitive non-generic types which aren't a generic IEnumerable by any means (for example ArrayList), so should we check for both IEnumerable (using using) and the non-generic one separately? I mean an IEnumerable<object> is an IEnumerable, while not vice versa. I didn't benchmark it tho.

[divega]

It is just not safe not to dispose the enumerator: Many enumerables use resources that need to be disposed deterministically, e.g. think about database queries, not just in-memory collections.

You could always achieve this by adding a try/catch/finally and checking if the IEnumerator happens to be an IDisposable and then dispose it in the finally block. But using IEnumerable<object> and then assigning the IEnumerator<object> in a using block is much simpler, and seems adequate because:

• Your stated goal was from the beginning to support IEnumerable<T> and not any arbitrary IEnumerable
• Thanks to generic covariance all IEnumerable<T> instances can be assigned to IEnumerable<object>

[weitzhandler]

Actually to satisfy my curiosity I created this benchmark, and the generic is much faster:

static void Main(string[] args)
{
  var priSw = new Stopwatch();
  var genSw = new Stopwatch();

  var list = new List<object>();
  for (int i = 0; i < 1000*1000; i++)
  {                             
    priSw.Start();
    var priEnumerator = ((IEnumerable)list).GetEnumerator();
    while (priEnumerator.MoveNext()) ;
    priSw.Stop();

    genSw.Start();
    using (var genEnumerator = list.GetEnumerator())
      while (priEnumerator.MoveNext()) ;
    genSw.Stop();

    Console.Write($"\r{i}");
  }

  Console.WriteLine($"Primitive: {priSw.Elapsed}");
  Console.WriteLine($"Generic: {genSw.Elapsed}");
  Console.ReadKey();
}

Results:
Primitive: 00:00:00.1682750
Generic: 00:00:00.0403097

So for conclusion I want to add a check for ICollection<T> (using reflection), then the existing ICollection check, IEnumerable<object>, and also for IEnumerable if it doesn't fall thru the generic one.
Will you accept a PR accomplishing this?

[ajcvickers]

@divega Still not in favor of doing this for enumerable. I don't think that the semantics align well enough, and I think that enumerating the collection could be both unexpected and potentially have side effects.

[divega]

@weitzhandler see @ajcvickers point on why not doing this for IEnumerable<T>. I also don't think there is necessarily a lot of value in it.

[weitzhandler]

@divega
OK so the verdict is to currently only add support for ICollection<T> is that right? I agree that covers most common collections indeed.

[divega]

That seems right.

[weitzhandler]

See PR dotnet/corefx#23664.

[jamesqo]

For completeness' sake, should IReadOnlyCollection<T> be supported too? It also has a Count property but is independent of ICollection or ICollection<T>.

[weitzhandler]

@jamesqo ICollection<T> doesn't inherit from IReadOnlyCollection<T>. See dotnet/corefx#23578 for the complete discussion on this painful Count property.
Can you name a proper scenario that's not covered by ICollection or ICollection<T> that is IReadOnlyCollection<T>? Notice this is done by reflection so there needs to be a justified reason to search the interface list again.