Change incorrect usage of ArgumentNullException #48243
Conversation
ghost
added
the
needs-area-label
captainsafia
added
area-infrastructure
|
Hey @dotnet/aspnet-build, looks like this PR is something you want to take a look at. |
captainsafia
requested review from
BrennanConroy,
halter73,
Tratcher,
JamesNK,
mgravell,
a team and
javiercn
as code owners
| @@ -28,7 +28,8 @@ public class UserClaimsPrincipalFactory<TUser> : IUserClaimsPrincipalFactory<TUs | |||
| 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)); | |||
There was a problem hiding this comment.
The first one is conditional if optionsAccessor is null, the second one is for optionsAccessor.Value.
| internal static partial class ArgumentThrowHelper | ||
| { | ||
| #if !NET7_0_OR_GREATER || NETSTANDARD ||NETFRAMEWORK | ||
| private const string EmptyString = ""; |
There was a problem hiding this comment.
Ah yeah, string.Empty is available in the versions targeted here so it should be fine.
There was a problem hiding this comment.
Took me a while to figure out why this was needed. string.Empty isn't a const so you can't use it in equality checks. We also cannot use string.IsNullOrEmpty because it isn't annotated for nullability in netfx and netstandard.
There was a problem hiding this comment.
can't use it in equality checks
o.O
You mean can't use it with or? Why not just use || then?
There was a problem hiding this comment.
Merp -- thought that was what I tried in b09309a but looks like I was wrong.
| @@ -599,10 +599,7 @@ internal async Task SendFileAsync(string fileName, long offset, long? count, Can | |||
| // It's too expensive to validate the file attributes before opening the file. Open the file and then check the lengths. | |||
| // This all happens inside of ResponseStreamAsyncResult. | |||
| // TODO: Verbose log parameters | |||
| if (string.IsNullOrWhiteSpace(fileName)) | |||
There was a problem hiding this comment.
We are losing the whitespace check, is that ok? A couple other spots have this too.
There was a problem hiding this comment.
Levi advised against ever using a WhiteSpace check because it does way more than you'd think.
| { | ||
| throw new ArgumentNullException(nameof(applicationName)); | ||
| } | ||
| ArgumentThrowHelper.ThrowIfNullOrEmpty(applicationName); |
There was a problem hiding this comment.
is it OK that these are theoretical breaking changes in the empty string case, changing the exception type?
There was a problem hiding this comment.
(I assume so, I believe dotnet/runtime does such things.)
There was a problem hiding this comment.
This is true, although I'm comfortable documenting this as a breaking change and explaining.
| @@ -26,8 +30,10 @@ public static void ThrowIfNull([NotNull] object? argument, [CallerArgumentExpres | |||
| #endif | |||
| } | |||
|
|
|||
| #if !NET7_0_OR_GREATER | |||
| #if !NET7_0_OR_GREATER || NETSTANDARD || NETFRAMEWORK | |||
There was a problem hiding this comment.
doesn't #if !NET7_0_OR_GREATER include NETSTANDARD and NETFRAMEWORK? Assuming SDK style projects.
There was a problem hiding this comment.
From my understanding of the docs and the errors I was seeing in the build that resulted in this fix NET7_0_OR_GREATER is only set for projects that target a .NET Core version. They aren't set at all for netstandard or netfx projects.
There was a problem hiding this comment.
I don't understand. If NET7_0_OR_GREATER is not defined then you're in netstandard or netframework, which is what we want. And that's what #if !NET7_0_OR_GREATER does.
Following up on #47731.