Fixing resource create after hard delete with CC enabled (#3568)

* Added test

* small db hints

* Converting invisible resources to deleted

* missing sp

* fix comments

* Removed wrong check

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

@@ -75,5 +75,6 @@ public enum SchemaVersion
         V63 = 63,
         V64 = 64,
         V65 = 65,
+        V66 = 66,
     }
 }

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

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

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

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

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

@@ -15,13 +15,13 @@ BEGIN TRY
         ,A.ResourceSurrogateId
         -- set version to 0 if there is no gap available. It would indicate conflict for the caller.
         ,Version = CASE 
-                     WHEN EXISTS (SELECT * FROM dbo.Resource B WHERE B.ResourceTypeId = A.ResourceTypeId AND B.ResourceId = A.ResourceId AND B.ResourceSurrogateId = A.ResourceSurrogateId) THEN 0
+                     WHEN EXISTS (SELECT * FROM dbo.Resource B WHERE B.ResourceTypeId = A.ResourceTypeId AND B.ResourceSurrogateId = A.ResourceSurrogateId) THEN 0
                      WHEN isnull(U.Version, 1) - isnull(L.Version, 0) > 1 THEN isnull(U.Version, 1) - 1 
                      ELSE 0 
                    END
     FROM (SELECT TOP (@DummyTop) * FROM @ResourceDateKeys) A
-         OUTER APPLY (SELECT TOP 1 * FROM dbo.Resource B WHERE B.ResourceTypeId = A.ResourceTypeId AND B.ResourceId = A.ResourceId AND B.ResourceSurrogateId < A.ResourceSurrogateId ORDER BY B.ResourceSurrogateId DESC) L -- lower
-         OUTER APPLY (SELECT TOP 1 * FROM dbo.Resource B WHERE B.ResourceTypeId = A.ResourceTypeId AND B.ResourceId = A.ResourceId AND B.ResourceSurrogateId > A.ResourceSurrogateId ORDER BY B.ResourceSurrogateId) U -- upper
+         OUTER APPLY (SELECT TOP 1 * FROM dbo.Resource B WITH (INDEX = IX_Resource_ResourceTypeId_ResourceId_Version) WHERE B.ResourceTypeId = A.ResourceTypeId AND B.ResourceId = A.ResourceId AND B.ResourceSurrogateId < A.ResourceSurrogateId ORDER BY B.ResourceSurrogateId DESC) L -- lower
+         OUTER APPLY (SELECT TOP 1 * FROM dbo.Resource B WITH (INDEX = IX_Resource_ResourceTypeId_ResourceId_Version) WHERE B.ResourceTypeId = A.ResourceTypeId AND B.ResourceId = A.ResourceId AND B.ResourceSurrogateId > A.ResourceSurrogateId ORDER BY B.ResourceSurrogateId) U -- upper
     OPTION (MAXDOP 1, OPTIMIZE FOR (@DummyTop = 1))
 
   EXECUTE dbo.LogEvent @Process=@SP,@Mode=@Mode,@Status='End',@Start=@st,@Rows=@@rowcount

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

@@ -20,7 +20,7 @@ BEGIN TRY
 
   IF @IsResourceChangeCaptureEnabled = 1 AND NOT EXISTS (SELECT * FROM dbo.Parameters WHERE Id = 'InvisibleHistory.IsEnabled' AND Number = 0)
     UPDATE dbo.Resource
-      SET IsHistory = 1
+      SET IsDeleted = 1
          ,RawResource = 0xF -- invisible value
          ,SearchParamHash = NULL
          ,HistoryTransactionId = @TransactionId

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

@@ -12,7 +12,6 @@ CREATE PROCEDURE dbo.MergeResources
    ,@SingleTransaction bit = 1
    ,@Resources dbo.ResourceList READONLY
    ,@ResourceWriteClaims dbo.ResourceWriteClaimList READONLY
-   ,@CompartmentAssignments dbo.CompartmentAssignmentList READONLY -- TODO: Remove after version 57 got deployed
    ,@ReferenceSearchParams dbo.ReferenceSearchParamList READONLY
    ,@TokenSearchParams dbo.TokenSearchParamList READONLY
    ,@TokenTexts dbo.TokenTextList READONLY
@@ -67,7 +66,7 @@ BEGIN TRY
   BEGIN
     IF EXISTS (SELECT * -- This extra statement avoids putting range locks when we don't need them
                  FROM @Resources A JOIN dbo.Resource B ON B.ResourceTypeId = A.ResourceTypeId AND B.ResourceSurrogateId = A.ResourceSurrogateId
-                 WHERE B.IsHistory = 0
+                 --WHERE B.IsHistory = 0 -- With this clause wrong plans are created on empty/small database. Commented until resource separation is in place.
               )
     BEGIN
       BEGIN TRANSACTION

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

@@ -18,7 +18,7 @@ BEGIN TRY
   BEGIN
     SET @TypeId = (SELECT TOP 1 TypeId FROM @Types ORDER BY TypeId)
 
