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 2021/09/02 12:24:42 UTC
[GitHub] [daffodil] stevedlawrence opened a new pull request #629: Add dfdlx:runtimeProperties extension
stevedlawrence opened a new pull request #629:
URL: https://github.com/apache/daffodil/pull/629
New dfdlx:runtimeProperties is a space-separated list of key=value
pairs. The pairs are converted to a Map and made available on the
ElementRuntimeData for which the properties is specified. This Map can
be accessed by custom implementations of InfosetInputters and
InfosetOutputters to allow modifications and custom behaviors to the
text of simple types.
The property is only valid on dfdl:element and xs:element tags for
simple types.
Also changed "Object" to "ObjectKind" to make it more clear that it
describes the dfdlx:objectKind extension property.
DAFFODIL-2537
--
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
[GitHub] [daffodil] tuxji commented on a change in pull request #629: Add dfdlx:runtimeProperties extension
Posted by GitBox <gi...@apache.org>.
tuxji commented on a change in pull request #629:
URL: https://github.com/apache/daffodil/pull/629#discussion_r703488550
##########
File path: daffodil-japi/src/main/scala/org/apache/daffodil/japi/infoset/Infoset.scala
##########
@@ -67,13 +70,29 @@ abstract class InfosetInputter extends SInfosetInputter {
*/
def getNamespaceURI(): String
+ /**
+ * Wrapper function to convert the runtimeProperties Scala Map to a Java Map
+ * and call the getSimpleText function. Proably should not be overridden by
+ * those using the Java API. Instead override getSimpleText(primType, runtimeProperties)
+ * or getSimpleText(primType)
+ */
+ override def getSimpleText(primType: NodeInfo.Kind, runtimeProperties: Map[String,String]): String =
+ getSimpleText(primType, runtimeProperties.asJava)
Review comment:
`asJava`'s new `MapWrapper` allocation occurs only if a Java API user calls this specific two-arg `getSimpleText` method, correct? There's a third choice worth adding to the previous two choices then:
1. Create Java Map instead of Scala Map in ElementBase.scala.
2. Continue using Scala Map, calling `asJava` and creating wrapper in this method.
3. Check for empty Scala Map in this method and if so, call Java Collections.emptyMap() instead of calling `asJava` and creating wrapper. This choice is more efficient because it doesn't create a new wrapper object; it just re-uses an existing empty and immutable object.
I think the third choice might be the best compromise.
--
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
[GitHub] [daffodil] tuxji commented on a change in pull request #629: Add dfdlx:runtimeProperties extension
Posted by GitBox <gi...@apache.org>.
tuxji commented on a change in pull request #629:
URL: https://github.com/apache/daffodil/pull/629#discussion_r703488550
##########
File path: daffodil-japi/src/main/scala/org/apache/daffodil/japi/infoset/Infoset.scala
##########
@@ -67,13 +70,29 @@ abstract class InfosetInputter extends SInfosetInputter {
*/
def getNamespaceURI(): String
+ /**
+ * Wrapper function to convert the runtimeProperties Scala Map to a Java Map
+ * and call the getSimpleText function. Proably should not be overridden by
+ * those using the Java API. Instead override getSimpleText(primType, runtimeProperties)
+ * or getSimpleText(primType)
+ */
+ override def getSimpleText(primType: NodeInfo.Kind, runtimeProperties: Map[String,String]): String =
+ getSimpleText(primType, runtimeProperties.asJava)
Review comment:
`asJava`'s new `MapWrapper` allocation occurs only if a Java API user calls this specific two-arg `getSimpleText` method, correct? There's a third choice worth adding to the previous two choices then:
1. Create Java Map instead of Scala Map in ElementBase.scala.
2. Continue using Scala Map, calling `asJava` and creating wrapper in this method.
3. Check for empty Scala Map in this method and if so, call Java `Collections.emptyMap()` instead of calling `asJava` and creating wrapper. This choice is more efficient because it doesn't create a new wrapper object; it just re-uses an existing empty and immutable object.
I think the third choice might be the best compromise.
--
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
[GitHub] [daffodil] mbeckerle commented on a change in pull request #629: Add dfdlx:runtimeProperties extension
Posted by GitBox <gi...@apache.org>.
mbeckerle commented on a change in pull request #629:
URL: https://github.com/apache/daffodil/pull/629#discussion_r701351158
##########
File path: daffodil-core/src/test/scala/org/apache/daffodil/general/TestRuntimeProperties.scala
##########
@@ -0,0 +1,187 @@
+/*
+ * 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.general
+
+import org.junit.Test
+import org.junit.Assert.assertEquals
+import org.junit.Assert.assertFalse
+import org.junit.Assert.assertTrue
+
+import org.apache.daffodil.Implicits.intercept
+import org.apache.daffodil.dpath.NodeInfo
+import org.apache.daffodil.infoset.DISimple
+import org.apache.daffodil.infoset.ScalaXMLInfosetInputter
+import org.apache.daffodil.infoset.ScalaXMLInfosetOutputter
+import org.apache.daffodil.io.InputSourceDataInputStream
+import org.apache.daffodil.util.SchemaUtils
+import org.apache.daffodil.util.TestUtils
+
+/**
+ * Test InfosetOutputter that extends the ScalaXMLInfosetOutputter and redacts
+ * words specified in the dfdlx:runtimeProperties extension
Review comment:
Ah, so in that case there's no need for the tests that address any interactions with validation, as there is none.
--
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
[GitHub] [daffodil] stevedlawrence commented on a change in pull request #629: Add dfdlx:runtimeProperties extension
Posted by GitBox <gi...@apache.org>.
stevedlawrence commented on a change in pull request #629:
URL: https://github.com/apache/daffodil/pull/629#discussion_r701195644
##########
File path: daffodil-runtime1/src/main/scala/org/apache/daffodil/processors/RuntimeData.scala
##########
@@ -697,7 +698,8 @@ sealed abstract class ErrorERD(local: String, namespaceURI: String)
null, // fillByteEvArg => FillByteEv
Nope, // maybeCheckByteAndBitOrderEvArg: => Maybe[CheckByteAndBitOrderEv],
Nope, // maybeCheckBitOrderAndCharsetEvArg: => Maybe[CheckBitOrderAndCharsetEv],
- false // isQuasiElementArg: => Boolean
+ false, // isQuasiElementArg: => Boolean
+ null, // runtimeProperties: Map[String,String]
Review comment:
I almost wonder if this is a good thing error? Fields in this ErrorERD should never be used, so having null values would ensure if they were used we could get a NPE and it would be very obvious something was using this when it shouldn't be. If we have valid empty values, we might not realize that some code is using it.
--
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
[GitHub] [daffodil] tuxji commented on a change in pull request #629: Add dfdlx:runtimeProperties extension
Posted by GitBox <gi...@apache.org>.
tuxji commented on a change in pull request #629:
URL: https://github.com/apache/daffodil/pull/629#discussion_r701217848
##########
File path: daffodil-runtime1/src/main/scala/org/apache/daffodil/infoset/InfosetInputter.scala
##########
@@ -121,6 +121,13 @@ abstract class InfosetInputter
* NonTextFoundInSimpleContentException. If the element does not have any
* simple content, this should return either null or the empty string.
*/
+ def getSimpleText(primType: NodeInfo.Kind, runtimeProperties: Map[String,String]): String =
+ getSimpleText(primType)
+
+ /**
+ * See getSimpleText(primType, runtimeProperties), which has a default
+ * implementation to call this function without the runtimeProperties Map
+ */
def getSimpleText(primType: NodeInfo.Kind): String
Review comment:
I'm fine with changing the internal InfosetInputter API and all the calls to use two arguments (no default parameter needed or used), although that's more work for you. For backwards compatibility, I agree the public Java/Scala API InfosetInputters still need to use overloaded one-arg and two-arg functions (Java) or a default parameter (Scala), although a default parameter still breaks binary compatibility but not source compatibility (if that matters).
Did you see my questions about the asciidoc design document and the custom XML InfosetInputter/Outputter for embedded XML strings in binary data too?
--
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
[GitHub] [daffodil] tuxji commented on a change in pull request #629: Add dfdlx:runtimeProperties extension
Posted by GitBox <gi...@apache.org>.
tuxji commented on a change in pull request #629:
URL: https://github.com/apache/daffodil/pull/629#discussion_r701102938
##########
File path: daffodil-japi/src/main/scala/org/apache/daffodil/japi/infoset/Infoset.scala
##########
@@ -67,13 +70,29 @@ abstract class InfosetInputter extends SInfosetInputter {
*/
def getNamespaceURI(): String
+ /**
+ * Wrapper function to convert the runtimeProperties Scala Map to a Java Map
+ * and call the getSimpleText function. Proably should not be overridden by
+ * those using the Java API. Instead override getSimpleText(primType, runtimeProperties)
+ * or getSimpleText(primType)
+ */
+ override def getSimpleText(primType: NodeInfo.Kind, runtimeProperties: Map[String,String]): String =
+ getSimpleText(primType, runtimeProperties.asJava)
Review comment:
Codecov reports that the new japi/sapi members aren't covered by tests....
##########
File path: daffodil-runtime1/src/main/scala/org/apache/daffodil/processors/RuntimeData.scala
##########
@@ -697,7 +698,8 @@ sealed abstract class ErrorERD(local: String, namespaceURI: String)
null, // fillByteEvArg => FillByteEv
Nope, // maybeCheckByteAndBitOrderEvArg: => Maybe[CheckByteAndBitOrderEv],
Nope, // maybeCheckBitOrderAndCharsetEvArg: => Maybe[CheckBitOrderAndCharsetEv],
- false // isQuasiElementArg: => Boolean
+ false, // isQuasiElementArg: => Boolean
+ null, // runtimeProperties: Map[String,String]
Review comment:
Even though this is only an `ErrorERD`, we might be safer passing `Map.empty` rather than null so that an `InfosetInputter ` doesn't end up calling `getSimpleText` with null as the second argument.
##########
File path: daffodil-core/src/main/scala/org/apache/daffodil/dsom/ElementBase.scala
##########
@@ -1179,4 +1179,17 @@ trait ElementBase
Nil
res
}
+
+ lazy val runtimeProperties = findPropertyOption("runtimeProperties").toOption match {
Review comment:
Even though trait ElementBase already has so many public/protected/private members mixed together that it's hard to understand its data structure/behavior (reordering by public/protected/private or data/behavior might help), I want to see this public member documented better. Please add a scaladoc comment like "This Map can be accessed by custom implementations of InfosetInputters and InfosetOutputters to allow modifications and custom behaviors to the text of simple types."
##########
File path: daffodil-runtime1/src/main/scala/org/apache/daffodil/infoset/InfosetInputter.scala
##########
@@ -121,6 +121,13 @@ abstract class InfosetInputter
* NonTextFoundInSimpleContentException. If the element does not have any
* simple content, this should return either null or the empty string.
*/
+ def getSimpleText(primType: NodeInfo.Kind, runtimeProperties: Map[String,String]): String =
+ getSimpleText(primType)
+
+ /**
+ * See getSimpleText(primType, runtimeProperties), which has a default
+ * implementation to call this function without the runtimeProperties Map
+ */
def getSimpleText(primType: NodeInfo.Kind): String
Review comment:
I question the need to overload `getSimpleText` with one and two parameters. Wouldn't it be easier to add a default argument to getSimpleText instead?
```scala
def getSimpleText(primType: NodeInfo.Kind, runtimeProperties: Map[String,String] = Map.empty[String, String]): String
```
Passing `Map.empty` as a default argument removes the need to worry about handling `null` or `Option` or auto-converting `Map` to `Option[Map]` (see StackOverflow on [optional parameters](https://stackoverflow.com/questions/4199393/are-options-and-named-default-arguments-like-oil-and-water-in-a-scala-api)).
The corresponding Java API still would need to overload `getSimpleText` with one and two parameters since Java doesn't have default parameters, but you still could simplify the corresponding Scala API the same way.
--
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
[GitHub] [daffodil] stevedlawrence commented on pull request #629: Add dfdlx:runtimeProperties extension
Posted by GitBox <gi...@apache.org>.
stevedlawrence commented on pull request #629:
URL: https://github.com/apache/daffodil/pull/629#issuecomment-916377711
A writeup of these feature, including a potential implementation using Scala XML Nodes for simplicity and brevity is added here:
https://cwiki.apache.org/confluence/display/DAFFODIL/Proposal%3A+Runtime+Properties
--
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
[GitHub] [daffodil] stevedlawrence commented on a change in pull request #629: Add dfdlx:runtimeProperties extension
Posted by GitBox <gi...@apache.org>.
stevedlawrence commented on a change in pull request #629:
URL: https://github.com/apache/daffodil/pull/629#discussion_r701193087
##########
File path: daffodil-japi/src/main/scala/org/apache/daffodil/japi/infoset/Infoset.scala
##########
@@ -67,13 +70,29 @@ abstract class InfosetInputter extends SInfosetInputter {
*/
def getNamespaceURI(): String
+ /**
+ * Wrapper function to convert the runtimeProperties Scala Map to a Java Map
+ * and call the getSimpleText function. Proably should not be overridden by
+ * those using the Java API. Instead override getSimpleText(primType, runtimeProperties)
+ * or getSimpleText(primType)
+ */
+ override def getSimpleText(primType: NodeInfo.Kind, runtimeProperties: Map[String,String]): String =
+ getSimpleText(primType, runtimeProperties.asJava)
Review comment:
Yeah, I noticed that :( In order to cover this we would need to write a custom infoset inputter in Java (and probably also in Scala for the Scala API). It's doable, but probably a pain. I'll see if I can come up with a real simple infoset inputter that avoids a lot of complexity that infoset inputters/outputters normally have.
--
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
[GitHub] [daffodil] stevedlawrence commented on a change in pull request #629: Add dfdlx:runtimeProperties extension
Posted by GitBox <gi...@apache.org>.
stevedlawrence commented on a change in pull request #629:
URL: https://github.com/apache/daffodil/pull/629#discussion_r701279275
##########
File path: daffodil-propgen/src/main/resources/org/apache/daffodil/xsd/dfdlx.xsd
##########
@@ -144,4 +145,16 @@
</xs:restriction>
</xs:simpleType>
+ <xs:attributeGroup name="RuntimePropertiesAG">
+ <xs:attribute form="qualified" name="runtimeProperties" type="dfdlx:RuntimePropertiesType" />
+ </xs:attributeGroup>
+
+ <xs:attributeGroup name="RuntimePropertiesAGQualified">
+ <xs:attribute form="qualified" name="runtimeProperties" type="dfdlx:RuntimePropertiesType" />
+ </xs:attributeGroup>
+
+ <xs:simpleType name="RuntimePropertiesType">
+ <xs:restriction base="xs:string" />
Review comment:
I think I can just change the split to happen on `\s+` instead of single-space to support that. I'd rather add that now while this is all fresh in my mind.
--
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
[GitHub] [daffodil] mbeckerle commented on a change in pull request #629: Add dfdlx:runtimeProperties extension
Posted by GitBox <gi...@apache.org>.
mbeckerle commented on a change in pull request #629:
URL: https://github.com/apache/daffodil/pull/629#discussion_r701248087
##########
File path: daffodil-runtime1/src/main/scala/org/apache/daffodil/processors/RuntimeData.scala
##########
@@ -697,7 +698,8 @@ sealed abstract class ErrorERD(local: String, namespaceURI: String)
null, // fillByteEvArg => FillByteEv
Nope, // maybeCheckByteAndBitOrderEvArg: => Maybe[CheckByteAndBitOrderEv],
Nope, // maybeCheckBitOrderAndCharsetEvArg: => Maybe[CheckBitOrderAndCharsetEv],
- false // isQuasiElementArg: => Boolean
+ false, // isQuasiElementArg: => Boolean
+ null, // runtimeProperties: Map[String,String]
Review comment:
Yeah, I guess I'll go with null is ok here as well.
--
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
[GitHub] [daffodil] stevedlawrence commented on a change in pull request #629: Add dfdlx:runtimeProperties extension
Posted by GitBox <gi...@apache.org>.
stevedlawrence commented on a change in pull request #629:
URL: https://github.com/apache/daffodil/pull/629#discussion_r701187788
##########
File path: daffodil-runtime1/src/main/scala/org/apache/daffodil/infoset/InfosetInputter.scala
##########
@@ -121,6 +121,13 @@ abstract class InfosetInputter
* NonTextFoundInSimpleContentException. If the element does not have any
* simple content, this should return either null or the empty string.
*/
+ def getSimpleText(primType: NodeInfo.Kind, runtimeProperties: Map[String,String]): String =
+ getSimpleText(primType)
+
+ /**
+ * See getSimpleText(primType, runtimeProperties), which has a default
+ * implementation to call this function without the runtimeProperties Map
+ */
def getSimpleText(primType: NodeInfo.Kind): String
Review comment:
I was trying to prevent any issues with backwards compatibility.
Thinking about it though, this InfosetInputter.scala stuff is "internal" API, so I think it shouldn't be a problem to have this use a default parameter. In fact, since this is internal, this really can drop the single-arg version entirely--Daffodil internals will never call this single-arg version. This does require updating all of our built-in infoset inputters to use the two-arg function, but that's trivial to just add the new parameter.
But I think the public Java/Scala API infoset inputters still need the changes I've added to Infoset.scala. Otherwise, we'll break existing InfosetInputters that don't implement the two arg version. I think...
Note that we also need to take a look at our InfosetOutputter/Inputter API's (see [Daffodil1909](https://issues.apache.org/jira/browse/DAFFODIL-1909)), so we'll probably need to break things at some point. So maybe we break things for that change?
--
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
[GitHub] [daffodil] stevedlawrence commented on a change in pull request #629: Add dfdlx:runtimeProperties extension
Posted by GitBox <gi...@apache.org>.
stevedlawrence commented on a change in pull request #629:
URL: https://github.com/apache/daffodil/pull/629#discussion_r701191045
##########
File path: daffodil-core/src/main/scala/org/apache/daffodil/dsom/ElementBase.scala
##########
@@ -1179,4 +1179,17 @@ trait ElementBase
Nil
res
}
+
+ lazy val runtimeProperties = findPropertyOption("runtimeProperties").toOption match {
Review comment:
Currently this is specific only to infoset inputters/outputters, but there's potential for other runtimes to use this in other ways. For example, maybe the runtime2 could use this property to enable some specific things that only runtime2 cares about. But I can add some document to that affect. It's up to runtimes to decide when and how they want to use this map.
There is no documentation for this property, I'll add a page to the wiki to describe it.
--
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
[GitHub] [daffodil] stevedlawrence commented on a change in pull request #629: Add dfdlx:runtimeProperties extension
Posted by GitBox <gi...@apache.org>.
stevedlawrence commented on a change in pull request #629:
URL: https://github.com/apache/daffodil/pull/629#discussion_r703531587
##########
File path: daffodil-japi/src/main/scala/org/apache/daffodil/japi/infoset/Infoset.scala
##########
@@ -67,13 +70,29 @@ abstract class InfosetInputter extends SInfosetInputter {
*/
def getNamespaceURI(): String
+ /**
+ * Wrapper function to convert the runtimeProperties Scala Map to a Java Map
+ * and call the getSimpleText function. Proably should not be overridden by
+ * those using the Java API. Instead override getSimpleText(primType, runtimeProperties)
+ * or getSimpleText(primType)
+ */
+ override def getSimpleText(primType: NodeInfo.Kind, runtimeProperties: Map[String,String]): String =
+ getSimpleText(primType, runtimeProperties.asJava)
Review comment:
> asJava's new MapWrapper allocation occurs only if a Java API user calls this specific two-arg getSimpleText method, correct?
Daffodil internals will call the two arg `getSimpleText()` version during unparsing for every simple element to be unparsed. So it's not up to the API users if it gets called. Daffodil will call it, and call it alot.
Essentially, only custom Java `InfosetInputter`s will have this overhead. The built-in ones all basically stay in Scala-land and so avoid this overhead. So it's only custom Java `InfosetInputter`s that would have this overhead. But unless they implement the two arg `getSimpleText` that takes a Scala `Map` (which is very unlikely) they'll have this `asJava` overhead.
I hadn't thought of option three. I guess the only concern here is if a user has lots of `dfdlx:runtimeProperties`, then there's still going to be a lot of `asJava` overhead. But maybe that's not a very common case and isn't worth worrying about. The logic is also a bit more complex though.
I guess it comes down to simpler logic but have to use Java Maps in a Scala world versus a bit more complex logic and maybe a little extra overhead in rare cases, but the Scala API is a bit more comfortable?
I think I lean towards the former, especially since I imagine once we switch to a single combined Scala + Java API instead of this separate sapi/japi stuff, that we'll basically just want to use Java structures everything, since that works for both Java and Scala without too much pain.
--
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
[GitHub] [daffodil] stevedlawrence commented on a change in pull request #629: Add dfdlx:runtimeProperties extension
Posted by GitBox <gi...@apache.org>.
stevedlawrence commented on a change in pull request #629:
URL: https://github.com/apache/daffodil/pull/629#discussion_r701323313
##########
File path: daffodil-core/src/test/scala/org/apache/daffodil/general/TestRuntimeProperties.scala
##########
@@ -0,0 +1,187 @@
+/*
+ * 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.general
+
+import org.junit.Test
+import org.junit.Assert.assertEquals
+import org.junit.Assert.assertFalse
+import org.junit.Assert.assertTrue
+
+import org.apache.daffodil.Implicits.intercept
+import org.apache.daffodil.dpath.NodeInfo
+import org.apache.daffodil.infoset.DISimple
+import org.apache.daffodil.infoset.ScalaXMLInfosetInputter
+import org.apache.daffodil.infoset.ScalaXMLInfosetOutputter
+import org.apache.daffodil.io.InputSourceDataInputStream
+import org.apache.daffodil.util.SchemaUtils
+import org.apache.daffodil.util.TestUtils
+
+/**
+ * Test InfosetOutputter that extends the ScalaXMLInfosetOutputter and redacts
+ * words specified in the dfdlx:runtimeProperties extension
Review comment:
This raises an interesting issue/limitation.
The way Daffodil does full validation is it tee's the infoset events to two InfosetOutputters: one provided by the user (e.g. our redacting outputter) and the other a normal XMLTextInfosetOutputter. At the end of parsing, we run Xerces validation on the result of the XMLTextInfosetOutputter. This is necessary because Daffodil only knows how to validate the result of the XMLTextInfosetOutputter. If the user provided InfosetOutputter output json, NiFi records, some proprietary thing, etc. it wouldn't be able enable full validation otherwise.
And the runtimeProperty redaction/transformation only affects the output of the user provied InfosetOutputter. It does not modify the internal infoset, and it does not propagate any changes to the XMLTextInfosetOutputter used for full validation.
So Daffodil validation has no knowledge about what changes the user provided InfosetOutputter makes, and so enabling full validation change anything. Daffodil only validates the infoset as parsed.
We can think of InfosetOutputters as post-processing of the infoset, and validation occurs before that post-processing takes place. If something is expecting to validate the post-processed data make by a custom InfosetOutputter, it needs to do that in a different step outside of Daffodil.
--
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
[GitHub] [daffodil] stevedlawrence commented on a change in pull request #629: Add dfdlx:runtimeProperties extension
Posted by GitBox <gi...@apache.org>.
stevedlawrence commented on a change in pull request #629:
URL: https://github.com/apache/daffodil/pull/629#discussion_r703433509
##########
File path: daffodil-japi/src/main/scala/org/apache/daffodil/japi/infoset/Infoset.scala
##########
@@ -67,13 +70,29 @@ abstract class InfosetInputter extends SInfosetInputter {
*/
def getNamespaceURI(): String
+ /**
+ * Wrapper function to convert the runtimeProperties Scala Map to a Java Map
+ * and call the getSimpleText function. Proably should not be overridden by
+ * those using the Java API. Instead override getSimpleText(primType, runtimeProperties)
+ * or getSimpleText(primType)
+ */
+ override def getSimpleText(primType: NodeInfo.Kind, runtimeProperties: Map[String,String]): String =
+ getSimpleText(primType, runtimeProperties.asJava)
Review comment:
I was wondering if it's worth getting rid of this `asJava`? Looking at the implementation, asJava allocates a new `MapWrapper`, which implements the `java.util.AbstractMap` interface to access the underlying Scala `Map`. So by using `asJava` we at least aren't creating a completely new `Map` and whatever allocations are involved with that, but we are still allocating this wrapper class. And we do this even when this Maps are empty (which is going to be the most common case). And even worse, most implementations of this Infoset are not even going to implement this two arg version of getSimpleText, so they'll this allocation will just be unused.
So, I'm wondering if when we create the `val runtimeProperties` in ElementBase.scala, we should just create a Java Map, which scala API users can use without a problem, and we never need to allocate anything extra? We could use the `asJava` method and create the wrapper mentioned above, or we could directly create a JavaMap to avoid that extra allocation and indirection? Any thoughts/concerns with this?
--
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
[GitHub] [daffodil] stevedlawrence merged pull request #629: Add dfdlx:runtimeProperties extension
Posted by GitBox <gi...@apache.org>.
stevedlawrence merged pull request #629:
URL: https://github.com/apache/daffodil/pull/629
--
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
[GitHub] [daffodil] mbeckerle commented on a change in pull request #629: Add dfdlx:runtimeProperties extension
Posted by GitBox <gi...@apache.org>.
mbeckerle commented on a change in pull request #629:
URL: https://github.com/apache/daffodil/pull/629#discussion_r701251996
##########
File path: daffodil-runtime1/src/main/scala/org/apache/daffodil/processors/RuntimeData.scala
##########
@@ -697,7 +698,8 @@ sealed abstract class ErrorERD(local: String, namespaceURI: String)
null, // fillByteEvArg => FillByteEv
Nope, // maybeCheckByteAndBitOrderEvArg: => Maybe[CheckByteAndBitOrderEv],
Nope, // maybeCheckBitOrderAndCharsetEvArg: => Maybe[CheckBitOrderAndCharsetEv],
- false // isQuasiElementArg: => Boolean
+ false, // isQuasiElementArg: => Boolean
+ null, // runtimeProperties: Map[String,String]
Review comment:
null is ok here per reasoning in earlier comments in this review.
--
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
[GitHub] [daffodil] mbeckerle commented on a change in pull request #629: Add dfdlx:runtimeProperties extension
Posted by GitBox <gi...@apache.org>.
mbeckerle commented on a change in pull request #629:
URL: https://github.com/apache/daffodil/pull/629#discussion_r701251068
##########
File path: daffodil-propgen/src/main/resources/org/apache/daffodil/xsd/dfdlx.xsd
##########
@@ -144,4 +145,16 @@
</xs:restriction>
</xs:simpleType>
+ <xs:attributeGroup name="RuntimePropertiesAG">
+ <xs:attribute form="qualified" name="runtimeProperties" type="dfdlx:RuntimePropertiesType" />
+ </xs:attributeGroup>
+
+ <xs:attributeGroup name="RuntimePropertiesAGQualified">
+ <xs:attribute form="qualified" name="runtimeProperties" type="dfdlx:RuntimePropertiesType" />
+ </xs:attributeGroup>
+
+ <xs:simpleType name="RuntimePropertiesType">
+ <xs:restriction base="xs:string" />
Review comment:
To follow up on this; however, a list type can be separated by any whitespace, your parsing code looks for single spaces. Only attributes get their values normalized to single spaces. While infrequently used, we do have this Element-form syntax like:
```
<dfdl:property name="dfdlx:runtimeProperties">key=value1
key=value2
key=value3
</dfdl:property>
```
And in that situation the list would not be separated by individual spaces, but by whitespace generally, possibly more than 1 character.
You can park this as a separate JIRA ticket that dfdlx:runtimeProperties is incompatible with dfdl:property style annotations.
--
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
[GitHub] [daffodil] tuxji commented on a change in pull request #629: Add dfdlx:runtimeProperties extension
Posted by GitBox <gi...@apache.org>.
tuxji commented on a change in pull request #629:
URL: https://github.com/apache/daffodil/pull/629#discussion_r703543087
##########
File path: daffodil-japi/src/main/scala/org/apache/daffodil/japi/infoset/Infoset.scala
##########
@@ -67,13 +70,29 @@ abstract class InfosetInputter extends SInfosetInputter {
*/
def getNamespaceURI(): String
+ /**
+ * Wrapper function to convert the runtimeProperties Scala Map to a Java Map
+ * and call the getSimpleText function. Proably should not be overridden by
+ * those using the Java API. Instead override getSimpleText(primType, runtimeProperties)
+ * or getSimpleText(primType)
+ */
+ override def getSimpleText(primType: NodeInfo.Kind, runtimeProperties: Map[String,String]): String =
+ getSimpleText(primType, runtimeProperties.asJava)
Review comment:
OK, a Java Map in ElementBase sounds good to me then.
--
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
[GitHub] [daffodil] stevedlawrence commented on pull request #629: Add dfdlx:runtimeProperties extension
Posted by GitBox <gi...@apache.org>.
stevedlawrence commented on pull request #629:
URL: https://github.com/apache/daffodil/pull/629#issuecomment-912750979
I've pushed updates that I think should address the comments, this includes new test InfosetInputters using the Scala/Java API to ensure coverage. It also shows some issues. For example, the Java API needs to access some Scala objects, which is pretty awkward.
> Is it worth adding an implementation and a test showing the motivating example of a custom XML Infoset Inputter and Outputter for embedded XML strings in binary data?
I don't really want to add such an implementation, just because I suspect it's going to be a decent amount of work, and a decent amount of testing. I think it's basically a implementation of one of the InfosetInputters/Outputters with quite a bit of extra complicated logic. If we do want to include that in the repo I think that's worth done in another PR. Once I add some documentation (next week) I'll add this as a motivating example and probably include some pseuocode about how this might be implemented.
--
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
[GitHub] [daffodil] mbeckerle commented on a change in pull request #629: Add dfdlx:runtimeProperties extension
Posted by GitBox <gi...@apache.org>.
mbeckerle commented on a change in pull request #629:
URL: https://github.com/apache/daffodil/pull/629#discussion_r701150072
##########
File path: daffodil-propgen/src/main/resources/org/apache/daffodil/xsd/dfdlx.xsd
##########
@@ -144,4 +145,16 @@
</xs:restriction>
</xs:simpleType>
+ <xs:attributeGroup name="RuntimePropertiesAG">
+ <xs:attribute form="qualified" name="runtimeProperties" type="dfdlx:RuntimePropertiesType" />
+ </xs:attributeGroup>
+
+ <xs:attributeGroup name="RuntimePropertiesAGQualified">
+ <xs:attribute form="qualified" name="runtimeProperties" type="dfdlx:RuntimePropertiesType" />
+ </xs:attributeGroup>
+
+ <xs:simpleType name="RuntimePropertiesType">
+ <xs:restriction base="xs:string" />
Review comment:
You should add a pattern here that enforces the key-value space separated list pattern.
I think this is most easily expressed by creating a simple type for a single key=value pair, and then
defining an XSD type which is a list of those.
So I think:
```
<xs:simpleType name="KVList">
<xs:list>
<xs:simpleType>
<xs:restriction base="xs:token">
<xs:pattern value="[a-zA-Z_][a-zA-Z0-9_]*=\S*"/>
</xs:restriction>
</xs:simpleType>
</xs:list>
</xs:simpleType>
```
##########
File path: daffodil-core/src/test/scala/org/apache/daffodil/general/TestRuntimeProperties.scala
##########
@@ -0,0 +1,187 @@
+/*
+ * 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.general
+
+import org.junit.Test
+import org.junit.Assert.assertEquals
+import org.junit.Assert.assertFalse
+import org.junit.Assert.assertTrue
+
+import org.apache.daffodil.Implicits.intercept
+import org.apache.daffodil.dpath.NodeInfo
+import org.apache.daffodil.infoset.DISimple
+import org.apache.daffodil.infoset.ScalaXMLInfosetInputter
+import org.apache.daffodil.infoset.ScalaXMLInfosetOutputter
+import org.apache.daffodil.io.InputSourceDataInputStream
+import org.apache.daffodil.util.SchemaUtils
+import org.apache.daffodil.util.TestUtils
+
+/**
+ * Test InfosetOutputter that extends the ScalaXMLInfosetOutputter and redacts
+ * words specified in the dfdlx:runtimeProperties extension
Review comment:
This redaction example is perhaps too simple.
An interesting thing is when the special infoset outputter creates XML that is NOT valid w.r.t. the DFDL schema interpreted as an XML Schema. One then needs a separate schema for validation from the one used for parsing/unparsing.
If you add a pattern facet to the filtered element below in your DFDL schemas, such that things with the word REDACTED in them will NOT be valid w.r.t the pattern, that would be more representative of how this feature can change the XML in ways that violate the XML validity.
This is tricky with respect to Daffodil's validation behaviors. "limited" validation will validate still, as it sees the infoset data during parsing. "full" validation should take the output of the infoset outputter and feed it to Xerces for validation, and that should create validation errors.
I think tests which characterize these two different interactions of this feature with validation would be good additions.
##########
File path: daffodil-runtime1/src/main/scala/org/apache/daffodil/processors/RuntimeData.scala
##########
@@ -697,7 +698,8 @@ sealed abstract class ErrorERD(local: String, namespaceURI: String)
null, // fillByteEvArg => FillByteEv
Nope, // maybeCheckByteAndBitOrderEvArg: => Maybe[CheckByteAndBitOrderEv],
Nope, // maybeCheckBitOrderAndCharsetEvArg: => Maybe[CheckBitOrderAndCharsetEv],
- false // isQuasiElementArg: => Boolean
+ false, // isQuasiElementArg: => Boolean
+ null, // runtimeProperties: Map[String,String]
Review comment:
You could use an empty map here. It's not like the other places where we use null here which are objects that would e hard to construct.
##########
File path: daffodil-core/src/test/scala/org/apache/daffodil/general/TestRuntimeProperties.scala
##########
@@ -0,0 +1,187 @@
+/*
+ * 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.general
+
+import org.junit.Test
+import org.junit.Assert.assertEquals
+import org.junit.Assert.assertFalse
+import org.junit.Assert.assertTrue
+
+import org.apache.daffodil.Implicits.intercept
+import org.apache.daffodil.dpath.NodeInfo
+import org.apache.daffodil.infoset.DISimple
+import org.apache.daffodil.infoset.ScalaXMLInfosetInputter
+import org.apache.daffodil.infoset.ScalaXMLInfosetOutputter
+import org.apache.daffodil.io.InputSourceDataInputStream
+import org.apache.daffodil.util.SchemaUtils
+import org.apache.daffodil.util.TestUtils
+
+/**
+ * Test InfosetOutputter that extends the ScalaXMLInfosetOutputter and redacts
+ * words specified in the dfdlx:runtimeProperties extension
+ */
+class RedactingScalaXMLInfosetOutputter
+ extends ScalaXMLInfosetOutputter {
+
+ override def startSimple(diSimple: DISimple): Boolean = {
+ super.startSimple(diSimple)
+
+ val runtimeProperties = diSimple.erd.runtimeProperties
+
+ val redactions = runtimeProperties.get("redact").toSeq.flatMap { key => key.split(",") }
+ if (!redactions.isEmpty) {
+ val thisElem = stack.top.last.asInstanceOf[scala.xml.Elem]
+ var text = thisElem.child(0).asInstanceOf[scala.xml.Text].data
+ val redactWord = runtimeProperties.get("redactWord").getOrElse("")
+ redactions.foreach { redaction =>
+ text = text.replace(redaction, redactWord)
+ }
+ val newChild = Seq(new scala.xml.Text(text))
+ val newElem = thisElem.copy(child = newChild)
+ stack.top(stack.top.size - 1) = newElem
+ }
+ true
+ }
+}
+
+/**
+ * Test InfosetInputter that extends the ScalaXMLInfosetInputter and redacts
+ * words specified in the dfdlx:runtimeProperties extension
+ */
+class RedactingScalaXMLInfosetInputter(rootNode: scala.xml.Node)
+ extends ScalaXMLInfosetInputter(rootNode) {
+
+ override def getSimpleText(primType: NodeInfo.Kind, runtimeProperties: Map[String, String]): String = {
+ var text = super.getSimpleText(primType)
+
+ val redactions = runtimeProperties.get("redact").toSeq.flatMap { key => key.split(",") }
+ if (!redactions.isEmpty) {
+ val redactWord = runtimeProperties.get("redactWord").getOrElse("")
+ redactions.foreach { redaction =>
+ text = text.replace(redaction, redactWord)
+ }
+ }
+ text
+ }
+}
+
+class TestRuntimeProperties {
+
+ val testSchema = SchemaUtils.dfdlTestSchema(
+ <xs:include schemaLocation="org/apache/daffodil/xsd/DFDLGeneralFormat.dfdl.xsd"/>,
+ <dfdl:format ref="tns:GeneralFormat" lengthKind="delimited"/>,
+ <xs:element name="root">
+ <xs:complexType>
+ <xs:sequence dfdl:separator=",">
+ <xs:element name="filtered" type="xs:string" dfdlx:runtimeProperties="redact=foo,baz redactWord=REDACTED" />
Review comment:
Per suggestion above, make another test schema which is just like this one, but which adds a pattern facet to filtered disallowing the "REDACTED" from showing up in the data. So that if redaction occurs, the resulting data will be invalid. Then have tests detect this validation error, but only if "full" validation is on.
##########
File path: daffodil-core/src/main/scala/org/apache/daffodil/dsom/ElementBase.scala
##########
@@ -1179,4 +1179,17 @@ trait ElementBase
Nil
res
}
+
+ lazy val runtimeProperties = findPropertyOption("runtimeProperties").toOption match {
+ case None => Map.empty[String, String]
+ case Some(value) => {
+ schemaDefinitionUnless(isSimpleType, "dfdlx:runtimeProperties is only valid on simple types")
+ value.split(" ").filter(_ != "").map { v =>
Review comment:
Worth a comment here to say that DFDL schema validation will check that the dfdlx:runtimeProperties property has valid syntax. (assuming you add the pattern facet to the property definition)
--
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