Addressed review comments.

microsoft · Jan 12, 2022 · 9ab3837 · 9ab3837
1 parent 865f879
commit 9ab3837
Show file tree

Hide file tree

Showing 6 changed files with 93 additions and 15 deletions.
diff --git a/src/lib/Microsoft.Health.Extensions.Fhir.R4/FhirClientFactory.cs b/src/lib/Microsoft.Health.Extensions.Fhir.R4/FhirClientFactory.cs
@@ -76,19 +76,17 @@ private static FhirClient CreateClient(TokenCredential tokenCredential, ITelemet
                 PreferredFormat = ResourceFormat.Json,
             };
 
-            FhirClient client;
+            FhirClient client = null;
             try
             {
                 client = new FhirClient(url, fhirClientSettings, new BearerTokenAuthorizationMessageHandler(uri, tokenCredential, logger));
+                FhirServiceValidator.ValidateFhirService(client, logger);
             }
             catch (Exception ex)
             {
-                client = null;
                 FhirServiceExceptionProcessor.ProcessException(ex, logger);
             }
 
-            FhirServiceValidator.ValidateFhirService(client, logger);
-
             return client;
         }
 

diff --git a/src/lib/Microsoft.Health.Extensions.Fhir.R4/Telemetry/Exceptions/FhirServiceErrorCode.cs b/src/lib/Microsoft.Health.Extensions.Fhir.R4/Telemetry/Exceptions/FhirServiceErrorCode.cs
@@ -17,6 +17,21 @@ public enum FhirServiceErrorCode
         /// </summary>
         AuthorizationError,
 
+        /// <summary>
+        /// Error code that categorizes invalid arguments (i.e. exceptions encountered of type ArgumentException), which may occur when FhirClient's endpoint is validated
+        /// </summary>
+        ArgumentError,
+
+        /// <summary>
+        /// Error code that categorizes HTTP request exceptions (i.e. exceptions encountered of type HttpRequestException)
+        /// </summary>
+        HttpRequestError,
+
+        /// <summary>
+        /// Error code that categorizes MSAL.NET exceptions (i.e. exceptions encountered of type MsalServiceException)
+        /// </summary>
+        MsalServiceError,
+
         /// <summary>
         /// Error code that categorizes all other generic exceptions
         /// </summary>

diff --git a/...Microsoft.Health.Extensions.Fhir.R4/Telemetry/Exceptions/FhirServiceExceptionProcessor.cs b/...Microsoft.Health.Extensions.Fhir.R4/Telemetry/Exceptions/FhirServiceExceptionProcessor.cs
@@ -58,21 +58,44 @@ public static (Exception customException, string errorName) CustomizeException(E
                             return (exception, status.ToString());
                     }
 
+                case ArgumentException _:
+                    var paramName = ((ArgumentException)exception).ParamName;
+                    if (paramName.Contains("endpoint", StringComparison.OrdinalIgnoreCase))
+                    {
+                        message = FhirResources.FhirServiceEndpointInvalid;
+                        errorName = nameof(FhirServiceErrorCode.ConfigurationError);
+                        return (new InvalidFhirServiceException(message, exception, errorName), errorName);
+                    }
+
+                    return (exception, $"{nameof(FhirServiceErrorCode.ArgumentError)}_{paramName}");
+
                 case UriFormatException _:
                     message = FhirResources.FhirServiceUriFormatInvalid;
                     errorName = nameof(FhirServiceErrorCode.ConfigurationError);
                     return (new InvalidFhirServiceException(message, exception, errorName), errorName);
 
                 case HttpRequestException _:
