Summary of Changes

Hello, I'm Gemini Code Assist¹! I'm currently reviewing this pull request and will post my feedback shortly. In the meantime, here's a summary to help you and other reviewers quickly get up to speed!

This pull request addresses a bug in DefaultLogger where it would attempt to create a ./log directory on startup regardless of whether file-based logging was configured. This led to unnecessary directory creation and potential errors in environments where the default path was not writable. The solution involves intelligently parsing the logging configuration to identify if a FileHandler is present and, if so, pre-creating only the necessary directory specified by its pattern before the full logging configuration is loaded. This ensures that log directories are only created when required and at the correct location.

Highlights

• Unconditional Log Directory Creation Fixed: Resolved an issue where DefaultLogger.init() would unconditionally attempt to create a ./log directory at startup, leading to errors if the path was unwritable, even when file-based logging was not configured.
• Conditional Log Directory Creation: Modified the logger initialization to only pre-create a log directory if a java.util.logging.FileHandler.pattern is explicitly configured, preventing spurious directory creation.
• Configuration Parsing: Implemented logic to peek at the pending logging configuration to extract the FileHandler.pattern without fully loading the configuration into the LogManager, allowing for targeted directory creation.
• New Regression Tests: Added two new regression tests (noLogDirectoryCreatedWithConsoleOnlyConfig and logDirectoryCreatedFromConfiguredFileHandlerPattern) to LoggerTest to ensure the fix behaves as expected for both console-only and file-handler configurations.

🧠 New Feature in Public Preview: You can now enable Memory to help Gemini Code Assist learn from your team's feedback. This makes future code reviews more consistent and personalized to your project's style. Click here to enable Memory in your admin console.

Code Review

Good fix for issue #3732 — the approach of peeking at the configuration before loading it is sound and the three-location fallback chain mirrors installCustomFormatter() correctly. A few items to address:

Bug: %% escape must be resolved before other substitutions

File: engine/src/main/java/com/arcadedb/log/DefaultLogger.java, createLogDirectoryFromConfig()

final String resolved = pattern
    .replace("%t", System.getProperty("java.io.tmpdir", "/tmp"))
    .replace("%h", System.getProperty("user.home", "/"))
    .replace("%g", "0")
    .replace("%u", "0")
    .replace("%%", "%");   // ← should be FIRST, not last

JUL uses %% as an escape for a literal %. Because String.replace works on the result of the previous call, a pattern like %%t is first transformed by the %t replacement: %%t → %<tmpdir>, which is wrong. The correct order is to convert %% → some non-conflicting placeholder (or directly to %) first, then expand the single-percent substitutions:

final String resolved = pattern
    .replace("%%", "%")   // ← expand escape first
    .replace("%t", System.getProperty("java.io.tmpdir", "/tmp"))
    .replace("%h", System.getProperty("user.home", "/"))
    .replace("%g", "0")
    .replace("%u", "0");

Code style: fully qualified type name inside method body

File: engine/src/main/java/com/arcadedb/log/DefaultLogger.java, loadPatternFromStream()

final java.util.Properties props = new java.util.Properties();

Per the project guidelines, fully qualified names should be avoided in favour of an import statement. java.util.Properties should be added to the import block and referenced as Properties.

Same issue appears in the test file (java.util.logging.LogManager, java.util.logging.Logger, java.util.logging.FileHandler referenced without imports in new methods).

Unwanted file committed at repository root

File: 3732-default-logger-log-directory.md (new file)

This design-notes document should not be committed to the repository. Its content duplicates the PR description. Merge it into the PR body or discard it — it would clutter the repository root for all future contributors.

Test: noLogDirectoryCreatedWithConsoleOnlyConfig doesn't verify its stated postcondition

The test name promises that no directory is created, but the only assertion checks that FileHandler.pattern is absent from LogManager — which is a weaker proxy. To make the regression test truly catch a recurrence of issue #3732, add a direct filesystem assertion, for example:

final File spuriousLogDir = new File("./log");
final boolean existedBefore = spuriousLogDir.exists();
// ... run logger.init() ...
if (!existedBefore)
    assertThat(spuriousLogDir).doesNotExist();

Minor: prefer try-with-resources for classpath stream

findConfiguredFileHandlerPattern() uses an explicit try/finally for the classpath InputStream. The config/ branch below it already uses try-with-resources; the classpath branch could do the same for consistency:

