Skip to content

Fix overload resolution issue for Delegate and RequestDelegate #60593

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Closed
wants to merge 3 commits into from
Closed

Fix overload resolution issue for Delegate and RequestDelegate #60593

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
3 commits
Select commit Hold shift + click to select a range
9571d6a
Fix RequestDelegate being chosen over Delegate in overload resolution
bjornen77 Feb 24, 2025
e23360b
fix failing tests
bjornen77 Feb 24, 2025
011305b
fix failing tests
bjornen77 Feb 24, 2025
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
8 changes: 8 additions & 0 deletions src/Http/Routing/src/Builder/EndpointRouteBuilderExtensions.cs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -2,6 +2,7 @@
// The .NET Foundation licenses this file to you under the MIT license.

using System.Diagnostics.CodeAnalysis;
using System.Runtime.CompilerServices;
using Microsoft.AspNetCore.Http;
using Microsoft.AspNetCore.Routing;
using Microsoft.AspNetCore.Routing.Patterns;
Expand Down Expand Up @@ -227,6 +228,7 @@ static RequestDelegateResult CreateHandlerRequestDelegate(Delegate handler, Requ
/// <returns>A <see cref="RouteHandlerBuilder"/> that can be used to further customize the endpoint.</returns>
[RequiresUnreferencedCode(MapEndpointUnreferencedCodeWarning)]
[RequiresDynamicCode(MapEndpointDynamicCodeWarning)]
[OverloadResolutionPriority(1)]
public static RouteHandlerBuilder MapGet(
this IEndpointRouteBuilder endpoints,
[StringSyntax("Route")] string pattern,
Expand All @@ -245,6 +247,7 @@ public static RouteHandlerBuilder MapGet(
/// <returns>A <see cref="RouteHandlerBuilder"/> that can be used to further customize the endpoint.</returns>
[RequiresUnreferencedCode(MapEndpointUnreferencedCodeWarning)]
[RequiresDynamicCode(MapEndpointDynamicCodeWarning)]
[OverloadResolutionPriority(1)]
public static RouteHandlerBuilder MapPost(
this IEndpointRouteBuilder endpoints,
[StringSyntax("Route")] string pattern,
Expand All @@ -263,6 +266,7 @@ public static RouteHandlerBuilder MapPost(
/// <returns>A <see cref="RouteHandlerBuilder"/> that can be used to further customize the endpoint.</returns>
[RequiresUnreferencedCode(MapEndpointUnreferencedCodeWarning)]
[RequiresDynamicCode(MapEndpointDynamicCodeWarning)]
[OverloadResolutionPriority(1)]
public static RouteHandlerBuilder MapPut(
this IEndpointRouteBuilder endpoints,
[StringSyntax("Route")] string pattern,
Expand All @@ -281,6 +285,7 @@ public static RouteHandlerBuilder MapPut(
/// <returns>A <see cref="RouteHandlerBuilder"/> that can be used to further customize the endpoint.</returns>
[RequiresUnreferencedCode(MapEndpointUnreferencedCodeWarning)]
[RequiresDynamicCode(MapEndpointDynamicCodeWarning)]
[OverloadResolutionPriority(1)]
public static RouteHandlerBuilder MapDelete(
this IEndpointRouteBuilder endpoints,
[StringSyntax("Route")] string pattern,
Expand All @@ -299,6 +304,7 @@ public static RouteHandlerBuilder MapDelete(
/// <returns>A <see cref="RouteHandlerBuilder"/> that can be used to further customize the endpoint.</returns>
[RequiresUnreferencedCode(MapEndpointUnreferencedCodeWarning)]
[RequiresDynamicCode(MapEndpointDynamicCodeWarning)]
[OverloadResolutionPriority(1)]
public static RouteHandlerBuilder MapPatch(
this IEndpointRouteBuilder endpoints,
[StringSyntax("Route")] string pattern,
Expand Down Expand Up @@ -338,6 +344,7 @@ public static RouteHandlerBuilder MapMethods(
/// <returns>A <see cref="RouteHandlerBuilder"/> that can be used to further customize the endpoint.</returns>
[RequiresUnreferencedCode(MapEndpointUnreferencedCodeWarning)]
[RequiresDynamicCode(MapEndpointDynamicCodeWarning)]
[OverloadResolutionPriority(1)]
public static RouteHandlerBuilder Map(
this IEndpointRouteBuilder endpoints,
[StringSyntax("Route")] string pattern,
Expand Down Expand Up @@ -413,6 +420,7 @@ public static RouteHandlerBuilder MapFallback(this IEndpointRouteBuilder endpoin
/// </remarks>
[RequiresUnreferencedCode(MapEndpointUnreferencedCodeWarning)]
[RequiresDynamicCode(MapEndpointDynamicCodeWarning)]
[OverloadResolutionPriority(1)]
public static RouteHandlerBuilder MapFallback(
this IEndpointRouteBuilder endpoints,
[StringSyntax("Route")] string pattern,
Expand Down
3 changes: 2 additions & 1 deletion src/Http/Routing/src/Builder/FallbackEndpointRouteBuilderExtensions.cs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -4,6 +4,7 @@
using System.Diagnostics.CodeAnalysis;
using Microsoft.AspNetCore.Http;
using Microsoft.AspNetCore.Routing;
using Microsoft.AspNetCore.Routing.Patterns;

namespace Microsoft.AspNetCore.Builder;

Expand Down Expand Up @@ -74,7 +75,7 @@ public static IEndpointConventionBuilder MapFallback(
ArgumentNullException.ThrowIfNull(pattern);
ArgumentNullException.ThrowIfNull(requestDelegate);

var conventionBuilder = endpoints.Map(pattern, requestDelegate);
var conventionBuilder = endpoints.Map(RoutePatternFactory.Parse(pattern), requestDelegate);
Copy link
Member

@halter73 halter73 Feb 25, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This appears to be related to #44970.

Is passing a RoutePattern instead of a string for the first argument the only way to force the compiler to select the RequestDelegate over the Delegate overload now? That doesn't feel very discoverable.

I see why you needed to do this. There are no RequiresUnreferencedCode or RequiresDynamicCode attributes currently on this method, and it'd be breaking to add them now. Unfortunately, for the exact same reason, it's a breaking change to prefer the Delegate overloads in cases where we were using RequestDelegate before.

It's true it's not binary breaking and rebuilding would present you with new IL2026 and IL3050 warnings, but I don't think it's worth it to even take a source break despite the warnings. The RequestDelegate overloads are older and simpler. And I think the analyzer introduced by #44393 catches the worst issues caused when the RequestDelegate overload gets selected for a Task<T>-returning endpoint.

Not everyone needs or wants the extra OpenAPI support added by the Delegate overloads. Those that do can easily opt in by casting the route handler to Delegate if need be. And everyone else that has been using the RequestDelegate overload since 3.0 just fine without need for the extra features supported by the Delegate overload can easily continue to do so without fighting IL2026 and IL3050 warnings.

Copy link
Contributor Author

@bjornen77 bjornen77 Feb 25, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

"Is passing a RoutePattern instead of a string for the first argument the only way to force the compiler to select the RequestDelegate over the Delegate overload now? That doesn't feel very discoverable."

Yes, I think so, as far as I know. I'm not entirely sure how the overload resolution works in every scenario.
According to VS intellisence, before the fix, the following examples used the RequestDelegate:

        builder.MapGet("/withoutPR1", async context => await Task.FromResult("RequestDelegate overload is used"));
        builder.MapGet("/withoutPR2", async (HttpContext context) => await Task.FromResult("RequestDelegate overload is used"));

After the PR, only the version that explicitly includes HttpContext uses the new Delegate overload:

        builder.MapGet("/withPR1", async context => await Task.FromResult("RequestDelegate overload is used"));
        builder.MapGet("/withPR2", async (HttpContext context) => await Task.FromResult("Delegate overload is used"));

"I think the analyzer introduced by #44393 catches the worst issues caused when the RequestDelegate overload gets selected for a Task-returning endpoint."
Yes, I think so too. It does seem to catch most cases, but for some reason, it doesn’t flag the ones above.

"It's true it's not binary breaking and rebuilding would present you with new IL2026 and IL3050 warnings,
but I don't think it's worth it to even take a source break despite the warnings."

I agree. This change only makes the experience better for new projects using minimal APIs.
As you mentioned, there are breaking changes for older code.
I’m totally fine with dropping this PR if it’s not worth the trade-off—I mainly wanted to explore whether the benefits outweighed the drawbacks.

conventionBuilder.WithDisplayName("Fallback " + pattern);
conventionBuilder.Add(b => ((RouteEndpointBuilder)b).Order = int.MaxValue);
conventionBuilder.WithMetadata(FallbackMetadata.Instance);
Expand Down
3 changes: 2 additions & 1 deletion src/Http/Routing/src/ShortCircuit/RouteShortCircuitEndpointRouteBuilderExtensions.cs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -3,6 +3,7 @@

using Microsoft.AspNetCore.Builder;
using Microsoft.AspNetCore.Http;
using Microsoft.AspNetCore.Routing.Patterns;

namespace Microsoft.AspNetCore.Routing;

Expand Down Expand Up @@ -34,7 +35,7 @@ public static IEndpointConventionBuilder MapShortCircuit(this IEndpointRouteBuil
{
route = $"{routePrefix}/{{**catchall}}";
}
group.Map(route, _shortCircuitDelegate)
group.Map(RoutePatternFactory.Parse(route), _shortCircuitDelegate)
.ShortCircuit(statusCode)
.Add(endpoint =>
{
Expand Down
3 changes: 2 additions & 1 deletion src/Middleware/HealthChecks/src/Builder/HealthCheckEndpointRouteBuilderExtensions.cs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -4,6 +4,7 @@
using System.Diagnostics.CodeAnalysis;
using Microsoft.AspNetCore.Diagnostics.HealthChecks;
using Microsoft.AspNetCore.Routing;
using Microsoft.AspNetCore.Routing.Patterns;
using Microsoft.Extensions.DependencyInjection;
using Microsoft.Extensions.Diagnostics.HealthChecks;
using Microsoft.Extensions.Options;
Expand Down Expand Up @@ -68,6 +69,6 @@ private static IEndpointConventionBuilder MapHealthChecksCore(IEndpointRouteBuil
.UseMiddleware<HealthCheckMiddleware>(args)
.Build();

return endpoints.Map(pattern, pipeline).WithDisplayName(DefaultDisplayName);
return endpoints.Map(RoutePatternFactory.Parse(pattern), pipeline).WithDisplayName(DefaultDisplayName);
}
}
5 changes: 3 additions & 2 deletions src/SignalR/common/Http.Connections/src/ConnectionEndpointRouteBuilderExtensions.cs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -7,6 +7,7 @@
using Microsoft.AspNetCore.Http.Connections.Internal;
using Microsoft.AspNetCore.Http.Timeouts;
using Microsoft.AspNetCore.Routing;
using Microsoft.AspNetCore.Routing.Patterns;
using Microsoft.Extensions.DependencyInjection;

namespace Microsoft.AspNetCore.Builder;
Expand Down Expand Up @@ -99,7 +100,7 @@ public static ConnectionEndpointRouteBuilder MapConnections(this IEndpointRouteB
app.Run(c => dispatcher.ExecuteNegotiateAsync(c, options));
var negotiateHandler = app.Build();

var negotiateBuilder = endpoints.Map(pattern + "/negotiate", negotiateHandler);
var negotiateBuilder = endpoints.Map(RoutePatternFactory.Parse(pattern + "/negotiate"), negotiateHandler);
conventionBuilders.Add(negotiateBuilder);
// Add the negotiate metadata so this endpoint can be identified
negotiateBuilder.WithMetadata(_negotiateMetadata);
Expand All @@ -111,7 +112,7 @@ public static ConnectionEndpointRouteBuilder MapConnections(this IEndpointRouteB
app.Run(c => dispatcher.ExecuteAsync(c, options, connectionDelegate));
var executehandler = app.Build();

var executeBuilder = endpoints.Map(pattern, executehandler);
var executeBuilder = endpoints.Map(RoutePatternFactory.Parse(pattern), executehandler);
executeBuilder.WithMetadata(new DisableRequestTimeoutAttribute());
conventionBuilders.Add(executeBuilder);

Expand Down
Loading