Re: [PR] fn:count(non-array) must issue SDE [daffodil]

["stevedlawrence (via GitHub)" <gi...@apache.org>]

stevedlawrence commented on code in PR #1137:
URL: https://github.com/apache/daffodil/pull/1137#discussion_r1479861987


##########
daffodil-core/src/main/scala/org/apache/daffodil/core/dpath/Expression.scala:
##########
@@ -744,21 +753,38 @@ abstract class PathExpression() extends Expression {
    * about an array such as its current count.
    */
   lazy val isPathToOneWholeArray: Boolean = {
-    if (steps == Nil) false // root is never an array
+    isNotRootOrSelf &&
+    steps.last.isArray &&
+    priorStepsHaveIndexingPredicates
+  }
+
+  /**
+   * Path to a single array element without a [N] predicate qualifying it
+   * or to an optional element.
+   *
+   * That is, the kind of path expression used to access information
+   * about an array or optional such as its current count.
+   */
+  override lazy val isPathToOneWholeArrayOrOptional: Boolean = {
+    isNotRootOrSelf &&
+    steps.last.isArrayOrOptional &&
+    priorStepsHaveIndexingPredicates
+  }
+
+  private lazy val isNotRootOrSelf: Boolean = {
     if (steps == Nil) false // root is never an array
     else if (isSelf) false // self path is never an array

Review Comment:
   These comments mention "an array", but this val name no longer has anything to do with this being an array or not. Suggest we just remove them to avoid confusion. The name now is pretty clear what it's checking and it directly matches the code.



##########
daffodil-core/src/main/scala/org/apache/daffodil/core/dpath/Expression.scala:
##########
@@ -2295,6 +2332,20 @@ abstract class FunctionCallBase(
     res
   }
 
+  protected def checkArgArrayOrOptional(): Unit = {

Review Comment:
   If this function is going to be in FunctionCallBase, then it would be useful if it could be generic enough to work for any possible function. Right now, it only looks at expression.head, so it's only useful for functions that require array/optional as the first function parameter. I'd suggest changing it to allow it to pass in an index that can be used to pick which expression it checks. The fn:count function would call it like `checkArgArrayOrOption(0)`.
   
   Alternatively, move this logic into FunctionCallArrayBase, since it's likely the only thing that need will need this kindof check. It already has `arrPath.isPathToOneWholeArray`, maybe that just needs to be updated to require this?
   
   That said, I'm not sure why arrayPath.isPathToOneWholeArray in FunctionCallArrayBase doesn't already result in an SDE. Seems like that's already doing the check we want. I wonder if there's just a bug there and that's not getting checked correctly, and we don't actually need this new checkArgArrayOrOptional check?



##########
daffodil-core/src/main/scala/org/apache/daffodil/core/dpath/Expression.scala:
##########
@@ -744,21 +753,38 @@ abstract class PathExpression() extends Expression {
    * about an array such as its current count.
    */
   lazy val isPathToOneWholeArray: Boolean = {
-    if (steps == Nil) false // root is never an array
+    isNotRootOrSelf &&
+    steps.last.isArray &&
+    priorStepsHaveIndexingPredicates
+  }
+
+  /**
+   * Path to a single array element without a [N] predicate qualifying it
+   * or to an optional element.
+   *
+   * That is, the kind of path expression used to access information
+   * about an array or optional such as its current count.
+   */
+  override lazy val isPathToOneWholeArrayOrOptional: Boolean = {
+    isNotRootOrSelf &&
+    steps.last.isArrayOrOptional &&
+    priorStepsHaveIndexingPredicates
+  }
+
+  private lazy val isNotRootOrSelf: Boolean = {
     if (steps == Nil) false // root is never an array
     else if (isSelf) false // self path is never an array
-    else
-      steps.last.isArray &&
-      !steps.last.pred.isDefined && // last cannot have a [N] pred
-      steps.dropRight(1).forall {
-        // for all the prior steps
-        // if they mention an array element, there
-        // must be a [N] predicate.
-        step =>
-          if (step.isInstanceOf[Up]) true
-          else if (step.isArray) step.pred.isDefined
-          else true
-      }
+    else true
+  }
+
+  private lazy val priorStepsHaveIndexingPredicates: Boolean = {
+    steps.last.pred.isEmpty && // last cannot have a [N] pred
+    steps.dropRight(1).forall {
+      case _: Up => true
+      case _: Up2 => SDE("Up2 step must not have indexing")
+      case _: SelfStepExpression => SDE("Self step must not have indexing")
+      case step => !step.isArray || step.pred.isDefined

Review Comment:
   I would expect `Up` and `Up2` to have the exact same logic. They are basically the same thing, just one asserts the parent QName must be a specific name. I'm not sure why one is `true` and the other is an SDE? I would expect these could be combined into a single `case _: UpStepExpression`.
   
   Also, "Up2" in the error message is an internal identifier that probably won't make sense to users. Maybe just call it an "up step".
   
   Though, thinking about this more, I don't think I understand these Up2 and SelfStep case. Say we have an expression like `fn:count(../parent::foo/bar/baz)`. I believe this function will throw an SDE here because of the `parent:foo` up-step? But this path is perfectly fine.
   
   Based on the error message, I think this case trying to check against something like `fn:count(../..[2]/some/other/element)', where the `..[2]` isn't legal? In fact, a related issue is mentioned in a comment in `SelfStepExpression` and references DAFFODIL-2182. So we probably aren't dealing with predicates with up- and self-steps correctly, but I'm not sure this is the right function, or PR, to fix it. The goal of this function just wants to make sure any steps to arrays have a predicate, which I think the last case correctly checks.
   
   Up- and self-steps can technically go to arrays (i.e. isArray can be true for them) but they always resolve to a single instance in the array, so they don't actually need predicates, so instead of SDE'ing, I think they just want to be `true`, like the Up-step case. So I think those other cases just want to be something like:
   
   ```scala
   // up and self expressions do not need a predicate because they always reference a single element, even if that element is part of an array
   case _: UpStepExpression | _: SelfStepExpression => true
   // either not an array, or is an array with a predicate
   case step => !step.isArray || step.pred.isDefined
   ```
   
   Though, we already have that check here:
   
   https://github.com/apache/daffodil/blob/main/daffodil-core/src/main/scala/org/apache/daffodil/core/dpath/Expression.scala#L1223
   
   I wonder if the issue is that the `targetType` for the `fn:count` is `NodeInfo.Exists`, which means that part of code that normally requires predicates isn't doing it? I think that is a bug? Maybe that wants to be changed to something like:
   
   ```scala
   } else if (!isLast && targetType == NodeInfo.Exists) {
     new DownArrayExists(nqn)
   } else {
   ```
   
   This way, even with fn:exists or fn:count, we still require a predicate for all intermediate named steps. The only one that doesn't require a predicate is the last step. This means you can no longer do something like this:
   
   ```
   fn:exists(/path/arrayOne/foo/bar/arrayTwo)
   ```
   Where arrayOne is an array without a predicate. You could do that in the past, but that doesnt make any sense. Only the arrayTwo step can not have a predicate. I'm not even sure what would currently happen in that case, since we require a specific instance of arrayOne to look for the foo child.
   
   Sorry for the long wall of text, but in sumary, I think we just need the `!isLast` change, and then priorStepsHavingIndexingPredicates goes away, since that is now enforced by the isLast change.



-- 
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.

To unsubscribe, e-mail: commits-unsubscribe@daffodil.apache.org

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