Add validation for MetadataType attribute - Issue #46678 #51772

Xaeco · 2021-04-24T02:07:27Z

If a MetadataType attribute is applied to a class the associated metadata class is now validated in .NET Core.

In .NET Framework 4.8 and older versions a similar behavior already exists if you use associate a type and handler like so:
TypeDescriptor.AddProviderTransparent( new AssociatedMetadataTypeTypeDescriptionProvider(typeof(Person), typeof(PersonMetadata)), typeof(Person));

To do this the System.ComponentModel.DataAnnotations.ValidationAttributeStore class was altered to check for a MetadataType attribute. If found it inspects the associated class and merges any validation attributes with any from the class being validated.

Added tests to the System.ComponentModel.DataAnnotations.Validator for as many scenarios as I could think of.

Fixes #46678

ghost · 2021-04-24T02:07:34Z

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

Issue Details

Fix for #46678

If MetadataType attribute is applied to a class the associated metadata class is now validated.

To do this the System.ComponentModel.DataAnnotations.ValidationAttributeStore class was altered to check for a MetadataType attribute and add the properties from associated class to the store.

Added tests to the System.ComponentModel.DataAnnotations.Validator for as many scenarios as I could think of.

Author:	Xaeco
Assignees:	-
Labels:	`area-System.ComponentModel.DataAnnotations`
Milestone:	-

dnfadmin · 2021-04-24T02:07:41Z

All CLA requirements met.

Xaeco · 2021-05-08T07:51:15Z

@ajcvickers what is the process and timeline to get someone to review my PR? Also I can't get this to build due to the Helix failures that keep happening. Seems to be a common issue.

Tagging in @lajones

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

Xaeco · 2021-05-28T09:46:13Z

@ajcvickers @eerhardt @lajones when's the next time this can be reviewed ?
Thanks

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

eerhardt · 2021-05-28T14:49:46Z

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

