Support ICollection with [MaxLength] and [MinLength] data annotations

[martincostello]

This PR adds support for the use of the [MaxLength] and [MaxLength] data annotation attributes on properties whose type implements ICollection so that they can be used on more properties than just those which are strings and arrays, such as List<T> and ICollection<T>.

This would then allow models such as the one below to use the attributes successfully without an InvalidCastException being thrown:

using System.Collections.Generic;
using System.ComponentModel.DataAnnotations;

namespace MyNamespace
{
    public class MyDataModel
    {
        [MinLength(1)]
        public ICollection<string> MySet { get; set; }
    }
}

If the value is not of type string or ICollection then the existing code is executed so that the exception behavior from before ICollection support was added is retained.

[dnfclas]

Hi @martincostello, I'm your friendly neighborhood .NET Foundation Pull Request Bot (You can call me DNFBOT). Thanks for your contribution!

In order for us to evaluate and accept your PR, we ask that you sign a contribution license agreement. It's all electronic and will take just minutes. I promise there's no faxing. https://cla2.dotnetfoundation.org.

TTYL, DNFBOT;

[dnfclas]

@martincostello, Thanks for signing the contribution license agreement so quickly! Actual humans will now validate the agreement and then evaluate the PR.

Thanks, DNFBOT;

[stephentoub]

cc: @krwq, @ianhays

[martincostello]

Jenkins build failure looks like an unrelated issue from a failing test in System.IO.FileSystem.Watcher.Tests.

xUnit.net console test runner (32-bit .NET Core)
  Copyright (C) 2014 Outercurve Foundation.

  Discovering: System.IO.FileSystem.Watcher.Tests
  Discovered:  System.IO.FileSystem.Watcher.Tests
  Starting:    System.IO.FileSystem.Watcher.Tests

  Unhandled Exception: System.ArgumentException: 'overlapped' was not allocated by this ThreadPoolBoundHandle instance.
  Parameter name: overlapped
     at System.Threading.ThreadPoolBoundHandle.FreeNativeOverlapped(NativeOverlapped* overlapped)
     at System.IO.FileSystemWatcher.CompletionStatusChanged(UInt32 errorCode, UInt32 numBytes, NativeOverlapped* overlappedPointer)
     at System.Threading.ThreadPoolBoundHandleOverlapped.CompletionCallback(UInt32 errorCode, UInt32 numBytes, NativeOverlapped* nativeOverlapped)
     at System.Threading._IOCompletionCallback.PerformIOCompletionCallback(UInt32 errorCode, UInt32 numBytes, NativeOverlapped* pOVERLAP)
d:\j\workspace\dotnet_corefx_windows_release_prtest\packages\Microsoft.DotNet.BuildTools.1.0.25-prerelease-00069\lib\tests.targets(130,5): warning MSB3073: The command "CoreRun.exe xunit.console.netcore.exe System.IO.FileSystem.Watcher.Tests.dll  -xml testResults.xml -notrait category=nonwindowstests  -notrait category=OuterLoop -notrait category=failing " exited with code -532462766. [d:\j\workspace\dotnet_corefx_windows_release_prtest\src\System.IO.FileSystem.Watcher\tests\System.IO.FileSystem.Watcher.Tests.csproj]
d:\j\workspace\dotnet_corefx_windows_release_prtest\packages\Microsoft.DotNet.BuildTools.1.0.25-prerelease-00069\lib\tests.targets(141,5): error : One or more tests failed while running tests from 'System.IO.FileSystem.Watcher.Tests' please check log for details! [d:\j\workspace\dotnet_corefx_windows_release_prtest\src\System.IO.FileSystem.Watcher\tests\System.IO.FileSystem.Watcher.Tests.csproj]

[martincostello]

Looks like #2655 is tracking that failure.

[stephentoub]

@terrajobst, this adds new functionality via allowing attributes in places they weren't previously supported. Is this something to be reviewed in an upcoming design meeting?

[terrajobst]

@weshaggard @KrzysztofCwalina I don't think we need to. This looks like a feature of data annotations. No public API changes, not did AttributeTargets change.

Seems @krwq should take a look.

[weshaggard]

cc @lajones as he is the owner for DataAnnotations

[lajones]

@rowanmiller / @divega - what do you think - this has been discussed in the past but we rejected it because we didn't think it was the time to change the way these annotations work. Has the time come?

[divega]

The code changes look good and I agree with @terrajobst that this enables new nice behavior without API changes so from that perspective I would be happy with us taking the PR.

I remember one of the concerns being that any changes to DataAnnotations would light up only on platforms in which you can actually get a newer version of DataAnnotations. E.g. desktop .NET is a separate codebase and won't automatically get this and AFAIR UWP would be a different train as well.

[stephentoub]

Thanks, @lajones and @divega. What would you like to do with this then?

[divega]

I suspect what I said in my last paragraph about functionality getting out of sync across platforms applies to most things in this repo. I am not sure if you have general guidelines on how to deal with that, but if you guys are fine with it happening in this case and if @rowanmiller gives the ok, I would like you to merge it.

[rowanmiller]

I'm fine with it. Mostly because it's currently a negative case on all platforms, so it's not a subtle change in behavior... it straight up doesn't work anywhere and will light up on some platforms with this change.

[stephentoub]

Sounds good. @weshaggard, however we track such things, we should track this to ensure the behavior gets ported to other platforms.

[martincostello]

Thanks for merging this change.

[weshaggard]

@stephentoub We don't have a great way to track such things. The owner in this @lajones should track it accordingly for each of the platforms we want to bring this behavior to.