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/09/04 15:11:01 UTC
[GitHub] [incubator-daffodil] rsteinberger opened a new pull request #414: Throw SDE in the case of inputValueCalc containing checkConstraints(.)
rsteinberger opened a new pull request #414:
URL: https://github.com/apache/incubator-daffodil/pull/414
For inputValueCalc, you cannot use checkConstraints(.)
because the inputValueCalc expression is computing that value.
This commit also addresses the case of test_assertFailShowsValue2
indicated in Daffodil-1043
Daffodil-1035 Daffodil-1043
----------------------------------------------------------------
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] rsteinberger commented on a change in pull request #414: Throw SDE in the case of inputValueCalc containing checkConstraints(.)
Posted by GitBox <gi...@apache.org>.
rsteinberger commented on a change in pull request #414:
URL: https://github.com/apache/incubator-daffodil/pull/414#discussion_r490406638
##########
File path: daffodil-core/src/main/scala/org/apache/daffodil/dsom/ElementBase.scala
##########
@@ -1114,4 +1115,10 @@ trait ElementBase
case (_, _) => this.subset((floating eq YesNo.No), "Property value floating='yes' is not supported.")
}
+ private lazy val checkInputValueCalcExpression: Boolean = {
+ if (this.inputValueCalcOption.toString.contains("checkConstraints(.)")) {
Review comment:
Looks like the isNilled thing is not a problem.
I pushed the solution Steve indicated for review knowing it will need cleanup is it looks like the way to go.
----------------------------------------------------------------
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 #414: Throw SDE in the case of inputValueCalc containing checkConstraints(.)
Posted by GitBox <gi...@apache.org>.
stevedlawrence commented on a change in pull request #414:
URL: https://github.com/apache/incubator-daffodil/pull/414#discussion_r483686640
##########
File path: daffodil-test/src/test/resources/org/apache/daffodil/section23/dfdl_expressions/expressions.tdml
##########
@@ -2440,38 +2440,47 @@
Purpose: This test demonstrates that checkConstraints cannot reference an inputValueCalc-ed element because the value is being computed by inputValueCalc
-->
- <tdml:parserTestCase name="dfdlCheckConstraints"
- root="e1" model="checkConstraints" description="DFDL-23-135R">
+ <tdml:parserTestCase name="dfdlCheckConstraintsFails"
+ root="e1" model="checkConstraints" description="DFDL-23-135R">
<tdml:document>6</tdml:document>
<tdml:errors>
<tdml:error>Schema Definition Error</tdml:error>
- <tdml:error>Placeholder</tdml:error>
+ <tdml:error>you cannot use checkConstraints(.)</tdml:error>
</tdml:errors>
</tdml:parserTestCase>
- <tdml:parserTestCase name="checkConstraintsComplexTypeFails"
- root="e7" model="checkConstraints" description="DFDL-23-135R">
- <tdml:document>s1s2</tdml:document>
- <tdml:errors>
- <tdml:error>Schema Definition Error</tdml:error>
- <tdml:error>checkConstraints</tdml:error>
- <tdml:error>AnyAtomic</tdml:error>
- </tdml:errors>
- </tdml:parserTestCase>
+<!-- <tdml:parserTestCase name="dfdlCheckConstraints"-->
+<!-- root="e1" model="checkConstraints" description="DFDL-23-135R">-->
+<!-- <tdml:document>6</tdml:document>-->
+<!-- <tdml:errors>-->
+<!-- <tdml:error>Schema Definition Error</tdml:error>-->
+<!-- <tdml:error>Placeholder</tdml:error>-->
+<!-- </tdml:errors>-->
+<!-- </tdml:parserTestCase>-->
+
+<!-- <tdml:parserTestCase name="checkConstraintsComplexTypeFails"-->
+<!-- root="e7" model="checkConstraints" description="DFDL-23-135R">-->
+<!-- <tdml:document>s1s2</tdml:document>-->
+<!-- <tdml:errors>-->
+<!-- <tdml:error>Schema Definition Error</tdml:error>-->
+<!-- <tdml:error>checkConstraints</tdml:error>-->
+<!-- <tdml:error>AnyAtomic</tdml:error>-->
+<!-- </tdml:errors>-->
+<!-- </tdml:parserTestCase>-->
<!--
Test Name: dfdlCheckConstraints2
Schema: checkConstraints
Purpose: This test demonstrates that checkConstraints cannot reference an inputValueCalc-ed element because the value is being computed by inputValueCalc
-->
- <tdml:parserTestCase name="dfdlCheckConstraints2" validation="on"
- root="e8" model="checkConstraints" description="DFDL-23-135R">
- <tdml:document></tdml:document>
- <tdml:errors>
- <tdml:error>Schema Definition Error</tdml:error>
- <tdml:error>Placeholder</tdml:error>
- </tdml:errors>
- </tdml:parserTestCase>
+<!-- <tdml:parserTestCase name="dfdlCheckConstraints2" validation="on"-->
+<!-- root="e8" model="checkConstraints" description="DFDL-23-135R">-->
+<!-- <tdml:document></tdml:document>-->
+<!-- <tdml:errors>-->
+<!-- <tdml:error>Schema Definition Error</tdml:error>-->
+<!-- <tdml:error>Placeholder</tdml:error>-->
+<!-- </tdml:errors>-->
+<!-- </tdml:parserTestCase>-->
Review comment:
Why comment out these tests? We usually keep TDML content uncommented and keep the tests in the scala file as commented if things don't work.
----------------------------------------------------------------
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 #414: Throw SDE in the case of inputValueCalc containing checkConstraints(.)
Posted by GitBox <gi...@apache.org>.
stevedlawrence commented on a change in pull request #414:
URL: https://github.com/apache/incubator-daffodil/pull/414#discussion_r483729413
##########
File path: daffodil-core/src/main/scala/org/apache/daffodil/dsom/ElementBase.scala
##########
@@ -1114,4 +1115,10 @@ trait ElementBase
case (_, _) => this.subset((floating eq YesNo.No), "Property value floating='yes' is not supported.")
}
+ private lazy val checkInputValueCalcExpression: Boolean = {
+ if (this.inputValueCalcOption.toString.contains("checkConstraints(.)")) {
Review comment:
To add more complication, is the list of nodes reachable aware of arrays? For example:
```scala
<xs:element name="foo" maxOccurs="unbounded">
<xs:complexType>
<xs:element name="bar" type="xs:int" dfdl:length="{ ../../foo[fn:count(../../foo) - 1]/bar }" />
</xs:complexType>
</xs:element>
```
So ``bar`` can access itself, but only if it is in a previous foo array index. This example feels contrived, but maybe there's an actual use case for something like 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] rsteinberger commented on a change in pull request #414: Throw SDE in the case of inputValueCalc containing checkConstraints(.)
Posted by GitBox <gi...@apache.org>.
rsteinberger commented on a change in pull request #414:
URL: https://github.com/apache/incubator-daffodil/pull/414#discussion_r489768766
##########
File path: daffodil-core/src/main/scala/org/apache/daffodil/dsom/ElementBase.scala
##########
@@ -1114,4 +1115,10 @@ trait ElementBase
case (_, _) => this.subset((floating eq YesNo.No), "Property value floating='yes' is not supported.")
}
+ private lazy val checkInputValueCalcExpression: Boolean = {
+ if (this.inputValueCalcOption.toString.contains("checkConstraints(.)")) {
Review comment:
Wonder if can I get a read from you guys on this as a solution before I clean up the tests. This is in UpDownMoves.scala
case object SelfMove extends RecipeOp {
override def run(dstate: DState): Unit = {
// do this entirely so it will fail at constant compile time
// also serves as a sort of assertion check.
try {
dstate.currentValue
} catch {
case error: Exception =>
throw new SchemaDefinitionError(None, None, "No value found while evaluating a self referencing expression for element %s.", dstate.currentElement)
}
dstate.selfMove()
}
}
----------------------------------------------------------------
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 #414: Throw SDE in the case of inputValueCalc containing checkConstraints(.)
Posted by GitBox <gi...@apache.org>.
stevedlawrence commented on a change in pull request #414:
URL: https://github.com/apache/incubator-daffodil/pull/414#discussion_r483719502
##########
File path: daffodil-core/src/main/scala/org/apache/daffodil/dsom/ElementBase.scala
##########
@@ -1114,4 +1115,10 @@ trait ElementBase
case (_, _) => this.subset((floating eq YesNo.No), "Property value floating='yes' is not supported.")
}
+ private lazy val checkInputValueCalcExpression: Boolean = {
+ if (this.inputValueCalcOption.toString.contains("checkConstraints(.)")) {
Review comment:
Does something similar also occur for things other then dfdl:inputValueCalc/checkConstraints? For example:
```xml
<xs:element name="foo" dfdl:length="{ ../foo }" ... />
```
Are there any DFDL properties where it is valid to access ones self except for checkConstraints? Do we have appropriate errors for those? I guess this issue comes up specificaly with checkConstraints because almost always the argument to checkConstraints?
----------------------------------------------------------------
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 #414: Throw SDE in the case of inputValueCalc containing checkConstraints(.)
Posted by GitBox <gi...@apache.org>.
stevedlawrence commented on a change in pull request #414:
URL: https://github.com/apache/incubator-daffodil/pull/414#discussion_r491990788
##########
File path: daffodil-runtime1/src/main/scala/org/apache/daffodil/dpath/DPath.scala
##########
@@ -160,6 +161,12 @@ final class RuntimeExpressionDPath[T <: AnyRef](qn: NamedQName, tt: NodeInfo.Kin
case _ => doSDE(e, ps)
}
}
+ case ParserNonBlocking => {
+ e match {
+ case nd: InfosetNoDataException => doSDE(nd, ps)
Review comment:
This doesn't change anything to use the new exception. I think you want something like
```scala
case nd: InfosetNoDataException => {
val selfRefException = new InfosetSelfReferencingException(nd.node)
doSDE(selfRefException, ps)
}
```
----------------------------------------------------------------
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] rsteinberger commented on a change in pull request #414: Throw SDE in the case of inputValueCalc containing checkConstraints(.)
Posted by GitBox <gi...@apache.org>.
rsteinberger commented on a change in pull request #414:
URL: https://github.com/apache/incubator-daffodil/pull/414#discussion_r495021484
##########
File path: daffodil-runtime1/src/main/scala/org/apache/daffodil/dpath/DPath.scala
##########
@@ -160,6 +161,12 @@ final class RuntimeExpressionDPath[T <: AnyRef](qn: NamedQName, tt: NodeInfo.Kin
case _ => doSDE(e, ps)
}
}
+ case ParserNonBlocking => {
+ e match {
+ case nd: InfosetNoDataExceptionBase => doSDE(new InfosetSelfReferencingException(state.dState.currentNode.asElement, state.dState.currentNode.erd), ps)
Review comment:
Done
----------------------------------------------------------------
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 pull request #414: Throw SDE in the case of inputValueCalc containing checkConstraints(.)
Posted by GitBox <gi...@apache.org>.
mbeckerle commented on pull request #414:
URL: https://github.com/apache/incubator-daffodil/pull/414#issuecomment-702334295
Rick if you squash this to a single commit and force push it, we can then rebase-merge it.
----------------------------------------------------------------
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 #414: Throw SDE in the case of inputValueCalc containing checkConstraints(.)
Posted by GitBox <gi...@apache.org>.
stevedlawrence commented on a change in pull request #414:
URL: https://github.com/apache/incubator-daffodil/pull/414#discussion_r486505308
##########
File path: daffodil-core/src/main/scala/org/apache/daffodil/dsom/ElementBase.scala
##########
@@ -1114,4 +1115,10 @@ trait ElementBase
case (_, _) => this.subset((floating eq YesNo.No), "Property value floating='yes' is not supported.")
}
+ private lazy val checkInputValueCalcExpression: Boolean = {
+ if (this.inputValueCalcOption.toString.contains("checkConstraints(.)")) {
Review comment:
I also did a similar test like this:
```xml
<xs:element name="foo" type="xs:int" dfdl:lengthKind="explicit" dfdl:length="{ . }" />
```
And get the same error about ``foo does not have a value``. So this issue of self referential equations produces the same diagnostic. Unfortunately it's a runtime diagnostic, but as discussed above sometimes we can't statically figure it out.
If we do want to improve this diagnostic, perhaps a better route would be to just detect if an expression attempts to access the value of the context from which the expression was run? It's still a runtime check, but it avoids the fragility and specificness of the expression.toString check.
----------------------------------------------------------------
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 #414: Throw SDE in the case of inputValueCalc containing checkConstraints(.)
Posted by GitBox <gi...@apache.org>.
mbeckerle commented on a change in pull request #414:
URL: https://github.com/apache/incubator-daffodil/pull/414#discussion_r491128293
##########
File path: daffodil-cli/src/it/scala/org/apache/daffodil/debugger/TestCLIDebugger.scala
##########
@@ -155,21 +155,21 @@ class TestCLIdebugger {
shell.sendLine(cmd)
shell.expect(contains("(debug)"))
- shell.sendLine("display eval (.)")
+ shell.sendLine("display eval (dfdl:occursIndex())")
shell.sendLine("step")
- shell.expect(contains("matrix"))
+ shell.expect(contains("1"))
shell.sendLine("info displays")
- shell.expect(contains("1: eval (.)"))
Review comment:
So if we evaluate "." in the debugger it throws? We should catch this and output something about having no value.
----------------------------------------------------------------
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] rsteinberger commented on a change in pull request #414: Throw SDE in the case of inputValueCalc containing checkConstraints(.)
Posted by GitBox <gi...@apache.org>.
rsteinberger commented on a change in pull request #414:
URL: https://github.com/apache/incubator-daffodil/pull/414#discussion_r491130049
##########
File path: daffodil-test/src/test/resources/org/apache/daffodil/section23/dfdl_expressions/expressions.tdml
##########
@@ -2309,23 +2309,50 @@
<tdml:defineSchema name="checkConstraints">
<xs:include schemaLocation="org/apache/daffodil/xsd/DFDLGeneralFormat.dfdl.xsd"/>
<dfdl:format ref="ex:GeneralFormat" />
+
+ <xs:simpleType name="cnt1">
+ <xs:restriction base="xs:int">
+ <xs:minInclusive value="0"/>
+ <xs:maxInclusive value="2"/>
+ </xs:restriction>
+ </xs:simpleType>
+
<xs:element name="e1">
<xs:complexType>
<xs:sequence>
- <xs:element name="cnt" type="xs:int"
- dfdl:lengthKind="delimited" />
- <xs:element name="a" minOccurs="0" maxOccurs="unbounded"
- dfdl:occursCountKind="expression" dfdl:occursCount="{ xs:int(../ex:cnt) }">
- <xs:complexType>
+ <xs:element name="cnt" type="xs:int" dfdl:lengthKind="delimited" />
<xs:sequence>
- <xs:element name="v" type="xs:int"
- dfdl:inputValueCalc="{ dfdl:checkConstraints(.) }" />
+
+ <!-- These Pass -->
+
+<!-- <xs:element name="v" type="ex:cnt1"-->
+<!-- dfdl:inputValueCalc="{ if (dfdl:checkConstraints(../ex:cnt)) then xs:int(../ex:cnt) else -1 }" />-->
+
+<!-- <xs:element name="v" type="ex:cnt1"-->
+<!-- dfdl:inputValueCalc="{ if (dfdl:checkConstraints(./../ex:cnt)) then xs:int(../ex:cnt) else -1 }" />-->
+
+<!-- <xs:element name="v" type="ex:cnt1"-->
+<!-- dfdl:inputValueCalc="{ if (dfdl:checkConstraints(./../ex:cnt/.)) then xs:int(../ex:cnt) else -1 }" />-->
+
+ <!-- These Fail -->
+
+<!-- <xs:element name="v" type="ex:cnt1"-->
+<!-- dfdl:inputValueCalc="{ if (dfdl:checkConstraints(.)) then xs:int(../ex:cnt) else -1 }" />-->
+
+<!-- <xs:element name="v" type="xs:int" dfdl:lengthKind="explicit" dfdl:length="{.}" />-->
+
+ <!-- Pass -->
+ <xs:element name="v" type="ex:cnt1"
+ dfdl:inputValueCalc="{ dfdl:checkConstraints(if(../ex:cnt eq 1) then ../ex:cnt else .) }" />
+ <!-- Fail -->
+<!-- <xs:element name="v" type="ex:cnt1"-->
+<!-- dfdl:inputValueCalc="{ dfdl:checkConstraints(if(../ex:cnt eq 2) then ../ex:cnt else .) }" />-->
Review comment:
Will do, just wanted to make sure I was on the right track before building them out.
----------------------------------------------------------------
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 #414: Throw SDE in the case of inputValueCalc containing checkConstraints(.)
Posted by GitBox <gi...@apache.org>.
mbeckerle commented on a change in pull request #414:
URL: https://github.com/apache/incubator-daffodil/pull/414#discussion_r483723572
##########
File path: daffodil-core/src/main/scala/org/apache/daffodil/dsom/ElementBase.scala
##########
@@ -1114,4 +1115,10 @@ trait ElementBase
case (_, _) => this.subset((floating eq YesNo.No), "Property value floating='yes' is not supported.")
}
+ private lazy val checkInputValueCalcExpression: Boolean = {
+ if (this.inputValueCalcOption.toString.contains("checkConstraints(.)")) {
Review comment:
And as you point out, if a non "." path leads back to self, that will also cause an error. So the ultimate fix is when compiling a path within an expression, do you end up back at the current element's context. This should be testable. There's logic that basically follows the steps of a path expression. Note that these can reach multiple nodes since expressions with relative paths upward can be inside shared parts of the schema so ".." may refer to multiple different containing parent nodes. The ultimate list of all nodes reachable from a path should not, however, contain the current node.
----------------------------------------------------------------
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 #414: Throw SDE in the case of inputValueCalc containing checkConstraints(.)
Posted by GitBox <gi...@apache.org>.
stevedlawrence commented on a change in pull request #414:
URL: https://github.com/apache/incubator-daffodil/pull/414#discussion_r483685311
##########
File path: daffodil-core/src/main/scala/org/apache/daffodil/dsom/ElementBase.scala
##########
@@ -1114,4 +1115,10 @@ trait ElementBase
case (_, _) => this.subset((floating eq YesNo.No), "Property value floating='yes' is not supported.")
}
+ private lazy val checkInputValueCalcExpression: Boolean = {
+ if (this.inputValueCalcOption.toString.contains("checkConstraints(.)")) {
+ SDE("For inputValueCalc, you cannot use checkConstraints(.) because the inputValueCalc expression is computing that value.")
+ }
+ true
+ }
Review comment:
This feels like too specific of a check, and it's going to fail in some edgecases. For example, what if the expression was ``checkConstraints( .)`` with a bunch of spaces. That wouldn't cause an error.
Really, we don't care about checkConstraints, we just want to error if a inputValueCalc references itself. I thought we already had checks for if a property references it self. This issue isn't specific to inputValueCalc. I can also imagine cases where it references itself not via just dot, for example:
``dfdl:inputValueCalc="{ ../../foo/bar/myself }``
And this isn't a trival check. It's not always possible to statically check if an element is referencing itself. For example, it could reference itself but in a previous array element, which is allowed.
All that to say, I'm not sure how we improve this. Maybe the existing error is sufficient? Or perhaps we do a runtime check that if an expression tries to get the value of the context element then we do a different error message?
----------------------------------------------------------------
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 #414: Throw SDE in the case of inputValueCalc containing checkConstraints(.)
Posted by GitBox <gi...@apache.org>.
mbeckerle commented on a change in pull request #414:
URL: https://github.com/apache/incubator-daffodil/pull/414#discussion_r483685777
##########
File path: daffodil-core/src/main/scala/org/apache/daffodil/dsom/ElementBase.scala
##########
@@ -1114,4 +1115,10 @@ trait ElementBase
case (_, _) => this.subset((floating eq YesNo.No), "Property value floating='yes' is not supported.")
}
+ private lazy val checkInputValueCalcExpression: Boolean = {
+ if (this.inputValueCalcOption.toString.contains("checkConstraints(.)")) {
Review comment:
So you are depending on the fact that toString of inputValueCalcOption displays the expression.
The type of inputValueCalcOption is PropertyLookupResult, and that is either a Found or NotFound object.
(annoying that inputValueCalcOption isn't an Option type, but all the properties work this way.)
I think you should not depend quite so much on the toString of a Found displaying the text of the property. If the property value was say, really big then the toString might elide that with "..." in the textual rep, and that would disable your check.
Suggest
```val optIVC = inputValueCalcOption.toOption ```
then use either
```if (optIVC.isDefined) SDEWhen(optIVC.get.value.contains(...), .... )```
or
```optIVC.map{ ivc => SDEWhen(ivc.value.contains(...) , ...) }```
Instead of contains, you should compare using a regex match so that "checkConstraints( . )" will still match (has spaces around the "."). You never know what sort of whitespace convention someone will use for their DFDL schema, particularly for these XPath-like expressions.
##########
File path: daffodil-test/src/test/resources/org/apache/daffodil/section23/dfdl_expressions/expressions.tdml
##########
@@ -2440,38 +2440,47 @@
Purpose: This test demonstrates that checkConstraints cannot reference an inputValueCalc-ed element because the value is being computed by inputValueCalc
-->
- <tdml:parserTestCase name="dfdlCheckConstraints"
- root="e1" model="checkConstraints" description="DFDL-23-135R">
+ <tdml:parserTestCase name="dfdlCheckConstraintsFails"
+ root="e1" model="checkConstraints" description="DFDL-23-135R">
<tdml:document>6</tdml:document>
<tdml:errors>
<tdml:error>Schema Definition Error</tdml:error>
- <tdml:error>Placeholder</tdml:error>
+ <tdml:error>you cannot use checkConstraints(.)</tdml:error>
</tdml:errors>
</tdml:parserTestCase>
- <tdml:parserTestCase name="checkConstraintsComplexTypeFails"
- root="e7" model="checkConstraints" description="DFDL-23-135R">
- <tdml:document>s1s2</tdml:document>
- <tdml:errors>
- <tdml:error>Schema Definition Error</tdml:error>
- <tdml:error>checkConstraints</tdml:error>
- <tdml:error>AnyAtomic</tdml:error>
- </tdml:errors>
- </tdml:parserTestCase>
+<!-- <tdml:parserTestCase name="dfdlCheckConstraints"-->
Review comment:
Take out these commented-out tests now.
##########
File path: daffodil-test/src/test/resources/org/apache/daffodil/section23/dfdl_expressions/expressions.tdml
##########
@@ -2408,8 +2408,8 @@
<!-- This test is incorrect. An element of simple type cannot use "."
in any inputValueCalc, as "." means "the value of this element", so
we're chasing our tail here. -->
- <xs:element name="e8" type="xs:int"
- dfdl:inputValueCalc="{ dfdl:checkConstraints(.) }" />
+<!-- <xs:element name="e8" type="xs:int"-->
Review comment:
This one doesn't have the problematic IVC. So the comment above "This test is incorrect" doesn't match does it?
Fix comment(moves up to before the prior element?), remove unused element decl.
##########
File path: daffodil-test/src/test/resources/org/apache/daffodil/section23/dfdl_expressions/expressions.tdml
##########
@@ -2440,38 +2440,47 @@
Purpose: This test demonstrates that checkConstraints cannot reference an inputValueCalc-ed element because the value is being computed by inputValueCalc
-->
- <tdml:parserTestCase name="dfdlCheckConstraints"
- root="e1" model="checkConstraints" description="DFDL-23-135R">
+ <tdml:parserTestCase name="dfdlCheckConstraintsFails"
+ root="e1" model="checkConstraints" description="DFDL-23-135R">
<tdml:document>6</tdml:document>
<tdml:errors>
<tdml:error>Schema Definition Error</tdml:error>
- <tdml:error>Placeholder</tdml:error>
+ <tdml:error>you cannot use checkConstraints(.)</tdml:error>
</tdml:errors>
</tdml:parserTestCase>
- <tdml:parserTestCase name="checkConstraintsComplexTypeFails"
- root="e7" model="checkConstraints" description="DFDL-23-135R">
- <tdml:document>s1s2</tdml:document>
- <tdml:errors>
- <tdml:error>Schema Definition Error</tdml:error>
- <tdml:error>checkConstraints</tdml:error>
- <tdml:error>AnyAtomic</tdml:error>
- </tdml:errors>
- </tdml:parserTestCase>
+<!-- <tdml:parserTestCase name="dfdlCheckConstraints"-->
+<!-- root="e1" model="checkConstraints" description="DFDL-23-135R">-->
+<!-- <tdml:document>6</tdml:document>-->
+<!-- <tdml:errors>-->
+<!-- <tdml:error>Schema Definition Error</tdml:error>-->
+<!-- <tdml:error>Placeholder</tdml:error>-->
+<!-- </tdml:errors>-->
+<!-- </tdml:parserTestCase>-->
+
+<!-- <tdml:parserTestCase name="checkConstraintsComplexTypeFails"-->
+<!-- root="e7" model="checkConstraints" description="DFDL-23-135R">-->
+<!-- <tdml:document>s1s2</tdml:document>-->
+<!-- <tdml:errors>-->
+<!-- <tdml:error>Schema Definition Error</tdml:error>-->
+<!-- <tdml:error>checkConstraints</tdml:error>-->
+<!-- <tdml:error>AnyAtomic</tdml:error>-->
+<!-- </tdml:errors>-->
+<!-- </tdml:parserTestCase>-->
<!--
Test Name: dfdlCheckConstraints2
Schema: checkConstraints
Purpose: This test demonstrates that checkConstraints cannot reference an inputValueCalc-ed element because the value is being computed by inputValueCalc
-->
- <tdml:parserTestCase name="dfdlCheckConstraints2" validation="on"
- root="e8" model="checkConstraints" description="DFDL-23-135R">
- <tdml:document></tdml:document>
- <tdml:errors>
- <tdml:error>Schema Definition Error</tdml:error>
- <tdml:error>Placeholder</tdml:error>
- </tdml:errors>
- </tdml:parserTestCase>
+<!-- <tdml:parserTestCase name="dfdlCheckConstraints2" validation="on"-->
Review comment:
and 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 #414: Throw SDE in the case of inputValueCalc containing checkConstraints(.)
Posted by GitBox <gi...@apache.org>.
mbeckerle commented on a change in pull request #414:
URL: https://github.com/apache/incubator-daffodil/pull/414#discussion_r483746323
##########
File path: daffodil-core/src/main/scala/org/apache/daffodil/dsom/ElementBase.scala
##########
@@ -1114,4 +1115,10 @@ trait ElementBase
case (_, _) => this.subset((floating eq YesNo.No), "Property value floating='yes' is not supported.")
}
+ private lazy val checkInputValueCalcExpression: Boolean = {
+ if (this.inputValueCalcOption.toString.contains("checkConstraints(.)")) {
Review comment:
Here's a more realistic example of the mistake we're trying to catch:
```<xs:element name="foo" type="xs:int"
dfdl:inputValueCalc="{ if (dfdl:checkConstraints(.)) then xs:int(../raw) else -1 }" />
```
The mistake here is they meant to do dfdl:checkConstraints(../raw). The intention here is if the raw value satisfies the facet constraints then use it converting to int. If not substitute -1.
If we create a test with this mistake in it, and the diagnostic is adequate to figure out what's wrong, then yeah, I'd say this issue is no longer a bug and we don't need an additional compile time check.
----------------------------------------------------------------
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] rsteinberger commented on pull request #414: Throw SDE in the case of inputValueCalc containing checkConstraints(.)
Posted by GitBox <gi...@apache.org>.
rsteinberger commented on pull request #414:
URL: https://github.com/apache/incubator-daffodil/pull/414#issuecomment-702351805
Done.
----------------------------------------------------------------
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 #414: Throw SDE in the case of inputValueCalc containing checkConstraints(.)
Posted by GitBox <gi...@apache.org>.
stevedlawrence commented on a change in pull request #414:
URL: https://github.com/apache/incubator-daffodil/pull/414#discussion_r490211242
##########
File path: daffodil-core/src/main/scala/org/apache/daffodil/dsom/ElementBase.scala
##########
@@ -1114,4 +1115,10 @@ trait ElementBase
case (_, _) => this.subset((floating eq YesNo.No), "Property value floating='yes' is not supported.")
}
+ private lazy val checkInputValueCalcExpression: Boolean = {
+ if (this.inputValueCalcOption.toString.contains("checkConstraints(.)")) {
Review comment:
Does an expression accessing a nilled element also create the ``InfosetNoDataException``? If so, it is definitely incorrect to convert that to a ``SelfReferencingException``. We likely need someone to differentiate the two. Maybe a different exception for trying to access the value of a nilled element is needed, e.g. ``InfosetNilledDataException``. That would probably also improve the dianostics for that error case as well, instead of it being "element has no data" it could be "element is nilled" or something.
----------------------------------------------------------------
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 #414: Throw SDE in the case of inputValueCalc containing checkConstraints(.)
Posted by GitBox <gi...@apache.org>.
mbeckerle commented on a change in pull request #414:
URL: https://github.com/apache/incubator-daffodil/pull/414#discussion_r483739280
##########
File path: daffodil-core/src/main/scala/org/apache/daffodil/dsom/ElementBase.scala
##########
@@ -1114,4 +1115,10 @@ trait ElementBase
case (_, _) => this.subset((floating eq YesNo.No), "Property value floating='yes' is not supported.")
}
+ private lazy val checkInputValueCalcExpression: Boolean = {
+ if (this.inputValueCalcOption.toString.contains("checkConstraints(.)")) {
Review comment:
I guess also not preceded nor followed by "," since it could appear as an argument of a multi-arg function call.
----------------------------------------------------------------
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 #414: Throw SDE in the case of inputValueCalc containing checkConstraints(.)
Posted by GitBox <gi...@apache.org>.
mbeckerle commented on a change in pull request #414:
URL: https://github.com/apache/incubator-daffodil/pull/414#discussion_r491128612
##########
File path: daffodil-cli/src/it/scala/org/apache/daffodil/debugger/TestCLIDebugger.scala
##########
@@ -155,21 +155,21 @@ class TestCLIdebugger {
shell.sendLine(cmd)
shell.expect(contains("(debug)"))
- shell.sendLine("display eval (.)")
+ shell.sendLine("display eval (dfdl:occursIndex())")
shell.sendLine("step")
- shell.expect(contains("matrix"))
+ shell.expect(contains("1"))
shell.sendLine("info displays")
- shell.expect(contains("1: eval (.)"))
Review comment:
There's a place in the debugger with a big bunch of catches for this sort of thing, e.g., you type an expression at the debugger which divides by zero. That should get caught similarly, and not throw you out of the debugger.
----------------------------------------------------------------
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] rsteinberger commented on pull request #414: Throw SDE in the case of inputValueCalc containing checkConstraints(.)
Posted by GitBox <gi...@apache.org>.
rsteinberger commented on pull request #414:
URL: https://github.com/apache/incubator-daffodil/pull/414#issuecomment-701876711
Looking for a second approval here. If it would help to see the final solution, I can squash the branch? Thanks
----------------------------------------------------------------
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 #414: Throw SDE in the case of inputValueCalc containing checkConstraints(.)
Posted by GitBox <gi...@apache.org>.
stevedlawrence commented on a change in pull request #414:
URL: https://github.com/apache/incubator-daffodil/pull/414#discussion_r494258400
##########
File path: daffodil-runtime1/src/main/scala/org/apache/daffodil/dpath/DPath.scala
##########
@@ -160,6 +161,12 @@ final class RuntimeExpressionDPath[T <: AnyRef](qn: NamedQName, tt: NodeInfo.Kin
case _ => doSDE(e, ps)
}
}
+ case ParserNonBlocking => {
+ e match {
+ case nd: InfosetNoDataExceptionBase => doSDE(new InfosetSelfReferencingException(state.dState.currentNode.asElement, state.dState.currentNode.erd), ps)
Review comment:
This line is a bit too long. Can you split it into multiple lines:
```suggestion
case nd: InfosetNoDataExceptionBase => {
val sre = new InfosetSelfReferencingException(state.dState.currentNode.asElement, state.dState.currentNode.erd)
doSDE(sre, ps)
}
```
----------------------------------------------------------------
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] rsteinberger commented on pull request #414: Throw SDE in the case of inputValueCalc containing checkConstraints(.)
Posted by GitBox <gi...@apache.org>.
rsteinberger commented on pull request #414:
URL: https://github.com/apache/incubator-daffodil/pull/414#issuecomment-698285805
I believe we have a solution now. I if I have missed any comments please let me know. Thanks.
----------------------------------------------------------------
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 #414: Throw SDE in the case of inputValueCalc containing checkConstraints(.)
Posted by GitBox <gi...@apache.org>.
mbeckerle commented on a change in pull request #414:
URL: https://github.com/apache/incubator-daffodil/pull/414#discussion_r483720727
##########
File path: daffodil-core/src/main/scala/org/apache/daffodil/dsom/ElementBase.scala
##########
@@ -1114,4 +1115,10 @@ trait ElementBase
case (_, _) => this.subset((floating eq YesNo.No), "Property value floating='yes' is not supported.")
}
+ private lazy val checkInputValueCalcExpression: Boolean = {
+ if (this.inputValueCalcOption.toString.contains("checkConstraints(.)")) {
Review comment:
I think the checkConstraints is a red herring. That was just copied out of assert tests into inputValueCalc tests, and so the IVC expression ended up being dfdl:checkConstraints(.).
The real issue is any use of a "." path in an IVC expression.
----------------------------------------------------------------
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 #414: Throw SDE in the case of inputValueCalc containing checkConstraints(.)
Posted by GitBox <gi...@apache.org>.
stevedlawrence commented on a change in pull request #414:
URL: https://github.com/apache/incubator-daffodil/pull/414#discussion_r483743360
##########
File path: daffodil-core/src/main/scala/org/apache/daffodil/dsom/ElementBase.scala
##########
@@ -1114,4 +1115,10 @@ trait ElementBase
case (_, _) => this.subset((floating eq YesNo.No), "Property value floating='yes' is not supported.")
}
+ private lazy val checkInputValueCalcExpression: Boolean = {
+ if (this.inputValueCalcOption.toString.contains("checkConstraints(.)")) {
Review comment:
Feels pretty fragile to me. And is pretty specific to one potential error case. Maybe the current error is actually sufficient? This bug was opened five years ago, maybe our diagnostics surrounding this have improved?
I guess it's better than nothing, but I can imagine building up a bunch of complicated regexes for different conditions that become difficult to maintain and verify correctness.
----------------------------------------------------------------
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] rsteinberger commented on a change in pull request #414: Throw SDE in the case of inputValueCalc containing checkConstraints(.)
Posted by GitBox <gi...@apache.org>.
rsteinberger commented on a change in pull request #414:
URL: https://github.com/apache/incubator-daffodil/pull/414#discussion_r489768766
##########
File path: daffodil-core/src/main/scala/org/apache/daffodil/dsom/ElementBase.scala
##########
@@ -1114,4 +1115,10 @@ trait ElementBase
case (_, _) => this.subset((floating eq YesNo.No), "Property value floating='yes' is not supported.")
}
+ private lazy val checkInputValueCalcExpression: Boolean = {
+ if (this.inputValueCalcOption.toString.contains("checkConstraints(.)")) {
Review comment:
Wonder if I get a read from you guys on this as a solution before I clean up the tests. This is in UpDownMoves.scala
case object SelfMove extends RecipeOp {
override def run(dstate: DState): Unit = {
// do this entirely so it will fail at constant compile time
// also serves as a sort of assertion check.
try {
dstate.currentValue
} catch {
case error: Exception =>
throw new SchemaDefinitionError(None, None, "No value found while evaluating a self referencing expression for element %s.", dstate.currentElement)
}
dstate.selfMove()
}
}
----------------------------------------------------------------
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] rsteinberger commented on a change in pull request #414: Throw SDE in the case of inputValueCalc containing checkConstraints(.)
Posted by GitBox <gi...@apache.org>.
rsteinberger commented on a change in pull request #414:
URL: https://github.com/apache/incubator-daffodil/pull/414#discussion_r491130148
##########
File path: daffodil-runtime1/src/main/scala/org/apache/daffodil/dpath/DPath.scala
##########
@@ -160,6 +161,12 @@ final class RuntimeExpressionDPath[T <: AnyRef](qn: NamedQName, tt: NodeInfo.Kin
case _ => doSDE(e, ps)
}
}
+ case ParserNonBlocking => {
+ e match {
+ case nd: InfosetNoDataException => throw new InfosetSelfReferencingException(state.dState.currentNode)
Review comment:
Will do.
----------------------------------------------------------------
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] rsteinberger commented on a change in pull request #414: Throw SDE in the case of inputValueCalc containing checkConstraints(.)
Posted by GitBox <gi...@apache.org>.
rsteinberger commented on a change in pull request #414:
URL: https://github.com/apache/incubator-daffodil/pull/414#discussion_r486462134
##########
File path: daffodil-core/src/main/scala/org/apache/daffodil/dsom/ElementBase.scala
##########
@@ -1114,4 +1115,10 @@ trait ElementBase
case (_, _) => this.subset((floating eq YesNo.No), "Property value floating='yes' is not supported.")
}
+ private lazy val checkInputValueCalcExpression: Boolean = {
+ if (this.inputValueCalcOption.toString.contains("checkConstraints(.)")) {
Review comment:
With the above inputValueCalc and restrictions in place the following error is thrown:
Runtime Schema Definition Error: Expression Evaluation Error: Element ex:v does not have a value.
----------------------------------------------------------------
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] rsteinberger commented on a change in pull request #414: Throw SDE in the case of inputValueCalc containing checkConstraints(.)
Posted by GitBox <gi...@apache.org>.
rsteinberger commented on a change in pull request #414:
URL: https://github.com/apache/incubator-daffodil/pull/414#discussion_r488837611
##########
File path: daffodil-core/src/main/scala/org/apache/daffodil/dsom/ElementBase.scala
##########
@@ -1114,4 +1115,10 @@ trait ElementBase
case (_, _) => this.subset((floating eq YesNo.No), "Property value floating='yes' is not supported.")
}
+ private lazy val checkInputValueCalcExpression: Boolean = {
+ if (this.inputValueCalcOption.toString.contains("checkConstraints(.)")) {
Review comment:
Ok, I have a solution which allows us to detect a null value in the case of a self reference. Does this get us closer?
----------------------------------------------------------------
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 #414: Throw SDE in the case of inputValueCalc containing checkConstraints(.)
Posted by GitBox <gi...@apache.org>.
mbeckerle commented on a change in pull request #414:
URL: https://github.com/apache/incubator-daffodil/pull/414#discussion_r488839582
##########
File path: daffodil-core/src/main/scala/org/apache/daffodil/dsom/ElementBase.scala
##########
@@ -1114,4 +1115,10 @@ trait ElementBase
case (_, _) => this.subset((floating eq YesNo.No), "Property value floating='yes' is not supported.")
}
+ private lazy val checkInputValueCalcExpression: Boolean = {
+ if (this.inputValueCalcOption.toString.contains("checkConstraints(.)")) {
Review comment:
If we can get a diagnostic out that provides reasonable information about what is wrong with the schema, then yes.
----------------------------------------------------------------
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 #414: Throw SDE in the case of inputValueCalc containing checkConstraints(.)
Posted by GitBox <gi...@apache.org>.
stevedlawrence commented on a change in pull request #414:
URL: https://github.com/apache/incubator-daffodil/pull/414#discussion_r491167777
##########
File path: daffodil-cli/src/it/scala/org/apache/daffodil/debugger/TestCLIDebugger.scala
##########
@@ -155,21 +155,21 @@ class TestCLIdebugger {
shell.sendLine(cmd)
shell.expect(contains("(debug)"))
- shell.sendLine("display eval (.)")
+ shell.sendLine("display eval (dfdl:occursIndex())")
shell.sendLine("step")
- shell.expect(contains("matrix"))
+ shell.expect(contains("1"))
shell.sendLine("info displays")
- shell.expect(contains("1: eval (.)"))
Review comment:
I suspect thismight get fixed by change it to call ``doSDE()`` rather than throwing the new Exception. I think the debugger explicitly handles SDE's (and also InfosetNoDataException) as special. So making this an SDE might fix it.
It might also make sense to have this new exception extend InfosetNoDataException, but with a new error message. That way things that expected an InfosetNoDataException will still work as before, but will have a more appropriate error message.
----------------------------------------------------------------
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 #414: Throw SDE in the case of inputValueCalc containing checkConstraints(.)
Posted by GitBox <gi...@apache.org>.
stevedlawrence commented on a change in pull request #414:
URL: https://github.com/apache/incubator-daffodil/pull/414#discussion_r491086788
##########
File path: daffodil-test/src/test/resources/org/apache/daffodil/section23/dfdl_expressions/expressions.tdml
##########
@@ -2309,23 +2309,50 @@
<tdml:defineSchema name="checkConstraints">
<xs:include schemaLocation="org/apache/daffodil/xsd/DFDLGeneralFormat.dfdl.xsd"/>
<dfdl:format ref="ex:GeneralFormat" />
+
+ <xs:simpleType name="cnt1">
+ <xs:restriction base="xs:int">
+ <xs:minInclusive value="0"/>
+ <xs:maxInclusive value="2"/>
+ </xs:restriction>
+ </xs:simpleType>
+
<xs:element name="e1">
<xs:complexType>
<xs:sequence>
- <xs:element name="cnt" type="xs:int"
- dfdl:lengthKind="delimited" />
- <xs:element name="a" minOccurs="0" maxOccurs="unbounded"
- dfdl:occursCountKind="expression" dfdl:occursCount="{ xs:int(../ex:cnt) }">
- <xs:complexType>
+ <xs:element name="cnt" type="xs:int" dfdl:lengthKind="delimited" />
<xs:sequence>
- <xs:element name="v" type="xs:int"
- dfdl:inputValueCalc="{ dfdl:checkConstraints(.) }" />
+
+ <!-- These Pass -->
+
+<!-- <xs:element name="v" type="ex:cnt1"-->
+<!-- dfdl:inputValueCalc="{ if (dfdl:checkConstraints(../ex:cnt)) then xs:int(../ex:cnt) else -1 }" />-->
+
+<!-- <xs:element name="v" type="ex:cnt1"-->
+<!-- dfdl:inputValueCalc="{ if (dfdl:checkConstraints(./../ex:cnt)) then xs:int(../ex:cnt) else -1 }" />-->
+
+<!-- <xs:element name="v" type="ex:cnt1"-->
+<!-- dfdl:inputValueCalc="{ if (dfdl:checkConstraints(./../ex:cnt/.)) then xs:int(../ex:cnt) else -1 }" />-->
+
+ <!-- These Fail -->
+
+<!-- <xs:element name="v" type="ex:cnt1"-->
+<!-- dfdl:inputValueCalc="{ if (dfdl:checkConstraints(.)) then xs:int(../ex:cnt) else -1 }" />-->
+
+<!-- <xs:element name="v" type="xs:int" dfdl:lengthKind="explicit" dfdl:length="{.}" />-->
+
+ <!-- Pass -->
+ <xs:element name="v" type="ex:cnt1"
+ dfdl:inputValueCalc="{ dfdl:checkConstraints(if(../ex:cnt eq 1) then ../ex:cnt else .) }" />
+ <!-- Fail -->
+<!-- <xs:element name="v" type="ex:cnt1"-->
+<!-- dfdl:inputValueCalc="{ dfdl:checkConstraints(if(../ex:cnt eq 2) then ../ex:cnt else .) }" />-->
Review comment:
Rather than having comments for certain things that pass fail, I woulc recommend creating a unique test for each of these. That ensures that we do not have any regressions for the expected behavior.
----------------------------------------------------------------
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 #414: Throw SDE in the case of inputValueCalc containing checkConstraints(.)
Posted by GitBox <gi...@apache.org>.
mbeckerle commented on a change in pull request #414:
URL: https://github.com/apache/incubator-daffodil/pull/414#discussion_r483715165
##########
File path: daffodil-core/src/main/scala/org/apache/daffodil/dsom/ElementBase.scala
##########
@@ -1114,4 +1115,10 @@ trait ElementBase
case (_, _) => this.subset((floating eq YesNo.No), "Property value floating='yes' is not supported.")
}
+ private lazy val checkInputValueCalcExpression: Boolean = {
+ if (this.inputValueCalcOption.toString.contains("checkConstraints(.)")) {
Review comment:
Ah. I went back and looked at the bug ticket.
The issue is not about checkConstraints(.), but really any expression involving "." as part of inputValueCalc. The example was calling checkConstraints(.) in an inputValueCalc, but that's just an example of one (rather obscure) use of "." in an inputValueCalc expression.
To fix this bug really you need to get the compiled expression that is created from the inputValueCalc property, and walk that compiled expression object to see if it has a Self path step reference in it.
----------------------------------------------------------------
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 #414: Throw SDE in the case of inputValueCalc containing checkConstraints(.)
Posted by GitBox <gi...@apache.org>.
stevedlawrence commented on a change in pull request #414:
URL: https://github.com/apache/incubator-daffodil/pull/414#discussion_r488154509
##########
File path: daffodil-core/src/main/scala/org/apache/daffodil/dsom/ElementBase.scala
##########
@@ -1114,4 +1115,10 @@ trait ElementBase
case (_, _) => this.subset((floating eq YesNo.No), "Property value floating='yes' is not supported.")
}
+ private lazy val checkInputValueCalcExpression: Boolean = {
+ if (this.inputValueCalcOption.toString.contains("checkConstraints(.)")) {
Review comment:
Note that this is a very simple example. Most self-referential expressions are going to be more complicated to statically check. And as mentioned before, Self steps are perfectly valid in some cases so doesn't necessarily mean there's a self-reference.
----------------------------------------------------------------
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 #414: Throw SDE in the case of inputValueCalc containing checkConstraints(.)
Posted by GitBox <gi...@apache.org>.
stevedlawrence commented on a change in pull request #414:
URL: https://github.com/apache/incubator-daffodil/pull/414#discussion_r494258400
##########
File path: daffodil-runtime1/src/main/scala/org/apache/daffodil/dpath/DPath.scala
##########
@@ -160,6 +161,12 @@ final class RuntimeExpressionDPath[T <: AnyRef](qn: NamedQName, tt: NodeInfo.Kin
case _ => doSDE(e, ps)
}
}
+ case ParserNonBlocking => {
+ e match {
+ case nd: InfosetNoDataExceptionBase => doSDE(new InfosetSelfReferencingException(state.dState.currentNode.asElement, state.dState.currentNode.erd), ps)
Review comment:
This line is a bit too long. Can you split it into multiple lines:
```suggestion
case nd: InfosetNoDataExceptionBase => {
val sre = new InfosetSelfReferencingException(state.dState.currentNode.asElement, state.dState.currentNode.erd)
doSDE(sre, ps)
}
```
----------------------------------------------------------------
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 #414: Throw SDE in the case of inputValueCalc containing checkConstraints(.)
Posted by GitBox <gi...@apache.org>.
stevedlawrence commented on a change in pull request #414:
URL: https://github.com/apache/incubator-daffodil/pull/414#discussion_r483722344
##########
File path: daffodil-core/src/main/scala/org/apache/daffodil/dsom/ElementBase.scala
##########
@@ -1114,4 +1115,10 @@ trait ElementBase
case (_, _) => this.subset((floating eq YesNo.No), "Property value floating='yes' is not supported.")
}
+ private lazy val checkInputValueCalcExpression: Boolean = {
+ if (this.inputValueCalcOption.toString.contains("checkConstraints(.)")) {
Review comment:
It feels like maybe sit's more than that? Like an expression can never reference itself except within checkConstraints on a non-inputValueCalc element?
----------------------------------------------------------------
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 #414: Throw SDE in the case of inputValueCalc containing checkConstraints(.)
Posted by GitBox <gi...@apache.org>.
stevedlawrence commented on a change in pull request #414:
URL: https://github.com/apache/incubator-daffodil/pull/414#discussion_r490211242
##########
File path: daffodil-core/src/main/scala/org/apache/daffodil/dsom/ElementBase.scala
##########
@@ -1114,4 +1115,10 @@ trait ElementBase
case (_, _) => this.subset((floating eq YesNo.No), "Property value floating='yes' is not supported.")
}
+ private lazy val checkInputValueCalcExpression: Boolean = {
+ if (this.inputValueCalcOption.toString.contains("checkConstraints(.)")) {
Review comment:
Does an expression accessing a nilled element also create the ``InfosetNoDataException``? If so, it is definitely incorrect to convert that to a ``SelfReferencingException``. We likely need some way to differentiate the two. Maybe a different exception for trying to access the value of a nilled element is needed, e.g. ``InfosetNilledDataException``. That would probably also improve the dianostics for that error case as well, instead of it being "element has no data" it could be "element is nilled" or something.
----------------------------------------------------------------
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 #414: Throw SDE in the case of inputValueCalc containing checkConstraints(.)
Posted by GitBox <gi...@apache.org>.
stevedlawrence commented on a change in pull request #414:
URL: https://github.com/apache/incubator-daffodil/pull/414#discussion_r488152120
##########
File path: daffodil-core/src/main/scala/org/apache/daffodil/dsom/ElementBase.scala
##########
@@ -1114,4 +1115,10 @@ trait ElementBase
case (_, _) => this.subset((floating eq YesNo.No), "Property value floating='yes' is not supported.")
}
+ private lazy val checkInputValueCalcExpression: Boolean = {
+ if (this.inputValueCalcOption.toString.contains("checkConstraints(.)")) {
Review comment:
It depends. For example, you could have something like ``dfdl:checkConstraints(./../foo)``, which is weird, but still valid, and has a Self step as the first step. It's possible we collapse steps like this that don't actually do anything, so there might not actually be a Self step in this case. But if there is, then just looking for Self could error on valid expressions.
But it gets even more complicated. Tthe argument to checkConstraints doesn't have to be just a path--it can be an expression to results in a path, so it could be something like this:
```xml
<xs:element name="baz"
dfdl:inputValueCalc="{ dfdl:checkConstraints(if(../bar eq 1) then ../foo else .) }" />
```
This is invalid, but only if bar is not 1. It would be nice to catch this but this is not trivial to find. You have to realize one of the results of an expression contains self. And as mentioned in other comments, you don't have you use ``.`` to self-refernce. You can step up to parents and then back down to self, and this is difficult to discover.
As I suggested above, I would suggest we just make this a runtime check when trying to access the value of a node. Then we can catch this same self-referencing issue for all expression and not just one tiny little function that's rarely even used. If we're going to do work to fix this issue of a bad diagnostic for self-referencing expressions, let's fix it for 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] stevedlawrence commented on a change in pull request #414: Throw SDE in the case of inputValueCalc containing checkConstraints(.)
Posted by GitBox <gi...@apache.org>.
stevedlawrence commented on a change in pull request #414:
URL: https://github.com/apache/incubator-daffodil/pull/414#discussion_r490189645
##########
File path: daffodil-core/src/main/scala/org/apache/daffodil/dsom/ElementBase.scala
##########
@@ -1114,4 +1115,10 @@ trait ElementBase
case (_, _) => this.subset((floating eq YesNo.No), "Property value floating='yes' is not supported.")
}
+ private lazy val checkInputValueCalcExpression: Boolean = {
+ if (this.inputValueCalcOption.toString.contains("checkConstraints(.)")) {
Review comment:
I think a check on ``SelfMove``, while getting closer to a complete solution, still misses a lot of common examples of self referential expressions. For example, it is not uncommon to have an array of integers and maintain a running sum, like this:
```xml
<xs:element name="root">
<xs:complexType>
<xs:sequence>
<xs:element name="array" maxOccurs="unbounded">
<xs:complexType>
<xs:sequence>
<xs:element name="int" type="xs:int" ... />
<xs:element name="sum" type="xs:int" dfdl:inputValueCalc="{
if (fn:count(../../array) eq 0)
then ../int
else ../int + ../../array[fn:count(../../array) - 1]/sum
}" />
</xs:sequence>
</xs:complexType>
</xs:element>
</xs:sequence>
</xs:complexType>
</xs:element>
```
So each ``sum`` element is an inputValueCalc that adds the current ``int`` element to the previous ``sum`` element.
But if a user accidentally leaves off the `` - 1`` when indexing the array, rather than referencing the previous ``sum`` element the expression would reference the current ``sum`` element, which will be a self referential expression and create the poor diagnostic. So we are doing a self reference without using ``.`` in an expression. I suspect this type of error is very common. (It would probably be worth adding this with the `` - 1`` removed as a test).
----
Thinking more about this, the issue isn't just self-referencing expressions--expressions are actually allowed to self reference it's own **node**. For example, I think it's fine to do something like ``fn:count(.)``, which accesses the node, but not the value (note: I'm not sure if there's a good reason for this type of expression, but I imagine one could come up with reasonble examples). The thing we aren't allowed to self reference is the *value* of a node. Only when you access the value does a self-referencing expression become a problem and need a helpful diagnostic.
And we actually already detect this right now. When a self referencing expression access the value of self, it simply won't have a value yet, and an ``InfosetNoDataException`` is thrown. When this happens when evaluating an expression, the ``handleCompileState`` function in ``DPath.scala`` catches this exception and creates an SDE. This is the diagnostic we ultimately see.
I /believe/ that here is where we need an improved diagnositc. There error we are getting is "element has no data" because this is an ``InfosetNoDataException``, but the underlying reason we have no data here and get an ``InfosetNoDataExcpetion`` in DPath parse must be because it was a self referential expression. I don't think there is any other way this exception can be thrown during parse expression evaluation that is due to something other than a self referencing expression.
Unfortunately, I don't think we can change the message of the ``InfosetNoDataException``, since it is used in other contexts. But I think we can just detect ``InfosetNoDataException`` in the parse part of ``handleCompileState`` and create a new exception with an better diagnostic message (e.g. ``InfosetSelfReferencingException``) and pass this new exception to ``doSDE``.
Note that this does mean we only ever detect this error at runtime, but I with the complexity of expression, detecting it at compile time can be difficult.
----------------------------------------------------------------
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] rsteinberger commented on a change in pull request #414: Throw SDE in the case of inputValueCalc containing checkConstraints(.)
Posted by GitBox <gi...@apache.org>.
rsteinberger commented on a change in pull request #414:
URL: https://github.com/apache/incubator-daffodil/pull/414#discussion_r488153148
##########
File path: daffodil-core/src/main/scala/org/apache/daffodil/dsom/ElementBase.scala
##########
@@ -1114,4 +1115,10 @@ trait ElementBase
case (_, _) => this.subset((floating eq YesNo.No), "Property value floating='yes' is not supported.")
}
+ private lazy val checkInputValueCalcExpression: Boolean = {
+ if (this.inputValueCalcOption.toString.contains("checkConstraints(.)")) {
Review comment:
In DPath.evaluateExpression:
<xs:element name="v" type="xs:int" dfdl:lengthKind="explicit" dfdl:length="{.}" />
Is evaluated as:
<CompiledDPath><SelfMove/><IntToLong/></CompiledDPath>
So could be detected in a similar way.
----------------------------------------------------------------
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] rsteinberger commented on a change in pull request #414: Throw SDE in the case of inputValueCalc containing checkConstraints(.)
Posted by GitBox <gi...@apache.org>.
rsteinberger commented on a change in pull request #414:
URL: https://github.com/apache/incubator-daffodil/pull/414#discussion_r488146083
##########
File path: daffodil-core/src/main/scala/org/apache/daffodil/dsom/ElementBase.scala
##########
@@ -1114,4 +1115,10 @@ trait ElementBase
case (_, _) => this.subset((floating eq YesNo.No), "Property value floating='yes' is not supported.")
}
+ private lazy val checkInputValueCalcExpression: Boolean = {
+ if (this.inputValueCalcOption.toString.contains("checkConstraints(.)")) {
Review comment:
In Expression.scala DFDLCheckConstraintsExpr we have the opportunity to examine the args passed to the expression.
In the case of:
dfdl:inputValueCalc="{ if (dfdl:checkConstraints(.)) then xs:int(../ex:cnt) else -1 }" />
The args list is: List(RelativePathExpression(List(Self(None)),false))
In the case of:
dfdl:inputValueCalc="{ if (dfdl:checkConstraints(../.)) then xs:int(../ex:cnt) else -1 }" />
The args list is: List(RelativePathExpression(List(Up(None), Self(None)),false))
Would it be reasonable to detect Self(None) and throw an SDE at compile time?
----------------------------------------------------------------
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] rsteinberger commented on a change in pull request #414: Throw SDE in the case of inputValueCalc containing checkConstraints(.)
Posted by GitBox <gi...@apache.org>.
rsteinberger commented on a change in pull request #414:
URL: https://github.com/apache/incubator-daffodil/pull/414#discussion_r490207087
##########
File path: daffodil-core/src/main/scala/org/apache/daffodil/dsom/ElementBase.scala
##########
@@ -1114,4 +1115,10 @@ trait ElementBase
case (_, _) => this.subset((floating eq YesNo.No), "Property value floating='yes' is not supported.")
}
+ private lazy val checkInputValueCalcExpression: Boolean = {
+ if (this.inputValueCalcOption.toString.contains("checkConstraints(.)")) {
Review comment:
In the case of xsi:nil="true" is it valid to throw this error?
----------------------------------------------------------------
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] rsteinberger commented on a change in pull request #414: Throw SDE in the case of inputValueCalc containing checkConstraints(.)
Posted by GitBox <gi...@apache.org>.
rsteinberger commented on a change in pull request #414:
URL: https://github.com/apache/incubator-daffodil/pull/414#discussion_r488155318
##########
File path: daffodil-core/src/main/scala/org/apache/daffodil/dsom/ElementBase.scala
##########
@@ -1114,4 +1115,10 @@ trait ElementBase
case (_, _) => this.subset((floating eq YesNo.No), "Property value floating='yes' is not supported.")
}
+ private lazy val checkInputValueCalcExpression: Boolean = {
+ if (this.inputValueCalcOption.toString.contains("checkConstraints(.)")) {
Review comment:
Got ya. Thanks for the input.
----------------------------------------------------------------
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 #414: Throw SDE in the case of inputValueCalc containing checkConstraints(.)
Posted by GitBox <gi...@apache.org>.
mbeckerle commented on a change in pull request #414:
URL: https://github.com/apache/incubator-daffodil/pull/414#discussion_r489780225
##########
File path: daffodil-core/src/main/scala/org/apache/daffodil/dsom/ElementBase.scala
##########
@@ -1114,4 +1115,10 @@ trait ElementBase
case (_, _) => this.subset((floating eq YesNo.No), "Property value floating='yes' is not supported.")
}
+ private lazy val checkInputValueCalcExpression: Boolean = {
+ if (this.inputValueCalcOption.toString.contains("checkConstraints(.)")) {
Review comment:
Thought. A path like ../../foo/bar/. (final dot) isn't a self-reference. So long as that gets converted into just ../../foo/bar (i.e., the self-move operation is removed) then I think we're fine for both parse and unparse. The only place where a self-move would "survive" is in expressions that are just ".", so those are always self-move path expressions.
----------------------------------------------------------------
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] rsteinberger commented on pull request #414: Throw SDE in the case of inputValueCalc containing checkConstraints(.)
Posted by GitBox <gi...@apache.org>.
rsteinberger commented on pull request #414:
URL: https://github.com/apache/incubator-daffodil/pull/414#issuecomment-698285805
I believe we have a solution now. I if I have missed any comments please let me know. Thanks.
----------------------------------------------------------------
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] rsteinberger commented on a change in pull request #414: Throw SDE in the case of inputValueCalc containing checkConstraints(.)
Posted by GitBox <gi...@apache.org>.
rsteinberger commented on a change in pull request #414:
URL: https://github.com/apache/incubator-daffodil/pull/414#discussion_r492256042
##########
File path: daffodil-runtime1/src/main/scala/org/apache/daffodil/dpath/DPath.scala
##########
@@ -160,6 +161,12 @@ final class RuntimeExpressionDPath[T <: AnyRef](qn: NamedQName, tt: NodeInfo.Kin
case _ => doSDE(e, ps)
}
}
+ case ParserNonBlocking => {
+ e match {
+ case nd: InfosetNoDataException => doSDE(nd, ps)
Review comment:
Right, I implemented your InfosetNoDataExceptionBase suggestion. That arrears to have corrected the IT error and improved test flexibility.
----------------------------------------------------------------
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 #414: Throw SDE in the case of inputValueCalc containing checkConstraints(.)
Posted by GitBox <gi...@apache.org>.
stevedlawrence commented on a change in pull request #414:
URL: https://github.com/apache/incubator-daffodil/pull/414#discussion_r491086434
##########
File path: daffodil-runtime1/src/main/scala/org/apache/daffodil/dpath/DPath.scala
##########
@@ -160,6 +161,12 @@ final class RuntimeExpressionDPath[T <: AnyRef](qn: NamedQName, tt: NodeInfo.Kin
case _ => doSDE(e, ps)
}
}
+ case ParserNonBlocking => {
+ e match {
+ case nd: InfosetNoDataException => throw new InfosetSelfReferencingException(state.dState.currentNode)
Review comment:
Rather than throwing, should this just creat the exception and the call doSDE? This way doSDE can convert it to an SDE so the error can be handled appropriately.
----------------------------------------------------------------
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] rsteinberger commented on a change in pull request #414: Throw SDE in the case of inputValueCalc containing checkConstraints(.)
Posted by GitBox <gi...@apache.org>.
rsteinberger commented on a change in pull request #414:
URL: https://github.com/apache/incubator-daffodil/pull/414#discussion_r491131307
##########
File path: daffodil-cli/src/it/scala/org/apache/daffodil/debugger/TestCLIDebugger.scala
##########
@@ -155,21 +155,21 @@ class TestCLIdebugger {
shell.sendLine(cmd)
shell.expect(contains("(debug)"))
- shell.sendLine("display eval (.)")
+ shell.sendLine("display eval (dfdl:occursIndex())")
shell.sendLine("step")
- shell.expect(contains("matrix"))
+ shell.expect(contains("1"))
shell.sendLine("info displays")
- shell.expect(contains("1: eval (.)"))
Review comment:
Yes it does. That's what was breaking the IT.
Cool, I'll take a look.
----------------------------------------------------------------
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 merged pull request #414: Throw SDE in the case of inputValueCalc containing checkConstraints(.)
Posted by GitBox <gi...@apache.org>.
mbeckerle merged pull request #414:
URL: https://github.com/apache/incubator-daffodil/pull/414
----------------------------------------------------------------
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 #414: Throw SDE in the case of inputValueCalc containing checkConstraints(.)
Posted by GitBox <gi...@apache.org>.
stevedlawrence commented on a change in pull request #414:
URL: https://github.com/apache/incubator-daffodil/pull/414#discussion_r491990788
##########
File path: daffodil-runtime1/src/main/scala/org/apache/daffodil/dpath/DPath.scala
##########
@@ -160,6 +161,12 @@ final class RuntimeExpressionDPath[T <: AnyRef](qn: NamedQName, tt: NodeInfo.Kin
case _ => doSDE(e, ps)
}
}
+ case ParserNonBlocking => {
+ e match {
+ case nd: InfosetNoDataException => doSDE(nd, ps)
Review comment:
This doesn't change anything to use the new exception. I think you want something like
```scala
case nd: InfosetNoDataException => {
val selfRefException = new InfosetSelfReferencingException(nd.node)
doSDE(selfRefException, ps)
}
```
----------------------------------------------------------------
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 #414: Throw SDE in the case of inputValueCalc containing checkConstraints(.)
Posted by GitBox <gi...@apache.org>.
mbeckerle commented on a change in pull request #414:
URL: https://github.com/apache/incubator-daffodil/pull/414#discussion_r490323511
##########
File path: daffodil-core/src/main/scala/org/apache/daffodil/dsom/ElementBase.scala
##########
@@ -1114,4 +1115,10 @@ trait ElementBase
case (_, _) => this.subset((floating eq YesNo.No), "Property value floating='yes' is not supported.")
}
+ private lazy val checkInputValueCalcExpression: Boolean = {
+ if (this.inputValueCalcOption.toString.contains("checkConstraints(.)")) {
Review comment:
Hmmm. expressions can't deal with nilled elements I think. There is no dfdl:isNilled and no way to return "nil" as a value, and no way to pass nil as a parameter.
----------------------------------------------------------------
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] rsteinberger commented on a change in pull request #414: Throw SDE in the case of inputValueCalc containing checkConstraints(.)
Posted by GitBox <gi...@apache.org>.
rsteinberger commented on a change in pull request #414:
URL: https://github.com/apache/incubator-daffodil/pull/414#discussion_r492256042
##########
File path: daffodil-runtime1/src/main/scala/org/apache/daffodil/dpath/DPath.scala
##########
@@ -160,6 +161,12 @@ final class RuntimeExpressionDPath[T <: AnyRef](qn: NamedQName, tt: NodeInfo.Kin
case _ => doSDE(e, ps)
}
}
+ case ParserNonBlocking => {
+ e match {
+ case nd: InfosetNoDataException => doSDE(nd, ps)
Review comment:
Right, I implemented your InfosetNoDataExceptionBase suggestion. That arrears to have corrected the IT error and improved test flexibility.
----------------------------------------------------------------
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 #414: Throw SDE in the case of inputValueCalc containing checkConstraints(.)
Posted by GitBox <gi...@apache.org>.
mbeckerle commented on a change in pull request #414:
URL: https://github.com/apache/incubator-daffodil/pull/414#discussion_r483738017
##########
File path: daffodil-core/src/main/scala/org/apache/daffodil/dsom/ElementBase.scala
##########
@@ -1114,4 +1115,10 @@ trait ElementBase
case (_, _) => this.subset((floating eq YesNo.No), "Property value floating='yes' is not supported.")
}
+ private lazy val checkInputValueCalcExpression: Boolean = {
+ if (this.inputValueCalcOption.toString.contains("checkConstraints(.)")) {
Review comment:
Yeah, the reaching of the current declaration but by way of prior array indexes throws the analyzability of all this into doubt.
Maybe we just want to scan IVC expressions for "." as an isolated expression. I think that can be done, as rick originally wrote, as a text analysis of the expression. It's "." optionally with whitespace surrounding, and not directly preceded nor directly followed by "/". So "././." would defeat the check, but who cares.
----------------------------------------------------------------
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] rsteinberger commented on a change in pull request #414: Throw SDE in the case of inputValueCalc containing checkConstraints(.)
Posted by GitBox <gi...@apache.org>.
rsteinberger commented on a change in pull request #414:
URL: https://github.com/apache/incubator-daffodil/pull/414#discussion_r486328224
##########
File path: daffodil-core/src/main/scala/org/apache/daffodil/dsom/ElementBase.scala
##########
@@ -1114,4 +1115,10 @@ trait ElementBase
case (_, _) => this.subset((floating eq YesNo.No), "Property value floating='yes' is not supported.")
}
+ private lazy val checkInputValueCalcExpression: Boolean = {
+ if (this.inputValueCalcOption.toString.contains("checkConstraints(.)")) {
Review comment:
Is the idea that this inputValueCalc should throw and error? I created a test around it, but no error is thrown.
----------------------------------------------------------------
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] rsteinberger commented on a change in pull request #414: Throw SDE in the case of inputValueCalc containing checkConstraints(.)
Posted by GitBox <gi...@apache.org>.
rsteinberger commented on a change in pull request #414:
URL: https://github.com/apache/incubator-daffodil/pull/414#discussion_r490207087
##########
File path: daffodil-core/src/main/scala/org/apache/daffodil/dsom/ElementBase.scala
##########
@@ -1114,4 +1115,10 @@ trait ElementBase
case (_, _) => this.subset((floating eq YesNo.No), "Property value floating='yes' is not supported.")
}
+ private lazy val checkInputValueCalcExpression: Boolean = {
+ if (this.inputValueCalcOption.toString.contains("checkConstraints(.)")) {
Review comment:
In the case of <f xsi:nil="true" /> is it valid to throw this error?
----------------------------------------------------------------
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 #414: Throw SDE in the case of inputValueCalc containing checkConstraints(.)
Posted by GitBox <gi...@apache.org>.
mbeckerle commented on a change in pull request #414:
URL: https://github.com/apache/incubator-daffodil/pull/414#discussion_r483716705
##########
File path: daffodil-core/src/main/scala/org/apache/daffodil/dsom/ElementBase.scala
##########
@@ -1114,4 +1115,10 @@ trait ElementBase
case (_, _) => this.subset((floating eq YesNo.No), "Property value floating='yes' is not supported.")
}
+ private lazy val checkInputValueCalcExpression: Boolean = {
+ if (this.inputValueCalcOption.toString.contains("checkConstraints(.)")) {
Review comment:
Of course even that is not entirely correct. A path like "../." which is equivalent to ".." should be allowed.
Another way to fix this would be to modify the expression compiler with another parameter boolean indicating if the expression is for inputValueCalc. Then for each relative path expresion within the expression, if the relative path is only the Self path, then test the boolean and SDE if true.
----------------------------------------------------------------
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 #414: Throw SDE in the case of inputValueCalc containing checkConstraints(.)
Posted by GitBox <gi...@apache.org>.
stevedlawrence commented on a change in pull request #414:
URL: https://github.com/apache/incubator-daffodil/pull/414#discussion_r486333416
##########
File path: daffodil-core/src/main/scala/org/apache/daffodil/dsom/ElementBase.scala
##########
@@ -1114,4 +1115,10 @@ trait ElementBase
case (_, _) => this.subset((floating eq YesNo.No), "Property value floating='yes' is not supported.")
}
+ private lazy val checkInputValueCalcExpression: Boolean = {
+ if (this.inputValueCalcOption.toString.contains("checkConstraints(.)")) {
Review comment:
Interesting. Do you have any restrictions on your inputValueCalc element? It's possible that if there are no restrictions checkConstraints doesn't do anything and so it's essentially a no-op that returns true. I'm not sure if that's the expected behavior or not. What happens if you add a restriction?
----------------------------------------------------------------
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 #414: Throw SDE in the case of inputValueCalc containing checkConstraints(.)
Posted by GitBox <gi...@apache.org>.
mbeckerle commented on a change in pull request #414:
URL: https://github.com/apache/incubator-daffodil/pull/414#discussion_r490326080
##########
File path: daffodil-core/src/main/scala/org/apache/daffodil/dsom/ElementBase.scala
##########
@@ -1114,4 +1115,10 @@ trait ElementBase
case (_, _) => this.subset((floating eq YesNo.No), "Property value floating='yes' is not supported.")
}
+ private lazy val checkInputValueCalcExpression: Boolean = {
+ if (this.inputValueCalcOption.toString.contains("checkConstraints(.)")) {
Review comment:
That said, I'm not sure those limitations aren't just oversights. It may be that these things are needed and will eventually get added. But right now I think there is no notion of nilled and expressions.
----------------------------------------------------------------
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 #414: Throw SDE in the case of inputValueCalc containing checkConstraints(.)
Posted by GitBox <gi...@apache.org>.
mbeckerle commented on a change in pull request #414:
URL: https://github.com/apache/incubator-daffodil/pull/414#discussion_r489774943
##########
File path: daffodil-core/src/main/scala/org/apache/daffodil/dsom/ElementBase.scala
##########
@@ -1114,4 +1115,10 @@ trait ElementBase
case (_, _) => this.subset((floating eq YesNo.No), "Property value floating='yes' is not supported.")
}
+ private lazy val checkInputValueCalcExpression: Boolean = {
+ if (this.inputValueCalcOption.toString.contains("checkConstraints(.)")) {
Review comment:
So self-move is a no-op if it appears interior to a path, and it is dstate.currentValue if it appears last in a path expression. So, if there is no value for a self move, that has to be "." and that has to be an expression on a simple element.
Worth a try. You may need to get some context information instead of None, None, so that the user will get the file/line of the expression at least.
----------------------------------------------------------------
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