-
Notifications
You must be signed in to change notification settings - Fork 10.4k
Added null check to CorsPolicyBuilder #19831
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
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Seems reasonable to me. Thanks for the fix!
Hello @anurse! Because this pull request has the p.s. you can customize the way I help with merging this pull request, such as holding this pull request until a specific person approves. Simply @mention me (
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Looks like the test you added is failing:
Assert.Throws() Failure
Expected: typeof(System.ArgumentNullException)
Actual: typeof(System.NullReferenceException): Object reference not set to an instance of an object.
---- System.NullReferenceException : Object reference not set to an instance of an object.
Looking at sharplab, what your test actually does is pass a null array rather than an array with a null item.
Working with params
is a little tricky, as you can see since I approved the change without realizing this would happen! Good thing we have tests!
I'd suggest a few changes here:
-
Add a null-check directly on
WithOrigins
and keep the current test you've added -
Add a second test that explicitly constructs an array with an empty element and passes it to
WithOrigins
(i.e..WithOrigins(new string[] { "foo", null, "bar" })
) and validate that the innerArgumentNullException
is properly thrown.
I think that will resolve the issue and add a little more coverage here.
@chrispickford you still haven't addressed @anurse 's comment regarding what you're actually testing. Your test doesn't match your change. Do you want to give this another try? |
@mkArtakMSFT @anurse Apologies for the delay, I've updated the test and added extra null check. |
Closed and reopened the PR to re-trigger the build |
@anurse this is blocked on you to sign-off. |
I’m not able to do it from my phone but I’m happy to have my review dismissed to unblock merging if there are other approving reviews. |
It was noticed that there is no null checking in the
CorsPolicyBuilder.GetNormalizedOrigin
method after aNullReferenceException
was picked up. In the case that a null string is passed to the method viaWithOrigins
(in our case it was because a configuration file read failed) this change throws anArgumentNullException
which gives a clearer indication of what has failed.Addresses #19830