You are viewing a plain text version of this content. The canonical link for it is here.
Posted to commits@daffodil.apache.org by GitBox <gi...@apache.org> on 2020/11/20 18:25:09 UTC
[GitHub] [incubator-daffodil] stevedlawrence opened a new pull request #460: Add validation support for tunables
stevedlawrence opened a new pull request #460:
URL: https://github.com/apache/incubator-daffodil/pull/460
- Adds xs:restrictions to tunables for which there is only a valid range
within the range of the primitive type. Modifies the tunable generator
to convert these restrictions to code checks and throw an
IllegalArgumentException if outside the valid range.
- Also sorts the tunable by name so they are organized.
DAFFODIL-2432
----------------------------------------------------------------
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [incubator-daffodil] stevedlawrence commented on pull request #460: Add validation support for tunables
Posted by GitBox <gi...@apache.org>.
stevedlawrence commented on pull request #460:
URL: https://github.com/apache/incubator-daffodil/pull/460#issuecomment-735946836
I've added a new commit to add additional validation so that we get more helpful errors if the schema doesn't follow our strict requirements.
This found one tunable that didn't have a default because the default was hardcoded. I changed it so the default in the XSD is the actual java code so that it the tunable has a default, but I'm not sure I like that. This means all properties have a default and we atually use it, but it feels wrong having java code in the XSD. Thoughts?
```diff
- new FileTunable("tempFilePath", """System.getProperty("java.io.tmpdir")""")
+ new FileTunable("tempFilePath", schemaDefault)
```
```diff
- <xs:element name="tempFilePath" type="xs:string" default="" minOccurs="0">
+ <xs:element name="tempFilePath" type="xs:string" default="System.getProperty("java.io.tmpdir")" minOccurs="0">
```
----------------------------------------------------------------
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [incubator-daffodil] mbeckerle commented on pull request #460: Add validation support for tunables
Posted by GitBox <gi...@apache.org>.
mbeckerle commented on pull request #460:
URL: https://github.com/apache/incubator-daffodil/pull/460#issuecomment-735961341
So this thunk of Java code (or is it scala code) gets somehow executed?
That has to be an exceptional case in the generator anyway, so why not just put the code there, and document this in the XSD by giving this string a value like "This string is ignored. Value is taken from java.io.tmpdir property" .
We probably don't want that to be generally done for more tunables. So I'd rather take this out of the XSD, document that this is hard-coded.
----------------------------------------------------------------
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [incubator-daffodil] stevedlawrence merged pull request #460: Add validation support for tunables
Posted by GitBox <gi...@apache.org>.
stevedlawrence merged pull request #460:
URL: https://github.com/apache/incubator-daffodil/pull/460
----------------------------------------------------------------
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [incubator-daffodil] tuxji commented on a change in pull request #460: Add validation support for tunables
Posted by GitBox <gi...@apache.org>.
tuxji commented on a change in pull request #460:
URL: https://github.com/apache/incubator-daffodil/pull/460#discussion_r532626166
##########
File path: daffodil-propgen/src/main/resources/org/apache/daffodil/xsd/dafext.xsd
##########
@@ -401,6 +471,11 @@
frequency of context switching, but increase the memory footprint.
</xs:documentation>
</xs:annotation>
+ <xs:simpleType>
Review comment:
Adding a comment that the tunable generator needs dafext.xsd to be easily parseable (with some explanation what the certain form needs to be) is fine with me.
----------------------------------------------------------------
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [incubator-daffodil] mbeckerle commented on a change in pull request #460: Add validation support for tunables
Posted by GitBox <gi...@apache.org>.
mbeckerle commented on a change in pull request #460:
URL: https://github.com/apache/incubator-daffodil/pull/460#discussion_r527998232
##########
File path: daffodil-propgen/src/main/resources/org/apache/daffodil/xsd/dafext.xsd
##########
@@ -401,6 +471,11 @@
frequency of context switching, but increase the memory footprint.
</xs:documentation>
</xs:annotation>
+ <xs:simpleType>
Review comment:
A defined simple type daf:OneOrGreater or some such would be more compact here as these would all remain one-liners with a type="daf:oneOrGreater" attribute.
##########
File path: daffodil-propgen/src/main/scala/org/apache/daffodil/propGen/TunableGenerator.scala
##########
@@ -197,16 +224,53 @@ class PrimitiveTunable(name: String, schemaType: String, schemaDefault: String)
case _ => schemaDefault
}
+ private def restrictionCheck(rCheck: String, rValue: String): Option[String] = {
+ if (rValue != "") {
+ Some(s""" if (!(v ${rCheck} ${rValue})) throwInvalidTunableValue(tunable, value)""")
+ } else {
+ None
+ }
+ }
+
+ private val minInclusive = node \\ "minInclusive" \@ "value"
Review comment:
So it appears this is depending on the XML schema of the tunables being structured not as just an XML schema, but as one that always uses immediate simple types definitions having exactly these attributes.
If so, you need comments in the tunables.xsd file because looking at it, first thing I'd do is factor out all the redundant type definitions which would break this scraper/generator code.
##########
File path: daffodil-propgen/src/main/resources/org/apache/daffodil/xsd/dafext.xsd
##########
@@ -110,55 +110,39 @@
</xs:documentation>
</xs:annotation>
</xs:element>
- <xs:element name="blobChunkSizeInBytes" type="xs:int" default="4096" minOccurs="0">
+ <xs:element name="blobChunkSizeInBytes" default="4096" minOccurs="0">
<xs:annotation>
<xs:documentation>
When reading/writing blob data, the maximum number of bytes to read/write at a time.
</xs:documentation>
</xs:annotation>
+ <xs:simpleType>
Review comment:
Your scraper generator depends on this being an immediate simpletype with a restriction with min/max/exclusive/inclusive in it manifestly.
You need to document that here, because changing this to say type="daf:oneOrGreater" with that defined elsewhere would break this I think.
----------------------------------------------------------------
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [incubator-daffodil] mbeckerle commented on a change in pull request #460: Add validation support for tunables
Posted by GitBox <gi...@apache.org>.
mbeckerle commented on a change in pull request #460:
URL: https://github.com/apache/incubator-daffodil/pull/460#discussion_r532775672
##########
File path: daffodil-propgen/src/main/scala/org/apache/daffodil/propGen/TunableGenerator.scala
##########
@@ -134,7 +138,9 @@ class TunableGenerator(schemaRootConfig: scala.xml.Node, schemaRootExt: scala.xm
val tunables =
tunableNodes.map { tunableNode =>
val schemaName = tunableNode \@ "name"
- val schemaType = tunableNode \@ "type"
+ val schemaType =
+ if (tunableNode \@ "type" != "") tunableNode \@ "type"
+ else tunableNode \\ "restriction" \@ "base"
Review comment:
This code is not going to give any sort of useful diagnostic if the XSD just happens to have constructs that aren't recognized in it.
Add a check here if there is no type attribute, then there must be a restriction, or throw a useful diagnostic stating that these are the only allowed options.
----------------------------------------------------------------
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [incubator-daffodil] stevedlawrence commented on a change in pull request #460: Add validation support for tunables
Posted by GitBox <gi...@apache.org>.
stevedlawrence commented on a change in pull request #460:
URL: https://github.com/apache/incubator-daffodil/pull/460#discussion_r532634956
##########
File path: daffodil-propgen/src/main/resources/org/apache/daffodil/xsd/dafext.xsd
##########
@@ -265,28 +302,48 @@
tunable tunable sets an upper limit for values to prevent overflow.
</xs:documentation>
</xs:annotation>
+ <xs:simpleType>
+ <xs:restriction base="xs:int">
+ <xs:minInclusive value="1" />
+ </xs:restriction>
+ </xs:simpleType>
</xs:element>
- <xs:element name="maximumRegexMatchLengthInCharacters" type="xs:int" default="1048576" minOccurs="0">
+ <xs:element name="maximumRegexMatchLengthInCharacters" default="1048576" minOccurs="0">
<xs:annotation>
<xs:documentation>
Maximum number of characters to match when performing regular expression
matches on input data.
</xs:documentation>
</xs:annotation>
+ <xs:simpleType>
+ <xs:restriction base="xs:int">
+ <xs:minInclusive value="1" />
+ </xs:restriction>
+ </xs:simpleType>
</xs:element>
- <xs:element name="maximumSimpleElementSizeInCharacters" type="xs:int" default="1048576" minOccurs="0">
+ <xs:element name="maximumSimpleElementSizeInCharacters" default="1048576" minOccurs="0">
<xs:annotation>
<xs:documentation>
Maximum number of characters to parse when parsing string data.
</xs:documentation>
</xs:annotation>
+ <xs:simpleType>
+ <xs:restriction base="xs:int">
+ <xs:minInclusive value="1" />
+ </xs:restriction>
+ </xs:simpleType>
</xs:element>
- <xs:element name="minBinaryDecimalVirtualPoint" type="xs:int" default="-200" minOccurs="0">
+ <xs:element name="minBinaryDecimalVirtualPoint" default="-200" minOccurs="0">
<xs:annotation>
<xs:documentation>
The smallest allowed value of the dfdl:binaryDecimalVirtualPoint property.
</xs:documentation>
</xs:annotation>
+ <xs:simpleType>
+ <xs:restriction base="xs:int">
+ <xs:maxInclusive value="-1" />
+ </xs:restriction>
+ </xs:simpleType>
</xs:element>
Review comment:
The min/maxBinaryDecimalVirtualPoint tunables describe the smallest and largest allowed values of the dfdl:binaryDecimalVirtualPoint property. The value of this property is essentially used by doing:
```scala
val bigDec = new JBigDecimal(bigInt, binaryDecimalVirtualPoint)
```
I believe we ran into a bug where very large and very small values of binaryDecimalVirtualPoint didn't work as expected so we put in placed an arbitrary limit of -200 to 200. And I think the tunable was added as a way to easily change this arbitrary limit if needed.
My thinking with min/maxBinaryDecimalVirtual point was that minBinaryDecimalVirtualPoint should always be negative so has a max value of -1. And maxBinaryDecimalVirtualPoint should be positive, so has a min value of 1.
----------------------------------------------------------------
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [incubator-daffodil] stevedlawrence commented on pull request #460: Add validation support for tunables
Posted by GitBox <gi...@apache.org>.
stevedlawrence commented on pull request #460:
URL: https://github.com/apache/incubator-daffodil/pull/460#issuecomment-735968964
That's a much better solution. Fixup commit added.
----------------------------------------------------------------
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [incubator-daffodil] stevedlawrence commented on a change in pull request #460: Add validation support for tunables
Posted by GitBox <gi...@apache.org>.
stevedlawrence commented on a change in pull request #460:
URL: https://github.com/apache/incubator-daffodil/pull/460#discussion_r532615540
##########
File path: daffodil-propgen/src/main/resources/org/apache/daffodil/xsd/dafext.xsd
##########
@@ -401,6 +471,11 @@
frequency of context switching, but increase the memory footprint.
</xs:documentation>
</xs:annotation>
+ <xs:simpleType>
Review comment:
My only concern here, and why I went this route, is it makes the generator quite a bit more complicated. It doesn't currently have any logic to find other types or support restrictions not defined directly as a child of the tunable. It's still pretty dumb. I'd prefer that we just require this schema be verbose in favor a being easily parsable. There's plenty of ways one might want to refactor this file (e.g. imports/includes, unions), that I don't really want to have to support. I'd rather add comments in the schema file that specifiy that the generator expects a certain form. Thoughts?
----------------------------------------------------------------
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [incubator-daffodil] stevedlawrence commented on a change in pull request #460: Add validation support for tunables
Posted by GitBox <gi...@apache.org>.
stevedlawrence commented on a change in pull request #460:
URL: https://github.com/apache/incubator-daffodil/pull/460#discussion_r532625868
##########
File path: daffodil-propgen/src/main/resources/org/apache/daffodil/xsd/dafext.xsd
##########
@@ -200,20 +194,44 @@
</xs:documentation>
</xs:annotation>
</xs:element>
- <xs:element name="maxBinaryDecimalVirtualPoint" type="xs:int" default="200" minOccurs="0">
+ <xs:element name="maxBinaryDecimalVirtualPoint" default="200" minOccurs="0">
<xs:annotation>
<xs:documentation>
The largest allowed value of the dfdl:binaryDecimalVirtualPoint property.
</xs:documentation>
</xs:annotation>
+ <xs:simpleType>
+ <xs:restriction base="xs:int">
+ <xs:minInclusive value="1" />
+ </xs:restriction>
+ </xs:simpleType>
+ </xs:element>
+ <xs:element name="maxByteArrayOutputStreamBufferSizeInBytes" default="2097152000" minOccurs="0">
+ <xs:annotation>
+ <xs:documentation>
+ When unparsing, this is the maximum size of the buffer that the
+ ByteArrayOutputStream can grow to before switching to a file based
+ output stream.
+ </xs:documentation>
+ </xs:annotation>
+ <xs:simpleType>
+ <xs:restriction base="xs:int">
+ <xs:minInclusive value="0" />
+ </xs:restriction>
+ </xs:simpleType>
Review comment:
I think this field actually must have a max size of IntMax, so using a long was incorrect.
This tunable sets the max size for an unparse buffer before we switch the buffer from memory based via a ByteArrayOutputStream to file based via a FileOutputStream. When the buffer is backed by a ByteArrayOutputStream, the max length must be IntMax since byte arrays can't be larger than an int. If we allowed this value to be a long, we could overfill the ByteArrayOutputStream instead of switching to a FileOutputStream and I think an exception will be thrown.
I think zero is also a valid value for this tunable. When this value is zero, it just means that we'll always use a FileOutputStream for buffers and never use an in-memory byte array. Perhaps this could be useful on a system with extremely limited memory?
----------------------------------------------------------------
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [incubator-daffodil] stevedlawrence commented on a change in pull request #460: Add validation support for tunables
Posted by GitBox <gi...@apache.org>.
stevedlawrence commented on a change in pull request #460:
URL: https://github.com/apache/incubator-daffodil/pull/460#discussion_r532644677
##########
File path: daffodil-propgen/src/main/resources/org/apache/daffodil/xsd/dafext.xsd
##########
@@ -401,6 +471,11 @@
frequency of context switching, but increase the memory footprint.
</xs:documentation>
</xs:annotation>
+ <xs:simpleType>
Review comment:
I've added a new comment that describes the required format.
----------------------------------------------------------------
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [incubator-daffodil] tuxji commented on a change in pull request #460: Add validation support for tunables
Posted by GitBox <gi...@apache.org>.
tuxji commented on a change in pull request #460:
URL: https://github.com/apache/incubator-daffodil/pull/460#discussion_r528987021
##########
File path: daffodil-propgen/src/main/resources/org/apache/daffodil/xsd/dafext.xsd
##########
@@ -200,20 +194,44 @@
</xs:documentation>
</xs:annotation>
</xs:element>
- <xs:element name="maxBinaryDecimalVirtualPoint" type="xs:int" default="200" minOccurs="0">
+ <xs:element name="maxBinaryDecimalVirtualPoint" default="200" minOccurs="0">
<xs:annotation>
<xs:documentation>
The largest allowed value of the dfdl:binaryDecimalVirtualPoint property.
</xs:documentation>
</xs:annotation>
+ <xs:simpleType>
+ <xs:restriction base="xs:int">
+ <xs:minInclusive value="1" />
+ </xs:restriction>
+ </xs:simpleType>
+ </xs:element>
+ <xs:element name="maxByteArrayOutputStreamBufferSizeInBytes" default="2097152000" minOccurs="0">
+ <xs:annotation>
+ <xs:documentation>
+ When unparsing, this is the maximum size of the buffer that the
+ ByteArrayOutputStream can grow to before switching to a file based
+ output stream.
+ </xs:documentation>
+ </xs:annotation>
+ <xs:simpleType>
+ <xs:restriction base="xs:int">
+ <xs:minInclusive value="0" />
+ </xs:restriction>
+ </xs:simpleType>
Review comment:
Noticed this `maxByteArrayOutputStreamBufferSizeInBytes` tunable has changed from xs:long to xs:int and it can go down to zero.
##########
File path: daffodil-propgen/src/main/resources/org/apache/daffodil/xsd/dafext.xsd
##########
@@ -401,6 +471,11 @@
frequency of context switching, but increase the memory footprint.
</xs:documentation>
</xs:annotation>
+ <xs:simpleType>
Review comment:
Yes, that's true. Most of the `xs:int` tunables are oneOrGreater types.
##########
File path: daffodil-propgen/src/main/resources/org/apache/daffodil/xsd/dafext.xsd
##########
@@ -265,28 +302,48 @@
tunable tunable sets an upper limit for values to prevent overflow.
</xs:documentation>
</xs:annotation>
+ <xs:simpleType>
+ <xs:restriction base="xs:int">
+ <xs:minInclusive value="1" />
+ </xs:restriction>
+ </xs:simpleType>
</xs:element>
- <xs:element name="maximumRegexMatchLengthInCharacters" type="xs:int" default="1048576" minOccurs="0">
+ <xs:element name="maximumRegexMatchLengthInCharacters" default="1048576" minOccurs="0">
<xs:annotation>
<xs:documentation>
Maximum number of characters to match when performing regular expression
matches on input data.
</xs:documentation>
</xs:annotation>
+ <xs:simpleType>
+ <xs:restriction base="xs:int">
+ <xs:minInclusive value="1" />
+ </xs:restriction>
+ </xs:simpleType>
</xs:element>
- <xs:element name="maximumSimpleElementSizeInCharacters" type="xs:int" default="1048576" minOccurs="0">
+ <xs:element name="maximumSimpleElementSizeInCharacters" default="1048576" minOccurs="0">
<xs:annotation>
<xs:documentation>
Maximum number of characters to parse when parsing string data.
</xs:documentation>
</xs:annotation>
+ <xs:simpleType>
+ <xs:restriction base="xs:int">
+ <xs:minInclusive value="1" />
+ </xs:restriction>
+ </xs:simpleType>
</xs:element>
- <xs:element name="minBinaryDecimalVirtualPoint" type="xs:int" default="-200" minOccurs="0">
+ <xs:element name="minBinaryDecimalVirtualPoint" default="-200" minOccurs="0">
<xs:annotation>
<xs:documentation>
The smallest allowed value of the dfdl:binaryDecimalVirtualPoint property.
</xs:documentation>
</xs:annotation>
+ <xs:simpleType>
+ <xs:restriction base="xs:int">
+ <xs:maxInclusive value="-1" />
+ </xs:restriction>
+ </xs:simpleType>
</xs:element>
Review comment:
The `minBinaryDecimalVirtualPoint` tunable has a default value of -200 and a maximum value of -1. Interesting.
----------------------------------------------------------------
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org