From cca20f157ee57c0ff3e69d6a4cbee05945c8fdc1 Mon Sep 17 00:00:00 2001 From: unknown Date: Tue, 22 Sep 2026 11:34:15 +0100 Subject: [PATCH 1/2] deletion of unexisting files --- .../Unit/EgressStorageClientTests.cs | 73 ++++++++++++++++--- .../Client/EgressStorageClient.cs | 52 ++++++++----- .../Models/Response/DeleteFilesResponse.cs | 4 + .../Unit/Durable/Activity/DeleteFilesTests.cs | 19 +++-- .../Durable/Activity/DeleteFiles.cs | 10 +-- 5 files changed, 115 insertions(+), 43 deletions(-) diff --git a/backend/CPS.ComplexCases.Egress.Tests/Unit/EgressStorageClientTests.cs b/backend/CPS.ComplexCases.Egress.Tests/Unit/EgressStorageClientTests.cs index 8b6720287..e7c0f05d1 100644 --- a/backend/CPS.ComplexCases.Egress.Tests/Unit/EgressStorageClientTests.cs +++ b/backend/CPS.ComplexCases.Egress.Tests/Unit/EgressStorageClientTests.cs @@ -367,7 +367,7 @@ public async Task DeleteFilesAsync_WithCodeZeroAndNullFileId_PopulatesDeletedFil var result = await _client.DeleteFilesAsync(filesToDelete, workspaceId); Assert.True(result.AllSuccessful); - Assert.Equal(["file-1", "file-2"], result.DeletedFiles); + Assert.Equal(["file1.txt", "file2.txt"], result.DeletedFiles); Assert.Empty(result.FailedFiles!); } @@ -711,7 +711,7 @@ public async Task DeleteFilesAsync_WithOnlyNullOrWhitespaceFileIds_ReturnsEmptyR } [Fact] - public async Task DeleteFilesAsync_WhenEgressOmitsFiles_TreatsRequestedIdsAsDeleted() + public async Task DeleteFilesAsync_WhenEgressOmitsFiles_ReturnsNotAllSuccessful() { var workspaceId = _fixture.Create(); var token = _fixture.Create(); @@ -736,36 +736,59 @@ public async Task DeleteFilesAsync_WhenEgressOmitsFiles_TreatsRequestedIdsAsDele var result = await _client.DeleteFilesAsync(filesToDelete, workspaceId); - Assert.True(result.AllSuccessful); - Assert.Equal(["file-1", "file-2"], result.DeletedFiles); + Assert.False(result.AllSuccessful); + Assert.Equal(["file-1"], result.DeletedFiles); Assert.Empty(result.FailedFiles!); } [Fact] - public async Task DeleteFilesAsync_WhenEgressOmitsFilesArray_TreatsRequestedIdsAsDeleted() + public async Task DeleteFilesAsync_WhenEgressReturnsEmptyResultsForUnknownId_DoesNotTreatAsDeleted() { var workspaceId = _fixture.Create(); var token = _fixture.Create(); var filesToDelete = new List { - new() { Path = "folder/file1.txt", FileId = "6a7b09840b11b5e3185286b7" } + new() { Path = "folder/missing.txt", FileId = "000000000000000000000000" } }; SetupTokenRequest(token); SetupDeleteFilesRequest(workspaceId, token); SetupHttpMockResponses( ("token", new GetWorkspaceTokenResponse { Token = token }), - ("delete", new { all_successful = true })); + ("delete", new { all_successful = true, results = Array.Empty() })); var result = await _client.DeleteFilesAsync(filesToDelete, workspaceId); - Assert.True(result.AllSuccessful); - Assert.Equal(["6a7b09840b11b5e3185286b7"], result.DeletedFiles); + Assert.False(result.AllSuccessful); + Assert.Empty(result.DeletedFiles!); Assert.Empty(result.FailedFiles!); } [Fact] - public async Task DeleteFilesAsync_WhenEgressReturnsIdInsteadOfFileId_TreatsRequestedIdsAsDeleted() + public async Task DeleteFilesAsync_WhenEgressReturnsEmptyFilesForUnknownId_DoesNotTreatAsDeleted() + { + var workspaceId = _fixture.Create(); + var token = _fixture.Create(); + var filesToDelete = new List + { + new() { Path = "folder/missing.txt", FileId = "000000000000000000000000" } + }; + + SetupTokenRequest(token); + SetupDeleteFilesRequest(workspaceId, token); + SetupHttpMockResponses( + ("token", new GetWorkspaceTokenResponse { Token = token }), + ("delete", new { all_successful = true, files = Array.Empty() })); + + var result = await _client.DeleteFilesAsync(filesToDelete, workspaceId); + + Assert.False(result.AllSuccessful); + Assert.Empty(result.DeletedFiles!); + Assert.Empty(result.FailedFiles!); + } + + [Fact] + public async Task DeleteFilesAsync_WhenEgressReturnsFilesWithId_PopulatesDeletedFiles() { var workspaceId = _fixture.Create(); var token = _fixture.Create(); @@ -794,6 +817,36 @@ public async Task DeleteFilesAsync_WhenEgressReturnsIdInsteadOfFileId_TreatsRequ Assert.Empty(result.FailedFiles!); } + [Fact] + public async Task DeleteFilesAsync_WhenEgressReturnsResultsWithId_PopulatesDeletedFiles() + { + var workspaceId = _fixture.Create(); + var token = _fixture.Create(); + var filesToDelete = new List + { + new() { Path = "4. Served Evidence/1000mb.txt", FileId = "6a7b09840b11b5e3185286b7" } + }; + + SetupTokenRequest(token); + SetupDeleteFilesRequest(workspaceId, token); + SetupHttpMockResponses( + ("token", new GetWorkspaceTokenResponse { Token = token }), + ("delete", new + { + all_successful = true, + results = new[] + { + new { code = 0, id = "6a7b09840b11b5e3185286b7", filename = "1000mb.txt", is_folder = false } + } + })); + + var result = await _client.DeleteFilesAsync(filesToDelete, workspaceId); + + Assert.True(result.AllSuccessful); + Assert.Equal(["6a7b09840b11b5e3185286b7"], result.DeletedFiles); + Assert.Empty(result.FailedFiles!); + } + [Fact] public async Task DeleteFilesAsync_WhenEgressReportsNotAllSuccessful_ReturnsNotAllSuccessful() { diff --git a/backend/CPS.ComplexCases.Egress/Client/EgressStorageClient.cs b/backend/CPS.ComplexCases.Egress/Client/EgressStorageClient.cs index 2184924ad..82f2c6298 100644 --- a/backend/CPS.ComplexCases.Egress/Client/EgressStorageClient.cs +++ b/backend/CPS.ComplexCases.Egress/Client/EgressStorageClient.cs @@ -1,4 +1,5 @@ using System.Net; +using System.Text.Json; using CPS.ComplexCases.Common.Extensions; using CPS.ComplexCases.Common.Models.Domain; using CPS.ComplexCases.Common.Models.Domain.Dtos; @@ -272,9 +273,25 @@ public async Task DeleteFilesAsync(List fi FileIds = [.. chunk] }; - var result = await SendRequestAsync(_egressRequestFactory.DeleteFilesRequest(deleteArg, token)); - var files = result.Files ?? []; + using var response = await SendRequestAsync(_egressRequestFactory.DeleteFilesRequest(deleteArg, token)); + var responseContent = await response.Content.ReadAsStringAsync(); + var result = JsonSerializer.Deserialize(responseContent) + ?? throw new InvalidOperationException("Deserialization returned null."); + allSuccessful &= result.AllSuccessful; + + var files = result.Items ?? []; + if (files.Count == 0) + { + _logger.LogWarning( + "Egress bulk delete returned no per-file confirmations for workspace {WorkspaceId}. AllSuccessful={AllSuccessful}, RequestedCount={RequestedCount}, Response={Response}", + workspaceId, + result.AllSuccessful, + chunk.Length, + responseContent); + } + var failedResults = files.Where(x => x.Code > 0).ToList(); + var successfulResults = files.Where(x => x.Code == 0).ToList(); failedFiles.AddRange(failedResults.Select(x => new FailedFileDeletion { @@ -283,23 +300,24 @@ public async Task DeleteFilesAsync(List fi Reason = GetDeleteFailureReason(x) })); - if (result.AllSuccessful && failedResults.Count == 0) - { - // Egress often returns HTTP 200 / all_successful without per-file - // `file_id` values (it uses `id`, or omits the files array). Trust - // that overall success and treat the requested IDs as deleted so - // move transfers are not marked PartiallyCompleted. - deletedFiles.AddRange(chunk); - continue; - } - - allSuccessful &= result.AllSuccessful; - - deletedFiles.AddRange(files - .Where(x => x.Code == 0) + var identifiedDeleted = successfulResults .Select(x => x.ResolvedFileId) .Where(id => !string.IsNullOrEmpty(id)) - .Select(id => id!)); + .Select(id => id!) + .ToList(); + deletedFiles.AddRange(identifiedDeleted); + + // A code-0 file/result entry with no id still counts as a confirmed delete. Pair leftover + // requested ids to those unidentified successes. An empty files/results list does not — + // AllSuccessful with no per-file rows must not be treated as deleted (AC 2). + var unidentifiedSuccessCount = successfulResults.Count - identifiedDeleted.Count; + if (unidentifiedSuccessCount > 0) + { + var remainingRequestedIds = chunk.Where(id => + !identifiedDeleted.Contains(id, StringComparer.OrdinalIgnoreCase) && + failedResults.All(f => !id.Equals(f.ResolvedFileId, StringComparison.OrdinalIgnoreCase))); + deletedFiles.AddRange(remainingRequestedIds.Take(unidentifiedSuccessCount)); + } } return new DeleteFilesResult diff --git a/backend/CPS.ComplexCases.Egress/Models/Response/DeleteFilesResponse.cs b/backend/CPS.ComplexCases.Egress/Models/Response/DeleteFilesResponse.cs index 5bcac2bbd..09239ab37 100644 --- a/backend/CPS.ComplexCases.Egress/Models/Response/DeleteFilesResponse.cs +++ b/backend/CPS.ComplexCases.Egress/Models/Response/DeleteFilesResponse.cs @@ -8,7 +8,11 @@ public class DeleteFilesResponse public bool AllSuccessful { get; set; } [JsonPropertyName("files")] public List Files { get; set; } = []; + [JsonPropertyName("results")] + public List Results { get; set; } = []; + [JsonIgnore] + public List Items => Results.Count > 0 ? Results : Files; } public class DeletedFileResult diff --git a/backend/CPS.ComplexCases.FileTransfer.API.Tests/Unit/Durable/Activity/DeleteFilesTests.cs b/backend/CPS.ComplexCases.FileTransfer.API.Tests/Unit/Durable/Activity/DeleteFilesTests.cs index e8e81bcfd..6ce7aff88 100644 --- a/backend/CPS.ComplexCases.FileTransfer.API.Tests/Unit/Durable/Activity/DeleteFilesTests.cs +++ b/backend/CPS.ComplexCases.FileTransfer.API.Tests/Unit/Durable/Activity/DeleteFilesTests.cs @@ -356,7 +356,7 @@ public async Task Run_UsesDeletedFilesCount_WhenDeletedFilesAreReturned() } [Fact] - public async Task Run_DoesNotRecordDeletionErrors_WhenDeletedFilesIsEmptyAndAllSuccessful() + public async Task Run_RecordsAllFilesAsDeletionErrors_WhenDeletedFilesIsEmpty() { var payload = CreateEgressToNetAppPayload(); var items = CreateCompletedItems(("file1.txt", "f1"), ("file2.txt", "f2")); @@ -373,16 +373,19 @@ public async Task Run_DoesNotRecordDeletionErrors_WhenDeletedFilesIsEmptyAndAllS c => c.DeleteMovedItemsCompleted( It.IsAny(), payload.TransferId, - It.Is>(errors => errors.Count == 0), + It.Is>(errors => + errors.Count == 2 && + errors.Any(e => e.FileId == "f1") && + errors.Any(e => e.FileId == "f2")), It.IsAny()), Times.Once); _telemetryClientMock.Verify( t => t.TrackEvent(It.Is(e => - e.TotalFilesDeleted == 2 && - e.TotalFilesFailedToDelete == 0 && - e.IsSuccessful && - string.IsNullOrEmpty(e.FailureReasons))), + e.TotalFilesDeleted == 0 && + e.TotalFilesFailedToDelete == 2 && + !e.IsSuccessful && + e.FailureReasons == "File was not confirmed deleted by Egress. (2)")), Times.Once); } @@ -429,7 +432,7 @@ public async Task Run_RecordsMissingFilesAsDeletionErrors_WhenDeletedFilesCountI SetupDeleteRun(payload, items, new DeleteFilesResult { - AllSuccessful = false, + AllSuccessful = true, DeletedFiles = items.Take(10).Select(x => x.FileId!).ToList(), FailedFiles = [] }); @@ -500,7 +503,7 @@ public async Task Run_RecordsEveryUnmatchedFile_WhenDeletedIdentifiersDoNotMatch var items = CreateCompletedItems(("file1.txt", "f1"), ("file2.txt", "f2"), ("file3.txt", "f3")); SetupDeleteRun(payload, items, new DeleteFilesResult { - AllSuccessful = false, + AllSuccessful = true, DeletedFiles = ["deleted", "f2"], FailedFiles = [] }); diff --git a/backend/CPS.ComplexCases.FileTransfer.API/Durable/Activity/DeleteFiles.cs b/backend/CPS.ComplexCases.FileTransfer.API/Durable/Activity/DeleteFiles.cs index 9f3c4361e..2679ba4f0 100644 --- a/backend/CPS.ComplexCases.FileTransfer.API/Durable/Activity/DeleteFiles.cs +++ b/backend/CPS.ComplexCases.FileTransfer.API/Durable/Activity/DeleteFiles.cs @@ -162,14 +162,8 @@ private static List BuildDeletionErrors(List f return deletionErrors; } - // Egress confirms overall success (all_successful / no failed files) without - // returning identifiers we can match. Do not treat that as a source-delete - // failure — that incorrectly marks a successful move as PartiallyCompleted. - if (failedFiles.Count == 0 && result.AllSuccessful) - { - return deletionErrors; - } - + // AC 2: compare confirmed deletes against the requested count. AllSuccessful with + // no DeletedFiles (for example an unknown file id) must still record DeletionErrors. foreach (var file in unaccountedFiles) { deletionErrors.Add(new DeletionError From 963cc13c75945f5e5ef5f88a57f5e3c3f5e568be Mon Sep 17 00:00:00 2001 From: unknown Date: Tue, 22 Sep 2026 11:39:13 +0100 Subject: [PATCH 2/2] deletion of existing files --- backend/CPS.ComplexCases.Egress/Client/EgressStorageClient.cs | 2 +- .../Durable/Activity/DeleteFiles.cs | 2 +- 2 files changed, 2 insertions(+), 2 deletions(-) diff --git a/backend/CPS.ComplexCases.Egress/Client/EgressStorageClient.cs b/backend/CPS.ComplexCases.Egress/Client/EgressStorageClient.cs index 82f2c6298..679673e5f 100644 --- a/backend/CPS.ComplexCases.Egress/Client/EgressStorageClient.cs +++ b/backend/CPS.ComplexCases.Egress/Client/EgressStorageClient.cs @@ -309,7 +309,7 @@ public async Task DeleteFilesAsync(List fi // A code-0 file/result entry with no id still counts as a confirmed delete. Pair leftover // requested ids to those unidentified successes. An empty files/results list does not — - // AllSuccessful with no per-file rows must not be treated as deleted (AC 2). + // AllSuccessful with no per-file rows must not be treated as deleted var unidentifiedSuccessCount = successfulResults.Count - identifiedDeleted.Count; if (unidentifiedSuccessCount > 0) { diff --git a/backend/CPS.ComplexCases.FileTransfer.API/Durable/Activity/DeleteFiles.cs b/backend/CPS.ComplexCases.FileTransfer.API/Durable/Activity/DeleteFiles.cs index 2679ba4f0..bc91514b7 100644 --- a/backend/CPS.ComplexCases.FileTransfer.API/Durable/Activity/DeleteFiles.cs +++ b/backend/CPS.ComplexCases.FileTransfer.API/Durable/Activity/DeleteFiles.cs @@ -162,7 +162,7 @@ private static List BuildDeletionErrors(List f return deletionErrors; } - // AC 2: compare confirmed deletes against the requested count. AllSuccessful with + // compare confirmed deletes against the requested count. AllSuccessful with // no DeletedFiles (for example an unknown file id) must still record DeletionErrors. foreach (var file in unaccountedFiles) {