+                    var properties = t.GetProperties()
+                        .Where(prop => !prop.GetIndexParameters().Any());
+
+                    foreach (PropertyInfo property in properties)
+                    {


Suggested change

var properties = t.GetProperties()

.Where(prop => !prop.GetIndexParameters().Any());

foreach (PropertyInfo property in properties)

{

foreach (PropertyInfo property in t.GetProperties())

{

if (property.GetIndexParameters().Length == 0)

{

Should you also be filtering static properties? GetProperties() returns both instance and static properties.

I've incorporated your suggested code. Is the reasoning of the style change to reduce memory and object iteration for performance reasons?

@eerhardt Re static properties: Only public static properties will be processed. This is the same behavior as in .Net Framework. My branch finally got through all the pipelines, yay, progress. Thanks for all your help.

Is the reasoning of the style change to reduce memory and object iteration for performance reasons?

Yes. In general, we have been moving away from using Linq in the BCL. See #44923 #44825 #47496 #47873 #44734

@eerhardt Kewl. No pressure but what's the usual turnaround for PR reviews like this one? Just setting my expectations 😎

I would hope the area owners would review this change. I'm just here reviewing because I was pinged on the issue, and was helping out on the "trim compatibility" parts of it.

@ajcvickers

Thanks. Would it be appropriate for you to assign the review to ajcvickers?

Xaeco · 2021-06-16T20:21:05Z

@ajcvickers can we progress this? I and others have a lot of work with this as a dependency.

Xaeco · 2021-06-25T22:55:45Z

Hi @eerhardt. Trying to progress this before .Net 6 is released 😂. I was wondering if this PR is setup/tagged correctly for the associated bug? Also any other settings, or advice to progress this would be appreciated. Thanks

ajcvickers · 2021-07-12T07:19:05Z

Sorry for being so slow on this. We are normally very hesitant to take changes in this area, especially where the behavior in .NET Framework and .NET Core would diverge. However, in this case we are bringing an existing .NET Framework behavior to .NET Core, which actually helps convergence.

@bricelam I think you have some knowledge in this area. Does the change make sense to you?
@roji Another set of eyes would be good.

roji

As @ajcvickers wrote, apologies for being so slow on this (I've been on vacation in the last three weeks).

I'm unfamiliar with this area, but the main question that comes to mind, is whether we shouldn't simply match the .NET Framework behavior here - more or less by copying the code across - rather than adding the new feature here (as @ajcvickers wrote this area is archived to a large extent). If I understand the situation correctly, this would mean that [MetadataType] wouldn't be respected for validation (it wasn't for .NET Framework), but TypeDescriptor.AddProviderTransparent would.

In any case, deferring to @bricelam (and @ajcvickers) who know this better.

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

bricelam · 2021-07-12T16:04:46Z

+1 for using the .NET Framework code as reference. When all this code came into .NET Core, MetadataTypeAttribute was deliberately excluded (no idea why), and it was later brought in as part of the push to make .NET Framework apps compatible with .NET Core.

Xaeco · 2021-07-16T03:22:42Z

@bricelam . So I'm wondering what the next steps are. I'd like to keep this moving.

Appreciate everyone's feedback, thanks.

roji · 2021-07-16T08:51:27Z

See #51772 (comment)

vitek-karas · 2021-07-20T12:01:11Z

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

+            /// </remarks>
+            /// <param name="propertyDescriptor">The property descriptor whose attributes are needed.</param>
+            /// <returns>A new <see cref="AttributeCollection"/> stripped of any attributes from the property's type.</returns>
+            public static AttributeCollection GetExplicitAttributes(PropertyDescriptor propertyDescriptor)


Why is this method public? It seems to only be used by this class.

🤔 that would be a copy & paste fail from .NET Framework. Changed to internal. Thanks

(nit) Does it even need to be internal? Why not private?

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

...tem.ComponentModel.Annotations/tests/System/ComponentModel/DataAnnotations/ValidatorTests.cs

karelz · 2021-07-21T15:17:39Z

Http3_MsQuic test failures are tracked in #56090. Should be fixed and merged shortly. Sorry for the inconvenience!

eerhardt · 2021-07-22T14:08:32Z

This looks good to me. Thanks @Xaeco for the contribution here.

@roji @bricelam - any more feedback?

Xaeco · 2021-07-22T23:59:46Z

Awesome it's approved! 😀. Thanks everyone. This is my first GitHub PR. Does it get auto-completed & merged or do I need to do something?

ajcvickers · 2021-07-26T08:23:45Z

@bricelam Can you take a final look at this when you get in today?

@danmoseley @Pilchie Does this need to go through Ask Mode to get in for .NET 6?

danmoseley · 2021-07-26T18:27:23Z

@ajcvickers there are no permissions (ever?) needed to fix anything in dotnet/runtime main. However on Aug 17 it will become a 7.0 branch so there would be ceremony if it wasn't in by then.

bricelam · 2021-07-26T22:37:47Z

@Xaeco It looks like GetTypeStoreItem should also use TypeDescriptor to pick up type-level attributes on the buddy type. ~~Wanna make the update and add some tests for this scenario?~~

Fix #46678

bricelam · 2021-07-27T20:07:11Z

Updated to honor class-level attributes. Squashed and rebased on top of main.

ghost · 2021-07-27T20:12:25Z

Hello @bricelam!

Because this pull request has the auto-merge label, I will be glad to assist with helping to merge this pull request once all check-in policies pass.

p.s. you can customize the way I help with merging this pull request, such as holding this pull request until a specific person approves. Simply @mention me (`@msftbot`) and give me an instruction to get started! Learn more here.

bricelam · 2021-07-27T20:22:58Z

Looks like scenarios involving dynamically added properties will also be enabled by this PR.

Xaeco · 2021-07-27T21:53:38Z

@Xaeco It looks like GetTypeStoreItem should also use TypeDescriptor to pick up type-level attributes on the buddy type. ~~Wanna make the update and add some tests for this scenario?~~

@bricelam thanks for making those changes. I started to but got interrupted and should've mentioned I was working on it.

IanKemp · 2021-07-28T19:03:53Z

Thanks guys!

bricelam · 2021-07-28T21:18:42Z

And thank you, community, for making it happen! Things like this that bring parity with .NET Framework make everyone's migration go a little smoother.

Gambero81 · 2021-07-29T01:29:52Z

Glad to find this issue merged in .net6, can be ported to .net5?

roji · 2021-07-29T08:17:27Z

@Gambero81 this is very unlikely to be backported to .NET 5.0; patches are only done for serious bugs for which no workaround is available.

dotnet-issue-labeler bot added the area-System.ComponentModel.DataAnnotations label Apr 24, 2021

eerhardt reviewed May 24, 2021

View reviewed changes

...onentModel.Annotations/src/System/ComponentModel/DataAnnotations/ValidationAttributeStore.cs Outdated Show resolved Hide resolved

eerhardt reviewed May 24, 2021

View reviewed changes

...onentModel.Annotations/src/System/ComponentModel/DataAnnotations/ValidationAttributeStore.cs Outdated Show resolved Hide resolved

eerhardt reviewed May 24, 2021

View reviewed changes

...onentModel.Annotations/src/System/ComponentModel/DataAnnotations/ValidationAttributeStore.cs Outdated Show resolved Hide resolved

eerhardt reviewed May 28, 2021

View reviewed changes

...onentModel.Annotations/src/System/ComponentModel/DataAnnotations/ValidationAttributeStore.cs Outdated Show resolved Hide resolved

eerhardt reviewed May 28, 2021

View reviewed changes

...onentModel.Annotations/src/System/ComponentModel/DataAnnotations/ValidationAttributeStore.cs Outdated Show resolved Hide resolved

eerhardt reviewed May 28, 2021

View reviewed changes

Xaeco requested a review from eerhardt May 29, 2021 08:45

eerhardt assigned ajcvickers Jun 7, 2021

roji reviewed Jul 12, 2021

View reviewed changes

bricelam self-assigned this Jul 12, 2021

terrajobst added the community-contribution Indicates that the PR has been added by a community member label Jul 19, 2021

vitek-karas reviewed Jul 20, 2021

View reviewed changes

Xaeco requested review from vitek-karas and roji July 21, 2021 10:15

eerhardt reviewed Jul 21, 2021

View reviewed changes

...onentModel.Annotations/src/System/ComponentModel/DataAnnotations/ValidationAttributeStore.cs Outdated Show resolved Hide resolved

eerhardt reviewed Jul 21, 2021

View reviewed changes

...tem.ComponentModel.Annotations/tests/System/ComponentModel/DataAnnotations/ValidatorTests.cs Show resolved Hide resolved

Xaeco requested a review from eerhardt July 22, 2021 06:44

eerhardt approved these changes Jul 22, 2021

View reviewed changes

Xaeco and others added 2 commits July 27, 2021 13:05

MetadataType attribute class properties are now validated.

7d67916

Fix #46678

Clean up PR #51772

d5012c6

bricelam approved these changes Jul 27, 2021

View reviewed changes

bricelam added the auto-merge label Jul 27, 2021

ghost merged commit 5d42771 into dotnet:main Jul 28, 2021

This was referenced Aug 2, 2021

[Perf] Regressions in System.Linq.dll #56631

Closed

[Perf] Regressions in System.ComponentModel.TypeConverter.dll, System.ComponentModel.Primitives.dll, and System.Private.Runtime.InteropServices.JavaScript.dll #56563

Closed

ghost locked as resolved and limited conversation to collaborators Aug 28, 2021

This pull request was closed.

Add validation for MetadataType attribute - Issue #46678 #51772

Add validation for MetadataType attribute - Issue #46678 #51772

Conversation

Xaeco commented Apr 24, 2021 • edited by bricelam Loading

ghost commented Apr 24, 2021

dnfadmin commented Apr 24, 2021 • edited Loading

Xaeco commented May 8, 2021 • edited Loading

Xaeco commented May 28, 2021

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

Xaeco commented Jun 16, 2021 • edited Loading

Xaeco commented Jun 25, 2021

ajcvickers commented Jul 12, 2021

roji left a comment • edited Loading

Choose a reason for hiding this comment

bricelam commented Jul 12, 2021

Xaeco commented Jul 16, 2021

roji commented Jul 16, 2021

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

karelz commented Jul 21, 2021

eerhardt commented Jul 22, 2021

Xaeco commented Jul 22, 2021

ajcvickers commented Jul 26, 2021

danmoseley commented Jul 26, 2021

bricelam commented Jul 26, 2021 • edited Loading

bricelam commented Jul 27, 2021

ghost commented Jul 27, 2021

p.s. you can customize the way I help with merging this pull request, such as holding this pull request until a specific person approves. Simply @mention me (@msftbot) and give me an instruction to get started! Learn more here.

bricelam commented Jul 27, 2021

Xaeco commented Jul 27, 2021

IanKemp commented Jul 28, 2021

bricelam commented Jul 28, 2021

Gambero81 commented Jul 29, 2021 • edited Loading

roji commented Jul 29, 2021

Xaeco commented Apr 24, 2021 •

edited by bricelam

Loading

dnfadmin commented Apr 24, 2021 •

edited

Loading

Xaeco commented May 8, 2021 •

edited

Loading

Xaeco commented Jun 16, 2021 •

edited

Loading

roji left a comment •

edited

Loading

bricelam commented Jul 26, 2021 •

edited

Loading

p.s. you can customize the way I help with merging this pull request, such as holding this pull request until a specific person approves. Simply @mention me (`@msftbot`) and give me an instruction to get started! Learn more here.

Gambero81 commented Jul 29, 2021 •

edited

Loading