Cdms-183 (#2)

* 1. Updated KeyDataPairsToDictionaryStringObjectJsonConverter to support when data is null, and added some tests 2. Updated audit log to use a double when ValueKind is a number to support real numbers, and added some tests 3. Updated document to documents 4. Fixed an issue with concurrency within Linking due to the etag being empty * changed the sample/test data to use documents * Couple of updates after reviewing PR * refactored to reduce the cognitive complixity --------- Co-authored-by: Thomas Anderson <[email protected]>
DEFRA · Dec 6, 2024 · c497790 · c497790
1 parent d2babf4
commit c497790
Show file tree

Hide file tree

Showing 24 changed files with 171 additions and 36 deletions.
diff --git a/Btms.Backend.Data/ConcurrencyException.cs b/Btms.Backend.Data/ConcurrencyException.cs
@@ -0,0 +1,8 @@
+namespace Btms.Backend.Data;
+
+public class ConcurrencyException(string entityId, string entityEtag) : Exception($"Failed up update {entityId} with etag {entityEtag}")
+{
+    public string EntityId { get; } = entityId;
+
+    public string EntityEtag { get; } = entityEtag;
+}
diff --git a/Btms.Backend.Data/InMemory/MemoryCollectionSet.cs b/Btms.Backend.Data/InMemory/MemoryCollectionSet.cs
@@ -48,7 +48,7 @@ public Task Update(T item, string etag, IMongoDbTransaction transaction = defaul
 
         if ((existingItem._Etag ?? "") != etag)
         {
-            throw new ConcurrencyException("Concurrency Error, change this to a Concurrency exception");
+            throw new ConcurrencyException(item.Id!, etag);
         }
 
         item._Etag = BsonObjectIdGenerator.Instance.GenerateId(null, null).ToString()!;

diff --git a/Btms.Backend.Data/MigrationException.cs b/Btms.Backend.Data/MigrationException.cs
diff --git a/Btms.Backend.Data/Mongo/MongoCollectionSet.cs b/Btms.Backend.Data/Mongo/MongoCollectionSet.cs
@@ -50,6 +50,7 @@ public Task Insert(T item, IMongoDbTransaction transaction = null!, Cancellation
         public async Task Update(T item, string etag, IMongoDbTransaction transaction = null!,
             CancellationToken cancellationToken = default)
         {
+            ArgumentNullException.ThrowIfNull(etag);
             var builder = Builders<T>.Filter;
 
             var filter = builder.Eq(x => x.Id, item.Id) & builder.Eq(x => x._Etag, etag);
@@ -66,7 +67,7 @@ public async Task Update(T item, string etag, IMongoDbTransaction transaction =
 
             if (updateResult.ModifiedCount == 0)
             {
-                throw new ConcurrencyException("Concurrency Error, change this to a Concurrency exception");
+                throw new ConcurrencyException(item.Id!, etag);
             }
         }
 

diff --git a/...ests/Fixtures/SmokeTest/ALVS/CHEDAGB20241041389-bbaff9af-4415-444c-b8c2-b62cde50786f.json b/...ests/Fixtures/SmokeTest/ALVS/CHEDAGB20241041389-bbaff9af-4415-444c-b8c2-b62cde50786f.json
@@ -33,7 +33,7 @@
       "itemSupplementaryUnits": null,
       "itemThirdQuantity": null,
       "itemOriginCountryCode": null,
-      "document": [
+      "documents": [
         {
           "documentCode": "C640",
           "documentReference": "GBCHD2024.1041389",

diff --git a/...ests/Fixtures/SmokeTest/ALVS/CHEDDGB20241004777-cbaff9af-4415-444c-b8c2-b62cde50786f.json b/...ests/Fixtures/SmokeTest/ALVS/CHEDDGB20241004777-cbaff9af-4415-444c-b8c2-b62cde50786f.json
@@ -33,7 +33,7 @@
       "itemSupplementaryUnits": null,
       "itemThirdQuantity": null,
       "itemOriginCountryCode": null,
-      "document": [
+      "documents": [
         {
           "documentCode": "N852",
           "documentReference": "GBCHD2024.1004777",

diff --git a/...ts/Fixtures/SmokeTest/ALVS/CHEDPGB20241042294A5-f5ee76a9-cf9a-40a9-bffb-1b59399071b5.json b/...ts/Fixtures/SmokeTest/ALVS/CHEDPGB20241042294A5-f5ee76a9-cf9a-40a9-bffb-1b59399071b5.json
@@ -32,7 +32,7 @@
       "itemSupplementaryUnits": 0,
       "itemThirdQuantity": null,
       "itemOriginCountryCode": "NZ",
-      "document": [
+      "documents": [
         {
           "documentCode": "N853",
           "documentReference": "GBCVD2024.1042294",

diff --git a/Btms.Backend.Test/Auditing/AuditingTests.cs b/Btms.Backend.Test/Auditing/AuditingTests.cs
@@ -0,0 +1,43 @@
+using Btms.Model.Auditing;
+using FluentAssertions;
+
+namespace Btms.Backend.Test.Auditing;
+
+public class AuditingTests
+{
+    public class TestClassOne
+    {
+        public double NumberValue { get; set; }
+    }
+
+    [Fact]
+    public void CreateAuditWhenDifferentIsDouble()
+    {
+        var previous = new TestClassOne() { NumberValue = 1.2 };
+        var current = new TestClassOne() { NumberValue = 2.2 };
+        var auditEntry = AuditEntry.CreateUpdated(previous, current, "testid", 1, DateTime.UtcNow);
+
+        auditEntry.Should().NotBeNull();
+        auditEntry.Diff.Count.Should().Be(1);
+        auditEntry.Diff[0].Value.Should().Be(2.2);
+        auditEntry.Diff[0].Op.Should().Be("Replace");
+        auditEntry.Diff[0].Path.Should().Be("/NumberValue");
+
+    }
+
+
+    [Fact]
+    public void CreateAuditWhenDifferentIsInt()
+    {
+        var previous = new TestClassOne() { NumberValue = 1 };
+        var current = new TestClassOne() { NumberValue = 2 };
+        var auditEntry = AuditEntry.CreateUpdated(previous, current, "testid", 1, DateTime.UtcNow);
+
+        auditEntry.Should().NotBeNull();
+        auditEntry.Diff.Count.Should().Be(1);
+        auditEntry.Diff[0].Value.Should().Be(2);
+        auditEntry.Diff[0].Op.Should().Be("Replace");
+        auditEntry.Diff[0].Path.Should().Be("/NumberValue");
+
+    }
+}
diff --git a/Btms.Backend.Test/JsonCoverters/KeyDataPairsToDictionaryStringObjectJsonConverterTests.cs b/Btms.Backend.Test/JsonCoverters/KeyDataPairsToDictionaryStringObjectJsonConverterTests.cs
@@ -0,0 +1,47 @@
+using System.Text.Json;
+using System.Text.Json.Serialization;
+using Btms.Types.Ipaffs;
+using FluentAssertions;
+
+namespace Btms.Backend.Test.JsonCoverters;
+
+public class KeyDataPairsToDictionaryStringObjectJsonConverterTests
+{
+    public class TestClass
+    {
+        [JsonPropertyName("keyDataPair")]
+        [JsonConverter(typeof(KeyDataPairsToDictionaryStringObjectJsonConverter))]
+        public IDictionary<string, object>? KeyDataPairs { get; set; }
+    }
+
+    [Fact]
+    public void GivenDataIsNotPresentInJson_ThenItShouldBeDeserializedAsNull()
+    {
+        var json =
+            "{\"keyDataPair\": [\r\n                        {\r\n                            \"key\": \"netweight\"\r\n                        },\r\n                        {\r\n                            \"key\": \"number_package\",\r\n                            \"data\": \"0\"\r\n                        },\r\n                        {\r\n                            \"key\": \"type_package\",\r\n                            \"data\": \"Case\"\r\n                        }\r\n                    ]}";
+
+        var result = JsonSerializer.Deserialize<TestClass>(json);
+
+        result.Should().NotBeNull();
+        result?.KeyDataPairs?.Count.Should().Be(3);
+        result?.KeyDataPairs?["netweight"].Should().BeNull();
+        result?.KeyDataPairs?["number_package"].Should().Be(0);
+        result?.KeyDataPairs?["type_package"].Should().Be("Case");
+
+    }
+
+    [Fact]
+    public void GivenValidJson_ThenEverythingShouldBeDeserialized()
+    {
+        var json =
+            "{\r\n\"keyDataPair\": [\r\n                        {\r\n                            \"key\": \"number_package\",\r\n                            \"data\": \"0\"\r\n                        },\r\n                        {\r\n                            \"key\": \"type_package\",\r\n                            \"data\": \"Case\"\r\n                        }\r\n                    ]\r\n}";
+
+        var result = JsonSerializer.Deserialize<TestClass>(json);
+
+        result.Should().NotBeNull();
+        result?.KeyDataPairs?.Count.Should().Be(2);
+        result?.KeyDataPairs?["number_package"].Should().Be(0);
+        result?.KeyDataPairs?["type_package"].Should().Be("Case");
+
+    }
+}
diff --git a/Btms.Business/Commands/DownloadNotificationsCommand.cs b/Btms.Business/Commands/DownloadNotificationsCommand.cs
@@ -15,6 +15,7 @@
 using Btms.SyncJob;
 using Btms.Types.Alvs;
 using Btms.Types.Gvms;
+using Microsoft.Extensions.Hosting;
 
 namespace Btms.Business.Commands;
 
@@ -27,7 +28,7 @@ public class DownloadCommand : IRequest, ISyncJob
     public string Timespan { get; } = null!;
     public string Resource { get; } = null!;
 
-    internal class Handler(IBlobService blobService, ISensitiveDataSerializer sensitiveDataSerializer, IWebHostEnvironment env) : IRequestHandler<DownloadCommand>
+    internal class Handler(IBlobService blobService, ISensitiveDataSerializer sensitiveDataSerializer, IHostEnvironment env) : IRequestHandler<DownloadCommand>
     {
 
         public async Task Handle(DownloadCommand request, CancellationToken cancellationToken)

diff --git a/Btms.Business/Services/ImportNotificationLinkContext.cs b/Btms.Business/Services/ImportNotificationLinkContext.cs
@@ -2,10 +2,10 @@
 
 namespace Btms.Business.Services;
 
-public record ImportNotificationLinkContext(ImportNotification ReceivedImportNotification, ImportNotification? ExistingImportNotification) : LinkContext
+public record ImportNotificationLinkContext(ImportNotification PersistedImportNotification, ImportNotification? ExistingImportNotification) : LinkContext
 {
     public override string GetIdentifiers()
     {
-        return ReceivedImportNotification._MatchReference;
+        return PersistedImportNotification._MatchReference;
     }
 }
diff --git a/Btms.Business/Services/LinkingService.cs b/Btms.Business/Services/LinkingService.cs
@@ -53,7 +53,7 @@ public async Task<LinkResult> Link(LinkContext linkContext, CancellationToken ca
                             return new LinkResult(LinkState.NotLinked);
                         }
 
-                        result = await FindMovementLinks(movementLinkContext.ReceivedMovement, cancellationToken);
+                        result = await FindMovementLinks(movementLinkContext.PersistedMovement, cancellationToken);
                         break;
                     case ImportNotificationLinkContext notificationLinkContext:
                         if (!ShouldLink(notificationLinkContext))
@@ -62,7 +62,7 @@ public async Task<LinkResult> Link(LinkContext linkContext, CancellationToken ca
                             return new LinkResult(LinkState.NotLinked);
                         }
 
-                        result = await FindImportNotificationLinks(notificationLinkContext.ReceivedImportNotification,
+                        result = await FindImportNotificationLinks(notificationLinkContext.PersistedImportNotification,
                             cancellationToken);
                         break;
                     default: throw new ArgumentException("context type not supported");
@@ -115,7 +115,7 @@ await dbContext.Notifications.Update(notification, notification._Etag, transacti
             }
             catch (Exception e)
             {
-                logger.LinkingFailed(e, linkContext.GetType().Name, linkContext.GetIdentifiers());
+                // No Exception is logged at this point, as its logged further up the stack
                 metrics.Faulted(e);
                 throw new LinkException(e);
             }
@@ -137,7 +137,7 @@ private static bool ShouldLink(MovementLinkContext movContext)
         if (movContext.ExistingMovement is null) return true;
 
         var existingItems = movContext.ExistingMovement.Items is null ? [] : movContext.ExistingMovement.Items;
-        var receivedItems = movContext.ReceivedMovement.Items is null ? [] : movContext.ReceivedMovement.Items;
+        var receivedItems = movContext.PersistedMovement.Items is null ? [] : movContext.PersistedMovement.Items;
 
         // Diff movements for fields of interest
         var existingDocs = existingItems
@@ -169,7 +169,7 @@ private static bool ShouldLink(ImportNotificationLinkContext notifContext)
                 c.CommodityId,
                 c.CommodityDescription
             }).ToList();
-        var receivedCommodities = notifContext.ReceivedImportNotification.Commodities?
+        var receivedCommodities = notifContext.PersistedImportNotification.Commodities?
             .Select(c => new
             {
                 c.CommodityId,

diff --git a/Btms.Business/Services/MovementLinkContext.cs b/Btms.Business/Services/MovementLinkContext.cs
@@ -2,10 +2,10 @@
 
 namespace Btms.Business.Services;
 
-public record MovementLinkContext(Movement ReceivedMovement, Movement? ExistingMovement) : LinkContext
+public record MovementLinkContext(Movement PersistedMovement, Movement? ExistingMovement) : LinkContext
 {
     public override string GetIdentifiers()
     {
-        return string.Join(',', ReceivedMovement._MatchReferences);
+        return string.Join(',', PersistedMovement._MatchReferences);
     }
 }
diff --git a/Btms.Consumers/AlvsClearanceRequestConsumer.cs b/Btms.Consumers/AlvsClearanceRequestConsumer.cs
@@ -8,6 +8,7 @@
 using SlimMessageBus;
 using System.Diagnostics.CodeAnalysis;
 using Btms.Consumers.Extensions;
+using Force.DeepCloner;
 using Items = Btms.Model.Alvs.Items;
 
 namespace Btms.Consumers
@@ -35,13 +36,14 @@ public async Task OnHandle(AlvsClearanceRequest message)
                 var internalClearanceRequest = AlvsClearanceRequestMapper.Map(message);
                 var movement = BuildMovement(internalClearanceRequest);
                 var existingMovement = await dbContext.Movements.Find(movement.Id!);
+                Movement persistedMovement = null!;
 
                 if (existingMovement is not null)
                 {
                     if (movement.ClearanceRequests[0].Header?.EntryVersionNumber >
                         existingMovement.ClearanceRequests[0].Header?.EntryVersionNumber)
                     {
-                        movement.AuditEntries = existingMovement.AuditEntries;
+                        persistedMovement = existingMovement.DeepClone();
                         var auditEntry = AuditEntry.CreateUpdated(existingMovement.ClearanceRequests[0],
                             movement.ClearanceRequests[0],
                             BuildNormalizedAlvsPath(auditId!),
@@ -65,10 +67,12 @@ public async Task OnHandle(AlvsClearanceRequest message)
                     {
                         logger.MessageSkipped(Context.GetJobId()!, auditId!, GetType().Name, message.Header?.EntryReference!);
                         Context.Skipped();
+                        return;
                     }
                 }
                 else
                 {
+                    persistedMovement = movement!;
                     var auditEntry = AuditEntry.CreateCreatedEntry(
                         movement.ClearanceRequests[0],
                         BuildNormalizedAlvsPath(auditId!),
@@ -78,7 +82,8 @@ public async Task OnHandle(AlvsClearanceRequest message)
                     await dbContext.Movements.Insert(movement);
                 }
 
-                var linkContext = new MovementLinkContext(movement, existingMovement);
+                //This should be existing, pre update (may need to clone)
+                var linkContext = new MovementLinkContext(persistedMovement, existingMovement);
                 var linkResult = await linkingService.Link(linkContext, Context.CancellationToken);
             }
         }

diff --git a/Btms.Consumers/Btms.Consumers.csproj b/Btms.Consumers/Btms.Consumers.csproj
@@ -17,6 +17,7 @@
   </ItemGroup>
 
   <ItemGroup>
+    <PackageReference Include="DeepCloner" Version="0.10.4" />
     <PackageReference Include="SlimMessageBus" Version="2.0.4" />
     <PackageReference Include="SlimMessageBus.Host.Memory" Version="2.5.3" />
   </ItemGroup>

diff --git a/Btms.Consumers/Extensions/ConsumerContextExtensions.cs b/Btms.Consumers/Extensions/ConsumerContextExtensions.cs
@@ -38,7 +38,7 @@ public static ActivityContext GetActivityContext(this IConsumerContext consumerC
 
     public static void Skipped(this IConsumerContext consumerContext)
     {
-        consumerContext.Properties.Add(MessageBusHeaders.Skipped, true);
+        consumerContext.Properties.TryAdd(MessageBusHeaders.Skipped, true);
     }
 
     public static bool WasSkipped(this IConsumerContext consumerContext)

diff --git a/Btms.Consumers/Interceptors/InMemoryConsumerErrorHandler.cs b/Btms.Consumers/Interceptors/InMemoryConsumerErrorHandler.cs
@@ -18,7 +18,7 @@ private async Task<ConsumerErrorHandlerResult> AttemptRetry(T message, IConsumer
 
         if (retryCount > 5)
         {
-            logger.LogError(exception, "Error Consuming Message Retry count {RetryCount} - {Record}", retryCount, message?.ToJson());
+            logger.LogError(exception, "Error Consuming Message Retry count {RetryCount}", retryCount);
             return ConsumerErrorHandlerResult.Failure;
         }
 

diff --git a/Btms.Consumers/NotificationConsumer.cs b/Btms.Consumers/NotificationConsumer.cs
@@ -6,6 +6,7 @@
 using SlimMessageBus;
 using System.Diagnostics.CodeAnalysis;
 using Btms.Consumers.Extensions;
+using Force.DeepCloner;
 using Microsoft.Extensions.Logging;
 
 namespace Btms.Consumers
@@ -34,11 +35,13 @@ public async Task OnHandle(ImportNotification message)
 
 
                 var existingNotification = await dbContext.Notifications.Find(message.ReferenceNumber!);
+                Model.Ipaffs.ImportNotification persistedNotification = null!;
                 if (existingNotification is not null)
                 {
                     if (internalNotification.UpdatedSource.TrimMicroseconds() >
                         existingNotification.UpdatedSource.TrimMicroseconds())
                     {
+                        persistedNotification = existingNotification.DeepClone();
                         internalNotification.AuditEntries = existingNotification.AuditEntries;
                         internalNotification.CreatedSource = existingNotification.CreatedSource;
                         internalNotification.Update(BuildNormalizedIpaffsPath(auditId!), existingNotification);
@@ -48,15 +51,17 @@ public async Task OnHandle(ImportNotification message)
                     {
                         logger.MessageSkipped(Context.GetJobId()!, auditId!, GetType().Name, message.ReferenceNumber!);
                         Context.Skipped();
+                        return;
                     }
                 }
                 else
                 {
                     internalNotification.Create(BuildNormalizedIpaffsPath(auditId!));
                     await dbContext.Notifications.Insert(internalNotification);
+                    persistedNotification = internalNotification!;
                 }
 
-                var linkContext = new ImportNotificationLinkContext(internalNotification, existingNotification);
+                var linkContext = new ImportNotificationLinkContext(persistedNotification, existingNotification);
                 var linkResult = await linkingService.Link(linkContext, Context.CancellationToken);
             }
         }

diff --git a/Btms.Model/Auditing/AuditEntry.cs b/Btms.Model/Auditing/AuditEntry.cs
@@ -149,7 +149,7 @@ internal static AuditDiffEntry Internal(PatchOperation operation)
                         value = operation.Value.GetValue<string>();
                         break;
                     case JsonValueKind.Number:
-                        value = operation.Value.GetValue<long>();
+                        value = operation.Value.GetValue<double>();
                         break;
                     case JsonValueKind.True:
                     case JsonValueKind.False:

diff --git a/Btms.Model/MatchIdentifier.cs b/Btms.Model/MatchIdentifier.cs
@@ -50,4 +50,18 @@ public static MatchIdentifier FromCds(string reference)
 
         return new MatchIdentifier(identifier);
     }
+
+    public static bool TryFromCds(string reference, out MatchIdentifier matchIdentifier)
+    {
+        try
+        {
+            matchIdentifier = MatchIdentifier.FromCds(reference);
+            return true;
+        }
+        catch (Exception)
+        {
+            matchIdentifier = default;
+            return false;
+        }
+    }
 }