-    DELETE FROM dbo.Resource WHERE ResourceTypeId = @TypeId AND HistoryTransactionId = @TransactionId AND IsHistory = 1 AND RawResource = 0xF
+    DELETE FROM dbo.Resource WHERE ResourceTypeId = @TypeId AND HistoryTransactionId = @TransactionId AND RawResource = 0xF
     SET @AffectedRows += @@rowcount
 
     DELETE FROM @Types WHERE TypeId = @TypeId

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

@@ -157,7 +157,8 @@ internal async Task<IDictionary<DataStoreOperationIdentifier, DataStoreOperation
             }
 
             // Ignore input resource version to get latest version from the store.
-            var existingResources = (await GetAsync(resources.Select(r => r.Wrapper.ToResourceKey(true)).Distinct().ToList(), cancellationToken)).ToDictionary(r => r.ToResourceKey(true), r => r);
+            // Include invisible records (true parameter), so version is correctly determined in case only invisible is left in store.
+            var existingResources = (await GetAsync(resources.Select(r => r.Wrapper.ToResourceKey(true)).Distinct().ToList(), true, cancellationToken)).ToDictionary(r => r.ToResourceKey(true), r => r);
 
             // Assume that most likely case is that all resources should be updated.
             (var transactionId, var minSequenceId) = await StoreClient.MergeResourcesBeginTransactionAsync(resources.Count, cancellationToken);
