ICollection<T> support in MinLengthAttribute and MaxLengthAttribute #23664

weitzhandler · 2017-08-29T23:10:07Z

Fixes #23461.

…ection<T>

lajones · 2017-08-30T19:11:43Z

src/System.ComponentModel.Annotations/tests/MaxLengthAttributeTests.cs

@@ -111,20 +112,13 @@ public static void GetValidationResult_ValueNotStringOrICollection_ThrowsInvalid
        [Fact]
        public static void GetValidationResult_ValueGenericICollection_ThrowsInvalidCastException()


Could you change the name of the test method (plus the one below) and add a couple of tests - one for ICollection and one for ICollection<T>?

divega · 2017-08-30T21:46:19Z

src/System.ComponentModel.Annotations/tests/MaxLengthAttributeTests.cs

        }
    }

-    class GenericICollectionClass : ICollection<int>
+    class GenericIEnumerableClass : IEnumerable<int>


Shouldn't this be reverted back to be an ICollection<T>? The test for IEnumerable<T> does not apply as we are not supporting those.

The purpose of this class is for the failing tests. The fact used to use this class as evidence that the attribute throws an exception when applied on ICollection<T>. Now that it's been fixed, I purposely minimized this class to IEnumerable<T>.
I'd hope we support IEnumerable<object> with greedy iteration to the point we know the result as I pointed out here, for completeness, but I guess it's a no.

Got it. Is there a positive test case for the added scenario?

I added them as requested.

divega · 2017-08-30T21:58:14Z

