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: [email protected]
For queries about this service, please contact Infrastructure at:
[email protected]
