Remove RequestDelegateFactory call from RouteEndpointBuilder

[halter73]

• Remove ModelEndpointDataSource. Use RouteEndpointDataSource instead
• Filters will no longer run for razor pages due to this change.

This is somewhat dependent on #42557 to ensure we have a nice experience where "endpoint" filters run on both route handlers, normal RequestDelegates and MVC actions.

Fixes #42703

[davidfowl]

I think we want to add this in either case so open api works.

[captainsafia]

I think adding it only for route handlers will actually help with our goal of making WithOpenApi something you can only apply on RouteHandlerBuilder types even though we removed that constraint. AFAIK, non-route handler scenarios don't need access to the method info.

[halter73]

That's the question. Do we want OpenAPI for these endpoints or not? What if someone manually adds [Consumes] or [Produces] metadata to these endpoints? For now, I'm trying to keep this behavior consistent with the behavior we've already have.

[davidfowl]

I think we should unify these endpoints mostly because of the amount of bugs where people end up calling the RequestDelegate overload and it things stop working.

[davidfowl]

Breaking change?

[halter73]

Technically.

You'd had to have defined a custom attribute that implemented IHttpMethodMetadata which no one on grepp.app has other than us in this very test, in one other test which notably didn't need to be changed because it wasn't overriding anything, and in .NET 6 preview diffs before I removed it from the MVC attributes. We certainly don't ship any such attributes.

Plus, we already made this "breaking" change for route handlers between .NET 6 and .NET 7 with #40926. See https://github.com/dotnet/aspnetcore/pull/40926/files#diff-f6727c82b3f6cff19c30c9c793f3e47c1c2ff2035a6453198ff4cedad8e84557

I almost went the other way and had RouteEndopintDataSource copy the old .NET 6 behavior here so we didn't "break" anything for either Delegate or RequestDelegate, but it makes sense to configure metadata added implicitly by the MapXxx call like the MethodInfo and HttpMethodMetadata before any additional metadata configured by the user.

[javiercn]

Filters will no longer run for razor pages due to this change.

This might be problematic as it will break people's expectations that these filters run in both cases like other MVC filters. Is there a specific reason to prevent these filters from applying to pages?

[halter73]

Filters will no longer run for razor pages due to this change.

This might be problematic as it will break people's expectations that these filters run in both cases like other MVC filters. Is there a specific reason to prevent these filters from applying to pages?

This is my position as well. The problem is the way they were running in earlier previews is over the RequestDelegate (i.e. Func<HttpContext, Task> for the Endpoints create for razor pages, and did not provide full fidelity MethodInfos or invocation contexts the way other MVC filters do.

The same is true right now for MVC actions (until after this change merged after which those too will be skipped). @davidfowl opened a draft PR (#42557) to run these filters the way our customers expect for MVC actions. I'm @captainsafia is going to take this over since he's on vacation.

We wanted to include razor pages in this PR as well, but the problem is that razor pages can effectively have multiple MethodInfos per endpoint, and that doesn't fit well with our new filter abstraction. We hope to resolve this issue and run filters razor pages in a future release, but we don't know that we can do that in time for .NET 7. We've been considering whether we can "light up" filters for razor pages in future releases without breaking apps.

[halter73]

I updated this PR to do the renames suggested in #43000 and #42592. I assigned both of these to myself.

I also reacted to @davidfowl's and @captainsafia's feedback about adding the MethodInfo to the endpoint metadata for old-fashioned RequestDelegate-defined endpoints. This will make it show up both with in the .WithOpenApi() output and the ApiExplorer stuff we shipped in .NET 6 used by things like swashbuckle and nswag.

Are we sure want this to affect ApiExplorer too? I like that this keeps things consistent with .WithOpenApi() but we'll certainly want to make a breaking change announcement.

[halter73]

Are we sure want this to affect ApiExplorer too?

Even if we do agree this is the direction we should take, I think we need an easy way to opt out of this globally for people who might not want this behavior but want to easily upgrade their existing apps using something like swashbuckle or nswag to .NET 7.

Applications might call into libraries that call MapXXX internally making adding metadata to these endpoints to the description might be tricky. Doing something like changing app.MapLibraryEndpoints() to app.MapGroup("").ExcludeFromDescription().MapLibraryEndpoints() might work. If .MapLibraryEndpoints() already returns an IEndpointConventionBuilder, you wouldn't even need the group with an empty prefix.

We might want to reopen #34068 and add an IgnoreApi() extension method that works for both ApiExplorer and WithOpenApi(). @martincostello

Edit: Updated to reflect ExcludeFromDescription() exists and is respected by both ApiExplorer and WithOpenApi. I knew we must have something.

[halter73]

I think I've addressed all the PR feedback.

I undid the renames to reduce merge conflicts in #42557. The renames weren't the point of this PR. I thought I was saving work, but I wasn't. We also want to expose the EndopintBuilder in more places (see #43000 (comment)). This will be done in another PR.