Use ArgumentException.ThrowIfNullOrWhitespace #48414

wcontayon · 2023-05-24T18:35:57Z

Use ArgumentException.ThrowIfNullOrWhitespace

You've read the Contributor Guide and Code of Conduct.
You've included unit or integration tests for your change, where applicable.
You've included inline docs for your change, where applicable.
There's an open issue for the PR that you are making. If you'd like to propose a new feature or change, please open an issue to discuss the change or find an existing issue.

Description

Use ArgumentException.ThrowIfNullOrWhitespace method from dotnet/runtime#86007
Fixes #48185 (partially, still check if we can replace some code with ArgumentException.ThrowIfNullOrWhitespace)

ghost · 2023-05-24T18:36:05Z

Thanks for your PR, @wcontayon. Someone from the team will get assigned to your PR shortly and we'll get it reviewed.

mgravell · 2023-05-27T10:24:41Z

src/Hosting/Hosting/src/Internal/HostingEnvironmentExtensions.cs

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


Definitely less explanatory

mgravell · 2023-05-27T10:25:28Z

src/Components/Server/src/ProtectedBrowserStorage/ProtectedBrowserStorage.cs

-        {
-            throw new ArgumentException("Cannot be null or empty", nameof(key));
-        }
+        ArgumentException.ThrowIfNullOrEmpty(purpose);


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

I'm missing what observable here?

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"

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

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.

mgravell · 2023-05-27T10:25:53Z

src/Hosting/Hosting/src/Internal/HostingEnvironmentExtensions.cs

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


Ditto less explanatory

mgravell · 2023-05-27T10:31:04Z

Not going to comment on each and every one of these, but I think there are 3 main scenarios in here:

trivial, no custom messages, just go for it
vague custom message; observable change, but... not a problem (and probably desirable for consistency)
humanized, context-specific message, that is erased here

The first two categories: IMO absolutely fine, let's encourage that. It is this final category that worries me a little - some of the existing message IMO cross an invisible and subjective line where the current message is more helpful than the new.

Not at a PC to check, but: I assume there isn't an overload that accepts a custom message?

Clarification: I mean this comment as a discussion starter, not as a definitive "this is the review". So to all: thoughts?

danmoseley · 2023-06-06T17:50:08Z

src/Hosting/Hosting/src/Internal/HostingEnvironmentExtensions.cs

@@ -13,7 +13,11 @@ internal static void Initialize(this IHostingEnvironment hostingEnvironment, str
 #pragma warning restore CS0618 // Type or member is obsolete
    {
        ArgumentNullException.ThrowIfNull(options);
-        ArgumentException.ThrowIfNullOrEmpty(contentRootPath);
+
+       if (string.IsNullOrEmpty(contentRootPath))


Suggested change

if (string.IsNullOrEmpty(contentRootPath))

if (string.IsNullOrEmpty(contentRootPath))

src/Http/Routing/src/DefaultLinkGenerator.cs

Co-authored-by: Dan Moseley <danmose@microsoft.com>

danmoseley · 2023-06-12T15:23:49Z

src/Security/CookiePolicy/src/CookiePolicyOptions.cs

-            {
-                throw new ArgumentException("Value cannot be null or empty string.", nameof(value));
-            }
+            ArgumentException.ThrowIfNullOrEmpty(value, nameof(value));


the 2nd parameter is required here?

Suggested change

ArgumentException.ThrowIfNullOrEmpty(value, nameof(value));

ArgumentException.ThrowIfNullOrEmpty(value);

danmoseley · 2023-06-12T15:26:25Z

src/Mvc/Mvc.RazorPages/src/ApplicationModels/PageConventionCollection.cs

-        {
-            throw new ArgumentException(Resources.ArgumentCannotBeNullOrEmpty, nameof(folderPath));
-        }
+        ArgumentException.ThrowIfNullOrEmpty(folderPath, nameof(folderPath));


Suggested change

ArgumentException.ThrowIfNullOrEmpty(folderPath, nameof(folderPath));

ArgumentException.ThrowIfNullOrEmpty(folderPath);

danmoseley · 2023-06-12T15:28:49Z

src/Middleware/Spa/SpaServices.Extensions/src/StaticFiles/DefaultSpaStaticFileProvider.cs

-            throw new ArgumentException($"The {nameof(options.RootPath)} property " +
-                $"of {nameof(options)} cannot be null or empty.");
-        }
+        ArgumentException.ThrowIfNullOrEmpty(options.RootPath, nameof(options.RootPath));


does this require the 2nd parameter to get it right?

Suggested change

ArgumentException.ThrowIfNullOrEmpty(options.RootPath, nameof(options.RootPath));

ArgumentException.ThrowIfNullOrEmpty(options.RootPath);

danmoseley · 2023-06-12T15:30:11Z

src/Middleware/Spa/SpaServices.Extensions/src/AngularCli/AngularCliMiddleware.cs

-        {
-            throw new ArgumentException("Cannot be null or empty", nameof(scriptName));
-        }
+        ArgumentException.ThrowIfNullOrEmpty(sourcePath, nameof(spaBuilder));


Suggested change

ArgumentException.ThrowIfNullOrEmpty(sourcePath, nameof(spaBuilder));

ArgumentException.ThrowIfNullOrEmpty(sourcePath);

better I think

danmoseley · 2023-06-12T15:31:52Z

src/Http/Routing/src/Patterns/RoutePatternFactory.cs

@@ -902,8 +880,6 @@ public static RoutePatternParameterPart ParameterPart(string parameterName, obje
            throw new ArgumentException(Resources.TemplateRoute_OptionalCannotHaveDefaultValue);
        }

-        ArgumentNullException.ThrowIfNull(parameterPolicies);


were these intentionally moved up?

danmoseley · 2023-06-12T15:39:12Z

src/Http/Headers/src/CookieHeaderValue.cs

-        {
-            throw new ArgumentNullException(nameof(name));
-        }
+        ArgumentNullException.ThrowIfNull(name, parameterName);


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);

