Add generic variants of MVC attributes

[captainsafia]

Closes #37767.

Since we have derived classes, the behavior of GetCustomAttribute is now different. I've updated the impacted codepaths that I could find.

I didn't replace all instances of ServiceFilter and TypeFilter in our tests because some of them don't reference types that actually implement the IFilterMetadata interface (example). These tests aren't currently failing because they don't actually instantiate the factories.

[JamesNK]

The new attributes are basically the same, e.g. [ModelMetadataType(typeof(DerivedModel))] vs [ModelMetadataType<DerivedModel>] - but it would be good to still have tests that using typeof(XXX) works - realistically existing MVC code that uses the non-generic attributes will always be the majority of users. Are there still some left or have the tests all been updated to be generic?

I'd like a test of multiple attributes, e.g. [ModelMetadataType(typeof(DerivedModel))] and [ModelMetadataType<DerivedModel>], and verify the error message is descriptive.

[amcasey]

LGTM, modulo @JamesNK's suggestions.

[captainsafia]

The new attributes are basically the same, e.g. [ModelMetadataType(typeof(DerivedModel))] vs [ModelMetadataType] - but it would be good to still have tests that using typeof(XXX) works - realistically existing MVC code that uses the non-generic attributes will always be the majority of users. Are there still some left or have the tests all been updated to be generic?

Yes, there are integration tests that still use the existing model (https://github.com/dotnet/aspnetcore/blob/fd211fb22e07f8b03e72d64b50117ba5d4ddf9a4/src/Mvc/test/Mvc.IntegrationTests/Models/SoftwareViewModel.cs). Also, I think we have decent coverage for it given that we call into the base constructor.

I'd like a test of multiple attributes, e.g. [ModelMetadataType(typeof(DerivedModel))] and [ModelMetadataType], and verify the error message is descriptive.

Done. Thoughts?

[JamesNK]

Great!

This covers ModelMetadataType. Are there other attributes that had AllowMultiple=false?

[captainsafia]

ModelBinder has it, but it's behavior is a little interesting. When constructing the model-binding context, we capture all the attributes on a type, including inherited ones (ref). When constructing the actual model binding context, we pick the attribute closest to the type were evaluating and use its properties to construct the context (ref).

So unlike the ModelMetadata scenario, there isn't a place in code where we try to evaluate a single attribute on a type. We could modify the binding logic above to throw if there are multiple attributes on the same type. That would introduce a new exception in the code (although it's not possible to hit that exception without this current change).

Ditto the same situation for ProducesAttribute. Although we only expect one attribute on a particular type, all the codepaths that consume it assume that attributes can be inherited and don't strictly expect a single attribute to be defined.

So we can throw exceptions at runtime that warn about multiple attributes on the same type but that feels a little heavy handed given the existing behavior for these attributes.

Thoughts?

[ghost]

Looks like this PR hasn't been active for some time and the codebase could have been changed in the meantime.
To make sure no breaking changes are introduced, please leave an /azp run comment here to rerun the CI pipeline and confirm success before merging the change.

[captainsafia]

Bump! Reviving this for preview5.

[marchy]

Stoked! The typeof( .. ) was always such a clunky add-on that added noise to all our authorized routes.

Great work team!

[ghost]

Hi @marchy. It looks like you just commented on a closed PR. The team will most probably miss it. If you'd like to bring something important up to their attention, consider filing a new issue and add enough details to build context.