stevedlawrence commented on code in PR #1572:
URL: https://github.com/apache/daffodil/pull/1572#discussion_r2429619066
##########
daffodil-core/src/main/java/org/apache/daffodil/api/package-info.java:
##########
@@ -250,41 +250,43 @@
* data ought to be written to. Any XMLReader implementation is permissible,
as long as they have
* XML Namespace support.
*
- * <pre>
- * {@code
- * ByteArrayInputStream is = new ByteArrayInputStream(data);
- * ByteArrayOutputStream os = new ByteArrayOutputStream();
- * WritableByteChannel wbc = java.nio.channels.Channels.newChannel(os);
- * DaffodilUnparseContentHandler unparseContentHandler =
dp.newContentHandlerInstance(wbc);
- * try {
- * XMLReader xmlReader =
SAXParserFactory.newInstance().newSAXParser().getXMLReader();
- * xmlReader.setContentHandler(unparseContentHandler)
- * xmlReader.parse(is)
- * } catch (ParserConfigurationException | SAXException e) {
- * ...
- * } catch (DaffodilUnparseErrorSAXException | DaffodilUnhandledSAXException
e) {
- * ...
- * }
- * }
- * </pre>
- *
* The call to the XMLReader.parse method must be wrapped in a try/catch, as
* {@link org.apache.daffodil.api.DaffodilUnparseContentHandler} relies on
throwing an exception to
* end processing in the case of any errors/failures.
* There are two kinds of errors to expect
* {@link
org.apache.daffodil.api.exceptions.DaffodilUnparseErrorSAXException}, for the
case when the
* {@link org.apache.daffodil.api.UnparseResult#isError()} is true, and
- * {@link org.apache.daffodil.api.exceptions.DaffodilUnhandledSAXException},
for any other errors.
+ * {@link org.apache.daffodil.api.exceptions.DaffodilUnhandledSAXException},
for any other errors,
+ * which generally indicate a bug in Daffodil
*
- * In the case of an {@link
org.apache.daffodil.api.exceptions.DaffodilUnhandledSAXException},
+ * In the case of a {@link
org.apache.daffodil.api.exceptions.DaffodilUnhandledSAXException},
* {@link
org.apache.daffodil.api.DaffodilUnparseContentHandler#getUnparseResult()} will
return null.
*
+ * After the XMLReader parse has completed, either successfully or via a
thrown exception, the
+ * {@link org.apache.daffodil.api.DaffodilUnparseContentHandler#finish()}
method should be called--this
+ * allows content handler state to be cleaned up and ensures the
+ * {@link
org.apache.daffodil.api.DaffodilUnparseContentHandler#getUnparseResult()}
returns an
+ * {@link org.apache.daffodil.api.UnparseResult} even in cases where the
XMLReader runs into an error.
+ *
* <pre>
* {@code
+ * InputSource input = ...
+ * OutputStream output = ...
+ * WritableByteChannel wbc = Channels.newChannel(output);
+ * DaffodilUnparseContentHandler unparseContentHandler =
dp.newContentHandlerInstance(wbc);
+ *
+ * XMLReader xmlReader = ...
+ * xmlReader.setContentHandler(unparseContentHandler)
* try {
- * xmlReader.parse(new InputSource(is));
- * } catch (DaffodilUnparseErrorSAXException | DaffodilUnhandledSAXException
e) {
+ * xmlReader.parse(input);
+ * } catch (DaffodilUnparseErrorSAXException e)
+ * // generally can be ignored, use getUnparseResult() instead
+ * } catch (DaffodilUnhandledSAXException e) {
+ * // should never happen, indicates a bug in daffodil
Review Comment:
Good question. The issue is that `DaffodilUnhandledSAXException` extends
SAXException, which is a checked exception so needs to either be caught or
declared as thrown. For an example, it's probably best to catch it--users can
mark it as thrown if they want, which is probably what they should do.
That said, because this exception indicates a bug in Daffodil, maybe the
correct thing to do is to change `DaffodilUnhandledSAXException` to extend
`RuntimeException` instead of `SAXException`. That way it isn't a checked
checked exception and no one needs to worry about it, and if it does occur it
can just bubble up and be handled like normal RuntimeExceptions.
And I *think* we could safely do that change without any real backwards
incompatibility issues? The only real issue is that anyone just catching
`SAXException` will no longer catch this exception, but that's probably the
right thing since it indicates a bug that should be handled differently. It's
also pretty unlikely to be thrown, since these kinds of bugs are relatively
rare.
Does that make sense? Should we switch it to a RuntimeException as part of
this PR?
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: [email protected]
For queries about this service, please contact Infrastructure at:
[email protected]
