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.

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

Jump to bottom

Use ArgumentException.ThrowIfNullOrWhitespace #48414

Closed
wants to merge 5 commits into from
Closed

Use ArgumentException.ThrowIfNullOrWhitespace #48414

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
5 commits
Select commit Hold shift + click to select a range
2ba2b7c
use throwifnullorwhitespace
wcontayon May 24, 2023
c536275
replace by argumentexception.throwifnullorwhitespace
wcontayon May 24, 2023
931d8a5
replace test by argument.throwifnullorempty
wcontayon May 27, 2023
2d63c6e
keep custom message on exception
comlanwcontayon Jun 6, 2023
32ceb03
Update src/Http/Routing/src/DefaultLinkGenerator.cs
wcontayon Jun 7, 2023
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
16 changes: 3 additions & 13 deletions src/Components/Server/src/ProtectedBrowserStorage/ProtectedBrowserStorage.cs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -33,10 +33,7 @@ private protected ProtectedBrowserStorage(string storeName, IJSRuntime jsRuntime
throw new PlatformNotSupportedException($"{GetType()} cannot be used when running in a browser.");
}

if (string.IsNullOrEmpty(storeName))
{
throw new ArgumentException("The value cannot be null or empty", nameof(storeName));
}
ArgumentException.ThrowIfNullOrEmpty(storeName);