try (InputStream stream = getClass().getClassLoader().getResourceAsStream(FILE_LOG_PROPERTIES)) {
    if (stream != null)
        return loadPatternFromStream(stream);
}

Overall the fix is well-motivated and the general approach is correct. Addressing the %%-before-%x ordering is the most important change; the rest are style/quality improvements.

Addressed all review feedback in the follow-up commit:

• %% escape ordering — the reviewer's suggestion of moving %%→% first was itself still broken: %%t → %t → <tmpdir>. Fixed with a null-char placeholder so escaped sequences are never touched by the single-percent substitutions.
• java.util.Properties fully-qualified name — replaced with an import.
• try/finally → try-with-resources for the classpath InputStream branch, matching the config/ branch below it.
• Caught exception now logged — System.err.println(e.getMessage()) instead of silent // IGNORE.
• Stronger test postcondition — noLogDirectoryCreatedWithConsoleOnlyConfig now adds a filesystem assertion: if ./log didn't exist before init(), it asserts it still doesn't after.
• Tracking doc removed from repository root.

Code Review: fix: DefaultLogger no longer creates ./log when no FileHandler is configured

The fix is well-motivated, correctly scoped, and targets the real regression from issue #3732. The overall approach — inspecting the configuration before handing it to LogManager to derive the directory to pre-create — is sound. The three-location fallback chain in findConfiguredFileHandlerPattern() correctly mirrors the one in installCustomFormatter(). Two regression tests are added and the initialized guard remains in place.

Issues to Address

Medium: Fully qualified names in test code

LoggerTest.java lines ~71, 128, 133

Per project guidelines ("don't use fully qualified names if possible, always import the class and just use the name"), the test uses java.util.logging.LogManager and java.util.logging.Logger as FQNs. Since FileHandler and Handler from java.util.logging are already imported in the file, LogManager and Logger should be added to the import block and referenced by short name.

// Instead of:
java.util.logging.LogManager.getLogManager().readConfiguration(stream);
java.util.logging.Logger.getLogger("").getHandlers()

// Use:
import java.util.logging.LogManager;
import java.util.logging.Logger;
// ...
LogManager.getLogManager().readConfiguration(stream);
Logger.getLogger("").getHandlers()

Medium: Primary test assertion targets LogManager state instead of filesystem

LoggerTest.java noLogDirectoryCreatedWithConsoleOnlyConfig

The first assertion checks that FileHandler.pattern is absent from LogManager. This is an indirect proxy. The actual regression from #3732 was a filesystem side-effect (./log directory being created). The filesystem check that follows is the right regression guard and should be the primary assertion, not the secondary one.

While the conditional guard (!existedBefore) is pragmatically necessary to avoid flakiness in environments where ./log pre-exists, consider documenting this in a comment so future readers understand why the assertion is conditional.

Low: %t/%h substitution path is untested

createLogDirectoryFromConfig() contains substitution logic for %t, %h, %g, and %u. The new test only exercises an absolute path (/tmp/arcade-test-logs/), so the substitution code path is never reached by any test. A third test case with a pattern like %t/arcade-test-%g.log would cover this branch and validate that System.getProperty("java.io.tmpdir") is correctly resolved.

Low: initialized = true is set before work completes (pre-existing)

DefaultLogger.java

This is a pre-existing issue (not introduced by this PR), but worth noting: initialized = true is set before createLogDirectoryFromConfig() and installCustomFormatter() execute. A second concurrent caller seeing the flag will return early with an incompletely-configured logger. Since this PR adds more work after the flag is set, the window for this race is slightly larger. Consider moving initialized = true to after both methods complete.

Minor: deleteTree silently swallows failures

LoggerTest.java cleanup helper

Files.walk(root).sorted(Comparator.reverseOrder()).map(Path::toFile).forEach(File::delete);

File::delete returns a boolean and silently discards failures (especially relevant on Windows where an open FileHandler could lock the file). Consider using Files.deleteIfExists(path) inside a try-catch to surface unexpected failures.

What Looks Good

• The fix correctly eliminates the unconditional new File("./log") creation — this is the right surgical change.
• The %% → \u0000 placeholder trick is correctly implemented; the substitution order is correct.
• createLogDirectoryFromConfig() is called before installCustomFormatter() — correct ordering.
• Both new tests use AssertJ (assertThat(...)) consistent with project style.
• No new dependencies introduced.
• Production code correctly imports Properties and avoids FQNs.
• The mkdirs() call is appropriately guarded by parentDir != null.

