ppkarwasz commented on PR #53:
URL: https://github.com/apache/commons-xml/pull/53#issuecomment-5442382158
I compared this design with #54 and I prefer this one: it keeps all (or
nearly all) the code for a given JAXP factory type in a single class.
I would only change a few things:
- Move the instance methods of each wrapper into a nested `Wrapper` class,
so that the public class no longer extends the JAXP factory:
```java
public final class HardeningDocumentBuilderFactory {
public static DocumentBuilderFactory newInstance() { ... }
private static final class Wrapper extends DocumentBuilderFactory {
... current code ...
}
}
```
This way no JAXP static method is hidden, and users cannot accidentally
obtain a non-hardened factory by calling an inherited method such as
`newInstance(String, ClassLoader)` or `newDefaultInstance()`.
- (follow-up PR) Add the remaining JAXP factory methods, as you proposed.
For the Java 9 and Java 13 methods we can use `MethodHandles.publicLookup()`
rather than core reflection.
- (follow-up PR) Merge `DocumentBuilderHardener` into
`HardeningDocumentBuilderFactory`, and likewise for the other types.
- (optional PR) Rename the classes, e.g. `SafeDocumentBuilderFactory`.
I dropped the Multi-Release jar idea: users shade this library without
setting `Multi-Release: true`, then report that methods which worked in their
IDE fail in "production". We have seen plenty of such issue reports for Log4j.
--
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]
