Fix URIFactoryTest failing on Windows#7527
Merged
Merged
Conversation
…continue_gh_actions
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>
Vest
pushed a commit
to Vest/pcgen
that referenced
this pull request
May 20, 2026
* 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
added a commit
that referenced
this pull request
May 22, 2026
* Replace commons-io FilenameUtils with JDK equivalents
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.
* Replace commons-io FileUtils.writeLines with Files.writeString
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.
* Add unit tests for PCGIOHandler.write party-file format
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.
* Refactor test method visibility in PCGIOHandlerWriteTest
Signed-off-by: Vest <Vest@users.noreply.github.com>
* Replace commons-io FileUtils.listFiles with Files.walk in DataTest
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.
* Drop commons-io TeeOutputStream from BatchExporter PDF export
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.
* Report FOP errors from exportPartyToPDF
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.
* Rewrite removeTemporaryFiles to use listFiles for what it is
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).
* Use StandardCharsets.UTF_8 consistently in BatchExporter
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.
* Use Files.delete for diagnosable temp-file cleanup failures
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.
* Use unnamed pattern for ignored ExportException in catch block
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.
* Convert Path to File directly in getXMLTemplate
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.
* Guard generateOutputFilename against dotless filenames
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.
* Replace commons-io filters in ExportUtilities with JDK Predicate
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.
* Replace commons-io filters in PrintPreviewDialog with Files.walk
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.
* Drop commons-io as a direct dependency
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.
* Refactor BatchExporter.removeTemporaryFiles to NIO and drop commons-io
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.
* Fix XTSE0710 xsl:use-attribute-sets for equipment.title across all game 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.
* Fix URIFactoryTest failing on Windows (#7527)
* 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>
---------
Signed-off-by: Vest <Vest@users.noreply.github.com>
Co-authored-by: Martijn Verburg <martijnverburg@gmail.com>
Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
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
1 participant
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.
Problem
URIFactoryTest.testGetURI_WithValidRootAndOffset()fails on Windows CI runners with:The test constructed the expected URI via
new URI("file:" + System.getProperty("user.dir") + "/data/file.txt"). On Windows,user.dircontains backslashes (e.g.D:\a\pcgen\pcgen) which are illegal in URIs.Fix
Use
new File(System.getProperty("user.dir"), "data/file.txt").toURI()which produces a properly formattedfile:///URI with forward slashes on all platforms.