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/05/06 13:01:20 UTC
[GitHub] [incubator-daffodil] jadams-tresys opened a new pull request #378: Added try/catch for potential unparse exceptions
jadams-tresys opened a new pull request #378:
URL: https://github.com/apache/incubator-daffodil/pull/378
There are a couple unhandled exceptions resulting from attempting to
unparse a string into various primitive types that was resulting in
NumberFormatExceptions and potentially IllegalArgumentExceptions. This
simply adds a try/catch to turn these exceptions into a reasonable
UnparseError.
DAFFODIL-2124
----------------------------------------------------------------
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 #378: Added try/catch for potential unparse exceptions
Posted by GitBox <gi...@apache.org>.
mbeckerle commented on a change in pull request #378:
URL: https://github.com/apache/incubator-daffodil/pull/378#discussion_r421580874
##########
File path: daffodil-runtime1/src/main/scala/org/apache/daffodil/dpath/NodeInfo.scala
##########
@@ -452,9 +454,30 @@ object NodeInfo extends Enum {
}
}
+ trait PrimThrowsIllegalArgument {
+ def fromString(s: String): DataValuePrimitive
+ def fromXMLString(s: String): DataValuePrimitive = {
+ try {
+ fromString(s)
+ } catch {
+ case iae: IllegalArgumentException =>
+ throw new InvalidPrimitiveDataException("%s is not a valid %s".format(iae.getMessage, this))
+ }
+ }
+ }
+
trait PrimNumeric { self: Numeric.Kind =>
def isValidRange(n: Number): Boolean
def fromNumber(n: Number): DataValueNumber
+ def fromString(s: String): DataValueNumber
Review comment:
Should fromString be protected? It is intended to override.
fromXMLString is more explicit about the nature of the string being converted. I.e., it came from XML; hence, strings like "1" should be equivalent to "1". I don't know that it is called in any context where the string argument isn't already converted though, such that there won't be character numeric entities or other goop to deal with.
----------------------------------------------------------------
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 #378: Added try/catch for potential unparse exceptions
Posted by GitBox <gi...@apache.org>.
mbeckerle commented on a change in pull request #378:
URL: https://github.com/apache/incubator-daffodil/pull/378#discussion_r421572407
##########
File path: daffodil-runtime1/src/main/scala/org/apache/daffodil/dpath/NodeInfo.scala
##########
@@ -452,9 +454,30 @@ object NodeInfo extends Enum {
}
}
+ trait PrimThrowsIllegalArgument {
+ def fromString(s: String): DataValuePrimitive
+ def fromXMLString(s: String): DataValuePrimitive = {
+ try {
+ fromString(s)
+ } catch {
+ case iae: IllegalArgumentException =>
+ throw new InvalidPrimitiveDataException("%s is not a valid %s".format(iae.getMessage, this))
Review comment:
I always like to just construct nested exceptions by passing the inner exception as a cause to the enclosing exception, and not constructing strings at all.
There's an idiom for deriving exception classes so that you have both string and throwable constructors for them. We don't do this for our exception classes, but we should so that you have string and cause constructors for them. Like this:
`class MyException protected (m: String, cause: Throwable)
extends Exception(m, cause) { // or extend an exception base class, 2-arg constructor
def this(message: String) = this(message, null)
def this(cause: Throwable) = this(null, cause)
....
}`
One can even omit the def this (m: String) constructor if the real usage pattern only involves nesting an exception.
----------------------------------------------------------------
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] jadams-tresys commented on a change in pull request #378: Added try/catch for potential unparse exceptions
Posted by GitBox <gi...@apache.org>.
jadams-tresys commented on a change in pull request #378:
URL: https://github.com/apache/incubator-daffodil/pull/378#discussion_r421781400
##########
File path: daffodil-runtime1/src/main/scala/org/apache/daffodil/infoset/InfosetInputter.scala
##########
@@ -420,7 +421,14 @@ abstract class InfosetInputter
}
} else if (erd.outputValueCalcExpr.isEmpty) {
val primType = elem.erd.optPrimType.get
- val obj = primType.fromXMLString(txt)
+ val obj = try {
+ primType.fromXMLString(txt)
+ } catch {
+ case ipd: InvalidPrimitiveDataException =>
+ UnparseError(One(elem.erd.schemaFileLocation), Nope, ipd.getMessage)
+ case iae: IllegalArgumentException =>
Review comment:
Nevermind, wasn't paying attention to which piece of code this is. You are right, no longer need IllegalArgument here.
----------------------------------------------------------------
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 #378: Added try/catch for potential unparse exceptions
Posted by GitBox <gi...@apache.org>.
mbeckerle commented on a change in pull request #378:
URL: https://github.com/apache/incubator-daffodil/pull/378#discussion_r420833001
##########
File path: daffodil-runtime1/src/main/scala/org/apache/daffodil/infoset/InfosetInputter.scala
##########
@@ -420,7 +420,14 @@ abstract class InfosetInputter
}
} else if (erd.outputValueCalcExpr.isEmpty) {
val primType = elem.erd.optPrimType.get
- val obj = primType.fromXMLString(txt)
+ val obj = try {
+ primType.fromXMLString(txt)
Review comment:
Consider this:
`/**
* Scala doesn't enforce that throws be handled. Hence APIs that involve throwing
* an exception that callers are expected to always handle are fragile because callsers
* might not handle them, and compilation doesn't tell you that.
*
* This solves that problem by putting the exception handling into the type signature of
* the return.
*
* Note: ResultType cannot be a class derived from Exception, in case that wasn't obvious.
*/
class TryVal[ResultType <: AnyRef, ExceptionType <: Exception] (private val thing: AnyRef) extends AnyVal {
@inline def isValue: Boolean = this.thing.isInstanceOf[ResultType]
@inline private def value: ResultType = this.asInstanceOf[ResultType]
@inline def isException : Boolean = this.thing.isInstanceOf[ExceptionType]
@inline def toss = throw this.asInstanceOf[ExceptionType]
@inline def get: ResultType = if (isValue) value else toss
}`
----------------------------------------------------------------
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 #378: Added try/catch for potential unparse exceptions
Posted by GitBox <gi...@apache.org>.
stevedlawrence commented on a change in pull request #378:
URL: https://github.com/apache/incubator-daffodil/pull/378#discussion_r425161729
##########
File path: daffodil-runtime1/src/main/scala/org/apache/daffodil/dpath/ConverterOps.scala
##########
@@ -179,159 +136,80 @@ case object DoubleToBoolean extends Converter {
}
}
case object FloatToDouble extends Converter {
- override def computeValue(a: DataValuePrimitive, dstate: DState): DataValueDouble = asDouble(a.getFloat)
+ override def computeValue(a: DataValuePrimitive, dstate: DState): DataValueDouble = PrimType.Double.fromNumber(a.getFloat).getDouble
}
case object IntegerToDecimal extends Converter {
- override def computeValue(a: DataValuePrimitive, dstate: DState): DataValueBigDecimal = new JBigDecimal(a.getBigInt)
+ override def computeValue(a: DataValuePrimitive, dstate: DState): DataValueBigDecimal = PrimType.Decimal.fromNumber(a.getBigInt).getBigDecimal
}
-case object IntegerToUnsignedLong extends Converter with NumericRangeCheck {
- override def computeValue(a: DataValuePrimitive, dstate: DState): DataValueBigInt = {
- val res = a.getBigInt
- checkRange(res)
- asBigInt(res)
- }
- override protected val rangePrim = PrimType.UnsignedLong
+case object IntegerToUnsignedLong extends Converter {
+ override def computeValue(a: DataValuePrimitive, dstate: DState): DataValueBigInt = PrimType.UnsignedLong.fromNumber(a.getBigInt).getBigInt
}
case object LongToBoolean extends Converter {
override def computeValue(a: DataValuePrimitive, dstate: DState): DataValueBool = asBoolean(if (a.getLong == 0) false else true)
}
-case object LongToByte extends Converter with NumericRangeCheck {
- override def computeValue(a: DataValuePrimitive, dstate: DState): DataValueByte = {
- val res = a.getLong
- checkRange(res)
- asByte(res)
- }
- override protected val rangePrim = PrimType.Byte
+case object LongToByte extends Converter {
+ override def computeValue(a: DataValuePrimitive, dstate: DState): DataValueByte = PrimType.Byte.fromNumber(a.getLong).getByte
}
case object LongToDecimal extends Converter {
- override def computeValue(a: DataValuePrimitive, dstate: DState): DataValueBigDecimal = asBigDecimal(a.getLong)
+ override def computeValue(a: DataValuePrimitive, dstate: DState): DataValueBigDecimal = PrimType.Decimal.fromNumber(a.getLong).getBigDecimal
}
case object LongToDouble extends Converter {
- override def computeValue(a: DataValuePrimitive, dstate: DState): DataValueDouble = asDouble(a.getLong)
+ override def computeValue(a: DataValuePrimitive, dstate: DState): DataValueDouble = PrimType.Double.fromNumber(a.getLong).getDouble
}
case object LongToFloat extends Converter {
- override def computeValue(a: DataValuePrimitive, dstate: DState): DataValueFloat = asFloat(a.getLong)
+ override def computeValue(a: DataValuePrimitive, dstate: DState): DataValueFloat = PrimType.Float.fromNumber(a.getLong).getFloat
}
-case object LongToInt extends Converter with NumericRangeCheck {
- override def computeValue(a: DataValuePrimitive, dstate: DState): DataValueInt = {
- val res = a.getLong
- checkRange(res)
- asInt(res)
- }
- override protected val rangePrim = PrimType.Int
+case object LongToInt extends Converter {
+ override def computeValue(a: DataValuePrimitive, dstate: DState): DataValueInt = PrimType.Int.fromNumber(a.getLong).getInt
}
case object LongToInteger extends Converter {
- override def computeValue(a: DataValuePrimitive, dstate: DState): DataValueBigInt = asBigInt(a.getLong)
+ override def computeValue(a: DataValuePrimitive, dstate: DState): DataValueBigInt = PrimType.Integer.fromNumber(a.getLong).getBigInt
}
-case object LongToShort extends Converter with NumericRangeCheck {
- override def computeValue(a: DataValuePrimitive, dstate: DState): DataValueShort = {
- val res = a.getLong
- checkRange(res)
- asShort(res)
- }
- override protected val rangePrim = PrimType.Short
+case object LongToShort extends Converter {
+ override def computeValue(a: DataValuePrimitive, dstate: DState): DataValueShort = PrimType.Short.fromNumber(a.getLong).getShort
}
case object LongToArrayIndex extends Converter {
- override def computeValue(a: DataValuePrimitive, dstate: DState): DataValueLong = {
- val res = a.getLong
- res
- }
+ override def computeValue(a: DataValuePrimitive, dstate: DState): DataValueLong = PrimType.Long.fromNumber(a.getLong).getLong
}
-case object LongToUnsignedByte extends Converter with NumericRangeCheck {
- override def computeValue(a: DataValuePrimitive, dstate: DState): DataValueShort = {
- val res = a.getLong
- checkRange(res)
- asShort(res)
- }
- override protected val rangePrim = PrimType.UnsignedByte
+case object LongToUnsignedByte extends Converter {
+ override def computeValue(a: DataValuePrimitive, dstate: DState): DataValueShort = PrimType.UnsignedByte.fromNumber(a.getLong).getShort
}
-case object LongToUnsignedInt extends Converter with NumericRangeCheck {
- override def computeValue(a: DataValuePrimitive, dstate: DState): DataValueLong = {
- val res = a.getLong
- checkRange(res)
- res
- }
- override protected val rangePrim = PrimType.UnsignedInt
+case object LongToUnsignedInt extends Converter {
+ override def computeValue(a: DataValuePrimitive, dstate: DState): DataValueLong = PrimType.UnsignedInt.fromNumber(a.getLong).getLong
}
-case object LongToUnsignedShort extends Converter with NumericRangeCheck {
- override def computeValue(a: DataValuePrimitive, dstate: DState): DataValueInt = {
- val res = a.getLong
- checkRange(res)
- asInt(res)
- }
- override protected val rangePrim = PrimType.UnsignedShort
+case object LongToUnsignedShort extends Converter {
+ override def computeValue(a: DataValuePrimitive, dstate: DState): DataValueInt = PrimType.UnsignedShort.fromNumber(a.getLong).getInt
}
-case object LongToNonNegativeInteger extends Converter with NumericRangeCheck {
- override def computeValue(a: DataValuePrimitive, dstate: DState): DataValueBigInt = {
- val res = a.getLong
- checkRange(res)
- asBigInt(res)
- }
- override protected val rangePrim = PrimType.NonNegativeInteger
+case object LongToNonNegativeInteger extends Converter {
+ override def computeValue(a: DataValuePrimitive, dstate: DState): DataValueBigInt = PrimType.NonNegativeInteger.fromNumber(a.getLong).getBigInt
}
-case object LongToUnsignedLong extends Converter with NumericRangeCheck {
- override def computeValue(a: DataValuePrimitive, dstate: DState): DataValueBigInt = {
- val res = a.getLong
- checkRange(res)
- asBigInt(res)
- }
- override protected val rangePrim = PrimType.UnsignedLong
+case object LongToUnsignedLong extends Converter {
+ override def computeValue(a: DataValuePrimitive, dstate: DState): DataValueBigInt = PrimType.UnsignedLong.fromNumber(a.getLong).getBigInt
}
case object NumericToDouble extends Converter {
override def computeValue(a: DataValuePrimitive, dstate: DState): DataValueDouble = asDouble(a.getAnyRef)
}
case object StringToBoolean extends Converter {
- override def computeValue(a: DataValuePrimitive, dstate: DState): DataValueBool = {
- val str = a.getString
- val res =
- if (str == "true" || str == "1") true
- else if (str == "false" || str == "0") false
- else throw new NumberFormatException("Value '%s' is not a valid boolean value {true, false, 1, 0}.".format(str))
- asBoolean(res)
- }
+ override def computeValue(a: DataValuePrimitive, dstate: DState): DataValueBool = PrimType.Boolean.fromXMLString(a.getString).getBoolean
}
case object StringToDecimal extends Converter {
- override def computeValue(a: DataValuePrimitive, dstate: DState): DataValueBigDecimal = new JBigDecimal(a.getString)
+ override def computeValue(a: DataValuePrimitive, dstate: DState): DataValueBigDecimal = PrimType.Decimal.fromXMLString(a.getString).getBigDecimal
}
case object StringToDouble extends Converter {
- override def computeValue(a: DataValuePrimitive, dstate: DState): DataValueDouble = {
- val str = a.getString
- val d =
- if (str == XMLUtils.PositiveInfinityString) JDouble.POSITIVE_INFINITY
- else if (str == XMLUtils.NegativeInfinityString) JDouble.NEGATIVE_INFINITY
- else if (str == XMLUtils.NaNString) JDouble.NaN
- else str.toDouble
- d
- }
+ override def computeValue(a: DataValuePrimitive, dstate: DState): DataValueDouble = PrimType.Double.fromXMLString(a.getString).getDouble
}
case object StringToLong extends Converter {
- override def computeValue(a: DataValuePrimitive, dstate: DState): DataValueLong = {
- val res =
- try {
- a.getString.toLong
- } catch {
- case nfe: NumberFormatException => {
- val e = new NumberFormatException("Cannot convert to type long: " + nfe.getMessage())
- throw e
- }
- }
- res
- }
+ override def computeValue(a: DataValuePrimitive, dstate: DState): DataValueLong = PrimType.Long.fromXMLString(a.getString).getLong
}
-case object StringToUnsignedLong extends Converter with NumericRangeCheck {
- override def computeValue(a: DataValuePrimitive, dstate: DState): DataValueBigInt = {
- val res = new JBigInt(a.getString)
- checkRange(res)
- res
- }
- override protected val rangePrim = PrimType.UnsignedLong
+case object StringToUnsignedLong extends Converter {
+ override def computeValue(a: DataValuePrimitive, dstate: DState): DataValueBigInt = PrimType.UnsignedLong.fromXMLString(a.getString).getBigInt
Review comment:
Missed addressing this comment. I think it would be worth wrapping these long lines.
##########
File path: daffodil-runtime1/src/main/scala/org/apache/daffodil/dpath/NodeInfo.scala
##########
@@ -452,21 +457,54 @@ object NodeInfo extends Enum {
}
}
+ trait PrimNonNumeric { self: AnyAtomic.Kind =>
+ protected def fromString(s: String): DataValuePrimitive
+ def fromXMLString(s: String): DataValuePrimitive = {
+ try {
+ fromString(s)
+ } catch {
+ case iae: IllegalArgumentException =>
+ throw new InvalidPrimitiveDataException("Value '%s' is not a valid %s: %s".format(s, this.globalQName, iae.getMessage))
+ case uri: URISyntaxException =>
+ throw new InvalidPrimitiveDataException("Value '%s' is not a valid %s: %s".format(s, this.globalQName, uri.getMessage))
+ }
+ }
+ }
+
trait PrimNumeric { self: Numeric.Kind =>
- def isValidRange(n: Number): Boolean
- def fromNumber(n: Number): DataValueNumber
+ def isValid(n: Number): Boolean
+ protected def fromNumberNoCheck(n: Number): DataValueNumber
+ def fromNumber(n: Number): DataValueNumber = {
+ if (!isValid(n))
+ throw new InvalidPrimitiveDataException("Value '%s' is out of range for type: %s".format(n, this.globalQName))
+ val num = fromNumberNoCheck(n)
+ num
+ }
+
+ protected def fromString(s: String): DataValueNumber
+ def fromXMLString(s: String): DataValueNumber = {
+ try {
+ val num = fromString(s)
+ if (!isValid(num.getNumber))
+ throw new InvalidPrimitiveDataException("Value '%s' is out of range for type: %s".format(s, this.globalQName))
+ num
+ } catch {
+ case nfe: NumberFormatException =>
+ throw new InvalidPrimitiveDataException("Value '%s' is not a valid %s".format(s, this.globalQName))
+ }
+ }
Review comment:
Super minor, but feels maybe a little odd to throw an exception inside a try block? Thoughs on moving it out, so something like this:
```scala
val num = try {
fromString(s)
} catch {
case nfd: NumberFormatException => throw new InvalidPrimitiveDataException(...)
}
if (!isValid(num.getNumber))
throw new InvalidPrimitiveDataException(...)
num
```
Makes it more clear that the NFE catch is really just catching the fromString conversion.
----------------------------------------------------------------
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] jadams-tresys commented on a change in pull request #378: Added try/catch for potential unparse exceptions
Posted by GitBox <gi...@apache.org>.
jadams-tresys commented on a change in pull request #378:
URL: https://github.com/apache/incubator-daffodil/pull/378#discussion_r421100177
##########
File path: daffodil-runtime1/src/main/scala/org/apache/daffodil/infoset/InfosetInputter.scala
##########
@@ -420,7 +420,14 @@ abstract class InfosetInputter
}
} else if (erd.outputValueCalcExpr.isEmpty) {
val primType = elem.erd.optPrimType.get
- val obj = primType.fromXMLString(txt)
+ val obj = try {
+ primType.fromXMLString(txt)
Review comment:
I've added some extra try/catch blocks in a few places, but not all of them. Some of the instances that I did not add try/catch to did not seem like it would necessarily need them. For example, there are multiple calls in XMLUtils, but those I believe are only really called in testing code. One place I think I do need to add something is in VariableUtils.convert(), but if I remember correctly we don't really want to throw exceptions from the runtime. Would the appropriate action be to catch the new exception and then do an assert?
----------------------------------------------------------------
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 #378: Added try/catch for potential unparse exceptions
Posted by GitBox <gi...@apache.org>.
mbeckerle commented on a change in pull request #378:
URL: https://github.com/apache/incubator-daffodil/pull/378#discussion_r421588622
##########
File path: daffodil-runtime1/src/main/scala/org/apache/daffodil/dpath/NodeInfo.scala
##########
@@ -594,7 +617,7 @@ object NodeInfo extends Enum {
protected sealed trait NonNegativeIntegerKind extends Integer.Kind
case object NonNegativeInteger extends PrimTypeNode(Integer, List(UnsignedLong)) with NonNegativeIntegerKind with PrimNumeric {
type Kind = NonNegativeIntegerKind
- override def fromXMLString(s: String): DataValueBigInt = new JBigInt(s)
+ override def fromString(s: String): DataValueBigInt = new JBigInt(s)
Review comment:
This isValidRange is called elsewhere in number parse primitives. Arguably, it could be checked here and exception thrown at the point where we synthesize the type, and then all the places currently calling isValidRange would instead have to catch the exception .
I think the design principle here is that we don't have context to report diagnostics properly here, so we do as little checking as possible here, and provide check calls that the calling context can use, assuming that it has better contextual information for diagnostics.
----------------------------------------------------------------
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] jadams-tresys commented on a change in pull request #378: Added try/catch for potential unparse exceptions
Posted by GitBox <gi...@apache.org>.
jadams-tresys commented on a change in pull request #378:
URL: https://github.com/apache/incubator-daffodil/pull/378#discussion_r425175178
##########
File path: daffodil-runtime1/src/main/scala/org/apache/daffodil/dpath/NodeInfo.scala
##########
@@ -452,21 +457,54 @@ object NodeInfo extends Enum {
}
}
+ trait PrimNonNumeric { self: AnyAtomic.Kind =>
+ protected def fromString(s: String): DataValuePrimitive
+ def fromXMLString(s: String): DataValuePrimitive = {
+ try {
+ fromString(s)
+ } catch {
+ case iae: IllegalArgumentException =>
+ throw new InvalidPrimitiveDataException("Value '%s' is not a valid %s: %s".format(s, this.globalQName, iae.getMessage))
+ case uri: URISyntaxException =>
+ throw new InvalidPrimitiveDataException("Value '%s' is not a valid %s: %s".format(s, this.globalQName, uri.getMessage))
+ }
+ }
+ }
+
trait PrimNumeric { self: Numeric.Kind =>
- def isValidRange(n: Number): Boolean
- def fromNumber(n: Number): DataValueNumber
+ def isValid(n: Number): Boolean
+ protected def fromNumberNoCheck(n: Number): DataValueNumber
+ def fromNumber(n: Number): DataValueNumber = {
+ if (!isValid(n))
+ throw new InvalidPrimitiveDataException("Value '%s' is out of range for type: %s".format(n, this.globalQName))
+ val num = fromNumberNoCheck(n)
+ num
+ }
+
+ protected def fromString(s: String): DataValueNumber
+ def fromXMLString(s: String): DataValueNumber = {
+ try {
+ val num = fromString(s)
+ if (!isValid(num.getNumber))
+ throw new InvalidPrimitiveDataException("Value '%s' is out of range for type: %s".format(s, this.globalQName))
+ num
+ } catch {
+ case nfe: NumberFormatException =>
+ throw new InvalidPrimitiveDataException("Value '%s' is not a valid %s".format(s, this.globalQName))
+ }
+ }
Review comment:
Will fix.
----------------------------------------------------------------
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 #378: Added try/catch for potential unparse exceptions
Posted by GitBox <gi...@apache.org>.
stevedlawrence commented on a change in pull request #378:
URL: https://github.com/apache/incubator-daffodil/pull/378#discussion_r425050969
##########
File path: daffodil-runtime1/src/main/scala/org/apache/daffodil/dpath/NodeInfo.scala
##########
@@ -462,7 +463,7 @@ object NodeInfo extends Enum {
fromString(s)
} catch {
case iae: IllegalArgumentException =>
- throw new InvalidPrimitiveDataException("%s is not a valid %s".format(iae.getMessage, this))
+ throw new InvalidPrimitiveDataException("Value '%s' is not a valid %s: %s".format(s, this, iae.getMessage))
Review comment:
Missed a this.globalQName here.
----------------------------------------------------------------
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] jadams-tresys commented on a change in pull request #378: Added try/catch for potential unparse exceptions
Posted by GitBox <gi...@apache.org>.
jadams-tresys commented on a change in pull request #378:
URL: https://github.com/apache/incubator-daffodil/pull/378#discussion_r421716842
##########
File path: daffodil-runtime1/src/main/scala/org/apache/daffodil/infoset/InfosetInputter.scala
##########
@@ -420,7 +421,14 @@ abstract class InfosetInputter
}
} else if (erd.outputValueCalcExpr.isEmpty) {
val primType = elem.erd.optPrimType.get
- val obj = primType.fromXMLString(txt)
+ val obj = try {
+ primType.fromXMLString(txt)
+ } catch {
+ case ipd: InvalidPrimitiveDataException =>
+ UnparseError(One(elem.erd.schemaFileLocation), Nope, ipd.getMessage)
+ case iae: IllegalArgumentException =>
Review comment:
Illegal argument exception gets thrown for invalid calendar, hexBinary, or URI, so I think we need it too, unless we want to just catch everything.
----------------------------------------------------------------
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 #378: Added try/catch for potential unparse exceptions
Posted by GitBox <gi...@apache.org>.
mbeckerle commented on a change in pull request #378:
URL: https://github.com/apache/incubator-daffodil/pull/378#discussion_r421576554
##########
File path: daffodil-runtime1/src/main/scala/org/apache/daffodil/dpath/NodeInfo.scala
##########
@@ -452,9 +454,30 @@ object NodeInfo extends Enum {
}
}
+ trait PrimThrowsIllegalArgument {
+ def fromString(s: String): DataValuePrimitive
+ def fromXMLString(s: String): DataValuePrimitive = {
+ try {
+ fromString(s)
+ } catch {
+ case iae: IllegalArgumentException =>
+ throw new InvalidPrimitiveDataException("%s is not a valid %s".format(iae.getMessage, this))
Review comment:
Also, since you don't know if an exception has a string message or a cause object, we have Misc.getSomeMessage(e) which returns a Some[String], i.e., you never get null back. Centralizes the logic of if you don't have a message, then check if there is a cause, and does it have a message or a cause, etc. etc.
----------------------------------------------------------------
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 #378: Added try/catch for potential unparse exceptions
Posted by GitBox <gi...@apache.org>.
stevedlawrence commented on a change in pull request #378:
URL: https://github.com/apache/incubator-daffodil/pull/378#discussion_r425049461
##########
File path: daffodil-runtime1/src/main/scala/org/apache/daffodil/dpath/ConverterOps.scala
##########
@@ -179,159 +136,80 @@ case object DoubleToBoolean extends Converter {
}
}
case object FloatToDouble extends Converter {
- override def computeValue(a: DataValuePrimitive, dstate: DState): DataValueDouble = asDouble(a.getFloat)
+ override def computeValue(a: DataValuePrimitive, dstate: DState): DataValueDouble = PrimType.Double.fromNumber(a.getFloat).getDouble
}
case object IntegerToDecimal extends Converter {
- override def computeValue(a: DataValuePrimitive, dstate: DState): DataValueBigDecimal = new JBigDecimal(a.getBigInt)
+ override def computeValue(a: DataValuePrimitive, dstate: DState): DataValueBigDecimal = PrimType.Decimal.fromNumber(a.getBigInt).getBigDecimal
}
-case object IntegerToUnsignedLong extends Converter with NumericRangeCheck {
- override def computeValue(a: DataValuePrimitive, dstate: DState): DataValueBigInt = {
- val res = a.getBigInt
- checkRange(res)
- asBigInt(res)
- }
- override protected val rangePrim = PrimType.UnsignedLong
+case object IntegerToUnsignedLong extends Converter {
+ override def computeValue(a: DataValuePrimitive, dstate: DState): DataValueBigInt = PrimType.UnsignedLong.fromNumber(a.getBigInt).getBigInt
}
case object LongToBoolean extends Converter {
override def computeValue(a: DataValuePrimitive, dstate: DState): DataValueBool = asBoolean(if (a.getLong == 0) false else true)
}
-case object LongToByte extends Converter with NumericRangeCheck {
- override def computeValue(a: DataValuePrimitive, dstate: DState): DataValueByte = {
- val res = a.getLong
- checkRange(res)
- asByte(res)
- }
- override protected val rangePrim = PrimType.Byte
+case object LongToByte extends Converter {
+ override def computeValue(a: DataValuePrimitive, dstate: DState): DataValueByte = PrimType.Byte.fromNumber(a.getLong).getByte
}
case object LongToDecimal extends Converter {
- override def computeValue(a: DataValuePrimitive, dstate: DState): DataValueBigDecimal = asBigDecimal(a.getLong)
+ override def computeValue(a: DataValuePrimitive, dstate: DState): DataValueBigDecimal = PrimType.Decimal.fromNumber(a.getLong).getBigDecimal
}
case object LongToDouble extends Converter {
- override def computeValue(a: DataValuePrimitive, dstate: DState): DataValueDouble = asDouble(a.getLong)
+ override def computeValue(a: DataValuePrimitive, dstate: DState): DataValueDouble = PrimType.Double.fromNumber(a.getLong).getDouble
}
case object LongToFloat extends Converter {
- override def computeValue(a: DataValuePrimitive, dstate: DState): DataValueFloat = asFloat(a.getLong)
+ override def computeValue(a: DataValuePrimitive, dstate: DState): DataValueFloat = PrimType.Float.fromNumber(a.getLong).getFloat
}
-case object LongToInt extends Converter with NumericRangeCheck {
- override def computeValue(a: DataValuePrimitive, dstate: DState): DataValueInt = {
- val res = a.getLong
- checkRange(res)
- asInt(res)
- }
- override protected val rangePrim = PrimType.Int
+case object LongToInt extends Converter {
+ override def computeValue(a: DataValuePrimitive, dstate: DState): DataValueInt = PrimType.Int.fromNumber(a.getLong).getInt
}
case object LongToInteger extends Converter {
- override def computeValue(a: DataValuePrimitive, dstate: DState): DataValueBigInt = asBigInt(a.getLong)
+ override def computeValue(a: DataValuePrimitive, dstate: DState): DataValueBigInt = PrimType.Integer.fromNumber(a.getLong).getBigInt
}
-case object LongToShort extends Converter with NumericRangeCheck {
- override def computeValue(a: DataValuePrimitive, dstate: DState): DataValueShort = {
- val res = a.getLong
- checkRange(res)
- asShort(res)
- }
- override protected val rangePrim = PrimType.Short
+case object LongToShort extends Converter {
+ override def computeValue(a: DataValuePrimitive, dstate: DState): DataValueShort = PrimType.Short.fromNumber(a.getLong).getShort
}
case object LongToArrayIndex extends Converter {
- override def computeValue(a: DataValuePrimitive, dstate: DState): DataValueLong = {
- val res = a.getLong
- res
- }
+ override def computeValue(a: DataValuePrimitive, dstate: DState): DataValueLong = PrimType.Long.fromNumber(a.getLong).getLong
}
-case object LongToUnsignedByte extends Converter with NumericRangeCheck {
- override def computeValue(a: DataValuePrimitive, dstate: DState): DataValueShort = {
- val res = a.getLong
- checkRange(res)
- asShort(res)
- }
- override protected val rangePrim = PrimType.UnsignedByte
+case object LongToUnsignedByte extends Converter {
+ override def computeValue(a: DataValuePrimitive, dstate: DState): DataValueShort = PrimType.UnsignedByte.fromNumber(a.getLong).getShort
}
-case object LongToUnsignedInt extends Converter with NumericRangeCheck {
- override def computeValue(a: DataValuePrimitive, dstate: DState): DataValueLong = {
- val res = a.getLong
- checkRange(res)
- res
- }
- override protected val rangePrim = PrimType.UnsignedInt
+case object LongToUnsignedInt extends Converter {
+ override def computeValue(a: DataValuePrimitive, dstate: DState): DataValueLong = PrimType.UnsignedInt.fromNumber(a.getLong).getLong
}
-case object LongToUnsignedShort extends Converter with NumericRangeCheck {
- override def computeValue(a: DataValuePrimitive, dstate: DState): DataValueInt = {
- val res = a.getLong
- checkRange(res)
- asInt(res)
- }
- override protected val rangePrim = PrimType.UnsignedShort
+case object LongToUnsignedShort extends Converter {
+ override def computeValue(a: DataValuePrimitive, dstate: DState): DataValueInt = PrimType.UnsignedShort.fromNumber(a.getLong).getInt
}
-case object LongToNonNegativeInteger extends Converter with NumericRangeCheck {
- override def computeValue(a: DataValuePrimitive, dstate: DState): DataValueBigInt = {
- val res = a.getLong
- checkRange(res)
- asBigInt(res)
- }
- override protected val rangePrim = PrimType.NonNegativeInteger
+case object LongToNonNegativeInteger extends Converter {
+ override def computeValue(a: DataValuePrimitive, dstate: DState): DataValueBigInt = PrimType.NonNegativeInteger.fromNumber(a.getLong).getBigInt
}
-case object LongToUnsignedLong extends Converter with NumericRangeCheck {
- override def computeValue(a: DataValuePrimitive, dstate: DState): DataValueBigInt = {
- val res = a.getLong
- checkRange(res)
- asBigInt(res)
- }
- override protected val rangePrim = PrimType.UnsignedLong
+case object LongToUnsignedLong extends Converter {
+ override def computeValue(a: DataValuePrimitive, dstate: DState): DataValueBigInt = PrimType.UnsignedLong.fromNumber(a.getLong).getBigInt
}
case object NumericToDouble extends Converter {
override def computeValue(a: DataValuePrimitive, dstate: DState): DataValueDouble = asDouble(a.getAnyRef)
}
case object StringToBoolean extends Converter {
- override def computeValue(a: DataValuePrimitive, dstate: DState): DataValueBool = {
- val str = a.getString
- val res =
- if (str == "true" || str == "1") true
- else if (str == "false" || str == "0") false
- else throw new NumberFormatException("Value '%s' is not a valid boolean value {true, false, 1, 0}.".format(str))
- asBoolean(res)
- }
+ override def computeValue(a: DataValuePrimitive, dstate: DState): DataValueBool = PrimType.Boolean.fromXMLString(a.getString).getBoolean
}
case object StringToDecimal extends Converter {
- override def computeValue(a: DataValuePrimitive, dstate: DState): DataValueBigDecimal = new JBigDecimal(a.getString)
+ override def computeValue(a: DataValuePrimitive, dstate: DState): DataValueBigDecimal = PrimType.Decimal.fromXMLString(a.getString).getBigDecimal
}
case object StringToDouble extends Converter {
- override def computeValue(a: DataValuePrimitive, dstate: DState): DataValueDouble = {
- val str = a.getString
- val d =
- if (str == XMLUtils.PositiveInfinityString) JDouble.POSITIVE_INFINITY
- else if (str == XMLUtils.NegativeInfinityString) JDouble.NEGATIVE_INFINITY
- else if (str == XMLUtils.NaNString) JDouble.NaN
- else str.toDouble
- d
- }
+ override def computeValue(a: DataValuePrimitive, dstate: DState): DataValueDouble = PrimType.Double.fromXMLString(a.getString).getDouble
}
case object StringToLong extends Converter {
- override def computeValue(a: DataValuePrimitive, dstate: DState): DataValueLong = {
- val res =
- try {
- a.getString.toLong
- } catch {
- case nfe: NumberFormatException => {
- val e = new NumberFormatException("Cannot convert to type long: " + nfe.getMessage())
- throw e
- }
- }
- res
- }
+ override def computeValue(a: DataValuePrimitive, dstate: DState): DataValueLong = PrimType.Long.fromXMLString(a.getString).getLong
}
-case object StringToUnsignedLong extends Converter with NumericRangeCheck {
- override def computeValue(a: DataValuePrimitive, dstate: DState): DataValueBigInt = {
- val res = new JBigInt(a.getString)
- checkRange(res)
- res
- }
- override protected val rangePrim = PrimType.UnsignedLong
+case object StringToUnsignedLong extends Converter {
+ override def computeValue(a: DataValuePrimitive, dstate: DState): DataValueBigInt = PrimType.UnsignedLong.fromXMLString(a.getString).getBigInt
Review comment:
I think this is a great refactoring, but the lines are now really long. Could you maybe move the PrimType... to a new line, e.g.:
```suggestion
override def computeValue(a: DataValuePrimitive, dstate: DState): DataValueBigInt =
PrimType.UnsignedLong.fromXMLString(a.getString).getBigInt
```
----------------------------------------------------------------
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 #378: Added try/catch for potential unparse exceptions
Posted by GitBox <gi...@apache.org>.
stevedlawrence commented on a change in pull request #378:
URL: https://github.com/apache/incubator-daffodil/pull/378#discussion_r422107673
##########
File path: daffodil-runtime1/src/main/scala/org/apache/daffodil/dpath/NodeInfo.scala
##########
@@ -594,7 +617,7 @@ object NodeInfo extends Enum {
protected sealed trait NonNegativeIntegerKind extends Integer.Kind
case object NonNegativeInteger extends PrimTypeNode(Integer, List(UnsignedLong)) with NonNegativeIntegerKind with PrimNumeric {
type Kind = NonNegativeIntegerKind
- override def fromXMLString(s: String): DataValueBigInt = new JBigInt(s)
+ override def fromString(s: String): DataValueBigInt = new JBigInt(s)
Review comment:
True, we aren't consistent about this at all right now. It's probably worth putting that change off, as you're right, it probably would be a pretty large refactor.
----------------------------------------------------------------
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] jadams-tresys merged pull request #378: Added try/catch for potential unparse exceptions
Posted by GitBox <gi...@apache.org>.
jadams-tresys merged pull request #378:
URL: https://github.com/apache/incubator-daffodil/pull/378
----------------------------------------------------------------
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 #378: Added try/catch for potential unparse exceptions
Posted by GitBox <gi...@apache.org>.
stevedlawrence commented on a change in pull request #378:
URL: https://github.com/apache/incubator-daffodil/pull/378#discussion_r420785919
##########
File path: daffodil-runtime1/src/main/scala/org/apache/daffodil/infoset/InfosetInputter.scala
##########
@@ -420,7 +420,14 @@ abstract class InfosetInputter
}
} else if (erd.outputValueCalcExpr.isEmpty) {
val primType = elem.erd.optPrimType.get
- val obj = primType.fromXMLString(txt)
+ val obj = try {
+ primType.fromXMLString(txt)
Review comment:
Grepping for ``fromXMLString``, there's a handful of other places where we call this without wrapping it with try/catch. I'm wondering if we have this same issue in other places and need the try/catch?
I'm also concerned that different primitive types use different logic in their fromXMLString implementation that might throw different exceptions that we might not be accounting for here. Thoughts on modifying the ``fromXMLString`` implementations to be more thorough about checking for exceptions, and then they rethrow a single exception that callers of ``fromXMLString`` should catch so they don't have to know about particulars of different prim type implementations? This is similar to [DAFFODIL-1681](https://issues.apache.org/jira/browse/DAFFODIL-1681).
##########
File path: daffodil-test/src/test/resources/org/apache/daffodil/section05/simple_types/SimpleTypes.tdml
##########
@@ -3981,7 +3982,27 @@
</tdml:dfdlInfoset>
</tdml:infoset>
</tdml:parserTestCase>
+
+ <!--
+ Test name: binaryInt_unparseError
+ Schema: SimpleTypes-binary
+ Purpose: Unparse a binary int with a value that is too large to be an xs:int
+ -->
+ <tdml:unparserTestCase name="binaryInt_unparseError" root="int02" roundTrip="false"
+ model="SimpleTypes-binary" description="Section 5 Schema types - int - DFDL-5-013R">
+ <tdml:infoset>
+ <tdml:dfdlInfoset>
+ <ex:int02>7089904312036126320</ex:int02>
+ </tdml:dfdlInfoset>
+ </tdml:infoset>
+ <tdml:document>7089904312036126320</tdml:document>
Review comment:
There shouldn't be a document if there are errors.
----------------------------------------------------------------
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 #378: Added try/catch for potential unparse exceptions
Posted by GitBox <gi...@apache.org>.
stevedlawrence commented on a change in pull request #378:
URL: https://github.com/apache/incubator-daffodil/pull/378#discussion_r421719260
##########
File path: daffodil-runtime1/src/main/scala/org/apache/daffodil/infoset/InfosetInputter.scala
##########
@@ -420,7 +421,14 @@ abstract class InfosetInputter
}
} else if (erd.outputValueCalcExpr.isEmpty) {
val primType = elem.erd.optPrimType.get
- val obj = primType.fromXMLString(txt)
+ val obj = try {
+ primType.fromXMLString(txt)
+ } catch {
+ case ipd: InvalidPrimitiveDataException =>
+ UnparseError(One(elem.erd.schemaFileLocation), Nope, ipd.getMessage)
+ case iae: IllegalArgumentException =>
Review comment:
Isn't that caugt by your new changes and turned into an InvalidPrimitiveDataException?
----------------------------------------------------------------
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] jadams-tresys commented on a change in pull request #378: Added try/catch for potential unparse exceptions
Posted by GitBox <gi...@apache.org>.
jadams-tresys commented on a change in pull request #378:
URL: https://github.com/apache/incubator-daffodil/pull/378#discussion_r420800840
##########
File path: daffodil-runtime1/src/main/scala/org/apache/daffodil/infoset/InfosetInputter.scala
##########
@@ -420,7 +420,14 @@ abstract class InfosetInputter
}
} else if (erd.outputValueCalcExpr.isEmpty) {
val primType = elem.erd.optPrimType.get
- val obj = primType.fromXMLString(txt)
+ val obj = try {
+ primType.fromXMLString(txt)
Review comment:
I did a look through all the different prim types and it looked like we would only see NumberFormatExceptions for all the various number primitives (int/short/float/double, etc) and IllegalArgurmentExceptions for HexBinary and Date/Time/Calendar types. Although I just double checked and realized I missed the URI type, which can potentially throw a NullPointerException or URISyntaxException. You are probably right that the right move is to modify all the primitive types to handle their own exceptions and then throw a more generic exception to be caught elsewhere.
##########
File path: daffodil-test/src/test/resources/org/apache/daffodil/section05/simple_types/SimpleTypes.tdml
##########
@@ -3981,7 +3982,27 @@
</tdml:dfdlInfoset>
</tdml:infoset>
</tdml:parserTestCase>
+
+ <!--
+ Test name: binaryInt_unparseError
+ Schema: SimpleTypes-binary
+ Purpose: Unparse a binary int with a value that is too large to be an xs:int
+ -->
+ <tdml:unparserTestCase name="binaryInt_unparseError" root="int02" roundTrip="false"
+ model="SimpleTypes-binary" description="Section 5 Schema types - int - DFDL-5-013R">
+ <tdml:infoset>
+ <tdml:dfdlInfoset>
+ <ex:int02>7089904312036126320</ex:int02>
+ </tdml:dfdlInfoset>
+ </tdml:infoset>
+ <tdml:document>7089904312036126320</tdml:document>
Review comment:
Yep, will remove.
----------------------------------------------------------------
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] jadams-tresys commented on pull request #378: Added try/catch for potential unparse exceptions
Posted by GitBox <gi...@apache.org>.
jadams-tresys commented on pull request #378:
URL: https://github.com/apache/incubator-daffodil/pull/378#issuecomment-628648689
I believe I have addressed all issues found in the review now.
----------------------------------------------------------------
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 #378: Added try/catch for potential unparse exceptions
Posted by GitBox <gi...@apache.org>.
mbeckerle commented on a change in pull request #378:
URL: https://github.com/apache/incubator-daffodil/pull/378#discussion_r421572407
##########
File path: daffodil-runtime1/src/main/scala/org/apache/daffodil/dpath/NodeInfo.scala
##########
@@ -452,9 +454,30 @@ object NodeInfo extends Enum {
}
}
+ trait PrimThrowsIllegalArgument {
+ def fromString(s: String): DataValuePrimitive
+ def fromXMLString(s: String): DataValuePrimitive = {
+ try {
+ fromString(s)
+ } catch {
+ case iae: IllegalArgumentException =>
+ throw new InvalidPrimitiveDataException("%s is not a valid %s".format(iae.getMessage, this))
Review comment:
I always like to just construct nested exceptions by passing the inner exception as a cause to the enclosing exception, and not constructing strings at all.
There's an idiom for deriving exception classes so that you have both string and throwable constructors for them. We don't do this for our exception classes, but we should so that you have string and cause constructors for them. Like this:
```
class MyException protected (m: String, cause: Throwable)
extends Exception(m, cause) { // or extend an exception base class, 2-arg constructor
def this(message: String) = this(message, null)
def this(cause: Throwable) = this(null, cause)
}
```
One can even omit the def this (m: String) constructor if the real usage pattern only involves nesting an exception.
----------------------------------------------------------------
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] jadams-tresys commented on a change in pull request #378: Added try/catch for potential unparse exceptions
Posted by GitBox <gi...@apache.org>.
jadams-tresys commented on a change in pull request #378:
URL: https://github.com/apache/incubator-daffodil/pull/378#discussion_r422413367
##########
File path: daffodil-runtime1/src/main/scala/org/apache/daffodil/dpath/NodeInfo.scala
##########
@@ -452,9 +454,30 @@ object NodeInfo extends Enum {
}
}
+ trait PrimThrowsIllegalArgument {
Review comment:
So I'm currently planning on leaving this in as I am now having PrimNumeric fromNumber/XMLString perform range checks too.
----------------------------------------------------------------
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 #378: Added try/catch for potential unparse exceptions
Posted by GitBox <gi...@apache.org>.
mbeckerle commented on a change in pull request #378:
URL: https://github.com/apache/incubator-daffodil/pull/378#discussion_r421572407
##########
File path: daffodil-runtime1/src/main/scala/org/apache/daffodil/dpath/NodeInfo.scala
##########
@@ -452,9 +454,30 @@ object NodeInfo extends Enum {
}
}
+ trait PrimThrowsIllegalArgument {
+ def fromString(s: String): DataValuePrimitive
+ def fromXMLString(s: String): DataValuePrimitive = {
+ try {
+ fromString(s)
+ } catch {
+ case iae: IllegalArgumentException =>
+ throw new InvalidPrimitiveDataException("%s is not a valid %s".format(iae.getMessage, this))
Review comment:
I always like to just construct nested exceptions by passing the inner exception as a cause to the enclosing exception, and not constructing strings at all.
There's an idiom for deriving exception classes so that you have both string and throwable constructors for them. We don't do this for our exception classes, but we should so that you have string and cause constructors for them. Like this:
```class MyException protected (m: String, cause: Throwable)
extends Exception(m, cause) { // or extend an exception base class, 2-arg constructor
def this(message: String) = this(message, null)
def this(cause: Throwable) = this(null, cause)
....
}```
One can even omit the def this (m: String) constructor if the real usage pattern only involves nesting an exception.
----------------------------------------------------------------
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 #378: Added try/catch for potential unparse exceptions
Posted by GitBox <gi...@apache.org>.
mbeckerle commented on a change in pull request #378:
URL: https://github.com/apache/incubator-daffodil/pull/378#discussion_r420835304
##########
File path: daffodil-runtime1/src/main/scala/org/apache/daffodil/infoset/InfosetInputter.scala
##########
@@ -420,7 +420,14 @@ abstract class InfosetInputter
}
} else if (erd.outputValueCalcExpr.isEmpty) {
val primType = elem.erd.optPrimType.get
- val obj = primType.fromXMLString(txt)
+ val obj = try {
+ primType.fromXMLString(txt)
Review comment:
Nah. Nevermind. Type tags would be needed due to type erasure, etc. etc. Too complicated.
----------------------------------------------------------------
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 #378: Added try/catch for potential unparse exceptions
Posted by GitBox <gi...@apache.org>.
mbeckerle commented on a change in pull request #378:
URL: https://github.com/apache/incubator-daffodil/pull/378#discussion_r421590480
##########
File path: daffodil-runtime1/src/main/scala/org/apache/daffodil/dpath/NodeInfo.scala
##########
@@ -594,7 +617,7 @@ object NodeInfo extends Enum {
protected sealed trait NonNegativeIntegerKind extends Integer.Kind
case object NonNegativeInteger extends PrimTypeNode(Integer, List(UnsignedLong)) with NonNegativeIntegerKind with PrimNumeric {
type Kind = NonNegativeIntegerKind
- override def fromXMLString(s: String): DataValueBigInt = new JBigInt(s)
+ override def fromString(s: String): DataValueBigInt = new JBigInt(s)
Review comment:
I'm not saying that this is a *good* design point, I think probably it would be cleaner contract if these types succeded and produced a value of the right type in the right range, or throw an appropriate exception. But that will require quite a bit of refactoring, will change some diagnostic messages, etc.
----------------------------------------------------------------
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 #378: Added try/catch for potential unparse exceptions
Posted by GitBox <gi...@apache.org>.
mbeckerle commented on a change in pull request #378:
URL: https://github.com/apache/incubator-daffodil/pull/378#discussion_r420817939
##########
File path: daffodil-runtime1/src/main/scala/org/apache/daffodil/infoset/InfosetInputter.scala
##########
@@ -420,7 +420,14 @@ abstract class InfosetInputter
}
} else if (erd.outputValueCalcExpr.isEmpty) {
val primType = elem.erd.optPrimType.get
- val obj = primType.fromXMLString(txt)
+ val obj = try {
+ primType.fromXMLString(txt)
Review comment:
Yeah, we should make fromXMLString a tighter API which throws a specific exception when unable to convert. This is a case where Scala not enforcing handling of exceptions is not doing us a favor however.
A perhaps more Scala-ish way of doing this is to make it return a Try[String] object instead of throwing, though we tend to avoid objects like Try, Option/Some, or little crud tuples, etc. in the runtime. Unfortunately we don't have our own non-allocating version of Try[T] as yet.
##########
File path: daffodil-test/src/test/resources/org/apache/daffodil/section05/simple_types/SimpleTypes.tdml
##########
@@ -3981,7 +3982,27 @@
</tdml:dfdlInfoset>
</tdml:infoset>
</tdml:parserTestCase>
+
+ <!--
+ Test name: binaryInt_unparseError
+ Schema: SimpleTypes-binary
+ Purpose: Unparse a binary int with a value that is too large to be an xs:int
+ -->
+ <tdml:unparserTestCase name="binaryInt_unparseError" root="int02" roundTrip="false"
+ model="SimpleTypes-binary" description="Section 5 Schema types - int - DFDL-5-013R">
+ <tdml:infoset>
+ <tdml:dfdlInfoset>
+ <ex:int02>7089904312036126320</ex:int02>
+ </tdml:dfdlInfoset>
+ </tdml:infoset>
+ <tdml:document>7089904312036126320</tdml:document>
Review comment:
This is a TDML runner bug. It's not checking for this. The XML schema for TDML could be tightened up to enforce this I think. There's already a JIRA ticket about this.
----------------------------------------------------------------
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 #378: Added try/catch for potential unparse exceptions
Posted by GitBox <gi...@apache.org>.
stevedlawrence commented on a change in pull request #378:
URL: https://github.com/apache/incubator-daffodil/pull/378#discussion_r421443756
##########
File path: daffodil-runtime1/src/main/scala/org/apache/daffodil/infoset/InfosetInputter.scala
##########
@@ -420,7 +421,14 @@ abstract class InfosetInputter
}
} else if (erd.outputValueCalcExpr.isEmpty) {
val primType = elem.erd.optPrimType.get
- val obj = primType.fromXMLString(txt)
+ val obj = try {
+ primType.fromXMLString(txt)
+ } catch {
+ case ipd: InvalidPrimitiveDataException =>
+ UnparseError(One(elem.erd.schemaFileLocation), Nope, ipd.getMessage)
+ case iae: IllegalArgumentException =>
Review comment:
We shouldn't needs this IllegalArgumentException anymore right?
##########
File path: daffodil-runtime1/src/main/scala/org/apache/daffodil/dpath/NodeInfo.scala
##########
@@ -452,9 +454,30 @@ object NodeInfo extends Enum {
}
}
+ trait PrimThrowsIllegalArgument {
Review comment:
I'm not sure this separatation between IllegalArgument and NumberFormatException gains a whole lot. It does mean we are a little more strict on what we expect to be thrown, but at the cost of duplicating code and I think making it a little harder to read. I don't think we lose much by catching both execptions in a single fromXMLString call.
##########
File path: daffodil-core/src/main/scala/org/apache/daffodil/dsom/ElementBase.scala
##########
@@ -342,7 +343,12 @@ trait ElementBase
// will work.
//
val str = defaultValueAsString
- val value = primType.fromXMLString(str)
+ val value = try {
+ primType.fromXMLString(str)
+ } catch {
+ case ipd: InvalidPrimitiveDataException =>
+ SDE("The default value (%s) is not a valid %s: %s".format(str, primType, ipd.getMessage))
Review comment:
This seems like it's going to have repetitive information. We can asume InvalidPrimiteDataException has the information about the string value and the type, so we don't need to duplicated it this SDE. The only extra informaion this SDE needs to provided is that the invalid thing is a default value. How about something like this:
```suggestion
SDE("Invalid default value: %s".format(ipd.getMessage))
```
##########
File path: daffodil-runtime1/src/main/scala/org/apache/daffodil/dpath/NodeInfo.scala
##########
@@ -594,7 +617,7 @@ object NodeInfo extends Enum {
protected sealed trait NonNegativeIntegerKind extends Integer.Kind
case object NonNegativeInteger extends PrimTypeNode(Integer, List(UnsignedLong)) with NonNegativeIntegerKind with PrimNumeric {
type Kind = NonNegativeIntegerKind
- override def fromXMLString(s: String): DataValueBigInt = new JBigInt(s)
+ override def fromString(s: String): DataValueBigInt = new JBigInt(s)
Review comment:
Looks like we aren't actually validating that the string is negative? We should add some range checking.
In fact, looks like all of our unsinged primitives don't validate that result of fromXMLString is unsigned. Should we add an ``isValidRange`` check in ``fromXMLString``, or maybe just have each of the unsigned prims manually check inside fromString?
##########
File path: daffodil-test/src/test/scala/org/apache/daffodil/section05/simple_types/TestSimpleTypes.scala
##########
@@ -421,6 +421,10 @@ class TestSimpleTypes {
@Test def test_int_binary_04() { runner.runOneTest("int_binary_04") }
@Test def test_int_binary_05() { runner.runOneTest("int_binary_05") }
+ @Test def test_binaryInt_unparseError() { runner.runOneTest("binaryInt_unparseError") }
+ @Test def test_hexBinary_unparseError() { runner.runOneTest("hexBinary_unparseError") }
+ @Test def test_calendar_unparseError() { runner.runOneTest("calendar_unparseError") }
+
Review comment:
Suggest adding similar tests for all of our priitive types? That would be good confirmation that DAFFODIL-1681 is resolved, in addition to the original DAFFODIL-2124 (make sure to update the commit message to reference both tickets).
##########
File path: daffodil-runtime1/src/main/scala/org/apache/daffodil/dpath/NodeInfo.scala
##########
@@ -452,9 +454,30 @@ object NodeInfo extends Enum {
}
}
+ trait PrimThrowsIllegalArgument {
+ def fromString(s: String): DataValuePrimitive
+ def fromXMLString(s: String): DataValuePrimitive = {
+ try {
+ fromString(s)
+ } catch {
+ case iae: IllegalArgumentException =>
+ throw new InvalidPrimitiveDataException("%s is not a valid %s".format(iae.getMessage, this))
Review comment:
Thoughts on using just ``s`` rather than ``iae.getMessage`` or ``nfe.getMessage`` and ignore whatever is in the exception? I'm concerened that the exception string isn't always going to make sense with "is not a valid %s". For example, it looks like we just use scala's ``String.toByte`` to for string to byte conversion. If a string is out of range for a byte, it results in the exception message
```
java.lang.NumberFormatException: Value out of range. Value:"256" Radix:10"
```
So this error message would become
```
Value out of range. Value:"256" Radix:10 is not a valid byte
```
which i find a little confusing. It's maybe nice that we get "out of range" information, but the radix info is just confusing and it doesn't fit well with the "is not a valid byte".
I'd also reccomend ``this.globalQName`` for the type, instead of just ``this``. That includes the XSD namespaces so makes it clear that the restriction is based on the XSD type system.
----------------------------------------------------------------
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 #378: Added try/catch for potential unparse exceptions
Posted by GitBox <gi...@apache.org>.
mbeckerle commented on a change in pull request #378:
URL: https://github.com/apache/incubator-daffodil/pull/378#discussion_r421557962
##########
File path: daffodil-runtime1/src/main/scala/org/apache/daffodil/infoset/InfosetInputter.scala
##########
@@ -420,7 +420,14 @@ abstract class InfosetInputter
}
} else if (erd.outputValueCalcExpr.isEmpty) {
val primType = elem.erd.optPrimType.get
- val obj = primType.fromXMLString(txt)
+ val obj = try {
+ primType.fromXMLString(txt)
Review comment:
If it's in the scope of a parse call, then you have to catch it and return a failed status with the cause being the caught exception. In unparser you convert the exception to a UnparseError which is then rethrown.
----------------------------------------------------------------
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] jadams-tresys commented on a change in pull request #378: Added try/catch for potential unparse exceptions
Posted by GitBox <gi...@apache.org>.
jadams-tresys commented on a change in pull request #378:
URL: https://github.com/apache/incubator-daffodil/pull/378#discussion_r422415041
##########
File path: daffodil-runtime1/src/main/scala/org/apache/daffodil/dpath/NodeInfo.scala
##########
@@ -452,9 +454,30 @@ object NodeInfo extends Enum {
}
}
+ trait PrimThrowsIllegalArgument {
+ def fromString(s: String): DataValuePrimitive
+ def fromXMLString(s: String): DataValuePrimitive = {
+ try {
+ fromString(s)
+ } catch {
+ case iae: IllegalArgumentException =>
+ throw new InvalidPrimitiveDataException("%s is not a valid %s".format(iae.getMessage, this))
+ }
+ }
+ }
+
trait PrimNumeric { self: Numeric.Kind =>
def isValidRange(n: Number): Boolean
def fromNumber(n: Number): DataValueNumber
+ def fromString(s: String): DataValueNumber
Review comment:
fromString will be protected now. As far as I can tell, I believe the string is already converted. I at least haven't run into any issues in any of our tests or any of the schema tests. I will add a test to verify this though.
----------------------------------------------------------------
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 #378: Added try/catch for potential unparse exceptions
Posted by GitBox <gi...@apache.org>.
mbeckerle commented on a change in pull request #378:
URL: https://github.com/apache/incubator-daffodil/pull/378#discussion_r424773176
##########
File path: daffodil-core/src/main/scala/org/apache/daffodil/dsom/SimpleTypes.scala
##########
@@ -568,7 +569,11 @@ final class EnumerationDef(
override lazy val optRepType = parentType.optRepType
lazy val enumValueRaw: String = (xml \ "@value").head.text
- lazy val enumValueCooked: DataValuePrimitive = parentType.primType.fromXMLString(enumValueRaw)
+ lazy val enumValueCooked: DataValuePrimitive = try {
+ parentType.primType.fromXMLString(enumValueRaw)
+ } catch {
+ case e: InvalidPrimitiveDataException => SDE("Invalid data for enumeration: %s".format(e.getMessage))
Review comment:
Remove call to .format. Unnecessary as SDE takes a format string and varargs for the arguments to that format string.
##########
File path: daffodil-runtime1/src/main/scala/org/apache/daffodil/processors/VariableMap1.scala
##########
@@ -105,8 +106,14 @@ object VariableUtils {
bindings.foreach { b => currentVMap.setExtVariable(b.varQName, b.varValue, referringContext) }
}
- def convert(v: String, rd: VariableRuntimeData): DataValuePrimitive =
- rd.primType.fromXMLString(v)
+ def convert(v: String, rd: VariableRuntimeData, referringContext: ThrowsSDE): DataValuePrimitive = {
+ try {
+ rd.primType.fromXMLString(v)
+ } catch {
+ case e: InvalidPrimitiveDataException => referringContext.SDE("Error processing variable %s: %s".format(rd.globalQName, e.getMessage))
Review comment:
Remove .format call.
Line is too long also.
----------------------------------------------------------------
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] jadams-tresys commented on a change in pull request #378: Added try/catch for potential unparse exceptions
Posted by GitBox <gi...@apache.org>.
jadams-tresys commented on a change in pull request #378:
URL: https://github.com/apache/incubator-daffodil/pull/378#discussion_r422413733
##########
File path: daffodil-runtime1/src/main/scala/org/apache/daffodil/dpath/NodeInfo.scala
##########
@@ -594,7 +617,7 @@ object NodeInfo extends Enum {
protected sealed trait NonNegativeIntegerKind extends Integer.Kind
case object NonNegativeInteger extends PrimTypeNode(Integer, List(UnsignedLong)) with NonNegativeIntegerKind with PrimNumeric {
type Kind = NonNegativeIntegerKind
- override def fromXMLString(s: String): DataValueBigInt = new JBigInt(s)
+ override def fromString(s: String): DataValueBigInt = new JBigInt(s)
Review comment:
I am now rolling in changes for DAFFODIL-2338 into this pull request. Ended up not needing such a large refactor.
----------------------------------------------------------------
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] jadams-tresys commented on a change in pull request #378: Added try/catch for potential unparse exceptions
Posted by GitBox <gi...@apache.org>.
jadams-tresys commented on a change in pull request #378:
URL: https://github.com/apache/incubator-daffodil/pull/378#discussion_r424771838
##########
File path: daffodil-runtime1/src/main/scala/org/apache/daffodil/infoset/InfosetInputter.scala
##########
@@ -413,14 +416,23 @@ abstract class InfosetInputter
} catch {
case ex: NonTextFoundInSimpleContentException =>
UnparseError(One(elem.erd.schemaFileLocation), Nope, ex.getMessage())
+ case uri: URISyntaxException =>
Review comment:
Missed this before pushing up my last commit, will fix first thing tomorrow.
----------------------------------------------------------------
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