From a134481872bd85894f01df6b88843312e6030109 Mon Sep 17 00:00:00 2001 From: Dustin Burson <47367432+dustinburson@users.noreply.github.com> Date: Fri, 22 Apr 2022 16:43:19 -0700 Subject: [PATCH] Improve Observation Value Merge Type Mismatch Errors (#187) * Add additional error information on existing type when merge value detect type mismatch. * Add additional check to fail observation save if the observation found on the initial get doesn't contain the desired identifier. * Update failing tests to properly mock data to pass the new identifier check. --- .../Template/FhirValueProcessor.cs | 6 +++++ .../CodeableConceptFhirValueProcessor.cs | 2 +- .../Template/QuantityFhirValueProcessor.cs | 2 +- .../Template/SampledDataFhirValueProcessor.cs | 2 +- .../Template/StringFhirValueProcessor.cs | 2 +- .../Service/R4FhirImportService.cs | 6 +++++ .../Service/R4FhirImportServiceTests.cs | 25 +++++++++++-------- 7 files changed, 31 insertions(+), 14 deletions(-) diff --git a/src/lib/Microsoft.Health.Fhir.Ingest/Template/FhirValueProcessor.cs b/src/lib/Microsoft.Health.Fhir.Ingest/Template/FhirValueProcessor.cs index ca5864d4..b68df027 100644 --- a/src/lib/Microsoft.Health.Fhir.Ingest/Template/FhirValueProcessor.cs +++ b/src/lib/Microsoft.Health.Fhir.Ingest/Template/FhirValueProcessor.cs @@ -11,6 +11,7 @@ namespace Microsoft.Health.Fhir.Ingest.Template public abstract class FhirValueProcessor : IFhirValueProcessor where TValueType : FhirValueType { + private const string NullValueText = "null"; private static readonly Type SupportedType = typeof(TValueType); public Type SupportedValueType => SupportedType; @@ -40,5 +41,10 @@ protected TValueType CastTemplate(FhirValueType template) protected abstract TOutValue CreateValueImpl(TValueType template, TInValue inValue); protected abstract TOutValue MergeValueImpl(TValueType template, TInValue inValue, TOutValue existingValue); + + protected static string GetValueTypeName(object value) + { + return value?.GetType()?.Name ?? NullValueText; + } } } diff --git a/src/lib/Microsoft.Health.Fhir.R4.Ingest.Templates/Template/CodeableConceptFhirValueProcessor.cs b/src/lib/Microsoft.Health.Fhir.R4.Ingest.Templates/Template/CodeableConceptFhirValueProcessor.cs index 3f8c05ab..b34d8dd2 100644 --- a/src/lib/Microsoft.Health.Fhir.R4.Ingest.Templates/Template/CodeableConceptFhirValueProcessor.cs +++ b/src/lib/Microsoft.Health.Fhir.R4.Ingest.Templates/Template/CodeableConceptFhirValueProcessor.cs @@ -35,7 +35,7 @@ protected override DataType MergeValueImpl(CodeableConceptFhirValueType template { if (!(existingValue is CodeableConcept)) { - throw new NotSupportedException($"Element {nameof(existingValue)} expected to be of type {typeof(CodeableConcept)}."); + throw new NotSupportedException($"Element {nameof(existingValue)} expected to be of type {typeof(CodeableConcept)}. Actual type is {GetValueTypeName(existingValue)}."); } // Currently no way to merge codeable concepts. Just replace. diff --git a/src/lib/Microsoft.Health.Fhir.R4.Ingest.Templates/Template/QuantityFhirValueProcessor.cs b/src/lib/Microsoft.Health.Fhir.R4.Ingest.Templates/Template/QuantityFhirValueProcessor.cs index 913768c6..6978964e 100644 --- a/src/lib/Microsoft.Health.Fhir.R4.Ingest.Templates/Template/QuantityFhirValueProcessor.cs +++ b/src/lib/Microsoft.Health.Fhir.R4.Ingest.Templates/Template/QuantityFhirValueProcessor.cs @@ -46,7 +46,7 @@ protected override DataType MergeValueImpl(QuantityFhirValueType template, IObse { if (!(existingValue is Quantity)) { - throw new NotSupportedException($"Element {nameof(existingValue)} expected to be of type {typeof(Quantity)}."); + throw new NotSupportedException($"Element {nameof(existingValue)} expected to be of type {typeof(Quantity)}. Actual type is {GetValueTypeName(existingValue)}."); } // Only a single value, just replace. diff --git a/src/lib/Microsoft.Health.Fhir.R4.Ingest.Templates/Template/SampledDataFhirValueProcessor.cs b/src/lib/Microsoft.Health.Fhir.R4.Ingest.Templates/Template/SampledDataFhirValueProcessor.cs index 56141244..06f7fbbe 100644 --- a/src/lib/Microsoft.Health.Fhir.R4.Ingest.Templates/Template/SampledDataFhirValueProcessor.cs +++ b/src/lib/Microsoft.Health.Fhir.R4.Ingest.Templates/Template/SampledDataFhirValueProcessor.cs @@ -46,7 +46,7 @@ protected override DataType MergeValueImpl(SampledDataFhirValueType template, IO if (!(existingValue is SampledData sampledData)) { - throw new NotSupportedException($"Element {nameof(existingValue)} expected to be of type {typeof(SampledData)}."); + throw new NotSupportedException($"Element {nameof(existingValue)} expected to be of type {typeof(SampledData)}. Actual type is {GetValueTypeName(existingValue)}."); } if (sampledData.Dimensions > 1) diff --git a/src/lib/Microsoft.Health.Fhir.R4.Ingest.Templates/Template/StringFhirValueProcessor.cs b/src/lib/Microsoft.Health.Fhir.R4.Ingest.Templates/Template/StringFhirValueProcessor.cs index 0931caa8..40577762 100644 --- a/src/lib/Microsoft.Health.Fhir.R4.Ingest.Templates/Template/StringFhirValueProcessor.cs +++ b/src/lib/Microsoft.Health.Fhir.R4.Ingest.Templates/Template/StringFhirValueProcessor.cs @@ -26,7 +26,7 @@ protected override DataType MergeValueImpl(StringFhirValueType template, IObserv { if (!(existingValue is FhirString)) { - throw new NotSupportedException($"Element {nameof(existingValue)} expected to be of type {typeof(FhirString)}."); + throw new NotSupportedException($"Element {nameof(existingValue)} expected to be of type {typeof(FhirString)}. Actual type is {GetValueTypeName(existingValue)}."); } // Only a single value, just replace. diff --git a/src/lib/Microsoft.Health.Fhir.R4.Ingest/Service/R4FhirImportService.cs b/src/lib/Microsoft.Health.Fhir.R4.Ingest/Service/R4FhirImportService.cs index 07396721..62c7ed73 100644 --- a/src/lib/Microsoft.Health.Fhir.R4.Ingest/Service/R4FhirImportService.cs +++ b/src/lib/Microsoft.Health.Fhir.R4.Ingest/Service/R4FhirImportService.cs @@ -76,6 +76,12 @@ public virtual async Task SaveObservationAsync(ILookupTemplate i.IsExactly(identifier))) + { + throw new NotSupportedException("FHIR Service returned matching observation but expected identifier was not present."); + } } var policyResult = await Policy<(Model.Observation observation, ResourceOperation operationType)> diff --git a/test/Microsoft.Health.Fhir.R4.Ingest.UnitTests/Service/R4FhirImportServiceTests.cs b/test/Microsoft.Health.Fhir.R4.Ingest.UnitTests/Service/R4FhirImportServiceTests.cs index 5dc27593..b7d9fb25 100644 --- a/test/Microsoft.Health.Fhir.R4.Ingest.UnitTests/Service/R4FhirImportServiceTests.cs +++ b/test/Microsoft.Health.Fhir.R4.Ingest.UnitTests/Service/R4FhirImportServiceTests.cs @@ -113,14 +113,7 @@ public async void GivenFoundObservation_WhenSaveObservationAsync_ThenUpdateInvok Device = new ResourceReference(@"Device/123"), Subject = new ResourceReference(@"Patient/abc"), Status = ObservationStatus.Final, - Identifier = new List - { - new Identifier - { - System = "Test", - Value = "id", - }, - }, + Identifier = BuildIdentifiers(), Effective = new Period(new FhirDateTime(DateTimeOffset.Now.AddHours(-1)), new FhirDateTime(DateTimeOffset.Now)), }; @@ -170,7 +163,7 @@ public async void GivenFoundObservation_WhenSaveObservationAsync_ThenUpdateInvok [Fact] public async void GivenFoundObservationAndConflictOnSave_WhenSaveObservationAsync_ThenGetAndUpdateInvoked_Test() { - var foundObservation1 = new Observation { Id = "1" }; + var foundObservation1 = new Observation { Id = "1", Identifier = BuildIdentifiers() }; var foundBundle1 = new Bundle { Entry = new List @@ -350,7 +343,7 @@ public async Task GivenCachedObservationUnchanged_WhenGenerateObservation_ThenCa [Fact] public async void GivenFoundObservationUnchanged_WhenSaveObservationAsync_ThenUpdateInvoked_Test() { - var foundObservation = new Observation { Id = "1" }; + var foundObservation = new Observation { Id = "1", Identifier = BuildIdentifiers() }; var foundBundle = new Bundle { Entry = new List @@ -398,6 +391,18 @@ private static IDictionary BuildIdCollection() return lookup; } + private static List BuildIdentifiers() + { + return new List + { + new Identifier + { + System = "https://azure.microsoft.com/en-us/services/iomt-fhir-connector/", + Value = "patientId.deviceId..", + }, + }; + } + private static Observation ThrowConflictException() { throw new FhirException(new FhirResponse(new HttpResponseMessage(HttpStatusCode.Conflict), new OperationOutcome()));