Fixing incorrect versioning on close records (#4786)

* Fixing incorrect versioning on close duplicates

* 85.diff

* Comment

* Changing 1 minute in the future to 8 seconds

Author: SergeyGaluzo

src/Microsoft.Health.Fhir.Shared.Core/Features/Operations/Import/ImportResourceParser.cs

@@ -42,6 +42,10 @@ public ImportResource Parse(long index, long offset, int length, string rawResou
             var lastUpdatedIsNull = importMode == ImportMode.InitialLoad || resource.Meta.LastUpdated == null;
             var lastUpdated = lastUpdatedIsNull ? Clock.UtcNow : resource.Meta.LastUpdated.Value;
             resource.Meta.LastUpdated = new DateTimeOffset(lastUpdated.DateTime.TruncateToMillisecond(), lastUpdated.Offset);
+            if (!lastUpdatedIsNull && resource.Meta.LastUpdated.Value > Clock.UtcNow.AddSeconds(10)) // 5 sec is the max for the computers in the domain
+            {
+                throw new NotSupportedException("LastUpdated in the resource cannot be in the future.");
+            }
 
             var keepVersion = true;
             if (lastUpdatedIsNull || string.IsNullOrEmpty(resource.Meta.VersionId) || !int.TryParse(resource.Meta.VersionId, out var _))

src/Microsoft.Health.Fhir.SqlServer/Features/Schema/Migrations/85.diff.sql

@@ -94,5 +94,6 @@ public enum SchemaVersion
         V82 = 82,
         V83 = 83,
         V84 = 84,
+        V85 = 85,
     }
 }

src/Microsoft.Health.Fhir.SqlServer/Features/Schema/Migrations/85.sql

@@ -8,7 +8,7 @@ namespace Microsoft.Health.Fhir.SqlServer.Features.Schema
     public static class SchemaVersionConstants
     {
         public const int Min = (int)SchemaVersion.V83;
-        public const int Max = (int)SchemaVersion.V84;
+        public const int Max = (int)SchemaVersion.V85;
         public const int MinForUpgrade = (int)SchemaVersion.V83; // this is used for upgrade tests only
         public const int SearchParameterStatusSchemaVersion = (int)SchemaVersion.V6;
         public const int SupportForReferencesWithMissingTypeVersion = (int)SchemaVersion.V7;

src/Microsoft.Health.Fhir.SqlServer/Features/Schema/SchemaVersion.cs

@@ -19,6 +19,6 @@ Go
 
 INSERT INTO dbo.SchemaVersion
 VALUES
-    (84, 'started')
+    (85, 'started')
 
 Go

src/Microsoft.Health.Fhir.SqlServer/Features/Schema/SchemaVersionConstants.cs

@@ -101,7 +101,8 @@ BEGIN TRY
                ON B.ResourceTypeId = A.ResourceTypeId AND B.ResourceId = A.ResourceId AND B.IsHistory = 0
         OPTION (MAXDOP 1, OPTIMIZE FOR (@DummyTop = 1))
 
-    IF @RaiseExceptionOnConflict = 1 AND EXISTS (SELECT * FROM @ResourceInfos WHERE PreviousVersion IS NOT NULL AND Version <= PreviousVersion)
+    -- Consider surrogate id out of allignment as a conflict
+    IF @RaiseExceptionOnConflict = 1 AND EXISTS (SELECT * FROM @ResourceInfos WHERE (PreviousVersion IS NOT NULL AND Version <= PreviousVersion) OR (PreviousSurrogateId IS NOT NULL AND SurrogateId <= PreviousSurrogateId))
       THROW 50409, 'Resource has been recently updated or added, please compare the resource content in code for any duplicate updates', 1
 
     INSERT INTO @PreviousSurrogateIds

src/Microsoft.Health.Fhir.SqlServer/Features/Schema/Sql/Scripts/TransactionCheckWithInitialiScript.sql

@@ -627,7 +627,8 @@ async Task MergeUnversioned(List<ImportResource> inputs, bool keepLastUpdated, b
                     var inputsNoVersionForCheck = new List<ImportResource>();
                     foreach (var input in inputs)
                     {
-                        if (currentInDb.TryGetValue(input.ResourceWrapper.ToResourceKey(true), out var current) && input.ResourceWrapper.LastModified < current.LastModified)
+                        // Include inputs only with explicit lastUpdated (input.KeepLastUpdated = true)
+                        if (currentInDb.TryGetValue(input.ResourceWrapper.ToResourceKey(true), out var current) && input.KeepLastUpdated && input.ResourceWrapper.LastModified < current.LastModified)
                         {
                             inputsNoVersionForCheck.Add(input);
                         }

src/Microsoft.Health.Fhir.SqlServer/Features/Schema/Sql/Sprocs/MergeResources.sql

@@ -1,7 +1,7 @@
 <Project Sdk="Microsoft.NET.Sdk">
   <!-- Properties used by sql task to generate full script -->
   <PropertyGroup>
-    <LatestSchemaVersion>84</LatestSchemaVersion>
+    <LatestSchemaVersion>85</LatestSchemaVersion>
     <GeneratedFullScriptPath>Features\Schema\Migrations\$(LatestSchemaVersion).sql</GeneratedFullScriptPath>
   </PropertyGroup>
 

src/Microsoft.Health.Fhir.SqlServer/Features/Storage/SqlServerFhirDataStore.cs

@@ -80,7 +80,7 @@ private async Task<List<Resource>> SavePrerequisiteResourcesViaHttp(List<Resourc
                 },
                 new()
                 {
-                    CreateTestPatient(id: sharedId, lastUpdated: DateTimeOffset.UtcNow.AddMinutes(1)),
+                    CreateTestPatient(id: sharedId, lastUpdated: DateTimeOffset.UtcNow.AddSeconds(8)),
                 });
         }
 
@@ -94,7 +94,7 @@ private async Task<List<Resource>> SavePrerequisiteResourcesViaHttp(List<Resourc
                 },
                 new()
                 {
-                    CreateTestPatient(id: sharedId, lastUpdated: DateTimeOffset.UtcNow.AddMinutes(1), versionId: "1"),
+                    CreateTestPatient(id: sharedId, lastUpdated: DateTimeOffset.UtcNow.AddSeconds(8), versionId: "1"),
                 });
         }
 