-                    message = FhirResources.FhirServiceHttpRequestError;
-                    errorName = nameof(FhirServiceErrorCode.ConfigurationError);
-                    return (new InvalidFhirServiceException(message, exception, errorName), errorName);
+                    // TODO: In .NET 5 and later, check HttpRequestException's StatusCode property instead of the Message property
+                    if (exception.Message.Contains(FhirResources.HttpRequestErrorNotKnown, StringComparison.CurrentCultureIgnoreCase))
+                    {
+                        message = FhirResources.FhirServiceHttpRequestError;
+                        errorName = nameof(FhirServiceErrorCode.ConfigurationError);
+                        return (new InvalidFhirServiceException(message, exception, errorName), errorName);
+                    }
+
+                    return (exception, $"{nameof(FhirServiceErrorCode.HttpRequestError)}");
 
                 case MsalServiceException _:
-                    string errorCode = ((MsalServiceException)exception).ErrorCode;
-                    message = FhirResources.FhirServiceMsalServiceError;
-                    errorName = $"{nameof(FhirServiceErrorCode.ConfigurationError)}_{errorCode}";
-                    return (new InvalidFhirServiceException(message, exception, errorName), errorName);
+                    var errorCode = ((MsalServiceException)exception).ErrorCode;
+                    if (string.Equals(errorCode, "invalid_resource", StringComparison.OrdinalIgnoreCase)
+                        || string.Equals(errorCode, "invalid_scope", StringComparison.OrdinalIgnoreCase))
+                    {
+                        message = FhirResources.FhirServiceMsalServiceError;
+                        errorName = $"{nameof(FhirServiceErrorCode.ConfigurationError)}";
+                        return (new InvalidFhirServiceException(message, exception, errorName), errorName);
+                    }
+
+                    return (exception, $"{nameof(FhirServiceErrorCode.MsalServiceError)}_{errorCode}");
 
                 default:
                     return (exception, nameof(FhirServiceErrorCode.GeneralError));

diff --git a/src/lib/Microsoft.Health.Extensions.Fhir/Resources/FhirResources.Designer.cs b/src/lib/Microsoft.Health.Extensions.Fhir/Resources/FhirResources.Designer.cs
diff --git a/src/lib/Microsoft.Health.Extensions.Fhir/Resources/FhirResources.resx b/src/lib/Microsoft.Health.Extensions.Fhir/Resources/FhirResources.resx
@@ -120,6 +120,9 @@
   <data name="FhirServiceAccessForbidden" xml:space="preserve">
     <value>Verify that the provided FHIR service's 'FHIR Data Writer' role has been assigned to the applicable Azure Active Directory security principal or managed identity.</value>
   </data>
+  <data name="FhirServiceEndpointInvalid" xml:space="preserve">
+    <value>Verify that the FHIR service URL is provided and is an absolute URL.</value>
+  </data>
   <data name="FhirServiceHttpRequestError" xml:space="preserve">
     <value>Verify that the provided FHIR service URL exists.</value>
   </data>
@@ -132,4 +135,7 @@
   <data name="FhirServiceUriFormatInvalid" xml:space="preserve">
     <value>Verify that the provided FHIR service URL is an absolute URL.</value>
   </data>
+  <data name="HttpRequestErrorNotKnown" xml:space="preserve">
+    <value>Name or service not known</value>
+  </data>
 </root>
diff --git a/test/Microsoft.Health.Extensions.Fhir.R4.UnitTests/FhirServiceExceptionProcessorTests.cs b/test/Microsoft.Health.Extensions.Fhir.R4.UnitTests/FhirServiceExceptionProcessorTests.cs
@@ -22,8 +22,14 @@ public class FhirServiceExceptionProcessorTests
         private static readonly Exception _fhirForbiddenEx = new FhirOperationException("test", HttpStatusCode.Forbidden);
         private static readonly Exception _fhirNotFoundEx = new FhirOperationException("test", HttpStatusCode.NotFound);
         private static readonly Exception _fhirBadRequestEx = new FhirOperationException("test", HttpStatusCode.BadRequest);
