MLE-26427 Refactoring JAXP usage #1877
Conversation
|
Copyright Validation Results ✅ Valid Files
✅ All files have valid copyright headers! |
There was a problem hiding this comment.
Pull request overview
This PR refactors JAXP usage to standardize XML parsing across the codebase by consolidating DocumentBuilderFactory creation into a single utility class. The refactoring replaces scattered instances of DocumentBuilderFactory configuration with calls to a centralized XmlFactories.getDocumentBuilderFactory() method. Additionally, a year-based fix is applied to LegalHoldsTest to prevent test failures due to date progression.
- Centralizes DocumentBuilderFactory creation in XmlFactories class with secure default configuration
- Removes duplicate DocumentBuilderFactory initialization code across test and production classes
- Updates LegalHoldsTest to address a year-progression issue
Reviewed changes
Copilot reviewed 19 out of 19 changed files in this pull request and generated 3 comments.
Show a summary per file
| File | Description |
|---|---|
| XmlFactories.java | Adds new makeNewDocumentBuilderFactory() method and getDocumentBuilderFactory() public method with secure defaults; refactors to use method references instead of anonymous classes |
| DOMHandle.java | Removes makeDocumentBuilderFactory() method and delegates to XmlFactories |
| NodeConverter.java | Removes static DocumentBuilderFactory field and getter, delegates to XmlFactories |
| DocumentMetadataHandle.java | Replaces inline DocumentBuilderFactory creation with XmlFactories call |
| Multiple test files | Replace inline DocumentBuilderFactory creation with XmlFactories calls; removes unused imports |
| LegalHoldsTest.java | Increments year roll from -10 to -11 to fix year-dependent test failure |
| DeleteSearchTest.java | Additional fix to swap assertEquals parameter order |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| } | ||
| }); | ||
| new CachedInstancePerThreadSupplier<>(XmlFactories::makeNewOutputFactory); | ||
|
|
There was a problem hiding this comment.
The new cachedDocumentBuilderFactory field should have a javadoc comment explaining its purpose, similar to how cachedOutputFactory has documentation. This helps maintain consistency in the code and clarifies why DocumentBuilderFactory instances are cached per thread.
| /** | |
| * Cached per-thread {@link DocumentBuilderFactory} instances. | |
| * <p> | |
| * Creating {@link DocumentBuilderFactory} instances is relatively expensive and they are | |
| * not guaranteed to be thread safe. To avoid repeated initialization cost while preserving | |
| * thread safety, we cache one instance per thread using {@link CachedInstancePerThreadSupplier}. | |
| * New instances are created via {@link #makeNewDocumentBuilderFactory()} when no cached | |
| * value is available for the current thread. | |
| */ |
| try { | ||
| factory.setFeature(XMLConstants.FEATURE_SECURE_PROCESSING, true); | ||
| } catch (ParserConfigurationException e) { | ||
| logger.warn("Unable to set FEATURE_SECURE_PROCESSING on DocumentBuilderFactory; cause: {}", e.getMessage()); |
There was a problem hiding this comment.
The warning message format is inconsistent with other warning messages in the same method. Lines 97, 102, 107, and 112 use feature name descriptions (e.g., 'disallow-doctype-decl'), while line 92 uses the constant name. For consistency, consider using the constant name format like 'Unable to set {} on DocumentBuilderFactory; cause: {}' with XMLConstants.FEATURE_SECURE_PROCESSING as a parameter, similar to how the logTransformerWarning method works.
| logger.warn("Unable to set FEATURE_SECURE_PROCESSING on DocumentBuilderFactory; cause: {}", e.getMessage()); | |
| logger.warn("Unable to set {} on DocumentBuilderFactory; cause: {}", XMLConstants.FEATURE_SECURE_PROCESSING, e.getMessage()); |
| // use some rework to ensure that it doesn't fail every time the year changes, but this comment is being left here | ||
| // so that if/when this does fail in the future, it'll be easy to fix. | ||
| date.roll(Calendar.YEAR, -10); | ||
| date.roll(Calendar.YEAR, -11); |
There was a problem hiding this comment.
This fix addresses an immediate issue but perpetuates the problem mentioned in the comment above (lines 88-90). The test is fragile and will continue to fail each year. Consider refactoring to use a fixed date (e.g., Calendar.getInstance(); date.set(2014, Calendar.JANUARY, 1)) or calculate the year offset dynamically based on the current year to prevent future annual failures.
rjrudin
force-pushed
the
feature/26427-refactor-dbf
branch
from
7090603 to
187fbd9
Compare
Doing this before adding support for XML exclusions in the incremental write feature. Standardizes on a single way of creating a DocumentBuilderFactory. Also made a year-change-based fix in LegalHoldsTest.
rjrudin
force-pushed
the
feature/26427-refactor-dbf
branch
from
187fbd9 to
c4ad7de
Compare
Doing this before adding support for XML exclusions in the incremental write feature. Standardizes on a single way of creating a DocumentBuilderFactory.
Also made a year-change-based fix in LegalHoldsTest.