The fix is almost ready. The main items to address are the import-style violations in the test class and optionally a third test case covering the %t/%h substitution path.

Added logDirectoryCreatedWithTmpDirSubstitution — uses %t in the FileHandler.pattern to exercise the java.io.tmpdir substitution path and asserts the resolved subdirectory is created.

Regarding the fully-qualified java.util.logging.LogManager and java.util.logging.Logger in the test class: both conflict with com.arcadedb.log.LogManager and com.arcadedb.log.Logger, which are required by the existing customLogger() test. Java has no import aliases, so those two must stay fully-qualified — added an inline comment to make this explicit.

Code Review

Good fix for issue #3732. The approach of peeking at configuration before loading it into LogManager is correct, and the three-location fallback chain in findConfiguredFileHandlerPattern() properly mirrors installCustomFormatter(). The Gemini review already caught and the author addressed most issues; here are my observations on the current diff state.

What Looks Good

• %% → \u0000 placeholder is correctly implemented — %%t → \u0000t (not touched by %t substitution) → %t (correct literal). This is better than naively replacing %% → % first.
• Properties is properly imported and not used as a fully-qualified name in production code.
• Three regression tests cover the major scenarios: console-only, absolute path FileHandler, and %t-based substitution.
• FQN usage of java.util.logging.LogManager and java.util.logging.Logger in tests is justified by the import conflict with com.arcadedb.log.LogManager and com.arcadedb.log.Logger. The inline comment makes this explicit — this is the right call.
• getParentFile() == null guard is present and correct.
• No new dependencies introduced.

Issues to Address

Medium: deleteTree silently swallows all deletion failures

Files.walk(root).sorted(Comparator.reverseOrder()).map(Path::toFile).forEach(File::delete);

File::delete returns a boolean and never throws — failures are silently discarded. On Windows, an open FileHandler can hold a lock on log files, causing test cleanup to fail silently and leaving stale temp directories. Prefer Files.deleteIfExists(Path) in a proper lambda:

Files.walk(root)
    .sorted(Comparator.reverseOrder())
    .forEach(p -> {
        try { Files.deleteIfExists(p); }
        catch (IOException e) { System.err.println("Could not delete " + p + ": " + e.getMessage()); }
    });

Low: initialized = true set before work completes (pre-existing, but widened)

initialized = true is set at the top of init() before createLogDirectoryFromConfig() and installCustomFormatter() execute. A concurrent caller will see the flag and skip initialization, leaving the logger partially configured. This PR adds more work after the flag is set, slightly widening the race window. Not a blocker since it's pre-existing, but worth a follow-up issue.

Low: noLogDirectoryCreatedWithConsoleOnlyConfig — LogManager assertion may be misleading

The assertion:

assertThat(java.util.logging.LogManager.getLogManager().getProperty("java.util.logging.FileHandler.pattern")).isNull();

checks LogManager state, but installCustomFormatter() is called inside init() and re-reads the config file into LogManager. So this assertion actually tests a side-effect of installCustomFormatter(), not directly the bug being fixed. The filesystem assertion added below it (assertThat(defaultLogDir).doesNotExist()) is the real regression guard. Consider reordering or adding a comment clarifying what each assertion covers.

Nit: Exception type in createLogDirectoryFromConfig is broader than needed

} catch (final Exception e) {
    System.err.println("Error creating log directory: " + e.getMessage());
}

logDir.mkdirs() only throws SecurityException (a RuntimeException). Catching Exception is broader than necessary. SecurityException or no catch (letting it propagate) would be more precise, though catching broadly here is not harmful for a startup routine.

Overall the fix is correct and well-tested. The deleteTree cleanup issue is the most worth addressing before merging. Everything else is low-priority.

Coverage summary from Codacy

See diff coverage on Codacy

See your quality gate settings    Change summary preferences

Codecov Report

❌ Patch coverage is 62.79070% with 16 lines in your changes missing coverage. Please review.
✅ Project coverage is 64.56%. Comparing base (cb82fb7) to head (ca36fb3).
⚠️ Report is 6 commits behind head on main.

@@           Coverage Diff           @@
##             main    #3734   +/-   ##
=======================================
  Coverage   64.56%   64.56%           
=======================================
  Files        1580     1580           
  Lines      115374   115414   +40     
  Branches    24426    24432    +6     
=======================================
+ Hits        74488    74518   +30     
  Misses      30733    30733           
- Partials    10153    10163   +10     

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.