_storeName = storeName;
_jsRuntime = jsRuntime ?? throw new ArgumentNullException(nameof(jsRuntime));
Expand Down Expand Up @@ -71,15 +68,8 @@ public ValueTask SetAsync(string key, object value)
/// <returns>A <see cref="ValueTask"/> representing the completion of the operation.</returns>
public ValueTask SetAsync(string purpose, string key, object value)
{
if (string.IsNullOrEmpty(purpose))
{
throw new ArgumentException("Cannot be null or empty", nameof(purpose));
}

if (string.IsNullOrEmpty(key))
{
throw new ArgumentException("Cannot be null or empty", nameof(key));
}
ArgumentException.ThrowIfNullOrEmpty(purpose);
Copy link
Member

Choose a reason for hiding this comment

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

Observable change - not saying good or bad, but: definitely a change; I'm fine with it personally

Copy link
Member

Choose a reason for hiding this comment

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

I'm missing what observable here?

Copy link
Member

@mgravell mgravell May 27, 2023 •
edited
Loading

Choose a reason for hiding this comment

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

The specific error message will change. Very much a pedantic "we should at least be aware that we're changing something, even if it is something that nobody should reasonably rely on", and in this case very much in the "trivial, no custom messages, just go for it" category. New message "The value cannot be an empty string." or "Value cannot be null"

Copy link
Contributor Author

@wcontayon wcontayon May 27, 2023 •
edited
Loading

Choose a reason for hiding this comment

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

Effectively, we do not have an overload that accepts a custom message.
I opened a Api Proposal on dotnet/runtime to have the option to set the custom message.
we can just update only those part with generic message

Copy link
Member

Choose a reason for hiding this comment

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

Ah, right. By precedent, I think we've had no problem changing exception messages, including in cases like this, so long as that doesn't lose actual useful information.

ArgumentException.ThrowIfNullOrEmpty(key);

return SetProtectedJsonAsync(key, Protect(purpose, value));
}
Expand Down
5 changes: 1 addition & 4 deletions src/Components/Web/src/JSComponents/JSComponentConfigurationStore.cs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -56,10 +56,7 @@ internal void Add([DynamicallyAccessedMembers(LinkerFlags.Component)] Type compo
{
Add(componentType, identifier);

if (string.IsNullOrEmpty(javaScriptInitializer))
{
throw new ArgumentException($"'{nameof(javaScriptInitializer)}' cannot be null or empty.", nameof(javaScriptInitializer));
}
ArgumentException.ThrowIfNullOrEmpty(javaScriptInitializer);

// Since it has a JS initializer, prepare the metadata we'll supply to JS code
if (!JSComponentIdentifiersByInitializer.TryGetValue(javaScriptInitializer, out var identifiersForInitializer))
Expand Down
6 changes: 1 addition & 5 deletions src/FileProviders/Embedded/src/Manifest/ManifestDirectory.cs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -47,11 +47,7 @@ public override ManifestEntry Traverse(StringSegment segment)

public static ManifestDirectory CreateDirectory(string name, ManifestEntry[] children)
{
if (string.IsNullOrWhiteSpace(name))
{
throw new ArgumentException($"'{nameof(name)}' must not be null, empty or whitespace.", nameof(name));
}

ArgumentNullThrowHelper.ThrowIfNullOrWhiteSpace(name);
ArgumentNullThrowHelper.ThrowIfNull(children);

var result = new ManifestDirectory(name, children);
Expand Down
6 changes: 5 additions & 1 deletion src/Hosting/Hosting/src/Internal/HostingEnvironmentExtensions.cs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -13,10 +13,12 @@ internal static void Initialize(this IHostingEnvironment hostingEnvironment, str
#pragma warning restore CS0618 // Type or member is obsolete
{
ArgumentNullException.ThrowIfNull(options);
if (string.IsNullOrEmpty(contentRootPath))

if (string.IsNullOrEmpty(contentRootPath))
Copy link
Member

Choose a reason for hiding this comment

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

Suggested change
if (string.IsNullOrEmpty(contentRootPath))
if (string.IsNullOrEmpty(contentRootPath))

{
throw new ArgumentException("A valid non-empty content root must be provided.", nameof(contentRootPath));
}

if (!Directory.Exists(contentRootPath))
{
throw new ArgumentException($"The content root '{contentRootPath}' does not exist.", nameof(contentRootPath));
Expand Down Expand Up @@ -67,10 +69,12 @@ internal static void Initialize(
IHostEnvironment? baseEnvironment = null)
{
ArgumentNullException.ThrowIfNull(options);

if (string.IsNullOrEmpty(contentRootPath))
{
throw new ArgumentException("A valid non-empty content root must be provided.", nameof(contentRootPath));
}

if (!Directory.Exists(contentRootPath))
{
throw new ArgumentException($"The content root '{contentRootPath}' does not exist.", nameof(contentRootPath));
Expand Down
5 changes: 1 addition & 4 deletions src/Hosting/Server.IntegrationTesting/src/Common/DeploymentParameters.cs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -52,10 +52,7 @@ public DeploymentParameters(
RuntimeFlavor runtimeFlavor,
RuntimeArchitecture runtimeArchitecture)
{
if (string.IsNullOrEmpty(applicationPath))
{
throw new ArgumentException("Value cannot be null.", nameof(applicationPath));
}
ArgumentException.ThrowIfNullOrEmpty(applicationPath);

if (!Directory.Exists(applicationPath))
{
Expand Down
5 changes: 1 addition & 4 deletions src/Http/Headers/src/ContentDispositionHeaderValue.cs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -333,10 +333,7 @@ private static int GetDispositionTypeExpressionLength(StringSegment input, int s

private static void CheckDispositionTypeFormat(StringSegment dispositionType, string parameterName)
{
if (StringSegment.IsNullOrEmpty(dispositionType))
{
throw new ArgumentException("An empty string is not allowed.", parameterName);
}
ArgumentException.ThrowIfNullOrEmpty(dispositionType.Value, parameterName);

// When adding values using strongly typed objects, no leading/trailing LWS (whitespaces) are allowed.
var dispositionTypeLength = GetDispositionTypeExpressionLength(dispositionType, 0, out var tempDispositionType);
Expand Down
11 changes: 3 additions & 8 deletions src/Http/Headers/src/CookieHeaderValue.cs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -164,10 +164,8 @@ public static bool TryParseStrictList(IList<string>? inputs, [NotNullWhen(true)]

internal static void CheckNameFormat(StringSegment name, string parameterName)
{
if (name == null)
{
throw new ArgumentNullException(nameof(name));
}
ArgumentNullException.ThrowIfNull(name, parameterName);
Copy link
Member

Choose a reason for hiding this comment

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

This is covered by the line below, and the implicit equals operator on the StringSegment struct, I think? Anyway in other cases you don't have this line.

Suggested change
ArgumentNullException.ThrowIfNull(name, parameterName);

ArgumentException.ThrowIfNullOrEmpty(name.Value, parameterName);

if (HttpRuleParser.GetTokenLength(name, 0) != name.Length)
{
Expand All @@ -177,10 +175,7 @@ internal static void CheckNameFormat(StringSegment name, string parameterName)

internal static void CheckValueFormat(StringSegment value, string parameterName)
{
if (value == null)
{
throw new ArgumentNullException(nameof(value));
}
ArgumentException.ThrowIfNullOrWhiteSpace(value.Value);
Copy link
Member

Choose a reason for hiding this comment

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

Behavior change, Why is this more restrictive than before? This might explain most of the auth test failures, an empty value is commonly used when removing a cookie.


var offset = 0;
var result = CookieHeaderParserShared.GetCookieValue(value, ref offset);
Expand Down
5 changes: 1 addition & 4 deletions src/Http/Headers/src/EntityTagHeaderValue.cs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -47,10 +47,7 @@ public EntityTagHeaderValue(StringSegment tag)
/// <param name="isWeak">A value that indicates if this entity-tag header is a weak validator.</param>
public EntityTagHeaderValue(StringSegment tag, bool isWeak)
{
if (StringSegment.IsNullOrEmpty(tag))
{
throw new ArgumentException("An empty string is not allowed.", nameof(tag));
}
ArgumentException.ThrowIfNullOrEmpty(tag.Value, nameof(tag));

if (!isWeak && StringSegment.Equals(tag, "*", StringComparison.Ordinal))
{
Expand Down
5 changes: 1 addition & 4 deletions src/Http/Headers/src/HeaderUtilities.cs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -70,10 +70,7 @@ internal static void SetQuality(IList<NameValueHeaderValue> parameters, double?

internal static void CheckValidToken(StringSegment value, string parameterName)
{
if (StringSegment.IsNullOrEmpty(value))
{
throw new ArgumentException("An empty string is not allowed.", parameterName);
}
ArgumentException.ThrowIfNullOrEmpty(value.Value, parameterName);

if (HttpRuleParser.GetTokenLength(value, 0) != value.Length)
{
Expand Down
5 changes: 1 addition & 4 deletions src/Http/Headers/src/MediaTypeHeaderValue.cs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -652,10 +652,7 @@ private static int GetMediaTypeExpressionLength(StringSegment input, int startIn

private static void CheckMediaTypeFormat(StringSegment mediaType, string parameterName)
{
if (StringSegment.IsNullOrEmpty(mediaType))
{
throw new ArgumentException("An empty string is not allowed.", parameterName);
}
ArgumentException.ThrowIfNullOrEmpty(mediaType.Value, parameterName);

// When adding values using strongly typed objects, no leading/trailing LWS (whitespace) is allowed.
// Also no LWS between type and subtype is allowed.
Expand Down
6 changes: 1 addition & 5 deletions src/Http/Http.Abstractions/src/Extensions/UsePathBaseMiddleware.cs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -21,11 +21,7 @@ public class UsePathBaseMiddleware
public UsePathBaseMiddleware(RequestDelegate next, PathString pathBase)
{
ArgumentNullException.ThrowIfNull(next);

if (!pathBase.HasValue)
{
throw new ArgumentException($"{nameof(pathBase)} cannot be null or empty.");
}
ArgumentException.ThrowIfNullOrEmpty(pathBase.Value, nameof(pathBase));

_next = next;
_pathBase = pathBase;
Expand Down
22 changes: 5 additions & 17 deletions src/Http/Http.Results/src/TypedResults.cs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -511,10 +511,7 @@ public static PhysicalFileHttpResult PhysicalFile(
EntityTagHeaderValue? entityTag = null,
bool enableRangeProcessing = false)
{
if (string.IsNullOrEmpty(path))
{
throw new ArgumentException("Argument cannot be null or empty", nameof(path));
}
ArgumentException.ThrowIfNullOrEmpty(path);

return new(path, contentType)
{
Expand Down Expand Up @@ -547,10 +544,7 @@ public static VirtualFileHttpResult VirtualFile(
EntityTagHeaderValue? entityTag = null,
bool enableRangeProcessing = false)
{
if (string.IsNullOrEmpty(path))
{
throw new ArgumentException("Argument cannot be null or empty", nameof(path));
}
ArgumentException.ThrowIfNullOrEmpty(path);

return new(path, contentType)
{
Expand All @@ -576,10 +570,7 @@ public static VirtualFileHttpResult VirtualFile(
/// <returns>The created <see cref="RedirectHttpResult"/> for the response.</returns>
public static RedirectHttpResult Redirect([StringSyntax(StringSyntaxAttribute.Uri)] string url, bool permanent = false, bool preserveMethod = false)
{
if (string.IsNullOrEmpty(url))
{
throw new ArgumentException("Argument cannot be null or empty", nameof(url));
}
ArgumentException.ThrowIfNullOrEmpty(url);

return new(url, permanent, preserveMethod);
}
Expand All @@ -599,10 +590,7 @@ public static RedirectHttpResult Redirect([StringSyntax(StringSyntaxAttribute.Ur
/// <returns>The created <see cref="RedirectHttpResult"/> for the response.</returns>
public static RedirectHttpResult LocalRedirect([StringSyntax(StringSyntaxAttribute.Uri, UriKind.Relative)] string localUrl, bool permanent = false, bool preserveMethod = false)
{
if (string.IsNullOrEmpty(localUrl))
{
throw new ArgumentException("Argument cannot be null or empty", nameof(localUrl));
}
ArgumentException.ThrowIfNullOrEmpty(localUrl);

return new(localUrl, acceptLocalUrlOnly: true, permanent, preserveMethod);
}
Expand Down Expand Up @@ -837,7 +825,7 @@ public static ValidationProblem ValidationProblem(

/// <summary>
/// Produces a <see cref="StatusCodes.Status201Created"/> response.
/// </summary>
/// </summary>
/// <returns>The created <see cref="HttpResults.Created"/> for the response.</returns>
public static Created Created()
{
Expand Down
54 changes: 11 additions & 43 deletions src/Http/Routing/src/Patterns/RoutePatternFactory.cs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -709,10 +709,7 @@ private static RoutePatternPathSegment SegmentCore(RoutePatternPart[] parts)
/// <returns>The <see cref="RoutePatternLiteralPart"/>.</returns>
public static RoutePatternLiteralPart LiteralPart(string content)
{
if (string.IsNullOrEmpty(content))
{
throw new ArgumentException(Resources.Argument_NullOrEmpty, nameof(content));
}
ArgumentException.ThrowIfNullOrEmpty(content);

if (content.Contains('?'))
{
Expand All @@ -735,11 +732,7 @@ private static RoutePatternLiteralPart LiteralPartCore(string content)
/// <returns>The <see cref="RoutePatternSeparatorPart"/>.</returns>
public static RoutePatternSeparatorPart SeparatorPart(string content)
{
if (string.IsNullOrEmpty(content))
{
throw new ArgumentException(Resources.Argument_NullOrEmpty, nameof(content));
}

ArgumentException.ThrowIfNullOrEmpty(content);
return SeparatorPartCore(content);
}

Expand All @@ -755,10 +748,7 @@ private static RoutePatternSeparatorPart SeparatorPartCore(string content)
/// <returns>The <see cref="RoutePatternParameterPart"/>.</returns>
public static RoutePatternParameterPart ParameterPart(string parameterName)
{
if (string.IsNullOrEmpty(parameterName))
{
throw new ArgumentException(Resources.Argument_NullOrEmpty, nameof(parameterName));
}
ArgumentException.ThrowIfNullOrEmpty(parameterName);

if (parameterName.AsSpan().IndexOfAny(RoutePatternParser.InvalidParameterNameChars) >= 0)
{
Expand All @@ -781,10 +771,7 @@ public static RoutePatternParameterPart ParameterPart(string parameterName)
/// <returns>The <see cref="RoutePatternParameterPart"/>.</returns>
public static RoutePatternParameterPart ParameterPart(string parameterName, object @default)
{
if (string.IsNullOrEmpty(parameterName))
{
throw new ArgumentException(Resources.Argument_NullOrEmpty, nameof(parameterName));
}
ArgumentException.ThrowIfNullOrEmpty(parameterName);

if (parameterName.AsSpan().IndexOfAny(RoutePatternParser.InvalidParameterNameChars) >= 0)
{
Expand All @@ -811,10 +798,7 @@ public static RoutePatternParameterPart ParameterPart(
object? @default,
RoutePatternParameterKind parameterKind)
{
if (string.IsNullOrEmpty(parameterName))
{
throw new ArgumentException(Resources.Argument_NullOrEmpty, nameof(parameterName));
}
ArgumentException.ThrowIfNullOrEmpty(parameterName);

if (parameterName.AsSpan().IndexOfAny(RoutePatternParser.InvalidParameterNameChars) >= 0)
{
Expand Down Expand Up @@ -848,10 +832,8 @@ public static RoutePatternParameterPart ParameterPart(
RoutePatternParameterKind parameterKind,
IEnumerable<RoutePatternParameterPolicyReference> parameterPolicies)
{
if (string.IsNullOrEmpty(parameterName))
{
throw new ArgumentException(Resources.Argument_NullOrEmpty, nameof(parameterName));
}
ArgumentException.ThrowIfNullOrEmpty(parameterName);
ArgumentNullException.ThrowIfNull(parameterPolicies);

if (parameterName.AsSpan().IndexOfAny(RoutePatternParser.InvalidParameterNameChars) >= 0)
{
Expand All @@ -863,8 +845,6 @@ public static RoutePatternParameterPart ParameterPart(
throw new ArgumentException(Resources.TemplateRoute_OptionalCannotHaveDefaultValue);
}

ArgumentNullException.ThrowIfNull(parameterPolicies);

return ParameterPartCore(
parameterName: parameterName,
@default: @default,
Expand All @@ -887,10 +867,8 @@ public static RoutePatternParameterPart ParameterPart(
RoutePatternParameterKind parameterKind,
params RoutePatternParameterPolicyReference[] parameterPolicies)
{
if (string.IsNullOrEmpty(parameterName))
{
throw new ArgumentException(Resources.Argument_NullOrEmpty, nameof(parameterName));
}
ArgumentException.ThrowIfNullOrEmpty(parameterName);
ArgumentNullException.ThrowIfNull(parameterPolicies);

if (parameterName.AsSpan().IndexOfAny(RoutePatternParser.InvalidParameterNameChars) >= 0)
{
Expand All @@ -902,8 +880,6 @@ public static RoutePatternParameterPart ParameterPart(
throw new ArgumentException(Resources.TemplateRoute_OptionalCannotHaveDefaultValue);
}

ArgumentNullException.ThrowIfNull(parameterPolicies);
Copy link
Member

Choose a reason for hiding this comment

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

were these intentionally moved up?


return ParameterPartCore(
parameterName: parameterName,
@default: @default,
Expand Down Expand Up @@ -986,11 +962,7 @@ public static RoutePatternParameterPolicyReference Constraint(IRouteConstraint c
/// <returns>The <see cref="RoutePatternParameterPolicyReference"/>.</returns>
public static RoutePatternParameterPolicyReference Constraint(string constraint)
{
if (string.IsNullOrEmpty(constraint))
{
throw new ArgumentException(Resources.Argument_NullOrEmpty, nameof(constraint));
}

ArgumentException.ThrowIfNullOrEmpty(constraint);
return ParameterPolicyCore(constraint);
}

Expand All @@ -1017,11 +989,7 @@ public static RoutePatternParameterPolicyReference ParameterPolicy(IParameterPol
/// <returns>The <see cref="RoutePatternParameterPolicyReference"/>.</returns>
public static RoutePatternParameterPolicyReference ParameterPolicy(string parameterPolicy)
{
if (string.IsNullOrEmpty(parameterPolicy))
{
throw new ArgumentException(Resources.Argument_NullOrEmpty, nameof(parameterPolicy));
}

ArgumentException.ThrowIfNullOrEmpty(parameterPolicy);
return ParameterPolicyCore(parameterPolicy);
}

Expand Down
Loading
Loading