[GitHub] [incubator-daffodil] stevedlawrence commented on a change in pull request #386: Implemented dfdl:assert failureType="recoverableError"

[GitBox <gi...@apache.org>]

stevedlawrence commented on a change in pull request #386:
URL: https://github.com/apache/incubator-daffodil/pull/386#discussion_r441490390



##########
File path: daffodil-core/src/main/scala/org/apache/daffodil/dsom/DFDLAssertion.scala
##########
@@ -105,6 +106,11 @@ abstract class DFDLAssertionBase(node: Node, decl: AnnotatedSchemaComponent)
     case None => TestKind.Expression
   }
 
+  final lazy val failureType = getAttributeOption("failureType") match {
+    case Some(str) => FailureType(str, decl)
+    case None => FailureType.ProcessingError
+  }
+

Review comment:
       Insted of using ``getAttributeOption`` and manually creating a ``FailureType`` if it was set, you should be able to use ``optionFailureType`` to handle all that logic for you. If you mixin the appropriate generated mixin (maybe DFDLAssertTypeMixin is the right thing?) you'll have access to that def. That mixin will also create a ``def failureType`` which conflicts with this val name, so a different name is needed. So maybe this becomes something like:
   ```scala
   final lazy val failureTypeDefaulted = optionFailureType.getOrElse(FailureType.ProcessingError)
   ```

##########
File path: daffodil-runtime1/src/main/scala/org/apache/daffodil/processors/parsers/AssertPatternParsers.scala
##########
@@ -75,8 +77,11 @@ class AssertPatternParser(
       val isMatch = dis.lookingAt(m, start)
       if (!isMatch) {
         val message = getAssertFailureMessage(start)
-        val diag = new AssertionFailed(context.schemaFileLocation, start, message)
-        start.setFailed(diag)
+        if (failureType == FailureType.ProcessingError) {
+          val diag = new AssertionFailed(context.schemaFileLocation, start, message)
+          start.setFailed(diag)
+        } else
+          start.SDW(message)

Review comment:
       Looking at the spec, it isn't really clear how recoverable error messages are expected to be presented to the user, and really what a recoverable error means. SDW doesn't seem unreasonble, but I could imagine people might skim over it thinking it's a warning? There's also no easy way to an API user to determine if a recoverable error happened, since they look idential to a SDW.
   
   I imagine when someone does an assert with recoverable error, it's to do some sort of check that shouldn't necessarily fail the parse, which feels a lot to me like a validation error. Maybe it makes sense to make this a validation error instead?
   
   Or maybe we should add a new error type that is RecoverableError, similar to ValidationError? Note that we have ``isProcessingError`` and ``isValidationError`` that one can check on a ParseResult. Do we need another ``isRecoverableError`` as well if we add a new error type? And some way to differentiate validation vs processing vs recoverable diagnostics in the API? And does this require adding new ``<tdml:recoverableErrors>`` to the tdml format?

##########
File path: daffodil-runtime1/src/main/scala/org/apache/daffodil/processors/parsers/ExpressionEvaluatingParsers.scala
##########
@@ -209,8 +211,11 @@ class AssertExpressionEvaluationParser(
       start.setDiscriminator(discrim)
     } else {
       val message = getAssertFailureMessage(start)

Review comment:
       Also, in addition to failureType="recoverableError", the spec also states that a recoverable error should also be created if evaluting the message expression for asserts/discriminators fails. It looks like right now if the message expression fails in getAssertFailureMessage, we just generate a new message without also creating a recoverable error. However we deicded recoverable errors should be presented to the user, we should do the same for when evaluating ``messageExpr`` fails.




----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.

For queries about this service, please contact Infrastructure at:
users@infra.apache.org