From c004820c341dfb4a2a62aaab54d764c55dd1ffb2 Mon Sep 17 00:00:00 2001
From: Arvid Ihrig <ihrig@fhi-berlin.mpg.de>
Date: Mon, 2 Jul 2018 13:37:26 +0200
Subject: [PATCH] Integrated Pipeline: ParsingResults now have an explicit
 field for the error message of failed parsing tasks

---
 .../messages/FileParsingSignals.scala         |  4 ++-
 .../CalculationParsingEngine.scala            | 25 +++++++++++-------
 .../integrated_pipeline_tests/Builders.scala  |  7 +++--
 .../CalculationParsingEngineSpec.scala        | 11 +++-----
 .../MessageMatchers.scala                     | 26 +++++++++++++++++++
 .../ParsingResultsProcessorFlowSpec.scala     |  3 ++-
 .../integrated_pipeline_tests/package.scala   |  3 ++-
 7 files changed, 56 insertions(+), 23 deletions(-)

diff --git a/integrated-pipeline/src/main/scala/eu/nomad_lab/integrated_pipeline/messages/FileParsingSignals.scala b/integrated-pipeline/src/main/scala/eu/nomad_lab/integrated_pipeline/messages/FileParsingSignals.scala
index 15aef0f1..ec8d650b 100644
--- a/integrated-pipeline/src/main/scala/eu/nomad_lab/integrated_pipeline/messages/FileParsingSignals.scala
+++ b/integrated-pipeline/src/main/scala/eu/nomad_lab/integrated_pipeline/messages/FileParsingSignals.scala
@@ -57,6 +57,7 @@ sealed trait FileParsingResult extends FileParsingResultSignal {
   val task: FileParsingTask
   val start: Option[StartedParsingSession]
   val end: Option[FinishedParsingSession]
+  val error: Option[String]
 
   val treeTask: FileTreeScanTask = task.treeTask
 }
@@ -66,5 +67,6 @@ case class InMemoryResult(
   result: ParseResult,
   start: Option[StartedParsingSession],
   events: Seq[ParseEvent],
-  end: Option[FinishedParsingSession]
+  end: Option[FinishedParsingSession],
+  error: Option[String]
 ) extends FileParsingResult
diff --git a/integrated-pipeline/src/main/scala/eu/nomad_lab/integrated_pipeline/stream_components/CalculationParsingEngine.scala b/integrated-pipeline/src/main/scala/eu/nomad_lab/integrated_pipeline/stream_components/CalculationParsingEngine.scala
index c30973b6..50e02de2 100644
--- a/integrated-pipeline/src/main/scala/eu/nomad_lab/integrated_pipeline/stream_components/CalculationParsingEngine.scala
+++ b/integrated-pipeline/src/main/scala/eu/nomad_lab/integrated_pipeline/stream_components/CalculationParsingEngine.scala
@@ -3,11 +3,10 @@ package eu.nomad_lab.integrated_pipeline.stream_components
 import java.nio.file.Path
 
 import com.typesafe.scalalogging.StrictLogging
-import eu.nomad_lab.TreeType._
 import eu.nomad_lab.integrated_pipeline.messages.{ FileParsingResult, FileParsingTask, InMemoryResult }
 import eu.nomad_lab.meta.MetaInfoEnv
 import eu.nomad_lab.parsers._
-import org.json4s.JsonAST.{ JArray, JObject, JString }
+import eu.nomad_lab.JsonSupport.formats
 
 import scala.collection.mutable.ListBuffer
 
