Add System.ComponentModel.Annotations tests #20934

hughbe · 2017-06-12T12:11:34Z

No description provided.

stephentoub · 2017-06-12T12:45:02Z

src/System.ComponentModel.Annotations/src/Resources/Strings.resx

  <data name="MinLengthAttribute_InvalidMinLength" xml:space="preserve">
    <value>MinLengthAttribute must have a Length value that is zero or greater.</value>
  </data>
  <data name="MinLengthAttribute_ValidationError" xml:space="preserve">
    <value>The field {0} must be a string or array type with a minimum length of '{1}'.</value>
  </data>
+  <data name="MinLengthAttribute_InvalidValueType" xml:space="preserve">
+    <value>The field of type {0} must be a string, array or ICollection type.</value>
+  </data>


This is the same string as MaxLengthAttribute_InvalidValueType... why do we need both?

stephentoub · 2017-06-12T12:45:20Z

...tem.ComponentModel.Annotations/src/System/ComponentModel/DataAnnotations/CompareAttribute.cs

@@ -56,35 +56,23 @@ protected override ValidationResult IsValid(object value, ValidationContext vali
            {
                if (OtherPropertyDisplayName == null)
                {
-                    OtherPropertyDisplayName = GetDisplayNameForProperty(validationContext.ObjectType, OtherProperty);
+                    OtherPropertyDisplayName = GetDisplayNameForProperty(validationContext.ObjectType, otherPropertyInfo);


Why did this change?

If you look above, we have the following code:

var otherPropertyInfo = validationContext.ObjectType.GetRuntimeProperty(OtherProperty);

In the GetDisplayNameForProperty method, we used to have the buggy code:

var property = containerType.GetRuntimeProperties() .SingleOrDefault( prop => IsPublic(prop) && string.Equals(propertyName, prop.Name, StringComparison.OrdinalIgnoreCase));

However, we already have the property otherPropertyInfo above. The name comparison is broken, and GetRuntimeProperty only returns public methods so this whole thing is redundant and broken.

So we can just use the actual PropertyInfo we found before

stephentoub · 2017-06-12T12:45:46Z

...tem.ComponentModel.Annotations/src/System/ComponentModel/DataAnnotations/CompareAttribute.cs

            var attributes = CustomAttributeExtensions.GetCustomAttributes(property, true);
            var display = attributes.OfType<DisplayAttribute>().FirstOrDefault();
            if (display != null)
            {
                return display.GetName();
            }

-            return propertyName;
+            return OtherProperty;


Why did this change?

The old value passed to the parameter propertyName was OtherProperty. Now that that parameter is gone, we use OtherProperty directly

stephentoub · 2017-06-12T12:46:37Z

...nentModel.Annotations/src/System/ComponentModel/DataAnnotations/CustomValidationAttribute.cs

-                }
-
-                throw;
+                throw tie.InnerException;


Do you know for sure that InnerException will always be non-null in all circumstances?

This is the try block:

try { // 1-parameter form is ValidationResult Method(object value) // 2-parameter form is ValidationResult Method(object value, ValidationContext context), var methodParams = _isSingleArgumentMethod ? new object[] { convertedValue } : new[] { convertedValue, validationContext }; var result = (ValidationResult)methodInfo.Invoke(null, methodParams); // We capture the message they provide us only in the event of failure, // otherwise we use the normal message supplied via the ctor _lastMessage = null; if (result != null) { _lastMessage = result.ErrorMessage; } return result; } catch (TargetInvocationException tie) { throw tie.InnerException; }

The only place where the TIE can be thrown is from reflection invoke, and there will always be a non-null inner exception in this case.

stephentoub · 2017-06-12T12:47:19Z

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

-                    // A cast exception previously occurred if a non-{string|array} property was passed
-                    // in so preserve this behavior if the value does not implement ICollection
-                    length = ((Array)value).Length;
+                    throw new InvalidCastException(SR.Format(SR.MaxLengthAttribute_InvalidValueType, value.GetType()));


Why did you change this?

What if it is an array? You're removing functionality here which was added to solve a previous issue.

In reply to: 121382258 [](ancestors = 121382258)

I think this was addressing https://github.com/dotnet/corefx/issues/18361

In reply to: 121792346 [](ancestors = 121792346,121382258)

stephentoub · 2017-06-12T12:49:07Z

...onentModel.Annotations/src/System/ComponentModel/DataAnnotations/ValidationAttributeStore.cs

@@ -212,12 +213,9 @@ internal bool TryGetPropertyStoreItem(string propertyName, out PropertyStoreItem

                if (_propertyStoreItems == null)
                {
-                    lock (_syncRoot)
+                    lock (_lock)


As far as I can tell, this is a buggy change. Why did you change this? It's now using a process-wide lock to protect instance state, and it removed the double-check that would ensure that multiple threads always see the same value even in the face of race conditions.

Okay. I wasn't sure about the double check. I reverted this change

stephentoub · 2017-06-12T12:50:14Z

src/System.ComponentModel.Annotations/src/System/ComponentModel/DataAnnotations/Validator.cs

@@ -236,7 +237,7 @@ public static void ValidateProperty(object value, ValidationContext validationCo
            var err = GetValidationErrors(value, validationContext, attributes, false).FirstOrDefault();
            if (err != null)
            {
-                err.ThrowValidationException();
+                throw err.GetValidationException();


Why did you change these? If it's purely about getting to 100% code coverage by allowing the compiler to see that the subsequent brace is unreachable, that's not worth the change.

That was why, yes. I reverted this change

stephentoub · 2017-06-12T12:54:26Z

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

+        }
+
+        [Serializable]
+        public class SubException : ValidationException


Why is a SubException type necessary for this test? Shouldn't a PNSE be produced as well when working with ValidationException directly?

ValidationException is not [Serializable] so can't be serialized. The test is deserialization, so obviously if we can't serialize, then we can't test deserialization

I've deleted this test

lajones · 2017-06-13T20:47:37Z

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

-                    // A cast exception previously occurred if a non-{string|array} property was passed
-                    // in so preserve this behavior if the value does not implement ICollection
-                    length = ((Array)value).Length;
+                    throw new InvalidCastException(SR.Format(SR.LengthAttribute_InvalidValueType, value.GetType()));


throw new InvalidCastException(SR.Format(SR.LengthAttribute_InvalidValueType, value.GetType())); [](start = 20, length = 96)

As for MaxLengthAttribute: what if it is an array? You're removing functionality here which was added to solve a previous issue.

So that's what I first thought when I saw the code and why I fixed this. All Arrays are ICollections, so this code path will never be hit as they will take the precious if block. We have tests that cover this in the project.
What this is actually doing is simulating an InvalidCastException for comparability with before we added ICollection support.

That makes sense - thanks for clarifying.

lajones · 2017-06-13T21:19:13Z

@hughbe General comment - could we have a separate PR for each issue please? Would like to address the fix for #20932 separately from the addition of other tests.

hughbe · 2017-06-13T21:47:20Z

Gladly! Will do so soon

hughbe · 2017-06-16T00:37:06Z

I've extracted the netfx compat fix to #21031, this just adds some tests and cleans up some code

hughbe · 2017-06-19T01:04:42Z

Test Portable Linux x64 Debug Build

hughbe · 2017-06-19T07:01:30Z

I rebased this PR to fix merge conflicts

lajones · 2017-06-19T20:40:01Z

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

@@ -69,6 +69,26 @@ public static void Constructor(Type validatorType, string method)
            Assert.Equal(method, attribute.Method);
        }

+        [Fact]
+        public void FormatErrorMessage_NoError_ContainsName()


Nit: Just checking - did you mean this to say "NoError"? The test is about what happens to the error message? The test below checks that validation fails - so did you mean "validation passes" here? If so maybe add that assert or rename the tests.

It's not quite that validation passes. It's actually that no validation has ever been performed on that instance - i've updated the comments accordingly

OK. Now it all makes sense. Thanks for the updates.

lajones · 2017-06-20T15:58:45Z

@ajcvickers This looks goods to me. Do you want to review before @hughbe merges this in?

hughbe · 2017-06-21T00:36:19Z

I don't actually have permission to merge it in as I'm not an MSFT employee so that's up to you guys :) thanks for reviewing

lajones · 2017-06-21T16:06:05Z

My bad. I meant "before we merge this in for @hughbe" 😄

hughbe · 2017-06-27T01:20:10Z

Friendly ping :)

* Moving the Utf8Formatter and Utf8Parser into S.P.Corelib * Doing some minimal cleanup to lineup types and get the Utf8Parser/Utf8Formatter building * Updating the Utf8 Float Parser to have different buffers for Single vs Double * Fixing the Utf8Parser to track trailing zero digits and to properly mark the end of the buffer * Fixing a couple of issues in Utf8Parser.Number Signed-off-by: dotnet-bot <[email protected]>

…otations-tests Add System.ComponentModel.Annotations tests Commit migrated from dotnet/corefx@1a76f61

dnfclas added the cla-already-signed label Jun 12, 2017

stephentoub reviewed Jun 12, 2017

View reviewed changes

karelz added the area-System.ComponentModel.DataAnnotations label Jun 12, 2017

karelz assigned hughbe Jun 12, 2017

karelz added this to the 2.1.0 milestone Jun 12, 2017

karelz assigned divega, ajcvickers and lajones Jun 12, 2017

lajones reviewed Jun 13, 2017

View reviewed changes

hughbe mentioned this pull request Jun 14, 2017

Fix netfx compat bug for cased properties in CompareAttribute #21031

Merged

hughbe changed the title ~~Add System.ComponentModel.Annotations tests and fix a netfx compat bug~~ Add System.ComponentModel.Annotations tests Jun 16, 2017

hughbe added 3 commits June 19, 2017 14:01

Add System.ComponentModel.Annotations tests

4467f39

Remove some dead code in System.ComponentModel.Annotations

4721339

Fixup weird Array casting dead code in Min/MaxLengthAttribute

6259768

lajones reviewed Jun 19, 2017

View reviewed changes

Update naming

1741a94

ajcvickers approved these changes Jun 27, 2017

View reviewed changes

lajones merged commit 1a76f61 into dotnet:master Jun 27, 2017

hughbe deleted the componentmodel-annotations-tests branch June 27, 2017 23:54

picenka21 pushed a commit to picenka21/runtime that referenced this pull request Feb 18, 2022

Merge pull request dotnet/corefx#20934 from hughbe/componentmodel-ann…

190448a

…otations-tests Add System.ComponentModel.Annotations tests Commit migrated from dotnet/corefx@1a76f61

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Add System.ComponentModel.Annotations tests #20934

Add System.ComponentModel.Annotations tests #20934

hughbe commented Jun 12, 2017 •

edited

Loading

stephentoub Jun 12, 2017

stephentoub Jun 12, 2017

hughbe Jun 13, 2017

stephentoub Jun 12, 2017

hughbe Jun 13, 2017

stephentoub Jun 12, 2017

hughbe Jun 13, 2017

stephentoub Jun 12, 2017

lajones Jun 13, 2017

lajones Jun 13, 2017

stephentoub Jun 12, 2017

hughbe Jun 13, 2017 •

edited

Loading

stephentoub Jun 12, 2017

hughbe Jun 13, 2017 •

edited

Loading

stephentoub Jun 12, 2017

hughbe Jun 13, 2017

hughbe Jun 14, 2017

lajones Jun 13, 2017

hughbe Jun 13, 2017

lajones Jun 20, 2017

lajones commented Jun 13, 2017

hughbe commented Jun 13, 2017

hughbe commented Jun 16, 2017

hughbe commented Jun 19, 2017

hughbe commented Jun 19, 2017

lajones Jun 19, 2017

hughbe Jun 20, 2017

lajones Jun 20, 2017

lajones commented Jun 20, 2017

hughbe commented Jun 21, 2017

lajones commented Jun 21, 2017

hughbe commented Jun 27, 2017

Add System.ComponentModel.Annotations tests #20934

Add System.ComponentModel.Annotations tests #20934

Conversation

hughbe commented Jun 12, 2017 • edited Loading

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

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

Choose a reason for hiding this comment

hughbe Jun 13, 2017 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

hughbe Jun 13, 2017 • edited Loading

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

Choose a reason for hiding this comment

Choose a reason for hiding this comment

lajones commented Jun 13, 2017

hughbe commented Jun 13, 2017

hughbe commented Jun 16, 2017

hughbe commented Jun 19, 2017

hughbe commented Jun 19, 2017

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

lajones commented Jun 20, 2017

hughbe commented Jun 21, 2017

lajones commented Jun 21, 2017

hughbe commented Jun 27, 2017

hughbe commented Jun 12, 2017 •

edited

Loading

hughbe Jun 13, 2017 •

edited

Loading

hughbe Jun 13, 2017 •

edited

Loading