[GitHub] [incubator-daffodil] stevedlawrence commented on a change in pull request #396: Add tests for nested choice branch cases that seemed problematic.

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

stevedlawrence commented on a change in pull request #396:
URL: https://github.com/apache/incubator-daffodil/pull/396#discussion_r453611084



##########
File path: daffodil-test/src/test/resources/org/apache/daffodil/section07/discriminators/nestedChoiceDiscriminator.tdml
##########
@@ -0,0 +1,306 @@
+<?xml version="1.0" encoding="UTF-8"?>
+<!--
+  Licensed to the Apache Software Foundation (ASF) under one or more
+  contributor license agreements.  See the NOTICE file distributed with
+  this work for additional information regarding copyright ownership.
+  The ASF licenses this file to You under the Apache License, Version 2.0
+  (the "License"); you may not use this file except in compliance with
+  the License.  You may obtain a copy of the License at
+
+      http://www.apache.org/licenses/LICENSE-2.0
+
+  Unless required by applicable law or agreed to in writing, software
+  distributed under the License is distributed on an "AS IS" BASIS,
+  WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
+  See the License for the specific language governing permissions and
+  limitations under the License.
+-->
+
+<tdml:testSuite
+  xmlns:ex="http://example.com"
+  xmlns:tdml="http://www.ibm.com/xmlns/dfdl/testData"
+  xmlns:xsi="http://www.w3.org/2001/XMLSchema-instance"
+  xmlns:dfdl="http://www.ogf.org/dfdl/dfdl-1.0/"
+  xmlns:daf="urn:ogf:dfdl:2013:imp:daffodil.apache.org:2018:ext"
+  xmlns:xs="http://www.w3.org/2001/XMLSchema"
+  xmlns:fn="http://www.w3.org/2005/xpath-functions"
+  defaultRoundTrip="true"
+  defaultValidation="on">
+
+  <tdml:defineSchema name="s0" elementFormDefault="unqualified">
+    <xs:include schemaLocation="org/apache/daffodil/xsd/DFDLGeneralFormat.dfdl.xsd"/>
+    <dfdl:format ref="ex:GeneralFormat" lengthKind="delimited"/>
+
+    <!-- <![CDATA[
+
+    This idiom where we capture unrecognized messages under control of a variable is common and
+    there are issues with discriminators that need testing relative to it.
+
+    ]]> -->
+
+    <dfdl:defineVariable name="capture" type="xs:boolean" external="true" defaultValue="false"/>
+
+    <xs:element name="checkVar" type="xs:string"
+                dfdl:inputValueCalc='{ xs:string($ex:capture) }'/>
+
+    <xs:group name="discriminate">
+      <xs:sequence>
+        <xs:annotation>
+          <xs:appinfo source="http://www.ogf.org/dfdl/">
+            <dfdl:discriminator>{ fn:true() }</dfdl:discriminator>
+          </xs:appinfo>
+        </xs:annotation>
+      </xs:sequence>
+    </xs:group>
+
+    <xs:complexType name="messageType">
+      <xs:sequence dfdl:separator="|">
+        <xs:element name="num" type="xs:int"/>
+        <xs:element name="text" type="xs:string"/>
+      </xs:sequence>
+    </xs:complexType>
+
+    <xs:element name="root">
+      <xs:complexType>
+        <xs:sequence>
+          <xs:element ref="ex:checkVar"/>
+          <xs:element name="msg" minOccurs="1" maxOccurs="unbounded">
+            <xs:complexType>
+              <xs:sequence dfdl:initiator="%WSP*;[" dfdl:terminator="]%WSP*;">
+                <xs:element name="messageID" type="xs:string" dfdl:lengthKind="explicit" dfdl:length="1"/>
+                <xs:sequence dfdl:initiator="|">
+                  <xs:choice>
+                    <xs:choice dfdl:choiceDispatchKey="{ ./messageID }">
+                      <xs:sequence dfdl:choiceBranchKey="1">
+                        <!--
+                        This techique, where we discriminate the outer choice if the choice dispatch
+                        is successful, is better during DFDL schema development because we won't backtrack
+                        and create unrecognized messages that suppress errors during the parsing of a recognized
+                        message type.
+                        -->
+                        <xs:group ref="ex:discriminate"/>

Review comment:
       This doesn't behave the way I would expect it to by my reading of the spec. From the spec, both the inner and outer choices create a point of uncertainty:
   {code}
   An xs:choice is always a point of uncertainty. It is resolved sequentially, or by direct dispatch ... Direct-dispatch choice resolution occurs by matching the value of the dfdl:choiceDispatchKey property to the value of the dfdl:choiceChoiceBranchKey [sic] property of one of the choice branches.
   {code}
   
   So the direct dispatch resolves the inner PoU, but isn't that inner PoU still in scope when this discriminator is evaluated? I would expect this discriminator to resolve the inner choice PoU to true, which is essentially a no-op since it's already been resolved to true by direct dispatch. So if something fails inside the inner choice branch, I would expect we backtrack to the outer choice PoU and speculatively parse to the second branch of the outer choice. So I would expect the nestedChoice2 test to result in the unrecognizedMessage element since nothing resolves the outer choice. Feels like a bug to me, unless I'm misreading the spec.
   
   The way I see it, the problem is there isn't a good way to differentiate between "a direct dispatch branch was found but something inside it failed" vs "a direct dispatch branch was not found". The outer PoU only ever sees that something about the inner choice failed, and it can't know why. The inner discrminator seems to be able to break outside the inner PoU and provide that information, but that seems wrong to me. Perhaps what is needed is some sort of ``dfdl:choiceBranchDefault="yes"`` property, which I think would no longer require the outer choice or inner discriminators and would work as expected?

##########
File path: daffodil-test/src/test/scala/org/apache/daffodil/section07/discriminators/TestNestedChoices.scala
##########
@@ -0,0 +1,45 @@
+/*
+ * Licensed to the Apache Software Foundation (ASF) under one or more
+ * contributor license agreements.  See the NOTICE file distributed with
+ * this work for additional information regarding copyright ownership.
+ * The ASF licenses this file to You under the Apache License, Version 2.0
+ * (the "License"); you may not use this file except in compliance with
+ * the License.  You may obtain a copy of the License at
+ *
+ *     http://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing, software
+ * distributed under the License is distributed on an "AS IS" BASIS,
+ * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
+ * See the License for the specific language governing permissions and
+ * limitations under the License.
+ */
+
+package org.apache.daffodil.section07.discriminators
+
+/*import org.junit.Test */

Review comment:
       Remove import comment.




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