Improve error handling by exposing exceptions instead of silently con…

…suming them (#45) * Early abort to improve readability. * * Don't catch Throwable * Make exceptions available via unused `validationErrors` field * Improve code readability. * Don't offer `Throwable`, offer `Exception` as `Errors` should not be handled here.
eu-digital-green-certificates · Jul 22, 2021 · 30cf728 · 30cf728
1 parent 012b453
commit 30cf728
Show file tree

Hide file tree

Showing 6 changed files with 61 additions and 45 deletions.
diff --git a/engine/src/main/java/dgca/verifier/app/engine/DefaultCertLogicEngine.kt b/engine/src/main/java/dgca/verifier/app/engine/DefaultCertLogicEngine.kt
@@ -67,48 +67,66 @@ class DefaultCertLogicEngine(
         externalParameter: ExternalParameter,
         payload: String
     ): List<ValidationResult> {
-        return if (rules.isNotEmpty()) {
-            val validationResults = mutableListOf<ValidationResult>()
-            val dataJsonNode = prepareData(externalParameter, payload)
-            val hcertVersion = hcertVersionString.toVersion()
-            rules.forEach { rule ->
-                val ruleEngineVersion = rule.engineVersion.toVersion()
-                val schemaVersion = rule.schemaVersion.toVersion()
-                val res = when {
-                    rule.engine == CERTLOGIC_KEY && ruleEngineVersion != null && CERTLOGIC_VERSION.isGreaterOrEqualThan(
-                        ruleEngineVersion
-                    ) && hcertVersion != null && schemaVersion != null && hcertVersion.first == schemaVersion.first && hcertVersion.isGreaterOrEqualThan(
-                        schemaVersion
-                    ) ->
-                        when (jsonLogicValidator.isDataValid(
-                            rule.logic,
-                            dataJsonNode
-                        )) {
-                            true -> Result.PASSED
-                            false -> Result.FAIL
-                            else -> Result.OPEN
-                        }
-                    else -> Result.OPEN
+        if (rules.isEmpty()) return emptyList()
+
+        val dataJsonNode = prepareData(externalParameter, payload)
+        val hcertVersion: Triple<Int, Int, Int>? = hcertVersionString.toVersion()
+
+        return rules.map {
+            checkRule(
+                rule = it,
+                dataJsonNode = dataJsonNode,
+                hcertVersion = hcertVersion,
+                certificateType = certificateType
+            )
+        }
+    }
+
+    private fun checkRule(
+        rule: Rule,
+        dataJsonNode: ObjectNode,
+        hcertVersion: Triple<Int, Int, Int>?,
+        certificateType: CertificateType
+    ): ValidationResult {
+        val ruleEngineVersion = rule.engineVersion.toVersion()
+        val schemaVersion = rule.schemaVersion.toVersion()
+
+        val validationErrors = mutableListOf<Exception>()
+
+        val isCompatibleVersion = rule.engine == CERTLOGIC_KEY &&
+                ruleEngineVersion != null &&
+                CERTLOGIC_VERSION.isGreaterOrEqualThan(ruleEngineVersion) &&
+                hcertVersion != null &&
+                schemaVersion != null &&
+                hcertVersion.first == schemaVersion.first &&
+                hcertVersion.isGreaterOrEqualThan(schemaVersion)
+
+        val res = if (isCompatibleVersion) {
+            try {
+                when (jsonLogicValidator.isDataValid(rule.logic, dataJsonNode)) {
+                    true -> Result.PASSED
+                    false -> Result.FAIL
                 }
-                val cur: String = affectedFieldsDataRetriever.getAffectedFieldsData(
-                    rule,
-                    dataJsonNode,
-                    certificateType
-                )
-                validationResults.add(
-                    ValidationResult(
-                        rule,
-                        res,
-                        cur,
-                        null
-                    )
-                )
+            } catch (e: Exception) {
+                validationErrors.add(e)
+                Result.OPEN
             }
-            validationResults
         } else {
-            emptyList()
+            Result.OPEN
         }
 
+        val cur: String = affectedFieldsDataRetriever.getAffectedFieldsData(
+            rule,
+            dataJsonNode,
+            certificateType
+        )
+
+        return ValidationResult(
+            rule,
+            res,
+            cur,
+            if (validationErrors.isEmpty()) null else validationErrors
+        )
     }
 
     private fun Triple<Int, Int, Int>.isGreaterOrEqualThan(version: Triple<Int, Int, Int>): Boolean =

diff --git a/engine/src/main/java/dgca/verifier/app/engine/DefaultJsonLogicValidator.kt b/engine/src/main/java/dgca/verifier/app/engine/DefaultJsonLogicValidator.kt
@@ -26,10 +26,7 @@ import eu.ehn.dcc.certlogic.evaluate
  * Created by osarapulov on 13.06.21 17:20
  */
 class DefaultJsonLogicValidator : JsonLogicValidator {
-    override fun isDataValid(rule: JsonNode, data: JsonNode): Boolean? = try {
+    override fun isDataValid(rule: JsonNode, data: JsonNode): Boolean =
         (evaluate(rule, data) as BooleanNode).asBoolean()
-    } catch (error: Throwable) {
-        null
-    }
 
 }
diff --git a/engine/src/main/java/dgca/verifier/app/engine/JsonLogicValidator.kt b/engine/src/main/java/dgca/verifier/app/engine/JsonLogicValidator.kt
@@ -24,5 +24,5 @@ import com.fasterxml.jackson.databind.JsonNode
  * Created by osarapulov on 13.06.21 17:18
  */
 interface JsonLogicValidator {
-    fun isDataValid(rule: JsonNode, data: JsonNode): Boolean?
+    fun isDataValid(rule: JsonNode, data: JsonNode): Boolean
 }
diff --git a/engine/src/main/java/dgca/verifier/app/engine/ValidationResult.kt b/engine/src/main/java/dgca/verifier/app/engine/ValidationResult.kt
@@ -31,5 +31,5 @@ class ValidationResult(
     val rule: Rule,
     val result: Result,
     val current: String,
-    val validationErrors: List<Throwable>?,
+    val validationErrors: List<Exception>?,
 )
diff --git a/engine/src/test/java/dgca/verifier/app/engine/DefaultCertLogicEngineTest.kt b/engine/src/test/java/dgca/verifier/app/engine/DefaultCertLogicEngineTest.kt
@@ -36,6 +36,7 @@ import org.mockito.Mock
 import org.mockito.junit.MockitoJUnitRunner
 import org.mockito.kotlin.any
 import org.mockito.kotlin.doReturn
+import org.mockito.kotlin.doThrow
 import java.io.InputStream
 import java.nio.charset.Charset
 import java.time.ZonedDateTime
@@ -240,7 +241,7 @@ internal class DefaultCertLogicEngineTest {
 
     @Test
     fun testValidatedWithException() {
-        doReturn(null).`when`(jsonLogicValidator).isDataValid(any(), any())
+        doThrow(RuntimeException()).`when`(jsonLogicValidator).isDataValid(any(), any())
         val hcertJson = mockHcertJson()
         val rules = listOf(mockRuleRemote()).toRules()
         val externalParameter = mockExternalParameter()

diff --git a/engine/src/test/java/dgca/verifier/app/engine/DefaultJsonLogicValidatorTest.kt b/engine/src/test/java/dgca/verifier/app/engine/DefaultJsonLogicValidatorTest.kt
@@ -52,7 +52,7 @@ import org.junit.jupiter.api.Assertions.*
 class DefaultJsonLogicValidatorTest {
  private val jsonLogicValidator = DefaultJsonLogicValidator()
 
-    @Test
+    @Test(expected = RuntimeException::class)
     fun testException() {
         assertNull(jsonLogicValidator.isDataValid(jacksonObjectMapper().readValue("{}"), jacksonObjectMapper().readValue("{}")))
     }