Address issue #184, observations not being updated when changed (#185)

* * 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.

* 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).

* Fix comment typo

src/lib/Microsoft.Health.Extensions.Fhir.R4/ServiceCollectionExtensions.cs → src/lib/Microsoft.Health.Extensions.Fhir.R4/FhirClientExtensions.cs

@@ -15,45 +15,56 @@
 
 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));
 
             var url = new Uri(configuration.GetValue<string>("FhirService:Url"));
             bool useManagedIdentity = configuration.GetValue<bool>("FhirClient:UseManagedIdentity");
 
-            serviceCollection.AddSingleton(typeof(ITelemetryLogger), typeof(IomtTelemetryLogger));
-            var serviceProvider = serviceCollection.BuildServiceProvider();
-            var logger = serviceProvider.GetRequiredService<ITelemetryLogger>();
-
-            serviceCollection.AddHttpClient<IFhirClient, FhirClient>(client =>
+            serviceCollection.AddHttpClient<IFhirClient, FhirClient>((client, sp) =>
             {
                 client.BaseAddress = url;
                 client.Timeout = TimeSpan.FromSeconds(60);
 
+                var logger = sp.GetRequiredService<ITelemetryLogger>();
+
                 // 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<ManagedIdentityAuthService>();
+            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<OAuthConfidentialClientAuthService>();
+            if (useManagedIdentity)
+            {
+                services.TryAddSingleton(new ManagedIdentityAuthService());
+                httpClientBuilder.AddHttpMessageHandler(sp =>
+                    new BearerTokenAuthorizationMessageHandler(uri, sp.GetRequiredService<ManagedIdentityAuthService>(), sp.GetRequiredService<ITelemetryLogger>()));
+            }
+            else
+            {
+                services.TryAddSingleton(new OAuthConfidentialClientAuthService());
+                httpClientBuilder.AddHttpMessageHandler(sp =>
+                    new BearerTokenAuthorizationMessageHandler(uri, sp.GetRequiredService<OAuthConfidentialClientAuthService>(), sp.GetRequiredService<ITelemetryLogger>()));
+            }
         }
     }
 }

src/lib/Microsoft.Health.Extensions.Fhir.R4/FhirClientValidator.cs → 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<bool> ValidateFhirClientAsync(
             this IFhirClient client,

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
+    {
+        /// <summary>
+        /// Compares two observations to see if they are different.  If they are different the <paramref name="updatedObservation"/> status is changed to Amended.
+        /// </summary>
+        /// <param name="originalObservation">The original unmodified observation.</param>
+        /// <param name="updatedObservation">The potentially modified observation.</param>
+        /// <returns>Returns true if the <paramref name="updatedObservation"/> is different than the <paramref name="originalObservation"/>.  Otherwise false is returned.</returns>
+        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;
+        }
+    }
+}

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
+    {
+        /// <summary>
+        /// Performs full deep copy of the resource.
+        /// </summary>
+        /// <typeparam name="TResource">Type of resource to return.</typeparam>
+        /// <param name="resource">Resource to copy.</param>
+        /// <returns>New resource object with the contents of the original.</returns>
+        public static TResource FullCopy<TResource>(this TResource resource)
+            where TResource : class, IDeepCopyable
+        {
+            EnsureArg.IsNotNull(resource, nameof(resource));
+
+            return resource.DeepCopy() as TResource;
+        }
+    }
+}

src/lib/Microsoft.Health.Extensions.Fhir.R4/Service/FhirService.cs

@@ -76,6 +76,12 @@ public async Task<T> UpdateResourceAsync<T>(
         {
             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);
         }
 

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<Observation.ComponentComponent>(template.Components.Count);
+                    mergedObservation.Component = new List<Observation.ComponentComponent>(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<IObservationGroup> CreateObservationGroupsImpl(CodeValueFhirTemplate template, IMeasurementGroup measurementGroup)

src/lib/Microsoft.Health.Fhir.R4.Ingest/Service/R4FhirImportService.cs

@@ -113,8 +113,8 @@ public virtual async Task<string> SaveObservationAsync(ILookupTemplate<IFhirTemp
                     // Merge the new data with the existing Observation.
                     var mergedObservation = MergeObservation(config, existingObservation, observationGroup);
 
-                    // Check to see if there are any changes after merging.
-                    if (mergedObservation.IsExactly(existingObservation))
+                    // Check to see if there are any changes after merging and update the Status to amended if changed.
+                    if (!existingObservation.AmendIfChanged(mergedObservation))
                     {
                         // There are no changes to the Observation - Do not update.
                         return (existingObservation, ResourceOperation.NoOperation);