[GitHub] [daffodil] stevedlawrence commented on a change in pull request #586: Improve setting of external variables

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

stevedlawrence commented on a change in pull request #586:
URL: https://github.com/apache/daffodil/pull/586#discussion_r645755672



##########
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:
       I started with the approach of having multiple exceptions, but felt it was a bit tedious because the way are japi/sapi would also need to have all those exceptions and have converters from the "internal" exceptions to the "public" API exceptions. Which is also why I just went with not chaining the exception here--I didn't want to have to chain it in the public API. Maybe it's worth it 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