From eedd776578a181067f086a8596654826b96e8bc3 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Bj=C3=B8rn=20Tore=20Gjerde?= Date: Tue, 17 Sep 2024 10:44:00 +0200 Subject: [PATCH] Feat/717 Ensure unique filenames in eFormidling shipment (#774) * Add missing await in DefaultEFormidlingService.cs. * Ensure filenames being sent to eFormidling are unique. * Suggested tests with fix * Fix sonar cloud issue * Process data elements ordered by created datetime. --------- Co-authored-by: Ivar Nesje --- .../DefaultEFormidlingService.cs | 86 ++++++++- .../DefaultEFormidlingServiceTests.cs | 176 ++++++++++++++++-- 2 files changed, 234 insertions(+), 28 deletions(-) diff --git a/src/Altinn.App.Core/EFormidling/Implementation/DefaultEFormidlingService.cs b/src/Altinn.App.Core/EFormidling/Implementation/DefaultEFormidlingService.cs index b96fd3748..bb6d48b19 100644 --- a/src/Altinn.App.Core/EFormidling/Implementation/DefaultEFormidlingService.cs +++ b/src/Altinn.App.Core/EFormidling/Implementation/DefaultEFormidlingService.cs @@ -101,14 +101,14 @@ public async Task SendEFormidlingShipment(Instance instance) StandardBusinessDocument sbd = await ConstructStandardBusinessDocument(instanceGuid, instance); await _eFormidlingClient.CreateMessage(sbd, requestHeaders); - (string metadataName, Stream stream) = await _eFormidlingMetadata.GenerateEFormidlingMetadata(instance); + (string metadataFilename, Stream stream) = await _eFormidlingMetadata.GenerateEFormidlingMetadata(instance); using (stream) { - await _eFormidlingClient.UploadAttachment(stream, instanceGuid, metadataName, requestHeaders); + await _eFormidlingClient.UploadAttachment(stream, instanceGuid, metadataFilename, requestHeaders); } - await SendInstanceData(instance, requestHeaders); + await SendInstanceData(instance, requestHeaders, metadataFilename); try { @@ -192,25 +192,47 @@ Instance instance return sbd; } - private async Task SendInstanceData(Instance instance, Dictionary requestHeaders) + private async Task SendInstanceData( + Instance instance, + Dictionary requestHeaders, + string eformidlingMetadataFilename + ) { ApplicationMetadata applicationMetadata = await _appMetadata.GetApplicationMetadata(); Guid instanceGuid = Guid.Parse(instance.Id.Split("/")[1]); int instanceOwnerPartyId = int.Parse(instance.InstanceOwner.PartyId, CultureInfo.InvariantCulture); - foreach (DataElement dataElement in instance.Data) + + // Keep track of already used file names to ensure they are unique. eFormidling does not allow duplicate filenames. + HashSet usedFileNames = [eformidlingMetadataFilename]; + + List dataTypeIds = applicationMetadata.DataTypes.Select(x => x.Id).ToList(); + + foreach (DataElement dataElement in instance.Data.OrderBy(x => x.Created)) { if (!applicationMetadata.EFormidling.DataTypes.Contains(dataElement.DataType)) { continue; } - bool appLogic = applicationMetadata.DataTypes.Any(d => - d.Id == dataElement.DataType && d.AppLogic?.ClassRef != null + DataType dataType = + applicationMetadata.DataTypes.Find(d => d.Id == dataElement.DataType) + ?? throw new InvalidOperationException( + $"DataType {dataElement.DataType} not found in application metadata" + ); + + bool hasAppLogic = dataType.AppLogic?.ClassRef is not null; + + string uniqueFileName = GetUniqueFileName( + dataElement.Filename, + dataType.Id, + hasAppLogic, + dataTypeIds, + usedFileNames ); + usedFileNames.Add(uniqueFileName); - string fileName = appLogic ? $"{dataElement.DataType}.xml" : dataElement.Filename; - using Stream stream = await _dataClient.GetBinaryData( + await using Stream stream = await _dataClient.GetBinaryData( applicationMetadata.Org, applicationMetadata.AppIdentifier.App, instanceOwnerPartyId, @@ -222,7 +244,7 @@ private async Task SendInstanceData(Instance instance, Dictionary dataTypeIds, + HashSet usedFileNames + ) + { + if (hasAppLogic) + { + // Data types with classRef should get filename based on DataType. + fileName = $"{dataTypeId}.xml"; + } + else if (string.IsNullOrWhiteSpace(fileName)) + { + // If no filename is set, default to DataType. + fileName = dataTypeId; + } + else if ( + !dataTypeIds.TrueForAll(id => + id == dataTypeId || !fileName.StartsWith(id, StringComparison.OrdinalIgnoreCase) + ) + ) + { + // If the file starts with another data types id, prepend the current data type id to avoid stealing the counter-less filename from the AppLogic data element. + fileName = $"{dataTypeId}-{fileName}"; + } + string name = Path.GetFileNameWithoutExtension(fileName); + string extension = Path.GetExtension(fileName); + + // Handle the case where there's no extension. + string uniqueFileName = string.IsNullOrEmpty(extension) ? name : $"{name}{extension}"; + var counter = 1; + + // Generate unique file name. + while (usedFileNames.Contains(uniqueFileName)) + { + uniqueFileName = string.IsNullOrEmpty(extension) ? $"{name}-{counter}" : $"{name}-{counter}{extension}"; + counter++; + } + + return uniqueFileName; + } } diff --git a/test/Altinn.App.Core.Tests/Eformidling/Implementation/DefaultEFormidlingServiceTests.cs b/test/Altinn.App.Core.Tests/Eformidling/Implementation/DefaultEFormidlingServiceTests.cs index eed41145a..3bc5423a9 100644 --- a/test/Altinn.App.Core.Tests/Eformidling/Implementation/DefaultEFormidlingServiceTests.cs +++ b/test/Altinn.App.Core.Tests/Eformidling/Implementation/DefaultEFormidlingServiceTests.cs @@ -1,4 +1,3 @@ -#nullable disable using Altinn.App.Core.Configuration; using Altinn.App.Core.Constants; using Altinn.App.Core.EFormidling; @@ -39,11 +38,57 @@ public void SendEFormidlingShipment() var eFormidlingClient = new Mock(); var tokenGenerator = new Mock(); var eFormidlingMetadata = new Mock(); + + const string eFormidlingMetadataFilename = "arkivmelding.xml"; + const string modelDataType = "model"; + const string fileAttachmentsDataType = "file-attachments"; + var instanceGuid = Guid.Parse("41C1099C-7EDD-47F5-AD1F-6267B497796F"); var instance = new Instance { - Id = "1337/41C1099C-7EDD-47F5-AD1F-6267B497796F", + Id = $"1337/{instanceGuid}", InstanceOwner = new InstanceOwner { PartyId = "1337", }, - Data = new List() + Data = + [ + new DataElement { Id = Guid.NewGuid().ToString(), DataType = modelDataType, }, + new DataElement + { + Id = Guid.NewGuid().ToString(), + DataType = fileAttachmentsDataType, + Filename = "attachment.txt" + }, + new DataElement + { + Id = Guid.NewGuid().ToString(), + DataType = fileAttachmentsDataType, + Filename = "attachment.txt" + }, + new DataElement + { + Id = Guid.NewGuid().ToString(), + DataType = fileAttachmentsDataType, + Filename = "no-extension" + }, + new DataElement + { + Id = Guid.NewGuid().ToString(), + DataType = fileAttachmentsDataType, + Filename = null + }, + //Same filename as the eFormidling metadata file. + new DataElement + { + Id = Guid.NewGuid().ToString(), + DataType = fileAttachmentsDataType, + Filename = eFormidlingMetadataFilename + }, + //Same filename as model data type. + new DataElement + { + Id = Guid.NewGuid().ToString(), + DataType = fileAttachmentsDataType, + Filename = modelDataType + ".xml" + } + ] }; appMetadata @@ -52,6 +97,15 @@ public void SendEFormidlingShipment() new ApplicationMetadata("ttd/test-app") { Org = "ttd", + DataTypes = + [ + new DataType + { + Id = modelDataType, + AppLogic = new ApplicationLogic { ClassRef = "SomeClass" } + }, + new DataType { Id = fileAttachmentsDataType } + ], EFormidling = new EFormidlingContract { Process = "urn:no:difi:profile:arkivmelding:plan:3.0", @@ -59,7 +113,7 @@ public void SendEFormidlingShipment() TypeVersion = "v8", Type = "arkivmelding", SecurityLevel = 3, - DataTypes = new List() + DataTypes = [modelDataType, fileAttachmentsDataType] } } ); @@ -70,8 +124,19 @@ public void SendEFormidlingShipment() .Setup(em => em.GenerateEFormidlingMetadata(instance)) .ReturnsAsync(() => { - return ("fakefilename.txt", Stream.Null); + return (eFormidlingMetadataFilename, Stream.Null); }); + dataClient + .Setup(x => + x.GetBinaryData( + It.IsAny(), + It.IsAny(), + It.IsAny(), + It.IsAny(), + It.IsAny() + ) + ) + .ReturnsAsync(Stream.Null); var defaultEformidlingService = new DefaultEFormidlingService( logger, @@ -104,15 +169,42 @@ public void SendEFormidlingShipment() eFormidlingReceivers.Verify(er => er.GetEFormidlingReceivers(instance)); eFormidlingMetadata.Verify(em => em.GenerateEFormidlingMetadata(instance)); eFormidlingClient.Verify(ec => ec.CreateMessage(It.IsAny(), expectedReqHeaders)); + eFormidlingClient.Verify(ec => + ec.UploadAttachment(Stream.Null, instanceGuid.ToString(), eFormidlingMetadataFilename, expectedReqHeaders) + ); + eFormidlingClient.Verify(ec => + ec.UploadAttachment(Stream.Null, instanceGuid.ToString(), $"{modelDataType}.xml", expectedReqHeaders) + ); + eFormidlingClient.Verify(ec => + ec.UploadAttachment(Stream.Null, instanceGuid.ToString(), "attachment.txt", expectedReqHeaders) + ); + eFormidlingClient.Verify(ec => + ec.UploadAttachment(Stream.Null, instanceGuid.ToString(), "attachment-1.txt", expectedReqHeaders) + ); + eFormidlingClient.Verify(ec => + ec.UploadAttachment(Stream.Null, instanceGuid.ToString(), "no-extension", expectedReqHeaders) + ); + eFormidlingClient.Verify(ec => + ec.UploadAttachment(Stream.Null, instanceGuid.ToString(), fileAttachmentsDataType, expectedReqHeaders) + ); eFormidlingClient.Verify(ec => ec.UploadAttachment( Stream.Null, - "41C1099C-7EDD-47F5-AD1F-6267B497796F", - "fakefilename.txt", + instanceGuid.ToString(), + $"{Path.GetFileNameWithoutExtension(eFormidlingMetadataFilename)}-1.xml", expectedReqHeaders ) ); - eFormidlingClient.Verify(ec => ec.SendMessage("41C1099C-7EDD-47F5-AD1F-6267B497796F", expectedReqHeaders)); + eFormidlingClient.Verify(ec => + ec.UploadAttachment( + Stream.Null, + instanceGuid.ToString(), + $"{fileAttachmentsDataType}-{modelDataType}.xml", + expectedReqHeaders + ) + ); + + eFormidlingClient.Verify(ec => ec.SendMessage(instanceGuid.ToString(), expectedReqHeaders)); eventClient.Verify(e => e.AddEvent(EformidlingConstants.CheckInstanceStatusEventType, instance)); eFormidlingClient.VerifyNoOtherCalls(); @@ -125,6 +217,57 @@ public void SendEFormidlingShipment() result.IsCompletedSuccessfully.Should().BeTrue(); } + [Theory] + // Filename does not have a prefix for any data type, but collides with previous test-1.txt file, so it skips + [InlineData("test.txt", "a", false, "test.txt", "test-2.txt")] + // App logic data types, always gets the {dataType}.xml name (and skips existing indexes) + [InlineData("test.txt", "a", true, "a.xml", "a-2.xml")] + // Filename gets "{dataType}-" prefix if the given name is a prefix of another type + [InlineData("abc.txt", "a", false, "a-abc.txt", "a-abc-1.txt")] + // Filename does not get "{dataType}-" prefix if the given name is a prefix of only the same type + [InlineData("abc.txt", "ab", false, "ab-abc.txt", "ab-abc-1.txt")] + // Filename is null without applogic, so just use the dataType, and add suffix for uniqueness + [InlineData(null, "ab", false, "ab", "ab-1")] + // Filename is null, but with app logic, so use {dataType}.xml + [InlineData(null, "ab", true, "ab.xml", "ab-1.xml")] + // Filename prefixes dataType c, so it gets the {dataType}- prefix + [InlineData("car.txt", "a", false, "a-car.txt", "a-car-1.txt")] + // Filename prefixes dataType c, but is the same as the dataType, so it doesn't get {dataType}- prefix + [InlineData("car.txt", "c", false, "car.txt", "car-1.txt")] + public void UniqueFileName( + string? fileName, + string dataTypeId, + bool hasAppLogic, + string expected1, + string expected2 + ) + { + var dataTypeIds = new List { "a", "ab", "c" }; + var usedFileNames = new HashSet { "test-1.txt", "a-1.xml" }; + + var uniqueFileName = DefaultEFormidlingService.GetUniqueFileName( + fileName, + dataTypeId, + hasAppLogic, + dataTypeIds, + usedFileNames + ); + usedFileNames.Add(uniqueFileName); + + uniqueFileName.Should().Be(expected1); + + uniqueFileName = DefaultEFormidlingService.GetUniqueFileName( + fileName, + dataTypeId, + hasAppLogic, + dataTypeIds, + usedFileNames + ); + usedFileNames.Add(uniqueFileName); + + uniqueFileName.Should().Be(expected2); + } + [Fact] public void SendEFormidlingShipment_throws_exception_if_send_fails() { @@ -142,9 +285,10 @@ public void SendEFormidlingShipment_throws_exception_if_send_fails() var eFormidlingClient = new Mock(); var tokenGenerator = new Mock(); var eFormidlingMetadata = new Mock(); + var instanceGuid = Guid.Parse("41C1099C-7EDD-47F5-AD1F-6267B497796F"); var instance = new Instance { - Id = "1337/41C1099C-7EDD-47F5-AD1F-6267B497796F", + Id = $"1337/{instanceGuid}", InstanceOwner = new InstanceOwner { PartyId = "1337", }, Data = new List() }; @@ -163,7 +307,8 @@ public void SendEFormidlingShipment_throws_exception_if_send_fails() Type = "arkivmelding", SecurityLevel = 3, DataTypes = new List() - } + }, + DataTypes = [] } ); tokenGenerator.Setup(t => t.GenerateAccessToken("ttd", "test-app")).Returns("access-token"); @@ -173,7 +318,7 @@ public void SendEFormidlingShipment_throws_exception_if_send_fails() .Setup(em => em.GenerateEFormidlingMetadata(instance)) .ReturnsAsync(() => { - return ("fakefilename.txt", Stream.Null); + return ("arkivmelding.xml", Stream.Null); }); eFormidlingClient .Setup(ec => ec.SendMessage(It.IsAny(), It.IsAny>())) @@ -212,14 +357,9 @@ public void SendEFormidlingShipment_throws_exception_if_send_fails() eFormidlingMetadata.Verify(em => em.GenerateEFormidlingMetadata(instance)); eFormidlingClient.Verify(ec => ec.CreateMessage(It.IsAny(), expectedReqHeaders)); eFormidlingClient.Verify(ec => - ec.UploadAttachment( - Stream.Null, - "41C1099C-7EDD-47F5-AD1F-6267B497796F", - "fakefilename.txt", - expectedReqHeaders - ) + ec.UploadAttachment(Stream.Null, instanceGuid.ToString(), "arkivmelding.xml", expectedReqHeaders) ); - eFormidlingClient.Verify(ec => ec.SendMessage("41C1099C-7EDD-47F5-AD1F-6267B497796F", expectedReqHeaders)); + eFormidlingClient.Verify(ec => ec.SendMessage(instanceGuid.ToString(), expectedReqHeaders)); eFormidlingClient.VerifyNoOtherCalls(); eventClient.VerifyNoOtherCalls();