@@ -106,7 +106,7 @@ private async Task<List<Resource>> SavePrerequisiteResourcesViaHttp(List<Resourc
                 new()
                 {
                     CreateTestPatient(id: sharedId, lastUpdated: DateTimeOffset.UtcNow, versionId: "1"),
-                    CreateTestPatient(id: sharedId, lastUpdated: DateTimeOffset.UtcNow.AddMinutes(1), versionId: "1"),
+                    CreateTestPatient(id: sharedId, lastUpdated: DateTimeOffset.UtcNow.AddSeconds(8), versionId: "1"),
                 });
         }
 

src/Microsoft.Health.Fhir.SqlServer/Microsoft.Health.Fhir.SqlServer.csproj

@@ -419,20 +419,33 @@ public async Task GivenIncrementalLoad_MultipleInputsWithImplicitVersionsExplici
             Assert.Equal("1", history.Resource.Entry[0].Resource.VersionId);
         }
 
+        [Fact]
+        public async Task GivenIncrementalLoad_LastUpdatedOnResourceCannotBeInTheFuture()
+        {
+            var id = Guid.NewGuid().ToString("N");
+            var ndJson = CreateTestPatient(id, DateTimeOffset.UtcNow.AddSeconds(20)); // set value higher than 10 seconds tolerance
+            var location = (await ImportTestHelper.UploadFileAsync(ndJson, _fixture.StorageAccount)).location;
+            var request = CreateImportRequest(location, ImportMode.IncrementalLoad, false, true);
+            var result = await ImportCheckAsync(request, null, 1);
+            var errorLocation = result.Error.ToArray()[0].Url;
+            var errorContent = await ImportTestHelper.DownloadFileAsync(errorLocation, _fixture.StorageAccount);
+            Assert.Contains("LastUpdated in the resource cannot be in the future.", errorContent);
+        }
+
         [Fact]
         public async Task GivenIncrementalLoad_MultipleInputsWithImplicitVersionsExplicitLastUpdatedAfterImplicit()
         {
             var id = Guid.NewGuid().ToString("N");
             var ndJson1 = CreateTestPatient(id);
-            var ndJson2 = CreateTestPatient(id, DateTimeOffset.UtcNow.AddDays(1));
+            var ndJson2 = CreateTestPatient(id, DateTimeOffset.UtcNow.AddSeconds(8), birhDate: "1990"); // last updated within 10 seconds tolerance, different content
             var location = (await ImportTestHelper.UploadFileAsync(ndJson1 + ndJson2, _fixture.StorageAccount)).location;
             var request = CreateImportRequest(location, ImportMode.IncrementalLoad, false, true);
             await ImportCheckAsync(request, null, 0);
-
             var history = await _client.SearchAsync($"Patient/{id}/_history");
             Assert.Equal(2, history.Resource.Entry.Count);
-            Assert.Equal("1", history.Resource.Entry[0].Resource.VersionId);
-            Assert.Equal("-1", history.Resource.Entry[1].Resource.VersionId);
+            //// same order
+            Assert.True(int.Parse(history.Resource.Entry[0].Resource.VersionId) > int.Parse(history.Resource.Entry[1].Resource.VersionId));
+            Assert.True(history.Resource.Entry[0].Resource.Meta.LastUpdated > history.Resource.Entry[1].Resource.Meta.LastUpdated);
         }
 
         [Fact]
@@ -1725,7 +1738,7 @@ public async Task GivenImportRequestWithMultipleSameFile_ThenBadRequestShouldBeR
             Assert.Equal(HttpStatusCode.BadRequest, fhirException.StatusCode);
         }
 
-        private async Task<Uri> ImportCheckAsync(ImportRequest request, TestFhirClient client = null, int? errorCount = null)
+        private async Task<ImportJobResult> ImportCheckAsync(ImportRequest request, TestFhirClient client = null, int? errorCount = null)
         {
             client = client ?? _client;
             Uri checkLocation = await ImportTestHelper.CreateImportTaskAsync(client, request);
@@ -1746,7 +1759,7 @@ private async Task<Uri> ImportCheckAsync(ImportRequest request, TestFhirClient c
 
             Assert.NotEmpty(result.Request);
 
-            return checkLocation;
+            return result;
         }
 
         private async Task<HttpResponseMessage> ImportWaitAsync(Uri checkLocation, bool checkSuccessStatus = true)