From 74d35d39cf391db00c793c0819da61b4efee0bc5 Mon Sep 17 00:00:00 2001 From: Ruairidh MacLeod Date: Wed, 24 Apr 2024 17:15:02 +0100 Subject: [PATCH 1/3] Revert "catch any exception raised during file classification" This reverts commit 6d12e4da6b1f99508bc6c8e2d99c35cd3101f3d9. --- .../Service/IsIdentifiableQueueConsumer.cs | 4 +-- .../IsIdentifiableQueueConsumerTests.cs | 25 ++++++++++++++++++- 2 files changed, 26 insertions(+), 3 deletions(-) diff --git a/src/microservices/Microservices.IsIdentifiable/Service/IsIdentifiableQueueConsumer.cs b/src/microservices/Microservices.IsIdentifiable/Service/IsIdentifiableQueueConsumer.cs index 9eae37958..eda589036 100644 --- a/src/microservices/Microservices.IsIdentifiable/Service/IsIdentifiableQueueConsumer.cs +++ b/src/microservices/Microservices.IsIdentifiable/Service/IsIdentifiableQueueConsumer.cs @@ -63,9 +63,9 @@ protected override void ProcessMessageImpl(IMessageHeader header, ExtractedFileS { failures = _classifier.Classify(toProcess); } - catch (Exception e) + catch (ArithmeticException ae) { - SendVerificationMessage(statusMessage, header, tag, VerifiedFileStatus.ErrorWontRetry, $"Exception while classifying {statusMessage.GetType().Name}:\n{e}. File could not be scanned."); + SendVerificationMessage(statusMessage, header, tag, VerifiedFileStatus.ErrorWontRetry, $"Exception while classifying {statusMessage.GetType().Name}:\n{ae}"); return; } diff --git a/tests/microservices/Microservices.IsIdentifiable.Tests/Service/IsIdentifiableQueueConsumerTests.cs b/tests/microservices/Microservices.IsIdentifiable.Tests/Service/IsIdentifiableQueueConsumerTests.cs index 6040ddf64..df828ad76 100644 --- a/tests/microservices/Microservices.IsIdentifiable.Tests/Service/IsIdentifiableQueueConsumerTests.cs +++ b/tests/microservices/Microservices.IsIdentifiable.Tests/Service/IsIdentifiableQueueConsumerTests.cs @@ -1,4 +1,4 @@ -using IsIdentifiable.Failures; +using IsIdentifiable.Failures; using IsIdentifiable.Reporting; using Microservices.IsIdentifiable.Service; using Moq; @@ -252,6 +252,29 @@ public void ProcessMessage_ClassifierArithmeticException_SendsErrorWontRetry() Assert.True(_response.Report.StartsWith("Exception while classifying ExtractedFileStatusMessage:\nSystem.ArithmeticException: divide by zero")); } + [Test] + public void ProcessMessage_ClassifierUnhandledException_CallsFatal() + { + // Arrange + + var mockClassifier = new Mock(MockBehavior.Strict); + mockClassifier.Setup(x => x.Classify(It.IsAny())).Throws(new Exception("whee")); + + var consumer = GetNewIsIdentifiableQueueConsumer(null, mockClassifier.Object); + + // Act + + consumer.TestMessage(_extractedFileStatusMessage); + + // Assert + + TestTimelineAwaiter.Await(() => _fatalArgs != null, "Expected Fatal to be called"); + Assert.AreEqual("ProcessMessageImpl threw unhandled exception", _fatalArgs?.Message); + Assert.AreEqual("whee", _fatalArgs.Exception.Message); + Assert.AreEqual(0, consumer.NackCount); + Assert.AreEqual(0, consumer.AckCount); + } + #endregion } } From 73b03a5f015f5735c4124fdb801eb855325c378c Mon Sep 17 00:00:00 2001 From: Ruairidh MacLeod Date: Wed, 24 Apr 2024 17:16:31 +0100 Subject: [PATCH 2/3] fixup nullability from revert --- .../Service/IsIdentifiableQueueConsumerTests.cs | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/tests/microservices/Microservices.IsIdentifiable.Tests/Service/IsIdentifiableQueueConsumerTests.cs b/tests/microservices/Microservices.IsIdentifiable.Tests/Service/IsIdentifiableQueueConsumerTests.cs index df828ad76..118779064 100644 --- a/tests/microservices/Microservices.IsIdentifiable.Tests/Service/IsIdentifiableQueueConsumerTests.cs +++ b/tests/microservices/Microservices.IsIdentifiable.Tests/Service/IsIdentifiableQueueConsumerTests.cs @@ -270,7 +270,7 @@ public void ProcessMessage_ClassifierUnhandledException_CallsFatal() TestTimelineAwaiter.Await(() => _fatalArgs != null, "Expected Fatal to be called"); Assert.AreEqual("ProcessMessageImpl threw unhandled exception", _fatalArgs?.Message); - Assert.AreEqual("whee", _fatalArgs.Exception.Message); + Assert.AreEqual("whee", _fatalArgs?.Exception?.Message); Assert.AreEqual(0, consumer.NackCount); Assert.AreEqual(0, consumer.AckCount); } From a70ba3401a0a10fd06ee99af4cf6dc385cc5361f Mon Sep 17 00:00:00 2001 From: Ruairidh MacLeod Date: Wed, 24 Apr 2024 17:31:34 +0100 Subject: [PATCH 3/3] fix exception handling in IsIdentifiableQueueConsumer This change allows us to catch all exceptions and choose cases we want to pass downstream, crashing on all others. This fixes an issue where IsIdentifiable should have crashed when encountering a corrupt file instead of sending an ErrorWontRetry message downstream. --- .../Service/IsIdentifiableQueueConsumer.cs | 11 ++++++--- .../IsIdentifiableQueueConsumerTests.cs | 23 +++++++++++++++++++ 2 files changed, 31 insertions(+), 3 deletions(-) diff --git a/src/microservices/Microservices.IsIdentifiable/Service/IsIdentifiableQueueConsumer.cs b/src/microservices/Microservices.IsIdentifiable/Service/IsIdentifiableQueueConsumer.cs index eda589036..997ccbf9a 100644 --- a/src/microservices/Microservices.IsIdentifiable/Service/IsIdentifiableQueueConsumer.cs +++ b/src/microservices/Microservices.IsIdentifiable/Service/IsIdentifiableQueueConsumer.cs @@ -63,10 +63,15 @@ protected override void ProcessMessageImpl(IMessageHeader header, ExtractedFileS { failures = _classifier.Classify(toProcess); } - catch (ArithmeticException ae) + catch (Exception e) { - SendVerificationMessage(statusMessage, header, tag, VerifiedFileStatus.ErrorWontRetry, $"Exception while classifying {statusMessage.GetType().Name}:\n{ae}"); - return; + if (e is ArithmeticException) + { + SendVerificationMessage(statusMessage, header, tag, VerifiedFileStatus.ErrorWontRetry, $"Exception while classifying {statusMessage.GetType().Name}:\n{e}"); + return; + } + + throw; } foreach (Failure f in failures) diff --git a/tests/microservices/Microservices.IsIdentifiable.Tests/Service/IsIdentifiableQueueConsumerTests.cs b/tests/microservices/Microservices.IsIdentifiable.Tests/Service/IsIdentifiableQueueConsumerTests.cs index 118779064..caa304a5d 100644 --- a/tests/microservices/Microservices.IsIdentifiable.Tests/Service/IsIdentifiableQueueConsumerTests.cs +++ b/tests/microservices/Microservices.IsIdentifiable.Tests/Service/IsIdentifiableQueueConsumerTests.cs @@ -275,6 +275,29 @@ public void ProcessMessage_ClassifierUnhandledException_CallsFatal() Assert.AreEqual(0, consumer.AckCount); } + [Test] + public void ProcessMessage_ClassifierInvalidFile_CallsFatal() + { + // Arrange + + var mockClassifier = new Mock(MockBehavior.Strict); + mockClassifier.Setup(x => x.Classify(It.IsAny())).Throws(new ApplicationException("File does not contain valid preamble and header")); + + var consumer = GetNewIsIdentifiableQueueConsumer(null, mockClassifier.Object); + + // Act + + consumer.TestMessage(_extractedFileStatusMessage); + + // Assert + + TestTimelineAwaiter.Await(() => _fatalArgs != null, "Expected Fatal to be called"); + Assert.AreEqual("ProcessMessageImpl threw unhandled exception", _fatalArgs?.Message); + Assert.AreEqual("File does not contain valid preamble and header", _fatalArgs?.Exception?.Message); + Assert.AreEqual(0, consumer.NackCount); + Assert.AreEqual(0, consumer.AckCount); + } + #endregion } }