@@ -407,7 +408,12 @@ public async Task<UpsertOutcome> UpsertAsync(ResourceWrapperOperation resource,
 
         public async Task<IReadOnlyList<ResourceWrapper>> GetAsync(IReadOnlyList<ResourceKey> keys, CancellationToken cancellationToken)
         {
-            return await _sqlStoreClient.GetAsync(keys, _model.GetResourceTypeId, _compressedRawResourceConverter.ReadCompressedRawResource, _model.GetResourceTypeName, cancellationToken);
+            return await GetAsync(keys, false, cancellationToken); // do not return invisible records in public interface
+        }
+
+        private async Task<IReadOnlyList<ResourceWrapper>> GetAsync(IReadOnlyList<ResourceKey> keys, bool includeInvisible, CancellationToken cancellationToken)
+        {
+            return await _sqlStoreClient.GetAsync(keys, _model.GetResourceTypeId, _compressedRawResourceConverter.ReadCompressedRawResource, _model.GetResourceTypeName, cancellationToken, includeInvisible);
         }
 
         public async Task<ResourceWrapper> GetAsync(ResourceKey key, CancellationToken cancellationToken)

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

@@ -30,6 +30,7 @@ internal class SqlStoreClient<T>
     {
         private readonly ISqlRetryService _sqlRetryService;
         private readonly ILogger<T> _logger;
+        private const string _invisibleResource = " ";
 
         public SqlStoreClient(ISqlRetryService sqlRetryService, ILogger<T> logger)
         {
@@ -52,12 +53,12 @@ internal async Task TryLogEvent(string process, string status, string text, Date
             await _sqlRetryService.TryLogEvent(process, status, text, startDate, cancellationToken);
         }
 
-        public async Task<IReadOnlyList<ResourceWrapper>> GetAsync(IReadOnlyList<ResourceKey> keys, Func<string, short> getResourceTypeId, Func<MemoryStream, string> decompress, Func<short, string> getResourceTypeName, CancellationToken cancellationToken)
+        public async Task<IReadOnlyList<ResourceWrapper>> GetAsync(IReadOnlyList<ResourceKey> keys, Func<string, short> getResourceTypeId, Func<MemoryStream, string> decompress, Func<short, string> getResourceTypeName, CancellationToken cancellationToken, bool includeInvisible = false)
         {
-            return await GetAsync(keys.Select(_ => new ResourceDateKey(getResourceTypeId(_.ResourceType), _.Id, 0, _.VersionId)).ToList(), decompress, getResourceTypeName, cancellationToken);
+            return await GetAsync(keys.Select(_ => new ResourceDateKey(getResourceTypeId(_.ResourceType), _.Id, 0, _.VersionId)).ToList(), decompress, getResourceTypeName, cancellationToken, includeInvisible);
         }
 
-        public async Task<IReadOnlyList<ResourceWrapper>> GetAsync(IReadOnlyList<ResourceDateKey> keys, Func<MemoryStream, string> decompress, Func<short, string> getResourceTypeName, CancellationToken cancellationToken)
+        public async Task<IReadOnlyList<ResourceWrapper>> GetAsync(IReadOnlyList<ResourceDateKey> keys, Func<MemoryStream, string> decompress, Func<short, string> getResourceTypeName, CancellationToken cancellationToken, bool includeInvisible = false)
         {
             if (keys == null || keys.Count == 0)
             {
@@ -73,8 +74,7 @@ public async Task<IReadOnlyList<ResourceWrapper>> GetAsync(IReadOnlyList<Resourc
             {
                 try
                 {
-                    //// ignore nulls returned for invisible resources
-                    return (await cmd.ExecuteReaderAsync(_sqlRetryService, (reader) => { return ReadResourceWrapper(reader, false, decompress, getResourceTypeName); }, _logger, cancellationToken)).Where(_ => _ != null).ToList();
+                    return (await cmd.ExecuteReaderAsync(_sqlRetryService, (reader) => { return ReadResourceWrapper(reader, false, decompress, getResourceTypeName); }, _logger, cancellationToken)).Where(_ => includeInvisible || _.RawResource.Data != _invisibleResource).ToList();
                 }
                 catch (Exception e)
                 {
@@ -119,8 +119,8 @@ internal async Task<IReadOnlyList<ResourceWrapper>> GetResourcesByTransactionIdA
         {
             using var cmd = new SqlCommand() { CommandText = "dbo.GetResourcesByTransactionId", CommandType = CommandType.StoredProcedure, CommandTimeout = 600 };
             cmd.Parameters.AddWithValue("@TransactionId", transactionId);
-            //// ignore nulls returned for invisible resources
-            return (await cmd.ExecuteReaderAsync(_sqlRetryService, (reader) => { return ReadResourceWrapper(reader, true, decompress, getResourceTypeName); }, _logger, cancellationToken)).Where(_ => _ != null).ToList();
+            //// ignore invisible resources
+            return (await cmd.ExecuteReaderAsync(_sqlRetryService, (reader) => { return ReadResourceWrapper(reader, true, decompress, getResourceTypeName); }, _logger, cancellationToken)).Where(_ => _.RawResource.Data != _invisibleResource).ToList();
         }
 
         private static ResourceWrapper ReadResourceWrapper(SqlDataReader reader, bool readRequestMethod, Func<MemoryStream, string> decompress, Func<short, string> getResourceTypeName)
@@ -136,13 +136,16 @@ private static ResourceWrapper ReadResourceWrapper(SqlDataReader reader, bool re
             var searchParamHash = reader.Read(VLatest.Resource.SearchParamHash, 8);
             var requestMethod = readRequestMethod ? reader.Read(VLatest.Resource.RequestMethod, 9) : null;
 
+            string rawResource;
             if (rawResourceBytes.Length == 1 && rawResourceBytes[0] == 0xF) // invisible resource
             {
-                return null;
+                rawResource = _invisibleResource;
+            }
+            else
+            {
+                using var rawResourceStream = new MemoryStream(rawResourceBytes);
+                rawResource = decompress(rawResourceStream);
             }
-
-            using var rawResourceStream = new MemoryStream(rawResourceBytes);
-            var rawResource = decompress(rawResourceStream);
 
             return new ResourceWrapper(
                 resourceId,

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

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

test/Microsoft.Health.Fhir.Shared.Tests.Integration/Features/ChangeFeed/SqlServerFhirResourceChangeCaptureEnabledTests.cs

@@ -193,6 +193,45 @@ public async Task GivenChangeCaptureEnabledAndNoVersionPolicy_AfterHardDeleting_
             DisableInvisibleHistory();
         }
 
+        [Fact]
+        public async Task GivenChangeCaptureEnabledAndNoVersionPolicy_AfterHardDeleting_CanRecreateResource()
+        {
+            EnableDatabaseLogging();
+            EnableInvisibleHistory();
+
+            var create = await _fixture.Mediator.CreateResourceAsync(Samples.GetDefaultOrganization());
+            Assert.Equal("1", create.VersionId);
+            var id = create.Id;
+
+            var store = (SqlServerFhirDataStore)_fixture.DataStore;
+            await store.HardDeleteAsync(new ResourceKey("Organization", id), false, CancellationToken.None);
+
+            var reCreate = await _fixture.Mediator.UpsertResourceAsync(Samples.GetDefaultOrganization().UpdateId(id));
+            Assert.Equal(id, reCreate.RawResourceElement.Id);
+            Assert.Equal("2", reCreate.RawResourceElement.VersionId);
+
+            DisableInvisibleHistory();
+        }
+
+        [Fact]
+        public async Task GivenChangeCaptureEnabledAndNoVersionPolicy_AfterSoftDeleting_CanRecreateResource()
+        {
+            EnableDatabaseLogging();
+            EnableInvisibleHistory();
+
+            var create = await _fixture.Mediator.CreateResourceAsync(Samples.GetDefaultOrganization());
+            Assert.Equal("1", create.VersionId);
+            var id = create.Id;
+
+            await _fixture.Mediator.DeleteResourceAsync(new ResourceKey("Organization", id), DeleteOperation.SoftDelete);
+
+            var reCreate = await _fixture.Mediator.UpsertResourceAsync(Samples.GetDefaultOrganization().UpdateId(id));
+            Assert.Equal(id, reCreate.RawResourceElement.Id);
+            Assert.Equal("3", reCreate.RawResourceElement.VersionId);
+
+            DisableInvisibleHistory();
+        }
+
         [Fact]
         public async Task GivenChangeCaptureEnabledAndNoVersionPolicy_AfterUpdating_HistoryIsNotReturnedAndChangesAreReturned()
         {