Skip to content
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.

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

Less allocating IdentityResult.Failed overload #42962

Closed
N0D4N opened this issue Jul 28, 2022 · 1 comment
Closed

Less allocating IdentityResult.Failed overload #42962

N0D4N opened this issue Jul 28, 2022 · 1 comment
Labels
api-suggestion Early API idea and discussion, it is NOT ready for implementation area-identity Includes: Identity and providers

Comments

@N0D4N
Copy link

N0D4N commented Jul 28, 2022

Background and Motivation

IdentityResult provides useful factory Failed method for creating failed IdentityResult, however it allocates array due to it accepting params IdentityError[] errors as argument, even if user passes only 1 error.
My proposal is to add overload to Failed method that will accept one IdentityError that will not allocate array in such case.

Proposed API

namespace Microsoft.AspNetCore.Identity;

public class IdentityResult
{
/*
		 Existing overload
    	 public static IdentityResult Failed(params IdentityError[] errors);
*/
+        public static IdentityResult Failed(IdentityError error);
}

Usage Examples

From user perspective nothing will change, the same method will be called, in the same way, unless they created params array explicitly;

// current
var result = IdentityResult.Failed();
result = IdentityResult.Failed(new IdentityError() // allocates array
					{
						Description = "Error description"
					});
result = IdentityResult.Failed(new IdentityError()
					{
						Description = "Error description"
					}, new IdentityError()
					{
						Description = "Error description2"
					});

// with proposed API
var result = IdentityResult.Failed();
result = IdentityResult.Failed(new IdentityError() // doesn't allocate array
					{
						Description = "Error description"
					});
result = IdentityResult.Failed(new IdentityError()
					{
						Description = "Error description"
					}, new IdentityError()
					{
						Description = "Error description2"
					});

Alternative Designs

Create overload accepting params Span<IdentityError> when/if dotnet/csharplang#1757 is implemented.

Risks

We would need to ensure that behavior of new overload is consistent with current overload, since after recompilation compiler will pick overload without params array, for cases when user calls Failed method with single error passed as argument. And behavior won't change unexpectedly for developer.

@N0D4N N0D4N added the api-suggestion Early API idea and discussion, it is NOT ready for implementation label Jul 28, 2022
@blowdart blowdart added the area-identity Includes: Identity and providers label Jul 28, 2022
@adityamandaleeka
Copy link
Member

Triage: this seems fairly low-value given the amount saved and the fact that it's not in a hot path.

@ghost ghost locked as resolved and limited conversation to collaborators Aug 28, 2022
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
api-suggestion Early API idea and discussion, it is NOT ready for implementation area-identity Includes: Identity and providers
Projects
None yet
Development

No branches or pull requests

3 participants