-
Notifications
You must be signed in to change notification settings - Fork 67
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
Customized FHIR service configuration exceptions #150
Merged
Merged
Changes from all commits
Commits
Show all changes
15 commits
Select commit
Hold shift + click to select a range
7a17b45
Added FHIR service configuration exceptions.
kyclai fd12e4e
Renamed FhirServerError to FhirServiceError for consistency with prod…
kyclai d3b9cd9
Updated comment.
kyclai fe567bb
Localized exception messages.
kyclai 7ed01ca
Added unit tests.
kyclai 4810905
Updated where to catch exception for FHIR service authorization failure.
kyclai 8e3dc20
Addressed review comment.
kyclai 865f879
Merge branch 'master' into personal/clai/fhir-exceptions
kyclai 9ab3837
Addressed review comments.
kyclai 2a4abf1
Updated validation of 'ErrorSource' metric dimension.
kyclai eac686f
Removed underscores in error names for consistency with other error n…
kyclai 7fb024a
Merge branch 'master' into personal/clai/fhir-exceptions
kyclai 551e55f
Nit change to error message when FHIR service doesn't exist.
kyclai 5eedc22
Updated unit test as per PR comment.
kyclai d42d282
Re-throw exception in case it's being retried on.
kyclai File filter
Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
There are no files selected for viewing
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
32 changes: 32 additions & 0 deletions
32
src/lib/Microsoft.Health.Extensions.Fhir.R4/FhirServiceValidator.cs
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,32 @@ | ||
// ------------------------------------------------------------------------------------------------- | ||
// Copyright (c) Microsoft Corporation. All rights reserved. | ||
// Licensed under the MIT License (MIT). See LICENSE in the repo root for license information. | ||
// ------------------------------------------------------------------------------------------------- | ||
|
||
using System; | ||
using EnsureThat; | ||
using Hl7.Fhir.Rest; | ||
using Microsoft.Health.Extensions.Fhir.Telemetry.Exceptions; | ||
using Microsoft.Health.Logging.Telemetry; | ||
|
||
namespace Microsoft.Health.Extensions.Fhir | ||
{ | ||
public static class FhirServiceValidator | ||
{ | ||
public static bool ValidateFhirService(FhirClient client, ITelemetryLogger logger) | ||
pallar-ms marked this conversation as resolved.
Show resolved
Hide resolved
|
||
{ | ||
EnsureArg.IsNotNull(client, nameof(client)); | ||
|
||
try | ||
{ | ||
client.CapabilityStatement(SummaryType.True); | ||
return true; | ||
} | ||
catch (Exception exception) | ||
{ | ||
FhirServiceExceptionProcessor.ProcessException(exception, logger); | ||
return false; | ||
} | ||
} | ||
} | ||
} |
40 changes: 40 additions & 0 deletions
40
src/lib/Microsoft.Health.Extensions.Fhir.R4/Telemetry/Exceptions/FhirServiceErrorCode.cs
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,40 @@ | ||
// ------------------------------------------------------------------------------------------------- | ||
// Copyright (c) Microsoft Corporation. All rights reserved. | ||
// Licensed under the MIT License (MIT). See LICENSE in the repo root for license information. | ||
// ------------------------------------------------------------------------------------------------- | ||
|
||
namespace Microsoft.Health.Extensions.Fhir.Telemetry.Exceptions | ||
{ | ||
public enum FhirServiceErrorCode | ||
{ | ||
/// <summary> | ||
/// Error code that categorizes invalid configurations (e.g. invalid FHIR service URL) | ||
/// </summary> | ||
ConfigurationError, | ||
|
||
/// <summary> | ||
/// Error code that categorizes authorization errors (e.g. missing role with permission to write FHIR data) | ||
/// </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> | ||
GeneralError, | ||
} | ||
} |
105 changes: 105 additions & 0 deletions
105
...Microsoft.Health.Extensions.Fhir.R4/Telemetry/Exceptions/FhirServiceExceptionProcessor.cs
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,105 @@ | ||
// ------------------------------------------------------------------------------------------------- | ||
// Copyright (c) Microsoft Corporation. All rights reserved. | ||
// Licensed under the MIT License (MIT). See LICENSE in the repo root for license information. | ||
// ------------------------------------------------------------------------------------------------- | ||
|
||
using System; | ||
using System.Net; | ||
using System.Net.Http; | ||
using EnsureThat; | ||
using Hl7.Fhir.Rest; | ||
using Microsoft.Health.Common.Telemetry; | ||
using Microsoft.Health.Extensions.Fhir.Resources; | ||
using Microsoft.Health.Extensions.Fhir.Telemetry.Metrics; | ||
using Microsoft.Health.Logging.Telemetry; | ||
using Microsoft.Identity.Client; | ||
|
||
namespace Microsoft.Health.Extensions.Fhir.Telemetry.Exceptions | ||
{ | ||
public static class FhirServiceExceptionProcessor | ||
{ | ||
private static readonly IExceptionTelemetryProcessor _exceptionTelemetryProcessor = new ExceptionTelemetryProcessor(); | ||
|
||
public static void ProcessException(Exception exception, ITelemetryLogger logger) | ||
{ | ||
EnsureArg.IsNotNull(logger, nameof(logger)); | ||
|
||
var (customException, errorName) = CustomizeException(exception); | ||
|
||
logger.LogError(customException); | ||
|
||
string exceptionName = customException.Equals(exception) ? $"{ErrorType.FHIRServiceError}{errorName}" : customException.GetType().Name; | ||
_exceptionTelemetryProcessor.LogExceptionMetric(customException, logger, FhirClientMetrics.HandledException(exceptionName, ErrorSeverity.Critical)); | ||
} | ||
|
||
public static (Exception customException, string errorName) CustomizeException(Exception exception) | ||
{ | ||
EnsureArg.IsNotNull(exception, nameof(exception)); | ||
|
||
string message; | ||
string errorName; | ||
|
||
switch (exception) | ||
{ | ||
case FhirOperationException _: | ||
var status = ((FhirOperationException)exception).Status; | ||
switch (status) | ||
{ | ||
case HttpStatusCode.Forbidden: | ||
message = FhirResources.FhirServiceAccessForbidden; | ||
string helpLink = "https://docs.microsoft.com/azure/healthcare-apis/iot/deploy-iot-connector-in-azure#accessing-the-iot-connector-from-the-fhir-service"; | ||
errorName = nameof(FhirServiceErrorCode.AuthorizationError); | ||
return (new UnauthorizedAccessFhirServiceException(message, exception, helpLink, errorName), errorName); | ||
pallar-ms marked this conversation as resolved.
Show resolved
Hide resolved
|
||
case HttpStatusCode.NotFound: | ||
message = FhirResources.FhirServiceNotFound; | ||
errorName = nameof(FhirServiceErrorCode.ConfigurationError); | ||
return (new InvalidFhirServiceException(message, exception, errorName), errorName); | ||
default: | ||
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, $"{FhirServiceErrorCode.ArgumentError}{paramName}"); | ||
|
||
case UriFormatException _: | ||
message = FhirResources.FhirServiceUriFormatInvalid; | ||
errorName = nameof(FhirServiceErrorCode.ConfigurationError); | ||
return (new InvalidFhirServiceException(message, exception, errorName), errorName); | ||
|
||
case HttpRequestException _: | ||
// 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 _: | ||
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, $"{FhirServiceErrorCode.MsalServiceError}{errorCode}"); | ||
|
||
default: | ||
return (exception, nameof(FhirServiceErrorCode.GeneralError)); | ||
} | ||
} | ||
} | ||
} |
48 changes: 48 additions & 0 deletions
48
...b/Microsoft.Health.Extensions.Fhir.R4/Telemetry/Exceptions/InvalidFhirServiceException.cs
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,48 @@ | ||
// ------------------------------------------------------------------------------------------------- | ||
// Copyright (c) Microsoft Corporation. All rights reserved. | ||
// Licensed under the MIT License (MIT). See LICENSE in the repo root for license information. | ||
// ------------------------------------------------------------------------------------------------- | ||
|
||
using System; | ||
using Microsoft.Health.Common.Telemetry; | ||
using Microsoft.Health.Common.Telemetry.Exceptions; | ||
|
||
namespace Microsoft.Health.Extensions.Fhir.Telemetry.Exceptions | ||
{ | ||
public sealed class InvalidFhirServiceException : IomtTelemetryFormattableException | ||
{ | ||
private static readonly string _errorType = ErrorType.FHIRServiceError; | ||
|
||
public InvalidFhirServiceException() | ||
{ | ||
} | ||
|
||
public InvalidFhirServiceException(string message) | ||
: base(message) | ||
{ | ||
} | ||
|
||
public InvalidFhirServiceException(string message, Exception innerException) | ||
: base(message, innerException) | ||
{ | ||
} | ||
|
||
public InvalidFhirServiceException( | ||
string message, | ||
Exception innerException, | ||
string errorName) | ||
: base( | ||
message, | ||
innerException, | ||
name: $"{_errorType}{errorName}", | ||
operation: ConnectorOperation.FHIRConversion) | ||
{ | ||
} | ||
|
||
public override string ErrType => _errorType; | ||
|
||
public override string ErrSeverity => ErrorSeverity.Critical; | ||
|
||
public override string ErrSource => nameof(ErrorSource.User); | ||
} | ||
} |
50 changes: 50 additions & 0 deletions
50
....Health.Extensions.Fhir.R4/Telemetry/Exceptions/UnauthorizedAccessFhirServiceException.cs
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,50 @@ | ||
// ------------------------------------------------------------------------------------------------- | ||
// Copyright (c) Microsoft Corporation. All rights reserved. | ||
// Licensed under the MIT License (MIT). See LICENSE in the repo root for license information. | ||
// ------------------------------------------------------------------------------------------------- | ||
|
||
using System; | ||
using Microsoft.Health.Common.Telemetry; | ||
using Microsoft.Health.Common.Telemetry.Exceptions; | ||
|
||
namespace Microsoft.Health.Extensions.Fhir.Telemetry.Exceptions | ||
{ | ||
public sealed class UnauthorizedAccessFhirServiceException : IomtTelemetryFormattableException | ||
{ | ||
private static readonly string _errorType = ErrorType.FHIRServiceError; | ||
|
||
public UnauthorizedAccessFhirServiceException() | ||
{ | ||
} | ||
|
||
public UnauthorizedAccessFhirServiceException(string message) | ||
: base(message) | ||
{ | ||
} | ||
|
||
public UnauthorizedAccessFhirServiceException(string message, Exception innerException) | ||
: base(message, innerException) | ||
{ | ||
} | ||
|
||
public UnauthorizedAccessFhirServiceException( | ||
string message, | ||
Exception innerException, | ||
string helpLink, | ||
string errorName) | ||
: base( | ||
message, | ||
innerException, | ||
name: $"{_errorType}{errorName}", | ||
operation: ConnectorOperation.FHIRConversion) | ||
{ | ||
HelpLink = helpLink; | ||
} | ||
|
||
public override string ErrType => _errorType; | ||
|
||
public override string ErrSeverity => ErrorSeverity.Critical; | ||
|
||
public override string ErrSource => nameof(ErrorSource.User); | ||
} | ||
} |
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Oops, something went wrong.
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
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.
So currently if this fails, we just log the exception and return false, so what happens in the rest of the execution flow in that case?
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.
In this case (when
ValidateFhirService
catches an exception, gets the exception logged, and returns false),FhirClient
is returned (Line 90), as before these code changes.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.
Sure but I was trying to understand that if this validation failed and we still returned the client, then what happens in the ingestion flow? Do we encounter other exceptions that block the ingestion?
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.
As discussed offline, what happens when an invalid client is returned is something to further investigate later. For now, the previous behaviour is still retained: a client, whether valid or not, gets returned.