From 450bd4e7185b444b96308e1b1c187b8e57107f38 Mon Sep 17 00:00:00 2001 From: Ruairidh MacLeod Date: Mon, 8 Feb 2021 17:46:40 +0000 Subject: [PATCH] Fixup newline parsing in CohortPackager and add test Drops the old ParseToCsvNewLine method since CsvHelper deals with newline strings directly now. --- CHANGELOG.md | 7 ++++-- .../Smi.Common/Options/GlobalOptions.cs | 5 ++++ .../Reporting/JobReporterBase.cs | 17 ++------------ .../Reporting/JobReporterBaseTest.cs | 23 ++++++++++++++++--- 4 files changed, 32 insertions(+), 20 deletions(-) diff --git a/CHANGELOG.md b/CHANGELOG.md index baa835aa5..2058b7229 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -6,10 +6,14 @@ The format is based on [Keep a Changelog](https://keepachangelog.com/en/1.0.0/), ## [Unreleased] -- +### Fixed + +- [#581](https://github.com/SMI/SmiServices/pull/581) Fixed a bug where newlines would never be correctly parsed from the config option in CohortPackager ## [1.14.1] - 2021-02-04 +### Fixed + - [#576](https://github.com/SMI/SmiServices/pull/576) Fixup Windows package build ## [1.14.0] - 2021-02-04 @@ -34,7 +38,6 @@ The format is based on [Keep a Changelog](https://keepachangelog.com/en/1.0.0/), ### Fixed -- Fixed a bug where newlines would never be correctly parsed from the config option in CohortPackager - CohortPackager: Don't try and create the jobId file when recreating an existing report - CohortPackager.Tests: Fix a flaky test caused by NUnit setup/teardown code when running tests in parallel - CohortPackager.Tests: Fix a flaky test caused by using the same MongoDB database name when running tests in parallel diff --git a/src/common/Smi.Common/Options/GlobalOptions.cs b/src/common/Smi.Common/Options/GlobalOptions.cs index b03a151a5..60b7ba8fe 100644 --- a/src/common/Smi.Common/Options/GlobalOptions.cs +++ b/src/common/Smi.Common/Options/GlobalOptions.cs @@ -312,6 +312,11 @@ public class CohortPackagerOptions : IOptions public string ReporterType { get; set; } public string NotifierType { get; set; } public string ReportFormat { get; set; } + + /// + /// The newline to use when writing extraction report files. Note that a "\r\n" string + /// in the YAML config will bee automatically escaped to "\\r\\n" in this string. + /// public string ReportNewLine { get; set; } public override string ToString() diff --git a/src/microservices/Microservices.CohortPackager/Execution/JobProcessing/Reporting/JobReporterBase.cs b/src/microservices/Microservices.CohortPackager/Execution/JobProcessing/Reporting/JobReporterBase.cs index 1b358f614..1b5b9078d 100644 --- a/src/microservices/Microservices.CohortPackager/Execution/JobProcessing/Reporting/JobReporterBase.cs +++ b/src/microservices/Microservices.CohortPackager/Execution/JobProcessing/Reporting/JobReporterBase.cs @@ -50,12 +50,12 @@ [CanBeNull] string reportNewLine else { Logger.Warn($"Not passed a specific newline string for creating reports. Defaulting to Environment.NewLine ('{Regex.Escape(Environment.NewLine)}')"); - ReportNewLine = Environment.NewLine; + ReportNewLine = Regex.Escape(Environment.NewLine); } _csvConfiguration = new CsvConfiguration(CultureInfo.InvariantCulture) { - NewLine = ParseToCsvNewLine(Regex.Escape(ReportNewLine)), + NewLine = ReportNewLine, }; } @@ -459,19 +459,6 @@ private void WriteVerificationValues(IEnumerable - newLine switch - { - @"\r" => "\r", - @"\r\n" => "\r\n", - @"\n" => "\n", - _ => throw new ArgumentException($"No case for '{Regex.Escape(newLine)}'") - }; - protected abstract void ReleaseUnmanagedResources(); public abstract void Dispose(); ~JobReporterBase() => ReleaseUnmanagedResources(); diff --git a/tests/microservices/Microservices.CohortPackager.Tests/Execution/JobProcessing/Reporting/JobReporterBaseTest.cs b/tests/microservices/Microservices.CohortPackager.Tests/Execution/JobProcessing/Reporting/JobReporterBaseTest.cs index 7437667a1..ae24fce83 100644 --- a/tests/microservices/Microservices.CohortPackager.Tests/Execution/JobProcessing/Reporting/JobReporterBaseTest.cs +++ b/tests/microservices/Microservices.CohortPackager.Tests/Execution/JobProcessing/Reporting/JobReporterBaseTest.cs @@ -7,6 +7,8 @@ using System.Collections.Generic; using System.Globalization; using System.IO; +using System.Text.RegularExpressions; +using Smi.Common.Options; namespace Microservices.CohortPackager.Tests.Execution.JobProcessing.Reporting @@ -15,8 +17,8 @@ namespace Microservices.CohortPackager.Tests.Execution.JobProcessing.Reporting [TestFixture] public class JobReporterBaseTest { - private const string WindowsNewLine = "\r\n"; - private const string LinuxNewLine = "\n"; + private const string WindowsNewLine = @"\r\n"; + private const string LinuxNewLine = @"\n"; private static readonly TestDateTimeProvider _dateTimeProvider = new TestDateTimeProvider(); @@ -754,7 +756,22 @@ public void Constructor_NoNewLine_SetToEnvironment() { var mockJobStore = new Mock(MockBehavior.Strict); var reporter = new TestJobReporter(mockJobStore.Object, ReportFormat.Split, null); - Assert.AreEqual(Environment.NewLine, reporter.ReportNewLine); + Assert.AreEqual(Regex.Escape(Environment.NewLine), reporter.ReportNewLine); + } + + [Test] + public void ReportNewLine_LoadFromYaml_EscapesNewlines() + { + string yaml = @" +CurrentDirectory: +CohortPackagerOptions: + ReportNewLine: '\r\n' +"; + string tmpConfig = Path.GetTempFileName() + ".yaml"; + File.WriteAllText(tmpConfig, yaml); + GlobalOptions globals = new GlobalOptionsFactory().Load(tmpConfig); + + Assert.AreEqual(WindowsNewLine, globals.CohortPackagerOptions.ReportNewLine); } }