[API Proposal]: SignOut overloads that take only one authentication scheme

[ghost]

Background and Motivation

When multiple authentication schemes are registered, if we only want to sign out from a particular one it is necessary to allocate a collection. It is not the most efficient in terms of allocation.

aspnetcore/src/Http/Http.Results/src/Results.cs

Line 80 in c343768

public static IResult SignOut(AuthenticationProperties? properties = null, IList<string>? authenticationSchemes = null)

aspnetcore/src/Http/Http.Results/src/TypedResults.cs

Line 84 in c343768

public static SignOutHttpResult SignOut(AuthenticationProperties? properties = null, IList<string>? authenticationSchemes = null)

aspnetcore/src/Mvc/Mvc.Core/src/ControllerBase.cs

Line 2459 in c343768

public virtual SignOutResult SignOut(params string[] authenticationSchemes)

Proposed API

Add overloads that take a single authentication scheme.

namespace Microsoft.AspNetCore.Http;

public static partial class Results
{
+    public static IResult SignOut(AuthenticationProperties? properties = null, string? authenticationScheme = null)
}

public static partial class TypedResults
{
+    public static SignOutHttpResult SignOut(AuthenticationProperties? properties = null, string? authenticationScheme = null)
}

namespace Microsoft.AspNetCore.Mvc;

public abstract partial class ControllerBase
{
+    public virtual SignOutResult SignOut(string authenticationScheme)
}

Usage Examples

app.MapGet("/signout", () =>
{
    return TypedResults.SignOut(authenticationScheme: "Cookies");
});

Risks

Nothing I can see now

[Tratcher]

Another:

aspnetcore/src/Mvc/Mvc.Core/src/SignOutResult.cs

Lines 41 to 42 in c343768

	public SignOutResult(string authenticationScheme)
	: this(new[] { authenticationScheme })

[ghost]

Thank you for submitting this for API review. This will be reviewed by @dotnet/aspnet-api-review at the next meeting of the ASP.NET Core API Review group. Please ensure you take a look at the API review process documentation and ensure that:

• The PR contains changes to the reference-assembly that describe the API change. Or, you have included a snippet of reference-assembly-style code that illustrates the API change.
• The PR describes the impact to users, both positive (useful new APIs) and negative (breaking changes).
• Someone is assigned to "champion" this change in the meeting, and they understand the impact and design of the change.

[halter73]

public static partial class TypedResults
{
+    public static SignOutHttpResult SignOut(AuthenticationProperties? properties = null, string? authenticationScheme = null)
}

We have to be careful with default values. We do not to make TypedResults.SignOut() ambiguous between this and SignOut(AuthenticationProperties? properties = null, IList<string>? authenticationSchemes = null). I would say we could just remove the default values from the existing overload to maintain both source and binary compatibility as recommended in the rosly docs for adding optional parameters in public API, but that doesn't work when the new overload takes a different type string? instead of IList<string>? in the same position.

We'll probably just have to leave out the default values. Which makes me wonder, if we should add just TypedResults.SignOut(params string[] authenticationScheme) similar to ControllerBase.SignOut()

namespace Microsoft.AspNetCore.Mvc;

public abstract partial class ControllerBase
{
+    public virtual SignOutResult SignOut(string authenticationScheme)
}

What's the motivation for this considering the params overload exists? Just to save an array allocation?

[halter73]

API Review Notes:

• We think that the updated code does look nicer than having to allocate an array:

   app.MapGet("/signout", () =>
   {
       return TypedResults.SignOut(authenticationScheme: "Cookies");
   });

  vs.

   app.MapGet("/signout", () =>
   {
       return TypedResults.SignOut(authenticationSchemes: new[] { "Cookies" });
   });

• The API as proposed is a breaking change as noted in my earlier comment, so we cannot accept it as-is.
• We might consider an API proposal that adds TypedResults.SignOut(params string[] authenticationScheme) similar to ControllerBase.SignOut(), but that does not remove the array allocation.
  • However, [Proposal]: Params Collections (VS 17.10, .NET 9) csharplang#7700 may allow us to add non-allocation params overloads in the future.

We tried to come up with an API proposal that would be non breaking. Below is as far as we got, but even this would probably break given a call like TypedResults.SignOut(authenticationSchemes: new List<string> { "Cookies" }) as soon as we add a params string[] authenticationSchemes overload.

namespace Microsoft.AspNetCore.Http;

public static partial class Results
{
+    public static IResult SignOut()
-    public static IResult SignOut(AuthenticationProperties? properties = null, IList<string>? authenticationSchemes = null)
+   public static IResult SignOut(AuthenticationProperties? properties, IList<string>? authenticationSchemes = null)
+    public static IResult SignOut(params string[] authenticationSchemes)
}

public static partial class TypedResults
{
+    public static SignOutHttpResult SignOut()
-     public static SignOutHttpResult SignOut(AuthenticationProperties? properties = null, IList<string>? authenticationSchemes = null)
+    public static SignOutHttpResult SignOut(AuthenticationProperties? properties, IList<string>? authenticationSchemes = null)
+    public static SignOutHttpResult SignOut(params string[] authenticationSchemes)
}

Because of the difficulty in maintaining backward compatibility, we've decided to reject the API.

[martincostello]

I guess C# 12 at least makes it slightly less ugly (and then maybe the compiler can optimise it?):

app.MapGet("/signout", () =>
 {
     return TypedResults.SignOut(authenticationScheme: ["Cookies"]);
 });

[halter73]

I agree it looks a bit better with the C# 12 syntax.

The final iteration at our attempt at a non-breaking change might work if we renamed params string[] authenticationSchemes to something like params string[] authenticationSchemeParams, and I'm sympathetic to wanting TypedResults.SignOut("Cookies") to just work, but I'm not sure it's worth it if we have to add a bunch of new overloads.