move error codes into their exception types [1004] #1033

MarkZuber · 2019-04-02T21:52:51Z

I've started working on [1004] for moving MsalError error constants into their respective exception classes.

Looking through the first run of this change, I'm actually not a fan of this approach. I believe we should go the other way and remove the constants from the exception class and have everything in MsalError (with corresponding messages in MsalErrorMessage)

Not all MsalError values are used in an MsalException, sometimes it's a response value or other usage.
A few of the errors cross boundaries and are used in ClientException and ServiceException
We throw MsalException itself in a few cases. I'm not sure why/how that makes sense and what this hierarchy is providing for customers
Now the error codes are in one of 3 classes (MsalException, MsalClientException, MsalErrorException) but the messages are all in one file (MsalErrorMessage). It's not always clear which place to pull the constant from. They were ALL in MsalError before.

Please comment on this feedback as well as what we should do with the errors that aren't used on any of the exceptions if you still believe we should go this way. Should they just go into MsalException (even though they'll never be attached to an exception)?

#1004

jennyf19 · 2019-04-02T22:16:33Z

src/Microsoft.Identity.Client/Instance/AdfsOpenIdConfigurationEndpointManager.cs

@@ -86,7 +86,7 @@ public AdfsOpenIdConfigurationEndpointManager(IServiceBundle serviceBundle)
                             a.Href.Equals(resource, StringComparison.OrdinalIgnoreCase)) == null)
                {
                    throw MsalExceptionFactory.GetClientException(
-                        MsalError.InvalidAuthority,
+                        MsalServiceException.InvalidAuthority,


should be client here.

jennyf19 · 2019-04-02T22:16:51Z

src/Microsoft.Identity.Client/Instance/AuthorityEndpointResolutionManager.cs

@@ -60,7 +60,7 @@ public AuthorityEndpointResolutionManager(IServiceBundle serviceBundle, bool sho
            if (authorityInfo.AuthorityType == AuthorityType.Adfs && string.IsNullOrEmpty(userPrincipalName))
            {
                throw MsalExceptionFactory.GetClientException(
-                    MsalError.UpnRequired,
+                    MsalServiceException.UpnRequired,


jennyf19 · 2019-04-02T22:17:38Z

src/Microsoft.Identity.Client/MsalClientException.cs

@@ -82,7 +82,7 @@ public class MsalClientException : MsalException
        /// implementing chrome tabs is missing on the Android phone (that's only an example: this exception can apply to other
        /// platforms as well)
        /// </summary>
-        public const string AuthenticationUiFailedError = "authentication_ui_failed";
+        public const string AuthenticationUiFailed = "authentication_ui_failed";


will this be a breaking change? w/the name change?

jennyf19 · 2019-04-02T22:19:22Z

src/Microsoft.Identity.Client/MsalError.cs


+        // todo: not used anywhere


can we just remove all this?

jennyf19 · 2019-04-02T22:22:30Z

src/Microsoft.Identity.Client/OAuth2/OAuth2Client.cs

                    MsalErrorMessage.NonParsableOAuthError,
                    response);
            }
            catch (Exception ex)
            {
-                exceptionToThrow = MsalExceptionFactory.GetServiceException(MsalError.UnknownError, response.Body, response, ex);
+                exceptionToThrow = MsalExceptionFactory.GetServiceException(MsalException.UnknownError, response.Body, response, ex);


should this be service?

jennyf19

🐑 🐺

jennyf19 · 2019-04-02T22:32:05Z

@mzuber@microsoft.com so i went too fast and didn't read this before reviewing. I agree. I like having MsalError and doing MsalExceptionFactory.GetClientException or GetServiceException. This is much clearer, and, as you point out, will be easier to maintain moving forward as all the consts will be in one place.

jmprieur · 2019-04-03T01:30:27Z

How would this affect the DevEx @MarkZuber and @jennyf19 ?

for instance, how would this transform the code in https://github.com/AzureAD/microsoft-authentication-library-for-dotnet/wiki/Username-Password-Authentication#how-to-use-it ?
(which BTW does not use the constants in the Exceptions … so … well …

If we do as you propose, @MarkZuber, let's make sure that the XML comments tell customers if an error message can be thrown in a client or service exception.

Finally, this discussion is also related to the Apple team's proposal: MSAL error handling in iOS. Apparently they prefer option 6.

bgavrilMS · 2019-04-03T12:05:22Z

src/Microsoft.Identity.Client/Http/RedirectUriHelper.cs

@@ -60,7 +60,7 @@ public static void Validate(Uri redirectUri, bool usesSystemBrowser = false)
                Constants.DefaultRedirectUri.Equals(redirectUri.AbsoluteUri, StringComparison.OrdinalIgnoreCase))
            {
                throw MsalExceptionFactory.GetClientException(


It might be a good moment to delete the exception factory.

MarkZuber · 2019-04-03T22:11:44Z

How would this affect the DevEx @MarkZuber and @jennyf19 ?

for instance, how would this transform the code in https://github.com/AzureAD/microsoft-authentication-library-for-dotnet/wiki/Username-Password-Authentication#how-to-use-it ?
(which BTW does not use the constants in the Exceptions … so … well …

If we do as you propose, @MarkZuber, let's make sure that the XML comments tell customers if an error message can be thrown in a client or service exception.

Finally, this discussion is also related to the Apple team's proposal: MSAL error handling in iOS. Apparently they prefer option 6.

So for us, option 6 would look like moving all error types into MsalError.cs which is also what I'm proposing. Splitting them into the different exceptions, i feel, looks odd in this PR.

jmprieur · 2019-04-04T11:53:14Z

@MarkZuber : I'm a bit confused by this PR.
Do we agree that we want to move all the errors in MsalError.cs ?
I see that they are also added to the Exception classes?

MarkZuber · 2019-04-04T15:10:19Z

@MarkZuber : I'm a bit confused by this PR.
Do we agree that we want to move all the errors in MsalError.cs ?
I see that they are also added to the Exception classes?

I have two PRs.

This PR is the attempt at "move error codes into the exception types". I'm suggesting we don't take this PR and abort it.
The other PR is here: Merge all error codes into MsalError, remove MsalExceptionFactory [1004] #1039 which is the one I recommend taking that moves all of the content into MsalError.cs

MarkZuber · 2019-04-04T15:31:40Z

Officially aborting this PR. The way we're going is this: #1039

jmprieur · 2019-04-04T16:30:49Z

Thanks for clarifying, @MarkZuber

move error codes into their exception types

a661f92

MarkZuber requested review from bgavrilMS, jmprieur, jennyf19 and henrik-me April 2, 2019 21:52

jennyf19 reviewed Apr 2, 2019

View reviewed changes

src/Microsoft.Identity.Client/MsalError.cs

// todo: not used anywhere

Copy link

Collaborator

jennyf19 Apr 2, 2019

Choose a reason for hiding this comment

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

can we just remove all this?

jennyf19 reviewed Apr 2, 2019

View reviewed changes

jennyf19 approved these changes Apr 2, 2019

View reviewed changes

bgavrilMS reviewed Apr 3, 2019

View reviewed changes

MarkZuber mentioned this pull request Apr 3, 2019

Merge all error codes into MsalError, remove MsalExceptionFactory [1004] #1039

Merged

MarkZuber closed this Apr 4, 2019

MarkZuber deleted the markzuber/errorcodes branch April 4, 2019 15:31

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

move error codes into their exception types [1004] #1033

move error codes into their exception types [1004] #1033

MarkZuber commented Apr 2, 2019

jennyf19 Apr 2, 2019

jennyf19 Apr 2, 2019

jennyf19 Apr 2, 2019

jennyf19 Apr 2, 2019

jennyf19 Apr 2, 2019

jennyf19 left a comment

jennyf19 commented Apr 2, 2019

jmprieur commented Apr 3, 2019

bgavrilMS Apr 3, 2019 •

edited

Loading

MarkZuber commented Apr 3, 2019

jmprieur commented Apr 4, 2019

MarkZuber commented Apr 4, 2019

MarkZuber commented Apr 4, 2019

jmprieur commented Apr 4, 2019

move error codes into their exception types [1004] #1033

move error codes into their exception types [1004] #1033

Conversation

MarkZuber commented Apr 2, 2019

jennyf19 Apr 2, 2019

Choose a reason for hiding this comment

jennyf19 Apr 2, 2019

Choose a reason for hiding this comment

jennyf19 Apr 2, 2019

Choose a reason for hiding this comment

jennyf19 Apr 2, 2019

Choose a reason for hiding this comment

jennyf19 Apr 2, 2019

Choose a reason for hiding this comment

jennyf19 left a comment

Choose a reason for hiding this comment

jennyf19 commented Apr 2, 2019

jmprieur commented Apr 3, 2019

bgavrilMS Apr 3, 2019 • edited Loading

Choose a reason for hiding this comment

MarkZuber commented Apr 3, 2019

jmprieur commented Apr 4, 2019

MarkZuber commented Apr 4, 2019

MarkZuber commented Apr 4, 2019

jmprieur commented Apr 4, 2019

bgavrilMS Apr 3, 2019 •

edited

Loading