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 2021/06/07 14:07:07 UTC
[GitHub] [daffodil] tuxji commented on a change in pull request #586: Improve setting of external variables
tuxji commented on a change in pull request #586:
URL: https://github.com/apache/daffodil/pull/586#discussion_r646598820
##########
File path: daffodil-japi/src/main/scala/org/apache/daffodil/japi/Daffodil.scala
##########
@@ -646,20 +648,30 @@ class DataProcessor private[japi] (private var dp: SDataProcessor)
* @see <a target="_blank" href='https://daffodil.apache.org/configuration/'>Daffodil Configuration File</a> - Daffodil configuration file format
*
* @param extVars file to read DFDL variables from.
+ * @throws ExternalVariableException if an error occurs while setting an external variable
*/
@deprecated("Use withExternalVariables.", "2.6.0")
- def setExternalVariables(extVars: File): Unit =
- dp = dp.withExternalVariables(extVars)
+ @throws(classOf[ExternalVariableException])
+ def setExternalVariables(extVars: File): Unit = {
+ //$COVERAGE-OFF$
+ try { dp = dp.withExternalVariables(extVars) }
+ catch { case e: SExternalVariableException => throw new ExternalVariableException(e.getMessage) }
+ //$COVERAGE-ON$
+ }
/**
* Obtain a new [[DataProcessor]] with external variables read from a Daffodil configuration file
*
* @see <a target="_blank" href='https://daffodil.apache.org/configuration/'>Daffodil Configuration File</a> - Daffodil configuration file format
*
* @param extVars file to read DFDL variables from.
+ * @throws ExternalVariableException if an error occurs while setting an external variable
*/
- def withExternalVariables(extVars: File): DataProcessor =
- copy(dp = dp.withExternalVariables(extVars))
+ @throws(classOf[ExternalVariableException])
+ def withExternalVariables(extVars: File): DataProcessor = {
+ try { copy(dp = dp.withExternalVariables(extVars)) }
+ catch { case e: SExternalVariableException => throw new ExternalVariableException(e.getMessage) }
Review comment:
I was worried codecov wasn't working right since it marked line 673 as not covered even though TestJavaAPI.java catches ExternalVariableException exceptions. Then I realized this method takes a File as an argument, not an AbstractMap which the test passes. Relieved to see codecov isn't marking lines with spurious "false positive" errors.
##########
File path: daffodil-runtime1/src/main/scala/org/apache/daffodil/processors/VariableMap1.scala
##########
@@ -459,66 +454,75 @@ class VariableMap private(vTable: Map[GlobalQName, ArrayBuffer[VariableInstance]
variableInstances.get.trimEnd(1)
}
-
- private lazy val externalVarGlobalQNames: Seq[GlobalQName] =
- vTable.map { case (_, variableInstances) if (variableInstances.last.rd.external) => variableInstances.last.rd.globalQName }.toSeq
-
/**
* Assigns an external variable and sets the variables state to VariableSet
*/
- def setExtVariable(bindingQName: RefQName, newValue: DataValuePrimitive, referringContext: ThrowsSDE) = {
- val varQName: RefQName =
+ def setExtVariable(bindingQName: RefQName, newValue: String, referringContext: ThrowsSDE) = {
+
+ val optVariableInstances =
if (bindingQName.namespace == UnspecifiedNamespace) {
- // The external variable binding was hoping to get away with not specifying a namespace.
- val candidateExtVarsQNames = externalVarGlobalQNames.filter {
- _.local == bindingQName.local
+ // The external variable binding was hoping to get away with not
+ // specifying a namespace. Let's try to find an unambiguous variable
+ // using just the local name. Note that this ignores the external value
+ // of variables. If we find a single variable, we'll check it's
+ // external value later. If we find two or more variables with the same
+ // name, we just consider that an ambiguity error. There is a chance of
+ // solving this ambiguity if only one of those varibles was external,
+ // but it's safer to err on the side of caution and require the user to
+ // be explicit about which variable they intended to set.
+ val candidates = vTable.filter { case (key, _) =>
+ key.local == bindingQName.local
}
- candidateExtVarsQNames match {
- case Seq() => bindingQName // just pass through. Will fail below when it isn't found
- case Seq(candidate) => candidate.toRefQName
+ candidates.size match {
+ case 0 => None
+ case 1 => Some(candidates.head._2)
case _ => {
- // ambiguous. There's multiple such.
- referringContext.SDE(
- "The externally defined variable binding %s is ambiguous. " +
- "A namespace is required to resolve the ambiguity.\nFound:\t%s",
- bindingQName, candidateExtVarsQNames.mkString(", "))
+ val msg = "External variable binding %s is ambiguous. A namespace is required to resolve the ambiguity. Found variables: %s".format(
+ bindingQName,
+ candidates.keys.map(_.toString).mkString(", "))
+ throw new ExternalVariableException(msg)
}
}
} else {
- bindingQName
+ vTable.get(bindingQName.toGlobalQName)
}
- val variableInstances = vTable.get(varQName.toGlobalQName)
- if (!variableInstances.isDefined)
- referringContext.schemaDefinitionError("unknown variable %s", varQName)
- else {
- val variable = variableInstances.get.last
- variable.state match {
- case VariableDefined if (!variable.rd.external) => {
- referringContext.SDE("Cannot set variable %s externally. State was: %s. Existing value: %s.",
- variable.rd.globalQName, VariableDefined, variable.value)
- }
-
- case VariableUndefined if (!variable.rd.external) => {
- referringContext.SDE("Cannot set variable %s externally. State was: %s.", variable.rd.globalQName, VariableUndefined)
+ optVariableInstances match {
+ case None => throw new ExternalVariableException("Variable definition not found: " + bindingQName)
+ case Some(variableInstances) => {
+ // This array of VariableInstances comes from the VariableMap that is
+ // part of the DataProcessor before being copied to pstate/ustate when
+ // a parse/unparse is started. So this array should contain only a
+ // single instance
+ Assert.invariant(variableInstances.size == 1)
+ val variable = variableInstances(0)
+ if (!variable.rd.external) {
+ throw new ExternalVariableException("Variable cannot be set externally: " + variable.rd.globalQName)
}
-
- case VariableSet => {
- referringContext.SDE("Cannot externally set variable %s twice. State was: %s. Existing value: %s",
- variable.rd.globalQName, VariableSet, variable.value)
- }
-
- case VariableRead => {
- referringContext.SDE("Cannot externally set variable %s after reading the default value. State was: %s. Existing value: %s",
- variable.rd.globalQName, VariableSet, variable.value)
- }
-
- case _ => {
- val value = VariableUtils.convert(newValue.getAnyRef.toString, variable.rd, referringContext)
- variable.setValue(value)
- variable.setState(VariableDefined)
+ variable.state match {
+ case VariableDefined | VariableUndefined => {
+ val value =
+ try {
+ variable.rd.primType.fromXMLString(newValue)
+ } catch {
+ case e: InvalidPrimitiveDataException => {
+ val msg = "Value for variable %s is not a valid %s: %s".format(
+ variable.rd.globalQName,
+ variable.rd.primType.globalQName,
+ newValue)
+ throw new ExternalVariableException(msg)
Review comment:
Yes, let's merge this as is and redesign the Daffodil APIs later.
I'm a little sad to see duplication of test DFDL schemas between japi and sapi tests. Can we put test DFDL schemas in only one place when we redesign the APIs later?
--
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