diff --git a/ignifyr-common/pom.xml b/ignifyr-common/pom.xml index a222a844..0c327235 100644 --- a/ignifyr-common/pom.xml +++ b/ignifyr-common/pom.xml @@ -15,6 +15,7 @@ src/main/scala + src/test/scala @@ -51,6 +52,22 @@ org.apache.maven.plugins maven-jar-plugin + + + org.scalatest + scalatest-maven-plugin + + + test + + test + + + io.ignifyr.common + + + + org.apache.maven.plugins @@ -79,5 +96,12 @@ io.onfhir onfhir-definition-commons + + + + org.scalatest + scalatest_${scala.binary.version} + test + diff --git a/ignifyr-common/src/test/scala/io/ignifyr/common/app/AppVersionTest.scala b/ignifyr-common/src/test/scala/io/ignifyr/common/app/AppVersionTest.scala new file mode 100644 index 00000000..646e64f7 --- /dev/null +++ b/ignifyr-common/src/test/scala/io/ignifyr/common/app/AppVersionTest.scala @@ -0,0 +1,17 @@ +package io.ignifyr.common.app + +import org.scalatest.flatspec.AnyFlatSpec +import org.scalatest.matchers.should.Matchers + +/** + * This module deliberately does not ship `version.properties` — the filtered file lives in + * `ignifyr-engine` and `ignifyr-server`. So on this classpath the fallback is the whole behaviour, and + * the fallback is what `/metadata` reports when a distribution forgets to filter the resource. + */ +class AppVersionTest extends AnyFlatSpec with Matchers { + + "getVersion" should "fall back to UNKNOWN when version.properties is not on the classpath" in { + getClass.getClassLoader.getResource("version.properties") shouldBe null + AppVersion.getVersion shouldBe "UNKNOWN" + } +} diff --git a/ignifyr-common/src/test/scala/io/ignifyr/common/util/CustomMappingFunctionsTest.scala b/ignifyr-common/src/test/scala/io/ignifyr/common/util/CustomMappingFunctionsTest.scala new file mode 100644 index 00000000..a8b4802d --- /dev/null +++ b/ignifyr-common/src/test/scala/io/ignifyr/common/util/CustomMappingFunctionsTest.scala @@ -0,0 +1,47 @@ +package io.ignifyr.common.util + +import io.onfhir.path.{FhirPathEvaluator, FhirPathException} +import org.json4s.JsonAST.JNull +import org.scalatest.flatspec.AnyFlatSpec +import org.scalatest.matchers.should.Matchers + +/** + * The `cst:` FHIRPath library. It is wired by class name only + * (`ignifyr.functionLibraries.cst.className`) and that block is **active** in `ignifyr-server`'s + * application.conf, so nothing in Scala references it and nothing else would notice a change here. + */ +class CustomMappingFunctionsTest extends AnyFlatSpec with Matchers { + + private val evaluator = + FhirPathEvaluator + .apply() + .withDefaultFunctionLibraries() + .withFunctionLibrary("cst", new CustomMappingFunctionsFactory()) + + /* + * createTimeSeriesData base64-*encodes* its input and then reads the encoded bytes back pairwise as + * little-endian shorts. For "AB": bytes [65,66] encode to "QUI=" = [81,85,73,61]; the pairs [81,85] + * and [73,61] read little-endian are 81 + (85<<8) = 21841 and 73 + (61<<8) = 15689, each widened to + * Double before being printed. + */ + "cst:createTimeSeriesData" should "decode the encoded bytes pairwise as little-endian shorts" in { + evaluator.evaluateOptionalString("cst:createTimeSeriesData('AB')", JNull) shouldBe Some("21841.0 15689.0") + } + + it should "produce one number per two encoded bytes" in { + // "ABCDEF" -> 8 base64 characters -> 4 pairs -> 4 numbers. + evaluator + .evaluateOptionalString("cst:createTimeSeriesData('ABCDEF')", JNull) + .map(_.split(" ").length) shouldBe Some(4) + } + + it should "be deterministic for the same input" in { + val first = evaluator.evaluateOptionalString("cst:createTimeSeriesData('some payload')", JNull) + evaluator.evaluateOptionalString("cst:createTimeSeriesData('some payload')", JNull) shouldBe first + } + + it should "reject an argument expression that does not return a single string" in { + a[FhirPathException] should be thrownBy + evaluator.evaluateOptionalString("cst:createTimeSeriesData(42)", JNull) + } +} diff --git a/ignifyr-common/src/test/scala/io/ignifyr/common/util/ExceptionUtilTest.scala b/ignifyr-common/src/test/scala/io/ignifyr/common/util/ExceptionUtilTest.scala new file mode 100644 index 00000000..5892d86f --- /dev/null +++ b/ignifyr-common/src/test/scala/io/ignifyr/common/util/ExceptionUtilTest.scala @@ -0,0 +1,45 @@ +package io.ignifyr.common.util + +import org.scalatest.flatspec.AnyFlatSpec +import org.scalatest.matchers.should.Matchers + +/** + * `extractExceptionMessages` is what turns a nested failure into the single `description` a + * `FhirMappingError` carries, so what it drops is what the user never sees in an execution log. + */ +class ExceptionUtilTest extends AnyFlatSpec with Matchers { + + "extractExceptionMessages" should "return the message of an exception with no cause" in { + ExceptionUtil.extractExceptionMessages(new RuntimeException("only message")) shouldBe "only message" + } + + it should "join the whole cause chain, outermost first" in { + val root = new IllegalStateException("root cause") + val middle = new IllegalArgumentException("middle cause", root) + val top = new RuntimeException("top level", middle) + ExceptionUtil.extractExceptionMessages(top) shouldBe "top level\nmiddle cause\nroot cause" + } + + it should "skip a null message in the middle of the chain" in { + val root = new IllegalStateException("root cause") + val middle = new IllegalArgumentException(null: String, root) + val top = new RuntimeException("top level", middle) + ExceptionUtil.extractExceptionMessages(top) shouldBe "top level\nroot cause" + } + + it should "skip an empty message" in { + ExceptionUtil.extractExceptionMessages(new RuntimeException("", new IllegalStateException("root"))) shouldBe "root" + } + + // Note the explicit null messages: the RuntimeException(Throwable) constructor would otherwise set the + // message to the cause's toString, which is exactly the noise this helper is meant to avoid emitting. + it should "return an empty string when nothing in the chain has a message" in { + val cause = new IllegalStateException(null: String) + ExceptionUtil.extractExceptionMessages(new RuntimeException(null: String, cause)) shouldBe "" + } + + it should "keep the cause's toString when it was used as the wrapper's message" in { + val messages = ExceptionUtil.extractExceptionMessages(new RuntimeException(new IllegalStateException("root"))) + messages shouldBe "java.lang.IllegalStateException: root\nroot" + } +} diff --git a/ignifyr-connector-file/src/main/scala/io/ignifyr/connector/file/format/FileFormatRegistry.scala b/ignifyr-connector-file/src/main/scala/io/ignifyr/connector/file/format/FileFormatRegistry.scala index 59ed3e8c..d1d26caa 100644 --- a/ignifyr-connector-file/src/main/scala/io/ignifyr/connector/file/format/FileFormatRegistry.scala +++ b/ignifyr-connector-file/src/main/scala/io/ignifyr/connector/file/format/FileFormatRegistry.scala @@ -38,7 +38,8 @@ object FileFormatRegistry { throw MissingFileFormatException(FileFormatHints.describeSourceFormat(contentType)) ) - private def indexUnique[V](what: String)(entries: Seq[(String, V)]): Map[String, V] = { + /** Visible to the connector package so the fail-fast contract can be asserted without a second classloader. */ + private[file] def indexUnique[V](what: String)(entries: Seq[(String, V)]): Map[String, V] = { val byKey = entries.groupBy(_._1).map { case (contentType, group) => if (group.size > 1) { val owners = group.map(_._2.getClass.getName).mkString(", ") diff --git a/ignifyr-connector-file/src/test/resources/file-data-source-reader-test-data/distinct-test/patients-with-duplicates.csv b/ignifyr-connector-file/src/test/resources/file-data-source-reader-test-data/distinct-test/patients-with-duplicates.csv new file mode 100644 index 00000000..aabf2bc0 --- /dev/null +++ b/ignifyr-connector-file/src/test/resources/file-data-source-reader-test-data/distinct-test/patients-with-duplicates.csv @@ -0,0 +1,6 @@ +pid,gender,homePostalCode +p1,male,G02547 +p2,female,H10564 +p1,male,G02547 +p3,male,V13135 +p2,female,H10564 diff --git a/ignifyr-connector-file/src/test/scala/io/ignifyr/connector/file/FileConnectorExtensionSpec.scala b/ignifyr-connector-file/src/test/scala/io/ignifyr/connector/file/FileConnectorExtensionSpec.scala index d903908a..4abd34e9 100644 --- a/ignifyr-connector-file/src/test/scala/io/ignifyr/connector/file/FileConnectorExtensionSpec.scala +++ b/ignifyr-connector-file/src/test/scala/io/ignifyr/connector/file/FileConnectorExtensionSpec.scala @@ -41,4 +41,26 @@ class FileConnectorExtensionSpec extends AnyFlatSpec with Matchers { val ex = intercept[MissingFileFormatException](FileFormatRegistry.sourceFormat(SourceContentTypes.JSON)) ex.getMessage should include("com.pontegra.ignifyr:ignifyr-format-json") } + + // The counterpart of the missing-format path: a content type claimed by *two* installed handlers. + // `FileConnectorExtension.initialize` force-materializes this registry so it surfaces at startup + // rather than at first read. ServiceLoader input cannot be staged on this classpath, so the guard is + // asserted on the indexing helper directly. + it should "fail fast naming both handlers when two claim the same content type" in { + val csv = FileFormatRegistry.sourceFormat(SourceContentTypes.CSV) + val parquet = FileFormatRegistry.sourceFormat(SourceContentTypes.PARQUET) + val ex = intercept[IllegalStateException] { + FileFormatRegistry.indexUnique("file source format")(Seq("csv" -> csv, "csv" -> parquet)) + } + ex.getMessage should include("Duplicate file source format registration") + ex.getMessage should include("csv") + ex.getMessage should (include(csv.getClass.getName) and include(parquet.getClass.getName)) + } + + it should "index one handler per content type" in { + val csv = FileFormatRegistry.sourceFormat(SourceContentTypes.CSV) + val parquet = FileFormatRegistry.sourceFormat(SourceContentTypes.PARQUET) + FileFormatRegistry.indexUnique("file source format")(Seq("a" -> csv, "b" -> parquet)) shouldBe + Map("a" -> csv, "b" -> parquet) + } } diff --git a/ignifyr-connector-file/src/test/scala/io/ignifyr/connector/file/FileDataSourceReaderOptionsTest.scala b/ignifyr-connector-file/src/test/scala/io/ignifyr/connector/file/FileDataSourceReaderOptionsTest.scala new file mode 100644 index 00000000..04c7389b --- /dev/null +++ b/ignifyr-connector-file/src/test/scala/io/ignifyr/connector/file/FileDataSourceReaderOptionsTest.scala @@ -0,0 +1,106 @@ +package io.ignifyr.connector.file + +import io.ignifyr.engine.config.IgnifyrConfig +import io.ignifyr.engine.model.{FileSystemSource, FileSystemSourceSettings, SourceContentTypes} +import org.apache.spark.sql.SparkSession +import org.scalatest.flatspec.AnyFlatSpec +import org.scalatest.matchers.should.Matchers + +import java.nio.file.Paths + +/** + * The two cross-cutting concerns `FileDataSourceReader` applies around the format handler, neither of + * which any other suite reaches: the `distinct` read option, and how a source path is resolved. + * + * Path resolution has a history worth guarding — an earlier `hdfs://` special case further down the + * write side silently turned parquet output into text — so what is pinned here is the one thing the read + * side branches on: an `hdfs://` data folder is handed through verbatim, while every other path is + * resolved against the workspace folder. + */ +class FileDataSourceReaderOptionsTest extends AnyFlatSpec with Matchers { + + private val sparkSession: SparkSession = IgnifyrConfig.sparkSession + private val reader = new FileDataSourceReader(sparkSession) + + private val testDataFolderPath: String = + Paths.get(getClass.getResource("/file-data-source-reader-test-data").toURI).toAbsolutePath.toString + + private def readDistinctTest(options: Map[String, String]) = + reader.read( + mappingSourceBinding = FileSystemSource( + path = "patients-with-duplicates.csv", + contentType = SourceContentTypes.CSV, + options = options + ), + mappingJobSourceSettings = FileSystemSourceSettings( + name = "test", + sourceUri = "urn:test", + dataFolderPath = s"$testDataFolderPath/distinct-test" + ), + schema = None + ) + + // The fixture holds 5 rows, of which 2 are exact repeats of earlier ones. + "the distinct option" should "drop repeated rows when it is set" in { + readDistinctTest(Map("distinct" -> "true")).count() shouldBe 3 + } + + it should "keep every row when it is absent" in { + readDistinctTest(Map.empty).count() shouldBe 5 + } + + // Only the exact string "true" enables it; anything else reads the file unchanged. + it should "keep every row for any value other than true" in { + readDistinctTest(Map("distinct" -> "false")).count() shouldBe 5 + readDistinctTest(Map("distinct" -> "yes")).count() shouldBe 5 + } + + /* + * Path resolution is asserted through the streaming directory check, which reports the *resolved* + * path: it runs straight after resolution and needs no Hadoop filesystem, so the branch can be pinned + * without an HDFS cluster. + */ + "path resolution" should "hand an hdfs:// data folder through without prefixing the workspace folder" in { + val thrown = the[IllegalArgumentException] thrownBy reader.read( + mappingSourceBinding = FileSystemSource(path = "patients", contentType = SourceContentTypes.CSV), + mappingJobSourceSettings = FileSystemSourceSettings( + name = "test", + sourceUri = "urn:test", + dataFolderPath = "hdfs://namenode:8020/data", + asStream = true + ), + schema = None + ) + thrown.getMessage should startWith("hdfs://namenode:8020/data/patients") + } + + it should "join an hdfs:// folder and path with exactly one separator" in { + val thrown = the[IllegalArgumentException] thrownBy reader.read( + mappingSourceBinding = FileSystemSource(path = "/patients", contentType = SourceContentTypes.CSV), + mappingJobSourceSettings = FileSystemSourceSettings( + name = "test", + sourceUri = "urn:test", + dataFolderPath = "hdfs://namenode:8020/data/", + asStream = true + ), + schema = None + ) + thrown.getMessage should startWith("hdfs://namenode:8020/data/patients") + } + + it should "resolve a non-hdfs data folder to an absolute local path" in { + val thrown = the[IllegalArgumentException] thrownBy reader.read( + mappingSourceBinding = FileSystemSource(path = "patients.csv", contentType = SourceContentTypes.CSV), + mappingJobSourceSettings = FileSystemSourceSettings( + name = "test", + sourceUri = "urn:test", + dataFolderPath = s"$testDataFolderPath/single-file-test", + asStream = true + ), + schema = None + ) + thrown.getMessage should not startWith "hdfs://" + thrown.getMessage should include("patients.csv") + thrown.getMessage should include("is not a directory") + } +} diff --git a/ignifyr-connector-kafka/src/main/scala/io/ignifyr/connector/kafka/KafkaSourceReader.scala b/ignifyr-connector-kafka/src/main/scala/io/ignifyr/connector/kafka/KafkaSourceReader.scala index c6ac2993..ba03105f 100644 --- a/ignifyr-connector-kafka/src/main/scala/io/ignifyr/connector/kafka/KafkaSourceReader.scala +++ b/ignifyr-connector-kafka/src/main/scala/io/ignifyr/connector/kafka/KafkaSourceReader.scala @@ -42,108 +42,12 @@ class KafkaSourceReader(spark: SparkSession) extends BaseDataSourceReader[KafkaS throw new IllegalArgumentException("Schema is required for streaming source") } + // Capture the schema itself, not this reader: the UDF is shipped to the executors, and closing over + // an instance method would drag the SparkSession along with it. + val sourceSchema = schema.get // user-defined function (UDF) to process message (json) coming from the Kafka topic - val processDataUDF = udf((message: String) => { - val json: Resource = - try { // try-catch block needed to handle unparseable json - message.parseJson - } catch { - case e: JsonParseException => - throw new InternalServerErrorException("Kafka message is an unparseable JSON", e) - } + val processDataUDF = udf((message: String) => KafkaSourceReader.coerceMessageToSchema(message, sourceSchema)) - // process each field so that string values are acceptable (if they are parseable of course) for some data types such as double and integer - json - .mapField(field => { - schema.get.fields - .find(p => p.name.contentEquals(field._1)) - .map(fieldType => { // get field type from the schema - fieldType.dataType match { - case _: DoubleType => - try { - field._1 -> { - // performs the following conversions: - // JString(v) => JDouble(v) - // JDouble(v) => JDouble(v) - // JString() => JNull - field._2.extract[String] match { - case str if str.nonEmpty => JDouble(str.toDouble) - case _ => JNull - } - } - } catch { - case e: NumberFormatException => - throw new InternalServerErrorException(s"${field._2} is not a parsable `Double`", e) - } - case _: IntegerType => - try { - field._1 -> { - // performs the following conversions: - // JString(v) => JInt(v) - // JInt(v) => JInt(v) - // JString() => JNull - field._2.extract[String] match { - case str if str.nonEmpty => JInt(str.toInt) - case _ => JNull - } - } - } catch { - case e: NumberFormatException => - throw new InternalServerErrorException(s"${field._2} is not a parsable `Integer`", e) - } - case _: LongType => - try { - field._1 -> { - // performs the following conversions: - // JString(v) => JLong(v) - // JLong(v) => JLong(v) - // JString() => JNull - field._2.extract[String] match { - case str if str.nonEmpty => JLong(str.toLong) - case _ => JNull - } - } - } catch { - case e: NumberFormatException => - throw new InternalServerErrorException(s"${field._2} is not a parsable `Long`", e) - } - case _: BooleanType => - try { - field._1 -> { - // performs the following conversions: - // JString(v) => JBool(v) - // JString("0") => JBool(false) - // JString("1") => JBool(true) - // JBool(v) => JBool(v) - // JString() => JNull - val bool = field._2.extractOpt[Boolean] // try to extract as boolean - bool match { - // matches JBool(v) - case Some(value) => JBool(value) - // try to extract as string - case None => - field._2.extractOpt[String] match { - // matches JString("0") or matches JString("1") - case Some(value) if value.contentEquals("0") || value.contentEquals("1") => - JBool(if (value.contentEquals("0")) false else true) - // matches JString(v) - case Some(value) if value.nonEmpty => JBool(value.toBoolean) - // matches JString() - case _ => JNull - } - } - } - } catch { - case e: IllegalArgumentException => - throw new InternalServerErrorException(s"${field._2} is not a parsable `Boolean`", e) - } - case _ => field - } - }) - .getOrElse(field) - }) - .toJson - }) // Determine whether to use batch or streaming read mode based on the 'asStream' setting if (mappingJobSourceSettings.asStream) { spark.readStream // Use streaming mode for continuous ingestion of new messages from Kafka @@ -155,7 +59,7 @@ class KafkaSourceReader(spark: SparkSession) extends BaseDataSourceReader[KafkaS .load() .select($"value".cast(StringType)) // change the type of message from binary to string .withColumn("value", processDataUDF(col("value"))) // replace 'value' column with the processed data - .select(from_json($"value", schema.get).as("record")) + .select(from_json($"value", sourceSchema).as("record")) .select("record.*") } else { // Filter out the 'startingOffsets' option as it always supposed to be "earliest" for batch kafka reads @@ -169,8 +73,127 @@ class KafkaSourceReader(spark: SparkSession) extends BaseDataSourceReader[KafkaS .load() .select($"value".cast(StringType)) // change the type of message from binary to string .withColumn("value", processDataUDF(col("value"))) // replace 'value' column with the processed data - .select(from_json($"value", schema.get).as("record")) + .select(from_json($"value", sourceSchema).as("record")) .select("record.*") } } } + +object KafkaSourceReader { + + /** + * Coerces a Kafka message (JSON) so that its values are acceptable for the schema's data types. REDCap + * and similar producers send every value as a string, so a field typed `double`/`integer`/`long`/ + * `boolean` in the schema has to be converted before `from_json` would accept it; an empty string means + * "no value" and becomes null. A value that cannot be converted fails the read rather than reaching the + * mapping as null. + * + * Lives on the companion object so the UDF that calls it carries no reference to the reader instance + * (and therefore none to the SparkSession) when it is serialized to the executors. + * + * @param message the raw Kafka message + * @param schema the schema of the source data + * @return the message with its values coerced to the schema's data types + */ + private[kafka] def coerceMessageToSchema(message: String, schema: StructType): String = { + val json: Resource = + try { // try-catch block needed to handle unparseable json + message.parseJson + } catch { + case e: JsonParseException => + throw new InternalServerErrorException("Kafka message is an unparseable JSON", e) + } + + // process each field so that string values are acceptable (if they are parseable of course) for some data types such as double and integer + json + .mapField(field => { + schema.fields + .find(p => p.name.contentEquals(field._1)) + .map(fieldType => { // get field type from the schema + fieldType.dataType match { + case _: DoubleType => + try { + field._1 -> { + // performs the following conversions: + // JString(v) => JDouble(v) + // JDouble(v) => JDouble(v) + // JString() => JNull + field._2.extract[String] match { + case str if str.nonEmpty => JDouble(str.toDouble) + case _ => JNull + } + } + } catch { + case e: NumberFormatException => + throw new InternalServerErrorException(s"${field._2} is not a parsable `Double`", e) + } + case _: IntegerType => + try { + field._1 -> { + // performs the following conversions: + // JString(v) => JInt(v) + // JInt(v) => JInt(v) + // JString() => JNull + field._2.extract[String] match { + case str if str.nonEmpty => JInt(str.toInt) + case _ => JNull + } + } + } catch { + case e: NumberFormatException => + throw new InternalServerErrorException(s"${field._2} is not a parsable `Integer`", e) + } + case _: LongType => + try { + field._1 -> { + // performs the following conversions: + // JString(v) => JLong(v) + // JLong(v) => JLong(v) + // JString() => JNull + field._2.extract[String] match { + case str if str.nonEmpty => JLong(str.toLong) + case _ => JNull + } + } + } catch { + case e: NumberFormatException => + throw new InternalServerErrorException(s"${field._2} is not a parsable `Long`", e) + } + case _: BooleanType => + try { + field._1 -> { + // performs the following conversions: + // JString(v) => JBool(v) + // JString("0") => JBool(false) + // JString("1") => JBool(true) + // JBool(v) => JBool(v) + // JString() => JNull + val bool = field._2.extractOpt[Boolean] // try to extract as boolean + bool match { + // matches JBool(v) + case Some(value) => JBool(value) + // try to extract as string + case None => + field._2.extractOpt[String] match { + // matches JString("0") or matches JString("1") + case Some(value) if value.contentEquals("0") || value.contentEquals("1") => + JBool(if (value.contentEquals("0")) false else true) + // matches JString(v) + case Some(value) if value.nonEmpty => JBool(value.toBoolean) + // matches JString() + case _ => JNull + } + } + } + } catch { + case e: IllegalArgumentException => + throw new InternalServerErrorException(s"${field._2} is not a parsable `Boolean`", e) + } + case _ => field + } + }) + .getOrElse(field) + }) + .toJson + } +} diff --git a/ignifyr-connector-kafka/src/test/scala/io/ignifyr/connector/kafka/KafkaSourceReaderTest.scala b/ignifyr-connector-kafka/src/test/scala/io/ignifyr/connector/kafka/KafkaSourceReaderTest.scala new file mode 100644 index 00000000..95effaa7 --- /dev/null +++ b/ignifyr-connector-kafka/src/test/scala/io/ignifyr/connector/kafka/KafkaSourceReaderTest.scala @@ -0,0 +1,110 @@ +package io.ignifyr.connector.kafka + +import org.apache.spark.sql.types._ +import org.json4s.jackson.JsonMethods +import org.scalatest.flatspec.AnyFlatSpec +import org.scalatest.matchers.should.Matchers + +import javax.ws.rs.InternalServerErrorException + +/** + * Kafka producers (REDCap among them) send every value as a JSON string, so the reader coerces each value + * to the type its schema declares before `from_json` sees it. An uncoerced value would be dropped as null + * by `from_json` instead of failing, which is why the conversion rules — and specifically which inputs are + * an error rather than a null — are worth pinning. The end-to-end read is covered by the long-tier + * `KafkaStreamingRedcapTest`. + */ +class KafkaSourceReaderTest extends AnyFlatSpec with Matchers { + + private def schemaOf(fieldType: DataType): StructType = StructType(Seq(StructField("field", fieldType))) + + private def coerce(json: String, fieldType: DataType): String = + KafkaSourceReader.coerceMessageToSchema(json, schemaOf(fieldType)) + + /** The coerced value of the single `field`, rendered back as compact JSON. */ + private def valueOf(json: String, fieldType: DataType): String = + JsonMethods.compact(JsonMethods.render(JsonMethods.parse(coerce(json, fieldType)) \ "field")) + + "coerceMessageToSchema" should "convert a stringified number to a double" in { + valueOf("""{"field":"1.5"}""", DoubleType) shouldBe "1.5" + } + + it should "leave an already numeric double alone" in { + valueOf("""{"field":1.5}""", DoubleType) shouldBe "1.5" + } + + it should "convert a stringified integer and long" in { + valueOf("""{"field":"42"}""", IntegerType) shouldBe "42" + valueOf("""{"field":"9999999999"}""", LongType) shouldBe "9999999999" + } + + // An empty string is REDCap's "not answered", and it has to become null rather than a parse failure. + it should "turn an empty string into null for every numeric type" in { + Seq[DataType](DoubleType, IntegerType, LongType).foreach { fieldType => + valueOf("""{"field":""}""", fieldType) shouldBe "null" + } + } + + it should "turn an empty string into null for a boolean" in { + valueOf("""{"field":""}""", BooleanType) shouldBe "null" + } + + it should "accept a real boolean unchanged" in { + valueOf("""{"field":true}""", BooleanType) shouldBe "true" + valueOf("""{"field":false}""", BooleanType) shouldBe "false" + } + + // REDCap encodes yes/no answers as "1"/"0", which `toBoolean` alone would reject. + it should "read the 1 and 0 strings as booleans" in { + valueOf("""{"field":"1"}""", BooleanType) shouldBe "true" + valueOf("""{"field":"0"}""", BooleanType) shouldBe "false" + } + + it should "read the true and false strings as booleans" in { + valueOf("""{"field":"true"}""", BooleanType) shouldBe "true" + valueOf("""{"field":"false"}""", BooleanType) shouldBe "false" + } + + it should "leave a field whose schema type needs no coercion untouched" in { + valueOf("""{"field":"plain text"}""", StringType) shouldBe "\"plain text\"" + } + + it should "leave a field that the schema does not declare untouched" in { + val coerced = KafkaSourceReader.coerceMessageToSchema("""{"known":"1","unknown":"1"}""", schemaOf(BooleanType)) + coerced should include("\"unknown\":\"1\"") + } + + it should "fail on a value that cannot be converted to the declared numeric type" in { + a[InternalServerErrorException] should be thrownBy coerce("""{"field":"not a number"}""", DoubleType) + a[InternalServerErrorException] should be thrownBy coerce("""{"field":"not a number"}""", IntegerType) + a[InternalServerErrorException] should be thrownBy coerce("""{"field":"not a number"}""", LongType) + } + + it should "fail on a value that cannot be converted to a boolean" in { + a[InternalServerErrorException] should be thrownBy coerce("""{"field":"maybe"}""", BooleanType) + } + + it should "fail on an unparseable message rather than passing it downstream" in { + val thrown = the[InternalServerErrorException] thrownBy coerce("{not json", StringType) + thrown.getMessage should include("unparseable JSON") + } + + it should "coerce every declared field of a multi-field message" in { + val schema = StructType( + Seq( + StructField("age", IntegerType), + StructField("weight", DoubleType), + StructField("consent", BooleanType), + StructField("name", StringType) + ) + ) + val coerced = + JsonMethods.parse( + KafkaSourceReader.coerceMessageToSchema("""{"age":"7","weight":"12.5","consent":"1","name":"p1"}""", schema) + ) + (coerced \ "age").values shouldBe 7 + (coerced \ "weight").values shouldBe 12.5 + (coerced \ "consent").values shouldBe true + (coerced \ "name").values shouldBe "p1" + } +} diff --git a/ignifyr-connector-sql/src/test/scala/io/ignifyr/integrationtest/SqlSourceTest.scala b/ignifyr-connector-sql/src/test/scala/io/ignifyr/integrationtest/SqlSourceTest.scala index c9eefd7c..6ad19371 100644 --- a/ignifyr-connector-sql/src/test/scala/io/ignifyr/integrationtest/SqlSourceTest.scala +++ b/ignifyr-connector-sql/src/test/scala/io/ignifyr/integrationtest/SqlSourceTest.scala @@ -214,13 +214,34 @@ class SqlSourceTest extends AsyncFlatSpec with BeforeAndAfterAll with IgnifyrTes .where("subject", "Patient/" + FhirMappingUtility.getHashedId("Patient", "p" + i)) .executeAndReturnBundle() }) + // This mapping emits MedicationAdministrations alongside the Observations. Those used to be + // rejected by the R5 server while the test still passed, because it only checked that the + // cleanup delete answered 200 — so assert both kinds actually landed before deleting them. + val medSearchFutures = (1 to 10).map(i => + onFhirClient + .search("MedicationAdministration") + .where("subject", "Patient/" + FhirMappingUtility.getHashedId("Patient", "p" + i)) + .executeAndReturnBundle() + ) Future.sequence(obsSearchFutures) flatMap { obsBundleList => - obsBundleList.foreach(observationBundle => { - observationBundle.searchResults - .foreach(obs => batchRequest = batchRequest.entry(_.delete("Observation", (obs \ "id").extract[String]))) - }) - batchRequest.returnMinimal().asInstanceOf[FhirBatchTransactionRequestBuilder].execute() map { res => - res.httpStatus shouldBe StatusCodes.OK + Future.sequence(medSearchFutures) flatMap { medBundleList => + obsBundleList.flatMap(_.searchResults) should not be empty + medBundleList.flatMap(_.searchResults) should not be empty + + obsBundleList.foreach(observationBundle => { + observationBundle.searchResults + .foreach(obs => + batchRequest = batchRequest.entry(_.delete("Observation", (obs \ "id").extract[String])) + ) + }) + medBundleList.foreach(medicationBundle => { + medicationBundle.searchResults.foreach(med => + batchRequest = batchRequest.entry(_.delete("MedicationAdministration", (med \ "id").extract[String])) + ) + }) + batchRequest.returnMinimal().asInstanceOf[FhirBatchTransactionRequestBuilder].execute() map { res => + res.httpStatus shouldBe StatusCodes.OK + } } } }) @@ -245,7 +266,8 @@ class SqlSourceTest extends AsyncFlatSpec with BeforeAndAfterAll with IgnifyrTes (organization1 \ "name").extract[String] shouldBe "Example care site name" (((organization1 \ "type").extract[Seq[JObject]].head \ "coding").extract[Seq[JObject]].head \ "code") .extract[String] shouldBe "21" - ((organization1 \ "address").extract[Seq[JObject]].head \ "state").extract[String] shouldBe "MO" + // R5 moved Organization.address under contact (ExtendedContactDetail). + (((organization1 \ "contact").extract[Seq[JObject]].head \ "address") \ "state").extract[String] shouldBe "MO" } } @@ -258,15 +280,30 @@ class SqlSourceTest extends AsyncFlatSpec with BeforeAndAfterAll with IgnifyrTes sinkSettings = fhirSinkSettings ) .flatMap(_ => { - // Delete care sites - var batchRequest: FhirBatchTransactionRequestBuilder = onFhirClient.batch() - (1 to 2).foreach { i => - batchRequest = - batchRequest.entry(_.delete("Organization", FhirMappingUtility.getHashedId("Organization", i.toString))) - } - batchRequest.returnMinimal().asInstanceOf[FhirBatchTransactionRequestBuilder].execute() map { res => - res.httpStatus shouldBe StatusCodes.OK - } + // Read the written resources back before deleting them. Without this the test asserted only that + // the cleanup delete answered 200, which it does whether or not anything was ever written — and + // for a while nothing was, because the mapping emitted an R4-shaped Organization the R5 server + // rejected. + Future + .sequence((1 to 2).map { i => + onFhirClient + .read("Organization", FhirMappingUtility.getHashedId("Organization", i.toString)) + .executeAndReturnResource() + }) + .flatMap { organizations => + organizations.size shouldBe 2 + organizations.foreach(FHIRUtil.extractResourceType(_) shouldBe "Organization") + + // Delete care sites + var batchRequest: FhirBatchTransactionRequestBuilder = onFhirClient.batch() + (1 to 2).foreach { i => + batchRequest = + batchRequest.entry(_.delete("Organization", FhirMappingUtility.getHashedId("Organization", i.toString))) + } + batchRequest.returnMinimal().asInstanceOf[FhirBatchTransactionRequestBuilder].execute() map { res => + res.httpStatus shouldBe StatusCodes.OK + } + } }) } @@ -334,7 +371,9 @@ class SqlSourceTest extends AsyncFlatSpec with BeforeAndAfterAll with IgnifyrTes .extract[String] shouldBe FhirMappingUtility.getHashedReference("Encounter", "43483680") ((procedureOccurrence \ "performer").extract[Seq[JObject]].head \ "actor" \ "reference") .extract[String] shouldBe FhirMappingUtility.getHashedReference("Practitioner", "48878") - (procedureOccurrence \ "performedDateTime").extract[String] shouldBe "2010-04-25" + // R5 renamed Procedure.performed[x] to occurrence[x] -- with two r's, unlike + // MedicationAdministration.occurence[x], which the spec spells with one. + (procedureOccurrence \ "occurrenceDateTime").extract[String] shouldBe "2010-04-25" } } @@ -359,6 +398,60 @@ class SqlSourceTest extends AsyncFlatSpec with BeforeAndAfterAll with IgnifyrTes }) } + /* + * The orchestration half of the batching strategy: one execution per entry of `batchParameterSets`, + * run sequentially, with the entry's values substituted into the task's `preprocessSql`. The + * substitution itself is unit-tested in the engine (`FhirMappingTaskTest`); what needs a real source + * and sink is that *every* set runs and their outputs accumulate — a fold that kept only the last + * result, or stopped after the first, would still produce a green job and silently drop data. + * + * The `patients` fixture holds five male and five female rows, so batching by gender writes all ten + * only if both batches executed. + */ + "Batched patient mapping" should "run the mapping once per batch parameter set" in { + val batchedPatientMappingTask: FhirMappingTask = FhirMappingTask( + name = "patient-sql-mapping", + mappingRef = "https://aiccelerate.eu/fhir/mappings/patient-sql-mapping", + sourceBinding = Map( + "source" -> SqlSource( + tableName = Some("patients"), + preprocessSql = Some("SELECT * FROM source WHERE gender = '$gender'") + ) + ), + batchingStrategy = Some(BatchingStrategy(Seq(Map("gender" -> "male"), Map("gender" -> "female")))) + ) + + fhirMappingJobManager + .executeMappingJob( + mappingJobExecution = + FhirMappingJobExecution(mappingTasks = Seq(batchedPatientMappingTask), job = fhirMappingJob), + sourceSettings = sqlSourceSettings, + sinkSettings = fhirSinkSettings + ) + .flatMap { _ => + // p1 can only come from the "male" batch and p8 only from the "female" one. + val fromFirstBatch = + onFhirClient.read("Patient", FhirMappingUtility.getHashedId("Patient", "p1")).executeAndReturnResource() + val fromLastBatch = + onFhirClient.read("Patient", FhirMappingUtility.getHashedId("Patient", "p8")).executeAndReturnResource() + + for { + male <- fromFirstBatch + female <- fromLastBatch + cleanup <- { + FHIRUtil.extractValue[String](male, "gender") shouldBe "male" + FHIRUtil.extractValue[String](female, "gender") shouldBe "female" + var batchRequest: FhirBatchTransactionRequestBuilder = onFhirClient.batch() + (1 to 10).foreach { i => + batchRequest = + batchRequest.entry(_.delete("Patient", FhirMappingUtility.getHashedId("Patient", "p" + i.toString))) + } + batchRequest.returnMinimal().asInstanceOf[FhirBatchTransactionRequestBuilder].execute() + } + } yield cleanup.httpStatus shouldBe StatusCodes.OK + } + } + it should "execute the FhirMappingJob with SQL source and sink settings restored from a file" in { val lMappingJob = FhirMappingJobFormatter.readMappingJobFromFile(testSqlMappingJobFilePath) diff --git a/ignifyr-engine/src/main/scala/io/ignifyr/engine/cli/CommandLineInterface.scala b/ignifyr-engine/src/main/scala/io/ignifyr/engine/cli/CommandLineInterface.scala index c9bbc546..79fd1a64 100644 --- a/ignifyr-engine/src/main/scala/io/ignifyr/engine/cli/CommandLineInterface.scala +++ b/ignifyr-engine/src/main/scala/io/ignifyr/engine/cli/CommandLineInterface.scala @@ -121,7 +121,10 @@ object CommandLineInterface { // Generic `--flag value` pair; command providers translate these into positional args. nextArg(map ++ Map(flag.stripPrefix("--") -> value), tail) case str :: tail => - nextArg(map ++ Map("command" -> str), tail) + // The first bare token is the command. A later bare token must not overwrite it — otherwise a + // trailing `--flag` with no value (e.g. `run --job`) is consumed here and reported as an + // unknown command instead of a missing option value. + nextArg(if (map.contains("command")) map else map + ("command" -> str), tail) } } diff --git a/ignifyr-engine/src/main/scala/io/ignifyr/engine/env/EnvironmentVariableResolver.scala b/ignifyr-engine/src/main/scala/io/ignifyr/engine/env/EnvironmentVariableResolver.scala index d550a020..e29d017a 100644 --- a/ignifyr-engine/src/main/scala/io/ignifyr/engine/env/EnvironmentVariableResolver.scala +++ b/ignifyr-engine/src/main/scala/io/ignifyr/engine/env/EnvironmentVariableResolver.scala @@ -37,10 +37,16 @@ object EnvironmentVariableResolver { * @param fileContent The file content potentially containing placeholders for environment variables. * @return The file content with all recognized environment variables resolved. */ - def resolveFileContent(fileContent: String): String = { + def resolveFileContent(fileContent: String): String = resolveFileContent(fileContent, sys.env) + + /** + * Same as [[resolveFileContent]] against an explicit environment. `sys.env` is fixed for the lifetime + * of the JVM, so this is the seam through which the substitution itself can be exercised. + */ + private[ignifyr] def resolveFileContent(fileContent: String, env: Map[String, String]): String = { EnvironmentVariable.values.foldLeft(fileContent) { (updatedContent, envVar) => val placeholder = "\\$\\{" + envVar.toString + "\\}" - sys.env.get(envVar.toString) match { + env.get(envVar.toString) match { case Some(envValue) => updatedContent.replaceAll(placeholder, envValue) case None => diff --git a/ignifyr-engine/src/main/scala/io/ignifyr/engine/execution/processing/FileStreamInputArchiver.scala b/ignifyr-engine/src/main/scala/io/ignifyr/engine/execution/processing/FileStreamInputArchiver.scala index 264b584a..66c39590 100644 --- a/ignifyr-engine/src/main/scala/io/ignifyr/engine/execution/processing/FileStreamInputArchiver.scala +++ b/ignifyr-engine/src/main/scala/io/ignifyr/engine/execution/processing/FileStreamInputArchiver.scala @@ -55,8 +55,11 @@ class FileStreamInputArchiver(runningJobRegistry: RunningJobRegistry) { // Get the sources file directory for this execution val sourcesDirectory: String = taskExecution.getSourceDirectory(mappingTaskName) - // There won't be any file (with name as an integer) during the initialization or after checkpoints are cleared - if (commitDirectory.listFiles().exists(file => file.isFile && !file.getName.contains("."))) { + // There won't be any file (with name as an integer) during the initialization or after checkpoints are cleared. + // `listFiles()` returns null when the directory itself is absent, which is the normal state until Spark + // writes its first commit and again right after `clearCheckpoints` deletes it. This runs inside the shared + // archiving TimerTask, so letting that NPE escape would kill the timer thread and stop archiving for every job. + if (Option(commitDirectory.listFiles()).exists(_.exists(file => file.isFile && !file.getName.contains(".")))) { // Apply archiving for the files as of the last processed offset until the last unprocessed offset val lastProcessedOffset: Int = processedOffsets.getOrElseUpdate(getOffsetKey(taskExecution.id, mappingTaskName), -1) diff --git a/ignifyr-engine/src/main/scala/io/ignifyr/engine/model/FhirMappingTask.scala b/ignifyr-engine/src/main/scala/io/ignifyr/engine/model/FhirMappingTask.scala index c3ecefc9..8074e815 100644 --- a/ignifyr-engine/src/main/scala/io/ignifyr/engine/model/FhirMappingTask.scala +++ b/ignifyr-engine/src/main/scala/io/ignifyr/engine/model/FhirMappingTask.scala @@ -30,10 +30,14 @@ case class FhirMappingTask( * @return A new mapping task with substituted preprocessSql in all source bindings */ def substituteBatchParameters(parameters: Map[String, String]): FhirMappingTask = { + // Longest parameter name first. Substituting a shorter name first would rewrite the prefix of a + // longer one that starts with it: with `year` and `yearEnd` both defined, `$yearEnd` would become + // `2020End`. Map iteration order is unspecified, so relying on it is not an option either. + val orderedParameters = parameters.toSeq.sortBy(-_._1.length) val updatedSourceBinding = sourceBinding.map { case (alias, binding) => alias -> (binding.preprocessSql match { case Some(sql) => - val substitutedSql = parameters.foldLeft(sql) { case (currentSql, (paramName, paramValue)) => + val substitutedSql = orderedParameters.foldLeft(sql) { case (currentSql, (paramName, paramValue)) => currentSql.replace(s"$$$paramName", paramValue) } binding.withPreprocessSql(Some(substitutedSql)) diff --git a/ignifyr-engine/src/main/scala/io/ignifyr/engine/spi/ExtensionRegistry.scala b/ignifyr-engine/src/main/scala/io/ignifyr/engine/spi/ExtensionRegistry.scala index 8e06d044..3b9a5521 100644 --- a/ignifyr-engine/src/main/scala/io/ignifyr/engine/spi/ExtensionRegistry.scala +++ b/ignifyr-engine/src/main/scala/io/ignifyr/engine/spi/ExtensionRegistry.scala @@ -91,9 +91,15 @@ object ExtensionRegistry { * comma-separated, additive list, so multiple contributors are concatenated (deduplicated); any * other key claimed by more than one extension is a configuration error and fails fast. */ - lazy val sparkConfContributions: Map[String, String] = { - val entries: Seq[(String, String, String)] = // (ownerExtensionId, key, value) - extensions.flatMap(e => e.sparkConfContributions.map { case (k, v) => (e.id, k, v) }) + lazy val sparkConfContributions: Map[String, String] = + mergeSparkConf(extensions.flatMap(e => e.sparkConfContributions.map { case (k, v) => (e.id, k, v) })) + + /** + * Merge `(ownerExtensionId, key, value)` Spark-conf triples, concatenating the additive + * `spark.sql.extensions` and failing on any other key claimed twice. Visible to `io.ignifyr` for the + * same reason as [[indexUnique]]. + */ + private[ignifyr] def mergeSparkConf(entries: Seq[(String, String, String)]): Map[String, String] = { entries.groupBy(_._2).map { case (key, group) => if (key == "spark.sql.extensions") { key -> group.map(_._3).distinct.mkString(",") @@ -109,8 +115,11 @@ object ExtensionRegistry { } } - /** Selects at most one capability provider, failing fast (naming owners) if more than one is installed. */ - private def singleCapability[V](what: String)(entries: Seq[(String, V)]): Option[V] = { + /** + * Selects at most one capability provider, failing fast (naming owners) if more than one is installed. + * Visible to `io.ignifyr` so the fail-fast contract can be asserted without a second classloader. + */ + private[ignifyr] def singleCapability[V](what: String)(entries: Seq[(String, V)]): Option[V] = { if (entries.size > 1) { throw new IllegalStateException( s"Multiple $what modules installed: ${entries.map(_._1).mkString(", ")}. Install exactly one." @@ -144,9 +153,10 @@ object ExtensionRegistry { /** * Index `(ownerExtensionId, key, value)` triples into a `key -> value` map, failing with both - * owner ids if any key is claimed twice. + * owner ids if any key is claimed twice. Visible to `io.ignifyr` for the same reason as + * [[singleCapability]]. */ - private def indexUnique[K, V](what: String)(entries: Seq[(String, K, V)]): Map[K, V] = + private[ignifyr] def indexUnique[K, V](what: String)(entries: Seq[(String, K, V)]): Map[K, V] = entries.groupBy(_._2).map { case (key, group) => if (group.size > 1) { val owners = group.map(_._1).mkString(", ") diff --git a/ignifyr-engine/src/main/scala/io/ignifyr/engine/util/SparkUtil.scala b/ignifyr-engine/src/main/scala/io/ignifyr/engine/util/SparkUtil.scala index d016cf8a..5a1d680b 100644 --- a/ignifyr-engine/src/main/scala/io/ignifyr/engine/util/SparkUtil.scala +++ b/ignifyr-engine/src/main/scala/io/ignifyr/engine/util/SparkUtil.scala @@ -84,11 +84,13 @@ object SparkUtil { * @return */ def getLastCommitOffset(commitFileDirectory: File): Int = { - commitFileDirectory - .listFiles() + Option(commitFileDirectory.listFiles()) // null when the directory does not exist (yet) + .getOrElse(Array.empty[File]) .filter(file => file.isFile && !file.getName.contains(".")) .map(file => file.getName.toInt) - .max + .maxOption + // No commit file yet. -1 keeps the caller's Range.inclusive(lastProcessed + 1, offset) empty. + .getOrElse(-1) } /** diff --git a/ignifyr-engine/src/test/scala/io/ignifyr/test/engine/cli/CommandLineInterfaceTest.scala b/ignifyr-engine/src/test/scala/io/ignifyr/test/engine/cli/CommandLineInterfaceTest.scala new file mode 100644 index 00000000..8e4f9ed8 --- /dev/null +++ b/ignifyr-engine/src/test/scala/io/ignifyr/test/engine/cli/CommandLineInterfaceTest.scala @@ -0,0 +1,67 @@ +package io.ignifyr.test.engine.cli + +import io.ignifyr.engine.cli.CommandLineInterface +import org.scalatest.flatspec.AnyFlatSpec +import org.scalatest.matchers.should.Matchers + +/** + * Covers `nextArg`, the parser behind every `Boot` invocation: it turns the process arguments into the + * `command` token plus the `--flag value` pairs that `Boot` and the CLI command providers read. + */ +class CommandLineInterfaceTest extends AnyFlatSpec with Matchers { + + private def parse(args: String*): Map[String, Any] = CommandLineInterface.nextArg(Map(), args.toList) + + "nextArg" should "return an empty map when there are no arguments" in { + parse() shouldBe empty + } + + it should "read a bare token as the command" in { + parse("cli") shouldBe Map("command" -> "cli") + } + + it should "map a --flag value pair onto the bare flag name" in { + parse("run", "--job", "jobs/patient.json") shouldBe + Map("command" -> "run", "job" -> "jobs/patient.json") + } + + it should "accept both --db and --db-path for the database folder" in { + parse("run", "--db", "./db") should contain("db-path" -> "./db") + parse("run", "--db-path", "./db") should contain("db-path" -> "./db") + } + + it should "collect several flags of an extension-contributed command" in { + parse( + "extract-redcap-schemas", + "--data-dictionary", + "dictionary.csv", + "--definition-root-url", + "http://example.com/fhir", + "--encoding", + "utf-8" + ) shouldBe Map( + "command" -> "extract-redcap-schemas", + "data-dictionary" -> "dictionary.csv", + "definition-root-url" -> "http://example.com/fhir", + "encoding" -> "utf-8" + ) + } + + it should "accept flags before the command" in { + parse("--db-path", "./db", "run") shouldBe Map("db-path" -> "./db", "command" -> "run") + } + + // Regression: the trailing `--job` used to fall through to the bare-token case and overwrite + // `command`, so `Boot` reported "unknown command --job" instead of falling back to the configured job. + it should "keep the command when a trailing flag has no value" in { + parse("run", "--job") should contain("command" -> "run") + } + + it should "keep the first bare token as the command" in { + parse("run", "extra") should contain("command" -> "run") + } + + it should "let a later flag override an earlier one with the same name" in { + parse("run", "--job", "first.json", "--job", "second.json") should contain("job" -> "second.json") + } +} diff --git a/ignifyr-engine/src/test/scala/io/ignifyr/test/engine/data/SinkHandlerTest.scala b/ignifyr-engine/src/test/scala/io/ignifyr/test/engine/data/SinkHandlerTest.scala new file mode 100644 index 00000000..be2101e0 --- /dev/null +++ b/ignifyr-engine/src/test/scala/io/ignifyr/test/engine/data/SinkHandlerTest.scala @@ -0,0 +1,109 @@ +package io.ignifyr.test.engine.data + +import io.ignifyr.engine.config.IgnifyrConfig +import io.ignifyr.engine.data.write.{BaseSinkWriter, SinkHandler} +import io.ignifyr.engine.execution.log.ExecutionLogger +import io.ignifyr.engine.model._ +import org.apache.spark.sql.{Dataset, SparkSession} +import org.apache.spark.util.CollectionAccumulator +import org.scalatest.flatspec.AnyFlatSpec +import org.scalatest.matchers.should.Matchers + +import java.sql.Timestamp +import java.time.Instant + +/** + * `SinkHandler` decides what actually reaches a sink: it splits the mapping results into invalid inputs, + * mapping errors and mapped resources, and hands only the last group to the writer. A row landing in the + * wrong group is silent — an error row written as if it were a resource, or a good resource dropped — + * so the split is asserted here rather than through a sink that would hide it. + */ +class SinkHandlerTest extends AnyFlatSpec with Matchers { + + private val sparkSession: SparkSession = IgnifyrConfig.sparkSession + + /** Captures the dataset it is handed instead of writing it anywhere. */ + private class CapturingWriter extends BaseSinkWriter(FileSystemSinkSettings("./out", SinkContentTypes.NDJSON)) { + var written: Seq[FhirMappingResult] = Seq.empty + override def write( + spark: SparkSession, + df: Dataset[FhirMappingResult], + problemsAccumulator: CollectionAccumulator[FhirMappingResult] + ): Unit = written = df.collect().toSeq + override def validate(): Unit = () + } + + private def result( + source: String, + mappedResource: Option[String] = None, + error: Option[FhirMappingError] = None + ): FhirMappingResult = + FhirMappingResult( + jobId = "job-1", + mappingTaskName = "task-1", + timestamp = Timestamp.from(Instant.now()), + source = source, + mappedFhirResource = mappedResource.map(resource => MappedFhirResource(mappedResource = Some(resource))), + error = error + ) + + private val execution: FhirMappingJobExecution = FhirMappingJobExecution( + id = "execution-1", + job = FhirMappingJob( + id = "job-1", + sourceSettings = Map.empty, + sinkSettings = FileSystemSinkSettings("./out", SinkContentTypes.NDJSON), + mappings = Seq.empty, + dataProcessingSettings = DataProcessingSettings() // saveErroneousRecords = false + ) + ) + + /* + * `SinkHandler` reports each chunk through `ExecutionLogger`, which keeps per-execution state and + * looks the execution up by id — so a chunk can only be logged for an execution that was already + * logged as STARTED. The launcher always does that first; the call here stands in for it, and + * omitting it is what a caller wiring up a new execution path would trip over. + */ + ExecutionLogger.logExecutionStatus(execution, FhirMappingJobResult.STARTED) + + private def write(results: FhirMappingResult*): Seq[FhirMappingResult] = { + import sparkSession.implicits._ + val writer = new CapturingWriter + SinkHandler.writeMappingResult(sparkSession, execution, "task-1", results.toSeq.toDS(), writer) + writer.written + } + + "writeMappingResult" should "hand the mapped resources to the writer" in { + val mapped = result("row-1", mappedResource = Some("""{"resourceType":"Patient"}""")) + write(mapped).map(_.source) shouldBe Seq("row-1") + } + + it should "keep an invalid input away from the writer" in { + val invalid = result("bad-row", error = Some(FhirMappingError(FhirMappingErrorCodes.INVALID_INPUT, "missing pid"))) + write(invalid) shouldBe empty + } + + it should "keep a mapping error away from the writer" in { + val failed = result("row-2", error = Some(FhirMappingError(FhirMappingErrorCodes.MAPPING_ERROR, "bad expression"))) + write(failed) shouldBe empty + } + + it should "write only the mapped resources of a mixed batch" in { + val written = write( + result("ok-1", mappedResource = Some("""{"resourceType":"Patient"}""")), + result("bad", error = Some(FhirMappingError(FhirMappingErrorCodes.INVALID_INPUT, "missing pid"))), + result("failed", error = Some(FhirMappingError(FhirMappingErrorCodes.MAPPING_ERROR, "bad expression"))), + result("ok-2", mappedResource = Some("""{"resourceType":"Observation"}""")) + ) + written.map(_.source) should contain theSameElementsAs Seq("ok-1", "ok-2") + } + + // A row with no payload and no error is nothing to write — a mapping whose precondition excluded it. + it should "skip a result that carries neither a payload nor an error" in { + write(result("skipped")) shouldBe empty + } + + it should "call the writer even when there is nothing to write" in { + write() shouldBe empty + } +} diff --git a/ignifyr-engine/src/test/scala/io/ignifyr/test/engine/env/EnvironmentVariableResolverTest.scala b/ignifyr-engine/src/test/scala/io/ignifyr/test/engine/env/EnvironmentVariableResolverTest.scala new file mode 100644 index 00000000..73911375 --- /dev/null +++ b/ignifyr-engine/src/test/scala/io/ignifyr/test/engine/env/EnvironmentVariableResolverTest.scala @@ -0,0 +1,94 @@ +package io.ignifyr.test.engine.env + +import io.ignifyr.engine.env.EnvironmentVariableResolver +import io.ignifyr.engine.model._ +import org.scalatest.flatspec.AnyFlatSpec +import org.scalatest.matchers.should.Matchers + +/** + * Covers `${ENV_VAR}` substitution in mapping-job definitions. Only the names listed in the + * `EnvironmentVariable` enumeration are substitutable; anything else is rejected rather than silently + * left in place, which is what makes a typo in a job file a startup error instead of a bad FHIR write. + * + * `resolveFileContent` is exercised against an explicit environment (`sys.env` cannot be set from + * inside the JVM); the object-level paths need no environment because they are the failure paths. + */ +class EnvironmentVariableResolverTest extends AnyFlatSpec with Matchers { + + private val env = Map("FHIR_REPO_URL" -> "http://onfhir:8080/fhir", "DATA_FOLDER_PATH" -> "/data") + + private def jobWith( + sourceSettings: Map[String, MappingJobSourceSettings], + sinkSettings: SinkSettings = FhirRepositorySinkSettings(fhirRepoUrl = "http://localhost/fhir"), + mappings: Seq[FhirMappingTask] = Seq.empty + ): FhirMappingJob = + FhirMappingJob(sourceSettings = sourceSettings, sinkSettings = sinkSettings, mappings = mappings) + + "resolveFileContent" should "replace a placeholder with the value from the environment" in { + EnvironmentVariableResolver.resolveFileContent("""{"fhirRepoUrl": "${FHIR_REPO_URL}"}""", env) shouldBe + """{"fhirRepoUrl": "http://onfhir:8080/fhir"}""" + } + + it should "replace every occurrence of every known placeholder" in { + EnvironmentVariableResolver.resolveFileContent( + "${DATA_FOLDER_PATH}:${FHIR_REPO_URL}:${DATA_FOLDER_PATH}", + env + ) shouldBe + "/data:http://onfhir:8080/fhir:/data" + } + + it should "leave a known placeholder untouched when the variable is not set" in { + EnvironmentVariableResolver.resolveFileContent("${SOURCE_URL}", env) shouldBe "${SOURCE_URL}" + } + + it should "leave content with no placeholder unchanged" in { + val content = """{"name": "no placeholders here"}""" + EnvironmentVariableResolver.resolveFileContent(content, env) shouldBe content + } + + "resolveFhirMappingJob" should "leave settings without a placeholder unchanged" in { + val job = jobWith( + Map("main" -> FileSystemSourceSettings(name = "src", sourceUri = "urn:test", dataFolderPath = "/plain/path")) + ) + val resolved = EnvironmentVariableResolver.resolveFhirMappingJob(job) + resolved.sourceSettings("main").asInstanceOf[FileSystemSourceSettings].dataFolderPath shouldBe "/plain/path" + resolved.sinkSettings.asInstanceOf[FhirRepositorySinkSettings].fhirRepoUrl shouldBe "http://localhost/fhir" + } + + it should "reject a placeholder whose name is not in the EnvironmentVariable enumeration" in { + val job = jobWith( + Map( + "main" -> FileSystemSourceSettings(name = "src", sourceUri = "urn:test", dataFolderPath = "${NOT_A_KNOWN_VAR}") + ) + ) + val thrown = the[RuntimeException] thrownBy EnvironmentVariableResolver.resolveFhirMappingJob(job) + thrown.getMessage should include("NOT_A_KNOWN_VAR") + thrown.getMessage should include("not recognized") + } + + it should "reject a known placeholder that is not set in the environment" in { + // SOURCE_URL is a legal name, so this fails on the value being absent rather than on the name. + val job = jobWith( + Map("main" -> FileSystemSourceSettings(name = "src", sourceUri = "urn:test", dataFolderPath = "${SOURCE_URL}")), + sinkSettings = FileSystemSinkSettings(path = "./out", contentType = SinkContentTypes.NDJSON) + ) + assume(sys.env.get("SOURCE_URL").isEmpty, "SOURCE_URL must be unset for this failure path") + val thrown = the[RuntimeException] thrownBy EnvironmentVariableResolver.resolveFhirMappingJob(job) + thrown.getMessage should include("SOURCE_URL") + thrown.getMessage should include("is not set") + } + + it should "not touch a sink type that carries no resolvable field" in { + val fileSink = FileSystemSinkSettings(path = "./out", contentType = SinkContentTypes.NDJSON) + val job = jobWith( + Map("main" -> FileSystemSourceSettings(name = "src", sourceUri = "urn:test", dataFolderPath = "/data")), + sinkSettings = fileSink + ) + EnvironmentVariableResolver.resolveFhirMappingJob(job).sinkSettings shouldBe fileSink + } + + "getEnvironmentVariables" should "only report names declared in the enumeration" in { + val known = Set("FHIR_REPO_URL", "DATA_FOLDER_PATH", "SOURCE_URL", "REDCAP_PROJECT_ID") + EnvironmentVariableResolver.getEnvironmentVariables.keySet.diff(known) shouldBe empty + } +} diff --git a/ignifyr-engine/src/test/scala/io/ignifyr/test/engine/execution/RunningJobRegistryTest.scala b/ignifyr-engine/src/test/scala/io/ignifyr/test/engine/execution/RunningJobRegistryTest.scala index 78ebce1e..edce7e66 100644 --- a/ignifyr-engine/src/test/scala/io/ignifyr/test/engine/execution/RunningJobRegistryTest.scala +++ b/ignifyr-engine/src/test/scala/io/ignifyr/test/engine/execution/RunningJobRegistryTest.scala @@ -1,8 +1,10 @@ package io.ignifyr.test.engine.execution import akka.actor.ActorSystem +import io.ignifyr.engine.config.IgnifyrConfig import io.ignifyr.engine.execution.RunningJobRegistry -import io.ignifyr.engine.model.{FhirMappingJob, FhirMappingJobExecution, FhirMappingTask, FileSystemSourceSettings} +import io.ignifyr.engine.model._ +import io.ignifyr.engine.util.FileUtils import org.apache.spark.SparkContext import org.apache.spark.sql.SparkSession import org.apache.spark.sql.streaming.StreamingQuery @@ -11,6 +13,8 @@ import org.mockito.{ArgumentCaptor, ArgumentMatchers} import org.scalatest.flatspec.AnyFlatSpec import org.scalatest.matchers.should.Matchers +import java.io.{File, PrintWriter} +import java.nio.file.Paths import scala.concurrent.duration.DurationInt import scala.concurrent.{Await, ExecutionContext, Future} import scala.language.postfixOps @@ -86,6 +90,77 @@ class RunningJobRegistryTest extends AnyFlatSpec with Matchers { runningTaskRegistry.getRunningExecutions().contains("j4") shouldBe false } + /* + * The contract `registerBatchJob` documents, and the reason it chains with `andThen` rather than + * `onComplete`: the future it hands back completes only after `handleCompletedBatchJob` — input + * archiving plus deregistration — has already run. The one-shot batch CLI awaits exactly this future + * and then calls System.exit(0), so if it completed on the raw mapping-task future instead, the JVM + * could exit mid-archive and `archiveMode = archive|delete` would silently leave inputs in place. + */ + "it" should "complete the returned future only after the processed inputs have been archived" in { + val sourceFolder = "test-batch-completion" + val inputFile = FileUtils.getPath(sourceFolder, "input.csv").toFile + inputFile.getParentFile.mkdirs() + val writer = new PrintWriter(inputFile) + writer.write("pid\np1") + writer.close() + + // The archiver preserves the input's path underneath the configured archive folder. + val relativePath = FileUtils.getPath("").toAbsolutePath.relativize(inputFile.toPath.toAbsolutePath) + val archivedFile = Paths.get(IgnifyrConfig.engineConfig.archiveFolder, relativePath.toString).toFile + + val execution = batchExecutionWithArchiving("j5", "e5", sourceFolder, "input.csv") + inputFile.exists() shouldBe true + archivedFile.exists() shouldBe false + + // A mapping-task future that finishes a little after registration, as a real batch run would. + val completion = runningTaskRegistry.registerBatchJob(execution, Some(Future(Thread.sleep(200))), "") + Await.result(completion, 10 seconds) + + // Asserted with no sleep in between: awaiting the returned future is the whole guarantee. + inputFile.exists() shouldBe false + archivedFile.exists() shouldBe true + runningTaskRegistry.getRunningExecutions().contains("j5") shouldBe false + + org.apache.commons.io.FileUtils.deleteDirectory(FileUtils.getPath(sourceFolder).toFile) + org.apache.commons.io.FileUtils.deleteDirectory(new File(IgnifyrConfig.engineConfig.archiveFolder)) + } + + "it" should "return an already completed future for a scheduled batch job with no mapping-task future" in { + // Scheduled runs have no future to hang off; the scheduling module calls handleCompletedBatchJob itself. + val execution = batchExecutionWithArchiving("j6", "e6", "test-batch-no-future", "input.csv") + val completion = runningTaskRegistry.registerBatchJob(execution, None, "") + completion.isCompleted shouldBe true + runningTaskRegistry.getRunningExecutions().contains("j6") shouldBe true + } + + /** A non-streaming execution whose single file source is archived once the job completes. */ + private def batchExecutionWithArchiving( + jobId: String, + executionId: String, + sourceFolderPath: String, + inputFileName: String + ): FhirMappingJobExecution = { + val sourceSettings = + FileSystemSourceSettings(name = "test", sourceUri = "urn:test", dataFolderPath = sourceFolderPath) + val mappingTask = FhirMappingTask( + name = "m", + mappingRef = "http://test/mappings/m", + sourceBinding = Map("source" -> FileSystemSource(path = inputFileName, contentType = SourceContentTypes.CSV)) + ) + FhirMappingJobExecution( + id = executionId, + job = FhirMappingJob( + id = jobId, + sourceSettings = Map("source" -> sourceSettings), + sinkSettings = FhirRepositorySinkSettings(fhirRepoUrl = "http://localhost/fhir"), + mappings = Seq(mappingTask), + dataProcessingSettings = DataProcessingSettings(archiveMode = ArchiveModes.ARCHIVE) + ), + mappingTasks = Seq(mappingTask) + ) + } + private def getTestInput( jobId: String, executionId: String, diff --git a/ignifyr-engine/src/test/scala/io/ignifyr/test/engine/mapping/SchemaConverterTest.scala b/ignifyr-engine/src/test/scala/io/ignifyr/test/engine/mapping/SchemaConverterTest.scala new file mode 100644 index 00000000..1cb2162a --- /dev/null +++ b/ignifyr-engine/src/test/scala/io/ignifyr/test/engine/mapping/SchemaConverterTest.scala @@ -0,0 +1,99 @@ +package io.ignifyr.test.engine.mapping + +import io.ignifyr.engine.mapping.schema.SchemaConverter +import io.ignifyr.engine.util.MajorFhirVersion +import org.apache.spark.sql.types._ +import org.scalatest.flatspec.AnyFlatSpec +import org.scalatest.matchers.should.Matchers + +/** + * Pins `fieldsToSchema`, the Spark-column -> FHIR-element table behind schema inference and schema + * export. It is a plain lookup table with no dependencies, so a silently changed entry produces a + * wrong element type in an exported StructureDefinition rather than an error. + * + * The opposite direction (`convertSchema`) is deliberately not unit-tested here: resolving a + * StructureDefinition's `differential` needs onFHIR's base FHIR config initialized (the server's + * `SchemaFolderRepository.initBaseFhirConfig`), so it is covered where that context exists — the + * server's long-tier `SchemaEndpointTest`. Its half of the mapping is referenced below only as the + * constant it is, to keep the two directions' known asymmetries visible. + */ +class SchemaConverterTest extends AnyFlatSpec with Matchers { + + private val schemaConverter = new SchemaConverter(MajorFhirVersion.R4) + + private def fhirTypeOf(sparkType: DataType, nullable: Boolean = true): Option[String] = + schemaConverter + .fieldsToSchema(StructField("col", sparkType, nullable), "Schema") + .dataTypes + .flatMap(_.headOption.map(_.dataType)) + + "fieldsToSchema" should "map the Spark integral types onto FHIR integer types" in { + fhirTypeOf(IntegerType) shouldBe Some("integer") + fhirTypeOf(ShortType) shouldBe Some("integer") + fhirTypeOf(ByteType) shouldBe Some("integer") + fhirTypeOf(LongType) shouldBe Some("integer64") + } + + it should "map the Spark fractional types onto FHIR decimal" in { + fhirTypeOf(DoubleType) shouldBe Some("decimal") + fhirTypeOf(FloatType) shouldBe Some("decimal") + fhirTypeOf(DataTypes.createDecimalType(10, 2)) shouldBe Some("decimal") + } + + it should "map the remaining supported Spark types" in { + fhirTypeOf(StringType) shouldBe Some("string") + fhirTypeOf(NullType) shouldBe Some("string") + fhirTypeOf(BooleanType) shouldBe Some("boolean") + fhirTypeOf(BinaryType) shouldBe Some("base64Binary") + fhirTypeOf(DateType) shouldBe Some("date") + fhirTypeOf(TimestampType) shouldBe Some("dateTime") + } + + it should "attach the canonical profile url of the mapped data type" in { + schemaConverter + .fieldsToSchema(StructField("col", BooleanType), "Schema") + .dataTypes + .flatMap(_.headOption.flatMap(_.profiles.flatMap(_.headOption))) shouldBe + Some("http://hl7.org/fhir/StructureDefinition/boolean") + } + + it should "report no data type for a Spark type it does not know" in { + fhirTypeOf(StructType(Seq(StructField("nested", StringType)))) shouldBe None + fhirTypeOf(MapType(StringType, StringType)) shouldBe None + } + + it should "mark an array element unbounded and a scalar element single" in { + val array = schemaConverter.fieldsToSchema(StructField("col", ArrayType(StringType)), "Schema") + array.isArray shouldBe true + array.maxCardinality shouldBe None // None represents "*" + array.dataTypes.flatMap(_.headOption.map(_.dataType)) shouldBe Some("string") + + val scalar = schemaConverter.fieldsToSchema(StructField("col", StringType), "Schema") + scalar.isArray shouldBe false + scalar.maxCardinality shouldBe Some(1) + } + + it should "derive the minimum cardinality from nullability" in { + schemaConverter.fieldsToSchema(StructField("col", StringType, nullable = true), "Schema").minCardinality shouldBe 0 + schemaConverter.fieldsToSchema(StructField("col", StringType, nullable = false), "Schema").minCardinality shouldBe 1 + } + + it should "name the element after the column and prefix its path with the schema" in { + val element = schemaConverter.fieldsToSchema(StructField("birthDate", DateType), "Ext-patient") + element.id shouldBe "birthDate" + element.path shouldBe "Ext-patient.birthDate" + element.isPrimitive shouldBe true + } + + /* + * The read direction (convertSchema) and this write direction are NOT inverses of each other, which is + * load-bearing knowledge rather than a defect to fix here: changing either table rewrites the + * StructureDefinitions the server exports for existing projects. The read-direction values quoted below + * are the ones asserted in SchemaConverter.getSparkType. + */ + it should "not round-trip the three FHIR types that share a Spark type with another" in { + fhirTypeOf(TimestampType) shouldBe Some("dateTime") // instant -> TimestampType -> dateTime + fhirTypeOf(LongType) shouldBe Some("integer64") // unsignedInt -> LongType -> integer64 + fhirTypeOf(StringType) shouldBe Some("string") // date/time/code/uri/id -> StringType -> string + } +} diff --git a/ignifyr-engine/src/test/scala/io/ignifyr/test/engine/model/FhirMappingTaskTest.scala b/ignifyr-engine/src/test/scala/io/ignifyr/test/engine/model/FhirMappingTaskTest.scala new file mode 100644 index 00000000..cbab31f2 --- /dev/null +++ b/ignifyr-engine/src/test/scala/io/ignifyr/test/engine/model/FhirMappingTaskTest.scala @@ -0,0 +1,88 @@ +package io.ignifyr.test.engine.model + +import io.ignifyr.engine.model.{BatchingStrategy, FhirMappingTask, SqlSource} +import org.scalatest.flatspec.AnyFlatSpec +import org.scalatest.matchers.should.Matchers + +/** + * Covers the substitution half of the batching strategy: `batchParameterSets` are rewritten into each + * source binding's `preprocessSql` as `$parameterName`. The orchestration half (one execution per + * parameter set) lives in `FhirMappingJobManager` and is exercised by the SQL integration suite. + */ +class FhirMappingTaskTest extends AnyFlatSpec with Matchers { + + /** A task whose single source binding carries the given preprocess SQL. */ + private def taskWith(preprocessSql: Option[String], aliases: Seq[String] = Seq("source")): FhirMappingTask = + FhirMappingTask( + name = "batched-task", + mappingRef = "http://test/mappings/batched", + sourceBinding = + aliases.map(alias => alias -> SqlSource(tableName = Some("visits"), preprocessSql = preprocessSql)).toMap, + batchingStrategy = Some(BatchingStrategy(Seq(Map("year" -> "2014")))) + ) + + private def sqlOf(task: FhirMappingTask, alias: String = "source"): Option[String] = + task.sourceBinding(alias).preprocessSql + + "substituteBatchParameters" should "substitute a single parameter" in { + val task = taskWith(Some("SELECT * FROM visits WHERE year = $year")) + sqlOf(task.substituteBatchParameters(Map("year" -> "2014"))) shouldBe + Some("SELECT * FROM visits WHERE year = 2014") + } + + it should "substitute every parameter of a multi-parameter batch" in { + val task = taskWith(Some("SELECT * FROM visits WHERE year = $year AND month = $month")) + sqlOf(task.substituteBatchParameters(Map("year" -> "2020", "month" -> "1"))) shouldBe + Some("SELECT * FROM visits WHERE year = 2020 AND month = 1") + } + + it should "substitute a parameter at every occurrence" in { + val task = taskWith(Some("SELECT $year FROM visits WHERE year = $year")) + sqlOf(task.substituteBatchParameters(Map("year" -> "2014"))) shouldBe + Some("SELECT 2014 FROM visits WHERE year = 2014") + } + + // Regression: substituting the shorter name first rewrote the prefix of the longer one, so + // `$yearEnd` came out as `2020End`. Map iteration order does not make either order reliable. + it should "not rewrite a longer parameter name that starts with a shorter one" in { + val task = taskWith(Some("SELECT * FROM visits WHERE year >= $year AND year < $yearEnd")) + sqlOf(task.substituteBatchParameters(Map("year" -> "2020", "yearEnd" -> "2021"))) shouldBe + Some("SELECT * FROM visits WHERE year >= 2020 AND year < 2021") + } + + it should "not rewrite the prefix regardless of the order the parameters are given in" in { + val task = taskWith(Some("$yearEnd/$year")) + val fromShortFirst = sqlOf(task.substituteBatchParameters(Map("year" -> "2020", "yearEnd" -> "2021"))) + val fromLongFirst = sqlOf(task.substituteBatchParameters(Map("yearEnd" -> "2021", "year" -> "2020"))) + fromShortFirst shouldBe Some("2021/2020") + fromLongFirst shouldBe fromShortFirst + } + + it should "substitute in every source binding of the task" in { + val task = taskWith(Some("SELECT * FROM t WHERE year = $year"), aliases = Seq("main", "secondary")) + val substituted = task.substituteBatchParameters(Map("year" -> "2014")) + sqlOf(substituted, "main") shouldBe Some("SELECT * FROM t WHERE year = 2014") + sqlOf(substituted, "secondary") shouldBe Some("SELECT * FROM t WHERE year = 2014") + } + + it should "leave a source binding without preprocess SQL untouched" in { + val task = taskWith(None) + val substituted = task.substituteBatchParameters(Map("year" -> "2014")) + sqlOf(substituted) shouldBe None + substituted.sourceBinding.keySet shouldBe task.sourceBinding.keySet + } + + it should "leave a placeholder with no matching parameter in place" in { + val task = taskWith(Some("SELECT * FROM visits WHERE year = $year AND site = $site")) + sqlOf(task.substituteBatchParameters(Map("year" -> "2014"))) shouldBe + Some("SELECT * FROM visits WHERE year = 2014 AND site = $site") + } + + it should "preserve everything else about the task" in { + val task = taskWith(Some("SELECT * FROM visits WHERE year = $year")) + val substituted = task.substituteBatchParameters(Map("year" -> "2014")) + substituted.name shouldBe task.name + substituted.mappingRef shouldBe task.mappingRef + substituted.batchingStrategy shouldBe task.batchingStrategy + } +} diff --git a/ignifyr-engine/src/test/scala/io/ignifyr/test/engine/spi/ExtensionRegistrySpec.scala b/ignifyr-engine/src/test/scala/io/ignifyr/test/engine/spi/ExtensionRegistrySpec.scala index b16dcc8c..725f8da9 100644 --- a/ignifyr-engine/src/test/scala/io/ignifyr/test/engine/spi/ExtensionRegistrySpec.scala +++ b/ignifyr-engine/src/test/scala/io/ignifyr/test/engine/spi/ExtensionRegistrySpec.scala @@ -45,4 +45,64 @@ class ExtensionRegistrySpec extends AnyFlatSpec with Matchers { ex.getMessage should include("No source reader registered") ex.getMessage should include("UnregisteredSource") } + + it should "materialize every rejecting registry without error on the core-only classpath" in { + noException should be thrownBy ExtensionRegistry.init() + } + + // The three fail-fast guards below are what `init()` exists to trigger at engine startup rather than + // mid-job. They are asserted on the indexing helpers directly: the registries themselves are fed by + // ServiceLoader, so a duplicate cannot be staged on this classpath without a second classloader. + it should "index one registration per key" in { + ExtensionRegistry.indexUnique[String, Int]("source reader")(Seq(("ext-a", "k1", 1), ("ext-b", "k2", 2))) shouldBe + Map("k1" -> 1, "k2" -> 2) + } + + it should "fail fast naming both owners when two extensions claim the same key" in { + val ex = intercept[IllegalStateException] { + ExtensionRegistry.indexUnique[String, Int]("source reader")( + Seq(("ext-a", "same-key", 1), ("ext-b", "same-key", 2)) + ) + } + ex.getMessage should include("Duplicate source reader registration") + ex.getMessage should include("same-key") + ex.getMessage should include("ext-a") + ex.getMessage should include("ext-b") + } + + it should "select the single installed capability provider, or none" in { + ExtensionRegistry.singleCapability[String]("streaming")(Seq(("ext-a", "provider"))) shouldBe Some("provider") + ExtensionRegistry.singleCapability[String]("streaming")(Seq.empty) shouldBe None + } + + it should "fail fast naming both owners when two single-capability providers are installed" in { + val ex = intercept[IllegalStateException] { + ExtensionRegistry.singleCapability[String]("streaming")(Seq(("ext-a", "p1"), ("ext-b", "p2"))) + } + ex.getMessage should include("Multiple streaming modules installed") + ex.getMessage should include("ext-a") + ex.getMessage should include("ext-b") + } + + it should "concatenate the additive spark.sql.extensions contributed by several modules" in { + ExtensionRegistry.mergeSparkConf( + Seq( + ("ext-a", "spark.sql.extensions", "ClassA"), + ("ext-b", "spark.sql.extensions", "ClassB"), + ("ext-c", "spark.sql.extensions", "ClassA") // duplicate value, contributed twice + ) + ) shouldBe Map("spark.sql.extensions" -> "ClassA,ClassB") + } + + it should "fail fast when two modules claim the same non-additive Spark-conf key" in { + val ex = intercept[IllegalStateException] { + ExtensionRegistry.mergeSparkConf( + Seq(("ext-a", "spark.sql.catalog.spark_catalog", "A"), ("ext-b", "spark.sql.catalog.spark_catalog", "B")) + ) + } + ex.getMessage should include("Conflicting Spark configuration") + ex.getMessage should include("spark.sql.catalog.spark_catalog") + ex.getMessage should include("ext-a") + ex.getMessage should include("ext-b") + } } diff --git a/ignifyr-engine/src/test/scala/io/ignifyr/test/engine/util/FhirMappingJobFormatterTest.scala b/ignifyr-engine/src/test/scala/io/ignifyr/test/engine/util/FhirMappingJobFormatterTest.scala new file mode 100644 index 00000000..66089d68 --- /dev/null +++ b/ignifyr-engine/src/test/scala/io/ignifyr/test/engine/util/FhirMappingJobFormatterTest.scala @@ -0,0 +1,111 @@ +package io.ignifyr.test.engine.util + +import io.onfhir.client.model.BasicAuthenticationSettings +import io.ignifyr.engine.model._ +import io.ignifyr.engine.util.FhirMappingJobFormatter +import org.json4s.MappingException +import org.scalatest.flatspec.AnyFlatSpec +import org.scalatest.matchers.should.Matchers + +import java.nio.file.Files + +/** + * Covers the mapping-job (de)serializer: the `ShortTypeHints` table that lets one job file carry any + * source/sink/scheduling type, and the duplicate-mappingTask-name rejection. The server rejects + * duplicate names on its own create path; this is the file-read path the CLI and the scheduler use. + */ +class FhirMappingJobFormatterTest extends AnyFlatSpec with Matchers { + + private def task(name: String, binding: MappingSourceBinding = FileSystemSource("p.csv", SourceContentTypes.CSV)) = + FhirMappingTask(name = name, mappingRef = s"http://test/mappings/$name", sourceBinding = Map("source" -> binding)) + + /** Round-trips a job through a temp file, the way the CLI reads a job handed to `run --job`. */ + private def roundTrip(job: FhirMappingJob): FhirMappingJob = { + val file = Files.createTempFile("ignifyr-job", ".json") + FhirMappingJobFormatter.saveMappingJobToFile(job, file.toString) + FhirMappingJobFormatter.readMappingJobFromFile(file.toString) + } + + "findDuplicateMappingTaskNames" should "return nothing when every name is unique" in { + FhirMappingJobFormatter.findDuplicateMappingTaskNames(Seq(task("a"), task("b"))) shouldBe empty + } + + it should "return each repeated name once" in { + val duplicates = + FhirMappingJobFormatter.findDuplicateMappingTaskNames(Seq(task("a"), task("a"), task("a"), task("b"), task("b"))) + duplicates should contain theSameElementsAs Seq("a", "b") + } + + "readMappingJobFromFile" should "restore every registered source and sink type" in { + val job = FhirMappingJob( + name = Some("multi-source-job"), + sourceSettings = Map( + "file" -> FileSystemSourceSettings(name = "f", sourceUri = "urn:f", dataFolderPath = "./data"), + "sql" -> SqlSourceSettings( + name = "s", + sourceUri = "urn:s", + databaseUrl = "jdbc:h2:mem:t", + username = "u", + password = "p" + ), + "fhir" -> FhirServerSourceSettings(name = "r", sourceUri = "urn:r", serverUrl = "http://onfhir/fhir") + ), + sinkSettings = FhirRepositorySinkSettings(fhirRepoUrl = "http://onfhir/fhir"), + mappings = Seq( + task("from-file"), + task("from-sql", SqlSource(tableName = Some("patients"))), + task("from-fhir", FhirServerSource(resourceType = "Patient")) + ) + ) + + val restored = roundTrip(job) + restored.sourceSettings("file") shouldBe a[FileSystemSourceSettings] + restored.sourceSettings("sql") shouldBe a[SqlSourceSettings] + restored.sourceSettings("fhir") shouldBe a[FhirServerSourceSettings] + restored.sinkSettings shouldBe a[FhirRepositorySinkSettings] + restored.mappings.map(_.sourceBinding("source").getClass) shouldBe + job.mappings.map(_.sourceBinding("source").getClass) + } + + it should "restore the sink security settings" in { + val job = FhirMappingJob( + sourceSettings = Map("file" -> FileSystemSourceSettings(name = "f", sourceUri = "urn:f", dataFolderPath = "./d")), + sinkSettings = FhirRepositorySinkSettings( + fhirRepoUrl = "http://secured/fhir", + securitySettings = Some(BasicAuthenticationSettings("user", "secret")) + ), + mappings = Seq(task("a")) + ) + + val restored = roundTrip(job).sinkSettings.asInstanceOf[FhirRepositorySinkSettings] + restored.securitySettings shouldBe Some(BasicAuthenticationSettings("user", "secret")) + } + + it should "restore a file sink with its content type and options" in { + val sink = FileSystemSinkSettings( + path = "./out", + contentType = SinkContentTypes.CSV, + options = Map("header" -> "true") + ) + val job = FhirMappingJob( + sourceSettings = Map("file" -> FileSystemSourceSettings(name = "f", sourceUri = "urn:f", dataFolderPath = "./d")), + sinkSettings = sink, + mappings = Seq(task("a")) + ) + roundTrip(job).sinkSettings shouldBe sink + } + + it should "reject a job whose mappingTasks share a name" in { + val job = FhirMappingJob( + sourceSettings = Map("file" -> FileSystemSourceSettings(name = "f", sourceUri = "urn:f", dataFolderPath = "./d")), + sinkSettings = FhirRepositorySinkSettings(fhirRepoUrl = "http://onfhir/fhir"), + mappings = Seq(task("duplicated"), task("duplicated")) + ) + val file = Files.createTempFile("ignifyr-job", ".json") + FhirMappingJobFormatter.saveMappingJobToFile(job, file.toString) + + val thrown = the[MappingException] thrownBy FhirMappingJobFormatter.readMappingJobFromFile(file.toString) + thrown.getMessage should include("duplicated") + thrown.getMessage should include("unique name") + } +} diff --git a/ignifyr-engine/src/test/scala/io/ignifyr/test/engine/util/SparkUtilTest.scala b/ignifyr-engine/src/test/scala/io/ignifyr/test/engine/util/SparkUtilTest.scala new file mode 100644 index 00000000..bf3c948a --- /dev/null +++ b/ignifyr-engine/src/test/scala/io/ignifyr/test/engine/util/SparkUtilTest.scala @@ -0,0 +1,45 @@ +package io.ignifyr.test.engine.util + +import io.ignifyr.engine.util.SparkUtil +import org.scalatest.flatspec.AnyFlatSpec +import org.scalatest.matchers.should.Matchers + +import java.io.File +import java.nio.file.{Files, Path, Paths} + +/** + * Covers the commit-directory bookkeeping `FileStreamInputArchiver` drives on a timer. The paths below + * are the ones the archiver hits before Spark has written anything: a checkpoint directory that does + * not exist yet, and one that exists but holds only Spark's own dotted metadata files. + */ +class SparkUtilTest extends AnyFlatSpec with Matchers { + + private def tempDirectory(): File = Files.createTempDirectory("ignifyr-spark-util-test").toFile + + private def touch(directory: File, name: String): Path = + Files.createFile(Paths.get(directory.getAbsolutePath, name)) + + "getLastCommitOffset" should "return the highest commit file name" in { + val commitDirectory = tempDirectory() + Seq("0", "1", "2", "10").foreach(touch(commitDirectory, _)) + SparkUtil.getLastCommitOffset(commitDirectory) shouldBe 10 + } + + it should "ignore Spark's dotted metadata files" in { + val commitDirectory = tempDirectory() + touch(commitDirectory, "0") + touch(commitDirectory, ".0.crc") + SparkUtil.getLastCommitOffset(commitDirectory) shouldBe 0 + } + + // -1 leaves the archiver's Range.inclusive(lastProcessed + 1, offset) empty, i.e. "nothing to archive". + it should "return -1 for a directory holding no commit file" in { + SparkUtil.getLastCommitOffset(tempDirectory()) shouldBe -1 + } + + it should "return -1 for a directory that does not exist" in { + val missing = Paths.get(tempDirectory().getAbsolutePath, "no-such-checkpoint").toFile + missing should not(exist) + SparkUtil.getLastCommitOffset(missing) shouldBe -1 + } +} diff --git a/ignifyr-observability/src/test/scala/io/ignifyr/observability/logback/MapMarkerToLogstashMarkerEncoderSpec.scala b/ignifyr-observability/src/test/scala/io/ignifyr/observability/logback/MapMarkerToLogstashMarkerEncoderSpec.scala index 9ec7c1da..496e2a94 100644 --- a/ignifyr-observability/src/test/scala/io/ignifyr/observability/logback/MapMarkerToLogstashMarkerEncoderSpec.scala +++ b/ignifyr-observability/src/test/scala/io/ignifyr/observability/logback/MapMarkerToLogstashMarkerEncoderSpec.scala @@ -1,20 +1,96 @@ package io.ignifyr.observability.logback +import ch.qos.logback.classic.spi.LoggingEvent +import ch.qos.logback.classic.{Level, LoggerContext} +import ch.qos.logback.more.appenders.marker.MapMarker import net.logstash.logback.encoder.LogstashEncoder import org.scalatest.flatspec.AnyFlatSpec import org.scalatest.matchers.should.Matchers +import java.nio.charset.StandardCharsets +import java.util + /** - * Smoke test: the encoder loads and is a [[LogstashEncoder]], so an enterprise logback.xml can - * reference it as an encoder. Its marker-conversion behaviour is exercised end-to-end when the - * server boots its Fluentd/Logstash logback pipeline. + * This encoder is the whole of the observability module's logic and the only thing that gets the engine's + * structured execution markers into Elasticsearch: the engine logs a `MapMarker` (a plain map), which + * Logstash's encoder does not understand, so this class rewrites it into a `LogstashMarker` before + * delegating. If the rewrite silently stopped happening, the Kibana "Executions" dashboard would go blank + * while every log line still looked fine. + * + * The class is referenced by name from the server's logback.xml, so it is not reachable from Scala. */ class MapMarkerToLogstashMarkerEncoderSpec extends AnyFlatSpec with Matchers { + private val loggerContext = new LoggerContext() + + private def startedEncoder: MapMarkerToLogstashMarkerEncoder = { + val encoder = new MapMarkerToLogstashMarkerEncoder + encoder.setContext(loggerContext) + encoder.start() + encoder + } + + /** A warn-level event carrying the given marker, shaped like the ones `SinkHandler` emits. */ + private def event(marker: org.slf4j.Marker): LoggingEvent = { + val loggingEvent = new LoggingEvent() + loggingEvent.setLoggerName("io.ignifyr.engine.data.write.SinkHandler") + loggingEvent.setLevel(Level.WARN) + loggingEvent.setMessage("Mapping failure") + loggingEvent.setTimeStamp(System.currentTimeMillis()) + loggingEvent.setLoggerContextRemoteView(loggerContext.getLoggerContextRemoteView) + loggingEvent.setMarker(marker) + loggingEvent + } + + private def mapMarker(entries: (String, Any)*): MapMarker = { + val map: util.Map[String, Any] = new util.HashMap[String, Any]() + entries.foreach { case (key, value) => map.put(key, value) } + new MapMarker("marker", map) + } + + private def encode(marker: org.slf4j.Marker): String = + new String(startedEncoder.encode(event(marker)), StandardCharsets.UTF_8) + behavior of "MapMarkerToLogstashMarkerEncoder" it should "be a LogstashEncoder that instantiates without configuration" in { - val encoder = new MapMarkerToLogstashMarkerEncoder - encoder shouldBe a[LogstashEncoder] + new MapMarkerToLogstashMarkerEncoder shouldBe a[LogstashEncoder] + } + + // The load-bearing assertion: without the MapMarker -> LogstashMarker rewrite these keys would not be + // top-level fields of the emitted JSON, and the dashboard indexes them as top-level fields. + it should "lift the MapMarker entries into top-level JSON fields" in { + val json = encode(mapMarker("jobId" -> "job-1", "executionId" -> "exec-1", "errorCode" -> "INVALID_INPUT")) + json should include("\"jobId\":\"job-1\"") + json should include("\"executionId\":\"exec-1\"") + json should include("\"errorCode\":\"INVALID_INPUT\"") + } + + it should "keep the standard log fields alongside the marker entries" in { + val json = encode(mapMarker("jobId" -> "job-1")) + json should include("\"level\":\"WARN\"") + json should include("\"message\":\"Mapping failure\"") + json should include("\"logger_name\":\"io.ignifyr.engine.data.write.SinkHandler\"") + } + + it should "preserve the value types of the marker entries" in { + val json = encode(mapMarker("numOfWritten" -> 12, "isStreaming" -> true)) + json should include("\"numOfWritten\":12") + json should include("\"isStreaming\":true") + } + + it should "encode an event with no marker at all" in { + val json = new String(startedEncoder.encode(event(null)), StandardCharsets.UTF_8) + json should include("\"message\":\"Mapping failure\"") + } + + it should "leave a marker that is not a MapMarker to the delegate" in { + val json = encode(org.slf4j.MarkerFactory.getMarker("PLAIN_MARKER")) + json should include("\"message\":\"Mapping failure\"") + } + + it should "encode an empty MapMarker without emitting spurious fields" in { + val json = encode(mapMarker()) + json should include("\"message\":\"Mapping failure\"") } } diff --git a/ignifyr-redcap/src/test/scala/io/ignifyr/redcap/RedCapUtilDataTypeSpec.scala b/ignifyr-redcap/src/test/scala/io/ignifyr/redcap/RedCapUtilDataTypeSpec.scala new file mode 100644 index 00000000..72042a19 --- /dev/null +++ b/ignifyr-redcap/src/test/scala/io/ignifyr/redcap/RedCapUtilDataTypeSpec.scala @@ -0,0 +1,256 @@ +package io.ignifyr.redcap + +import io.onfhir.api.FHIR_DATA_TYPES +import io.onfhir.definitions.common.model.SimpleStructureDefinition +import org.scalatest.flatspec.AnyFlatSpec +import org.scalatest.matchers.should.Matchers + +import javax.ws.rs.BadRequestException + +/** + * The REDCap field type + text-validation type -> FHIR data type table, exercised through the pure + * entry point `extractSchemasAsSchemaDefinitions`. This is the widest lookup table in the repo and the + * only thing standing between a data dictionary and the column types every downstream mapping reads, so + * a wrong entry produces a schema that parses but types its data incorrectly — never an error. + */ +class RedCapUtilDataTypeSpec extends AnyFlatSpec with Matchers { + + private val definitionRootUrl = "https://ignifyr.io/fhir" + + /** One data-dictionary row, with only the columns the extractor actually reads. */ + private def row( + fieldType: String, + textValidationType: Option[String] = None, + variableName: String = "field1", + formName: String = "instrument", + required: String = "", + fieldLabel: String = "A label", + fieldNotes: Option[String] = None + ): Map[String, String] = + Map( + RedCapDataDictionaryColumns.FORM_NAME -> formName, + RedCapDataDictionaryColumns.VARIABLE_FIELD_NAME -> variableName, + RedCapDataDictionaryColumns.FIELD_TYPE -> fieldType, + RedCapDataDictionaryColumns.FIELD_LABEL -> fieldLabel, + RedCapDataDictionaryColumns.REQUIRED_FIELD -> required + ) ++ + textValidationType.map(RedCapDataDictionaryColumns.TEXT_VALIDATION_TYPE -> _) ++ + fieldNotes.map(RedCapDataDictionaryColumns.FIELD_NOTES -> _) + + /** The extracted field definitions of the single form, excluding the injected record-identifier field. */ + private def fieldsOf(rows: Map[String, String]*): Seq[SimpleStructureDefinition] = + RedCapUtil + .extractSchemasAsSchemaDefinitions(rows, definitionRootUrl, recordIdField = "record_id") + .head + .fieldDefinitions + .get + .filterNot(_.id == "record_id") + + private def fhirTypeOf(fieldType: String, textValidationType: Option[String] = None): String = + fieldsOf(row(fieldType, textValidationType)).head.dataTypes.get.head.dataType + + "getDataType" should "map every date text-validation type onto a FHIR date" in { + Seq( + RedCapTextValidationTypes.DATE_DMY, + RedCapTextValidationTypes.DATE_MDY, + RedCapTextValidationTypes.DATE_YMD + ).foreach(validation => fhirTypeOf(RedCapDataTypes.TEXT, Some(validation)) shouldBe FHIR_DATA_TYPES.DATE) + } + + it should "map every datetime text-validation type onto a FHIR dateTime" in { + Seq( + RedCapTextValidationTypes.DATETIME_DMY, + RedCapTextValidationTypes.DATETIME_MDY, + RedCapTextValidationTypes.DATETIME_YMD, + RedCapTextValidationTypes.DATETIME_SECOND_DMY, + RedCapTextValidationTypes.DATETIME_SECONDS_MDY, + RedCapTextValidationTypes.DATETIME_SECONDS_YMD + ).foreach(validation => fhirTypeOf(RedCapDataTypes.TEXT, Some(validation)) shouldBe FHIR_DATA_TYPES.DATETIME) + } + + it should "map both time text-validation types onto a FHIR time" in { + Seq(RedCapTextValidationTypes.TIME, RedCapTextValidationTypes.TIME_MM_SS) + .foreach(validation => fhirTypeOf(RedCapDataTypes.TEXT, Some(validation)) shouldBe FHIR_DATA_TYPES.TIME) + } + + it should "map the numeric text-validation types onto FHIR integer and decimal" in { + fhirTypeOf(RedCapDataTypes.TEXT, Some(RedCapTextValidationTypes.INTEGER)) shouldBe FHIR_DATA_TYPES.INTEGER + fhirTypeOf(RedCapDataTypes.TEXT, Some(RedCapTextValidationTypes.NUMBER)) shouldBe FHIR_DATA_TYPES.DECIMAL + fhirTypeOf(RedCapDataTypes.TEXT, Some(RedCapTextValidationTypes.NUMBER_2DP)) shouldBe FHIR_DATA_TYPES.DECIMAL + } + + it should "map the free-form text validations onto a FHIR string" in { + Seq( + RedCapTextValidationTypes.EMAIL, + RedCapTextValidationTypes.PHONE, + RedCapTextValidationTypes.ZIP_CODE, + RedCapTextValidationTypes.POSTAL_CODE_GERMANY + ).foreach(validation => fhirTypeOf(RedCapDataTypes.TEXT, Some(validation)) shouldBe FHIR_DATA_TYPES.STRING) + } + + // REDCap leaves the validation column empty for an unvalidated text field, and the column may be absent + // altogether in an older export. Both mean "plain string". + it should "map an empty and an absent text validation onto a FHIR string" in { + fhirTypeOf(RedCapDataTypes.TEXT, Some("")) shouldBe FHIR_DATA_TYPES.STRING + fhirTypeOf(RedCapDataTypes.TEXT, None) shouldBe FHIR_DATA_TYPES.STRING + } + + it should "treat a notes field like a text field" in { + fhirTypeOf(RedCapDataTypes.NOTES, None) shouldBe FHIR_DATA_TYPES.STRING + fhirTypeOf(RedCapDataTypes.NOTES, Some(RedCapTextValidationTypes.DATE_YMD)) shouldBe FHIR_DATA_TYPES.DATE + } + + it should "map the multiple-choice field types onto a FHIR code" in { + Seq(RedCapDataTypes.RADIO, RedCapDataTypes.DROPDOWN, RedCapDataTypes.CHECKBOXES, RedCapDataTypes.SQL) + .foreach(fieldType => fhirTypeOf(fieldType) shouldBe FHIR_DATA_TYPES.CODE) + } + + it should "map the boolean field types onto a FHIR boolean" in { + Seq(RedCapDataTypes.YES_NO, RedCapDataTypes.TRUE_FALSE) + .foreach(fieldType => fhirTypeOf(fieldType) shouldBe FHIR_DATA_TYPES.BOOLEAN) + } + + it should "map calc onto a decimal and slider onto an integer" in { + fhirTypeOf(RedCapDataTypes.CALC) shouldBe FHIR_DATA_TYPES.DECIMAL + fhirTypeOf(RedCapDataTypes.SLIDER) shouldBe FHIR_DATA_TYPES.INTEGER + } + + it should "map a file onto base64Binary, or a Signature when it is signed" in { + fhirTypeOf(RedCapDataTypes.FILE, None) shouldBe FHIR_DATA_TYPES.BASE64BINARY + fhirTypeOf(RedCapDataTypes.FILE, Some("")) shouldBe FHIR_DATA_TYPES.BASE64BINARY + fhirTypeOf(RedCapDataTypes.FILE, Some(RedCapTextValidationTypes.SIGNATURE)) shouldBe FHIR_DATA_TYPES.SIGNATURE + } + + it should "attach the canonical profile url of the mapped type" in { + fieldsOf(row(RedCapDataTypes.YES_NO)).head.dataTypes.get.head.profiles.get.head shouldBe + s"http://hl7.org/fhir/StructureDefinition/${FHIR_DATA_TYPES.BOOLEAN}" + } + + it should "reject an unknown field type" in { + val thrown = the[IllegalArgumentException] thrownBy fieldsOf(row("hologram")) + thrown.getMessage should include("Invalid data type: hologram") + } + + it should "reject an unknown text validation type" in { + val thrown = the[IllegalArgumentException] thrownBy fieldsOf(row(RedCapDataTypes.TEXT, Some("runes"))) + thrown.getMessage should include("Invalid text validation type for texts: runes") + } + + it should "reject an unknown text validation type on a file field" in { + val thrown = the[IllegalArgumentException] thrownBy fieldsOf(row(RedCapDataTypes.FILE, Some("runes"))) + thrown.getMessage should include("Invalid text validation type for files: runes") + } + + "getCardinality" should "make a checkbox field repeating and everything else single" in { + val checkbox = fieldsOf(row(RedCapDataTypes.CHECKBOXES)).head + checkbox.isArray shouldBe true + checkbox.maxCardinality shouldBe None // "*" + + val radio = fieldsOf(row(RedCapDataTypes.RADIO)).head + radio.isArray shouldBe false + radio.maxCardinality shouldBe Some(1) + } + + it should "require a field only when the dictionary marks it with y" in { + fieldsOf(row(RedCapDataTypes.TEXT, required = "y")).head.minCardinality shouldBe 1 + fieldsOf(row(RedCapDataTypes.TEXT, required = "")).head.minCardinality shouldBe 0 + fieldsOf(row(RedCapDataTypes.TEXT, required = "n")).head.minCardinality shouldBe 0 + } + + "extractSchemasAsSchemaDefinitions" should "produce one schema per form, named after it" in { + val schemas = RedCapUtil.extractSchemasAsSchemaDefinitions( + Seq( + row(RedCapDataTypes.TEXT, variableName = "a", formName = "demographics"), + row(RedCapDataTypes.TEXT, variableName = "b", formName = "vitals") + ), + definitionRootUrl, + recordIdField = "record_id" + ) + schemas.map(_.name) should contain theSameElementsAs Seq("demographics", "vitals") + schemas.map(_.id) should contain theSameElementsAs Seq("Demographics", "Vitals") + schemas.map(_.url) should contain(s"$definitionRootUrl/StructureDefinition/Vitals") + } + + // A descriptive field displays text and returns no data, so REDCap's export omits it and so must the schema. + it should "omit a descriptive field entirely" in { + val fields = fieldsOf( + row(RedCapDataTypes.DESCRIPTIVE, variableName = "banner"), + row(RedCapDataTypes.TEXT, variableName = "name") + ) + fields.map(_.id) shouldBe Seq("name") + } + + it should "inject the record identifier field when the form does not declare it" in { + val schema = RedCapUtil + .extractSchemasAsSchemaDefinitions( + Seq(row(RedCapDataTypes.TEXT, variableName = "name")), + definitionRootUrl, + "record_id" + ) + .head + val recordId = schema.fieldDefinitions.get.head + recordId.id shouldBe "record_id" + recordId.path shouldBe "Instrument.record_id" + recordId.short shouldBe Some("Record Identifier") + recordId.dataTypes.get.head.dataType shouldBe FHIR_DATA_TYPES.STRING + recordId.minCardinality shouldBe 0 + recordId.maxCardinality shouldBe Some(1) + } + + it should "not inject the record identifier field twice when the form already declares it" in { + val schema = RedCapUtil + .extractSchemasAsSchemaDefinitions( + Seq(row(RedCapDataTypes.TEXT, variableName = "record_id")), + definitionRootUrl, + "record_id" + ) + .head + schema.fieldDefinitions.get.count(_.id == "record_id") shouldBe 1 + } + + /* + * `recordIdField` defaults to "" and the CLI (`extract-redcap-schemas`) takes that default — only the + * server import route passes a real value from a query parameter. The result is an injected field with + * an empty id and a trailing-dot path. Pinned rather than changed: CLI-extracted schemas already on + * disk carry exactly this shape, so altering it is a migration decision, not a test fix. + */ + it should "inject an unnamed record identifier field when none is supplied (the CLI default)" in { + val schema = RedCapUtil + .extractSchemasAsSchemaDefinitions(Seq(row(RedCapDataTypes.TEXT, variableName = "name")), definitionRootUrl, "") + .head + val recordId = schema.fieldDefinitions.get.head + recordId.id shouldBe "" + recordId.path shouldBe "Instrument." + } + + // REDCap exports the dictionary as UTF-8-with-BOM, so the first column name arrives BOM-prefixed. + it should "fall back to the BOM-prefixed variable name column" in { + val bomRow = Map( + RedCapDataDictionaryColumns.FORM_NAME -> "instrument", + RedCapDataDictionaryColumns.VARIABLE_FIELD_NAME_WITH_BOM -> "name", + RedCapDataDictionaryColumns.FIELD_TYPE -> RedCapDataTypes.TEXT, + RedCapDataDictionaryColumns.FIELD_LABEL -> "Name", + RedCapDataDictionaryColumns.REQUIRED_FIELD -> "" + ) + fieldsOf(bomRow).map(_.id) shouldBe Seq("name") + } + + it should "reject a dictionary with no variable name column at all" in { + val noNameRow = Map( + RedCapDataDictionaryColumns.FORM_NAME -> "instrument", + RedCapDataDictionaryColumns.FIELD_TYPE -> RedCapDataTypes.TEXT, + RedCapDataDictionaryColumns.FIELD_LABEL -> "Name", + RedCapDataDictionaryColumns.REQUIRED_FIELD -> "" + ) + a[BadRequestException] should be thrownBy fieldsOf(noNameRow) + } + + it should "use the field label as the definition when there are no field notes" in { + val withNotes = fieldsOf(row(RedCapDataTypes.TEXT, fieldLabel = "Label", fieldNotes = Some("Notes"))).head + withNotes.short shouldBe Some("Label") + withNotes.definition shouldBe Some("Notes") + + val withoutNotes = fieldsOf(row(RedCapDataTypes.TEXT, fieldLabel = "Label")).head + withoutNotes.definition shouldBe Some("Label") + } +} diff --git a/ignifyr-runtime-scheduling/src/main/scala/io/ignifyr/runtime/scheduling/Cron4jSchedulerProvider.scala b/ignifyr-runtime-scheduling/src/main/scala/io/ignifyr/runtime/scheduling/Cron4jSchedulerProvider.scala index b2f1c223..033b9370 100644 --- a/ignifyr-runtime-scheduling/src/main/scala/io/ignifyr/runtime/scheduling/Cron4jSchedulerProvider.scala +++ b/ignifyr-runtime-scheduling/src/main/scala/io/ignifyr/runtime/scheduling/Cron4jSchedulerProvider.scala @@ -155,7 +155,7 @@ class Cron4jSchedulerProvider extends SchedulerProvider { * Reads the latest synchronization time point for the job from its last-sync file, returning the * time range (lastSyncTime, now) — or (startTime, now) when no sync has happened yet. */ - private def getScheduledTimeRange( + private[scheduling] def getScheduledTimeRange( mappingJobId: String, folderUri: URI, startTime: LocalDateTime diff --git a/ignifyr-runtime-scheduling/src/test/scala/io/ignifyr/runtime/scheduling/ScheduledTimeRangeSpec.scala b/ignifyr-runtime-scheduling/src/test/scala/io/ignifyr/runtime/scheduling/ScheduledTimeRangeSpec.scala new file mode 100644 index 00000000..1e82cf1d --- /dev/null +++ b/ignifyr-runtime-scheduling/src/test/scala/io/ignifyr/runtime/scheduling/ScheduledTimeRangeSpec.scala @@ -0,0 +1,102 @@ +package io.ignifyr.runtime.scheduling + +import org.scalatest.flatspec.AnyFlatSpec +import org.scalatest.matchers.should.Matchers + +import java.io.{File, FileWriter} +import java.nio.file.{Files, Paths} +import java.time.LocalDateTime +import java.time.format.DateTimeParseException + +/** + * The incremental-sync bookkeeping behind every scheduled run. Each fire reads the previous + * synchronisation instant from `/scheduler/.txt` and syncs `(lastSync, now)`; + * afterwards it appends the new instant to that same file. + * + * Getting the read wrong is silent in both directions — too early a lower bound re-imports data that was + * already mapped, too late a one skips a window forever — and the long-tier `SchedulingTest` only ever + * exercises a single fire against a fresh directory, so none of the states below are reached there. + */ +class ScheduledTimeRangeSpec extends AnyFlatSpec with Matchers { + + private val provider = new Cron4jSchedulerProvider + private val epoch = LocalDateTime.of(1970, 1, 1, 0, 0) + + /** A fresh, empty scheduler state directory. */ + private def schedulerFolder(): File = + Files.createTempDirectory("ignifyr-scheduler-state").toFile + + /** Appends lines the way `runnableMappingJob` does: the instant's `toString`, one per line. */ + private def writeSyncFile(folder: File, jobId: String, instants: LocalDateTime*): Unit = { + val writer = new FileWriter(s"${folder.toURI.getPath}/$jobId.txt", true) + try instants.foreach(instant => writer.write(instant.toString + "\n")) + finally writer.close() + } + + "getScheduledTimeRange" should "start from the job's initial time when no sync has happened yet" in { + val (from, to) = provider.getScheduledTimeRange("job-1", schedulerFolder().toURI, epoch) + from shouldBe epoch + to should be > from + } + + it should "resume from the recorded synchronisation instant" in { + val folder = schedulerFolder() + val lastSync = LocalDateTime.of(2026, 3, 1, 12, 0) + writeSyncFile(folder, "job-1", lastSync) + + provider.getScheduledTimeRange("job-1", folder.toURI, epoch)._1 shouldBe lastSync + } + + // The file is append-only: every completed run adds a line, so only the last one is the current state. + it should "take the last line of an append-only file" in { + val folder = schedulerFolder() + writeSyncFile( + folder, + "job-1", + LocalDateTime.of(2026, 3, 1, 12, 0), + LocalDateTime.of(2026, 3, 1, 13, 0), + LocalDateTime.of(2026, 3, 1, 14, 0) + ) + + provider.getScheduledTimeRange("job-1", folder.toURI, epoch)._1 shouldBe LocalDateTime.of(2026, 3, 1, 14, 0) + } + + it should "keep each job's synchronisation state separate" in { + val folder = schedulerFolder() + writeSyncFile(folder, "job-1", LocalDateTime.of(2026, 3, 1, 12, 0)) + writeSyncFile(folder, "job-2", LocalDateTime.of(2026, 3, 2, 12, 0)) + + provider.getScheduledTimeRange("job-1", folder.toURI, epoch)._1 shouldBe LocalDateTime.of(2026, 3, 1, 12, 0) + provider.getScheduledTimeRange("job-2", folder.toURI, epoch)._1 shouldBe LocalDateTime.of(2026, 3, 2, 12, 0) + provider.getScheduledTimeRange("job-3", folder.toURI, epoch)._1 shouldBe epoch + } + + it should "create the state directory when it does not exist yet" in { + val missing = Paths.get(schedulerFolder().getAbsolutePath, "scheduler").toFile + missing should not(exist) + + provider.getScheduledTimeRange("job-1", missing.toURI, epoch)._1 shouldBe epoch + missing should exist + } + + it should "end the range at the current time" in { + val before = LocalDateTime.now() + val (_, to) = provider.getScheduledTimeRange("job-1", schedulerFolder().toURI, epoch) + to should be >= before + to should be <= LocalDateTime.now() + } + + /* + * Pinned rather than changed: only FileNotFoundException is caught, so a sync file that exists but has + * no parsable last line fails the run instead of falling back to `startTime`. A crash between opening + * the writer and writing the line leaves exactly such a zero-byte file, and from then on the job stops + * syncing. Changing that is a scheduling-semantics decision (silently re-syncing from `initialTime` + * could re-import a lot of data), so it is recorded here rather than fixed in passing. + */ + it should "fail rather than fall back when the sync file holds no parsable instant" in { + val folder = schedulerFolder() + Files.createFile(Paths.get(folder.getAbsolutePath, "job-1.txt")) + + a[DateTimeParseException] should be thrownBy provider.getScheduledTimeRange("job-1", folder.toURI, epoch) + } +} diff --git a/ignifyr-runtime-streaming/src/test/scala/io/ignifyr/runtime/streaming/StreamingSinkHandlerTest.scala b/ignifyr-runtime-streaming/src/test/scala/io/ignifyr/runtime/streaming/StreamingSinkHandlerTest.scala index dbee6008..79747660 100644 --- a/ignifyr-runtime-streaming/src/test/scala/io/ignifyr/runtime/streaming/StreamingSinkHandlerTest.scala +++ b/ignifyr-runtime-streaming/src/test/scala/io/ignifyr/runtime/streaming/StreamingSinkHandlerTest.scala @@ -85,5 +85,14 @@ class StreamingSinkHandlerTest extends AnyFlatSpec with BeforeAndAfterAll { // Wait for data generation for 5 seconds and then terminate the query streamingQuery.awaitTermination(5000) streamingQuery.stop() + + // `awaitTermination` rethrowing already proves the query survived; this is the other half of the + // contract — the writer was handed a further chunk *after* the first one threw. Without it the test + // would still pass if the stream produced only the failing chunk and nothing more. + verify(mockWriter, atLeast(2)).write( + ArgumentMatchers.any(), + ArgumentMatchers.any[Dataset[FhirMappingResult]](), + ArgumentMatchers.any() + ) } } diff --git a/ignifyr-rxnorm/pom.xml b/ignifyr-rxnorm/pom.xml index de3cf139..99d2aa01 100644 --- a/ignifyr-rxnorm/pom.xml +++ b/ignifyr-rxnorm/pom.xml @@ -47,6 +47,25 @@ + + + org.scalatest + scalatest-maven-plugin + + + test + + test + + + io.ignifyr.rxnorm + + + + diff --git a/ignifyr-rxnorm/src/test/scala/RxNormApiClientTest.scala b/ignifyr-rxnorm/src/test/scala/RxNormApiClientTest.scala deleted file mode 100644 index 5ed83a90..00000000 --- a/ignifyr-rxnorm/src/test/scala/RxNormApiClientTest.scala +++ /dev/null @@ -1,41 +0,0 @@ -import akka.actor.ActorSystem -import io.onfhir.definitions.common.model.Json4sSupport.formats -import io.ignifyr.rxnorm.RxNormApiClient -import org.scalatest.flatspec.AnyFlatSpec -import org.scalatest.matchers.should.Matchers - -class RxNormApiClientTest extends AnyFlatSpec with Matchers { - implicit val actorSystem = ActorSystem("test") - val client = new RxNormApiClient("https://rxnav.nlm.nih.gov", 10) - - "RxNormApiClient" should "get corresponding RxNorm CUI for given NDC" in { - client.findRxConceptIdByNdc("63739054410") shouldBe Seq("313096") - } - - it should "not found a non existent NDC" in { - client.findRxConceptIdByNdc("123") shouldBe Nil - } - - it should "get the details of the medication with with concept id" in { - client.getRxcuiHistoryStatus("603748").isDefined shouldBe true - } - - it should "get the ingredients for given RxNorm concept id" in { - val result = client.getIngredientProperties("476556") - result.length shouldBe 2 - (result.head \ "Active_ingredient_RxCUI").extract[String] shouldBe "276237" - (result.head \ "Active_ingredient_name").extract[String] shouldBe "emtricitabine" - (result.head \ "Numerator_Value").extract[Int] shouldBe 200 - (result.head \ "Numerator_Units").extract[String] shouldBe "MG" - } - - it should "not found for a non existent id" in { - val result = client.getIngredientProperties("476") - result.length shouldBe 0 - } - - it should "get the ATC code for given RxNorm concept id" in { - client.getAtcCode("276237") shouldBe Seq("J05AF09") - } - -} diff --git a/ignifyr-rxnorm/src/test/scala/RxNormApiFunctionLibraryTest.scala b/ignifyr-rxnorm/src/test/scala/RxNormApiFunctionLibraryTest.scala deleted file mode 100644 index abd6d87a..00000000 --- a/ignifyr-rxnorm/src/test/scala/RxNormApiFunctionLibraryTest.scala +++ /dev/null @@ -1,15 +0,0 @@ -import io.onfhir.path.FhirPathEvaluator -import io.ignifyr.rxnorm.RxNormApiFunctionLibraryFactory -import org.json4s.JsonAST.JNull -import org.scalatest.flatspec.AnyFlatSpec -import org.scalatest.matchers.should.Matchers - -class RxNormApiFunctionLibraryTest extends AnyFlatSpec with Matchers { - val rxNormApiFunctionLibraryFactory = new RxNormApiFunctionLibraryFactory("https://rxnav.nlm.nih.gov", 10) - val fhirPathEvaluator = - FhirPathEvaluator.apply().withDefaultFunctionLibraries().withFunctionLibrary("rxn", rxNormApiFunctionLibraryFactory) - - "RxNormApiFunctionLibrary" should "handle findRxConceptIdsByNdc" in { - fhirPathEvaluator.evaluateOptionalString("rxn:findRxConceptIdsByNdc(63739054410)", JNull) shouldBe Some("313096") - } -} diff --git a/ignifyr-rxnorm/src/test/scala/io/ignifyr/rxnorm/RxNormApiClientTest.scala b/ignifyr-rxnorm/src/test/scala/io/ignifyr/rxnorm/RxNormApiClientTest.scala new file mode 100644 index 00000000..4d86e2aa --- /dev/null +++ b/ignifyr-rxnorm/src/test/scala/io/ignifyr/rxnorm/RxNormApiClientTest.scala @@ -0,0 +1,104 @@ +package io.ignifyr.rxnorm + +import io.onfhir.definitions.common.model.Json4sSupport.formats +import io.onfhir.path.FhirPathException +import org.scalatest.flatspec.AnyFlatSpec +import org.scalatest.matchers.should.Matchers + +/** + * Covers how the client reads RxNorm's responses: which JSON path each call picks its answer out of, + * and what an absent or malformed answer turns into. The bodies below are trimmed real RxNorm + * responses served by [[RxNormApiStub]]. + */ +class RxNormApiClientTest extends AnyFlatSpec with Matchers with RxNormApiStub { + + override protected val cannedResponses: Map[String, String] = Map( + "/REST/ndcproperties.json?id=63739054410&ndcstatus=ALL" -> + """{"ndcPropertyList":{"ndcProperty":[{"ndcItem":"63739-0544-10","rxcui":"313096"}]}}""", + // RxNorm answers an unknown NDC with an empty object rather than a 404. + "/REST/ndcproperties.json?id=123&ndcstatus=ALL" -> "{}", + // A concept whose rxcui entries are placeholders; the client filters "" and "/" out. + "/REST/ndcproperties.json?id=00000000000&ndcstatus=ALL" -> + """{"ndcPropertyList":{"ndcProperty":[{"rxcui":""},{"rxcui":"/"},{"rxcui":"111"}]}}""", + "/REST/rxcui.json?name=aspirin&search=2" -> """{"idGroup":{"name":"aspirin","rxnormId":["1191"]}}""", + "/REST/rxcui.json?name=nothing&search=2" -> """{"idGroup":{"name":"nothing"}}""", + "/REST/rxcui/603748/historystatus.json" -> + """{"rxcuiStatusHistory":{"metaData":{"status":"Active"},"attributes":{"rxcui":"603748"}}}""", + "/REST/rxcui/999999999/historystatus.json" -> """{"rxcuiStatusHistory":{"metaData":{"status":"UNKNOWN"}}}""", + "/REST/rxcui/476556/allProperties.json?prop=attributes" -> + """{"propConceptGroup":{"propConcept":[ + |{"propCategory":"ATTRIBUTES","propName":"Active_ingredient_RxCUI","propValue":"276237"}, + |{"propCategory":"ATTRIBUTES","propName":"Active_ingredient_RxCUI","propValue":"282401"}, + |{"propCategory":"ATTRIBUTES","propName":"Active_ingredient_name","propValue":"emtricitabine"}, + |{"propCategory":"ATTRIBUTES","propName":"Active_ingredient_name","propValue":"tenofovir disoproxil fumarate"}, + |{"propCategory":"ATTRIBUTES","propName":"Numerator_Value","propValue":"200"}, + |{"propCategory":"ATTRIBUTES","propName":"Numerator_Value","propValue":"300"}, + |{"propCategory":"ATTRIBUTES","propName":"Numerator_Units","propValue":"MG"}, + |{"propCategory":"ATTRIBUTES","propName":"Numerator_Units","propValue":"MG"}, + |{"propCategory":"ATTRIBUTES","propName":"Denominator_Value","propValue":"1"}, + |{"propCategory":"ATTRIBUTES","propName":"Denominator_Value","propValue":"1"}, + |{"propCategory":"ATTRIBUTES","propName":"Denominator_Units","propValue":"EA"}, + |{"propCategory":"ATTRIBUTES","propName":"Denominator_Units","propValue":"EA"} + |]}}""".stripMargin, + "/REST/rxcui/476/allProperties.json?prop=attributes" -> "{}", + "/REST/rxcui/276237/property.json?propName=ATC" -> + """{"propConceptGroup":{"propConcept":[{"propName":"ATC","propValue":"J05AF09"}]}}""" + ) + + private lazy val client = RxNormApiClient(rxNormRootUrl, timeoutInSec = 10) + + "RxNormApiClient" should "get the corresponding RxNorm CUI for a given NDC" in { + client.findRxConceptIdByNdc("63739054410") shouldBe Seq("313096") + } + + it should "return nothing for a non-existent NDC" in { + client.findRxConceptIdByNdc("123") shouldBe Nil + } + + it should "drop the empty and slash placeholder concept ids" in { + client.findRxConceptIdByNdc("00000000000") shouldBe Seq("111") + } + + it should "find a concept id by drug name" in { + client.findRxConceptIdByName("aspirin") shouldBe Some("1191") + } + + it should "return nothing when a drug name resolves to no concept" in { + client.findRxConceptIdByName("nothing") shouldBe None + } + + it should "get the history status of a known concept" in { + client.getRxcuiHistoryStatus("603748").isDefined shouldBe true + } + + // RxNorm answers for an unknown concept too, with status UNKNOWN; the client must not treat that as a hit. + it should "report no history status for a concept RxNorm does not know" in { + client.getRxcuiHistoryStatus("999999999") shouldBe None + } + + it should "get the ingredients of a drug, pairing each property by position" in { + val result = client.getIngredientProperties("476556") + result.length shouldBe 2 + (result.head \ "Active_ingredient_RxCUI").extract[String] shouldBe "276237" + (result.head \ "Active_ingredient_name").extract[String] shouldBe "emtricitabine" + (result.head \ "Numerator_Value").extract[Int] shouldBe 200 + (result.head \ "Numerator_Units").extract[String] shouldBe "MG" + (result(1) \ "Active_ingredient_name").extract[String] shouldBe "tenofovir disoproxil fumarate" + } + + it should "return no ingredients for a concept that has none" in { + client.getIngredientProperties("476") shouldBe empty + } + + it should "get the ATC code of a concept" in { + client.getAtcCode("276237") shouldBe Seq("J05AF09") + } + + // Any status other than 200 is turned into a FhirPathException naming the root url, because these + // calls run inside FHIRPath evaluation where that message is all the mapping author will see. + it should "raise a FhirPathException naming the root url when RxNorm does not answer with 200" in { + val thrown = the[FhirPathException] thrownBy client.getAtcCode("no-such-concept") + thrown.getMessage should include(rxNormRootUrl) + thrown.getMessage should include("404") + } +} diff --git a/ignifyr-rxnorm/src/test/scala/io/ignifyr/rxnorm/RxNormApiFunctionLibraryTest.scala b/ignifyr-rxnorm/src/test/scala/io/ignifyr/rxnorm/RxNormApiFunctionLibraryTest.scala new file mode 100644 index 00000000..2e6c4bbf --- /dev/null +++ b/ignifyr-rxnorm/src/test/scala/io/ignifyr/rxnorm/RxNormApiFunctionLibraryTest.scala @@ -0,0 +1,44 @@ +package io.ignifyr.rxnorm + +import io.onfhir.path.FhirPathEvaluator +import org.json4s.JsonAST.JNull +import org.scalatest.flatspec.AnyFlatSpec +import org.scalatest.matchers.should.Matchers + +/** + * The `rxn:` FHIRPath functions, resolved against [[RxNormApiStub]] rather than the live RxNorm API. + * This is the layer a mapping author actually writes against, and it is attached by configuration only + * (`ignifyr.functionLibraries.rxn.className`), so nothing in Scala would notice if a function stopped + * resolving. + */ +class RxNormApiFunctionLibraryTest extends AnyFlatSpec with Matchers with RxNormApiStub { + + override protected val cannedResponses: Map[String, String] = Map( + "/REST/ndcproperties.json?id=63739054410&ndcstatus=ALL" -> + """{"ndcPropertyList":{"ndcProperty":[{"rxcui":"313096"}]}}""", + // The library normalises an NDC to 11 digits before calling, so the stub is keyed on the padded form. + "/REST/ndcproperties.json?id=00000000123&ndcstatus=ALL" -> "{}", + "/REST/rxcui/276237/property.json?propName=ATC" -> + """{"propConceptGroup":{"propConcept":[{"propName":"ATC","propValue":"J05AF09"}]}}""" + ) + + private lazy val evaluator = + FhirPathEvaluator + .apply() + .withDefaultFunctionLibraries() + .withFunctionLibrary("rxn", new RxNormApiFunctionLibraryFactory(rxNormRootUrl, 10)) + + "RxNormApiFunctionLibrary" should "resolve findRxConceptIdsByNdc" in { + evaluator.evaluateOptionalString("rxn:findRxConceptIdsByNdc(63739054410)", JNull) shouldBe Some("313096") + } + + it should "return nothing for an NDC RxNorm does not know" in { + evaluator.evaluateOptionalString("rxn:findRxConceptIdsByNdc(123)", JNull) shouldBe None + } + + // Unlike findRxConceptIdsByNdc, getATC rejects a numeric literal: its rxcui parameter must be a string. + it should "resolve getATC" in { + evaluator.evaluateOptionalString("rxn:getATC('276237')", JNull) shouldBe Some("J05AF09") + } + +} diff --git a/ignifyr-rxnorm/src/test/scala/io/ignifyr/rxnorm/RxNormApiStub.scala b/ignifyr-rxnorm/src/test/scala/io/ignifyr/rxnorm/RxNormApiStub.scala new file mode 100644 index 00000000..8a84bded --- /dev/null +++ b/ignifyr-rxnorm/src/test/scala/io/ignifyr/rxnorm/RxNormApiStub.scala @@ -0,0 +1,63 @@ +package io.ignifyr.rxnorm + +import akka.actor.ActorSystem +import akka.http.scaladsl.Http +import akka.http.scaladsl.model._ +import org.scalatest.{BeforeAndAfterAll, Suite} + +import scala.concurrent.Await +import scala.concurrent.duration.DurationInt + +/** + * A local stand-in for the RxNorm REST API, bound on an ephemeral port for the lifetime of a suite. + * + * The client under test takes its root url as a constructor argument, so pointing it here is the whole + * of the seam — no HTTP interception and no extra dependency (the stub uses the same akka-http the + * client itself calls through). Testing against rxnav.nlm.nih.gov instead would make a green build + * depend on a third party's uptime, and would give no way at all to exercise the not-found and + * non-200 branches. + * + * A suite declares the canned bodies it needs in [[cannedResponses]], keyed by request path plus its + * query parameters sorted by name; anything unlisted is answered with 404, which is what makes an + * unexpected call visible instead of silent. + */ +trait RxNormApiStub extends BeforeAndAfterAll { this: Suite => + + // The client keeps its own singleton ActorSystem; reusing it avoids standing up a second one. + private implicit val actorSystem: ActorSystem = RxNormApiClient.actorSystem + + private var binding: Http.ServerBinding = _ + + /** Root url to hand to the code under test. Valid between `beforeAll` and `afterAll`. */ + protected def rxNormRootUrl: String = s"http://localhost:${binding.localAddress.getPort}" + + /** Canned JSON bodies, keyed as `` or `?` with the names sorted. */ + protected def cannedResponses: Map[String, String] + + override protected def beforeAll(): Unit = { + super.beforeAll() + binding = Await.result( + Http() + .newServerAt("localhost", 0) + .bindSync { request => + cannedResponses.get(keyOf(request)) match { + case Some(body) => HttpResponse(entity = HttpEntity(ContentTypes.`application/json`, body)) + case None => HttpResponse(StatusCodes.NotFound) + } + }, + 10.seconds + ) + } + + override protected def afterAll(): Unit = { + try Await.result(binding.unbind(), 10.seconds) + finally super.afterAll() + } + + /** The client builds some uris with an ordered query map and others by hand, so sort before matching. */ + private def keyOf(request: HttpRequest): String = { + val path = request.uri.path.toString + val parameters = request.uri.query().toMap.toSeq.sorted.map { case (name, value) => s"$name=$value" } + if (parameters.isEmpty) path else s"$path?${parameters.mkString("&")}" + } +} diff --git a/ignifyr-server/src/main/scala/io/ignifyr/server/util/CsvUtil.scala b/ignifyr-server/src/main/scala/io/ignifyr/server/util/CsvUtil.scala index 15b1b0a9..f7e5ee1e 100644 --- a/ignifyr-server/src/main/scala/io/ignifyr/server/util/CsvUtil.scala +++ b/ignifyr-server/src/main/scala/io/ignifyr/server/util/CsvUtil.scala @@ -98,7 +98,10 @@ object CsvUtil { // "header1" is deleted, // "header3" changed to "headerChanged" // "header4" is added - existingContentFuture.map { existingContent => + // flatMap, not map: the write below returns its own Future. Mapping over it would let the returned + // Future complete while the file was still being written, so a caller (the mapping-context header + // endpoint) could answer OK and then read back the old content. + existingContentFuture.flatMap { existingContent => // Create a new list of lists where each list is a row and the first element in the tuples is the header val updatedContent = existingContent.map { row => // iterate each row newHeaders.map { header => // iterate each new header @@ -126,6 +129,7 @@ object CsvUtil { Source(csvContent) .intersperse(ByteString("\n")) .runWith(FileIO.toPath(file.toPath, Set(StandardOpenOption.WRITE, StandardOpenOption.TRUNCATE_EXISTING))) + .map(_ => ()) } } diff --git a/ignifyr-server/src/test/resources/test-mappings/other-observation-mapping.json b/ignifyr-server/src/test/resources/test-mappings/other-observation-mapping.json index e5cf31e5..dc90e02e 100644 --- a/ignifyr-server/src/test/resources/test-mappings/other-observation-mapping.json +++ b/ignifyr-server/src/test/resources/test-mappings/other-observation-mapping.json @@ -175,7 +175,7 @@ "source": "{{%sourceSystem.sourceUri}}" }, "status": "completed", - "category": { + "category": [{ "coding": [ { "system": "http://terminology.hl7.org/CodeSystem/medication-admin-category", @@ -183,19 +183,21 @@ "display": "Inpatient" } ] - }, - "medicationCodeableConcept": { - "coding": [ - { - "system": "http://www.nlm.nih.gov/research/umls/rxnorm", - "code": "{{code}}", - "display": "{{mpp:getConcept(%obsConceptMap, code, 'source_display')}}" - } - ] + }], + "medication": { + "concept": { + "coding": [ + { + "system": "http://www.nlm.nih.gov/research/umls/rxnorm", + "code": "{{code}}", + "display": "{{mpp:getConcept(%obsConceptMap, code, 'source_display')}}" + } + ] + } }, "subject": "{{mpp:createFhirReferenceWithHashedId('Patient', pid)}}", - "context": "{{? mpp:createFhirReferenceWithHashedId('Encounter', encounterId)}}", - "effectiveDateTime": "{{time.utl:toFhirDateTime('yyyy-MM-dd HH:mm:ss')}}", + "encounter": "{{? mpp:createFhirReferenceWithHashedId('Encounter', encounterId)}}", + "occurenceDateTime": "{{time.utl:toFhirDateTime('yyyy-MM-dd HH:mm:ss')}}", "dosage": { "dose": { "value": "{{value.toDecimal()}}", diff --git a/ignifyr-server/src/test/scala/io/ignifyr/server/endpoint/IgnifyrErrorHandlerTest.scala b/ignifyr-server/src/test/scala/io/ignifyr/server/endpoint/IgnifyrErrorHandlerTest.scala new file mode 100644 index 00000000..bcbb8f9a --- /dev/null +++ b/ignifyr-server/src/test/scala/io/ignifyr/server/endpoint/IgnifyrErrorHandlerTest.scala @@ -0,0 +1,87 @@ +package io.ignifyr.server.endpoint + +import akka.http.scaladsl.model.{HttpEntity, HttpMethods, StatusCodes, Uri} +import akka.http.scaladsl.server.Directives._ +import akka.http.scaladsl.server.Route +import akka.http.scaladsl.testkit.ScalatestRouteTest +import io.onfhir.definitions.resource.model +import io.ignifyr.server.common.interceptor.IErrorHandler +import io.ignifyr.server.common.model.{IgnifyrRestCall, RequestTimeout, ResourceNotFound} +import org.scalatest.matchers.should.Matchers +import org.scalatest.wordspec.AnyWordSpec + +/** + * The exception half of the server's error handling. `IgnifyrRejectionHandlerTest` covers *rejections* — + * a different Akka mechanism, reached when no route matches or a directive refuses the request. What is + * covered here is what happens when a route **throws**: every endpoint is wrapped in this handler, so it + * is the last thing standing between an unexpected failure and an empty 500 with a leaked stack trace. + * + * The suite drives a minimal route rather than the real API because no endpoint can be made to throw an + * arbitrary exception on demand. + */ +class IgnifyrErrorHandlerTest extends AnyWordSpec with Matchers with ScalatestRouteTest with IErrorHandler { + + private val restCall = + new IgnifyrRestCall(HttpMethods.GET, Uri("/ignifyr/boom"), "test-request", HttpEntity.Empty) + + /** A route whose only job is to throw whatever the test hands it. */ + private def routeThrowing(exception: Exception): Route = + handleExceptions(exceptionHandler(restCall)) { + path("boom") { + get { + complete { + throw exception + } + } + } + } + + "The error handler" should { + + "answer an unexpected exception with 500 and name the exception type" in { + Get("/boom") ~> routeThrowing(new IllegalStateException("spark session is gone")) ~> check { + status shouldEqual StatusCodes.InternalServerError + val body = responseAs[String] + body should include("Type: https://ignifyr.io/errors/InternalError") + body should include("java.lang.IllegalStateException") + body should include("spark session is gone") + } + } + + // An IgnifyrError already carries its own status; the handler must pass it through untouched rather + // than flattening everything to 500. + "pass an IgnifyrError through with its own status" in { + Get("/boom") ~> routeThrowing(ResourceNotFound("Job not found", "No job with id x")) ~> check { + status shouldEqual StatusCodes.NotFound + val body = responseAs[String] + body should include("Type: https://ignifyr.io/errors/ResourceNotFound") + body should include("No job with id x") + } + } + + "pass a request timeout through as 408" in { + Get("/boom") ~> routeThrowing(RequestTimeout("Timed out", "The FHIR server did not answer")) ~> check { + status shouldEqual StatusCodes.RequestTimeout + responseAs[String] should include("Type: https://ignifyr.io/errors/RequestTimeout") + } + } + + // onFHIR's definitions layer raises its own error type; a bad request from it must stay a 400 rather + // than being reported to the user as an internal failure. + "translate an onFHIR bad request into a 400" in { + Get("/boom") ~> routeThrowing(model.BadRequest("Invalid profile", "Profile url is malformed", None)) ~> check { + status shouldEqual StatusCodes.BadRequest + val body = responseAs[String] + body should include("Type: https://ignifyr.io/errors/BadRequest") + body should include("Profile url is malformed") + } + } + + "answer with 500 when an exception carries no message" in { + Get("/boom") ~> routeThrowing(new RuntimeException()) ~> check { + status shouldEqual StatusCodes.InternalServerError + responseAs[String] should include("Type: https://ignifyr.io/errors/InternalError") + } + } + } +} diff --git a/ignifyr-server/src/test/scala/io/ignifyr/server/endpoint/JobExecutionControlEndpointTest.scala b/ignifyr-server/src/test/scala/io/ignifyr/server/endpoint/JobExecutionControlEndpointTest.scala new file mode 100644 index 00000000..e30e02e2 --- /dev/null +++ b/ignifyr-server/src/test/scala/io/ignifyr/server/endpoint/JobExecutionControlEndpointTest.scala @@ -0,0 +1,110 @@ +package io.ignifyr.server.endpoint + +import akka.http.scaladsl.model.{ContentTypes, HttpEntity, StatusCodes} +import io.ignifyr.engine.model._ +import io.ignifyr.engine.util.FhirMappingJobFormatter.formats +import io.ignifyr.server.BaseEndpointTest +import org.json4s.JArray +import org.json4s.jackson.JsonMethods +import org.json4s.jackson.Serialization.writePretty + +/** + * The execution-control half of the job API — status, list executions, stop, deschedule. Starting an + * execution needs a real source and a FHIR server (that is the long-tier `MappingExecutionEndpointTest`), + * but the answers for a job that is *not* running are pure registry lookups, and they are what the web UI + * polls on every job page. They also have to distinguish "no such job" (404) from "job exists, nothing + * running" (200 with an empty list) — collapsing those two is the mistake this suite guards against. + */ +class JobExecutionControlEndpointTest extends BaseEndpointTest { + + private val job: FhirMappingJob = FhirMappingJob( + name = Some("execution-control-job"), + sourceSettings = Map.empty, + sinkSettings = FileSystemSinkSettings(path = "./out", contentType = SinkContentTypes.CSV), + mappings = Seq.empty, + dataProcessingSettings = DataProcessingSettings() + ) + + private def jobUri(jobId: String = job.id): String = + s"/${webServerConfig.baseUri}/${ProjectEndpoint.SEGMENT_PROJECTS}/$projectId/${JobEndpoint.SEGMENT_JOB}/$jobId" + + "The job execution control endpoints" should { + + // Note the quotes: the route stringifies the boolean before marshalling, so the body is the JSON + // string "false", not the JSON literal false. That is the contract the web UI parses. + "report a job that was never started as not running" in { + Get(s"${jobUri()}/${JobEndpoint.SEGMENT_STATUS}") ~> route ~> check { + status shouldEqual StatusCodes.OK + responseAs[String] shouldEqual "\"false\"" + } + } + + "report an unknown job as not running rather than failing" in { + // The status route consults only the running-job registry, so it does not resolve the job at all. + Get(s"${jobUri("no-such-job")}/${JobEndpoint.SEGMENT_STATUS}") ~> route ~> check { + status shouldEqual StatusCodes.OK + responseAs[String] shouldEqual "\"false\"" + } + } + + "return an empty execution list for a job that has never run" in { + Get(s"${jobUri()}/${JobEndpoint.SEGMENT_EXECUTIONS}") ~> route ~> check { + status shouldEqual StatusCodes.OK + JsonMethods.parse(responseAs[String]).asInstanceOf[JArray].arr shouldBe empty + } + } + + "return 404 when listing the executions of a job that does not exist" in { + Get(s"${jobUri("no-such-job")}/${JobEndpoint.SEGMENT_EXECUTIONS}") ~> route ~> check { + status shouldEqual StatusCodes.NotFound + } + } + + // Stopping every execution of a job is idempotent on purpose: the UI offers it whenever a job page is + // open, and it must not fail just because nothing happens to be running. + "accept a request to stop all executions of a job that is not running" in { + Delete(s"${jobUri()}/${JobEndpoint.SEGMENT_EXECUTIONS}") ~> route ~> check { + status.isSuccess() shouldBe true + } + } + + "return 404 when stopping an execution that is not running" in { + Delete( + s"${jobUri()}/${JobEndpoint.SEGMENT_EXECUTIONS}/no-such-execution/${JobEndpoint.SEGMENT_STOP}" + ) ~> route ~> check { + status shouldEqual StatusCodes.NotFound + } + } + + "return 404 when stopping a mappingTask execution that is not running" in { + Delete( + s"${jobUri()}/${JobEndpoint.SEGMENT_EXECUTIONS}/no-such-execution/" + + s"${JobEndpoint.SEGMENT_MAPPINGS}/no-such-mapping/${JobEndpoint.SEGMENT_STOP}" + ) ~> route ~> check { + status shouldEqual StatusCodes.NotFound + } + } + + "return 404 when descheduling an execution that is not scheduled" in { + Delete( + s"${jobUri()}/${JobEndpoint.SEGMENT_EXECUTIONS}/no-such-execution/${JobEndpoint.SEGMENT_DESCHEDULE}" + ) ~> route ~> check { + status shouldEqual StatusCodes.NotFound + } + } + } + + /** + * Creates the project and the job the execution-control routes are exercised against. + * */ + override def beforeAll(): Unit = { + super.beforeAll() + this.createProject() + Post( + s"/${webServerConfig.baseUri}/${ProjectEndpoint.SEGMENT_PROJECTS}/$projectId/${JobEndpoint.SEGMENT_JOB}", + HttpEntity(ContentTypes.`application/json`, writePretty(job)) + ) ~> route ~> check { + status shouldEqual StatusCodes.Created + } + } +} diff --git a/ignifyr-server/src/test/scala/io/ignifyr/server/repository/FolderDBInitializerTest.scala b/ignifyr-server/src/test/scala/io/ignifyr/server/repository/FolderDBInitializerTest.scala new file mode 100644 index 00000000..faa9263e --- /dev/null +++ b/ignifyr-server/src/test/scala/io/ignifyr/server/repository/FolderDBInitializerTest.scala @@ -0,0 +1,266 @@ +package io.ignifyr.server.repository + +import io.onfhir.definitions.common.model.SchemaDefinition +import io.ignifyr.engine.model.{FhirMapping, FhirMappingJob, FhirMappingSource, FhirRepositorySinkSettings} +import io.ignifyr.engine.util.FileUtils +import io.ignifyr.server.model.Project +import io.ignifyr.server.repository.job.JobFolderRepository +import io.ignifyr.server.repository.mapping.ProjectMappingFolderRepository +import io.ignifyr.server.repository.mappingContext.MappingContextFolderRepository +import io.ignifyr.server.repository.project.ProjectFolderRepository +import io.ignifyr.server.repository.schema.SchemaFolderRepository +import org.mockito.ArgumentCaptor +import org.mockito.MockitoSugar._ +import org.scalatest.BeforeAndAfterEach +import org.scalatest.flatspec.AnyFlatSpec +import org.scalatest.matchers.should.Matchers + +import java.nio.charset.StandardCharsets +import java.nio.file.Files +import scala.concurrent.Future + +/** + * Server startup: the project index is either read back from `projects.json` or, when that file is gone, + * rebuilt by scanning the repository folders. Both paths run before the first request is served, so a + * failure here is "the server does not start" rather than a failing call — and the rebuild path is the + * only recovery there is if the index file is ever lost. + * + * The repositories are mocked so the initializer's own resolution logic is what is under test, not the + * folder repositories it delegates to. + */ +class FolderDBInitializerTest extends AnyFlatSpec with Matchers with BeforeAndAfterEach { + + private val projectId = "project-1" + + private val schema = SchemaDefinition( + id = "schema-1", + url = "https://ignifyr.io/fhir/StructureDefinition/Ext-patient", + version = SchemaDefinition.VERSION_LATEST, + `type` = "Ext-patient", + name = "ext-patient", + description = None, + rootDefinition = None, + fieldDefinitions = None + ) + + private val mapping = FhirMapping( + id = "mapping-1", + url = "https://ignifyr.io/fhir/mappings/patient-mapping", + name = "patient-mapping", + source = Seq(FhirMappingSource(alias = "source", url = schema.url)), + context = Map.empty, + mapping = Seq.empty + ) + + private val job = FhirMappingJob( + id = "job-1", + sourceSettings = Map.empty, + sinkSettings = FhirRepositorySinkSettings(fhirRepoUrl = "http://localhost/fhir"), + mappings = Seq.empty + ) + + private def projectsJsonFile = FileUtils.getPath(ProjectFolderRepository.PROJECTS_JSON).toFile + + private def writeProjectsJson(content: String): Unit = { + projectsJsonFile.getParentFile.mkdirs() + Files.write(projectsJsonFile.toPath, content.getBytes(StandardCharsets.UTF_8)) + } + + override def beforeEach(): Unit = if (projectsJsonFile.exists()) projectsJsonFile.delete() + override def afterEach(): Unit = if (projectsJsonFile.exists()) projectsJsonFile.delete() + + /** + * Builds an initializer over mocked repositories, returning it with the project repository to verify. + * The `*ById` maps stub what each repository answers for a resource id the index file references; an + * id with no entry resolves to None, which is the "file is gone" case. + */ + private def initializerOver( + schemas: Map[String, Seq[SchemaDefinition]] = Map.empty, + mappings: Map[String, Seq[FhirMapping]] = Map.empty, + jobs: Map[String, Seq[FhirMappingJob]] = Map.empty, + contexts: Map[String, Seq[String]] = Map.empty, + schemasById: Map[String, Option[SchemaDefinition]] = Map.empty, + mappingsById: Map[String, Option[FhirMapping]] = Map.empty, + jobsById: Map[String, Option[FhirMappingJob]] = Map.empty + ): (FolderDBInitializer, ProjectFolderRepository) = { + val projectRepository = mock[ProjectFolderRepository] + + val schemaRepository = mock[SchemaFolderRepository] + when(schemaRepository.getProjectPairs).thenReturn(schemas) + schemasById.foreach { case (id, answer) => + when(schemaRepository.getSchema(projectId, id)).thenReturn(Future.successful(answer)) + } + + val mappingRepository = mock[ProjectMappingFolderRepository] + when(mappingRepository.getProjectPairs).thenReturn(mappings) + mappingsById.foreach { case (id, answer) => + when(mappingRepository.getMapping(projectId, id)).thenReturn(Future.successful(answer)) + } + + val jobRepository = mock[JobFolderRepository] + when(jobRepository.getProjectPairs).thenReturn(jobs) + jobsById.foreach { case (id, answer) => + when(jobRepository.getJob(projectId, id)).thenReturn(Future.successful(answer)) + } + + val contextRepository = mock[MappingContextFolderRepository] + when(contextRepository.getProjectPairs).thenReturn(contexts) + + ( + new FolderDBInitializer(projectRepository, schemaRepository, mappingRepository, jobRepository, contextRepository), + projectRepository + ) + } + + /** The projects the initializer handed to the repository. */ + private def injectedProjects(projectRepository: ProjectFolderRepository): Map[String, Project] = { + val captor: ArgumentCaptor[Map[String, Project]] = + ArgumentCaptor.forClass(classOf[Map[String, Project]]) + verify(projectRepository).setProjects(captor.capture()) + captor.getValue + } + + "init" should "rebuild the project index from the repository folders when there is no index file" in { + val (initializer, projectRepository) = initializerOver( + schemas = Map(projectId -> Seq(schema)), + mappings = Map(projectId -> Seq(mapping)), + jobs = Map(projectId -> Seq(job)), + contexts = Map(projectId -> Seq("unit-conversion")) + ) + initializer.init() + + val projects = injectedProjects(projectRepository) + projects.keySet shouldBe Set(projectId) + val project = projects(projectId) + project.schemas.map(_.id) shouldBe Seq("schema-1") + project.mappings.map(_.id) shouldBe Seq("mapping-1") + project.mappingJobs.map(_.id) shouldBe Seq("job-1") + project.mappingContexts shouldBe Seq("unit-conversion") + } + + it should "write the index file it did not find, so the rebuild happens only once" in { + val (initializer, _) = initializerOver() + projectsJsonFile should not(exist) + initializer.init() + projectsJsonFile should exist + } + + // With no index file there is no project name either, so the folder name stands in for it. + it should "name a rebuilt project after its folder and derive the url prefixes from its resources" in { + val (initializer, projectRepository) = initializerOver( + schemas = Map(projectId -> Seq(schema)), + mappings = Map(projectId -> Seq(mapping)) + ) + initializer.init() + + val project = injectedProjects(projectRepository)(projectId) + project.name shouldBe projectId + project.schemaUrlPrefix shouldBe Some("https://ignifyr.io/fhir/StructureDefinition/") + project.mappingUrlPrefix shouldBe Some("https://ignifyr.io/fhir/mappings/") + } + + it should "collect a project that owns only some of the resource kinds" in { + val (initializer, projectRepository) = initializerOver( + schemas = Map("only-schemas" -> Seq(schema)), + jobs = Map("only-jobs" -> Seq(job)) + ) + initializer.init() + + val projects = injectedProjects(projectRepository) + projects.keySet shouldBe Set("only-schemas", "only-jobs") + projects("only-schemas").mappingJobs shouldBe empty + projects("only-jobs").schemas shouldBe empty + } + + it should "resolve the resources referenced by an existing index file" in { + writeProjectsJson(s"""[{ + | "id": "$projectId", + | "name": "Example project", + | "description": "an example", + | "schemaUrlPrefix": "https://ignifyr.io/fhir/StructureDefinition/", + | "mappingUrlPrefix": "https://ignifyr.io/fhir/mappings/", + | "mappingContexts": ["unit-conversion"], + | "schemas": [{"id": "schema-1"}], + | "mappings": [{"id": "mapping-1"}], + | "mappingJobs": [{"id": "job-1"}] + |}]""".stripMargin) + + val (initializer, projectRepository) = initializerOver( + schemasById = Map("schema-1" -> Some(schema)), + mappingsById = Map("mapping-1" -> Some(mapping)), + jobsById = Map("job-1" -> Some(job)) + ) + initializer.init() + + val project = injectedProjects(projectRepository)(projectId) + project.name shouldBe "Example project" + project.description shouldBe Some("an example") + project.schemas.map(_.id) shouldBe Seq("schema-1") + project.mappings.map(_.id) shouldBe Seq("mapping-1") + project.mappingJobs.map(_.id) shouldBe Seq("job-1") + project.mappingContexts shouldBe Seq("unit-conversion") + } + + it should "read an index file that lists a project with no resources" in { + writeProjectsJson(s"""[{ + | "id": "$projectId", "name": "Empty", "mappingContexts": [], + | "schemas": [], "mappings": [], "mappingJobs": [] + |}]""".stripMargin) + + val (initializer, projectRepository) = initializerOver() + initializer.init() + + val project = injectedProjects(projectRepository)(projectId) + project.schemas shouldBe empty + project.description shouldBe None + } + + /* + * The realistic corruption: somebody deletes a mapping file by hand, leaving the index pointing at it. + * The initializer refuses to start rather than silently serving a project with a hole in it — the + * failure must name the id so the operator can find the missing file. + */ + it should "refuse to start when the index references a mapping that is not on disk" in { + writeProjectsJson(s"""[{ + | "id": "$projectId", "name": "Example", "mappingContexts": [], + | "schemas": [], "mappings": [{"id": "mapping-gone"}], "mappingJobs": [] + |}]""".stripMargin) + + val (initializer, _) = initializerOver(mappingsById = Map("mapping-gone" -> None)) + val thrown = the[IllegalStateException] thrownBy initializer.init() + thrown.getMessage should include("mapping-gone") + } + + it should "refuse to start when the index references a schema that is not on disk" in { + writeProjectsJson(s"""[{ + | "id": "$projectId", "name": "Example", "mappingContexts": [], + | "schemas": [{"id": "schema-gone"}], "mappings": [], "mappingJobs": [] + |}]""".stripMargin) + + val (initializer, _) = initializerOver(schemasById = Map("schema-gone" -> None)) + val thrown = the[IllegalStateException] thrownBy initializer.init() + thrown.getMessage should include("schema-gone") + } + + it should "refuse to start when the index references a job that is not on disk" in { + writeProjectsJson(s"""[{ + | "id": "$projectId", "name": "Example", "mappingContexts": [], + | "schemas": [], "mappings": [], "mappingJobs": [{"id": "job-gone"}] + |}]""".stripMargin) + + val (initializer, _) = initializerOver(jobsById = Map("job-gone" -> None)) + val thrown = the[IllegalStateException] thrownBy initializer.init() + thrown.getMessage should include("job-gone") + } + + "removeProjectsJsonFile" should "delete the index file, and do nothing when it is already gone" in { + val (initializer, _) = initializerOver() + writeProjectsJson("[]") + projectsJsonFile should exist + + initializer.removeProjectsJsonFile() + projectsJsonFile should not(exist) + + noException should be thrownBy initializer.removeProjectsJsonFile() + } +} diff --git a/ignifyr-server/src/test/scala/io/ignifyr/server/service/MetadataServiceTest.scala b/ignifyr-server/src/test/scala/io/ignifyr/server/service/MetadataServiceTest.scala new file mode 100644 index 00000000..dd04189c --- /dev/null +++ b/ignifyr-server/src/test/scala/io/ignifyr/server/service/MetadataServiceTest.scala @@ -0,0 +1,81 @@ +package io.ignifyr.server.service + +import com.typesafe.config.ConfigFactory +import io.ignifyr.engine.config.IgnifyrEngineConfig +import io.ignifyr.server.common.config.WebServerConfig +import io.ignifyr.server.common.spi.IgnifyrServerExtension +import io.onfhir.definitions.resource.fhir.FhirDefinitionsConfig +import org.scalatest.flatspec.AnyFlatSpec +import org.scalatest.matchers.should.Matchers + +import scala.concurrent.{ExecutionContext, Future} + +/** + * `/metadata` is fetched on every load of the Ignifyr frontend, so the extension lookup behind it has a + * hard 1-second bound and swallows every failure. Both properties are invisible from the endpoint test + * (which only sees a 200), and both are what keep a hung external service from blocking the UI. + * + * The lookup is also **not** generic: only the extension whose id is exactly "redcap" is ever asked. + */ +class MetadataServiceTest extends AnyFlatSpec with Matchers { + + private val rootConfig = ConfigFactory.load() + + private def serviceWith(extensions: Seq[IgnifyrServerExtension]): MetadataService = + new MetadataService( + new IgnifyrEngineConfig(rootConfig.getConfig("ignifyr")), + new WebServerConfig(rootConfig.getConfig("webserver")), + new FhirDefinitionsConfig(rootConfig.getConfig("fhir")), + extensions + ) + + /** An extension answering `externalComponentVersion` however the test needs it to. */ + private class StubExtension(val id: String, answer: ExecutionContext => Future[Option[String]]) + extends IgnifyrServerExtension { + override def externalComponentVersion()(implicit ec: ExecutionContext): Future[Option[String]] = answer(ec) + } + + private def responding(id: String, version: Option[String]) = + new StubExtension(id, _ => Future.successful(version)) + + "getMetadata" should "report the version of the redcap extension" in { + serviceWith(Seq(responding("redcap", Some("1.2.3")))).getMetadata.ignifyrRedcapVersion shouldBe Some("1.2.3") + } + + it should "report no redcap version when no extension is installed" in { + serviceWith(Seq.empty).getMetadata.ignifyrRedcapVersion shouldBe None + } + + // The seam is documented as generic but is not: MetadataService matches on the literal id "redcap", + // so implementing the hook in any other module has no effect on /metadata today. + it should "ignore the version reported by an extension other than redcap" in { + serviceWith(Seq(responding("observability", Some("9.9.9")))).getMetadata.ignifyrRedcapVersion shouldBe None + } + + it should "swallow a failing redcap lookup instead of failing the whole metadata response" in { + val failing = new StubExtension("redcap", _ => Future.failed(new RuntimeException("connection refused"))) + val metadata = serviceWith(Seq(failing)).getMetadata + metadata.ignifyrRedcapVersion shouldBe None + metadata.name shouldBe "Ignifyr" + } + + it should "give up on a redcap lookup that outlasts the one-second bound" in { + val slow = new StubExtension("redcap", ec => Future { Thread.sleep(3000); Some("too-late") }(ec)) + val startedAt = System.currentTimeMillis() + serviceWith(Seq(slow)).getMetadata.ignifyrRedcapVersion shouldBe None + (System.currentTimeMillis() - startedAt) should be < 3000L + } + + it should "report the repository folders and the archiving settings from the engine config" in { + val metadata = serviceWith(Seq.empty).getMetadata + metadata.repositoryNames.mappings should not be empty + metadata.repositoryNames.schemas should not be empty + metadata.repositoryNames.jobs should not be empty + metadata.archiving.archiveFolder should not be empty + } + + it should "publish the mapping execution configurations the UI displays" in { + val names = serviceWith(Seq.empty).getMetadata.executionConfigurations.map(_.name) + names should contain allOf ("Mapping Timeout", "Maximum Chunk Size", "Batch Group Size") + } +} diff --git a/ignifyr-server/src/test/scala/io/ignifyr/server/util/CsvUtilTest.scala b/ignifyr-server/src/test/scala/io/ignifyr/server/util/CsvUtilTest.scala new file mode 100644 index 00000000..66240bd2 --- /dev/null +++ b/ignifyr-server/src/test/scala/io/ignifyr/server/util/CsvUtilTest.scala @@ -0,0 +1,201 @@ +package io.ignifyr.server.util + +import akka.stream.scaladsl.{Sink, Source} +import akka.util.ByteString +import io.ignifyr.engine.Execution.actorSystem +import io.ignifyr.server.model.csv.CsvHeader +import org.scalatest.matchers.should.Matchers +import org.scalatest.wordspec.AsyncWordSpec + +import java.io.File +import java.nio.charset.StandardCharsets +import java.nio.file.Files +import scala.concurrent.Future + +/** + * Covers the CSV editing behind the mapping-context and terminology `content`/`header` routes. Every + * function here rewrites a file the user owns in place, so an off-by-one in the page arithmetic or a + * mis-tracked column rename is silent data loss rather than an error. + */ +class CsvUtilTest extends AsyncWordSpec with Matchers { + + /** Writes the given lines to a fresh temp CSV and returns it. */ + private def csvFile(lines: String*): File = { + val file = Files.createTempFile("ignifyr-csv-util", ".csv").toFile + Files.write(file.toPath, lines.mkString("\n").getBytes(StandardCharsets.UTF_8)) + file + } + + private def linesOf(file: File): Seq[String] = + new String(Files.readAllBytes(file.toPath), StandardCharsets.UTF_8).split("\n").map(_.trim).filter(_.nonEmpty).toSeq + + private def drain(source: Source[ByteString, Any]): Future[Seq[String]] = + source.runWith(Sink.seq).map(_.map(_.utf8String).mkString.split("\n").map(_.trim).filter(_.nonEmpty).toSeq) + + "writeCsvHeaders" should { + + "keep the values of a renamed column" in { + val file = csvFile("code,unit", "\"10839-9\",\"ng/ml\"") + CsvUtil + .writeCsvHeaders(file, Seq(CsvHeader("code", "code"), CsvHeader("targetUnit", "unit"))) + .map { _ => + linesOf(file) shouldBe Seq("code,targetUnit", "\"10839-9\",\"ng/ml\"") + } + } + + "drop the values of a removed column" in { + val file = csvFile("a,b,c", "1,2,3") + CsvUtil + .writeCsvHeaders(file, Seq(CsvHeader("a", "a"), CsvHeader("c", "c"))) + .map { _ => + linesOf(file) shouldBe Seq("a,c", "\"1\",\"3\"") + } + } + + // A newly added column has no data yet, so each row gets a visible placeholder rather than an empty + // cell — that is what tells the user in the UI which column still needs filling in. + "fill an added column with a placeholder naming it" in { + val file = csvFile("a", "1") + CsvUtil + .writeCsvHeaders(file, Seq(CsvHeader("a", "a"), CsvHeader("b", "b"))) + .map { _ => + linesOf(file) shouldBe Seq("a,b", "\"1\",\"\"") + } + } + + "reorder the columns to follow the given headers" in { + val file = csvFile("a,b", "1,2") + CsvUtil + .writeCsvHeaders(file, Seq(CsvHeader("b", "b"), CsvHeader("a", "a"))) + .map { _ => + linesOf(file) shouldBe Seq("b,a", "\"2\",\"1\"") + } + } + + "keep a value that contains the separator inside its quotes" in { + val file = csvFile("code,label", "\"1\",\"a,b\"") + CsvUtil + .writeCsvHeaders(file, Seq(CsvHeader("code", "code"), CsvHeader("label", "label"))) + .map { _ => + linesOf(file) shouldBe Seq("code,label", "\"1\",\"a,b\"") + } + } + + "write only the header row for a file that has no data rows" in { + val file = csvFile("a,b") + CsvUtil.writeCsvHeaders(file, Seq(CsvHeader("a", "a"))).map(_ => linesOf(file) shouldBe Seq("a")) + } + + // Regression: the returned Future used to complete before the write did, so a caller could observe + // the previous content right after being told the update succeeded. + "complete only after the file has been written" in { + val file = csvFile("a,b", "1,2") + CsvUtil + .writeCsvHeaders(file, Seq(CsvHeader("renamed", "a"))) + .map(_ => linesOf(file) shouldBe Seq("renamed", "\"1\"")) + } + } + + "getPaginatedCsvContent" should { + + "return the header plus the requested page" in { + val file = csvFile("h", "r1", "r2", "r3", "r4", "r5") + CsvUtil.getPaginatedCsvContent(file, pageNumber = 2, pageSize = 2).flatMap { case (source, total) => + total shouldBe 5 + drain(source).map(_ shouldBe Seq("h", "r3", "r4")) + } + } + + "return the header plus the first page" in { + val file = csvFile("h", "r1", "r2", "r3") + CsvUtil.getPaginatedCsvContent(file, pageNumber = 1, pageSize = 2).flatMap { case (source, _) => + drain(source).map(_ shouldBe Seq("h", "r1", "r2")) + } + } + + "return a short last page" in { + val file = csvFile("h", "r1", "r2", "r3") + CsvUtil.getPaginatedCsvContent(file, pageNumber = 2, pageSize = 2).flatMap { case (source, _) => + drain(source).map(_ shouldBe Seq("h", "r3")) + } + } + + "return only the header for a page past the end" in { + val file = csvFile("h", "r1") + CsvUtil.getPaginatedCsvContent(file, pageNumber = 5, pageSize = 2).flatMap { case (source, total) => + total shouldBe 1 + drain(source).map(_ shouldBe Seq("h")) + } + } + + "not count the header row in the total" in { + val file = csvFile("h") + CsvUtil.getPaginatedCsvContent(file, pageNumber = 1, pageSize = 10).map { case (_, total) => total shouldBe 0 } + } + } + + "writeCsvAndReturnRowNumber" should { + + "replace exactly the rows of the requested page" in { + val file = csvFile("h", "r1", "r2", "r3", "r4") + val replacement = Source(List(ByteString("new3"), ByteString("new4"))) + CsvUtil.writeCsvAndReturnRowNumber(file, replacement, pageNumber = 2, pageSize = 2).map { total => + linesOf(file) shouldBe Seq("h", "r1", "r2", "new3", "new4") + total shouldBe 4 + } + } + + "leave the header untouched when replacing the first page" in { + val file = csvFile("h", "r1", "r2") + val replacement = Source(List(ByteString("new1"), ByteString("new2"))) + CsvUtil.writeCsvAndReturnRowNumber(file, replacement, pageNumber = 1, pageSize = 2).map { total => + linesOf(file) shouldBe Seq("h", "new1", "new2") + total shouldBe 2 + } + } + + // The page is replaced in place, so a shorter replacement shrinks the file by the difference: the + // two rows of page 2 give way to one, and the rows before the page are untouched. + "shrink the file when the page is replaced by fewer rows" in { + val file = csvFile("h", "r1", "r2", "r3", "r4") + val replacement = Source(List(ByteString("only3"))) + CsvUtil.writeCsvAndReturnRowNumber(file, replacement, pageNumber = 2, pageSize = 2).map { total => + linesOf(file) shouldBe Seq("h", "r1", "r2", "only3") + total shouldBe 3 + } + } + + "report the row count excluding the header" in { + val file = csvFile("h", "r1") + CsvUtil + .writeCsvAndReturnRowNumber(file, Source(List(ByteString("r1'"))), pageNumber = 1, pageSize = 1) + .map(_ shouldBe 1) + } + } + + "saveFileContent" should { + + "overwrite the file with the given content" in { + val file = csvFile("old,header", "old,row") + CsvUtil + .saveFileContent(file, Source(List(ByteString("new,header\n"), ByteString("new,row")))) + .map(_ => linesOf(file) shouldBe Seq("new,header", "new,row")) + } + + "leave no trailing remnant of a longer previous content" in { + val file = csvFile("a,b,c,d,e,f,g,h", "1,2,3,4,5,6,7,8") + CsvUtil + .saveFileContent(file, Source.single(ByteString("x"))) + .map(_ => linesOf(file) shouldBe Seq("x")) + } + + "strip carriage returns so Windows line endings do not leak into the stored file" in { + val file = csvFile("placeholder") + CsvUtil + .saveFileContent(file, Source.single(ByteString("a,b\r\n1,2"))) + .map { _ => + new String(Files.readAllBytes(file.toPath), StandardCharsets.UTF_8) should not include "\r" + } + } + } +} diff --git a/ignifyr-server/src/test/scala/io/ignifyr/server/util/DataFrameUtilTest.scala b/ignifyr-server/src/test/scala/io/ignifyr/server/util/DataFrameUtilTest.scala new file mode 100644 index 00000000..a17d6dda --- /dev/null +++ b/ignifyr-server/src/test/scala/io/ignifyr/server/util/DataFrameUtilTest.scala @@ -0,0 +1,58 @@ +package io.ignifyr.server.util + +import io.ignifyr.engine.config.IgnifyrConfig +import io.ignifyr.server.model.{ResourceFilter, RowSelectionOrder} +import org.apache.spark.sql.{DataFrame, SparkSession} +import org.scalatest.flatspec.AnyFlatSpec +import org.scalatest.matchers.should.Matchers + +/** + * Row selection for the "test a mapping" route: the user picks how many source rows to run the mapping + * against and whether to take them from the start or at random. The endpoint suite only ever passes + * "start", so the random branch — which divides by `df.count()` — is only covered here. + */ +class DataFrameUtilTest extends AnyFlatSpec with Matchers { + + private val sparkSession: SparkSession = IgnifyrConfig.sparkSession + + private def rows(n: Int): DataFrame = { + import sparkSession.implicits._ + (1 to n).toDF("value") + } + + "applyResourceFilter" should "take the first rows in order for the start selection" in { + val filtered = DataFrameUtil.applyResourceFilter(rows(10), ResourceFilter(3, RowSelectionOrder.START)) + filtered.count() shouldBe 3 + filtered.collect().map(_.getInt(0)).toSeq shouldBe Seq(1, 2, 3) + } + + it should "not fail when fewer rows exist than requested for the start selection" in { + DataFrameUtil.applyResourceFilter(rows(2), ResourceFilter(10, RowSelectionOrder.START)).count() shouldBe 2 + } + + it should "return at most the requested number of rows for the random selection" in { + DataFrameUtil + .applyResourceFilter(rows(100), ResourceFilter(5, RowSelectionOrder.RANDOM)) + .count() should be <= 5L + } + + // The random branch computes numberOfRows / df.count() as a sampling fraction, so it has to cope with + // a request larger than the frame (fraction > 1, which Spark rejects) and with an empty frame + // (division by zero). + it should "clamp the sampling fraction when more rows are requested than exist" in { + DataFrameUtil + .applyResourceFilter(rows(3), ResourceFilter(10, RowSelectionOrder.RANDOM)) + .count() should be <= 3L + } + + it should "return nothing for the random selection over an empty frame" in { + val empty = rows(1).filter("value < 0") + DataFrameUtil.applyResourceFilter(empty, ResourceFilter(5, RowSelectionOrder.RANDOM)).count() shouldBe 0 + } + + "RowSelectionOrder" should "accept only the two documented orders" in { + RowSelectionOrder.isValid(RowSelectionOrder.START) shouldBe true + RowSelectionOrder.isValid(RowSelectionOrder.RANDOM) shouldBe true + RowSelectionOrder.isValid("sideways") shouldBe false + } +} diff --git a/ignifyr-sink-fhir/src/main/scala/io/ignifyr/sink/fhir/FhirRepositoryWriter.scala b/ignifyr-sink-fhir/src/main/scala/io/ignifyr/sink/fhir/FhirRepositoryWriter.scala index 5927d727..d05a4cdc 100644 --- a/ignifyr-sink-fhir/src/main/scala/io/ignifyr/sink/fhir/FhirRepositoryWriter.scala +++ b/ignifyr-sink-fhir/src/main/scala/io/ignifyr/sink/fhir/FhirRepositoryWriter.scala @@ -267,7 +267,7 @@ class FhirRepositoryWriter(sinkSettings: FhirRepositorySinkSettings) extends Bas * @param outcomeIssues The sequence of OutcomeIssues to be grouped. * @return A map where the keys are resource entry indices, and the values are sequences of OutcomeIssues associated with each index. */ - private def groupOutcomeIssuesByEntryIndex(outcomeIssues: Seq[OutcomeIssue]): Map[Int, Seq[OutcomeIssue]] = { + private[fhir] def groupOutcomeIssuesByEntryIndex(outcomeIssues: Seq[OutcomeIssue]): Map[Int, Seq[OutcomeIssue]] = { outcomeIssues .groupBy { issue => if (issue.expression.isEmpty) { diff --git a/ignifyr-sink-fhir/src/test/scala/io/ignifyr/sink/fhir/FhirRepositoryWriterTest.scala b/ignifyr-sink-fhir/src/test/scala/io/ignifyr/sink/fhir/FhirRepositoryWriterTest.scala new file mode 100644 index 00000000..32b35ba0 --- /dev/null +++ b/ignifyr-sink-fhir/src/test/scala/io/ignifyr/sink/fhir/FhirRepositoryWriterTest.scala @@ -0,0 +1,64 @@ +package io.ignifyr.sink.fhir + +import io.onfhir.api.model.OutcomeIssue +import io.ignifyr.engine.model.FhirRepositorySinkSettings +import org.scalatest.flatspec.AnyFlatSpec +import org.scalatest.matchers.should.Matchers + +/** + * Covers the entry-attribution step of the Firely error path. Firely answers a batch in which *any* + * entry failed with HTTP 400 (onFHIR does not), so the writer has to re-derive which input produced + * which problem from the `OutcomeIssue` expressions alone. An issue that cannot be attributed is + * dropped here — that is the behaviour worth pinning, since a wrong index would blame the wrong record. + * + * The surrounding write path needs a live FHIR server and is covered by the Docker integration suites + * in the modules that produce data (`connector-file`, `connector-sql`, `runtime-scheduling`). + */ +class FhirRepositoryWriterTest extends AnyFlatSpec with Matchers { + + private val writer = new FhirRepositoryWriter(FhirRepositorySinkSettings(fhirRepoUrl = "http://localhost/fhir")) + + private def issue(expression: String*): OutcomeIssue = + OutcomeIssue(severity = "error", code = "invalid", details = None, diagnostics = None, expression = expression) + + "groupOutcomeIssuesByEntryIndex" should "attribute an issue to the bundle entry named in its expression" in { + val first = issue("Bundle.entry[0].resource.name[0].family") + val third = issue("Bundle.entry[2].resource.birthDate") + writer.groupOutcomeIssuesByEntryIndex(Seq(first, third)) shouldBe Map(0 -> Seq(first), 2 -> Seq(third)) + } + + it should "group several issues reported for the same entry" in { + val one = issue("Bundle.entry[1].resource.gender") + val two = issue("Bundle.entry[1].resource.birthDate") + writer.groupOutcomeIssuesByEntryIndex(Seq(one, two)) shouldBe Map(1 -> Seq(one, two)) + } + + it should "read a multi-digit entry index" in { + writer.groupOutcomeIssuesByEntryIndex(Seq(issue("Bundle.entry[42].resource"))).keySet shouldBe Set(42) + } + + // Firely does not always return an expression; without one the issue cannot be blamed on any input + // record, so it is dropped rather than attached to an arbitrary index. + it should "drop an issue that carries no expression" in { + writer.groupOutcomeIssuesByEntryIndex(Seq(issue())) shouldBe empty + } + + it should "drop an issue whose expression does not point at a bundle entry" in { + writer.groupOutcomeIssuesByEntryIndex(Seq(issue("Patient.name[0].family"))) shouldBe empty + } + + it should "use the first expression when an issue reports several" in { + val multi = issue("Bundle.entry[3].resource.gender", "Bundle.entry[7].resource.gender") + writer.groupOutcomeIssuesByEntryIndex(Seq(multi)) shouldBe Map(3 -> Seq(multi)) + } + + it should "keep the attributable issues and drop only the rest" in { + val attributable = issue("Bundle.entry[5].resource") + writer.groupOutcomeIssuesByEntryIndex(Seq(issue(), attributable, issue("Observation.value"))) shouldBe + Map(5 -> Seq(attributable)) + } + + it should "return an empty map for no issues" in { + writer.groupOutcomeIssuesByEntryIndex(Seq.empty) shouldBe empty + } +} diff --git a/ignifyr-sink-file/src/main/scala/io/ignifyr/sink/file/format/FileSinkFormatRegistry.scala b/ignifyr-sink-file/src/main/scala/io/ignifyr/sink/file/format/FileSinkFormatRegistry.scala index e7595919..d473d06c 100644 --- a/ignifyr-sink-file/src/main/scala/io/ignifyr/sink/file/format/FileSinkFormatRegistry.scala +++ b/ignifyr-sink-file/src/main/scala/io/ignifyr/sink/file/format/FileSinkFormatRegistry.scala @@ -37,7 +37,8 @@ object FileSinkFormatRegistry { throw MissingFileSinkFormatException(FileSinkFormatHints.describeSinkFormat(contentType)) ) - private def indexUnique[V](what: String)(entries: Seq[(String, V)]): Map[String, V] = { + /** Visible to the sink package so the fail-fast contract can be asserted without a second classloader. */ + private[file] def indexUnique[V](what: String)(entries: Seq[(String, V)]): Map[String, V] = { val byKey = entries.groupBy(_._1).map { case (contentType, group) => if (group.size > 1) { val owners = group.map(_._2.getClass.getName).mkString(", ") diff --git a/ignifyr-sink-file/src/test/scala/io/ignifyr/sink/file/FileSinkExtensionSpec.scala b/ignifyr-sink-file/src/test/scala/io/ignifyr/sink/file/FileSinkExtensionSpec.scala index 16069d9e..7e7dc0f3 100644 --- a/ignifyr-sink-file/src/test/scala/io/ignifyr/sink/file/FileSinkExtensionSpec.scala +++ b/ignifyr-sink-file/src/test/scala/io/ignifyr/sink/file/FileSinkExtensionSpec.scala @@ -38,4 +38,26 @@ class FileSinkExtensionSpec extends AnyFlatSpec with Matchers { val ex = intercept[MissingFileSinkFormatException](FileSinkFormatRegistry.sinkFormat(SinkContentTypes.DELTA_LAKE)) ex.getMessage should include("com.pontegra.ignifyr:ignifyr-format-delta") } + + // The counterpart of the missing-format path: a content type claimed by *two* installed handlers. + // `FileSinkExtension.initialize` force-materializes this registry so it surfaces at startup rather + // than at first write. ServiceLoader input cannot be staged on this classpath, so the guard is + // asserted on the indexing helper directly. + it should "fail fast naming both handlers when two claim the same content type" in { + val ndjson = FileSinkFormatRegistry.sinkFormat(SinkContentTypes.NDJSON) + val parquet = FileSinkFormatRegistry.sinkFormat(SinkContentTypes.PARQUET) + val ex = intercept[IllegalStateException] { + FileSinkFormatRegistry.indexUnique("file sink format")(Seq("ndjson" -> ndjson, "ndjson" -> parquet)) + } + ex.getMessage should include("Duplicate file sink format registration") + ex.getMessage should include("ndjson") + ex.getMessage should (include(ndjson.getClass.getName) and include(parquet.getClass.getName)) + } + + it should "index one handler per content type" in { + val ndjson = FileSinkFormatRegistry.sinkFormat(SinkContentTypes.NDJSON) + val parquet = FileSinkFormatRegistry.sinkFormat(SinkContentTypes.PARQUET) + FileSinkFormatRegistry.indexUnique("file sink format")(Seq("a" -> ndjson, "b" -> parquet)) shouldBe + Map("a" -> ndjson, "b" -> parquet) + } } diff --git a/ignifyr-testkit/src/main/resources/test-mappings/some-folder-3/care-site-mapping.json b/ignifyr-testkit/src/main/resources/test-mappings/some-folder-3/care-site-mapping.json index 62d43c8c..63d621b7 100644 --- a/ignifyr-testkit/src/main/resources/test-mappings/some-folder-3/care-site-mapping.json +++ b/ignifyr-testkit/src/main/resources/test-mappings/some-folder-3/care-site-mapping.json @@ -36,11 +36,13 @@ } ] }], - "address": [{ - "line": ["{{? address_1}}", "{{? address_2}}"], - "city": "{{? city}}", - "state": "{{? state}}", - "postalCode": "{{? zip}}" + "contact": [{ + "address": { + "line": ["{{? address_1}}", "{{? address_2}}"], + "city": "{{? city}}", + "state": "{{? state}}", + "postalCode": "{{? zip}}" + } }] } } diff --git a/ignifyr-testkit/src/main/resources/test-mappings/some-folder-3/other-observation-mapping.json b/ignifyr-testkit/src/main/resources/test-mappings/some-folder-3/other-observation-mapping.json index 340d7614..b57b3000 100644 --- a/ignifyr-testkit/src/main/resources/test-mappings/some-folder-3/other-observation-mapping.json +++ b/ignifyr-testkit/src/main/resources/test-mappings/some-folder-3/other-observation-mapping.json @@ -163,7 +163,7 @@ "source": "{{%sourceSystem.sourceUri}}" }, "status": "completed", - "category": { + "category": [{ "coding": [ { "system": "http://terminology.hl7.org/CodeSystem/medication-admin-category", @@ -171,19 +171,21 @@ "display": "Inpatient" } ] - }, - "medicationCodeableConcept": { - "coding": [ - { - "system": "http://www.nlm.nih.gov/research/umls/rxnorm", - "code": "{{code}}", - "display": "{{mpp:getConcept(%obsConceptMap, code, 'source_display')}}" - } - ] + }], + "medication": { + "concept": { + "coding": [ + { + "system": "http://www.nlm.nih.gov/research/umls/rxnorm", + "code": "{{code}}", + "display": "{{mpp:getConcept(%obsConceptMap, code, 'source_display')}}" + } + ] + } }, "subject": "{{mpp:createFhirReferenceWithHashedId('Patient', pid)}}", - "context": "{{? mpp:createFhirReferenceWithHashedId('Encounter', encounterId)}}", - "effectiveDateTime": "{{time.utl:toFhirDateTime()}}", + "encounter": "{{? mpp:createFhirReferenceWithHashedId('Encounter', encounterId)}}", + "occurenceDateTime": "{{time.utl:toFhirDateTime()}}", "dosage": { "dose": { "value": "{{value.toDecimal()}}", diff --git a/ignifyr-testkit/src/main/resources/test-mappings/some-folder-3/procedure-occurrence-mapping.json b/ignifyr-testkit/src/main/resources/test-mappings/some-folder-3/procedure-occurrence-mapping.json index b6787caf..a869d7ed 100644 --- a/ignifyr-testkit/src/main/resources/test-mappings/some-folder-3/procedure-occurrence-mapping.json +++ b/ignifyr-testkit/src/main/resources/test-mappings/some-folder-3/procedure-occurrence-mapping.json @@ -40,7 +40,7 @@ } ] }, - "performedDateTime": "{{? iif(procedure_datetime.empty(), procedure_date, procedure_datetime.utl:toFhirDateTime()}}", + "occurrenceDateTime": "{{? iif(procedure_datetime.empty(), procedure_date, procedure_datetime.utl:toFhirDateTime()}}", "performer": [ { "actor": "{{? mpp:createFhirReferenceWithHashedId('Practitioner', provider_id.toString())}}" diff --git a/ignifyr-testkit/src/test/scala/io/ignifyr/test/FhirPathMappingFunctionsTest.scala b/ignifyr-testkit/src/test/scala/io/ignifyr/test/FhirPathMappingFunctionsTest.scala index d8fcf56a..1e02dfb2 100644 --- a/ignifyr-testkit/src/test/scala/io/ignifyr/test/FhirPathMappingFunctionsTest.scala +++ b/ignifyr-testkit/src/test/scala/io/ignifyr/test/FhirPathMappingFunctionsTest.scala @@ -109,6 +109,20 @@ class FhirPathMappingFunctionsTest extends AsyncFlatSpec with IgnifyrTestSpec { an[Exception] should be thrownBy fhirEvaluator.evaluateOptionalNumerical("mpp:getHashedIntId('p1', 100, 5)", JNull) } + it should "correctly execute createFhirReferenceWithHashedId" in { + val fhirEvaluator = FhirPathEvaluator().withFunctionLibrary("mpp", new FhirMappingFunctionsFactory(Map.empty)) + val reference = fhirEvaluator.evaluateAndReturnJson("mpp:createFhirReferenceWithHashedId('Patient','p1')", JNull) + // The reference must agree with the id the resource itself is written under, otherwise the mapped + // resources point at Patients that do not exist. + val hashedId = fhirEvaluator.evaluateOptionalString("mpp:getHashedId('Patient','p1')", JNull).get + reference shouldBe Some(JObject("reference" -> JString(s"Patient/$hashedId"))) + + // Given several ids, it yields one reference per id. + val references = + fhirEvaluator.evaluateAndReturnJson("mpp:createFhirReferenceWithHashedId('Patient', ('p1' | 'p2'))", JNull) + references.get.asInstanceOf[JArray].arr.length shouldBe 2 + } + it should "correctly execute nonEmptyLoopedFields" in { val fhirEvaluator = FhirPathEvaluator().withFunctionLibrary("mpp", new FhirMappingFunctionsFactory(Map.empty)) val sct = fhirEvaluator.evaluateAndReturnJson("mpp:nonEmptyLoopedFields('sct_8116006_',1,5)", loopJson).head diff --git a/test-flow/check-test-tiers.sh b/test-flow/check-test-tiers.sh index cff6da62..42c920c7 100644 --- a/test-flow/check-test-tiers.sh +++ b/test-flow/check-test-tiers.sh @@ -73,6 +73,21 @@ for pom in */pom.xml; do fi done +# ---- 2b. every module with test sources actually runs them ------------------- +# Invariant 2 only constrains modules that DO declare the plugin. A module with test sources and no +# plugin at all is worse: its suites compile, look like coverage, and never run under Maven. +log "2b. No module owns test sources without a " +for pom in */pom.xml; do + mod="$(dirname "$pom")" + [ -d "$mod/src/test/scala" ] || continue + [ -n "$(find "$mod/src/test/scala" -name '*.scala' -print -quit 2>/dev/null)" ] || continue + if grep -q 'scalatest-maven-plugin' "$pom"; then + ok "$mod runs its test sources" + else + bad "$mod has test sources but declares no scalatest-maven-plugin -- its suites never run under Maven" + fi +done + # ---- 3. integration suites and integration executions agree ------------------ log "3. Modules owning integration suites wire an 'integration-test' execution behind \${skipITs}" for pom in */pom.xml; do