stevedlawrence commented on code in PR #1574:
URL: https://github.com/apache/daffodil/pull/1574#discussion_r2429978972
##########
daffodil-core/src/main/scala/org/apache/daffodil/core/dpath/Expression.scala:
##########
@@ -780,6 +780,23 @@ sealed abstract class StepExpression(val step: String, val
pred: Option[Predicat
)
}
+ def checkIfTargetIsLastStepAndArray(): Unit = {
Review Comment:
I'm not sure this function belongs in StepExpression. The assumptions and
error message are specific to UpSteps, so as written it should never be called
from any other StepExpression so probably doesn't belong here to prevent
someone from accidentally calling it.
Note that this is different from checkIfNodeIndexedLikeArray, which does not
make any assumptions about what the step kind it. It just should be called if
the step should disallow indexing, which all steps might need to do.
Suggest we just move this function to UpStepExpressions, or just inline the
logic into the UpStepExpression compiledDPath function.
##########
daffodil-core/src/main/scala/org/apache/daffodil/core/dpath/Expression.scala:
##########
@@ -780,6 +780,23 @@ sealed abstract class StepExpression(val step: String, val
pred: Option[Predicat
)
}
+ def checkIfTargetIsLastStepAndArray(): Unit = {
+ if (isLastStep && isArray) {
+ if (tunable.allowLastUpStepToResolveToArray) {
+ SDW(
+ WarnID.DeprecatedLastUpStepToResolveToArray,
+ ".. resolving to an array is deprecated. Try ../../$array_name
instead. Offending expression: '%s'.",
Review Comment:
Some thoughts on these diagnostics messages
1. It's potentially cofusing to start a sentence with '..'. Maybe reword to
something like
> Last path step '..' resolving to an array is deprecated....
2. I would avoid using `$array_name` since it looks like a variable (both
Scala and DFDL). I generally use angle brackets, e.g. `<array_name>` but
something else that has unique syntax if fine
3. Is it normal to include the whole expression in the diagnostics? I feel
like we don't normally do that since the schema file location is available in
the diagnostic and the user can use that to find the expression.
##########
daffodil-test/src/test/resources/org/apache/daffodil/section15/choice_groups/choice-unparse2.tdml:
##########
@@ -214,6 +214,11 @@
</ex:r>
</tdml:dfdlInfoset>
</tdml:infoset>
+
+ <tdml:warnings>
+ <tdml:warning>deprecatedLastUpStepToResolveToArray</tdml:warning>
Review Comment:
I think we maybe need to make a decision on if we will allow backwards
incompatible changes in 4.1.0. It's a minor release so technically we shouldn't
but we can always say certain things are bugs and were not intended to be
supported. I have no strong opinion either way.
This particular issue seems to only affect mil-std-2045/VMF, though other
schemas we don't know about could possibly use the `fn:count(..)` thing.
##########
daffodil-core/src/main/scala/org/apache/daffodil/core/dpath/Expression.scala:
##########
@@ -1062,11 +1079,13 @@ sealed abstract class UpStepExpression(s: String,
predArg: Option[PredicateExpre
extends StepExpression(s, predArg) {
final override lazy val compiledDPath = {
- val areAllArrays = isLastStep && stepElements.forall {
+ checkIfNodeIndexedLikeArray()
+ checkIfTargetIsLastStepAndArray()
+ lazy val areAllArrays = isLastStep && stepElements.forall {
Review Comment:
Minor cleanup, but I think we can just change this to `isLastStep && isArray
&& isTargetType == NodeInfo.Array`? I think we have a check that ensures steps
are either all arrays or all non-arrays, so isArray is essentially the same as
checking `forall { _.isArray }`--if one stepElement is an array (i.e. isArray
== true) then they all must be arrays.
##########
daffodil-propgen/src/main/resources/org/apache/daffodil/xsd/dafext.xsd:
##########
@@ -137,6 +137,15 @@
</xs:documentation>
</xs:annotation>
</xs:element>
+ <xs:element name="allowLastUpStepToResolveToArray" type="xs:boolean"
default="true" minOccurs="0">
+ <xs:annotation>
+ <xs:documentation>
+ When .. (UpStep) is the last step in an expression, it should
only ever return a single node,
+ not an array. Issue a warning when UpStep is the last step and
resolves to an array element,
+ otherwise (if false) issue an SDE.
+ </xs:documentation>
Review Comment:
Minor tweak, maybe say "If true, allow this but issue a warning. If false,
issue an SDE."
Also maybe say something to indicate a value of "true" is deprecated and
might be ignored in future versions.
##########
daffodil-core/src/main/scala/org/apache/daffodil/core/dpath/Expression.scala:
##########
@@ -1062,11 +1079,13 @@ sealed abstract class UpStepExpression(s: String,
predArg: Option[PredicateExpre
extends StepExpression(s, predArg) {
final override lazy val compiledDPath = {
- val areAllArrays = isLastStep && stepElements.forall {
+ checkIfNodeIndexedLikeArray()
+ checkIfTargetIsLastStepAndArray()
+ lazy val areAllArrays = isLastStep && stepElements.forall {
_.isArray
} && targetType == NodeInfo.Array
checkIfNodeIndexedLikeArray()
- if (areAllArrays) {
+ if (tunable.allowLastUpStepToResolveToArray && areAllArrays) {
Review Comment:
Is this tunable check needed? If `allowLastUpStepToResolveToArray` is false
then I think we would have SDE'ed in `checkIfTargetIsLastStepAndArray`. So if
we get here, I think this tunable must always be true.
Based on the comment above, maybe we drop areAllArrays and just do this:
```scala
checkIfNodeIndexedLikeArray()
checkIfTargetIsLastStepAndArray()
if (isLastStep && isArray && targetType == NodeInfo.Array) {
CompiledDPath(UpMoveArray)
} else {
CompiledDPath(UpMove)
}
```
--
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: [email protected]
For queries about this service, please contact Infrastructure at:
[email protected]
