Add org.apache.logging.log4j.core.appender.FileManager.getPath()

[garydgregory]

Add org.apache.logging.log4j.core.appender.FileManager.getPath()

Important

Base your changes on 2.x branch if you are targeting Log4j 2; use main otherwise.

Checklist

Before we can review and merge your changes, please go through the checklist below. If you're still working on some items, feel free to submit your pull request as a draft—our CI will help guide you through the remaining steps.

✅ Required checks

• License: I confirm that my changes are submitted under the Apache License, Version 2.0.

• Commit signatures: All commits are signed and verifiable. (See GitHub Docs on Commit Signature Verification).

• Code formatting: The code is formatted according to the project’s style guide.

  How to check and fix formatting

  • To check formatting: ./mvnw spotless:check
  • To fix formatting: ./mvnw spotless:apply

  See the build instructions for details.

• Build & Test: I verified that the project builds and all unit tests pass.

  How to build the project

  Run: ./mvnw verify

  See the build instructions for details.

Note that there are still problems on Windows:

[ERROR] Failures:
[ERROR]   SyslogAppenderCustomLayoutTest>SyslogAppenderTest.testTCPAppender:56->SyslogAppenderTestBase.sendAndCheckLegacyBsdMessage:74->SyslogAppenderTestBase.checkTheNumberOfSentAndReceivedMessages:112 The number of received messages should be equal with the number of sent messages ==> expected: <1> but was: <0>
[ERROR]   RollingAppenderDeleteScriptTest.testAppender:74 target\rolling-with-delete-script\test\test-2.log should have odd index expected:<1> but was:<0>
[ERROR]   AsyncLoggersWithAsyncLoggerConfigTest.testLoggingWorks:46 Incorrect number of events. Expected 2, got 0 expected:<2> but was:<0>
[INFO]
[ERROR] Tests run: 8447, Failures: 3, Errors: 0, Skipped: 36

🧪 Tests (select one)

• I have added or updated tests to cover my changes.
• No additional tests are needed for this change.

📝 Changelog (select one)

• I added a changelog entry in src/changelog/.2.x.x. (See Changelog Entry File Guide).
• This is a trivial change and does not require a changelog entry.

[github-actions]

Job	Requested goals	Build Tool Version	Build Outcome	Build Scan®
build-macos-latest	clean install	3.9.8	✅
build-ubuntu-latest	clean install	3.9.8	✅
build-windows-latest	clean install	3.9.8	✅

Generated by gradle/develocity-actions

[vy]

@garydgregory, would you mind sharing the use case, please? What problem is this change set addressing?

[garydgregory]

The use case is that we (proprietary code base) have custom code that interacts with configurations, file Appender and Managers, we are moving from IO to NIO, and Strings shouldn't be used as an abstraction for where to find a file. Java has a nice abstraction for that: Path.

You can also see that internally the file manager also needs a Path.

HTH

[ppkarwasz]

I'm a strong advocate for using Path instead of String or File. This proposal, however, feels like an ad hoc patch that only addresses one of the file appenders and doesn't even touch on rolling file appenders.

Given that this is clearly important for your employer, would it be possible to ask them to let you work on #2117 during work hours? 🙏 I recall you volunteered for that on Slack, though it's totally understandable that tackling something of that scope in your spare time isn’t feasible, especially with all the Commons tasks already on your plate.

There are clear benefits to transitioning Log4j’s file handling from IO to NIO:

• You're primarily developing on Windows, where the low-level file access flags differ: NIO automatically sets FILE_SHARE_DELETE, which avoids many file locking issues.
• A NIO-based Log4j would better align with your employer’s own applications, after you migrate them to NIO.
• NIO opens the door to alternative FileSystemProvider implementations. This could make it possible (for example) to write logs from Unix-based servers directly to Windows shares using SMB or CIFS.
• Testing Log4j from Windows would become more reliable: many current flaky tests stem from attempts to delete files with open handles.
• This change feels incremental and uninspired. In contrast, tackling Consider replacing FileOutputStream with Files.newOutputStream #2117 would be a substantial improvement, something we could confidently showcase in the release notes for 2.26.0 or 2.27.0 (whenever the timing works for you).

[garydgregory]

Hi all,

TLDR: Let's bring this PR in and then...

Let's not confuse file Appenders and file Managers ;-)

I took a glance at the rolling file manager and it's a much larger code base. This PR is small and focused, which is a good thing. I am willing to volunteer to migrate the other manager code ((rolling) from IO to NIO in a separate PR over the weekend probably, or at the very least initially from presenting Path instead/in addition to String in the API. I can do this in a binary compatible way of course, but view is that these are internal classes up for breakage. So we'd need to agree on what kind of compatibility we want for AbstractRolloverStrategy and friends. This work would include #2117 as part of PR 2 (see below).

