Drop commons-io dependency in favor of JDK equivalents#7562
Merged
Conversation
Contributor
Author
|
I will keep it as draft, until I merge it after Java Modules are in master. |
Step 1 of removing the commons-io dependency. Adds private static helpers in BatchExporter for getExtension/removeExtension semantics (last-dot-in-final-segment), and uses java.io.File.getName() with a null guard in SpellsKnownTab.
Step 2 of removing commons-io. The behavior is preserved: UTF-8 encoding, System.lineSeparator() between lines, and a trailing separator after the last line — matching FileUtils.writeLines.
Covers the structural contract of the party file: two lines, version prefix, comma-separated character paths, trailing line separator, UTF-8 encoding. Verified to pass against both the old FileUtils.writeLines implementation and the new Files.writeString one.
Signed-off-by: Vest <Vest@users.noreply.github.com>
Step 3 of removing commons-io. Adds a small private helper that mirrors FileUtils.listFiles(File, String[], boolean) semantics: recursive, regular files only, case-sensitive extension match.
The original code wrapped a ByteArrayOutputStream in a TeeOutputStream so the export simultaneously fed an in-memory buffer (consumed by FOP) and an optional debug temp file. Since the bytes are fully materialised in memory before FOP runs, the tee was unnecessary: write the temp file sequentially after the export completes instead. This removes the last commons-io dependency in this file and lets us delete the internal TeeOutputStream helper.
exportCharacterToPDF inspects task.getErrorMessages() after running FOP and returns false (with a logged message) when FOP reports problems. exportPartyToPDF was missing the same check, so a party PDF that FOP failed to render would silently return true and leave the user thinking the export succeeded. Mirror the character path: log and return false when FOP produces a non-blank error message.
The previous implementation called File.list(FilenameFilter) and used
the filter lambda for its side effect — deleting matching files inside
the predicate and always returning false so list() would return an
empty array the caller then ignored. That misuse hid three real
problems:
- tf.delete() returned false silently; failures to delete were
invisible.
- The defensive try/catch around the filter caught exceptions a
FilenameFilter is not allowed to throw — pure cargo cult.
- File.list returns null when the directory does not exist; the
accidental side-effect form happened to tolerate that, but only
by coincidence.
Use listFiles to actually list the temp files matching the prefix,
guard against the null result, then iterate and delete with a logged
warning when delete fails. Drop the manual File.separator concatenation
in favour of SettingsHandler.getTempPath() (already a File).
exportCharacterToNonPDF already used StandardCharsets.UTF_8 but four sibling methods (exportPartyToNonPDF, both exportParty overloads, and the private exportCharacter overload) still passed the literal string "UTF-8" to OutputStreamWriter. The two forms behave identically but the string form forces callers to deal with UnsupportedEncodingException and lets a typo go undetected until runtime. Switch all four sites to the constant so the file is internally consistent and the encoding is checked at compile time.
removeTemporaryFiles previously used File.delete, which returns a bare boolean on failure with no indication of why the OS refused the operation — permission denied, file locked by another process, filesystem read-only, path no longer present, etc. The log entry could only say "Could not delete X", which is unactionable. Switch to Files.delete(Path), which throws an IOException carrying the underlying cause (NoSuchFileException, AccessDeniedException, or a generic IOException with the platform message). Log the exception along with the path so the operator has something to act on. Sonar S4042 flags exactly this pattern.
exportCharacterToNonPDF catches ExportException only to short-circuit to a false return; the comment "Error will already be reported to the log" makes the intent explicit. Naming the variable "e" suggested it might be used and forced a reader to scan the catch body to confirm it was not. Use the unnamed pattern (_) introduced in JEP 456 / Java 21 to declare the variable as deliberately unused. The compiler now enforces the intent and the discard is visible at the declaration site. Sonar S7467 flags exactly this case.
The original code converted the Path to a URI and then constructed a File from that URI. The intermediate URI step served no purpose: it introduced URI parsing and a second File constructor invocation while producing an equivalent File. Use Path.toFile() instead, which is the direct API for this conversion.
generateOutputFilename built the output name as
charname.substring(0, charname.lastIndexOf('.')) + '.' + extension
If charname has no dot, lastIndexOf returns -1 and substring(0, -1)
throws StringIndexOutOfBoundsException. Today this cannot happen in
practice because the only caller (exportCharacter / exportParty)
gates on PCGFile.isPCGenCharacterFile / isPCGenPartyFile, which
require .pcg or .pcp suffixes. But the guard is implicit and the
crash is a latent landmine if validation ever changes upstream.
Reuse removeFileExtension, which already handles the dotless case by
returning the input unchanged. Behaviour is identical for every name
that reaches this method today and safe for any name that might in
the future.
getValidFiles built its template filter as a chain of commons-io IOFileFilter wrappers: FileFilterUtils.asFileFilter(sheetFilter) + prefixFileFilter + and() + filterList. The SheetFilter enum already implements java.io.FilenameFilter, and the prefix check is a one-line String.startsWith — there is nothing here that needs the IOFileFilter abstraction. Combine the two checks into a single Predicate<File> lambda and run it through Stream.filter().sorted().collect(toList()) so the result matches the prior FileFilterUtils.filterList + Collections.sort semantics. This drops both commons-io.filefilter imports from the file and removes one more user of the dependency we are stripping.
SheetLoader located printable templates with FileUtils.listFiles plus a FileFilterUtils.and() chain of pdfFilter / suffixFilter / sheetFilter and a TrueFileFilter for directory descent. To do this it also implemented FilenameFilter purely so its accept() could be wrapped via FileFilterUtils.asFileFilter(this) — the class was serving as both worker and filter without need. Replace the listing with Files.walk(...) filtered by Files::isRegularFile and a single Predicate<File> that combines all three checks (parent dir name == "pdf", name does not end with .fo, name starts with the character template prefix). Drop the FilenameFilter implementation and the @NotNull import that came with it; the directory walk is recursive by default so no separate dir filter is needed. Failure mode is now explicit: an IOException from Files.walk is logged with the offending directory rather than swallowed inside commons-io. Removes the only remaining commons-io.* usage in this file.
With ExportUtilities, PrintPreviewDialog, BatchExporter and the rest of code/src/java migrated off org.apache.commons.io there are no remaining direct call sites — verified by grep across all source sets and by compiling main, test, itest and slowtest with the declarations removed. Drop the implementation declaration from build.gradle, the forceMerge entry under the jlink block, and the corresponding requires org.apache.commons.io from module-info.java. Commons-io may still be pulled transitively by FOP / batik on the runtime classpath, but our own module no longer reads it.
Rewrite the cleanup loop using java.nio.file: Files.list returns a Stream<Path>, so the temp-file scan, filter, and delete collapse into a try-with-resources block with two small helpers (isTemporaryExportFile, deleteQuietly). Behavioural improvements: - An unreadable temp directory is now logged, not silently treated as empty (the legacy listFiles returned null for both "not a directory" and "I/O error"). - A missing temp directory is handled explicitly via Files.isDirectory before listing, instead of relying on listFiles' implicit null. - Stream is closed deterministically by try-with-resources (matters on Windows file locking). This was the last in-tree usage of commons-io, so drop the dependency from build.gradle and the requires directive from module-info.java.
…me modes Replace xsl:use-attribute-sets="equipment.title" with an equivalent xsl:call-template name="attrib" call in fantasy_common.xsl (fantasy, 5e, starfinder, pathfinder_2, 4e, sagaborn) and killshot_common.xsl. Saxon-HE rejects references to undeclared attribute sets at compile time (XTSE0710); the attrib template is the correct runtime skinning mechanism. Add XsltPdfTemplateCompilationTest to guard against future regressions: compiles each fixed file directly under Saxon and fails on any XTSE0710/XTSE0720 error, ignoring expected fragment-level errors.
* Create separate action
* get the tag you created earlier
* upload zip artifacts for all platforms
* upload zip artifacts for all platforms
* Fix URIFactoryTest failing on Windows
Use File.toURI() instead of manually constructing a URI string with
new URI("file:" + path). On Windows, System.getProperty("user.dir")
returns backslash-separated paths (e.g. D:\a\pcgen\pcgen) which are
illegal characters in a URI, causing URISyntaxException.
Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
---------
Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
karianna
approved these changes
May 22, 2026
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
2 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Summary
Remove
commons-ioas a direct dependency by migrating its remaining call sites to JDK equivalents. Also fix pre-existing Saxon XSLT compilation errors exposed after the switch from Xalan to Saxon-HE.Changes
commons-io removal
BatchExporter— replacedTeeOutputStreamfan-out withByteArrayOutputStream+ sequentialFiles.write; cleaned upremoveTemporaryFiles, error reporting, and resource handling along the way.ExportUtilities.getValidFiles—IOFileFilter/FileFilterUtilschain → singlePredicate<File>+Stream.filter().sorted().PrintPreviewDialog.SheetLoader—FileUtils.listFiles+ filter chain →Files.walk+Predicate<File>; dropped the spuriousFilenameFilterself-implementation.build.gradle/module-info.java— dropped thecommons-ioimplementation declaration, thejlinkforceMergeentry, and therequires org.apache.commons.io.XSLT Saxon compatibility fixes
fantasy_common.xsl(fantasy, 5e, starfinder, pathfinder_2, 4e, sagaborn) andkillshot_common.xsl— replacedxsl:use-attribute-sets="equipment.title"with an equivalentxsl:call-template name="attrib"call. Saxon-HE rejects references to undeclared attribute sets at compile time (XTSE0710); theattribtemplate is the correct runtime skinning mechanism.XsltPdfTemplateCompilationTest— new unit test that compiles each fixed stylesheet under Saxon and fails on any XTSE0710/XTSE0720 error, guarding against future regressions.Test plan
./gradlew compileJava compileTestJava compileItestJava compileSlowtestJavapassesXsltPdfTemplateCompilationTestpasses (7/7)Print Preview is failing because of bugs not related to me.