Merge pull request #704 from SMI/bugfix/703-report-literal-newlines

Bugfix/703 report literal newlines
SMI · Apr 6, 2021 · 1160390 · 1160390
2 parents 9281b58 + c31f077
commit 1160390
Show file tree

Hide file tree

Showing 7 changed files with 40 additions and 16 deletions.
diff --git a/news/704-bugfix.md b/news/704-bugfix.md
@@ -0,0 +1 @@
+Fix ReportNewLine being incorrectly set to a pre-escaped string. Fixes #703
diff --git a/src/microservices/Microservices.CohortPackager/Execution/CohortPackagerHost.cs b/src/microservices/Microservices.CohortPackager/Execution/CohortPackagerHost.cs
@@ -12,6 +12,7 @@
 using Smi.Common.Options;
 using System;
 using System.IO.Abstractions;
+using System.Text.RegularExpressions;
 
 
 namespace Microservices.CohortPackager.Execution
@@ -76,7 +77,7 @@ public CohortPackagerHost(
                     fileSystem ?? new FileSystem(),
                     Globals.FileSystemOptions.ExtractRoot,
                     reportFormatStr,
-                    Globals.CohortPackagerOptions.ReportNewLine
+                    Regex.Unescape(Globals.CohortPackagerOptions.ReportNewLine)
                 );
             }
             else