danmoseley · 2023-06-12T15:41:26Z

Just a few bits of feedback. Have you verified in the debugger that it's working as expected on StringSegment? Normally you can't compare structs to null but I guess it's using the implicit equality operator.

BTW, there's still 6 uses of Resources.ArgumentCannotBeNullOrEmpty in the tree. Do you want to remove those, and then remove the string as well?

Tratcher · 2023-06-12T17:06:31Z

src/Http/Headers/src/CookieHeaderValue.cs

-        {
-            throw new ArgumentNullException(nameof(value));
-        }
+        ArgumentException.ThrowIfNullOrWhiteSpace(value.Value);


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.

ghost · 2023-06-20T03:00:50Z

Looks like this PR hasn't been active for some time and the codebase could have been changed in the meantime.
To make sure no breaking changes are introduced, please leave an /azp run comment here to rerun the CI pipeline and confirm success before merging the change.

wcontayon · 2023-06-24T01:54:09Z

/azp run

azure-pipelines · 2023-06-24T01:54:14Z

Commenter does not have sufficient privileges for PR 48414 in repo dotnet/aspnetcore

danmoseley · 2023-06-24T02:19:11Z

@wcontayon To rerun validation you have to click close then click open again..

danmoseley · 2023-06-24T03:00:57Z

#48414 (comment)

@mkArtakMSFT this comment seems to only work for folks with write access -- should we clarify the text that other folks should close/reopen? (my assumption is that it runs on latest HEAD either way @wtgodbe ?)

danmoseley · 2023-06-24T03:02:19Z

meanwhile, @wcontayon did you have a chance to address the feedback? You can click resolve when you have.
If it makes things easier, your option, you could split the PR up.

wtgodbe · 2023-06-26T17:08:03Z

my assumption is that it runs on latest HEAD either way @wtgodbe ?

That's right

danmoseley · 2023-06-30T21:19:30Z

@wcontayon do you expect to be able to continue addressing feedback? thanks for doing this. BTW, if it's easier for you, you could choose to split it into smaller PR's.

wcontayon · 2023-07-01T07:57:37Z

@wcontayon do you expect to be able to continue addressing feedback? thanks for doing this. BTW, if it's easier for you, you could choose to split it into smaller PR's.

@danmoseley yes sure !!
I’ll try to push the update by the end of next week 🙂

danmoseley · 2023-07-01T16:44:49Z

Cool! Work on your own schedule, just want to make sure we end up merging your work.

danmoseley · 2023-09-10T22:14:09Z

@wcontayon do you plan to continue with this PR? I'll close it for now, but feel free to reopen when you're ready.

use throwifnullorwhitespace

2ba2b7c

dotnet-issue-labeler bot added the area-identity Includes: Identity and providers label May 24, 2023

ghost added the community-contribution Indicates that the PR has been added by a community member label May 24, 2023

wcontayon added 2 commits May 24, 2023 22:12

replace by argumentexception.throwifnullorwhitespace

c536275

replace test by argument.throwifnullorempty

931d8a5

wcontayon marked this pull request as ready for review May 27, 2023 01:57

wcontayon requested review from Tratcher, halter73, BrennanConroy, JamesNK, mgravell, a team, javiercn and captainsafia as code owners May 27, 2023 01:57

mgravell reviewed May 27, 2023

View reviewed changes

wcontayon mentioned this pull request May 27, 2023

[API Proposal]: ArgumentException.ThrowIfNullOrEmpty / ThrowIfNull / ThrowIfNullOrWhiteSpace with custom message dotnet/runtime#86833

