-
Notifications
You must be signed in to change notification settings - Fork 337
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
Refactored WwwAuthenticateParameters #2907
Conversation
- Used `is null`, `is object` for null checks - Used `string.Equals()` with `StringComparison.OrdinalIgnoreCase` for string comparison - Removed dummy `try/catch` that does nothing but erases the exception stack trace
@abatishchev will you update unit tests? |
@@ -32,7 +33,7 @@ public IEnumerable<string> Scopes | |||
{ | |||
get | |||
{ | |||
if (_scopes != null) | |||
if (_scopes is object) |
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.
I have not seen this type of check before. I am curious - how does it differ?
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.
This is a new feature in C# 7.0, part of added pattern matching. It bypasses any potential operator overloads (for either ==
and/or !=
) and directly emits the intended IL for null/not null comparison.
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.
nit: While this is definitely interesting, these checks obfuscate the fact that we are checking for null here. The word "is" usually implies that you are checking if an object is of a certain type, not whether or not it is null so this may be confusing to someone who is not aware of this feature and they may end up adding the null check in addition to this.
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.
I agree that it's a (relatively) new feature and might be confusing. As any other new feature :)
There are some potential benefits in using it, plus many most popular OSS projects have switched to using it too.
So here's our options:
- Continue to use
!= null
and== null
- Use
is object
andis null
(my preference) - Use
is not null
andis null
(I personally find the former the least readable)
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.
I like option 1 or 3.
Thoughts @bgavrilMS?
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.
Very interesting problem. It looks like "obj is null" is something we can do today. But "object is not null" is a C# 9.0 feature, which we don't currently use (but we probably should).
I agree that "a is object" is a bit counter-intuitive.
According to https://stackoverflow.com/questions/40676426/what-is-the-difference-between-x-is-null-and-x-null it looks like the compiler (Roslyn) was actually modified so that it emits the same kind of logic as long as ==
and !=
are not overloaded (I believe we haven't overloaded them for any object in MSAL). '
So maybe let's keep on using option 1 for consistency with MSAL, and separately we can look at moving to C# 9 and adding some analyzers for this kind of problem. Any changes should be consistent across the lib.
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.
Issue tracking the analyzer work #2921
src/client/Microsoft.Identity.Client/WwwAuthenticateParameters.cs
Outdated
Show resolved
Hide resolved
@jmprieur sorry, rushed to open a pull request. Working on adding/updating tests. |
69ba426
to
d202fa8
Compare
@bgavrilMS, @jmprieur all tests have passed. I think it's ready for a re-review. thanks! |
...t.Identity.Test.Integration.netfx/HeadlessTests/WwwAuthenticateParametersIntegrationTests.cs
Outdated
Show resolved
Hide resolved
...t.Identity.Test.Integration.netfx/HeadlessTests/WwwAuthenticateParametersIntegrationTests.cs
Show resolved
Hide resolved
...t.Identity.Test.Integration.netfx/HeadlessTests/WwwAuthenticateParametersIntegrationTests.cs
Show resolved
Hide resolved
{ | ||
if (httpClientFactory is null) | ||
{ | ||
throw new ArgumentException($"'{nameof(httpClientFactory)}' cannot be null.", nameof(httpClientFactory)); |
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.
nit: We typically use ArgumentNullException for these types of checks on the api surface.
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.
Yeah, I noticed that too. Let me explain.
The only already existing check is for resourceId for not null or empty or white space. And it throws just ArgumentException
. Some people may argue that if the input is an empty string the code can't throw ArgumentNullException
because it's not null. But I personally think that's nitpicking and matters less than readable and consistent code.
In this PR I'm adding two more checks, both are for reference types so I'm just checking for not null.
And the caveat is in the order of parameters: ArgumentException(message, paramName)
vs ArgumentNullException(paramName, message)
. Why the inconsistency is a question to BCL. So in our case the code looks little bit more messy and less readable.
So what I would do is to throw ArgumentNullException
in all checks. What you think?
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.
Yes, we do that as well in most places, i.e. throw ArgumentNullException
for empty string.
Strings should really not be nullable :), but unfortunately we are not a library that uses the nullable references feature, so we have to live with these small inconsistencies.
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.
Minor nit pick and a comment about the is object check
@abatishchev - I made 2 small changes as per @trwalke 's comments and I'm merging this, so as to be able to review your other PR with the tenant id. |
Changes proposed in this request
cancellationToken
toCreateFromResourceResponseAsync(string resourceUri, CancellationToken cancellationToken = default)
is null
,is object
for null checksstring.Equals()
withStringComparison.OrdinalIgnoreCase
for string comparisontry/catch
that does nothing but erases the exception stack traceCreateFromResourceResponseAsync(HttpClient httpClient, string resourceUri, CancellationToken cancellationToken = default)
CreateFromResourceResponseAsync(IMsalHttpClientFactory httpClientFactory, string resourceUri, CancellationToken cancellationToken = default)
Testing
Performance impact
Will slightly improve the performance of the helper methods.