Add new ModelMetadata constructor for EndpointMetadataApiDescriptionProvider

[captainsafia]

Background and Motivation

Background on this issue can be found in this comment.

Proposed API

// Assembly: Microsoft.AspNetCore.Mvc.Abstractions;
namespace Microsoft.AspNetCore.Mvc.ModelBinding;

public abstract class ModelMetadata : IEquatable<ModelMetadata?>, IModelMetadataProvider
{
+	protected ModelMetadata(Type type, IParameterBindingMetadata? parameterBindingMetadata)
}

Usage Examples

This API is primarily intended for consumption within the EndpointMetadataApiDescriptionProvider to construct the ModelMetadta implementation that is a required part of ApiExplorer metadata.

aspnetcore/src/Mvc/Mvc.Abstractions/src/ApiExplorer/ApiParameterDescription.cs

Lines 7 to 17 in a21c9ed

	namespace Microsoft.AspNetCore.Mvc.ApiExplorer;
	/// <summary>
	/// A metadata description of an input to an API.
	/// </summary>
	public class ApiParameterDescription
	{
	/// <summary>
	/// Gets or sets the <see cref="ModelMetadata"/>.
	/// </summary>
	public ModelMetadata ModelMetadata { get; set; } = default!;

For the minimal API-based implementaton of ApiDescriptionProvider, there is a custom EndpointModelMetadtata class that extends the abstract ModelMetadata class that we are modifying:

aspnetcore/src/Mvc/Mvc.ApiExplorer/src/EndpointModelMetadata.cs

Lines 12 to 18 in 6812d1d

	internal sealed class EndpointModelMetadata : ModelMetadata
	{
	public EndpointModelMetadata(Type type, IParameterBindingMetadata? parameterBindingMetadata) : base(type, parameterBindingMetadata)
	{
	IsBindingAllowed = true;
	}

For a complete example of how these APIs are used, see aa3ac70.

Alternative Designs

• Implement ApiExplorer abstraction layer with no MVC dependencies 😛
• Cry 😭
• Shift ModelMetadatas use of ParameterBindingCache to the concrete DefaultModelMetadata implementation

Risks

The current shape of this change introduces a regression for scenarios where we generate ModelMetadata for responses or request bodies annotated with Accepts. Unlike parameters, where we can take advantage of the IParameterBindingMetadata that we introduced earlier, there is no similar abstraction layer for response types/accepts metadata. This means that EndpointModelMetadata generated for these will not have information about whether a type is parseable or complex.

We could solve this in one of two ways:

• Introduce metadata similar to IParameterBindingMetadata for responses and accepts metada and introduce new constructs on ModelMetadata that consume this metadata
• Call it what it is and document this is a breaking change in the API

I lean slightly towards the later option here -- it's cheaper and we can always revisit and approach #2 if we discover that there were libraries that were taking advantage of ModelMetadata for responses in the minimal API-based API explorer.

[KennethHoff]

Implement ApiExplorer abstraction layer with no MVC dependencies 😛

How difficult would this be? I assume "quite" because it feels like a bad/unnecessary coupling. Feels like this should be the opposite; MVC depended on ModelMetadata.

It does feel like a fairly extreme alternative over simply creating a new protected constructor however.

[martincostello]

It's probably also far too late in the release cycle for that 😄

[captainsafia]

How difficult would this be? I assume "quite" because it feels like a bad/unnecessary coupling. Feels like this should be the opposite; MVC depended on ModelMetadata.

I've actually done some thinking about this in the past. ApiExplorer is a really useful concept but the fact that it's tied to MVC's APIs limits its potential and keeps it from being completely useable as an intermediary abstraction layer for minimal APIs and other open-source API frameworks on top of ASP.NET Core.

The easy part of the work around this is the API review required to examine the pre-existing types in ApiExplorer and figure out implementation-neutral variants of them. The hard part is replatting everything on top of those new APIs.

It's one of those things that always slips below the cost/benefit threshold, IMO. More people would need to be taking advantage of IApiDescriptionProvider abstractions. There might be a future where the cost/benefit threshold changes and a neutral implementation becomes more compelling.

[amcasey]

Is this the actual call?

[amcasey]

[API Review]

• The breaking change is not in the API - it's in the behavior changes that motivate this API change
  • We want the changes to improve trimming support
  • The break will affect non-trimming scenarios
• Affected properties
  • IsParsable - all scenarios, impacts types where we don't have parameter binding metadata (responses and things added via Accepts annotations)
  • IsCollection - only breaking in trimming scenarios
  • IsEnumerable - only breaking in trimming scenarios
    • Implies published app doesn't behave the same as the unpublished app
• Not presently planning changes that benefit non-parameters (since we don't already have metadata types for them)
• Can we somehow exempt MVC, which we're not trying to make trim-safe?
  • The behavior change is behind a feature switch (which is coupled to trimming), so you can imagine not applying it in MVC scenarios

	MVC	Minimal
Trimmed	C,E	P,C,E
Untrimmed		P,C,E

• Note: Trimmed MVC isn't actually a supported scenario
• As far as we can tell, nswag and swashbuckle will be unaffected
• Alternative: could we have a ctor that leaves the base type uninitialized?
  • We wouldn't need to incorporate derived type implementation details
  • The exact properties that we care about aren't virtual
• Can we make some or all of initialization virtual?
• In any of these alternative approaches, we need to ensure that certain key properties are still initialized (e.g. IsComplexType)
• Alternative: could accept the important property values as parameters
• Trimming and non-trimming should be consistent - if the behavior isn't acceptable, maybe we should defer trimmability
• The definition if IsParsable needs to be confirmed
• We think the presence of TryParse might exactly correspond to whether it is called in trimming scenarios, but we're not certain
• Could we expose an explicit property indicating whether or not trimming has occurred?
• Proposal: ctor parameter to disable some initialization
  • Always set for Minimal
  • Never set for MVC
• Returning a bad value seems suboptimal - can we throw?
  • Seems doable - we'll have a flag to control this
  • Alternatively: we could pass in (or virtualize) correct values
• We could make IsParseableType internal protected
  • This wouldn't work for IsCollection and IsEnumerable
    • We'll just throw
• What do we call the flag that limits the functionality?
  • We don't like "trimSafe" because it sounds desirable and we don't want people to use this
• There seem to be some other reflection calls in ModelMetadata - will they impact trimming?

namespace Microsoft.AspNetCore.Mvc.ModelBinding;
 
public abstract class ModelMetadata
{
+     protected ModelMetadata(ModelMetadataIdentity identity, bool disableDynamicDependencies)

-     internal virtual bool IsParseableType { get; private set; }
+     protected internal virtual bool IsParseableType { get; private set; }
}

Revised API approved!

[captainsafia]

Closing....ended up going with another approach here that did not require new API.