dotnet / runtime

.NET is a cross-platform runtime for cloud, mobile, desktop, and IoT apps.
https://docs.microsoft.com/dotnet/core/
MIT License
14.98k stars 4.66k forks source link

Bug: Change in behavior with .NET 5 vs .NET 6 in AllowMultiple attributes #63195

Open Rutix opened 2 years ago

Rutix commented 2 years ago

Description

We are upgrading our solutions from .NET 5 and .NET 6 and it went well until in one solutions a lot of unittests started failing. It seems that when you use TryValidateObject from System.ComponentModel.DataAnnotations it won't go through all the (custom) validation attributes of the same type when there are multiple on a property. While debugging in .NET 5 you go through the IsValid method as many times as you have added the attribute but in .NET 6 it will only call the IsValid and it seems to be of the last attribute.

Reproduction Steps

I have tried to make an as minimal repo as possible:

.NET 5: https://github.com/Rutix/ValidationTest/blob/main/ValidationTest.Unittest/UnitTest1.cs

Running this will pass the test.

.NET 6: https://github.com/Rutix/ValidationTest/blob/net6/ValidationTest.Unittest/UnitTest1.cs

Running this will fail the test.

Expected behavior

The expected behavior (unless i missed a breaking change) would be the behavior which we get in .NET 5.

Actual behavior

IsValid isn't called on all the attributes of the same type.

Regression?

Worked in .NET 5

Known Workarounds

No response

Configuration

No response

Other information

No response

dotnet-issue-labeler[bot] commented 2 years ago

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.

ghost commented 2 years ago

Tagging subscribers to this area: @ajcvickers, @bricelam, @roji See info in area-owners.md if you want to be subscribed.

Issue Details
### Description We are upgrading our solutions from .NET 5 and .NET 6 and it went well until in one solutions a lot of unittests started failing. It seems that when you use `TryValidateObject` from `System.ComponentModel.DataAnnotations` it won't go through all the (custom) validation attributes of the same type when there are multiple on a property. While debugging in .NET 5 you through the `IsValid` method as many times as you have added the attribute but in .NET 6 it will only call the `IsValid` and it seems to be of the last attribute. ### Reproduction Steps I have tried to make an as minimal repo as possible: .NET 5: https://github.com/Rutix/ValidationTest/blob/main/ValidationTest.Unittest/UnitTest1.cs Running this will pass the test. .NET 6: https://github.com/Rutix/ValidationTest/blob/net6/ValidationTest.Unittest/UnitTest1.cs Running this will fail the test. ### Expected behavior The expected behavior (unless i missed a breaking change) would be the behavior which we get in .NET 5. ### Actual behavior IsValid isn't called on all the attributes of the same type. ### Regression? Worked in .NET 5 ### Known Workarounds _No response_ ### Configuration _No response_ ### Other information _No response_
Author: Rutix
Assignees: -
Labels: `area-System.ComponentModel.DataAnnotations`, `untriaged`
Milestone: -
StacyCash commented 2 years ago

Is there any information on this - it's a showstopper for us at the moment 😭

ajcvickers commented 2 years ago

@bricelam Can you investigate this? Could be around the validation changes we made.

KalleOlaviNiemitalo commented 2 years ago

The second attribute with the same type is already missing from PropertyDescriptor.Attributes:

foreach (PropertyDescriptor prop in TypeDescriptor.GetProperties(typeof(Foobar)))
{
    Console.WriteLine(prop.Name);
    Console.WriteLine(prop.Attributes.Count);
    foreach (Attribute attr in prop.Attributes)
    {
        Console.WriteLine(attr);
    }
}

And this happens even if the attribute type overrides Attribute.Match to compare by reference equality.

KalleOlaviNiemitalo commented 2 years ago

But if the attribute type has

public override object TypeId => this;

then PropertyDescriptor.Attributes preserves both attributes, and validation uses both attributes.

Attribute instances with duplicate Attribute.TypeId values are discarded by MemberDescriptor.FilterAttributesIfNeeded: https://github.com/dotnet/runtime/blob/4822e3c3aa77eb82b2fb33c9321f923cf11ddde6/src/libraries/System.ComponentModel.TypeConverter/src/System/ComponentModel/MemberDescriptor.cs#L359-L377

I don't know what the intended behaviour is -- how does this work in .NET Framework? Anyway, one can work around the issue by overriding TypeId. Except maybe that will also prevent the attributes from hiding similar attributes that were inherited from the base class or attached to the type of the property.

KalleOlaviNiemitalo commented 2 years ago

.NET Framework 4.8 likewise discards one of the duplicate attributes, if the attribute type does not override TypeId. Also, the .NET Framework implementation of System.ComponentModel.DataAnnotations.CustomValidationAttribute does override TypeId, so this seems something that custom validation attributes are expected to do.

I think the behavior is by design, then.

Rutix commented 2 years ago

What was the intended behavior in .NET 3.1 / .NET 5 since it worked there. I will try your workaround to see if that helps any further.

KalleOlaviNiemitalo commented 2 years ago

The behaviour change was likely caused by https://github.com/dotnet/runtime/pull/51772, which made ValidationAttributeStore.GetTypeStoreItem(Type type) use TypeDescriptor.GetAttributes (which lets MemberDescriptor.FilterAttributesIfNeeded discard duplicate attributes by TypeId) rather than CustomAttributeExtensions.GetAttributes (which doesn't).

Because using TypeDescriptor was necessary for fixing https://github.com/dotnet/runtime/issues/46678, and the behaviour now matches .NET Framework, I think it's unlikely to be changed back.

KalleOlaviNiemitalo commented 2 years ago

Perhaps an analyzer should be added, though. Warn if an attribute type is (indirectly) derived from ValidationAttribute and has AllowMultiple = true and does not override Attribute.TypeId.

Rutix commented 2 years ago

I will wait for either @@ajcvickers, @bricelam, @roji to confirm this behavior is staying. If it's staying an analyzer and docs update seems to be wise to do. This is totally unexpected behavior if you don't know this case :(

WT-Solutions commented 2 years ago

We ran into this issue as well (see #64672) and we noticed that the validation using this Validator is different than the one that is used during model binding in MVC. There both attributes are evaluated. So from that point of view I think this is still a bug rather than a design decision.

y-vitly commented 2 years ago

Hi, is there any updates on this issue?

teo-tsirpanis commented 2 years ago

@ajcvickers I think the Servicing-Consider label applies to PRs. To get a fix for this issue reviewed for servicing, a PR has to be opened with this label.