You are viewing a plain text version of this content. The canonical link for it is here.
Posted to commits@daffodil.apache.org by GitBox <gi...@apache.org> on 2020/06/16 19:10:20 UTC
[GitHub] [incubator-daffodil] jadams-tresys opened a new pull request #386: Implemented dfdl:assert failureType="recoverableError"
jadams-tresys opened a new pull request #386:
URL: https://github.com/apache/incubator-daffodil/pull/386
Simple addition of allowing recoverable errors to occur from
dfdl:assert. Right now it is simply creating a runtime Schema Definition
Warning with the assert failure message.
DAFFODIL-357
----------------------------------------------------------------
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
[GitHub] [incubator-daffodil] mbeckerle commented on a change in pull request #386: Implemented dfdl:assert failureType="recoverableError"
Posted by GitBox <gi...@apache.org>.
mbeckerle commented on a change in pull request #386:
URL: https://github.com/apache/incubator-daffodil/pull/386#discussion_r441131003
##########
File path: daffodil-core/src/main/scala/org/apache/daffodil/grammar/primitives/PrimitivesExpressions.scala
##########
@@ -98,7 +101,7 @@ abstract class AssertBooleanPrimBase(
decl: AnnotatedSchemaComponent,
stmt: DFDLAssertionBase,
discrim: Boolean, // are we a discriminator or not.
- assertKindName: String) extends AssertBase(decl, Found(stmt.testTxt, stmt, "test", false), stmt.messageAttrib, discrim, assertKindName)
+ assertKindName: String) extends AssertBase(decl, Found(stmt.testTxt, stmt, "test", false), stmt.messageAttrib, discrim, assertKindName, stmt.failureType)
Review comment:
Please wrap lines before the extends keyword.
----------------------------------------------------------------
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
[GitHub] [incubator-daffodil] stevedlawrence merged pull request #386: Implemented dfdl:assert failureType="recoverableError"
Posted by GitBox <gi...@apache.org>.
stevedlawrence merged pull request #386:
URL: https://github.com/apache/incubator-daffodil/pull/386
----------------------------------------------------------------
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
[GitHub] [incubator-daffodil] stevedlawrence commented on a change in pull request #386: Implemented dfdl:assert failureType="recoverableError"
Posted by 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
[GitHub] [incubator-daffodil] stevedlawrence commented on a change in pull request #386: Implemented dfdl:assert failureType="recoverableError"
Posted by GitBox <gi...@apache.org>.
stevedlawrence commented on a change in pull request #386:
URL: https://github.com/apache/incubator-daffodil/pull/386#discussion_r444182898
##########
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:
+1, with future fixes to address above comments
----------------------------------------------------------------
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