@@ -67,18 +66,13 @@ class CalculationParsingEngine(parsers: ParserCollection)(implicit metaInfo: Met
    * @return a FileParsingResult representing the failure
    */
   def failParseRequest(request: FileParsingTask, reason: String): FileParsingResult = {
-    val end = FinishedParsingSession(
-      Some(ParseResult.ParseFailure),
-      JArray(List(JString(reason))),
-      Some(request.mainFileUri),
-      JObject(), Map()
-    )
     InMemoryResult(
       task = request,
       result = ParseResult.ParseFailure,
       start = None,
       events = Seq(),
-      end = Some(end)
+      end = None,
+      error = Some(reason)
     )
   }
 
@@ -88,12 +82,23 @@ class CalculationParsingEngine(parsers: ParserCollection)(implicit metaInfo: Met
     val backend = new ParseEventsEmitter(metaInfo, buffer.handleParseEvents,
       buffer.handleStartAndEndParsing)
     val result = SafeParsing.parse(parser, request.mainFileUri, pathToMainFile, backend, request.parserName)
+    val error = result match {
+      case ParseResult.ParseFailure => buffer.endEvent match {
+        case Some(end) => end.parserErrors.children.headOption match {
+          case Some(message) => Some(message.extract[String])
+          case None => Some("no specific error message provided by parser")
+        }
+        case None => Some("parsing did not terminate cleanly")
+      }
+      case _ => None
+    }
     InMemoryResult(
       task = request,
       result = result,
       start = buffer.startEvent,
       events = buffer.events.result(),
-      end = buffer.endEvent
+      end = buffer.endEvent,
+      error = error
     )
   }
 
diff --git a/integrated-pipeline/src/test/scala/eu/nomad_lab/integrated_pipeline_tests/Builders.scala b/integrated-pipeline/src/test/scala/eu/nomad_lab/integrated_pipeline_tests/Builders.scala
index 414f0708..43418faa 100644
--- a/integrated-pipeline/src/test/scala/eu/nomad_lab/integrated_pipeline_tests/Builders.scala
+++ b/integrated-pipeline/src/test/scala/eu/nomad_lab/integrated_pipeline_tests/Builders.scala
@@ -95,7 +95,8 @@ object Builders {
       private val result: ParseResult = ParseResult.ParseSkipped,
       private val start: Option[StartedParsingSession] = None,
       private val events: Seq[ParseEvent] = Seq(),
-      private val end: Option[FinishedParsingSession] = None
+      private val end: Option[FinishedParsingSession] = None,
+      private val error: Option[String] = None
   ) {
 
     def withTask(newTask: FileParsingTask) = copy(task = newTask)
@@ -106,13 +107,15 @@ object Builders {
     def withTreeTask(newTask: FileTreeScanTask) = copy(task = task.copy(treeTask = newTask))
     def withRelativePath(newPath: Path) = copy(task = task.copy(relativePath = newPath))
     def withRelativePath(newPath: String) = copy(task = task.copy(relativePath = Paths.get(newPath)))
+    def withErrorMessage(message: Option[String]) = copy(error = message)
 
     def build() = InMemoryResult(
       task = task,
       result = result,
       start = start,
       events = events,
-      end = end
+      end = end,
+      error = error
     )
   }
 
diff --git a/integrated-pipeline/src/test/scala/eu/nomad_lab/integrated_pipeline_tests/CalculationParsingEngineSpec.scala b/integrated-pipeline/src/test/scala/eu/nomad_lab/integrated_pipeline_tests/CalculationParsingEngineSpec.scala
index cdde43e4..4198a789 100644
--- a/integrated-pipeline/src/test/scala/eu/nomad_lab/integrated_pipeline_tests/CalculationParsingEngineSpec.scala
+++ b/integrated-pipeline/src/test/scala/eu/nomad_lab/integrated_pipeline_tests/CalculationParsingEngineSpec.scala
@@ -2,7 +2,6 @@ package eu.nomad_lab.integrated_pipeline_tests
 
 import java.nio.file.Paths
 
-import eu.nomad_lab.integrated_pipeline.messages.InMemoryResult
 import eu.nomad_lab.integrated_pipeline.stream_components.CalculationParsingEngine
 import eu.nomad_lab.meta.KnownMetaInfoEnvs
 import eu.nomad_lab.parsers.ParseResult.ParseResult
@@ -12,8 +11,8 @@ import org.mockito.ArgumentMatchers.{ eq => raw, _ }
 import org.mockito.Mockito._
 import org.mockito.invocation.InvocationOnMock
 import org.mockito.stubbing.Answer
-import org.scalatest.{ Matchers, WordSpec }
 import org.scalatest.mockito.MockitoSugar
+import org.scalatest.{ Matchers, WordSpec }
 
 class CalculationParsingEngineSpec extends WordSpec with MockitoSugar with TestDataBuilders with Matchers with FileParsingResultMatchers {
 
@@ -95,18 +94,14 @@ class CalculationParsingEngineSpec extends WordSpec with MockitoSugar with TestD
         val f = new Fixture
         f.prepareParserFailureInvocation()
         val result = f.worker.processRequest(sampleParseRequest, unusedPath)
-        result should have(status(ParseResult.ParseFailure))
-        assert(result.end.nonEmpty, "should have a termination event")
-        assert(result.end.get.parserErrors.children.nonEmpty, "should have error messages")
+        result should have(status(ParseResult.ParseFailure), errorMessage("had exception", "just crashed"))
       }
 
       "gracefully fail parsing requests with unknown parsers" in {
         val f = new Fixture
         val anotherRequest = sampleParseRequest.withParserName("nonSenseParser")
         val result = f.worker.processRequest(anotherRequest, unusedPath)
-        result should have(status(ParseResult.ParseFailure))
-        assert(result.end.nonEmpty, "should have a termination event")
-        assert(result.end.get.parserErrors.children.nonEmpty, "should have error messages")
+        result should have(status(ParseResult.ParseFailure), errorMessage("unknown", "nonSenseParser"))
       }
 
       "specify the correct main file to the parser" in {
diff --git a/integrated-pipeline/src/test/scala/eu/nomad_lab/integrated_pipeline_tests/MessageMatchers.scala b/integrated-pipeline/src/test/scala/eu/nomad_lab/integrated_pipeline_tests/MessageMatchers.scala
index a3519ade..364397ed 100644
--- a/integrated-pipeline/src/test/scala/eu/nomad_lab/integrated_pipeline_tests/MessageMatchers.scala
+++ b/integrated-pipeline/src/test/scala/eu/nomad_lab/integrated_pipeline_tests/MessageMatchers.scala
@@ -23,6 +23,24 @@ private object Helpers {
       override def toString(): String = s"$propertyName '$expected'"
     }
   }
+
+  def subStringPropertyMatcher[T](propertyName: String, expected: Seq[String],
+    test: T => String, display: T => String, ignoreCase: Boolean = true): HavePropertyMatcher[T, String] = {
+    new HavePropertyMatcher[T, String] {
+      def apply(element: T): HavePropertyMatchResult[String] = {
+        val message = if (ignoreCase) test(element).toLowerCase else test(element)
+        HavePropertyMatchResult(
+          matches = expected.forall(test => message.contains(
+            if (ignoreCase) test.toLowerCase else test
+          )),
+          propertyName = "error message content",
+          expectedValue = expected.mkString("all of '", "', '", "'"),
+          actualValue = display(element)
+        )
+      }
+      override def toString(): String = s"$propertyName '${expected.mkString("all of '", "', '", "'")}'"
+    }
+  }
 }
 
 trait FileTreeTaskMatchers {
@@ -129,4 +147,12 @@ trait FileParsingResultMatchers {
     }
     )
 
+  def errorMessage(first: String, other: String*): HavePropertyMatcher[FileParsingResult, String] =
+    Helpers.subStringPropertyMatcher(
+      propertyName = "error message content",
+      expected = first +: other,
+      test = (x: FileParsingResult) => x.error.getOrElse(""),
+      display = (x: FileParsingResult) => x.error.toString
+    )
+
 }