diff --git a/...ervices/Microservices.CohortPackager/Execution/JobProcessing/Reporting/JobReporterBase.cs b/...ervices/Microservices.CohortPackager/Execution/JobProcessing/Reporting/JobReporterBase.cs
@@ -45,12 +45,17 @@ [CanBeNull] string reportNewLine
             // NOTE(rkm 2020-11-20) IsNullOrWhiteSpace returns true for newline characters!
             if (!string.IsNullOrEmpty(reportNewLine))
             {
+                if (reportNewLine.Contains(@"\"))
+                    throw new ArgumentException("ReportNewLine contained an escaped backslash");
+
                 ReportNewLine = reportNewLine;
             }
             else
             {
+                // NOTE(rkm 2021-04-06) Escape the newline here so it prints correctly...
                 Logger.Warn($"Not passed a specific newline string for creating reports. Defaulting to Environment.NewLine ('{Regex.Escape(Environment.NewLine)}')");
-                ReportNewLine = Regex.Escape(Environment.NewLine);
+                // ... and just use the (unescaped) value as-is
+                ReportNewLine = Environment.NewLine;
             }
 
             _csvConfiguration = new CsvConfiguration(CultureInfo.InvariantCulture)

diff --git a/src/microservices/Microservices.CohortPackager/Program.cs b/src/microservices/Microservices.CohortPackager/Program.cs
@@ -4,14 +4,14 @@
 using Microservices.CohortPackager.Options;
 using MongoDB.Driver;
 using NLog;
-using Smi.Common;
 using Smi.Common.Execution;
 using Smi.Common.MongoDB;
 using Smi.Common.Options;
 using System;
 using System.Collections.Generic;
 using System.IO;
 using System.IO.Abstractions;
+using System.Text.RegularExpressions;
 
 
 namespace Microservices.CohortPackager
@@ -44,14 +44,16 @@ private static int RecreateReport(GlobalOptions globalOptions, CohortPackagerCli
             MongoClient client = MongoClientHelpers.GetMongoClient(mongoDbOptions, globalOptions.HostProcessName);
             var jobStore = new MongoExtractJobStore(client, mongoDbOptions.DatabaseName);
 
+            string newLine = Regex.Unescape(cliOptions.OutputNewLine ?? globalOptions.CohortPackagerOptions.ReportNewLine);
+
             // NOTE(rkm 2020-10-22) Sets the extraction root to the current directory
             IJobReporter reporter = JobReporterFactory.GetReporter(
                 "FileReporter",
                 jobStore,
                 new FileSystem(),
                 Directory.GetCurrentDirectory(),
                 cliOptions.ReportFormat.ToString(),
-                cliOptions.OutputNewLine ?? globalOptions.CohortPackagerOptions.ReportNewLine,
+                newLine,
                 createJobIdFile: false
             );
 

diff --git a/tests/common/Smi.Common.Tests/RequiresMongoDb.cs b/tests/common/Smi.Common.Tests/RequiresMongoDb.cs
@@ -1,4 +1,4 @@
-
+
 using MongoDB.Bson;
 using MongoDB.Driver;
 using NUnit.Framework;
@@ -23,11 +23,17 @@ public void ApplyToContext(TestExecutionContext context)
 
             try
             {
-                IAsyncCursor<BsonDocument> dbs = client.ListDatabases();
+                IAsyncCursor<BsonDocument> _ = client.ListDatabases();
             }
             catch (Exception e)
             {
-                var msg = $"Could not connect to MongoDB at {address}: {e}";
+                string msg =
+                    e is MongoNotPrimaryException
+                    ? "Connected to non-primary MongoDB server. Check replication is enabled"
+                    : $"Could not connect to MongoDB at {address}";
+
+                msg += $": {e}";
+
                 if (FailIfUnavailable)
                     Assert.Fail(msg);
                 else

diff --git a/tests/microservices/Microservices.CohortPackager.Tests/Execution/CohortPackagerHostTest.cs b/tests/microservices/Microservices.CohortPackager.Tests/Execution/CohortPackagerHostTest.cs
@@ -105,21 +105,21 @@ private void VerifyReports(GlobalOptions globals, PathFixtures pf, ReportFormat
                 host.Stop("Test end");
             }
 
-            var firstLine = $"# SMI extraction validation report for testProj1/{pf.ExtractName}";
+            var firstLine = $"# SMI extraction validation report for testProj1/{pf.ExtractName}{globals.CohortPackagerOptions.ReportNewLine}";
             switch (reportFormat)
             {
                 case ReportFormat.Combined:
                     {
-                        string reportContent = File.ReadAllText(Path.Combine(pf.ProjReportsDirAbsolute, $"{pf.ExtractName}_report.txt"));
-                        Assert.True(reportContent.StartsWith(firstLine));
+                        string[] reportContent = File.ReadAllLines(Path.Combine(pf.ProjReportsDirAbsolute, $"{pf.ExtractName}_report.txt"));
+                        Assert.AreEqual(firstLine, reportContent[0]);
                         break;
                     }
                 case ReportFormat.Split:
                     {
                         string extractReportsDirAbsolute = Path.Combine(pf.ProjReportsDirAbsolute, pf.ExtractName);
                         Assert.AreEqual(6, Directory.GetFiles(extractReportsDirAbsolute).Length);
-                        string reportContent = File.ReadAllText(Path.Combine(extractReportsDirAbsolute, "README.md"));
-                        Assert.True(reportContent.StartsWith(firstLine));
+                        string[] reportContent = File.ReadAllLines(Path.Combine(extractReportsDirAbsolute, "README.md"));
+                        Assert.AreEqual(firstLine, reportContent[0]);
                         break;
                     }
                 default:

diff --git a/...croservices.CohortPackager.Tests/Execution/JobProcessing/Reporting/JobReporterBaseTest.cs b/...croservices.CohortPackager.Tests/Execution/JobProcessing/Reporting/JobReporterBaseTest.cs
@@ -17,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();
 
@@ -756,7 +756,7 @@ public void Constructor_NoNewLine_SetToEnvironment()
         {
             var mockJobStore = new Mock<IExtractJobStore>(MockBehavior.Strict);
             var reporter = new TestJobReporter(mockJobStore.Object, ReportFormat.Split, null);
-            Assert.AreEqual(Regex.Escape(Environment.NewLine), reporter.ReportNewLine);
+            Assert.AreEqual(Environment.NewLine, reporter.ReportNewLine);
         }
 
         [Test]
@@ -771,7 +771,16 @@ public void ReportNewLine_LoadFromYaml_EscapesNewlines()
             File.WriteAllText(tmpConfig, yaml);
             GlobalOptions globals = new GlobalOptionsFactory().Load(nameof(ReportNewLine_LoadFromYaml_EscapesNewlines), tmpConfig);
 
-            Assert.AreEqual(WindowsNewLine, globals.CohortPackagerOptions.ReportNewLine);
+            // NOTE(rkm 2021-04-06) Verify we get an *escaped* newline from the YAML load here
+            Assert.AreEqual(Regex.Escape(WindowsNewLine), globals.CohortPackagerOptions.ReportNewLine);
+        }
+
+        [Test]
+        public void ReportNewline_EscapedString_IsDetected()
+        {
+            const string newLine = @"\n";
+            var exc = Assert.Throws<ArgumentException>(() => new TestJobReporter(new Mock<IExtractJobStore>().Object, ReportFormat.Combined, newLine));
+            Assert.AreEqual("ReportNewLine contained an escaped backslash", exc.Message);
         }
     }
Original file line number	Diff line number	Diff line change
		@@ -0,0 +1 @@
		Fix ReportNewLine being incorrectly set to a pre-escaped string. Fixes #703