From d6b2c9a27b89e6442b66f23d1b75ce4fd98642d3 Mon Sep 17 00:00:00 2001 From: Dustin Burson Date: Fri, 15 Apr 2022 12:31:05 -0700 Subject: [PATCH 1/3] * Address issue #184, observations not being updated when changed. - Defect was due to using two different references to the same object for the change comparison. - Updated CodeValueFhirTemplateProcessor.MergeObservation to perform a DeepCopy of the passed in Observation parameter. The copy is what is returned now by the function to ensure modifications are made to a new instance. - Moved the logic to set the status of the Observation from the MergeObservation function to the R4FhirImportService.SaveObservationAsync logic. Without the move, the first time a created observation is "updated" will always result in the FHIR resource being saved even if nothing else changed because we were always setting the status to amended. - Updated unit tests to account for new logic. - Added extension methods for some common scenarios * Rename FhirClientValidator to FhirClientValidatorExtensions to match other extension class names * Rename ServiceCollectionExtrensions to FhirClientExtensions to match purpose. * Merge content of HttpClientBuilderRegistraionExtensions into FhirClientExtensions. * Remove registering ITelemetryLogger singleton with FhirClient. ITelemetryLogger is resolved from the service provider now. This was preventing local debugging with the Azure Function. * Update AddAuthenticationHandler to use the registered singletons for the auth service instead of creating new instances. --- ...nExtensions.cs => FhirClientExtensions.cs} | 45 +++--- ...or.cs => FhirClientValidatorExtensions.cs} | 2 +- ...HttpClientBuilderRegistrationExtensions.cs | 42 ------ .../ObservationExtensions.cs | 34 +++++ .../ResourceExtensions.cs | 27 ++++ .../CodeValueFhirTemplateProcessor.cs | 22 +-- .../Service/R4FhirImportService.cs | 4 +- .../Service/R4FhirImportServiceTests.cs | 140 +++++++++++------- .../CodeValueFhirTemplateProcessorTests.cs | 20 +-- 9 files changed, 199 insertions(+), 137 deletions(-) rename src/lib/Microsoft.Health.Extensions.Fhir.R4/{ServiceCollectionExtensions.cs => FhirClientExtensions.cs} (51%) rename src/lib/Microsoft.Health.Extensions.Fhir.R4/{FhirClientValidator.cs => FhirClientValidatorExtensions.cs} (95%) delete mode 100644 src/lib/Microsoft.Health.Extensions.Fhir.R4/HttpClientBuilderRegistrationExtensions.cs create mode 100644 src/lib/Microsoft.Health.Extensions.Fhir.R4/ObservationExtensions.cs create mode 100644 src/lib/Microsoft.Health.Extensions.Fhir.R4/ResourceExtensions.cs diff --git a/src/lib/Microsoft.Health.Extensions.Fhir.R4/ServiceCollectionExtensions.cs b/src/lib/Microsoft.Health.Extensions.Fhir.R4/FhirClientExtensions.cs similarity index 51% rename from src/lib/Microsoft.Health.Extensions.Fhir.R4/ServiceCollectionExtensions.cs rename to src/lib/Microsoft.Health.Extensions.Fhir.R4/FhirClientExtensions.cs index 0fdc7860..b73741f9 100644 --- a/src/lib/Microsoft.Health.Extensions.Fhir.R4/ServiceCollectionExtensions.cs +++ b/src/lib/Microsoft.Health.Extensions.Fhir.R4/FhirClientExtensions.cs @@ -15,9 +15,9 @@ namespace Microsoft.Health.Extensions.Fhir { - public static class ServiceCollectionExtensions + public static class FhirClientExtensions { - public static void AddFhirClient(this IServiceCollection serviceCollection, IConfiguration configuration) + public static IServiceCollection AddFhirClient(this IServiceCollection serviceCollection, IConfiguration configuration) { EnsureArg.IsNotNull(serviceCollection, nameof(serviceCollection)); EnsureArg.IsNotNull(configuration, nameof(configuration)); @@ -25,35 +25,46 @@ public static void AddFhirClient(this IServiceCollection serviceCollection, ICon var url = new Uri(configuration.GetValue("FhirService:Url")); bool useManagedIdentity = configuration.GetValue("FhirClient:UseManagedIdentity"); - serviceCollection.AddSingleton(typeof(ITelemetryLogger), typeof(IomtTelemetryLogger)); - var serviceProvider = serviceCollection.BuildServiceProvider(); - var logger = serviceProvider.GetRequiredService(); - - serviceCollection.AddHttpClient(client => + serviceCollection.AddHttpClient((client, sp) => { client.BaseAddress = url; client.Timeout = TimeSpan.FromSeconds(60); + var logger = sp.GetRequiredService(); + // Using discard because we don't need result var fhirClient = new FhirClient(client); _ = fhirClient.ValidateFhirClientAsync(logger); + return fhirClient; }) - .AddAuthenticationHandler(serviceCollection, logger, url, useManagedIdentity); - } - - public static void AddNamedManagedIdentityCredentialProvider(this IServiceCollection serviceCollection) - { - EnsureArg.IsNotNull(serviceCollection, nameof(serviceCollection)); + .AddAuthenticationHandler(serviceCollection, url, useManagedIdentity); - serviceCollection.TryAddSingleton(); + return serviceCollection; } - public static void AddNamedOAuth2ClientCredentialProvider(this IServiceCollection serviceCollection) + public static void AddAuthenticationHandler( + this IHttpClientBuilder httpClientBuilder, + IServiceCollection services, + Uri uri, + bool useManagedIdentity) { - EnsureArg.IsNotNull(serviceCollection, nameof(serviceCollection)); + EnsureArg.IsNotNull(httpClientBuilder, nameof(httpClientBuilder)); + EnsureArg.IsNotNull(services, nameof(services)); + EnsureArg.IsNotNull(uri, nameof(uri)); - serviceCollection.TryAddSingleton(); + if (useManagedIdentity) + { + services.TryAddSingleton(new ManagedIdentityAuthService()); + httpClientBuilder.AddHttpMessageHandler(sp => + new BearerTokenAuthorizationMessageHandler(uri, sp.GetRequiredService(), sp.GetRequiredService())); + } + else + { + services.TryAddSingleton(new OAuthConfidentialClientAuthService()); + httpClientBuilder.AddHttpMessageHandler(sp => + new BearerTokenAuthorizationMessageHandler(uri, sp.GetRequiredService(), sp.GetRequiredService())); + } } } } diff --git a/src/lib/Microsoft.Health.Extensions.Fhir.R4/FhirClientValidator.cs b/src/lib/Microsoft.Health.Extensions.Fhir.R4/FhirClientValidatorExtensions.cs similarity index 95% rename from src/lib/Microsoft.Health.Extensions.Fhir.R4/FhirClientValidator.cs rename to src/lib/Microsoft.Health.Extensions.Fhir.R4/FhirClientValidatorExtensions.cs index 3461d967..bf52c493 100644 --- a/src/lib/Microsoft.Health.Extensions.Fhir.R4/FhirClientValidator.cs +++ b/src/lib/Microsoft.Health.Extensions.Fhir.R4/FhirClientValidatorExtensions.cs @@ -12,7 +12,7 @@ namespace Microsoft.Health.Extensions.Fhir { - public static class FhirClientValidator + public static class FhirClientValidatorExtensions { public static async Task ValidateFhirClientAsync( this IFhirClient client, diff --git a/src/lib/Microsoft.Health.Extensions.Fhir.R4/HttpClientBuilderRegistrationExtensions.cs b/src/lib/Microsoft.Health.Extensions.Fhir.R4/HttpClientBuilderRegistrationExtensions.cs deleted file mode 100644 index a334f275..00000000 --- a/src/lib/Microsoft.Health.Extensions.Fhir.R4/HttpClientBuilderRegistrationExtensions.cs +++ /dev/null @@ -1,42 +0,0 @@ -// ------------------------------------------------------------------------------------------------- -// 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 Microsoft.Extensions.DependencyInjection; -using Microsoft.Health.Extensions.Host.Auth; -using Microsoft.Health.Logging.Telemetry; - -namespace Microsoft.Health.Extensions.Fhir -{ - public static class HttpClientBuilderRegistrationExtensions - { - public static void AddAuthenticationHandler( - this IHttpClientBuilder httpClientBuilder, - IServiceCollection services, - ITelemetryLogger logger, - Uri uri, - bool useManagedIdentity) - { - EnsureArg.IsNotNull(httpClientBuilder, nameof(httpClientBuilder)); - EnsureArg.IsNotNull(services, nameof(services)); - EnsureArg.IsNotNull(logger, nameof(logger)); - EnsureArg.IsNotNull(uri, nameof(uri)); - - if (useManagedIdentity) - { - services.AddNamedManagedIdentityCredentialProvider(); - httpClientBuilder.AddHttpMessageHandler(x => - new BearerTokenAuthorizationMessageHandler(uri, new ManagedIdentityAuthService(), logger)); - } - else - { - services.AddNamedOAuth2ClientCredentialProvider(); - httpClientBuilder.AddHttpMessageHandler(x => - new BearerTokenAuthorizationMessageHandler(uri, new OAuthConfidentialClientAuthService(), logger)); - } - } - } -} diff --git a/src/lib/Microsoft.Health.Extensions.Fhir.R4/ObservationExtensions.cs b/src/lib/Microsoft.Health.Extensions.Fhir.R4/ObservationExtensions.cs new file mode 100644 index 00000000..29757769 --- /dev/null +++ b/src/lib/Microsoft.Health.Extensions.Fhir.R4/ObservationExtensions.cs @@ -0,0 +1,34 @@ +// ------------------------------------------------------------------------------------------------- +// Copyright (c) Microsoft Corporation. All rights reserved. +// Licensed under the MIT License (MIT). See LICENSE in the repo root for license information. +// ------------------------------------------------------------------------------------------------- + +using EnsureThat; +using Hl7.Fhir.Model; + +namespace Microsoft.Health.Extensions.Fhir +{ + public static class ObservationExtensions + { + /// + /// Compares two observations to see if they are different. If they are different the status is changed to Amended. + /// + /// The original unmodified observation. + /// The potentially modified observation. + /// Returns true if the is different than the . Otherwise false is returned. + public static bool AmendIfChanged(this Observation originalObservation, Observation updatedObservation) + { + EnsureArg.IsNotNull(originalObservation, nameof(originalObservation)); + EnsureArg.IsNotNull(updatedObservation, nameof(updatedObservation)); + EnsureArg.IsFalse(originalObservation == updatedObservation, optsFn: o => o.WithMessage($"Parameters {nameof(originalObservation)} and {nameof(updatedObservation)} are the same reference.")); + + if (!originalObservation.IsExactly(updatedObservation)) + { + updatedObservation.Status = ObservationStatus.Amended; + return true; + } + + return false; + } + } +} diff --git a/src/lib/Microsoft.Health.Extensions.Fhir.R4/ResourceExtensions.cs b/src/lib/Microsoft.Health.Extensions.Fhir.R4/ResourceExtensions.cs new file mode 100644 index 00000000..118f1830 --- /dev/null +++ b/src/lib/Microsoft.Health.Extensions.Fhir.R4/ResourceExtensions.cs @@ -0,0 +1,27 @@ +// ------------------------------------------------------------------------------------------------- +// Copyright (c) Microsoft Corporation. All rights reserved. +// Licensed under the MIT License (MIT). See LICENSE in the repo root for license information. +// ------------------------------------------------------------------------------------------------- + +using EnsureThat; +using Hl7.Fhir.Model; + +namespace Microsoft.Health.Extensions.Fhir +{ + public static class ResourceExtensions + { + /// + /// Performs full deep copy of the resource. + /// + /// Type of resource to return. + /// Resource to copy. + /// New resource object with the contents of the original. + public static TResource FullCopy(this TResource resource) + where TResource : class, IDeepCopyable + { + EnsureArg.IsNotNull(resource, nameof(resource)); + + return resource.DeepCopy() as TResource; + } + } +} diff --git a/src/lib/Microsoft.Health.Fhir.R4.Ingest.Templates/Template/CodeValueFhirTemplateProcessor.cs b/src/lib/Microsoft.Health.Fhir.R4.Ingest.Templates/Template/CodeValueFhirTemplateProcessor.cs index d4586fc7..10f272e3 100644 --- a/src/lib/Microsoft.Health.Fhir.R4.Ingest.Templates/Template/CodeValueFhirTemplateProcessor.cs +++ b/src/lib/Microsoft.Health.Fhir.R4.Ingest.Templates/Template/CodeValueFhirTemplateProcessor.cs @@ -128,42 +128,42 @@ protected override Observation MergeObservationImpl(CodeValueFhirTemplate templa EnsureArg.IsNotNull(grp, nameof(grp)); EnsureArg.IsNotNull(existingObservation, nameof(existingObservation)); - existingObservation.Status = ObservationStatus.Amended; + var mergedObservation = existingObservation.DeepCopy() as Observation; - existingObservation.Category = null; + mergedObservation.Category = null; if (template?.Category?.Count > 0) { - existingObservation.Category = ResolveCategory(template.Category); + mergedObservation.Category = ResolveCategory(template.Category); } var values = grp.GetValues(); - (DateTime start, DateTime end) observationPeriod = GetObservationPeriod(existingObservation); + (DateTime start, DateTime end) observationPeriod = GetObservationPeriod(mergedObservation); // Update observation value if (!string.IsNullOrWhiteSpace(template?.Value?.ValueName) && values.TryGetValue(template?.Value?.ValueName, out var obValues)) { - existingObservation.Value = _valueProcessor.MergeValue(template.Value, CreateMergeData(grp.Boundary, observationPeriod, obValues), existingObservation.Value); + mergedObservation.Value = _valueProcessor.MergeValue(template.Value, CreateMergeData(grp.Boundary, observationPeriod, obValues), mergedObservation.Value); } // Update observation component values if (template?.Components?.Count > 0) { - if (existingObservation.Component == null) + if (mergedObservation.Component == null) { - existingObservation.Component = new List(template.Components.Count); + mergedObservation.Component = new List(template.Components.Count); } foreach (var component in template.Components) { if (values.TryGetValue(component.Value.ValueName, out var compValues)) { - var foundComponent = existingObservation.Component + var foundComponent = mergedObservation.Component .Where(c => c.Code.Coding.Any(code => code.Code == component.Value.ValueName && code.System == FhirImportService.ServiceSystem)) .FirstOrDefault(); if (foundComponent == null) { - existingObservation.Component.Add( + mergedObservation.Component.Add( new Observation.ComponentComponent { Code = ResolveCode(component.Value.ValueName, component.Codes), @@ -189,9 +189,9 @@ protected override Observation MergeObservationImpl(CodeValueFhirTemplate templa observationPeriod.end = grp.Boundary.End; } - existingObservation.Effective = observationPeriod.ToPeriod(); + mergedObservation.Effective = observationPeriod.ToPeriod(); - return existingObservation; + return mergedObservation; } protected override IEnumerable CreateObservationGroupsImpl(CodeValueFhirTemplate template, IMeasurementGroup measurementGroup) 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 2f972c61..ddac7041 100644 --- a/src/lib/Microsoft.Health.Fhir.R4.Ingest/Service/R4FhirImportService.cs +++ b/src/lib/Microsoft.Health.Fhir.R4.Ingest/Service/R4FhirImportService.cs @@ -113,8 +113,8 @@ public virtual async Task SaveObservationAsync(ILookupTemplate(), }; - var templateProcessor = Substitute.For, Model.Observation>>() + var templateProcessor = Substitute.For, Observation>>() .Mock(m => m.CreateObservationGroups(default, default).ReturnsForAnyArgs(observationGroups)) - .Mock(m => m.CreateObservation(default, default).ReturnsForAnyArgs(new Model.Observation())); + .Mock(m => m.CreateObservation(default, default).ReturnsForAnyArgs(new Observation())); var cache = Substitute.For(); var measurementGroup = Substitute.For(); @@ -68,7 +70,7 @@ public async void GivenValidData_WhenProcessAsync_ThenSaveObservationInvokedForE public async void GivenNotFoundObservation_WhenSaveObservationAsync_ThenCreateInvoked_Test() { var fhirClient = Utilities.CreateMockFhirService(); - fhirClient.CreateResourceAsync(Arg.Any()).ReturnsForAnyArgs(Task.FromResult(new Model.Observation())); + fhirClient.CreateResourceAsync(Arg.Any()).ReturnsForAnyArgs(Task.FromResult(new Observation())); var ids = BuildIdCollection(); var identityService = Substitute.For() @@ -76,8 +78,8 @@ public async void GivenNotFoundObservation_WhenSaveObservationAsync_ThenCreateIn var observationGroup = Substitute.For(); - var templateProcessor = Substitute.For, Model.Observation>>() - .Mock(m => m.CreateObservation(default, default).ReturnsForAnyArgs(new Model.Observation())); + var templateProcessor = Substitute.For, Observation>>() + .Mock(m => m.CreateObservation(default, default).ReturnsForAnyArgs(new Observation())); var cache = Substitute.For(); var config = Substitute.For>(); @@ -93,24 +95,53 @@ public async void GivenNotFoundObservation_WhenSaveObservationAsync_ThenCreateIn [Fact] public async void GivenFoundObservation_WhenSaveObservationAsync_ThenUpdateInvoked_Test() { - var foundObservation = new Model.Observation { Id = "1" }; - var foundBundle = new Model.Bundle + var foundObservation = new Observation { - Entry = new List + Id = "1", + Code = new CodeableConcept { Text = "Test Code" }, + Value = new SampledData { - new Model.Bundle.EntryComponent + Origin = new Model.Quantity { - Resource = foundObservation, + Value = 0m, + Unit = "count/min", }, + Period = 1000m, + Dimensions = 1, + Data = "1 E E E E E E E E E", }, + Device = new ResourceReference(@"Device/123"), + Subject = new ResourceReference(@"Patient/abc"), + Status = ObservationStatus.Final, + Identifier = new List + { + new Identifier + { + System = "Test", + Value = "id", + }, + }, + Effective = new Period(new FhirDateTime(DateTimeOffset.Now.AddHours(-1)), new FhirDateTime(DateTimeOffset.Now)), }; - var savedObservation = new Model.Observation(); + var mergedObservation = (Observation)foundObservation.DeepCopy(); + mergedObservation.Status = ObservationStatus.Amended; + ((SampledData)mergedObservation.Value).Data = "1 1 E E E E E E E E"; + + var foundBundle = new Bundle + { + Entry = new List + { + new Bundle.EntryComponent + { + Resource = foundObservation, + }, + }, + }; var fhirClient = Utilities.CreateMockFhirService(); - fhirClient.CreateResourceAsync(Arg.Any()).ReturnsForAnyArgs(Task.FromResult(new Model.Observation())); fhirClient.SearchForResourceAsync(Arg.Any(), Arg.Any()).ReturnsForAnyArgs(Task.FromResult(foundBundle)); - fhirClient.UpdateResourceAsync(Arg.Any()).ReturnsForAnyArgs(Task.FromResult(savedObservation)); + fhirClient.UpdateResourceAsync(Arg.Any()).ReturnsForAnyArgs(Task.FromResult(mergedObservation)); var ids = BuildIdCollection(); var identityService = Substitute.For() @@ -118,9 +149,9 @@ public async void GivenFoundObservation_WhenSaveObservationAsync_ThenUpdateInvok var observationGroup = Substitute.For(); - var templateProcessor = Substitute.For, Model.Observation>>() - .Mock(m => m.CreateObservation(default, default).ReturnsForAnyArgs(new Model.Observation())) - .Mock(m => m.MergeObservation(default, default, default).ReturnsForAnyArgs(new Model.Observation() { Id = "2" })); + var templateProcessor = Substitute.For, Observation>>() + .Mock(m => m.CreateObservation(default, default).ReturnsForAnyArgs(new Observation())) + .Mock(m => m.MergeObservation(default, default, default).ReturnsForAnyArgs(mergedObservation)); var cache = Substitute.For(); var config = Substitute.For>(); @@ -132,41 +163,42 @@ public async void GivenFoundObservation_WhenSaveObservationAsync_ThenUpdateInvok var result = await service.SaveObservationAsync(config, observationGroup, ids); templateProcessor.ReceivedWithAnyArgs(1).MergeObservation(default, default, default); + await fhirClient.ReceivedWithAnyArgs(1).UpdateResourceAsync(default); logger.Received(1).LogMetric(Arg.Is(x => Equals("ObservationUpdated", x.Dimensions[DimensionNames.Name])), 1); } [Fact] public async void GivenFoundObservationAndConflictOnSave_WhenSaveObservationAsync_ThenGetAndUpdateInvoked_Test() { - var foundObservation1 = new Model.Observation { Id = "1" }; - var foundBundle1 = new Model.Bundle + var foundObservation1 = new Observation { Id = "1" }; + var foundBundle1 = new Bundle { - Entry = new List + Entry = new List { - new Model.Bundle.EntryComponent + new Bundle.EntryComponent { Resource = foundObservation1, }, }, }; - var foundObservation2 = new Model.Observation { Id = "2" }; - var foundBundle2 = new Model.Bundle + var foundObservation2 = new Observation { Id = "2" }; + var foundBundle2 = new Bundle { - Entry = new List + Entry = new List { - new Model.Bundle.EntryComponent + new Bundle.EntryComponent { Resource = foundObservation2, }, }, }; - var savedObservation = new Model.Observation(); + var savedObservation = new Observation(); var fhirClient = Utilities.CreateMockFhirService(); fhirClient.SearchForResourceAsync(Arg.Any(), Arg.Any()).ReturnsForAnyArgs(Task.FromResult(foundBundle1)); - fhirClient.UpdateResourceAsync(Arg.Any()) + fhirClient.UpdateResourceAsync(Arg.Any()) .Returns( x => { throw new FhirException(new FhirResponse(new HttpResponseMessage(HttpStatusCode.Conflict), new OperationOutcome())); }, x => Task.FromResult(savedObservation)); @@ -177,8 +209,8 @@ public async void GivenFoundObservationAndConflictOnSave_WhenSaveObservationAsyn var observationGroup = Substitute.For(); - var templateProcessor = Substitute.For, Model.Observation>>() - .Mock(m => m.CreateObservation(default, default).ReturnsForAnyArgs(new Model.Observation())) + var templateProcessor = Substitute.For, Observation>>() + .Mock(m => m.CreateObservation(default, default).ReturnsForAnyArgs(new Observation())) .Mock(m => m.MergeObservation(default, default, default).ReturnsForAnyArgs(foundObservation2)); var cache = Substitute.For(); @@ -203,12 +235,12 @@ public void GivenValidTemplate_WhenGenerateObservation_ExpectedReferencesSet_Tes ids[ResourceType.Encounter] = "encounterId"; var identityService = Substitute.For(); - var templateProcessor = Substitute.For, Model.Observation>>() - .Mock(m => m.CreateObservation(default, default).ReturnsForAnyArgs(new Model.Observation())); + var templateProcessor = Substitute.For, Observation>>() + .Mock(m => m.CreateObservation(default, default).ReturnsForAnyArgs(new Observation())); var cache = Substitute.For(); var observationGroup = Substitute.For(); - var identifer = new Model.Identifier(); + var identifer = new Identifier(); var config = Substitute.For>(); var logger = Substitute.For(); @@ -233,21 +265,21 @@ public void GivenValidTemplate_WhenGenerateObservation_ExpectedReferencesSet_Tes [Fact] public async Task GivenCachedObservationDeleted_WhenGenerateObservation_ThenCacheIsUpdatedAndCreateInvoked_Test() { - var savedObservation = new Model.Observation { Id = "1" }; + var savedObservation = new Observation { Id = "1" }; // Mock the cached Observation var cache = Substitute.For(); - cache.TryGetValue(Arg.Any(), out Model.Observation observation) + cache.TryGetValue(Arg.Any(), out Observation observation) .Returns(x => { - x[1] = new Model.Observation(); + x[1] = new Observation(); return true; }); var fhirClient = Utilities.CreateMockFhirService(); - fhirClient.UpdateResourceAsync(Arg.Any()).ThrowsForAnyArgs(new FhirException(new FhirResponse(new HttpResponseMessage(HttpStatusCode.Conflict), new OperationOutcome()))); - fhirClient.SearchForResourceAsync(Arg.Any(), Arg.Any()).ReturnsForAnyArgs(Task.FromResult(new Model.Bundle())); - fhirClient.CreateResourceAsync(Arg.Any()).ReturnsForAnyArgs(Task.FromResult(savedObservation)); + fhirClient.UpdateResourceAsync(Arg.Any()).ThrowsForAnyArgs(new FhirException(new FhirResponse(new HttpResponseMessage(HttpStatusCode.Conflict), new OperationOutcome()))); + fhirClient.SearchForResourceAsync(Arg.Any(), Arg.Any()).ReturnsForAnyArgs(Task.FromResult(new Bundle())); + fhirClient.CreateResourceAsync(Arg.Any()).ReturnsForAnyArgs(Task.FromResult(savedObservation)); var ids = BuildIdCollection(); var identityService = Substitute.For() @@ -255,9 +287,9 @@ public async Task GivenCachedObservationDeleted_WhenGenerateObservation_ThenCach var observationGroup = Substitute.For(); - var templateProcessor = Substitute.For, Model.Observation>>() - .Mock(m => m.CreateObservation(default, default).ReturnsForAnyArgs(new Model.Observation())) - .Mock(m => m.MergeObservation(default, default, default).ReturnsForAnyArgs(new Model.Observation { Id = "2" })); + var templateProcessor = Substitute.For, Observation>>() + .Mock(m => m.CreateObservation(default, default).ReturnsForAnyArgs(new Observation())) + .Mock(m => m.MergeObservation(default, default, default).ReturnsForAnyArgs(new Observation { Id = "2" })); var config = Substitute.For>(); @@ -278,11 +310,11 @@ public async Task GivenCachedObservationDeleted_WhenGenerateObservation_ThenCach [Fact] public async Task GivenCachedObservationUnchanged_WhenGenerateObservation_ThenCacheNoOperation_Test() { - var cachedObservation = new Model.Observation { Id = "1" }; + var cachedObservation = new Observation { Id = "1" }; // Mock the cached Observation var cache = Substitute.For(); - cache.TryGetValue(Arg.Any(), out Model.Observation observation) + cache.TryGetValue(Arg.Any(), out Observation observation) .Returns(x => { x[1] = cachedObservation; @@ -297,8 +329,8 @@ public async Task GivenCachedObservationUnchanged_WhenGenerateObservation_ThenCa var observationGroup = Substitute.For(); - var templateProcessor = Substitute.For, Model.Observation>>() - .Mock(m => m.MergeObservation(default, default, default).ReturnsForAnyArgs(cachedObservation)); + var templateProcessor = Substitute.For, Observation>>() + .Mock(m => m.MergeObservation(default, default, default).ReturnsForAnyArgs(cachedObservation.FullCopy())); var config = Substitute.For>(); @@ -318,23 +350,23 @@ public async Task GivenCachedObservationUnchanged_WhenGenerateObservation_ThenCa [Fact] public async void GivenFoundObservationUnchanged_WhenSaveObservationAsync_ThenUpdateInvoked_Test() { - var foundObservation = new Model.Observation { Id = "1" }; - var foundBundle = new Model.Bundle + var foundObservation = new Observation { Id = "1" }; + var foundBundle = new Bundle { - Entry = new List + Entry = new List { - new Model.Bundle.EntryComponent + new Bundle.EntryComponent { Resource = foundObservation, }, }, }; - var savedObservation = new Model.Observation(); + var savedObservation = new Observation(); var fhirClient = Utilities.CreateMockFhirService(); fhirClient.SearchForResourceAsync(Arg.Any(), Arg.Any()).ReturnsForAnyArgs(Task.FromResult(foundBundle)); - fhirClient.UpdateResourceAsync(Arg.Any()).ReturnsForAnyArgs(Task.FromResult(savedObservation)); + fhirClient.UpdateResourceAsync(Arg.Any()).ReturnsForAnyArgs(Task.FromResult(savedObservation)); var ids = BuildIdCollection(); var identityService = Substitute.For() @@ -342,8 +374,8 @@ public async void GivenFoundObservationUnchanged_WhenSaveObservationAsync_ThenUp var observationGroup = Substitute.For(); - var templateProcessor = Substitute.For, Model.Observation>>() - .Mock(m => m.MergeObservation(default, default, default).ReturnsForAnyArgs(foundObservation)); + var templateProcessor = Substitute.For, Observation>>() + .Mock(m => m.MergeObservation(default, default, default).ReturnsForAnyArgs(foundObservation.FullCopy())); var cache = Substitute.For(); var config = Substitute.For>(); @@ -358,7 +390,7 @@ public async void GivenFoundObservationUnchanged_WhenSaveObservationAsync_ThenUp logger.Received(1).LogMetric(Arg.Is(x => Equals("ObservationNoOperation", x.Dimensions[DimensionNames.Name])), 1); } - private static IDictionary BuildIdCollection() + private static IDictionary BuildIdCollection() { var lookup = IdentityLookupFactory.Instance.Create(); lookup[ResourceType.Device] = "deviceId"; @@ -366,7 +398,7 @@ public async void GivenFoundObservationUnchanged_WhenSaveObservationAsync_ThenUp return lookup; } - private static Model.Observation ThrowConflictException() + private static Observation ThrowConflictException() { throw new FhirException(new FhirResponse(new HttpResponseMessage(HttpStatusCode.Conflict), new OperationOutcome())); } diff --git a/test/Microsoft.Health.Fhir.R4.Ingest.UnitTests/Template/CodeValueFhirTemplateProcessorTests.cs b/test/Microsoft.Health.Fhir.R4.Ingest.UnitTests/Template/CodeValueFhirTemplateProcessorTests.cs index 1ceacd11..6a40dda6 100644 --- a/test/Microsoft.Health.Fhir.R4.Ingest.UnitTests/Template/CodeValueFhirTemplateProcessorTests.cs +++ b/test/Microsoft.Health.Fhir.R4.Ingest.UnitTests/Template/CodeValueFhirTemplateProcessorTests.cs @@ -290,7 +290,7 @@ public void GivenEmptyTemplate_WhenMergObservation_ThenObservationReturned_Test( var newObservation = processor.MergeObservation(template, observationGroup, oldObservation); - Assert.Equal(ObservationStatus.Amended, newObservation.Status); + Assert.Equal(oldObservation.Status, newObservation.Status); valueProcessor.DidNotReceiveWithAnyArgs().MergeValue(default, default, default); } @@ -336,7 +336,7 @@ public void GivenTemplateWithValue_WhenMergObservation_ThenObservationReturned_T var newObservation = processor.MergeObservation(template, observationGroup, oldObservation); Assert.Equal(dataType, newObservation.Value); - Assert.Equal(ObservationStatus.Amended, newObservation.Status); + Assert.Equal(oldObservation.Status, newObservation.Status); valueProcessor.Received(1) .MergeValue( valueType, @@ -346,7 +346,7 @@ public void GivenTemplateWithValue_WhenMergObservation_ThenObservationReturned_T && v.ObservationPeriod.start == boundary.start && v.ObservationPeriod.end == boundary.end && v.Data.First().Item2 == "v1"), - oldValue); + Arg.Is(v => v.IsExactly(oldValue))); } [Fact] @@ -460,7 +460,7 @@ public void GivenTemplateWithComponent_WhenMergObservation_ThenObservationReturn Assert.Equal(dataType, c.Value); }); - Assert.Equal(ObservationStatus.Amended, newObservation.Status); + Assert.Equal(oldObservation.Status, newObservation.Status); valueProcessor.Received(1) .MergeValue( valueType1, @@ -470,7 +470,7 @@ public void GivenTemplateWithComponent_WhenMergObservation_ThenObservationReturn && v.ObservationPeriod.start == boundary.start && v.ObservationPeriod.end == boundary.end && v.Data.First().Item2 == "v2"), - oldValue); + Arg.Is(v => v.IsExactly(oldValue))); valueProcessor.Received(1) .CreateValue( @@ -558,7 +558,7 @@ public void GivenTemplateWithComponentAndObservationWithOutComponent_WhenMergObs Assert.Equal(dataType, c.Value); }); - Assert.Equal(ObservationStatus.Amended, newObservation.Status); + Assert.Equal(oldObservation.Status, newObservation.Status); valueProcessor.Received(1) .CreateValue( @@ -820,7 +820,7 @@ public void GivenExistingObservation_WhenMergeObservationWithDataOutsideEffectiv Assert.Equal(boundary.start, start); Assert.Equal(boundary.end, end); - Assert.Equal(ObservationStatus.Amended, newObservation.Status); + Assert.Equal(oldObservation.Status, newObservation.Status); valueProcessor.Received(1) .MergeValue( valueType, @@ -830,7 +830,7 @@ public void GivenExistingObservation_WhenMergeObservationWithDataOutsideEffectiv && v.ObservationPeriod.start == observationStart && v.ObservationPeriod.end == observationEnd && v.Data.First().Item2 == "v1"), - oldValue); + Arg.Is(v => v.IsExactly(oldValue))); } [Fact] @@ -875,7 +875,7 @@ public void GivenExistingObservation_WhenMergeObservationWithDataInsideEffective Assert.Equal(observationStart, start); Assert.Equal(observationEnd, end); - Assert.Equal(ObservationStatus.Amended, newObservation.Status); + Assert.Equal(oldObservation.Status, newObservation.Status); valueProcessor.Received(1) .MergeValue( valueType, @@ -885,7 +885,7 @@ public void GivenExistingObservation_WhenMergeObservationWithDataInsideEffective && v.ObservationPeriod.start == observationStart && v.ObservationPeriod.end == observationEnd && v.Data.First().Item2 == "v1"), - oldValue); + Arg.Is(v => v.IsExactly(oldValue))); } } } From 0eb5879271073f7d5a80548373b9cf7a6dc2e73f Mon Sep 17 00:00:00 2001 From: Dustin Burson Date: Fri, 15 Apr 2022 12:50:09 -0700 Subject: [PATCH 2/3] Prior update that changed the FHIR Client lost the isVersionAware parameter. Update the FHIR Service UpdateResourceAsync to automatically set the ifMatchVersion if one isn't already specified and the resource being updated has a version set (i.e. not new). --- .../ResourceExtensions.cs | 10 +++++----- .../Service/FhirService.cs | 6 ++++++ 2 files changed, 11 insertions(+), 5 deletions(-) diff --git a/src/lib/Microsoft.Health.Extensions.Fhir.R4/ResourceExtensions.cs b/src/lib/Microsoft.Health.Extensions.Fhir.R4/ResourceExtensions.cs index 118f1830..a09145b3 100644 --- a/src/lib/Microsoft.Health.Extensions.Fhir.R4/ResourceExtensions.cs +++ b/src/lib/Microsoft.Health.Extensions.Fhir.R4/ResourceExtensions.cs @@ -1,8 +1,8 @@ -// ------------------------------------------------------------------------------------------------- -// Copyright (c) Microsoft Corporation. All rights reserved. -// Licensed under the MIT License (MIT). See LICENSE in the repo root for license information. -// ------------------------------------------------------------------------------------------------- - +// ------------------------------------------------------------------------------------------------- +// Copyright (c) Microsoft Corporation. All rights reserved. +// Licensed under the MIT License (MIT). See LICENSE in the repo root for license information. +// ------------------------------------------------------------------------------------------------- + using EnsureThat; using Hl7.Fhir.Model; diff --git a/src/lib/Microsoft.Health.Extensions.Fhir.R4/Service/FhirService.cs b/src/lib/Microsoft.Health.Extensions.Fhir.R4/Service/FhirService.cs index 41d9e110..31a6159a 100644 --- a/src/lib/Microsoft.Health.Extensions.Fhir.R4/Service/FhirService.cs +++ b/src/lib/Microsoft.Health.Extensions.Fhir.R4/Service/FhirService.cs @@ -76,6 +76,12 @@ public async Task UpdateResourceAsync( { EnsureArg.IsNotNull(resource, nameof(resource)); + if (string.IsNullOrWhiteSpace(ifMatchVersion) && resource.HasVersionId) + { + // Underlying FhirClient already adds the W/"" formating and inserts content of the ifMatchVersion + ifMatchVersion = resource.VersionId.ToString(); + } + return await _fhirClient.UpdateAsync(resource, ifMatchVersion, provenanceHeader, cancellationToken).ConfigureAwait(false); } From ea206f52874e41622563d4cdc5800b00f133421d Mon Sep 17 00:00:00 2001 From: Dustin Burson Date: Fri, 15 Apr 2022 12:56:54 -0700 Subject: [PATCH 3/3] Fix comment typo --- .../Service/R4FhirImportService.cs | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) 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 ddac7041..07396721 100644 --- a/src/lib/Microsoft.Health.Fhir.R4.Ingest/Service/R4FhirImportService.cs +++ b/src/lib/Microsoft.Health.Fhir.R4.Ingest/Service/R4FhirImportService.cs @@ -113,7 +113,7 @@ public virtual async Task SaveObservationAsync(ILookupTemplate