Fix a bug where data files are deleted if GetAsync fails when updating (#787)

Author: pmachapman

src/Serval/src/Serval.DataFiles/Services/DataFileService.cs

@@ -40,7 +40,7 @@ public async Task CreateAsync(DataFile dataFile, Stream stream, CancellationToke
         string path = GetDataFilePath(filename);
         try
         {
-            using Stream fileStream = _fileSystem.OpenWrite(path);
+            await using Stream fileStream = _fileSystem.OpenWrite(path);
             await stream.CopyToAsync(fileStream, cancellationToken);
             await Entities.InsertAsync(dataFile with { Filename = filename }, cancellationToken);
         }
@@ -67,10 +67,10 @@ public async Task<DataFile> UpdateAsync(string id, Stream stream, CancellationTo
         bool deleteFile = false;
         try
         {
-            using (Stream fileStream = _fileSystem.OpenWrite(path))
+            await using (Stream fileStream = _fileSystem.OpenWrite(path))
                 await stream.CopyToAsync(fileStream, cancellationToken);
             await _dataAccessContext.WithTransactionAsync(
-                async (ct) =>
+                async ct =>
                 {
                     DataFile? originalDataFile = await Entities.UpdateAsync(
                         id,
@@ -79,21 +79,16 @@ await _dataAccessContext.WithTransactionAsync(
                         cancellationToken: ct
                     );
                     if (originalDataFile is null)
-                    {
                         throw new EntityNotFoundException($"Could not find the DataFile '{id}'.");
-                    }
-                    else
-                    {
-                        await _deletedFiles.InsertAsync(
-                            new DeletedFile { Filename = originalDataFile.Filename, DeletedAt = DateTime.UtcNow },
-                            cancellationToken: ct
-                        );
-                    }
+
+                    await _deletedFiles.InsertAsync(
+                        new DeletedFile { Filename = originalDataFile.Filename, DeletedAt = DateTime.UtcNow },
+                        cancellationToken: ct
+                    );
                     await _mediator.Publish(new DataFileUpdated { DataFileId = id, Filename = filename }, ct);
                 },
                 cancellationToken: cancellationToken
             );
-            return await GetAsync(id, cancellationToken);
         }
         catch
         {
@@ -105,12 +100,13 @@ await _deletedFiles.InsertAsync(
             if (deleteFile)
                 _fileSystem.DeleteFile(path);
         }
+
+        return await GetAsync(id, cancellationToken);
     }
 
-    public override async Task DeleteAsync(string id, CancellationToken cancellationToken = default)
-    {
+    public override async Task DeleteAsync(string id, CancellationToken cancellationToken = default) =>
         await _dataAccessContext.WithTransactionAsync(
-            async (ct) =>
+            async ct =>
             {
                 DataFile? dataFile = await Entities.DeleteAsync(id, ct);
                 if (dataFile is null)
@@ -124,10 +120,6 @@ await _deletedFiles.InsertAsync(
             },
             cancellationToken: cancellationToken
         );
-    }
 
-    private string GetDataFilePath(string filename)
-    {
-        return Path.Combine(_options.CurrentValue.FilesDirectory, filename);
-    }
+    private string GetDataFilePath(string filename) => Path.Combine(_options.CurrentValue.FilesDirectory, filename);
 }

src/Serval/test/Serval.DataFiles.Tests/Services/DataFileServiceTests.cs

@@ -82,6 +82,39 @@ public async Task UpdateAsync_Exists()
         Assert.That(deletedFile.Filename, Is.EqualTo("file1.txt"));
     }
 
+    [Test]
+    public void UpdateAsync_GetAsyncFails()
+    {
+        var env = new TestEnvironment();
+
+        // We will use the mediator to cancel the token, which will cause GetAsync() to fail
+        // What we are testing for is GetAsync() failing due to network or other connectivity issues, token cancellation being one source
+        var cts = new CancellationTokenSource();
+        env.Mediator.When(x => x.Publish(Arg.Any<DataFileUpdated>(), Arg.Any<CancellationToken>()))
+            .Do(_ => cts.Cancel());
+
+        // Set up a valid existing file
+        env.DataFiles.Add(DefaultDataFile with { });
+        using var fileStream = new MemoryStream();
+        env.FileSystem.OpenWrite(Arg.Any<string>()).Returns(fileStream);
+        string content = "This is a file.";
+        using (var stream = new MemoryStream(Encoding.UTF8.GetBytes(content)))
+        {
+            Assert.ThrowsAsync<OperationCanceledException>(
+                () => env.Service.UpdateAsync(DataFileId, stream, cts.Token)
+            );
+        }
+
+        // Verify the file was updated
+        DataFile dataFile = env.DataFiles.Get(DataFileId);
+        Assert.That(dataFile.Revision, Is.EqualTo(2));
+        Assert.That(Encoding.UTF8.GetString(fileStream.ToArray()), Is.EqualTo(content));
+        DeletedFile deletedFile = env.DeletedFiles.Entities.Single();
+        Assert.That(deletedFile.Filename, Is.EqualTo("file1.txt"));
+
+        env.FileSystem.DidNotReceive().DeleteFile(Arg.Any<string>());
+    }
+
     [Test]
     public void UpdateAsync_DoesNotExist()
     {