+        private static readonly Exception _argEndpointNullEx = new ArgumentNullException("endpoint");
+        private static readonly Exception _argEndpointEx = new ArgumentException("endpoint", "Endpoint must be absolute");
+        private static readonly Exception _argEx = new ArgumentException("test_message", "test_param");
         private static readonly Exception _uriEx = new UriFormatException();
+        private static readonly Exception _httpNotKnownEx = new HttpRequestException("Name or service not known");
         private static readonly Exception _httpEx = new HttpRequestException();
+        private static readonly Exception _msalInvalidResourceEx = new MsalServiceException("invalid_resource", "test_message");
+        private static readonly Exception _msalInvalidScopeEx = new MsalServiceException("invalid_scope", "test_message");
         private static readonly Exception _msalEx = new MsalServiceException("test_code", "test_message");
         private static readonly Exception _ex = new Exception();
 
@@ -33,9 +39,15 @@ public class FhirServiceExceptionProcessorTests
                 new object[] { _fhirForbiddenEx, "FHIRServiceErrorAuthorizationError" },
                 new object[] { _fhirNotFoundEx, "FHIRServiceErrorConfigurationError", nameof(ErrorSource.User) },
                 new object[] { _fhirBadRequestEx, "FHIRServiceErrorBadRequest" },
+                new object[] { _argEndpointNullEx, "FHIRServiceErrorConfigurationError", nameof(ErrorSource.User) },
+                new object[] { _argEndpointEx, "FHIRServiceErrorConfigurationError", nameof(ErrorSource.User) },
+                new object[] { _argEx, "FHIRServiceErrorArgumentError_test_param" },
                 new object[] { _uriEx, "FHIRServiceErrorConfigurationError", nameof(ErrorSource.User) },
-                new object[] { _httpEx, "FHIRServiceErrorConfigurationError", nameof(ErrorSource.User) },
-                new object[] { _msalEx, "FHIRServiceErrorConfigurationError_test_code", nameof(ErrorSource.User) },
+                new object[] { _httpNotKnownEx, "FHIRServiceErrorConfigurationError", nameof(ErrorSource.User) },
+                new object[] { _httpEx, "FHIRServiceErrorHttpRequestError" },
+                new object[] { _msalInvalidResourceEx, "FHIRServiceErrorConfigurationError", nameof(ErrorSource.User) },
+                new object[] { _msalInvalidScopeEx, "FHIRServiceErrorConfigurationError", nameof(ErrorSource.User) },
+                new object[] { _msalEx, "FHIRServiceErrorMsalServiceError_test_code" },
                 new object[] { _ex, "FHIRServiceErrorGeneralError" },
             };
 
@@ -45,9 +57,15 @@ public class FhirServiceExceptionProcessorTests
                 new object[] { _fhirForbiddenEx, typeof(UnauthorizedAccessFhirServiceException) },
                 new object[] { _fhirNotFoundEx, typeof(InvalidFhirServiceException) },
                 new object[] { _fhirBadRequestEx, typeof(FhirOperationException) },
+                new object[] { _argEndpointNullEx, typeof(InvalidFhirServiceException) },
+                new object[] { _argEndpointEx, typeof(InvalidFhirServiceException) },
+                new object[] { _argEx, typeof(ArgumentException) },
                 new object[] { _uriEx, typeof(InvalidFhirServiceException) },
-                new object[] { _httpEx, typeof(InvalidFhirServiceException) },
-                new object[] { _msalEx, typeof(InvalidFhirServiceException) },
+                new object[] { _httpNotKnownEx, typeof(InvalidFhirServiceException) },
+                new object[] { _httpEx, typeof(HttpRequestException) },
+                new object[] { _msalInvalidResourceEx, typeof(InvalidFhirServiceException) },
+                new object[] { _msalInvalidScopeEx, typeof(InvalidFhirServiceException) },
+                new object[] { _msalEx, typeof(MsalServiceException) },
                 new object[] { _ex, typeof(Exception) },
             };