stevedlawrence commented on code in PR #1192:
URL: https://github.com/apache/daffodil/pull/1192#discussion_r1539081840
##########
daffodil-core/src/main/scala/org/apache/daffodil/core/dsom/ElementBase.scala:
##########
@@ -960,12 +962,73 @@ trait ElementBase
typeDef.optRestriction.flatMap { _.enumerationValues }
}
+ final lazy val length: java.math.BigDecimal = {
Review Comment:
This looks like mostly a copy from computeMinMaxLength, I wonder if we can
move this logic into computeMinMaxLenghth I'd like to avoid duplicating
code--if we ever need to change one place we're probably going to forget to
change it in the other.
##########
daffodil-core/src/main/scala/org/apache/daffodil/core/dsom/Facets.scala:
##########
@@ -93,8 +96,27 @@ trait Facets { self: Restriction =>
final lazy val localMaxInclusiveValue: String = maxInclusive(xml)
final lazy val localMinExclusiveValue: String = minExclusive(xml)
final lazy val localMaxExclusiveValue: String = maxExclusive(xml)
- final lazy val localMinLengthValue: String = minLength(xml)
- final lazy val localMaxLengthValue: String = maxLength(xml)
+ final lazy val localLengthValue: String = length(xml)
+ final lazy val localMinLengthValue: String = {
+ val ml = minLength(xml)
+ // Xerces checks for the case where length and min/maxLength are used
together,
+ // so we won't get to this code in those cases unless Xerces validation is
turned off
+ Assert.usage(
+ ml.isEmpty || localLengthValue.isEmpty,
+ "Length and minLength cannot be specified together",
Review Comment:
It might be wroth specify something like "Length and minLength facets ...",
just to make it clear this is about facets and now about dfdl:length?
##########
daffodil-core/src/main/scala/org/apache/daffodil/core/dsom/ElementBase.scala:
##########
@@ -960,12 +962,73 @@ trait ElementBase
typeDef.optRestriction.flatMap { _.enumerationValues }
}
+ final lazy val length: java.math.BigDecimal = {
+ Assert.invariant(hasLength)
+ schemaDefinitionUnless(
+ isSimpleType,
+ "Facet length is allowed only on simple types or types derived from
simple types.",
+ )
+ typeDef match {
+ case _ if hasRepType => {
+ // this length is only used for the length of the representation. The
+ // values used for facet restrictions during limited validation come
from elsewhere
+ repTypeElementDecl.length
+ }
+ case prim: PrimitiveType => {
+ val pt = prim.primType
+ schemaDefinitionWhen(
+ (pt == PrimType.String || pt == PrimType.HexBinary) && lengthKind ==
LengthKind.Implicit,
+ "Facet length must be defined for type %s with
lengthKind='implicit'",
+ pt.name,
+ )
+ //
+ // We handle text numbers by getting a stringValue first, then
+ // we convert to the number type.
+ //
+ // This means we cannot check and SDE here on incorrect simple type.
+ zeroBD
+ }
+ case st: SimpleTypeDefBase if st.optRestriction.isDefined => {
+ val r = st.optRestriction.get
+ val pt = st.primType
+ val typeOK = pt == PrimType.String || pt == PrimType.HexBinary
+ schemaDefinitionWhen(
+ !typeOK && hasLength,
+ "Facet length is not allowed on types derived from type %s.\nIt is
allowed only on types derived from string and hexBinary.",
+ pt.name,
+ )
+ val res = (hasLength, lengthKind) match {
+ case (false, LengthKind.Implicit) =>
+ SDE(
+ "When lengthKind='implicit', length facet must be specified.",
+ )
+ case (true, _) => r.lengthValue
+ case (false, _) => zeroBD
+ case _ => Assert.impossible()
+ }
+ res
+ }
+ case st: SimpleTypeDefBase => {
+ Assert.invariant(st.optRestriction.isEmpty)
+ zeroBD
+ }
+ }
+ }
+
/**
* Compute minLength and maxLength together to share error-checking
* and case dispatch that would otherwise have to be repeated.
+ *
+ * Also set them to the value of length, in the case we've used the length
facet
*/
- final lazy val (minLength: java.math.BigDecimal, maxLength:
java.math.BigDecimal) =
- computeMinMaxLength
+ final lazy val (minLength: java.math.BigDecimal, maxLength:
java.math.BigDecimal) = {
+ // if hasLength is true, and min/maxLength is queried, set it to length
+ if (hasLength) {
+ (length, length)
+ } else {
+ computeMinMaxLength
+ }
+ }
Review Comment:
This condition goes away if we can merge length into computeMinMaxLength, so
it simplifies the code a bit.
##########
daffodil-runtime1/src/main/scala/org/apache/daffodil/runtime1/processors/RuntimeData.scala:
##########
@@ -407,7 +416,32 @@ final class SimpleTypeRuntimeData(
// Note: dont check occurs counts // if(!checkMinMaxOccurs(e,
pstate.arrayIterationPos)) { return java.lang.Boolean.FALSE }
OK
}
+ private def checkLength(
+ diNode: DISimple,
+ lenValue: java.math.BigDecimal,
+ e: ThrowsSDE,
+ primType: PrimType,
+ ): java.lang.Boolean = {
+ val lenAsLong = lenValue.longValueExact()
+ primType match {
+ case PrimType.String => {
+ val data = diNode.dataValue.getString
+ val dataLen = data.length.toLong
+ val isDataLengthEqual = dataLen.compareTo(lenAsLong) == 0
+ if (isDataLengthEqual) java.lang.Boolean.TRUE
+ else java.lang.Boolean.FALSE
+ }
+ case PrimType.HexBinary => {
+ val data = diNode.dataValue.getByteArray
+ val dataLen = data.length.toLong
+ val isDataLengthEqual = dataLen.compareTo(lenAsLong) == 0
Review Comment:
Should length compare against the actual number of bytes, or the number of
chars in the string representation of the infoset?
Note that min/maxLength have the exact same logic, but shouldn't they have
less-than or greather-than? I wonder if those are unrelated bugs?
##########
daffodil-core/src/main/scala/org/apache/daffodil/core/dsom/ElementBase.scala:
##########
@@ -711,8 +712,8 @@ trait ElementBase
Assert.invariant(repElement.lengthKind =:= LengthKind.Implicit)
// it's a string with implicit length. get from facets
schemaDefinitionUnless(
- repElement.hasMaxLength,
- "String with dfdl:lengthKind='implicit' must have an XSD maxLength
facet value.",
+ repElement.hasMaxLength || repElement.hasLength,
+ "String with dfdl:lengthKind='implicit' must have an XSD maxLength or
XSD Length facet value.",
Review Comment:
Make `XSD Length` a lowercase `l`
##########
daffodil-runtime1/src/main/scala/org/apache/daffodil/runtime1/processors/RuntimeData.scala:
##########
@@ -345,7 +346,15 @@ final class SimpleTypeRuntimeData(
return Error("facet enumeration(s):
%s".format(e.enumerationValues.mkString(",")))
}
}
-
+ // Check length
+ e.length.foreach { length =>
+ val lenAsLong = length.longValue()
+ val isLengthGreaterThanEqToZero = lenAsLong.compareTo(0L) >= 0
+ if (isLengthGreaterThanEqToZero) {
Review Comment:
I don't really understand why we do a length >= 0 check. I see it's just
copied from min/maxLength, but seems like that should never happen and should
be caught during complication? length, minLength, and maxLength can never be
negative.
##########
daffodil-core/src/main/scala/org/apache/daffodil/core/dsom/Facets.scala:
##########
@@ -387,7 +413,7 @@ trait Facets { self: Restriction =>
errorOnLocalLessThanBaseFacet(theLocalFacet, theRemoteFacet, facetType)
localFacet
}
- case Facet.maxLength | Facet.fractionDigits => {
+ case Facet.length | Facet.maxLength | Facet.fractionDigits => {
errorOnLocalGreaterThanBaseFacet(theLocalFacet, theRemoteFacet,
facetType)
Review Comment:
This means that a local length facet must be less than the base length
facet, is that correct? Have we confirmed with the XSD spec that that is the
right behavior? Feels like make a local length should always be the same as a
base length, since there is no way to narrow a single value instead of a range
like min/maxValue have.
##########
daffodil-core/src/main/scala/org/apache/daffodil/core/dsom/ElementBase.scala:
##########
@@ -974,7 +1037,7 @@ trait ElementBase
private def computeMinMaxLength: (java.math.BigDecimal,
java.math.BigDecimal) = {
schemaDefinitionUnless(
isSimpleType,
- "Facets minLength and maxLength are allowed only on types string and
hexBinary.",
+ "Facets minLength and maxLength are allowed only on simple types or
types derived from simple types.",
Review Comment:
Is this correct, do we allow min/maxLength on numeric types? I wonder if the
message was correct but the check was wrong?
##########
daffodil-core/src/main/scala/org/apache/daffodil/core/grammar/ElementBaseGrammarMixin.scala:
##########
@@ -1616,29 +1616,33 @@ trait ElementBaseGrammarMixin
* the type is a type that respects minLength and maxLength, and the
constant length
* is not in range.
*/
- val isTypeUsingMinMaxLengthFacets = typeDef.typeNode match {
+ val isTypeUsingLengthOrMinMaxLengthFacets = typeDef.typeNode match {
case s: NodeInfo.String.Kind => true
case s: NodeInfo.HexBinary.Kind => true
case _ => false
}
if (
(lengthKind eq LengthKind.Explicit) &&
- isTypeUsingMinMaxLengthFacets &&
+ isTypeUsingLengthOrMinMaxLengthFacets &&
optLengthConstant.isDefined
) {
val len = optLengthConstant.get
- val maxLengthLong = maxLength.longValueExact
- val minLengthLong = minLength.longValueExact
+ lazy val maxLengthLong = maxLength.longValueExact
+ lazy val minLengthLong = minLength.longValueExact
+ lazy val lengthLong = length.longValueExact
def warn(m: String, value: Long): Unit = SDW(
WarnID.FacetExplicitLengthOutOfRange,
- "Explicit dfdl:length of %s is out of range for facet %sLength='%s'.",
+ "Explicit dfdl:length of %s is out of range for facet %sength='%s'.",
len,
m,
value,
)
- if (maxLengthLong != -1 && len > maxLengthLong) warn("max",
maxLengthLong)
- Assert.invariant(minLengthLong >= 0)
- if (minLengthLong > 0 && len < minLengthLong) warn("min", minLengthLong)
+ if (hasLength && len != lengthLong) warn("l", lengthLong)
+ else if (hasMinLength || hasMaxLength) {
+ if (maxLengthLong != -1 && len > maxLengthLong) warn("maxL",
maxLengthLong)
+ Assert.invariant(minLengthLong >= 0)
+ if (minLengthLong > 0 && len < minLengthLong) warn("minL",
minLengthLong)
Review Comment:
Suggest we just pass in the string "minLength", "maxLength" and "length",
the above `%sength` is kindof hard to parse and only saves us a few characters.
--
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]
