Skip to content

Change incorrect usage of ArgumentNullException #48243

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

Merged
merged 10 commits into from
May 19, 2023
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
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
10 changes: 2 additions & 8 deletions src/DataProtection/Extensions/src/DataProtectionProvider.cs
Original file line number Diff line number Diff line change
Expand Up @@ -24,10 +24,7 @@ public static class DataProtectionProvider
/// applications on the machine.</param>
public static IDataProtectionProvider Create(string applicationName)
{
if (string.IsNullOrEmpty(applicationName))
{
throw new ArgumentNullException(nameof(applicationName));
}
ArgumentThrowHelper.ThrowIfNullOrEmpty(applicationName);
Copy link
Member

Choose a reason for hiding this comment

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

is it OK that these are theoretical breaking changes in the empty string case, changing the exception type?

Copy link
Member

Choose a reason for hiding this comment

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

(I assume so, I believe dotnet/runtime does such things.)

Copy link
Member Author

Choose a reason for hiding this comment

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

This is true, although I'm comfortable documenting this as a breaking change and explaining.


return CreateProvider(
keyDirectory: null,
Expand Down Expand Up @@ -74,10 +71,7 @@ public static IDataProtectionProvider Create(
/// <param name="certificate">The <see cref="X509Certificate2"/> to be used for encryption.</param>
public static IDataProtectionProvider Create(string applicationName, X509Certificate2 certificate)
{
if (string.IsNullOrEmpty(applicationName))
{
throw new ArgumentNullException(nameof(applicationName));
}
ArgumentThrowHelper.ThrowIfNullOrEmpty(applicationName);
ArgumentNullThrowHelper.ThrowIfNull(certificate);

return CreateProvider(
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -14,6 +14,7 @@
<Compile Include="..\..\shared\src\*.cs" LinkBase="Shared" />
<Compile Include="$(SharedSourceRoot)TrimmingAttributes.cs" LinkBase="Shared"
Condition="'$(TargetFramework)' != '$(DefaultNetCoreTargetFramework)'" />
<Compile Include="$(SharedSourceRoot)ThrowHelpers\ArgumentThrowHelper.cs" LinkBase="Shared" />
<Compile Include="$(SharedSourceRoot)ThrowHelpers\ArgumentNullThrowHelper.cs" LinkBase="Shared" />
<Compile Include="$(SharedSourceRoot)CallerArgument\CallerArgumentExpressionAttribute.cs" LinkBase="Shared" />
</ItemGroup>
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -36,7 +36,7 @@ public static void StoreTokens(this AuthenticationProperties properties, IEnumer
{
if (token.Name is null)
{
throw new ArgumentNullException(nameof(tokens), "Token name cannot be null.");
throw new ArgumentException("Token name cannot be null for any token.", nameof(tokens));
}

// REVIEW: should probably check that there are no ; in the token name and throw or encode
Expand Down
10 changes: 2 additions & 8 deletions src/Http/Http.Abstractions/src/Internal/ParsingHelpers.cs
Original file line number Diff line number Diff line change
Expand Up @@ -46,11 +46,8 @@ public static StringValues GetHeaderUnmodified(IHeaderDictionary headers, string
public static void SetHeaderJoined(IHeaderDictionary headers, string key, StringValues value)
{
ArgumentNullException.ThrowIfNull(headers);
ArgumentException.ThrowIfNullOrEmpty(key);

if (string.IsNullOrEmpty(key))
{
throw new ArgumentNullException(nameof(key));
}
if (StringValues.IsNullOrEmpty(value))
{
headers.Remove(key);
Expand Down Expand Up @@ -87,11 +84,8 @@ public static void SetHeaderJoined(IHeaderDictionary headers, string key, String
public static void SetHeaderUnmodified(IHeaderDictionary headers, string key, StringValues? values)
{
ArgumentNullException.ThrowIfNull(headers);
ArgumentException.ThrowIfNullOrEmpty(key);

if (string.IsNullOrEmpty(key))
{
throw new ArgumentNullException(nameof(key));
}
if (!values.HasValue || StringValues.IsNullOrEmpty(values.GetValueOrDefault()))
{
headers.Remove(key);
Expand Down
6 changes: 3 additions & 3 deletions src/Http/Routing/src/Patterns/RoutePatternFactory.cs
Original file line number Diff line number Diff line change
Expand Up @@ -823,7 +823,7 @@ public static RoutePatternParameterPart ParameterPart(

if (@default != null && parameterKind == RoutePatternParameterKind.Optional)
{
throw new ArgumentNullException(nameof(parameterKind), Resources.TemplateRoute_OptionalCannotHaveDefaultValue);
throw new ArgumentException(Resources.TemplateRoute_OptionalCannotHaveDefaultValue);
}

return ParameterPartCore(
Expand Down Expand Up @@ -860,7 +860,7 @@ public static RoutePatternParameterPart ParameterPart(

if (@default != null && parameterKind == RoutePatternParameterKind.Optional)
{
throw new ArgumentNullException(nameof(parameterKind), Resources.TemplateRoute_OptionalCannotHaveDefaultValue);
throw new ArgumentException(Resources.TemplateRoute_OptionalCannotHaveDefaultValue);
}

ArgumentNullException.ThrowIfNull(parameterPolicies);
Expand Down Expand Up @@ -899,7 +899,7 @@ public static RoutePatternParameterPart ParameterPart(

if (@default != null && parameterKind == RoutePatternParameterKind.Optional)
{
throw new ArgumentNullException(nameof(parameterKind), Resources.TemplateRoute_OptionalCannotHaveDefaultValue);
throw new ArgumentException(Resources.TemplateRoute_OptionalCannotHaveDefaultValue);
}

ArgumentNullException.ThrowIfNull(parameterPolicies);
Expand Down
5 changes: 1 addition & 4 deletions src/Identity/EntityFrameworkCore/src/UserStore.cs
Original file line number Diff line number Diff line change
Expand Up @@ -611,10 +611,7 @@ join user in Users on userclaims.UserId equals user.Id
{
cancellationToken.ThrowIfCancellationRequested();
ThrowIfDisposed();
if (string.IsNullOrEmpty(normalizedRoleName))
{
throw new ArgumentNullException(nameof(normalizedRoleName));
}
ArgumentException.ThrowIfNullOrEmpty(normalizedRoleName);

var role = await FindRoleAsync(normalizedRoleName, cancellationToken);

Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -28,7 +28,8 @@ public UserClaimsPrincipalFactory(
ArgumentNullThrowHelper.ThrowIfNull(userManager);
if (optionsAccessor == null || optionsAccessor.Value == null)
{
throw new ArgumentNullException(nameof(optionsAccessor));
ArgumentNullThrowHelper.ThrowIfNull(optionsAccessor);
throw new ArgumentException($"{nameof(optionsAccessor)} cannot wrap a null value.", nameof(optionsAccessor));
Copy link
Member

Choose a reason for hiding this comment

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

Why two throw calls?

Copy link
Member

Choose a reason for hiding this comment

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

The first one is conditional if optionsAccessor is null, the second one is for optionsAccessor.Value.

}
UserManager = userManager;
Options = optionsAccessor.Value;
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -15,7 +15,7 @@ public async Task CreateIdentityNullChecks()
var userManager = MockHelpers.MockUserManager<PocoUser>().Object;
var roleManager = MockHelpers.MockRoleManager<PocoRole>().Object;
var options = new Mock<IOptions<IdentityOptions>>();
Assert.Throws<ArgumentNullException>("optionsAccessor",
Assert.Throws<ArgumentException>("optionsAccessor",
() => new UserClaimsPrincipalFactory<PocoUser, PocoRole>(userManager, roleManager, options.Object));
var identityOptions = new IdentityOptions();
options.Setup(a => a.Value).Returns(identityOptions);
Expand Down
5 changes: 1 addition & 4 deletions src/Identity/test/InMemory.Test/InMemoryStore.cs
Original file line number Diff line number Diff line change
Expand Up @@ -57,10 +57,7 @@ public class InMemoryStore<TUser, TRole> :
// RoleId == rolename for inmemory store tests
public Task<IList<TUser>> GetUsersInRoleAsync(string roleName, CancellationToken cancellationToken = default(CancellationToken))
{
if (String.IsNullOrEmpty(roleName))
{
throw new ArgumentNullException(nameof(roleName));
}
ArgumentException.ThrowIfNullOrEmpty(roleName);

var role = _roles.Values.Where(x => x.NormalizedName.Equals(roleName)).SingleOrDefault();
if (role == null)
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -23,6 +23,7 @@
</ItemGroup>

<ItemGroup>
<Compile Include="$(SharedSourceRoot)ThrowHelpers\ArgumentThrowHelper.cs" LinkBase="Shared" />
<Compile Include="$(SharedSourceRoot)ThrowHelpers\ArgumentNullThrowHelper.cs" LinkBase="Shared" />
<Compile Include="$(SharedSourceRoot)CallerArgument\CallerArgumentExpressionAttribute.cs" LinkBase="Shared" />
</ItemGroup>
Expand Down
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;
using Microsoft.AspNetCore.Shared;

namespace Microsoft.Extensions.Localization;

Expand All @@ -17,10 +18,7 @@ public class ResourceLocationAttribute : Attribute
/// <param name="resourceLocation">The location of resources for this Assembly.</param>
public ResourceLocationAttribute(string resourceLocation)
{
if (string.IsNullOrEmpty(resourceLocation))
{
throw new ArgumentNullException(nameof(resourceLocation));
}
ArgumentThrowHelper.ThrowIfNullOrEmpty(resourceLocation);

ResourceLocation = resourceLocation;
}
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -76,11 +76,7 @@ protected virtual string GetResourcePrefix(TypeInfo typeInfo)
protected virtual string GetResourcePrefix(TypeInfo typeInfo, string? baseNamespace, string? resourcesRelativePath)
{
ArgumentNullThrowHelper.ThrowIfNull(typeInfo);

if (string.IsNullOrEmpty(baseNamespace))
{
throw new ArgumentNullException(nameof(baseNamespace));
}
ArgumentThrowHelper.ThrowIfNullOrEmpty(baseNamespace);

if (string.IsNullOrEmpty(typeInfo.FullName))
{
Expand All @@ -107,15 +103,8 @@ protected virtual string GetResourcePrefix(TypeInfo typeInfo, string? baseNamesp
/// <returns>The prefix for resource lookup.</returns>
protected virtual string GetResourcePrefix(string baseResourceName, string baseNamespace)
{
if (string.IsNullOrEmpty(baseResourceName))
{
throw new ArgumentNullException(nameof(baseResourceName));
}

if (string.IsNullOrEmpty(baseNamespace))
{
throw new ArgumentNullException(nameof(baseNamespace));
}
ArgumentThrowHelper.ThrowIfNullOrEmpty(baseResourceName);
ArgumentThrowHelper.ThrowIfNullOrEmpty(baseNamespace);

var assemblyName = new AssemblyName(baseNamespace);
var assembly = Assembly.Load(assemblyName);
Expand Down
6 changes: 2 additions & 4 deletions src/Localization/Localization/src/RootNamespaceAttribute.cs
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;
using Microsoft.AspNetCore.Shared;

namespace Microsoft.Extensions.Localization;

Expand All @@ -18,10 +19,7 @@ public class RootNamespaceAttribute : Attribute
/// <param name="rootNamespace">The RootNamespace for this Assembly.</param>
public RootNamespaceAttribute(string rootNamespace)
{
if (string.IsNullOrEmpty(rootNamespace))
{
throw new ArgumentNullException(nameof(rootNamespace));
}
ArgumentThrowHelper.ThrowIfNullOrEmpty(rootNamespace);

RootNamespace = rootNamespace;
}
Expand Down
7 changes: 3 additions & 4 deletions src/Logging.AzureAppServices/src/AzureFileLoggerOptions.cs
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;
using Microsoft.AspNetCore.Shared;

namespace Microsoft.Extensions.Logging.AzureAppServices;

Expand Down Expand Up @@ -59,10 +60,8 @@ public string FileName
get { return _fileName; }
set
{
if (string.IsNullOrEmpty(value))
{
throw new ArgumentNullException(nameof(value));
}
ArgumentThrowHelper.ThrowIfNullOrEmpty(value);

_fileName = value;
}
}
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -28,4 +28,11 @@
<ItemGroup>
<InternalsVisibleTo Include="DynamicProxyGenAssembly2" Key="$(MoqPublicKey)" />
</ItemGroup>

<ItemGroup>
<Compile Include="$(SharedSourceRoot)ThrowHelpers\ArgumentThrowHelper.cs" LinkBase="Shared" />
<Compile Include="$(SharedSourceRoot)ThrowHelpers\ArgumentNullThrowHelper.cs" LinkBase="Shared" />
<Compile Include="$(SharedSourceRoot)CallerArgument\CallerArgumentExpressionAttribute.cs" LinkBase="Shared" />
</ItemGroup>

</Project>
10 changes: 2 additions & 8 deletions src/Middleware/HttpLogging/src/W3CLoggerOptions.cs
Original file line number Diff line number Diff line change
Expand Up @@ -64,10 +64,7 @@ public string FileName
get { return _fileName; }
set
{
if (string.IsNullOrEmpty(value))
{
throw new ArgumentNullException(nameof(value));
}
ArgumentException.ThrowIfNullOrEmpty(value);
_fileName = value;
}
}
Expand All @@ -83,10 +80,7 @@ public string LogDirectory
get { return _logDirectory; }
set
{
if (string.IsNullOrEmpty(value))
{
throw new ArgumentNullException(nameof(value));
}
ArgumentException.ThrowIfNullOrEmpty(value);
_logDirectory = value;
}
}
Expand Down
4 changes: 2 additions & 2 deletions src/Middleware/HttpLogging/test/W3CLoggerOptionsTests.cs
Original file line number Diff line number Diff line change
Expand Up @@ -26,14 +26,14 @@ public void ThrowsOnNegativeFileSizeLimit()
public void ThrowsOnEmptyFileName()
{
var options = new W3CLoggerOptions();
Assert.Throws<ArgumentNullException>(() => options.FileName = "");
Assert.Throws<ArgumentException>(() => options.FileName = "");
}

[Fact]
public void ThrowsOnEmptyLogDirectory()
{
var options = new W3CLoggerOptions();
Assert.Throws<ArgumentNullException>(() => options.LogDirectory = "");
Assert.Throws<ArgumentException>(() => options.LogDirectory = "");
}

[Fact]
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -16,10 +16,7 @@ internal sealed class CookieActionFactory
/// <returns>The action</returns>
public static ChangeCookieAction Create(string flagValue)
{
if (string.IsNullOrEmpty(flagValue))
{
throw new ArgumentNullException(nameof(flagValue));
}
ArgumentException.ThrowIfNullOrEmpty(flagValue);

var i = 0;
var separator = ':';
Expand Down
15 changes: 3 additions & 12 deletions src/Middleware/Rewrite/src/IISUrlRewrite/IISRewriteMap.cs
Original file line number Diff line number Diff line change
Expand Up @@ -9,10 +9,7 @@ internal sealed class IISRewriteMap

public IISRewriteMap(string name)
{
if (string.IsNullOrEmpty(name))
{
throw new ArgumentNullException(nameof(name));
}
ArgumentException.ThrowIfNullOrEmpty(name);
Name = name;
}

Expand All @@ -26,14 +23,8 @@ public string? this[string key]
}
set
{
if (string.IsNullOrEmpty(key))
{
throw new ArgumentNullException(nameof(key));
}
if (string.IsNullOrEmpty(value))
{
throw new ArgumentNullException(nameof(value));
}
ArgumentException.ThrowIfNullOrEmpty(key);
ArgumentException.ThrowIfNullOrEmpty(value);
_map[key] = value;
}
}
Expand Down
11 changes: 2 additions & 9 deletions src/Middleware/Rewrite/src/RedirectRule.cs
Original file line number Diff line number Diff line change
Expand Up @@ -16,15 +16,8 @@ internal sealed class RedirectRule : IRule
public int StatusCode { get; }
public RedirectRule(string regex, string replacement, int statusCode)
{
if (string.IsNullOrEmpty(regex))
{
throw new ArgumentNullException(nameof(regex));
}

if (string.IsNullOrEmpty(replacement))
{
throw new ArgumentNullException(nameof(replacement));
}
ArgumentException.ThrowIfNullOrEmpty(regex);
ArgumentException.ThrowIfNullOrEmpty(replacement);

InitialMatch = new Regex(regex, RegexOptions.Compiled | RegexOptions.CultureInvariant, _regexTimeout);
Replacement = replacement;
Expand Down
11 changes: 2 additions & 9 deletions src/Middleware/Rewrite/src/RewriteRule.cs
Original file line number Diff line number Diff line change
Expand Up @@ -16,15 +16,8 @@ internal sealed class RewriteRule : IRule
public bool StopProcessing { get; }
public RewriteRule(string regex, string replacement, bool stopProcessing)
{
if (string.IsNullOrEmpty(regex))
{
throw new ArgumentNullException(nameof(regex));
}

if (string.IsNullOrEmpty(replacement))
{
throw new ArgumentNullException(nameof(replacement));
}
ArgumentException.ThrowIfNullOrEmpty(regex);
ArgumentException.ThrowIfNullOrEmpty(replacement);

InitialMatch = new Regex(regex, RegexOptions.Compiled | RegexOptions.CultureInvariant, _regexTimeout);
Replacement = replacement;
Expand Down
8 changes: 2 additions & 6 deletions src/Middleware/Rewrite/src/UrlActions/ChangeCookieAction.cs
Original file line number Diff line number Diff line change
Expand Up @@ -18,13 +18,9 @@ public ChangeCookieAction(string name)
// for testing
internal ChangeCookieAction(string name, Func<DateTimeOffset> timeSource)
{
_timeSource = timeSource;

if (string.IsNullOrEmpty(name))
{
throw new ArgumentNullException(nameof(name));
}
ArgumentException.ThrowIfNullOrEmpty(name);

_timeSource = timeSource;
Name = name;
}

Expand Down
2 changes: 1 addition & 1 deletion src/Middleware/StaticFiles/src/StaticFileContext.cs
Original file line number Diff line number Diff line change
Expand Up @@ -45,7 +45,7 @@ public StaticFileContext(HttpContext context, StaticFileOptions options, ILogger
{
if (subPath.Value == null)
{
throw new ArgumentNullException(nameof(subPath));
throw new ArgumentException($"{nameof(subPath)} cannot wrap a null value.", nameof(subPath));
}

_context = context;
Expand Down
Loading