\ No newline at end of file
diff --git a/integrated-pipeline/src/test/scala/eu/nomad_lab/integrated_pipeline_tests/ParsingResultsProcessorFlowSpec.scala b/integrated-pipeline/src/test/scala/eu/nomad_lab/integrated_pipeline_tests/ParsingResultsProcessorFlowSpec.scala
index cdfcca45..a1e4c95d 100644
--- a/integrated-pipeline/src/test/scala/eu/nomad_lab/integrated_pipeline_tests/ParsingResultsProcessorFlowSpec.scala
+++ b/integrated-pipeline/src/test/scala/eu/nomad_lab/integrated_pipeline_tests/ParsingResultsProcessorFlowSpec.scala
@@ -56,7 +56,8 @@ class ParsingResultsProcessorFlowSpec extends WordSpec with MockitoSugar {
       result = ParseResult.ParseSuccess,
       start = None,
       events = Seq(),
-      end = None
+      end = None,
+      error = None
     )
 
     val (probeInFiles, probeOutput) = testGraph.run()
diff --git a/integrated-pipeline/src/test/scala/eu/nomad_lab/integrated_pipeline_tests/package.scala b/integrated-pipeline/src/test/scala/eu/nomad_lab/integrated_pipeline_tests/package.scala
index 8c7a6fc9..92f6a32a 100644
--- a/integrated-pipeline/src/test/scala/eu/nomad_lab/integrated_pipeline_tests/package.scala
+++ b/integrated-pipeline/src/test/scala/eu/nomad_lab/integrated_pipeline_tests/package.scala
@@ -105,7 +105,8 @@ package object integrated_pipeline_tests {
         mainFileUri = Some(task.mainFileUri),
         parserInfo = parserInfo,
         parsingStats = Map("foo-stat" -> 42l)
-      ))
+      )),
+      error = None
     )
   }
 
-- 
GitLab