[GitHub] [daffodil] tuxji commented on a diff in pull request #819: Support XML strings in XMLTextInfosetInputter/Outputter

[GitBox <gi...@apache.org>]

tuxji commented on code in PR #819:
URL: https://github.com/apache/daffodil/pull/819#discussion_r941373667


##########
daffodil-runtime1/src/main/scala/org/apache/daffodil/infoset/XMLTextInfosetInputter.scala:
##########
@@ -17,24 +17,27 @@
 
 package org.apache.daffodil.infoset
 
+import java.io.StringWriter
+import java.nio.charset.StandardCharsets
+import javax.xml.XMLConstants
+import javax.xml.stream.XMLInputFactory
+import javax.xml.stream.XMLStreamConstants._
+import javax.xml.stream.XMLStreamException
+import javax.xml.stream.XMLStreamReader
+import javax.xml.stream.XMLStreamWriter
+import javax.xml.stream.util.XMLEventAllocator
+
+import org.apache.daffodil.dpath.NodeInfo
 import org.apache.daffodil.exceptions.Assert
+import org.apache.daffodil.infoset.InfosetInputterEventType._
+import org.apache.daffodil.util.MaybeBoolean
 import org.apache.daffodil.util.Misc
 import org.apache.daffodil.xml.XMLUtils
-import org.apache.daffodil.util.MaybeBoolean
-import org.apache.daffodil.dpath.NodeInfo
-import org.apache.daffodil.infoset.InfosetInputterEventType._
-
-import javax.xml.stream.XMLStreamReader
-import javax.xml.stream.XMLStreamConstants._
-import javax.xml.stream.XMLInputFactory
-import javax.xml.stream.util.XMLEventAllocator
-import javax.xml.stream.XMLStreamException
-import javax.xml.XMLConstants
 
-object XMLTextInfosetInputter {
+object XMLTextInfoset {

Review Comment:
   What's the reason for the name change?  It might be nice to put a one-line comment above here explaining the object's purpose.



##########
daffodil-runtime1/src/main/scala/org/apache/daffodil/infoset/InfosetWalker.scala:
##########
@@ -452,7 +466,7 @@ class InfosetWalker private (
       if (child.isSimple) {
         if (!child.isHidden || walkHidden) {
           val simple = child.asInstanceOf[DISimple]
-          outputter.startSimple(simple)
+          doOutputter(outputter.startSimple(simple), "start infoset simple element", simple.erd)
           outputter.endSimple(simple)

Review Comment:
   Does `outputter.endSimple(simple)` need a `doOutputter` wrapper too?
   
   All in all, it seems like a pain to have to wrap all the infosetinputter calls.  It would seem simpler to let InfosetInputter throw an exception and let the exception propagate outside of this method without having to catch it and handle it.



##########
daffodil-runtime1/src/main/scala/org/apache/daffodil/infoset/InfosetWalker.scala:
##########
@@ -408,14 +413,23 @@ class InfosetWalker private (
     containerIndexStack.setTop(top + 1)
   }
 
+  @inline
+  private def doOutputter(outputterFunc: => Boolean, desc: String, context: ThrowsSDE): Unit = {
+    Try(outputterFunc) match {
+      case Success(true) => // success
+      case Success(false) => context.SDE("Failed to %s", desc)
+      case Failure(e) => context.SDE("Failed to %s: %s", desc, e.toString)

Review Comment:
   Note that `e.toString` only prints the exception's name and message.  If the exception has a good chance of containing a real reason exception object itself, you should print that real reason object in the SDE message or simply throw the whole exception encapsulated by XMLTextInfosetOutputterFailure(e) to ensure the user sees the real reason for the failure.



##########
daffodil-runtime1/src/main/scala/org/apache/daffodil/infoset/InfosetWalker.scala:
##########
@@ -408,14 +413,23 @@ class InfosetWalker private (
     containerIndexStack.setTop(top + 1)
   }
 
+  @inline
+  private def doOutputter(outputterFunc: => Boolean, desc: String, context: ThrowsSDE): Unit = {
+    Try(outputterFunc) match {
+      case Success(true) => // success
+      case Success(false) => context.SDE("Failed to %s", desc)

Review Comment:
   I'll defer to your opinions on changing the InfosetInputter API since I don't know how important backward compatibility is to Daffodil users.  However, I agree that returning a boolean false and throwing an exception are redundant ways of informing the caller of failure.  Furthermore, an exception can contain a message and a real reason exception object while a boolean false contains no additional information, leaving the user puzzled and wondering why a failure happened unless the InfosetInputter object also has the Diagnostics trait (which means it's a mutable object with all the problems of mutable state).



-- 
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