Closed

mkArtakMSFT added area-runtime and removed area-identity Includes: Identity and providers labels May 30, 2023

mkArtakMSFT assigned adityamandaleeka May 30, 2023

amcasey added area-middleware Includes: URL rewrite, redirect, response cache/compression, session, and other general middlesware area-networking Includes servers, yarp, json patch, bedrock, websockets, http client factory, and http abstractions and removed area-runtime labels Jun 6, 2023

keep custom message on exception

2d63c6e

danmoseley reviewed Jun 6, 2023

View reviewed changes

src/Http/Routing/src/DefaultLinkGenerator.cs Outdated Show resolved Hide resolved

Update src/Http/Routing/src/DefaultLinkGenerator.cs

32ceb03

Co-authored-by: Dan Moseley <danmose@microsoft.com>

danmoseley reviewed Jun 12, 2023

View reviewed changes

Tratcher reviewed Jun 12, 2023

View reviewed changes

ghost added the pending-ci-rerun When assigned to a PR indicates that the CI checks should be rerun label Jun 20, 2023

danmoseley closed this Jun 24, 2023

danmoseley reopened this Jun 24, 2023

danmoseley closed this Sep 10, 2023

divyeshio mentioned this pull request Sep 12, 2023

Use ArgumentException.ThrowIfNullOrEmpty #50666

Closed

4 tasks

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Use ArgumentException.ThrowIfNullOrWhitespace #48414

Use ArgumentException.ThrowIfNullOrWhitespace #48414

wcontayon commented May 24, 2023

ghost commented May 24, 2023

mgravell May 27, 2023

mgravell May 27, 2023

danmoseley May 27, 2023

mgravell May 27, 2023 •

edited

Loading

wcontayon May 27, 2023 •

edited

Loading

danmoseley May 28, 2023

mgravell May 27, 2023

mgravell commented May 27, 2023 •

edited

Loading

danmoseley Jun 6, 2023

danmoseley Jun 12, 2023

danmoseley Jun 12, 2023

danmoseley Jun 12, 2023

danmoseley Jun 12, 2023

danmoseley Jun 12, 2023

danmoseley Jun 12, 2023

danmoseley Jun 12, 2023

danmoseley Jun 12, 2023

danmoseley commented Jun 12, 2023

Tratcher Jun 12, 2023

ghost commented Jun 20, 2023

wcontayon commented Jun 24, 2023

azure-pipelines bot commented Jun 24, 2023

danmoseley commented Jun 24, 2023

danmoseley commented Jun 24, 2023

danmoseley commented Jun 24, 2023

wtgodbe commented Jun 26, 2023

danmoseley commented Jun 30, 2023

wcontayon commented Jul 1, 2023

danmoseley commented Jul 1, 2023

danmoseley commented Sep 10, 2023

	if (string.IsNullOrEmpty(contentRootPath))
	if (string.IsNullOrEmpty(contentRootPath))

	ArgumentException.ThrowIfNullOrEmpty(value, nameof(value));
	ArgumentException.ThrowIfNullOrEmpty(value);

	ArgumentException.ThrowIfNullOrEmpty(folderPath, nameof(folderPath));
	ArgumentException.ThrowIfNullOrEmpty(folderPath);

	ArgumentException.ThrowIfNullOrEmpty(options.RootPath, nameof(options.RootPath));
	ArgumentException.ThrowIfNullOrEmpty(options.RootPath);

	ArgumentException.ThrowIfNullOrEmpty(sourcePath, nameof(spaBuilder));
	ArgumentException.ThrowIfNullOrEmpty(sourcePath);

Use ArgumentException.ThrowIfNullOrWhitespace #48414

Use ArgumentException.ThrowIfNullOrWhitespace #48414

Conversation

wcontayon commented May 24, 2023

Use ArgumentException.ThrowIfNullOrWhitespace

Description

ghost commented May 24, 2023

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

mgravell May 27, 2023 • edited Loading

Choose a reason for hiding this comment

wcontayon May 27, 2023 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

mgravell commented May 27, 2023 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

danmoseley commented Jun 12, 2023

Choose a reason for hiding this comment

ghost commented Jun 20, 2023

wcontayon commented Jun 24, 2023

azure-pipelines bot commented Jun 24, 2023

danmoseley commented Jun 24, 2023

danmoseley commented Jun 24, 2023

danmoseley commented Jun 24, 2023

wtgodbe commented Jun 26, 2023

danmoseley commented Jun 30, 2023

wcontayon commented Jul 1, 2023

danmoseley commented Jul 1, 2023

danmoseley commented Sep 10, 2023

mgravell May 27, 2023 •

edited

Loading

wcontayon May 27, 2023 •

edited

Loading

mgravell commented May 27, 2023 •

edited

Loading