...m.ComponentModel.Annotations/src/System/ComponentModel/DataAnnotations/MaxLengthAttribute.cs

            else
            {
-                if (value is ICollection collection)
+                Type genericCol = value.GetType().GetInterfaces().FirstOrDefault(i => i.IsGenericType && i.GetGenericTypeDefinition() == typeof(ICollection<>));


Stepping back a bit, it seems that it won't be needed in the majority of scenarios we are trying to enable. It turns out that most concrete types that implement ICollection<T> also implement the non-generic ICollection, so they will match the previous condition.

To be honest, there is the fact that code smells (it seems correct but it looks more complex and expensive than I was hoping it would be) and if we are going to include it we should probably move it into an internal helper so that both attributes can share it.

cc @ajcvickers @lajones

@divega I'm ok with just targeting ICollection<T>.
And I can't agree more that code smells. Please also take a look at #23578. There needs to be an intermediate non-generic collection between ICollection<T> and IEnumerable, and between ICollection and IEnumerable, which exposes a Count property available regardless of generics.

@weitzhandler I assume you mean targeting ICollection (non-generic). Then fine, let's remove the whole check of ICollection<T>.

Re ICountable, I don't hate the idea 😄 I am not sure what the best decision is, but if you want to make the case for it more complete, I suggest you show right there in the top comment the in the issue the code you have to write in order to obtain the Count of and ICollection<T> without knowing what the T is.

@divega
I think we should leave the ICollection check here, so if the value does implement ICollection (most collections do), we will spare the reflection overhead.
BTW, HashSet<T> is an example of a collection not inheriting from ICollection, and is pretty common use, also in validation. I wouldn't have bothered implementing this just for the heck of it...

Anyway, I updated the post on the ICountable, please check it out #23578.

@weitzhandler

I think we should leave the ICollection check here, so if the value does implement ICollection (most collections do), we will spare the reflection overhead.

👍 that is what I meant as well.

HashSet is an example of a collection not inheriting from ICollection

You are correct. I missed HashSet<T> when I looked around, and that one is quite important.

I talked about this briefly with @ajcvickers and realized that when we wrote the comment at https://github.com/dotnet/corefx/issues/23461#issuecomment-324767348 we both were thinking that the way to go was to just use reflection to look for a Count property that has a getter and returns int, i.e. without all the goop to try to find the "right" interface.

Incidentally, I believe this simple pattern matching approach aligns better with the spirit of your ICountable proposal.

weitzhandler · 2017-09-01T08:40:54Z

@divega @lajones why do the checks fail? It passed the tests on my machine. Do I miss something?
I'll try switching to TypeInfo.

- Linux x64 Release Build - UWP CoreCLR x64 Debug Build

weitzhandler · 2017-09-01T10:07:02Z

I'll try switching to TypeInfo.

I guess that did the job.

weitzhandler · 2017-09-04T22:42:59Z

@divega @karelz Any news?
I'm not familiar with the local merging rules, but how long could take until a PR is evaluated / merged?

divega · 2017-09-05T01:38:58Z

@weitzhandler

Please allow a few days. We usually discuss/review DataAnnotation issues once a week.

I made a suggestion to simplify the reflection code to just check for an int Count property directly on the object received. Even after that, there seems to be an opportunity to reduce code duplication between the two attributes. I will leave the final feedback to @ajcvickers and @lajones.

karelz · 2017-09-05T16:05:06Z

@weitzhandler PRs typically take from hours (for really small ones) to couple of weeks. If they are controversial, or if there's a lot of back and forth, it can take longer - a month or more. It depends also on the speed of feedback discussions.
If there are compatibility concerns or the change impacts public API surface, that also slows down the process as we often need to pause the review process and think about the change in principle - do we want it or not? What should it do? Etc.
I hope it helps.

ajcvickers · 2017-09-05T22:47:05Z

...m.ComponentModel.Annotations/src/System/ComponentModel/DataAnnotations/MaxLengthAttribute.cs

            else
            {
-                if (value is ICollection collection)
+                Type genericCol = value.GetType().GetTypeInfo().ImplementedInterfaces.Select(t => t.GetTypeInfo()).FirstOrDefault(i => i.IsGenericType && i.GetGenericTypeDefinition() == typeof(ICollection<>));


What should be the behavior if ICollection<T> is implemented multiple times? Options I see are:

Throw

Choose one in an arbitrary but deterministic way

Don't do it this way

Throwing isn't very helpful because there probably isn't anything that can be done in the application to make it work.

I'm not against choosing one implementation deterministically, but I'm not convinced that the above code is deterministic across platforms/architectures, since it relies on Reflection ordering. We probably should do something like ordering by the name of the generic type.

A different approach would be to just look for a "Count" property with an integer return type on the actual type of value.GetType--essentially pattern matching, rather than using the interface typing. This would be my preference.

So should I change it to just look for a Count property?
BTW @ajcvickers did you see #23578?

@weitzhandler That would be my preference. I saw the ICountable discussion; I have no strong feelings one way or another on that--that is, done carefully it probably won't cause harm (i.e. break anything) but I also don't see a huge amount of value.

The real amount of value is IMHO that the Count property is exposed the wrong way in .NET collections.

ajcvickers · 2017-09-05T22:49:32Z

...m.ComponentModel.Annotations/src/System/ComponentModel/DataAnnotations/MinLengthAttribute.cs

@@ -60,11 +63,16 @@ public override bool IsValid(object value)
            {
                length = str.Length;
            }
+            else if (value is ICollection collection)


Factor out code for getting the Count/Length into a helper to DRY it up.

I actually started it like this, but then thought let's keep the classes with less methods.
The MinLengthAttribute and MaxLengthAttribute have a lot of shared code written twice. That's why I kept this implementation separate too.

ajcvickers · 2017-09-05T22:53:44Z

src/System.ComponentModel.Annotations/tests/MaxLengthAttributeTests.cs

+
+            yield return new object[] { new MaxLengthAttribute(-1), new HashSet<int>(Enumerable.Range(1, 20)) };
+            yield return new object[] { new MaxLengthAttribute(15), new HashSet<string>(Enumerable.Range(1, 14).Select(i => i.ToString())) };
+            yield return new object[] { new MaxLengthAttribute(16), new HashSet<string>(Enumerable.Range(1, 16).Select(i => i.ToString())) };


Make sure there are tests for:

Types that are ICollection but not ICollection<T> and vice versa

Types that implement ICollection<T> multiple times

If we do pattern matching, then types that have a Count but are not ICollection.

Types that are ICollection but not ICollection<T> (a) and vice versa (b)

a. Since we rely on the Count property this is redundant. I could add ArrayList which will fall through ICollection.
b. That's the purpose of HashSet<T> I've added to the tests

Types that implement ICollection<T> multiple times

Again since ICollection<T> is out of the picture this is redundant.

If we do pattern matching, then types that have a Count but are not ICollection.

Again, HashSet<T> satisfies this test case.

See my latest commit: 6353a8f

@weitzhandler I recognize that some of these will hit the same code path multiple times, but they are still useful in a couple of ways:

They make sure that our reasoning about which cases are covered is correct

They act as regression tests so that if the implementation changes in the future (for example, if ICountable or something like it happens) then the existing behaviors are still tested.

@ajcvickers
Done 4cba646.
The multi ICollection<T> looks totally redundant to me but I implemented it anyway per your request.

ajcvickers

See comments.

- Extracted Count checking to an external helper to obey DRY - Removed dependency of ICollection<T> and changed to simple reflection Count property lookup

weitzhandler · 2017-09-13T22:44:53Z

@ajcvickers why are the checks not successful?

ajcvickers · 2017-09-14T18:56:41Z

@weitzhandler Doesn't look like the failure was related to your changes.

ajcvickers · 2017-09-14T22:46:04Z

@weitzhandler Thanks for the contribution. This is now merged.

…otnet/corefx#23664) * Updated in MinLengthAttribute and MaxLengthAttribute to support ICollection<T> * Added tests * Fixed typo * Trying to address two failing checks: - Linux x64 Release Build - UWP CoreCLR x64 Debug Build * Implemented changes requested in review - Extracted Count checking to an external helper to obey DRY - Removed dependency of ICollection<T> and changed to simple reflection Count property lookup * Added requested tests Commit migrated from dotnet/corefx@f711317

Updated in MinLengthAttribute and MaxLengthAttribute to support IColl…

8650ca0

…ection<T>

dnfclas added the cla-already-signed label Aug 29, 2017

danmoseley requested a review from lajones August 30, 2017 04:48

karelz added the area-System.ComponentModel.DataAnnotations label Aug 30, 2017

karelz assigned weitzhandler, divega, ajcvickers and lajones Aug 30, 2017

lajones reviewed Aug 30, 2017

View reviewed changes

divega reviewed Aug 30, 2017

View reviewed changes

Shimmy Weitzhandler added 2 commits August 31, 2017 03:55

Added tests

7d23869

Fixed typo

86ace1d

Trying to address two failing checks:

dfb5e1d

- Linux x64 Release Build - UWP CoreCLR x64 Debug Build

ajcvickers reviewed Sep 5, 2017

View reviewed changes

ajcvickers suggested changes Sep 5, 2017

View reviewed changes

Implemented changes requested in review

6353a8f

- Extracted Count checking to an external helper to obey DRY - Removed dependency of ICollection<T> and changed to simple reflection Count property lookup

Added requested tests

4cba646

ajcvickers approved these changes Sep 14, 2017

View reviewed changes

ajcvickers merged commit f711317 into dotnet:master Sep 14, 2017

weitzhandler deleted the dataannotations-length-fix branch September 15, 2017 10:55

karelz added this to the 2.1.0 milestone Oct 11, 2017

This was referenced Jan 31, 2020

Make a new interface that exposes collection count dotnet/runtime#8841

Closed

Add a non-generic IReadOnlyCollection interface dotnet/runtime#23337

Open

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

ICollection<T> support in MinLengthAttribute and MaxLengthAttribute #23664

ICollection<T> support in MinLengthAttribute and MaxLengthAttribute #23664

weitzhandler commented Aug 29, 2017

lajones Aug 30, 2017

weitzhandler Aug 31, 2017

divega Aug 30, 2017

weitzhandler Aug 30, 2017

divega Aug 30, 2017

weitzhandler Aug 31, 2017 •

edited

Loading

divega Aug 30, 2017

weitzhandler Aug 30, 2017 •

edited

Loading

divega Aug 30, 2017

weitzhandler Aug 31, 2017

divega Aug 31, 2017 •

edited

Loading

weitzhandler commented Sep 1, 2017 •

edited

Loading

weitzhandler commented Sep 1, 2017

weitzhandler commented Sep 4, 2017

divega commented Sep 5, 2017

karelz commented Sep 5, 2017

ajcvickers Sep 5, 2017

weitzhandler Sep 6, 2017

ajcvickers Sep 6, 2017

weitzhandler Sep 6, 2017

ajcvickers Sep 5, 2017

weitzhandler Sep 6, 2017 •

edited

Loading

ajcvickers Sep 5, 2017

weitzhandler Sep 13, 2017 •

edited by divega

Loading

ajcvickers Sep 14, 2017

weitzhandler Sep 14, 2017

ajcvickers left a comment

weitzhandler commented Sep 13, 2017

ajcvickers commented Sep 14, 2017

ajcvickers commented Sep 14, 2017

		@@ -111,20 +112,13 @@ public static void GetValidationResult_ValueNotStringOrICollection_ThrowsInvalid
		[Fact]
		public static void GetValidationResult_ValueGenericICollection_ThrowsInvalidCastException()

ICollection<T> support in MinLengthAttribute and MaxLengthAttribute #23664

ICollection<T> support in MinLengthAttribute and MaxLengthAttribute #23664

Conversation

weitzhandler commented Aug 29, 2017

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

weitzhandler Aug 31, 2017 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

weitzhandler Aug 30, 2017 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

divega Aug 31, 2017 • edited Loading

Choose a reason for hiding this comment

weitzhandler commented Sep 1, 2017 • edited Loading

weitzhandler commented Sep 1, 2017

weitzhandler commented Sep 4, 2017

divega commented Sep 5, 2017

karelz commented Sep 5, 2017

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

weitzhandler Sep 6, 2017 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

weitzhandler Sep 13, 2017 • edited by divega Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

ajcvickers left a comment

Choose a reason for hiding this comment

weitzhandler commented Sep 13, 2017

ajcvickers commented Sep 14, 2017

ajcvickers commented Sep 14, 2017

weitzhandler Aug 31, 2017 •

edited

Loading

weitzhandler Aug 30, 2017 •

edited

Loading

divega Aug 31, 2017 •

edited

Loading

weitzhandler commented Sep 1, 2017 •

edited

Loading

weitzhandler Sep 6, 2017 •

edited

Loading

weitzhandler Sep 13, 2017 •

edited by divega

Loading