Conflict with Microsoft.IdentityModel.Tokens.CollectionUtilities.IsNullOrEmpty extension

[CodeBlanch]

This Microsoft.IdentityModel.Tokens.CollectionUtilities.IsNullOrEmpty extension introduced in the latest version broke one of the codebases I work on because there is a similar extension already present. It was easy enough to fully quality things where we had issues but I thought I would raise the question here: Should CollectionUtilities be internal to Microsoft.IdentityModel.Tokens?

[brentschmaltz]

@CodeBlanch rats.
Did you use the same namespace prefix M.IM.Tokens?

It should have been internal, that is a mistake.
It would now be breaking for us to remove it, can you live with it?

[CodeBlanch]

@brentschmaltz Not exactly. Let's say my extension is:

namespace Company.Extensions
{
    public static class EnumerableExtensions
    {
          public static bool IsNullOrEmpty<T>(this IEnumerable<T> enumerable) { ... }
    }
}

I had some code like...

using Microsoft.IdentityModel.Tokens;
using Company.Extensions;

public class SomeClass
{
    public TokenValidationParameters BuildValidationParameters(ValidationOptions options)
    {
        return new TokenValidationParameters
	{
		ValidAudiences = options.ValidAudiences.IsNullOrEmpty()
			? options.DefaultValidAudiences
			: options.ValidAudiences;
	};
    }
}

In that case, because both IsNullOrEmpty extensions are in scope, the compiler doesn't know which one to pick and it complains.

Fix is just to qualify it like...

using Microsoft.IdentityModel.Tokens;
using Company.Extensions;

public class SomeClass
{
    public TokenValidationParameters BuildValidationParameters(ValidationOptions options)
    {
        return new TokenValidationParameters
	{
		ValidAudiences = EnumerableExtensions.IsNullOrEmpty(options.ValidAudiences)
			? options.DefaultValidAudiences
			: options.ValidAudiences;
	};
    }
}

Easy enough workaround, no big deal to keep it like that.

[brockallen]

It should have been internal, that is a mistake.

Too bad this wasn't addressed when you could. We're now seeing conflicts as well. Why can't you make this internal?

[aaron-meyers]

@brentschmaltz can you please reconsider leaving this public? We just wasted a couple days hunting down build issues in Power BI which ended up being related to accidental use of this extension method. Your assembly really should not be exposing an extremely generic and easily misused extension like this. Because of the signature, this extension method even shows up on standard strings where it really should not be used in place of the static string.IsNullOrEmpty method.

Components can introduce breaking changes with a major version bump. I don't think leaving this public is the right tradeoff - better to rip off the band-aid and prevent future misuse of this extremely broad extension method.

[brentschmaltz]

@aaron-meyers i reopened so we can reconsider.

[Dreamescaper]

Personally I've hoped it would be "internalized" for v7 :(

[brentschmaltz]

@brockallen i agree, would have been nice to fix this before our 7 release.
We can't get it all done, our focus in 7 was on enabling AOT and improving perf.

[Kebechet]

Will this be a part of v8 ?

[jmprieur]

Closing as there is an easy workaround (fully qualify conflicting methods), and customers are used to using this convenience method now.
Ideally this should be in the .NET Framework

cc: @eerhardt FYI

[Dreamescaper]

Please consider reopening it.
I understand not wanting to fix it now, since it's a breaking change, but would be really nice to have it removed in the next major version.

[Kebechet]

@jmprieur I agree with @Dreamescaper this extension method is PITA. So please in next major version remove this

and customers are used to using this convenience method now.

No, responsibility of this package is not to provide this type of extension methods. So it simply shouldnt be there because for others(us included) it causes other inconveniences.

[Dreamescaper]

@brentschmaltz
I can see that the next major version is in preview now (8.0.0-preview1). Is is possible to include this change there?