I want to think about the migration from IO to NIO in two steps:

• PR 1: Migrate the API: Use Path where possible instead of String and File
• PR 2: Migrate the implementation, there might be behavioral differences using Files that we only want to give users in 3.0 for example.

WDYT?

For 3.0, all bets are off and breaking BC is OK IMO in this area.

[ppkarwasz]

Hi @garydgregory,

TLDR: Let's bring this PR in and then...

This PR is small and focused, which is a good thing. I am willing to volunteer to migrate the other manager code ((rolling) from IO to NIO in a separate PR over the weekend probably, or at the very least initially from presenting Path instead/in addition to String in the API.

I believe it's actually too narrowly scoped, to the point where it loses sight of the broader context. To be candid: would anyone upgrade Log4j just because the changelog says that FileManager gained a new getPath() method? Instead of merging this in isolation, I’d suggest a slightly broader approach.

Tackling the rolling file appender is definitely hard (otherwise I wouldn’t have asked you in the first place!). I’ve actually got a local branch where I started migrating the o.a.l.l.core.appender.rolling.action.Action to NIO but it still requires a lot of work.

What would you say about:

Step 1: Fix fileName handling in the config layer

• Convert fileName from String to Path during configuration parsing (there’s already a TypeConverters.PathConverter that makes this easier).
  This would have immediate security and maintenance benefits. Every call to new File(String) or Paths.get(String) triggers a PATH_TRAVERSAL_IN warning in SpotBugs. By confining all Path creation to the appender builders, we make the codebase more robust and auditable from a security standpoint.
• Expose the Path-based filename via the APIs of all relevant file appenders: File, RandomFile, and MemoryMappedFile.

This change would provide real value worth highlighting in release notes. Would you be open to expand your PR to cover this step?

Step 2: Migrate rolling file appenders to NIO

• This is a larger task and can be handled in a follow-up PR or even after the release of Step 1.
• I’d love to bundle that with an additional feature: protection against accidental file overwrites during rollover.

[ppkarwasz]

Hi @garydgregory,

I am willing to volunteer to migrate the other manager code ((rolling) from IO to NIO in a separate PR over the weekend probably, or at the very least initially from presenting Path instead/in addition to String in the API.

I appreciate the willingness to take this on, but realistically this migration is larger than a "weekend task", especially given your already significant OSS workload. It’s important that we set expectations we can actually deliver on, both for ourselves and for the users waiting on this work. Let’s plan this in achievable steps so it doesn’t stall again.

As proposed on dev@logging (see this thread), I can take care of migrating the internal handling of the current log file to NIO. That part is already in progress and a draft exists. We can address the rolling-related logic separately in a later release since that’s a larger task.

What would really help in this PR is expanding it so that the public API is consistent across all file-based managers, not just FileManager. Right now, this PR introduces getPath() only in one class, which would result in a partial and uneven API. Users often rely on multiple appenders, and staggered API availability makes adoption confusing.

Therefore I propose to modify this PR to:

• Add the equivalent API method (getPath() or getFilePath()) to:

  • RandomAccessFileManager
  • MemoryMappedFileManager

• Think about adding such methods to FileAppender, FileAppender.Builder and similar.

That way:

• We avoid shipping a release with a half-finished API.
• The change is meaningful enough to highlight in release notes.
• Users can start adopting the new API consistently.

[ppkarwasz]

A small naming suggestion: since we already have a getFileName() method, it might be clearer to name this new method getFilePath() instead of getPath().

It would also help to document the expected characteristics of the returned Path, for example:

• Whether the returned path is absolute and normalized. Since multiple appenders (even across different logger contexts) may share a single FileManager, using an absolute, normalized Path avoids ambiguity and makes comparisons safer.
• Whether the value may change over time. In particular, RollingFileManager can update its active file during rollover (especially with the DirectWriteRolloverStrategy), so it would be useful to document that the returned path may not be stable across invocations.

[garydgregory]

A small naming suggestion: since we already have a getFileName() method, it might be clearer to name this new method getFilePath() instead of getPath().

Names are important and I'm glad we're discussing them.

IMO, "getFilePath()" is bad because it implies this is a Path to a file:// only, and since a Path is an interface for locating a file in a file system, we should not set expectations that the implementation of the Path is only for file-schemed Paths.

It doesn't matter that there's already an API called "getFileName()", a file name is very different from a Path, where the expectation (from a NIO Path POV) is that the file name is the last segment, the "bar.txt" in "/foo/bar.txt". Having a common prefix between these two methods has no benefit.

[ppkarwasz]

I was interpreting filePath as “path of the file” as opposed to the path to its parent directory or similar.

I agree that “path with file: scheme” would be the wrong interpretation, even if currently that is the only scheme we support.