Only allow ambiguous path steps between scalar and optional elements#1180
Conversation
|
@pkatlic, can you take a look at this? This makes a tweak to the changes you made to fn:count |
tuxji
left a comment
tuxji
left a comment
There was a problem hiding this comment.
+1
I expanded the test results diffs to see the tests' comments, and the changes to the results appear to make sense (scalar & optional from ambiguous to good, array & optional from good to ambiguous).
| val (arrays, nonArrays) = stepElements.partition { _.isArray } | ||
| schemaDefinitionWhen( | ||
| arrays.length > 0 && nonArrays.length > 0, | ||
| "Path step %s is ambiguous. It can reference both an array or a non-array element, which is not allowed.\n" + |
There was a problem hiding this comment.
I suggest changing or to and in the error message above, even though it may cause some tests to require changes to their expected output. When you say "both", that implies "and" rather than "or".
| <tdml:error>ex:foo</tdml:error> | ||
| <tdml:error>ambiguous</tdml:error> | ||
| </tdml:errors> | ||
| <tdml:infoset> |
There was a problem hiding this comment.
@pkatlic: As @stevedlawrence asked, please check these changes to test results carefully.
There was a problem hiding this comment.
The new test results appear to correct the behavior, with scalarOptional parsing properly and arrayOptional throwing the SDE regarding ambiguous path steps.
| <!-- throws ClassCastException, possible bug in Daffodil group evaluation --> | ||
| <!-- <xs:group ref="countGroup"/> --> | ||
| <xs:element name="count" type="xs:int" dfdl:representation="binary" dfdl:inputValueCalc="{ fn:count(../ex:foo) }"/> | ||
| <xs:group ref="countGroup"/> |
There was a problem hiding this comment.
Yes, this now uses the group reference instead of standalone count element.
|
+1 The tests now have the expected results for the scalar/optional and array/optional element groups. |
Scalar and optional elements are both implemented in the infoset exactly the same as DIElements, the only real difference is optional elements might not appear in the infoset. Arrays on the other hand are implemented differently as a DIArray. Because of this, it adds complexity if we allow expression path steps that can ambiguously reference either an array or an optional. We currently allow this, but this complexity leads to bugs and thrown exceptions. In fact, in most cases ambiguously referencing an array or an optional doesn't even work since arrays need a predicate and optionals do not allow predicates. The only case it could potentially work is as the last step as an argument to a function like fn:count. The added complexity is not worth supporting this edge case. Instead of allowing this, we should instead only allow ambiguous paths steps to scalar and optional elements. This modifies the path step logic to enforce this. A step must now unambiguously reference an array or it must reference scalars/optionals. Note that this slightly affects the behavior of the fn:count() function. This function must still reference an array or optional as before, but it can now reference a scalar only if it is an ambiguous path that could also reference an optional. If the argument unambiguously references a scalar, it is an SDE as it was before. It can also no longer ambiguously reference an array or optional, but that was previously broken. DAFFODIL-2879
stevedlawrence
force-pushed
the
daffodil-2879-array-optional-ambiguous-path-step
branch
from
0754845 to
e08ee67
Compare
stevedlawrence
deleted the
daffodil-2879-array-optional-ambiguous-path-step
branch
3 participants
Scalar and optional elements are both implemented in the infoset exactly the same as DIElements, the only real difference is optional elements might not appear in the infoset. Arrays on the other hand are implemented differently as a DIArray. Because of this, it adds complexity if we allow expression path steps that can ambiguously reference either an array or an optional. We currently allow this, but this complexity leads to bugs and thrown exceptions.
In fact, in most cases ambiguously referencing an array or an optional doesn't even work since arrays need a predicate and optionals do not allow predicates. The only case it could potentially work is as the last step as an argument to a function like fn:count. The added complexity is not worth supporting this edge case.
Instead of allowing this, we should instead only allow ambiguous paths steps to scalar and optional elements. This modifies the path step logic to enforce this. A step must now unambiguously reference an array or it must reference scalars/optionals.
Note that this slightly affects the behavior of the fn:count() function. This function must still reference an array or optional as before, but it can now reference a scalar only if it is an ambiguous path that could also reference an optional. If the argument unambiguously references a scalar, it is an SDE as it was before. It can also no longer ambiguously reference an array or optional, but that was previously broken.
DAFFODIL-2879