You are viewing a plain text version of this content. The canonical link for it is here.
Posted to commits@daffodil.apache.org by "mike-mcgann (via GitHub)" <gi...@apache.org> on 2023/03/09 20:19:08 UTC

[GitHub] [daffodil] mike-mcgann opened a new pull request, #985: Throw exceptions on fatal SAX parsing errors

mike-mcgann opened a new pull request, #985:
URL: https://github.com/apache/daffodil/pull/985

   The error handlers for SAX parsing were accumulating fatal errors into a list and allowing parsing to continue. In a certain case with a malformed XML document, the same error was being continiously reported causing an infinite loop. This change now records the error into the list and then re-raises the SAXParseException to stop execution.
   
   [DAFFODIL-2586](https://issues.apache.org/jira/browse/DAFFODIL-2586)


-- 
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] mike-mcgann commented on a diff in pull request #985: Throw exceptions on fatal SAX parsing errors

Posted by "mike-mcgann (via GitHub)" <gi...@apache.org>.
mike-mcgann commented on code in PR #985:
URL: https://github.com/apache/daffodil/pull/985#discussion_r1131652870


##########
daffodil-test/src/test/scala/org/apache/daffodil/section00/general/TestDisallowDocType.scala:
##########
@@ -87,7 +86,7 @@ class TestDisallowDocType {
   }
 
   @Test def test_infosetFileMustNotHaveDocType(): Unit = {
-    val e = intercept[TDMLException] {
+    val e = intercept[SAXParseException] {

Review Comment:
   It still needs to stop execution on the fatal error here or it will get caught in the infinite loop. I originally had fatalError throwing an SDE but then I found out the TDML runner installs its own custom error handler. So, to mimic the same behavior I tried to throw an SDE in that runner but it appears that the concrete implementations of those are in runtime1 and not lib. That is why I left it as a SAXParseException. Should the TDML runner catch any unhanded SAXParseExceptions and wrap them in TDMLExceptions or is there a better 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 a diff in pull request #985: Throw exceptions on fatal SAX parsing errors

Posted by "stevedlawrence (via GitHub)" <gi...@apache.org>.
stevedlawrence commented on code in PR #985:
URL: https://github.com/apache/daffodil/pull/985#discussion_r1132337698


##########
daffodil-test/src/test/scala/org/apache/daffodil/section00/general/TestDisallowDocType.scala:
##########
@@ -87,7 +86,7 @@ class TestDisallowDocType {
   }
 
   @Test def test_infosetFileMustNotHaveDocType(): Unit = {
-    val e = intercept[TDMLException] {
+    val e = intercept[SAXParseException] {

Review Comment:
   I wonder if the problem of the leaking exception is here:
   
   https://github.com/apache/daffodil/blob/b1a5ab06c3bd194ecd43f37789654c89c83beb49/daffodil-lib/src/main/scala/org/apache/daffodil/lib/xml/DaffodilConstructingLoader.scala#L477
   
   When parsing fatalError will be called which throws an exception. That is likely caught in this catch block, but then calls fatalError which will throw that exception. Not only does this mean we'll see the exception twice, it means the exception leaks out. It probably also means the exception is duplicated in the diagnostics, unless the exception was thrown by scala-xml without calling fatalError.  
   



-- 
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] mike-mcgann commented on a diff in pull request #985: Throw exceptions on fatal SAX parsing errors

Posted by "mike-mcgann (via GitHub)" <gi...@apache.org>.
mike-mcgann commented on code in PR #985:
URL: https://github.com/apache/daffodil/pull/985#discussion_r1132373687


##########
daffodil-test/src/test/scala/org/apache/daffodil/section00/general/TestDisallowDocType.scala:
##########
@@ -87,7 +86,7 @@ class TestDisallowDocType {
   }
 
   @Test def test_infosetFileMustNotHaveDocType(): Unit = {
-    val e = intercept[TDMLException] {
+    val e = intercept[SAXParseException] {

Review Comment:
   Now that makes me wonder if Daffodil itself is the one causing the infinite loop. Especially since you said that it looks like scala-xml isn't calling it directly. Maybe the case where an exception is being handled but delegated but then the handler doesn't know it is the delegate so it tries to handle it and then delegates and gets caught in a loop. I'll take a deeper look and follow the error handling paths. 



-- 
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] mike-mcgann commented on a diff in pull request #985: Throw exceptions on fatal SAX parsing errors

Posted by "mike-mcgann (via GitHub)" <gi...@apache.org>.
mike-mcgann commented on code in PR #985:
URL: https://github.com/apache/daffodil/pull/985#discussion_r1132489712


##########
daffodil-test/src/test/scala/org/apache/daffodil/section00/general/TestDisallowDocType.scala:
##########
@@ -87,7 +86,7 @@ class TestDisallowDocType {
   }
 
   @Test def test_infosetFileMustNotHaveDocType(): Unit = {
-    val e = intercept[TDMLException] {
+    val e = intercept[SAXParseException] {

Review Comment:
   I found the root cause of the issue. Daffodil was working fine. It is a bug in scala-xml. In this specific case, the infinite loop is caused when the scanner does not advance when this error is reported. There is evidence of error correction elsewhere so I think this was simply overlooked. My proposed solution is to now only throw an exception when that specific error is raised. Yes, looking at the error message is a bit hackish, but I'm thinking that it is a good compromise where it can continue as much as possible without causing the infinite loop. We have a test now and if the message changes, it should be caught. 



-- 
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 diff in pull request #985: Throw exceptions on fatal SAX parsing errors

Posted by "stevedlawrence (via GitHub)" <gi...@apache.org>.
stevedlawrence commented on code in PR #985:
URL: https://github.com/apache/daffodil/pull/985#discussion_r1131661855


##########
daffodil-test/src/test/scala/org/apache/daffodil/section00/general/TestDisallowDocType.scala:
##########
@@ -87,7 +86,7 @@ class TestDisallowDocType {
   }
 
   @Test def test_infosetFileMustNotHaveDocType(): Unit = {
-    val e = intercept[TDMLException] {
+    val e = intercept[SAXParseException] {

Review Comment:
   >  Should the TDML runner catch any unhanded SAXParseExceptions and wrap them in TDMLExceptions or is there a better way?
   
   It probably depends where the errors are coming from. If the errors are from compiling a DFDL schema used by a TDML test, those error should all end up as `Diagnositc`s that the TDML runner can look for. If they are coming from something else (e.g. TDMLRunner validating a .tdml file), then it probably wants to be caught and turned into a TDML exception.



-- 
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 #985: Throw exceptions on fatal SAX parsing errors

Posted by "stevedlawrence (via GitHub)" <gi...@apache.org>.
stevedlawrence commented on PR #985:
URL: https://github.com/apache/daffodil/pull/985#issuecomment-1474291981

   Taking a second look at things and at the PR you created for scala-xml to fix the underlying issue, it doesn't look like the infinite loop issue is specific to having `<` characters in an attribute like your `msg` check looks for. Could other illegal characters cause this issue? I wonder if we need to check for a more generic error message not specific to `<`?


-- 
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 diff in pull request #985: Throw exceptions on fatal SAX parsing errors

Posted by "stevedlawrence (via GitHub)" <gi...@apache.org>.
stevedlawrence commented on code in PR #985:
URL: https://github.com/apache/daffodil/pull/985#discussion_r1132497880


##########
daffodil-test/src/test/scala/org/apache/daffodil/section00/general/TestDisallowDocType.scala:
##########
@@ -87,7 +86,7 @@ class TestDisallowDocType {
   }
 
   @Test def test_infosetFileMustNotHaveDocType(): Unit = {
-    val e = intercept[TDMLException] {
+    val e = intercept[SAXParseException] {

Review Comment:
   Do you think the fix in scala-xml would be fairly straightforward? It would be nice if we could submit a PR so we can eventually get rid of the special casing, since I agree it does sound like a bit of a hack.



-- 
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] mike-mcgann commented on a diff in pull request #985: Throw exceptions on fatal SAX parsing errors

Posted by "mike-mcgann (via GitHub)" <gi...@apache.org>.
mike-mcgann commented on code in PR #985:
URL: https://github.com/apache/daffodil/pull/985#discussion_r1132533243


##########
daffodil-test/src/test/scala/org/apache/daffodil/section00/general/TestDisallowDocType.scala:
##########
@@ -87,7 +86,7 @@ class TestDisallowDocType {
   }
 
   @Test def test_infosetFileMustNotHaveDocType(): Unit = {
-    val e = intercept[TDMLException] {
+    val e = intercept[SAXParseException] {

Review Comment:
   The code for that is as follows:
   
   ```scala
     /**
      * attribute value, terminated by either `'` or `"`. value may not contain `<`.
      * @param endCh either `'` or `"`
      */
     def xAttributeValue(endCh: Char): String = {
       val buf = new StringBuilder
       while (ch != endCh && !eof) {
         // well-formedness constraint
         if (ch == '<') reportSyntaxError("'<' not allowed in attrib value")
         else if (ch == SU) truncatedError("")
         else buf append ch_returning_nextch
       }
       ch_returning_nextch
       // @todo: normalize attribute value
       buf.toString
     }
   ```
   
   I think that when a `<` character is seen, the error recovery needs to be a call to `ch_returning_nextch`  without appending it to `buf`. So I think it is probably straightforward but I'm not sure. The call to `truncatedError` doesn't seem to handle that either. Maybe there is some other expectation here. It might be worth time boxing a fix attempt. If a working PR can be prepared in less than X hours, great, otherwise drop the attempt. 



-- 
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] mike-mcgann commented on pull request #985: Throw exceptions on fatal SAX parsing errors

Posted by "mike-mcgann (via GitHub)" <gi...@apache.org>.
mike-mcgann commented on PR #985:
URL: https://github.com/apache/daffodil/pull/985#issuecomment-1474301186

   > Taking a second look at things and at the PR you created for scala-xml to fix the underlying issue, it doesn't look like the infinite loop issue is specific to having `<` characters in an attribute like your `msg` check looks for. Could other illegal characters cause this issue? I wonder if we need to check for a more generic error message not specific to `<`?
   
   That PR isn't for this exact issue. I decided to put in a PR for a similar issue that had an existing bug report. If that goes well, then I'll file another bug report for this issue and a PR along with it. I wanted to test the waters first. 


-- 
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] mike-mcgann commented on a diff in pull request #985: Throw exceptions on fatal SAX parsing errors

Posted by "mike-mcgann (via GitHub)" <gi...@apache.org>.
mike-mcgann commented on code in PR #985:
URL: https://github.com/apache/daffodil/pull/985#discussion_r1140212034


##########
daffodil-test/src/test/scala/org/apache/daffodil/section00/general/TestDisallowDocType.scala:
##########
@@ -87,7 +86,7 @@ class TestDisallowDocType {
   }
 
   @Test def test_infosetFileMustNotHaveDocType(): Unit = {
-    val e = intercept[TDMLException] {
+    val e = intercept[SAXParseException] {

Review Comment:
   Do we want to keep this explicit check in for now and remove it if, and when, a fix gets merged into scala-xml? I'll try to get a fix in over there but there is no guarantee on when that could happen. 



-- 
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] mike-mcgann commented on a diff in pull request #985: Throw exceptions on fatal SAX parsing errors

Posted by "mike-mcgann (via GitHub)" <gi...@apache.org>.
mike-mcgann commented on code in PR #985:
URL: https://github.com/apache/daffodil/pull/985#discussion_r1131698400


##########
daffodil-core/src/main/scala/org/apache/daffodil/core/dsom/DFDLSchemaFile.scala:
##########
@@ -92,12 +92,12 @@ class DFDLSchemaFileLoadErrorHandler(schemaFileLocation: SchemaFileLocation)
   }
 
   /**
-   * Called on a fatal exception. The parser/validator throws the exception after
-   * this call returns.
+   * Called on a fatal exception and throws the exception
    */
-  def fatalError(exception: SAXParseException) = error(
-    exception,
-  ) // same as non-fatal exception.
+  def fatalError(exception: SAXParseException) = {
+    loaderErrors_ :+= exception
+    throw exception

Review Comment:
   This is needed to workaround what appears to be a bug in scala-xml and its trying to stop it because it is getting caught in that loop. Yes, I wouldn't expect that behavior and a comment can be added to explain what is going on. I was just saying that the driver may continue as much as it wants and doesn't have to throw an exception when the error handler is installed. 
   
   As an alternative, we could always keep around the last error seen. If the next error is exactly the same as the last error, no progress is being made, and we could throw the exception at that point. That way we can get as many errors as possible without triggering the infinite loop. 



-- 
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 diff in pull request #985: Throw exceptions on fatal SAX parsing errors

Posted by "stevedlawrence (via GitHub)" <gi...@apache.org>.
stevedlawrence commented on code in PR #985:
URL: https://github.com/apache/daffodil/pull/985#discussion_r1132332675


##########
daffodil-core/src/main/scala/org/apache/daffodil/core/dsom/DFDLSchemaFile.scala:
##########
@@ -92,12 +92,12 @@ class DFDLSchemaFileLoadErrorHandler(schemaFileLocation: SchemaFileLocation)
   }
 
   /**
-   * Called on a fatal exception. The parser/validator throws the exception after
-   * this call returns.
+   * Called on a fatal exception and throws the exception
    */
-  def fatalError(exception: SAXParseException) = error(
-    exception,
-  ) // same as non-fatal exception.
+  def fatalError(exception: SAXParseException) = {
+    loaderErrors_ :+= exception
+    throw exception

Review Comment:
   > As an alternative, we could always keep around the last error seen...
   
   It's probably not worth the added complexity, I think just commenting about why we have the throw an limitations (e.g. maybe don't get as many errors as possible) is fine.
   
   Also, I grepped through the scala-xml code and couldn't find any calls to `fatalError`. I wonder if under the hood it's just using Xerces or some Java built-in XML parser, and the issue is actually in that parser and not in scala-xml. Can you output a stack trace? It would be nice to open a ticket with whatever is doing this and see if it could be fixed.



-- 
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 diff in pull request #985: Throw exceptions on fatal SAX parsing errors

Posted by "stevedlawrence (via GitHub)" <gi...@apache.org>.
stevedlawrence commented on code in PR #985:
URL: https://github.com/apache/daffodil/pull/985#discussion_r1131657976


##########
daffodil-core/src/main/scala/org/apache/daffodil/core/dsom/DFDLSchemaFile.scala:
##########
@@ -92,12 +92,12 @@ class DFDLSchemaFileLoadErrorHandler(schemaFileLocation: SchemaFileLocation)
   }
 
   /**
-   * Called on a fatal exception. The parser/validator throws the exception after
-   * this call returns.
+   * Called on a fatal exception and throws the exception
    */
-  def fatalError(exception: SAXParseException) = error(
-    exception,
-  ) // same as non-fatal exception.
+  def fatalError(exception: SAXParseException) = {
+    loaderErrors_ :+= exception
+    throw exception

Review Comment:
   > it up to the application to decide whether to throw an exception for different types of errors and warnings. 
   
   This makes me think that we (the application) shouldn't need to throw an exception to get the behavior we need from Xerces (the driver). The behavior we want to just add warnings/errors/fatalErrots to a list and then to let the driver continue gathering errors if it can do so.
   
   And no where does it say the fatalError implementation *must* throw the exception. If that's actually needed because of a bug in Xerces/Scala XML/whatever is calling fatalError requires that behavior, we should document that.
   
   > In other words, a SAX driver class may throw an exception after reporting any fatalError.
   
   This is what I would expect. Xerces calls fatal error, we handle it and add the error to our list, and then Xerces throws the exception if can't make progress OR continues calling error/fatalError more if it can. By throwing an exception, we are telling Xerces to stop even if it could continue, which I don't think is what we want--we want as many errors from Xerces as possible.



-- 
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] mike-mcgann merged pull request #985: Throw exceptions when scala-xml cannot continue

Posted by "mike-mcgann (via GitHub)" <gi...@apache.org>.
mike-mcgann merged PR #985:
URL: https://github.com/apache/daffodil/pull/985


-- 
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] mike-mcgann commented on pull request #985: Throw exceptions on fatal SAX parsing errors

Posted by "mike-mcgann (via GitHub)" <gi...@apache.org>.
mike-mcgann commented on PR #985:
URL: https://github.com/apache/daffodil/pull/985#issuecomment-1474301979

   I also accidentally pushed to the wrong branch and just did a force push to revert that. 


-- 
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] mike-mcgann commented on a diff in pull request #985: Throw exceptions on fatal SAX parsing errors

Posted by "mike-mcgann (via GitHub)" <gi...@apache.org>.
mike-mcgann commented on code in PR #985:
URL: https://github.com/apache/daffodil/pull/985#discussion_r1131638458


##########
daffodil-core/src/main/scala/org/apache/daffodil/core/dsom/DFDLSchemaFile.scala:
##########
@@ -92,12 +92,12 @@ class DFDLSchemaFileLoadErrorHandler(schemaFileLocation: SchemaFileLocation)
   }
 
   /**
-   * Called on a fatal exception. The parser/validator throws the exception after
-   * this call returns.
+   * Called on a fatal exception and throws the exception
    */
-  def fatalError(exception: SAXParseException) = error(
-    exception,
-  ) // same as non-fatal exception.
+  def fatalError(exception: SAXParseException) = {
+    loaderErrors_ :+= exception
+    throw exception

Review Comment:
   I was reading the SE8 documentation for ErrorHandler and it has this paragraph:
   
   >For XML processing errors, a SAX driver must use this interface in preference to throwing an exception: it is up to the application to decide whether to throw an exception for different types of errors and warnings. Note, however, that there is no requirement that the parser continue to report additional errors after a call to [fatalError](https://docs.oracle.com/javase/8/docs/api/org/xml/sax/ErrorHandler.html#fatalError-org.xml.sax.SAXParseException-). In other words, a SAX driver class may throw an exception after reporting any fatalError. Also parsers may throw appropriate exceptions for non-XML errors. For example, [XMLReader.parse()](https://docs.oracle.com/javase/8/docs/api/org/xml/sax/XMLReader.html#parse-org.xml.sax.InputSource-) would throw an IOException for errors accessing entities or the document.
   
   From that I concluded that if you want to stop execution on a fatal error, an exception should be thrown in `fatalError` since it said the driver *may* throw an exception. I don't think the driver is required to throw an exception in this case and there is flexibility for it to throw one or not. 
   



-- 
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] mike-mcgann commented on a diff in pull request #985: Throw exceptions on fatal SAX parsing errors

Posted by "mike-mcgann (via GitHub)" <gi...@apache.org>.
mike-mcgann commented on code in PR #985:
URL: https://github.com/apache/daffodil/pull/985#discussion_r1132490472


##########
daffodil-tdml-lib/src/main/scala/org/apache/daffodil/tdml/TDMLRunner.scala:
##########
@@ -273,7 +273,7 @@ class DFDLTestSuite private[tdml] (
     }
 
     def fatalError(exception: SAXParseException) = {
-      error(exception)
+      loadingExceptions += exception

Review Comment:
   I'll change this back to the original for the final commit



-- 
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 diff in pull request #985: Throw exceptions on fatal SAX parsing errors

Posted by "stevedlawrence (via GitHub)" <gi...@apache.org>.
stevedlawrence commented on code in PR #985:
URL: https://github.com/apache/daffodil/pull/985#discussion_r1131575400


##########
daffodil-core/src/main/scala/org/apache/daffodil/core/dsom/DFDLSchemaFile.scala:
##########
@@ -92,12 +92,12 @@ class DFDLSchemaFileLoadErrorHandler(schemaFileLocation: SchemaFileLocation)
   }
 
   /**
-   * Called on a fatal exception. The parser/validator throws the exception after
-   * this call returns.
+   * Called on a fatal exception and throws the exception
    */
-  def fatalError(exception: SAXParseException) = error(
-    exception,
-  ) // same as non-fatal exception.
+  def fatalError(exception: SAXParseException) = {
+    loaderErrors_ :+= exception
+    throw exception

Review Comment:
   Reading the javadoc for fatalError, it seems like we shouldn't need to throw an exception here, and that the parser should throw the exception after calling fatalError if it is unable to continue.
   
   If this is needed to workaround a bug in Scala-xml that's fine, but it should be documented why it's needed. Maybe scala-xml expects fatalError implementations to throw the exceptions?



##########
daffodil-test/src/test/scala/org/apache/daffodil/section00/general/TestDisallowDocType.scala:
##########
@@ -87,7 +86,7 @@ class TestDisallowDocType {
   }
 
   @Test def test_infosetFileMustNotHaveDocType(): Unit = {
-    val e = intercept[TDMLException] {
+    val e = intercept[SAXParseException] {

Review Comment:
   Seems we shouldn't let SAXParseExceptions bubble  to the very top? I think the way the DaffodilConstructorLoader is intended to work is it doesn't throw exceptions but instead gathers up all errors in the error handler diagnostics to we can report them later?



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