Support multiple calls to WithTags in WithOpenApi

[halter73]

Currently, if you call .WithTags() multiple times on an endpoint, only the last call shows up in the generated OpenApiOperation. This isn't a big deal today because you can just call .WithTags() with multiple parameters, but I imagine this will be more important once we add the group support described in #41428.

[captainsafia]

I'm generally fine with this although this change in behavior does prompt a consideration on naming.

With the advent of AddFilter, we set a standard that the WithX model would be used for metadata that could only be set once and the AddX model would be used with metadata that was additive and didn't follow the "last in wins" principle of other metadata.

Accounting for this API note does make the change a little bit bigger but has the benefit of being consisting with our naming philosophy elsewhere (although AddFilter is the only real place where it applies at the moment).

[halter73]

With the advent of AddFilter, we set a standard that the WithX model would be used for metadata that could only be set once and the AddX model would be used with metadata that was additive and didn't follow the "last in wins" principle of other metadata.

This is a really good point. I agree we should do something before .NET 7 to be more consistent here. Unfortunately, WithTags already shipped in .NET 6, so we cannot rename it to AddTags. We could add an AddTags variant since we're adding overloads for group support anyway. But I would still want WithTags to be additive in that case, and I don't really see the benefit in a new variant with a different name that does exactly the same thing.

Even though the other WithX IEndpointConventionBuidler extension methods typically follow the "last in wins" pattern (with the notable exception of WithMetadata itself), nothing about "With" indicates it replaces instead of adds. For example, CorsPolicyBuilder.WithOrigins/WithMethods/WithHeaders are all additive. That's just a convention we decided on in this release.

When we originally considering the AddFilter name, I thought WithFilter better aligned with the existing IEndpointConventionBuidler extension methods but was talked into AddFilter because most of the other existing With methods are indeed last in wins. Now considering the WithTags and WithMetadata counterexamples, I'm back to thinking we should name the API WithFilter.

AddX methods are more closely associated with IServiceCollection extension method anyway, so seeing it in a different context could be a little confusing. With methods are strongly associated with modifying endpoint metadata unlike Add methods. If a With method can reasonably be interpreted additively like WithTags, I think it needs to add instead of replace metadata for the groups use case. If you really need to, you can remove items from EndpointBuilder.Metadata.

[captainsafia]

For example, CorsPolicyBuilder.WithOrigins/WithMethods/WithHeaders are all additive. That's just a convention we decided on in this release.

Ah, I wasn't think about this precedence when adding the AddFilter API. Given it exists, I'm fine renaming the API to WithFilters. However, this essentially means that there is no way to disambiguate on how a modifying method operates (additive vs. "last-in-wins"). This is generally fine, although the filters scenario is unique because it is additive and ordering matters and I felt that the Add prefix communicated this well. So maybe the principal is:

• With for metadata that is additive or follows the "last-in-wins" principle
• Add for metadata that is additive and where the order of invocation/insertion matters

[JamesNK]

I've been thinking about this change some more. Do you want tags to always be additive? For example, will there ever be a situation where a controller or group specifies tags with "WithTags", and actions/APIs then replace the tags? With this change the tags will always be merged together. An action/API can never replace its parent's tags.

[halter73]

This is a problem. I want to be able to say that EndpointBuilder.Metadata is mutable so extension methods applied to a single endpoint could just remove any tags added earlier by the group.

Unfortunately, even though groups add metadata earlier in the list sequentially than metadata added by the specific endpoint, the endpoint-specific metadata is added first temporally. I called out exactly this problem when I added MapGroup because the inability to distinguish overriding vs adding is annoying.

aspnetcore/src/Http/Routing/src/RouteGroupBuilder.cs

Lines 107 to 115 in c08123a

	// Any metadata already on the RouteEndpoint must have been applied directly to the endpoint or to a nested group.
	// This makes the metadata more specific than what's being applied to this group. So add it after this group's conventions.
	//
	// REVIEW: This means group conventions don't get visibility into endpoint-specific metadata nor the ability to override it.
	// We should consider allowing group-aware conventions the ability to read and mutate this metadata in future releases.
	foreach (var metadata in routeEndpoint.Metadata)
	{
	routeEndpointBuilder.Metadata.Add(metadata);
	}

No one really discussed this in the PR though, and I'm not sure if there's a good way to improve this. Do you have any ideas @JamesNK? Do you think we should add API for extension methods that are group-aware so they can override things like a groups tags?

[halter73]

Another option might be to add something like bool ITagsMetadata.OverridePreviousTags { get; } and have it default to false. That would require all the consumers to know about it, but I think that's okay. We weren't even looking at multiple pieces of metadata until recently.

[JamesNK]

I haven't looked closely at what you're doing in Minimal APIs but I would have expected that group/endpoint would behave the same as controllers/actions. Devs can apply metadata on a controller and it's automatically used by all actions, but actions can override if they choose to, e.g. a [Consumes("application/json", "application/xml")] can be placed on a controller and all its endpoints also have it, but an action can have [Consumes("application/ini")]. The types aren't combined, the action metadata takes presidence.

There are very few places where GetOrderedMetadata is used. Generally, metadata that should have multiple values has a collection, and one instance of metadata is used: IAcceptsMetadata, IHttpMethodMetadata, IHostMetadata, etc.

[halter73]

The trickiness with MapGroup is that it defines and IEndpointRouteBuilder that registers itself as a EndpointDataSource of a parent IEndpointRouteBuilder.

Unfortunately, this means that the metadata from the "inner" EndpointDataSource.Endpoints has already been added by endpoint-specific Action<EndpointBuilder> conventions before the groupwide Action<EndpointBuilder> conventions time-wise. Metadata added by the groupwide conventions still get added first to the final Endpoint.Metadata collection returned to the root IEndpointRouteBuilder, but endpoint-specific conventions never "see" the group metadata because of how early they are run.

That's why I suggest adding a new API. For example, EndpointDataSource that are group aware that allow MapGroup to pass in the group conventions to the "inner" EndpointDataSource. Maybe via something like:

public abstract class EndpointDataSource
{
+    IReadOnlyList<Endpoint> GetEndpointsForGroup(RoutePattern prefix, IEnumerable<Action<EndpointBuilder>> conventions);
}

It might be better as an interface so we could easily check which EndpointDataSource implemented the API.

I don't see another great way to let existing conventions "see" metadata from outer groups without updating all the conventions themselves. In practice, I suspect most of the EndpointDataSource implementations are our own, and we can always fallback to the existing MapGroup behavior of reordering metadata for EndpointDataSource implementations that don't implement the new API.

[halter73]

@JamesNK Thanks for looking into this. If you have any more thoughts, lets continue the discussion on this open issue: #41427