WstxSAXParser error handling when used with JAXB validation

[winfriedgerlach]

Use case: JAXB unmarshalling with schema validation enabled, unmarshalling an invalid XML document
Environment: WstxSAXParser 6.6.0 for parsing, JDK 17 Xerces for validation, jaxb-runtime 4.0.4
Expected outcome: org.xml.sax.SAXParseException with a useful message (e.g., "Content of element x incomplete, y expected")
Actual outcome: java.lang.AssertionError without any useful message or cause

We are using the Woodstox SAX parser with JAXB as a drop-in replacement for JDK's default SAX parser due to better performance. We have enabled XML Schema validation for JAXB by calling Unmarshaller.setSchema(). As Woodstox does not provide a javax.xml.validation.SchemaFactory, JAXB automatically falls back to JDK's built-in Xerces for XML Schema validation.

We can indeed confirm that this setup works, XML messages are parsed and validated. Unfortunately, due to the error handling in WstxSAXParser, the helpful SAXParseException generated by Xerces during XML validation is not thrown, but instead jaxb-runtime throws an AssertionError in UnmarshallingContext.endDocument():

https://github.com/eclipse-ee4j/jaxb-ri/blob/13402b7f1f19c840206936b56ad73e693eff2463/jaxb-ri/runtime/impl/src/main/java/org/glassfish/jaxb/runtime/v2/runtime/unmarshaller/UnmarshallingContext.java#L584-L585

When looking at the relevant code in Woodstox, two things come to mind:

woodstox/src/main/java/com/ctc/wstx/sax/WstxSAXParser.java

Lines 624 to 631 in 4a6d227

	} catch (IOException io) {
	throwSaxException(io);
	} catch (XMLStreamException strex) {
	throwSaxException(strex);
	} finally {
	if (mContentHandler != null) {
	mContentHandler.endDocument();
	}

1. the SAXParseException from schema validation is not handled in the catch (might be OK?)
2. in finally, endDocument() is called, which leads to the AssertionError in UnmarshallingContext.endDocument()

If I understand JDK's JavaDoc correctly, it seems to be wrong to call endDocument() in case of an "fatal error", which may explain the diffferent behavior when using the Xerces SAX parser:

https://github.com/openjdk/jdk/blob/b419e9517361ed9d28f8ab2f5beacf5adfe3db91/src/java.xml/share/classes/org/xml/sax/ContentHandler.java#L144-L151

Could the .endDocument() call be moved away from the finally without breaking anything else? Looking forward to your opinion!

[cowtowncoder]

@winfriedgerlach From quick read your analysis sounds correct: I cannot remember why I put content handler call in finally block as that does, indeed, seem wrong.

But I'll have to think about this a bit since I usually have some reason for doing things, even if I do not immediately remember why :)
(my former self proves often surprisingly clever when I try changing things without careful thought)
I do wish that I had left a comment for everyone's benefit there tho.

[cowtowncoder]

@winfriedgerlach Created #197 for change.

Passes CI, but to be honest, SAX parser test coverage is rather low (compared to Stax side). Still, I think change makes sense and could make it into 7.0.0 release.

[winfriedgerlach]

@cowtowncoder thank you so much for the super quick reply!
This fix works for me - the behavior is now the same as with Xerces SAX, a jakarta.xml.bind.UnmarshalException being thrown with linked exception SAXParseException containing the useful detail message.

I am still not 100% sure whether it would make sense to call the error handler in case of a SAXParseException, as you do for the other exceptions (via throwSaxException())?

catch (SAXParseException sex) {
    if (mErrorHandler != null) {
        mErrorHandler.fatalError(sex);
    }
}

[cowtowncoder]

@winfriedgerlach Good point, I'll have a look later tonight.

[cowtowncoder]

@winfriedgerlach Ok, so, Woodstox parser itself does not throw SAXParseExceptions which is probably why no such catch block exists. But could such thing be thrown by registered validator?

I'll update PR with what I think makes sense here.

[cowtowncoder]

@winfriedgerlach Ok, I think I am done -- LMK if there are remaining concerns.

[winfriedgerlach]

@cowtowncoder LGTM, thank you so much!

[ThisIsMyWay]

Hello @cowtowncoder ,
is there any plan to release it?

Thanks in advance for your reply :)

[cowtowncoder]

Hmmh. Not too many things yet queued, but it has been a while.

So I'll consider releasing 7.0.0 in near future to get the fix out.

[ThisIsMyWay]

Great to hear. Thanks for your reply.

[cowtowncoder]

Versions 6.7.0 and 7.0.0 have been released, see: https://repo1.maven.org/maven2/com/fasterxml/woodstox/woodstox-core/