From 3f3ff10e2d33780ec636b287e339ea970c40e5b5 Mon Sep 17 00:00:00 2001
From: John Lambert <john_lambert@sil.org>
Date: Tue, 5 Sep 2023 16:19:10 -0400
Subject: [PATCH] Fix race condition #104 (#108)

Add unit test
---
 .../Services/DataFileService.cs               | 27 ++++++++++++------
 .../Services/DataFileServiceTests.cs          | 28 +++++++++++++++++++
 2 files changed, 46 insertions(+), 9 deletions(-)

diff --git a/src/Serval.DataFiles/Services/DataFileService.cs b/src/Serval.DataFiles/Services/DataFileService.cs
index e8341498..701c0c91 100644
--- a/src/Serval.DataFiles/Services/DataFileService.cs
+++ b/src/Serval.DataFiles/Services/DataFileService.cs
@@ -103,19 +103,28 @@ await _deletedFiles.InsertAsync(
 
     public override async Task<bool> DeleteAsync(string id, CancellationToken cancellationToken = default)
     {
+        // return true if the deletion was successful, false if the file did not exist or was already deleted.
         await _dataAccessContext.BeginTransactionAsync(cancellationToken);
         DataFile? dataFile = await Entities.DeleteAsync(id, cancellationToken);
         if (dataFile is not null)
         {
-            await _deletedFiles.InsertAsync(
-                new DeletedFile
-                {
-                    Id = id,
-                    Filename = dataFile.Filename,
-                    DeletedAt = DateTime.UtcNow
-                },
-                cancellationToken
-            );
+            try
+            {
+                await _deletedFiles.InsertAsync(
+                    new DeletedFile
+                    {
+                        Id = id,
+                        Filename = dataFile.Filename,
+                        DeletedAt = DateTime.UtcNow
+                    },
+                    cancellationToken
+                );
+            }
+            catch (DuplicateKeyException)
+            {
+                // if it was already deleted, return false
+                return false;
+            }
         }
         await _mediator.Publish(new DataFileDeleted { DataFileId = id }, cancellationToken);
         await _dataAccessContext.CommitTransactionAsync(CancellationToken.None);
diff --git a/tests/Serval.DataFiles.Tests/Services/DataFileServiceTests.cs b/tests/Serval.DataFiles.Tests/Services/DataFileServiceTests.cs
index 81cbf6e6..3257ca49 100644
--- a/tests/Serval.DataFiles.Tests/Services/DataFileServiceTests.cs
+++ b/tests/Serval.DataFiles.Tests/Services/DataFileServiceTests.cs
@@ -115,6 +115,34 @@ public async Task DeleteAsync_DoesNotExist()
         Assert.That(deleted, Is.False);
     }
 
+    [Test]
+    public async Task DeleteAsync_Twice()
+    {
+        var env = new TestEnvironment();
+        env.DataFiles.Add(
+            new DataFile
+            {
+                Id = DATA_FILE_ID,
+                Name = "file1",
+                Filename = "file1.txt"
+            }
+        );
+        bool deleted = await env.Service.DeleteAsync(DATA_FILE_ID);
+        Assert.That(deleted, Is.True);
+        // The same file would not be added twice, but this is to check
+        env.DataFiles.Add(
+            new DataFile
+            {
+                Id = DATA_FILE_ID,
+                Name = "file1",
+                Filename = "file1.txt"
+            }
+        );
+        deleted = await env.Service.DeleteAsync(DATA_FILE_ID);
+        // The real condition is a race condition - trying to delete the same file twice (at the same time).  One should fail.
+        Assert.That(deleted, Is.False);
+    }
+
     private class TestEnvironment
     {
         public TestEnvironment()