You are viewing a plain text version of this content. The canonical link for it is here.
Posted to commits@daffodil.apache.org by "stevedlawrence (via GitHub)" <gi...@apache.org> on 2024/09/06 19:22:15 UTC
[PR] Fix check for explicit length within length facet range [daffodil]
stevedlawrence opened a new pull request, #1293:
URL: https://github.com/apache/daffodil/pull/1293
When we check if an explicit dfdl:length is within range of the minLength/maxLength/length facets, we do not take into account units or character encoding, which can lead to incorrect and confusing warnings.
This modifies the check to use the existing LengthInBitsEv to calculate the length in bits of an element (which uses lengthUnits), and uses CharsetEv for string types to correctly convert the explicit bit length to infoset length, where possible
DAFFODIL-2888
--
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] Fix check for explicit length within length facet range [daffodil]
Posted by "olabusayoT (via GitHub)" <gi...@apache.org>.
olabusayoT commented on code in PR #1293:
URL: https://github.com/apache/daffodil/pull/1293#discussion_r1763760470
##########
daffodil-core/src/main/scala/org/apache/daffodil/core/grammar/ElementBaseGrammarMixin.scala:
##########
@@ -1611,37 +1611,47 @@ trait ElementBaseGrammarMixin
)
case _ => // ok
}
+
/*
- * When lengthKind is explicit and length is a constant, it is a warning if
- * the type is a type that respects minLength and maxLength, and the constant length
- * is not in range.
+ * Warn if a type respects minLength/maxLength/length facets and we can calculate that the
+ * infoset length will be out of range of the facet values. Note that we can only do this in
+ * specific cases, like when the length and encodig properties are constant and the encoding
Review Comment:
encoding*
--
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] Fix check for explicit length within length facet range [daffodil]
Posted by "stevedlawrence (via GitHub)" <gi...@apache.org>.
stevedlawrence merged PR #1293:
URL: https://github.com/apache/daffodil/pull/1293
--
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] Fix check for explicit length within length facet range [daffodil]
Posted by "stevedlawrence (via GitHub)" <gi...@apache.org>.
stevedlawrence commented on code in PR #1293:
URL: https://github.com/apache/daffodil/pull/1293#discussion_r1750135463
##########
daffodil-core/src/main/scala/org/apache/daffodil/core/grammar/ElementBaseGrammarMixin.scala:
##########
@@ -1611,37 +1611,47 @@ trait ElementBaseGrammarMixin
)
case _ => // ok
}
+
/*
- * When lengthKind is explicit and length is a constant, it is a warning if
- * the type is a type that respects minLength and maxLength, and the constant length
- * is not in range.
+ * Warn if a type respects minLength/maxLength/length facets and we can calculate that the
+ * infoset length will be out of range of the facet values. Note that we can only do this in
+ * specific cases, like when the length and encodig properties are constant and the encoding
+ * is fixed width characters.
*/
- val isTypeUsingLengthOrMinMaxLengthFacets = typeDef.typeNode match {
- case s: NodeInfo.String.Kind => true
- case s: NodeInfo.HexBinary.Kind => true
- case _ => false
- }
- if (
- (lengthKind eq LengthKind.Explicit) &&
- isTypeUsingLengthOrMinMaxLengthFacets &&
- optLengthConstant.isDefined
- ) {
- val len = optLengthConstant.get
- lazy val maxLengthLong = maxLength.longValueExact
- lazy val minLengthLong = minLength.longValueExact
- def warn(m: String, value: Long): Unit = SDW(
- WarnID.FacetExplicitLengthOutOfRange,
- "Explicit dfdl:length of %s is out of range for facet %s='%s'.",
- len,
- m,
- value
- )
- if (hasLength && len != minLengthLong && len != maxLengthLong)
- warn("length", minLengthLong)
- else if (hasMinLength || hasMaxLength) {
- if (maxLengthLong != -1 && len > maxLengthLong) warn("maxLength", maxLengthLong)
- Assert.invariant(minLengthLong >= 0)
- if (minLengthLong > 0 && len < minLengthLong) warn("minLength", minLengthLong)
+ if ((lengthKind eq LengthKind.Explicit) && (hasLength || hasMinLength || hasMaxLength)) {
+ val optInfosetLen = elementLengthInBitsEv.optConstant.flatMap { maybeKnownLenInBits =>
Review Comment:
Yep, the type of the `elementLengthInBits` is a `MaybeJULong` so it matches the "maybe" in "maybeKnownLenInBits".
--
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] Fix check for explicit length within length facet range [daffodil]
Posted by "mbeckerle (via GitHub)" <gi...@apache.org>.
mbeckerle commented on code in PR #1293:
URL: https://github.com/apache/daffodil/pull/1293#discussion_r1747735949
##########
daffodil-core/src/main/scala/org/apache/daffodil/core/grammar/ElementBaseGrammarMixin.scala:
##########
@@ -1611,37 +1611,47 @@ trait ElementBaseGrammarMixin
)
case _ => // ok
}
+
/*
- * When lengthKind is explicit and length is a constant, it is a warning if
- * the type is a type that respects minLength and maxLength, and the constant length
- * is not in range.
+ * Warn if a type respects minLength/maxLength/length facets and we can calculate that the
+ * infoset length will be out of range of the facet values. Note that we can only do this in
+ * specific cases, like when the length and encodig properties are constant and the encoding
+ * is fixed width characters.
*/
- val isTypeUsingLengthOrMinMaxLengthFacets = typeDef.typeNode match {
- case s: NodeInfo.String.Kind => true
- case s: NodeInfo.HexBinary.Kind => true
- case _ => false
- }
- if (
- (lengthKind eq LengthKind.Explicit) &&
- isTypeUsingLengthOrMinMaxLengthFacets &&
- optLengthConstant.isDefined
- ) {
- val len = optLengthConstant.get
- lazy val maxLengthLong = maxLength.longValueExact
- lazy val minLengthLong = minLength.longValueExact
- def warn(m: String, value: Long): Unit = SDW(
- WarnID.FacetExplicitLengthOutOfRange,
- "Explicit dfdl:length of %s is out of range for facet %s='%s'.",
- len,
- m,
- value
- )
- if (hasLength && len != minLengthLong && len != maxLengthLong)
- warn("length", minLengthLong)
- else if (hasMinLength || hasMaxLength) {
- if (maxLengthLong != -1 && len > maxLengthLong) warn("maxLength", maxLengthLong)
- Assert.invariant(minLengthLong >= 0)
- if (minLengthLong > 0 && len < minLengthLong) warn("minLength", minLengthLong)
+ if ((lengthKind eq LengthKind.Explicit) && (hasLength || hasMinLength || hasMaxLength)) {
+ val optInfosetLen = elementLengthInBitsEv.optConstant.flatMap { maybeKnownLenInBits =>
Review Comment:
naming convention. Normally the prefix "maybe" is used for things of type Maybe. Is the type of optConstant's result a Maybe? If so then all good. I just want to make sure we're following those conventions if all these types are getting omitted.
--
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