You are viewing a plain text version of this content. The canonical link for it is here.
Posted to commits@daffodil.apache.org by "olabusayoT (via GitHub)" <gi...@apache.org> on 2025/10/20 17:03:19 UTC
[PR] Add TDML tests for choice branch selection [daffodil]
olabusayoT opened a new pull request, #1579:
URL: https://github.com/apache/daffodil/pull/1579
- include test cases to verify choice branch unparser behavior
DAFFODIL-2324
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@daffodil.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] Add TDML tests for choice branch selection [daffodil]
Posted by "stevedlawrence (via GitHub)" <gi...@apache.org>.
stevedlawrence commented on code in PR #1579:
URL: https://github.com/apache/daffodil/pull/1579#discussion_r2445626036
##########
daffodil-test/src/test/resources/org/apache/daffodil/section15/choice_groups/ChoiceBranches.tdml:
##########
@@ -0,0 +1,112 @@
+<?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 suiteName="choiceBranches" description="Tests for choice branch selection"
+ 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:xs="http://www.w3.org/2001/XMLSchema"
+ xmlns:ex="http://example.com"
+ xmlns:fn="http://www.w3.org/2005/xpath-functions"
+ xmlns="http://example.com">
+
+ <tdml:defineSchema name="choiceBranches">
+ <xs:include schemaLocation="org/apache/daffodil/xsd/DFDLGeneralFormat.dfdl.xsd"/>
+ <dfdl:format ref="ex:GeneralFormat"/>
+
+ <xs:group name="empty">
+ <xs:sequence>
+ <xs:element name="empty" type="xs:string" dfdl:lengthKind="explicit" dfdl:length="1"
+ dfdl:outputValueCalc="{ '0' }"/>
+ </xs:sequence>
+ </xs:group>
+
+ <xs:group name="notEmpty">
+ <xs:sequence>
+ <xs:element name="notEmpty" type="xs:string" dfdl:lengthKind="explicit" dfdl:length="1"
+ dfdl:outputValueCalc="{ '1' }"/>
+ </xs:sequence>
+ </xs:group>
+
+ <xs:element name="e1" dfdl:lengthKind="implicit">
+ <xs:complexType>
+ <xs:sequence>
+ <xs:choice>
+ <xs:sequence dfdl:hiddenGroupRef="ex:empty"/>
+ <xs:sequence>
+ <xs:sequence dfdl:hiddenGroupRef="ex:notEmpty"/>
+ <xs:element name="char" type="xs:string" minOccurs="0" dfdl:lengthKind="explicit" dfdl:length="1"/>
+ </xs:sequence>
+ </xs:choice>
+ </xs:sequence>
+ </xs:complexType>
+ </xs:element>
+
+ <xs:element name="e2" dfdl:lengthKind="implicit">
+ <xs:complexType>
+ <xs:choice>
+ <xs:sequence>
+ <xs:sequence dfdl:hiddenGroupRef="ex:notEmpty"/>
+ <xs:element name="char" type="xs:string" minOccurs="0" dfdl:lengthKind="explicit" dfdl:length="1"/>
+ </xs:sequence>
+ <xs:sequence dfdl:hiddenGroupRef="ex:empty"/>
+ </xs:choice>
+ </xs:complexType>
+ </xs:element>
+
Review Comment:
I think these schemas are only considering the "empty" case and not things like non-hidden OVCs or defaulatable elements that can be unparsed without being in the infoset. I *think* if those are in the schema before a hidden group ref, for example, then they should be chosen as the default unparse branch, but I think that currently isn't the case. We always prefer a hidden group ref branch even if it appears later in the choice.
I think the tests we really need is multiple schemas that have the same empty-ish branches but in different orders and it should always unparse whichever appears first in the schema.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@daffodil.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] Add TDML tests for choice branch selection [daffodil]
Posted by "mbeckerle (via GitHub)" <gi...@apache.org>.
mbeckerle commented on code in PR #1579:
URL: https://github.com/apache/daffodil/pull/1579#discussion_r2461103086
##########
daffodil-core/src/main/scala/org/apache/daffodil/core/runtime1/ChoiceTermRuntime1Mixin.scala:
##########
@@ -80,35 +80,25 @@ trait ChoiceTermRuntime1Mixin { self: ChoiceTermBase =>
// element event to be in the infoset (for round-tripping from parse). It's value will
// get recomputed, but it can appear with a stale value (left-over from parse)
// in the arriving infoset for unparse.
- //
- val optDefaultBranch = {
- val optEmpty: Option[Term] = {
- val emptyBranches = groupMembers.filter { gm =>
- val ies = gm.identifyingEventsForChoiceBranch
- ies.pnes.isEmpty // empty event list makes it the default, not simply isOpen
- }
- if (emptyBranches.length > 1)
- SDW(
- WarnID.MultipleChoiceBranches,
- "Multiple choice branches with no required elements detected and the infoset does not specify a branch, selecting the first branch for unparsing"
- )
- emptyBranches.headOption
- }
- val optOpen: Option[Term] =
- optEmpty.orElse {
- groupMembers.find { gm =>
- val ies = gm.identifyingEventsForChoiceBranch
- ies.isOpen // first open one is used if there is no branch that has empty event list. (test ptg_1u)
- }
- }
- val optDefault: Option[Term] =
- optOpen.orElse {
- groupMembers.find {
- _.canUnparseIfHidden
- } // optional, defaultable, OVC, or IVC
- }
- optDefault
+ // Rather than defaulting to the first empty branch, we instead default to the first
+ // empty, open or defaultable branch.
+ val emptyOpenOrDefault = groupMembers.filter {
+ case gm if gm.identifyingEventsForChoiceBranch.pnes.isEmpty => true
+ case gm if gm.identifyingEventsForChoiceBranch.isOpen => true
+ case gm if gm.canUnparseIfHidden => true
+ case _ => false
+ }
+ if (
+ emptyOpenOrDefault.length > 1 && emptyOpenOrDefault.forall(
+ _.identifyingEventsForChoiceBranch.pnes.isEmpty
+ )
+ ) {
+ SDW(
+ WarnID.MultipleChoiceBranches,
+ "Multiple choice branches with no required elements detected and the infoset does not specify a branch, selecting the first branch for unparsing"
+ )
Review Comment:
One can certainly have multiple open branches and this warning would be spurious. One infoset when unparsed could contain an optional part of one open branch, a different infoset when unparsed could contain an optional part of a different open branch.
So this warning should not be issued if all the branches are open.
If you have multiple empty branches then the 2nd and beyond empty branches could never be unparsed, and produce no infoset element when parsing. But... it's not illegal. It just is a bit weird, but someone could do it to explicitly absorb various optional syntax when parsing that is not to be regenerated when unparsing.
So I guess I agree this warning should be removed.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@daffodil.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] Add TDML tests for choice branch selection [daffodil]
Posted by "stevedlawrence (via GitHub)" <gi...@apache.org>.
stevedlawrence commented on code in PR #1579:
URL: https://github.com/apache/daffodil/pull/1579#discussion_r2474791022
##########
daffodil-core/src/main/scala/org/apache/daffodil/core/dsom/ParticleMixin.scala:
##########
@@ -91,8 +91,15 @@ trait RequiredOptionalMixin { self: ElementBase =>
val res = {
if (isScalar) !this.isOutputValueCalc
else if (isOptional) false
- // Treat all arrays as non-required so that we can tolerate invalid
- // infosets where the number of events is fewer than the array minimum occurrences.
+ // Treat all arrays as non-required if it has a default value or is optional
+ // so that we can tolerate invalid infosets where the number of events is
+ // fewer than the array minimum occurrences.
+ // But we don't want to unparse malformed data, so we only return true (required) if it is
+ // not defaultable (not yet implemented, so it will never be true at this point, but will
+ // produce a subset error with required arrays so people don't depend on broken/nyi behavior)
+ // and minOCcurs > 0
Review Comment:
This comment feels a bit confusing, we first say we treat all arrays as optional, then say we don't actually want to do because we don't want to create malformed data. So the two sentences kindof conflict. What if we instead just say something like
> Arrays with minOccurs = 0 are not required to b in the infoset. Arrays that have at least one require element (i.e. minOccurs > 0) are required to have unparser events. Otherwise we could unparser malformed data. The exception to this is required arrays that are defaultable (which is not yet implemented, so will never be true at this point but will produce a subset error so people don't depend on broken/nyi behavior).
##########
daffodil-core/src/main/scala/org/apache/daffodil/core/dsom/ParticleMixin.scala:
##########
Review Comment:
I think this comment needs to be updated? Arrays are only treated as optional if the have minOccurs="0"
##########
daffodil-test/src/test/scala/org/apache/daffodil/section15/choice_groups/TestChoiceBranches.scala:
##########
@@ -0,0 +1,63 @@
+/*
+ * 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.section15.choice_groups
+
+import org.apache.daffodil.junit.tdml.TdmlSuite
+import org.apache.daffodil.junit.tdml.TdmlTests
+
+import org.junit.Test
+
+object TestChoiceBranches extends TdmlSuite {
+ val tdmlResource =
+ "/org/apache/daffodil/section15/choice_groups/ChoiceBranches.tdml"
+}
+
+class TestChoiceBranches extends TdmlTests {
+
+ val tdmlSuite = TestChoiceBranches
+
+ @Test def choiceBranch_e1 = test
+
+ @Test def choiceBranch_e2 = test
+
+ @Test def choiceBranch_e3 = test
+
+ @Test def choiceBranch_e1_req = test
+
+ @Test def choiceBranch_e2_req = test
+
+ @Test def choiceBranch_e3_req = test
+
+ @Test def choiceBranch_e1_opt = test
+
+ @Test def choiceBranch_e2_opt = test
+
+ @Test def choiceBranch_e3_opt = test
+
+ @Test def choiceBranch_e1_reqElements = test
+
+ @Test def choiceBranch_e2_reqElements = test
+
+ @Test def choiceBranch_e3_reqElements = test
+
+ @Test def choiceBranch_e4 = test
+
+ @Test def choiceBranch_e5 = test
+
+ @Test def choiceBranch_e6 = test
+}
Review Comment:
Can you remove all the empty lines between tests?
##########
daffodil-test/src/test/resources/org/apache/daffodil/section15/choice_groups/choice-unparse2.tdml:
##########
@@ -123,7 +124,6 @@
</xs:complexType>
</xs:element>
</xs:sequence>
- <xs:sequence dfdl:hiddenGroupRef="ex:PI_false" />
Review Comment:
Following up on this comment, I think order used to matter because we treated arrays as optional, even if they had minOccurs="1", so the PI_true branch was technically optional and could be unparsed without infosets events so was teh default.
But I think with the latest change that's no longer true? The "rag" element is now considered required so the PI_true cannot be parsed if hidden. So I think now order doesn't matter for this particular test?
##########
daffodil-core/src/main/scala/org/apache/daffodil/core/runtime1/ChoiceTermRuntime1Mixin.scala:
##########
@@ -80,36 +80,15 @@ trait ChoiceTermRuntime1Mixin { self: ChoiceTermBase =>
// element event to be in the infoset (for round-tripping from parse). It's value will
// get recomputed, but it can appear with a stale value (left-over from parse)
// in the arriving infoset for unparse.
- //
- val optDefaultBranch = {
- val optEmpty: Option[Term] = {
- val emptyBranches = groupMembers.filter { gm =>
- val ies = gm.identifyingEventsForChoiceBranch
- ies.pnes.isEmpty // empty event list makes it the default, not simply isOpen
- }
- if (emptyBranches.length > 1)
- SDW(
- WarnID.MultipleChoiceBranches,
- "Multiple choice branches with no required elements detected and the infoset does not specify a branch, selecting the first branch for unparsing"
- )
- emptyBranches.headOption
- }
- val optOpen: Option[Term] =
- optEmpty.orElse {
- groupMembers.find { gm =>
- val ies = gm.identifyingEventsForChoiceBranch
- ies.isOpen // first open one is used if there is no branch that has empty event list. (test ptg_1u)
- }
- }
- val optDefault: Option[Term] =
- optOpen.orElse {
- groupMembers.find {
- _.canUnparseIfHidden
- } // optional, defaultable, OVC, or IVC
- }
- optDefault
+ // Rather than defaulting to the first empty (i.e no possible next elements) branch,
+ // we instead default to the first defaultable element (i.e has a defined dfdl:outputValueCalc,
+ // dfdl:inputValueCalc, an optional branch or an array branch with no required elements).
+ // It is important to note that canUnparseIfNoEvent subsumes empty branches, as well as
+ // open/optional branches, so we don't need to explicitly check for those
Review Comment:
I think saying "we instead default to the first defaultable element" is potentially confusing. "defaultable" could be confused with things that have `default="..."` which isn't necessary (or currently supported). And saying "element" is confusing because we could chose a branch that has no elements in it--it could just be a empty sequence with asserts, or a hidden group.
Maybe instead, just say something like
> We default to the first branch can be unparsed entirely without any infoset events (i.e. made up of elements that are dfdl:outputValueCalc, dfdl:inputValueCalc, option branches, zero length arrays, or defaultable elements)
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@daffodil.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] Add TDML tests for choice branch selection [daffodil]
Posted by "stevedlawrence (via GitHub)" <gi...@apache.org>.
stevedlawrence commented on code in PR #1579:
URL: https://github.com/apache/daffodil/pull/1579#discussion_r2454828980
##########
daffodil-core/src/main/scala/org/apache/daffodil/core/runtime1/ChoiceTermRuntime1Mixin.scala:
##########
@@ -80,35 +80,25 @@ trait ChoiceTermRuntime1Mixin { self: ChoiceTermBase =>
// element event to be in the infoset (for round-tripping from parse). It's value will
// get recomputed, but it can appear with a stale value (left-over from parse)
// in the arriving infoset for unparse.
- //
- val optDefaultBranch = {
- val optEmpty: Option[Term] = {
- val emptyBranches = groupMembers.filter { gm =>
- val ies = gm.identifyingEventsForChoiceBranch
- ies.pnes.isEmpty // empty event list makes it the default, not simply isOpen
- }
- if (emptyBranches.length > 1)
- SDW(
- WarnID.MultipleChoiceBranches,
- "Multiple choice branches with no required elements detected and the infoset does not specify a branch, selecting the first branch for unparsing"
- )
- emptyBranches.headOption
- }
- val optOpen: Option[Term] =
- optEmpty.orElse {
- groupMembers.find { gm =>
- val ies = gm.identifyingEventsForChoiceBranch
- ies.isOpen // first open one is used if there is no branch that has empty event list. (test ptg_1u)
- }
- }
- val optDefault: Option[Term] =
- optOpen.orElse {
- groupMembers.find {
- _.canUnparseIfHidden
- } // optional, defaultable, OVC, or IVC
- }
- optDefault
+ // Rather than defaulting to the first empty branch, we instead default to the first
+ // empty, open or defaultable branch.
Review Comment:
Can you expand these comments to explain what these cases represent? For example, what is an "empty" branch? Is that just a branch that literally has no elements in it, like just asserts or something? What is an open branch? A branch that is made up of entirely optional elements?
##########
daffodil-test/src/test/resources/org/apache/daffodil/section15/choice_groups/choice-unparse2.tdml:
##########
@@ -123,7 +124,6 @@
</xs:complexType>
</xs:element>
</xs:sequence>
- <xs:sequence dfdl:hiddenGroupRef="ex:PI_false" />
Review Comment:
Does order of these branches matter? Feels like it shouldn't.
This choice has two branches, one representing PI_false and one representing PI_true.
The PI_false branch has a hiddenGroupRef, so is entirely hidden and could be a default branch (assuming it is unparsable, which it is in our case).
The PI_true branch starts with a hidden group ref, but that is followed a non-optional "rag" array. So the PI_true branch has at least one required "rag" element. Meaning we should never unparse this branch unless we see a "rag" start element event, and so it should not be considered for being used as the default branch.
So of the two branches, only one can actually be a default branch so order shouldn't matter for this particular test. If the new logic could pick the PI_true branch as the default branch then I think it indicates we have a bug somewhere?
##########
daffodil-core/src/main/scala/org/apache/daffodil/core/runtime1/ChoiceTermRuntime1Mixin.scala:
##########
@@ -80,35 +80,25 @@ trait ChoiceTermRuntime1Mixin { self: ChoiceTermBase =>
// element event to be in the infoset (for round-tripping from parse). It's value will
// get recomputed, but it can appear with a stale value (left-over from parse)
// in the arriving infoset for unparse.
- //
- val optDefaultBranch = {
- val optEmpty: Option[Term] = {
- val emptyBranches = groupMembers.filter { gm =>
- val ies = gm.identifyingEventsForChoiceBranch
- ies.pnes.isEmpty // empty event list makes it the default, not simply isOpen
- }
- if (emptyBranches.length > 1)
- SDW(
- WarnID.MultipleChoiceBranches,
- "Multiple choice branches with no required elements detected and the infoset does not specify a branch, selecting the first branch for unparsing"
- )
- emptyBranches.headOption
- }
- val optOpen: Option[Term] =
- optEmpty.orElse {
- groupMembers.find { gm =>
- val ies = gm.identifyingEventsForChoiceBranch
- ies.isOpen // first open one is used if there is no branch that has empty event list. (test ptg_1u)
- }
- }
- val optDefault: Option[Term] =
- optOpen.orElse {
- groupMembers.find {
- _.canUnparseIfHidden
- } // optional, defaultable, OVC, or IVC
- }
- optDefault
+ // Rather than defaulting to the first empty branch, we instead default to the first
+ // empty, open or defaultable branch.
+ val emptyOpenOrDefault = groupMembers.filter {
+ case gm if gm.identifyingEventsForChoiceBranch.pnes.isEmpty => true
+ case gm if gm.identifyingEventsForChoiceBranch.isOpen => true
+ case gm if gm.canUnparseIfHidden => true
+ case _ => false
+ }
+ if (
+ emptyOpenOrDefault.length > 1 && emptyOpenOrDefault.forall(
+ _.identifyingEventsForChoiceBranch.pnes.isEmpty
+ )
+ ) {
+ SDW(
+ WarnID.MultipleChoiceBranches,
+ "Multiple choice branches with no required elements detected and the infoset does not specify a branch, selecting the first branch for unparsing"
+ )
Review Comment:
I wonder if we should remove this warning? I think it is perfectly reasonable to have a schema where multiple branches could be used as the default, and the errata makes it clear that the first one will be used.
It might also mean we could change the above `filter` to a `find` to avoid some allocations.
##########
daffodil-test/src/test/resources/org/apache/daffodil/section15/choice_groups/ChoiceBranches.tdml:
##########
@@ -0,0 +1,278 @@
+<?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 suiteName="choiceBranches" description="Tests for choice branch selection"
+ 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:xs="http://www.w3.org/2001/XMLSchema"
+ xmlns:ex="http://example.com"
+ xmlns:fn="http://www.w3.org/2005/xpath-functions"
+ xmlns="http://example.com">
+
+ <tdml:defineSchema name="choiceBranches">
+ <xs:include schemaLocation="/org/apache/daffodil/xsd/DFDLGeneralFormat.dfdl.xsd"/>
+ <dfdl:format ref="ex:GeneralFormat"/>
+
+ <xs:group name="group_a">
+ <xs:sequence>
+ <xs:element name="a" type="xs:string" minOccurs="0" dfdl:lengthKind="explicit" dfdl:length="1"/>
+ </xs:sequence>
+ </xs:group>
+
+ <xs:group name="group_b">
+ <xs:sequence>
+ <xs:element name="b" type="xs:string" dfdl:lengthKind="explicit" dfdl:length="1" dfdl:outputValueCalc="{'b'}"/>
+ </xs:sequence>
+ </xs:group>
+
+ <xs:group name="optElement">
+ <xs:sequence>
+ <xs:element name="opt" type="xs:string" minOccurs="0" dfdl:lengthKind="explicit" dfdl:length="1"/>
+ </xs:sequence>
+ </xs:group>
+
+ <xs:group name="reqElements">
+ <xs:sequence>
+ <xs:element name="reqElements_a" type="xs:string" dfdl:lengthKind="explicit" dfdl:outputValueCalc="{'a'}"
+ dfdl:length="1"/>
+ <xs:element name="reqElements_b" type="xs:string" dfdl:lengthKind="explicit" dfdl:outputValueCalc="{'b'}"
+ dfdl:length="1"/>
+ </xs:sequence>
+ </xs:group>
+
+ <xs:element name="e1" dfdl:lengthKind="implicit">
+ <xs:complexType>
+ <xs:sequence>
+ <xs:choice>
+ <xs:group dfdl:initiator="first_defaultable" ref="ex:optElement"/> <!-- canUnparseIfHidden/isOpen -->
+ <xs:sequence dfdl:initiator="second_open"> <!--isOpen/canUnparseIfHidden-->
+ <xs:sequence dfdl:hiddenGroupRef="ex:reqElements"/>
+ <xs:element name="char_a" minOccurs="0" type="xs:string" dfdl:lengthKind="explicit"
+ dfdl:length="1"/>
+ </xs:sequence>
+ <xs:sequence dfdl:initiator="third_empty"> <!-- isOpen/isEmpty -->
+ <xs:choice>
+ <xs:sequence dfdl:hiddenGroupRef="ex:group_a"/>
+ <xs:sequence dfdl:hiddenGroupRef="ex:group_b"/>
+ </xs:choice>
+ </xs:sequence>
+ <xs:element name="req" type="xs:string" dfdl:lengthKind="explicit" dfdl:length="1"/>
+ </xs:choice>
+ <xs:element name="after" type="xs:string" dfdl:lengthKind="explicit" dfdl:length="1"/>
+ </xs:sequence>
+ </xs:complexType>
+ </xs:element>
+
+ <xs:element name="e2" dfdl:lengthKind="implicit">
+ <xs:complexType>
+ <xs:sequence>
+ <xs:choice>
+ <xs:sequence dfdl:initiator="first_open"> <!--isOpen/canUnparseIfHidden-->
+ <xs:sequence dfdl:hiddenGroupRef="ex:reqElements"/>
+ <xs:element name="char_a" minOccurs="0" type="xs:string" dfdl:lengthKind="explicit"
+ dfdl:length="1"/>
+ </xs:sequence>
+ <xs:group dfdl:initiator="second_defaultable" ref="ex:optElement"/> <!-- canUnparseIfHidden/isOpen -->
+ <xs:sequence dfdl:initiator="third_empty"> <!-- isOpen/isEmpty -->
+ <xs:choice>
+ <xs:sequence dfdl:hiddenGroupRef="ex:group_a"/>
+ <xs:sequence dfdl:hiddenGroupRef="ex:group_b"/>
+ </xs:choice>
+ </xs:sequence>
+ <xs:element name="req" type="xs:string" dfdl:lengthKind="explicit" dfdl:length="1"/>
+ </xs:choice>
+ <xs:element name="after" type="xs:string" dfdl:lengthKind="explicit" dfdl:length="1"/>
+ </xs:sequence>
+ </xs:complexType>
+ </xs:element>
+
+ <xs:element name="e3" dfdl:lengthKind="implicit">
+ <xs:complexType>
+ <xs:sequence>
+ <xs:choice>
+ <xs:sequence dfdl:initiator="first_empty"> <!-- isOpen/isEmpty -->
+ <xs:choice>
+ <xs:sequence dfdl:hiddenGroupRef="ex:group_a"/>
+ <xs:sequence dfdl:hiddenGroupRef="ex:group_b"/>
+ </xs:choice>
+ </xs:sequence>
+ <xs:sequence dfdl:initiator="second_open"> <!--isOpen/canUnparseIfHidden-->
+ <xs:sequence dfdl:hiddenGroupRef="ex:reqElements"/>
+ <xs:element name="char_a" minOccurs="0" type="xs:string" dfdl:lengthKind="explicit"
+ dfdl:length="1"/>
+ </xs:sequence>
+ <xs:group dfdl:initiator="third_defaultable" ref="ex:optElement"/> <!-- canUnparseIfHidden/isOpen -->
+
+ <xs:element name="req" type="xs:string" dfdl:lengthKind="explicit" dfdl:length="1"/>
+ </xs:choice>
+ <xs:element name="after" type="xs:string" dfdl:lengthKind="explicit" dfdl:length="1"/>
+ </xs:sequence>
+ </xs:complexType>
+ </xs:element>
+
+ </tdml:defineSchema>
+
+ <tdml:unparserTestCase name="choiceBranch_e1" model="choiceBranches" root="e1">
+ <tdml:infoset>
+ <tdml:dfdlInfoset>
+ <ex:e1>
+ <after>1</after>
+ </ex:e1>
+ </tdml:dfdlInfoset>
+ </tdml:infoset>
+ <tdml:document>first_defaultable1</tdml:document>
+ </tdml:unparserTestCase>
+
+ <tdml:unparserTestCase name="choiceBranch_e2" model="choiceBranches" root="e2">
+ <tdml:infoset>
+ <tdml:dfdlInfoset>
+ <e2>
+ <after>1</after>
+ </e2>
+ </tdml:dfdlInfoset>
+ </tdml:infoset>
+ <tdml:document>first_openab1</tdml:document>
+ </tdml:unparserTestCase>
+
+ <tdml:unparserTestCase name="choiceBranch_e3" model="choiceBranches" root="e3">
+ <tdml:infoset>
+ <tdml:dfdlInfoset>
+ <e3>
+ <after>1</after>
+ </e3>
+ </tdml:dfdlInfoset>
+ </tdml:infoset>
+ <tdml:document>first_empty1</tdml:document>
+ </tdml:unparserTestCase>
+
+ <tdml:unparserTestCase name="choiceBranch_e1_req" model="choiceBranches" root="e1">
+ <tdml:infoset>
+ <tdml:dfdlInfoset>
+ <ex:e1>
+ <req>0</req>
+ <after>1</after>
+ </ex:e1>
+ </tdml:dfdlInfoset>
+ </tdml:infoset>
+ <tdml:document>01</tdml:document>
+ </tdml:unparserTestCase>
+
+ <tdml:unparserTestCase name="choiceBranch_e2_req" model="choiceBranches" root="e2">
+ <tdml:infoset>
+ <tdml:dfdlInfoset>
+ <e2>
+ <req>0</req>
+ <after>1</after>
+ </e2>
+ </tdml:dfdlInfoset>
+ </tdml:infoset>
+ <tdml:document>01</tdml:document>
+ </tdml:unparserTestCase>
+
+ <tdml:unparserTestCase name="choiceBranch_e3_req" model="choiceBranches" root="e3">
+ <tdml:infoset>
+ <tdml:dfdlInfoset>
+ <e3>
+ <req>0</req>
+ <after>1</after>
+ </e3>
+ </tdml:dfdlInfoset>
+ </tdml:infoset>
+ <tdml:document>01</tdml:document>
+ </tdml:unparserTestCase>
+
+ <tdml:unparserTestCase name="choiceBranch_e1_opt" model="choiceBranches" root="e1">
+ <tdml:infoset>
+ <tdml:dfdlInfoset>
+ <ex:e1>
+ <opt>0</opt>
+ <after>1</after>
+ </ex:e1>
+ </tdml:dfdlInfoset>
+ </tdml:infoset>
+ <tdml:document>first_defaultable01</tdml:document>
+ </tdml:unparserTestCase>
+
+ <tdml:unparserTestCase name="choiceBranch_e2_opt" model="choiceBranches" root="e2">
+ <tdml:infoset>
+ <tdml:dfdlInfoset>
+ <e2>
+ <opt>0</opt>
+ <after>1</after>
+ </e2>
+ </tdml:dfdlInfoset>
+ </tdml:infoset>
+ <tdml:document>second_defaultable01</tdml:document>
+ </tdml:unparserTestCase>
+
+ <tdml:unparserTestCase name="choiceBranch_e3_opt" model="choiceBranches" root="e3">
+ <tdml:infoset>
+ <tdml:dfdlInfoset>
+ <e3>
+ <opt>0</opt>
+ <after>1</after>
+ </e3>
+ </tdml:dfdlInfoset>
+ </tdml:infoset>
+ <tdml:document>third_defaultable01</tdml:document>
+
+ <tdml:warnings>
+ <tdml:warning>MultipleChoiceBranches</tdml:warning>
+ <tdml:warning>selecting</tdml:warning>
+ <tdml:warning>first branch</tdml:warning>
+ <tdml:warning>for unparsing</tdml:warning>
+ </tdml:warnings>
+ </tdml:unparserTestCase>
+
+
+ <tdml:unparserTestCase name="choiceBranch_e1_reqElements" model="choiceBranches" root="e1">
+ <tdml:infoset>
+ <tdml:dfdlInfoset>
+ <ex:e1>
+ <char_a>c</char_a>
+ <after>1</after>
+ </ex:e1>
+ </tdml:dfdlInfoset>
+ </tdml:infoset>
+ <tdml:document>second_openabc1</tdml:document>
+ </tdml:unparserTestCase>
+
+ <tdml:unparserTestCase name="choiceBranch_e2_reqElements" model="choiceBranches" root="e2">
+ <tdml:infoset>
+ <tdml:dfdlInfoset>
+ <e2>
+ <char_a>c</char_a>
+ <after>1</after>
+ </e2>
+ </tdml:dfdlInfoset>
+ </tdml:infoset>
+ <tdml:document>first_openabc1</tdml:document>
+ </tdml:unparserTestCase>
+
+ <tdml:unparserTestCase name="choiceBranch_e3_reqElements" model="choiceBranches" root="e3">
+ <tdml:infoset>
+ <tdml:dfdlInfoset>
+ <e3>
+ <char_a>c</char_a>
+ <after>1</after>
+ </e3>
+ </tdml:dfdlInfoset>
+ </tdml:infoset>
+ <tdml:document>second_openabc1</tdml:document>
+ </tdml:unparserTestCase>
+
Review Comment:
Nice tests!
##########
daffodil-test/src/test/scala/org/apache/daffodil/section15/choice_groups/TestChoiceBranches.scala:
##########
@@ -0,0 +1,50 @@
+/*
+ * 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.section15.choice_groups
+
+import org.apache.daffodil.junit.tdml.TdmlSuite
+import org.apache.daffodil.junit.tdml.TdmlTests
+import org.apache.daffodil.tdml.Runner
+
+import org.junit.Test
+
+object TestChoiceBranches extends TdmlSuite {
+ val tdmlResource =
+ "/org/apache/daffodil/section15/choice_groups/ChoiceBranches.tdml"
+
+ override def createRunner() =
+ Runner(tdmlDir, tdmlFile, validateTDMLFile = false)
Review Comment:
Can we remove this custom `createRunner`? The only difference between this and the default `createRunner` is this sets `validateTDMLFile = false`, but I *think* TDML file looks valid maybe isn't needed?
##########
daffodil-core/src/main/scala/org/apache/daffodil/core/runtime1/ChoiceTermRuntime1Mixin.scala:
##########
@@ -80,35 +80,25 @@ trait ChoiceTermRuntime1Mixin { self: ChoiceTermBase =>
// element event to be in the infoset (for round-tripping from parse). It's value will
// get recomputed, but it can appear with a stale value (left-over from parse)
// in the arriving infoset for unparse.
- //
- val optDefaultBranch = {
- val optEmpty: Option[Term] = {
- val emptyBranches = groupMembers.filter { gm =>
- val ies = gm.identifyingEventsForChoiceBranch
- ies.pnes.isEmpty // empty event list makes it the default, not simply isOpen
- }
- if (emptyBranches.length > 1)
- SDW(
- WarnID.MultipleChoiceBranches,
- "Multiple choice branches with no required elements detected and the infoset does not specify a branch, selecting the first branch for unparsing"
- )
- emptyBranches.headOption
- }
- val optOpen: Option[Term] =
- optEmpty.orElse {
- groupMembers.find { gm =>
- val ies = gm.identifyingEventsForChoiceBranch
- ies.isOpen // first open one is used if there is no branch that has empty event list. (test ptg_1u)
- }
- }
- val optDefault: Option[Term] =
- optOpen.orElse {
- groupMembers.find {
- _.canUnparseIfHidden
- } // optional, defaultable, OVC, or IVC
- }
- optDefault
+ // Rather than defaulting to the first empty branch, we instead default to the first
+ // empty, open or defaultable branch.
+ val emptyOpenOrDefault = groupMembers.filter {
+ case gm if gm.identifyingEventsForChoiceBranch.pnes.isEmpty => true
+ case gm if gm.identifyingEventsForChoiceBranch.isOpen => true
+ case gm if gm.canUnparseIfHidden => true
+ case _ => false
+ }
+ if (
+ emptyOpenOrDefault.length > 1 && emptyOpenOrDefault.forall(
+ _.identifyingEventsForChoiceBranch.pnes.isEmpty
+ )
+ ) {
+ SDW(
+ WarnID.MultipleChoiceBranches,
+ "Multiple choice branches with no required elements detected and the infoset does not specify a branch, selecting the first branch for unparsing"
+ )
}
+ val optDefaultBranch = emptyOpenOrDefault.headOption
Review Comment:
Since this is an opt, it means it is possible that we didn't find a default branch. In most cases this usually means that none of the the branches are empty/open/etc, and so we'll never need a default branch. But, could it also mean that there *are* empty branches, but they just can't be unparsed for some reason (e.g. `canUnparseIfHidden == false`)? In those cases would it mean that not having a default branch should be considered an error? We don't catch that here, but do we catch it somewhere else? Do we have any tests that error if we don't find a default branch but we might need one?
##########
daffodil-test/src/test/scala/org/apache/daffodil/section15/choice_groups/TestChoiceBranches.scala:
##########
@@ -0,0 +1,50 @@
+/*
+ * 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.section15.choice_groups
+
+import org.apache.daffodil.junit.tdml.TdmlSuite
+import org.apache.daffodil.junit.tdml.TdmlTests
+import org.apache.daffodil.tdml.Runner
+
+import org.junit.Test
+
+object TestChoiceBranches extends TdmlSuite {
+ val tdmlResource =
+ "/org/apache/daffodil/section15/choice_groups/ChoiceBranches.tdml"
+
+ override def createRunner() =
+ Runner(tdmlDir, tdmlFile, validateTDMLFile = false)
+}
+
+class TestChoiceBranches extends TdmlTests {
+
+ val tdmlSuite = TestChoiceBranches
+
+ @Test def choiceBranch_e1(): Unit = test
+ @Test def choiceBranch_e2(): Unit = test
+ @Test def choiceBranch_e3(): Unit = test
+ @Test def choiceBranch_e1_req(): Unit = test
+ @Test def choiceBranch_e2_req(): Unit = test
+ @Test def choiceBranch_e3_req(): Unit = test
+ @Test def choiceBranch_e1_opt(): Unit = test
+ @Test def choiceBranch_e2_opt(): Unit = test
+ @Test def choiceBranch_e3_opt(): Unit = test
+ @Test def choiceBranch_e1_reqElements(): Unit = test
+ @Test def choiceBranch_e2_reqElements(): Unit = test
+ @Test def choiceBranch_e3_reqElements(): Unit = test
Review Comment:
The convention we have been using for these tests is to exclude the parentheses and the Unit return type, e.g.
```scala
@Test def choiceBranch_e3_reqElements = test
```
They aren't required and keeps our tests a bit more minimal.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@daffodil.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] Add TDML tests for choice branch selection [daffodil]
Posted by "olabusayoT (via GitHub)" <gi...@apache.org>.
olabusayoT merged PR #1579:
URL: https://github.com/apache/daffodil/pull/1579
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@daffodil.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] Add TDML tests for choice branch selection [daffodil]
Posted by "stevedlawrence (via GitHub)" <gi...@apache.org>.
stevedlawrence commented on code in PR #1579:
URL: https://github.com/apache/daffodil/pull/1579#discussion_r2478872180
##########
daffodil-test-integration/src/test/scala/org/apache/daffodil/cliTest/TestCLIDebugger.scala:
##########
@@ -1208,10 +1207,6 @@ class TestCLIDebugger {
cli.sendLine("step")
cli.sendLine("step")
cli.sendLine("step")
- cli.expect(regexp("RegionSplit.* for cell"))
- cli.sendLine("info suspensions")
- cli.expect(regexp("Suppressable.* for cell"))
- cli.expect(regexp("RegionSplit.* for cell"))
Review Comment:
Removing these `expects` and `sendLines` changes this test. And looking at the commit when this was added, it was specifically about checking to make sure `info suspensions` and `info diff` with suspensions works as expected. So I'm not sure this is the right fix.
If this schema doesn't create suspensions anymore then we should probably switch this test to use a different schema so that we can keep info suspensions and info diff with suspensions in the test.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@daffodil.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] Add TDML tests for choice branch selection [daffodil]
Posted by "stevedlawrence (via GitHub)" <gi...@apache.org>.
stevedlawrence commented on code in PR #1579:
URL: https://github.com/apache/daffodil/pull/1579#discussion_r2499060165
##########
daffodil-core/src/main/scala/org/apache/daffodil/core/dsom/Term.scala:
##########
@@ -511,6 +511,24 @@ trait Term
* Note that this currently only requires OVC and Optionality since defaults
* aren't fully implemented everywhere. This function may need to change when
* defaults are fully implemented.
+ *
+ * Hidden IVCs — elements computed during parse but never represented in the
+ * infoset or needed during unparse — are not meaningful for unparsing and
+ * should not be supported. So we remove !e.isRepresented from the ElementBase
+ * case.
+ *
+ * Unparsing should instead require IVC elements (minOccurs = maxOccurs = 1)
+ * to appear in the infoset, even though their values are ignored at unparse
+ * time. This behavior is more intuitive, and is in line with IVC being MustExist in
+ * canUnparseIfNoEvents and preserves expected branch selection semantics for
+ * choice/dispatch constructs (e.g., the “raster” schema case).
Review Comment:
Suggest we remove mention of "raster", this is not a publicly available schema so doesn't really provide value.
##########
daffodil-core/src/main/scala/org/apache/daffodil/core/dsom/Term.scala:
##########
@@ -511,6 +511,24 @@ trait Term
* Note that this currently only requires OVC and Optionality since defaults
* aren't fully implemented everywhere. This function may need to change when
* defaults are fully implemented.
+ *
+ * Hidden IVCs — elements computed during parse but never represented in the
+ * infoset or needed during unparse — are not meaningful for unparsing and
+ * should not be supported. So we remove !e.isRepresented from the ElementBase
Review Comment:
If someone was reading this code/comment for the first time, I think saying "So we remove !e.isRepresented" is potentially confusing. They don't know that the code used to consider e.isRepresented, so it feels like it would be confusing why you even bring it up.
In fact, as I read through this whole comment a couple times, it feels more appropriate for a commit message. It isn't so much explaining what the code does, but is really adding justification for why checking e.isRepresented was not the correct thing do to. Which is exactly the kind of thing that should be in a commit message rather than a comment.
I think if we want to call out IVC here, which we should probably do so it's clear we are intentionally not allowing it to be unparsed if hidden, we can just say something like:
> Note that even though inputValueCalc elements are essentially ignored for unparsing, they are still considered required elements (canBeAbsnsetFromUnparseInfoset returns false) and so cannot be unparsed if they do not appear in the infoset (e.g. inside a hidden grep ref). This does not necessarily exclude inputValueCalc elements from appearing in hidden groups, but they must be in the non-default branch of a choice, or a child to an optional complex type or non-required array.
I'm not sure that's the best comment, but something along those lines that explains the current state of the code.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@daffodil.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org