Copilot commented on code in PR #95:
URL: https://github.com/apache/commons-secure-xml/pull/95#discussion_r4001073038
##########
src/main/java/org/apache/commons/xml/secure/SecureXMLFilter.java:
##########
@@ -156,6 +163,51 @@ public void parse(final InputSource input) throws
SAXException, IOException {
}
}
+ /**
+ * {@inheritDoc}
+ *
+ * <p>Builds the destination the transformation writes to, so a parse only
has to run it. A handler that is also a {@link LexicalHandler} receives the
+ * result's comments and CDATA boundaries too, the way {@link
javax.xml.transform.sax.SAXResult} expects them to be supplied.</p>
+ */
+ @Override
+ public void setContentHandler(final ContentHandler handler) {
+ super.setContentHandler(handler);
+ result = handler == null ? null : new SAXResult(handler);
+ if (handler instanceof LexicalHandler) {
+ result.setLexicalHandler((LexicalHandler) handler);
+ }
+ }
+
+ /**
+ * {@inheritDoc}
+ *
+ * <p>Wires the filter onto the new parent the way {@link
XMLFilterImpl#setupParse()} would, minus the ContentHandler: the transformer
owns the parent's
+ * content events and delivers the transformed stream to the caller's
handler through a {@link SAXResult} instead. Wiring the parent here rather than
per
+ * parse is enough because it is the filter that is installed, not the
caller's callbacks, so a callback the caller sets afterwards is still
reached.</p>
+ */
+ @Override
+ public void setParent(final XMLReader parent) {
+ super.setParent(parent);
+ // XMLFilterImpl tolerates a null parent, so do not wire one.
+ if (parent != null) {
+ parent.setEntityResolver(this);
+ parent.setDTDHandler(this);
+ parent.setErrorHandler(this);
+ }
+ }
+
+ /**
+ * Fails: events pushed into the {@link ContentHandler} role inherited
from {@link XMLFilterImpl} would reach the caller's handler untransformed.
+ *
+ * <p>The stock filters make that role inert too, by dropping the events
(Apache Xalan, the JDK) or by not implementing it at all (Saxon).</p>
+ *
+ * @throws SAXException Always.
+ */
+ @Override
+ public void startDocument() throws SAXException {
+ throw new SAXException("This XMLFilter only implements ContentHandler
for technical reasons. To push SAX events, use newTransformerHandler instead.");
Review Comment:
Only `startDocument` is overridden here. The remaining `ContentHandler`
methods inherited from `XMLFilterImpl`, such as `characters`, `startElement`,
and `endDocument`, still forward directly to `getContentHandler()`, so callers
can push SAX events into the filter and bypass the transformation despite this
class's stated contract. Reject or drop every inherited `ContentHandler`
callback, or avoid inheriting the forwarding implementation.
##########
src/main/javadoc/overview.html:
##########
@@ -315,8 +315,20 @@ <h2>Caching and Thread-Safety</h2>
<p>
There is no caching or pooling inside
<code>org.apache.commons.xml.secure</code>;
- callers on a hot path are responsible for their own caching. The
returned factories inherit the thread-safety properties of the underlying JAXP
- implementation, which in practice means they are not thread-safe.
Create a new factory per thread or synchronize externally.
+ callers on a hot path are responsible for their own caching.
Review Comment:
This paragraph still says there is no caching inside the package, but each
new filter now retains and reuses a Transformer. That leaves the overview
self-contradictory with the new filter-specific paragraph below; qualify this
statement to exclude the per-filter Transformer reuse.
--
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]
