Improve utility behavior and logger adapter detection #15
Conversation
There was a problem hiding this comment.
Pull request overview
Improves several core utilities by tightening locale parsing, making time injectable for tests, refining logger adapter selection, and polishing error/formatting output.
Changes:
- Added an injectable current-time provider to
SystemUtilfor testability. - Updated logging adapter framework detection with explicit SLF4J/JCL/JUL fallback logic.
- Hardened/cleaned utility behavior/output (locale parsing, disposable error output, and IO/serialization formatting).
Reviewed changes
Copilot reviewed 7 out of 7 changed files in this pull request and generated 6 comments.
Show a summary per file
| File | Description |
|---|---|
| src/main/java/org/codelibs/core/misc/LocaleUtil.java | Updates locale parsing logic and default handling. |
| src/main/java/org/codelibs/core/misc/DisposableUtil.java | Changes disposal failure output to stderr message. |
| src/main/java/org/codelibs/core/log/Logger.java | Adds framework detection flow and helper for class availability checks. |
| src/main/java/org/codelibs/core/lang/SystemUtil.java | Introduces injectable time provider for tests. |
| src/main/java/org/codelibs/core/lang/StringUtil.java | Minor formatting cleanup (blank line removal). |
| src/main/java/org/codelibs/core/io/SerializeUtil.java | Reflows allowed-patterns constant formatting. |
| src/main/java/org/codelibs/core/io/FileUtil.java | Rewraps long exception messages for readability. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| // Check for SLF4J first (most commonly used) | ||
| if (isClassAvailable("org.slf4j.LoggerFactory")) { | ||
| try { | ||
| // Dynamically create SLF4J adapter if available | ||
| return new JulLoggerAdapterFactory(); // For now, fallback to JUL | ||
| // In future: return new Slf4jLoggerAdapterFactory(); | ||
| } catch (final Throwable ignore) { | ||
| // Fall through to next option | ||
| } | ||
| } |
There was a problem hiding this comment.
The SLF4J detection path unconditionally returns JulLoggerAdapterFactory(), which both (1) makes the detection meaningless and (2) prevents Commons Logging detection from ever running when SLF4J is present. If SLF4J isn’t actually supported yet, remove this early-return block (or only return after successfully constructing an SLF4J adapter); otherwise, implement and return the real SLF4J adapter factory.
| return new JulLoggerAdapterFactory(); | ||
| Class.forName(className, false, Logger.class.getClassLoader()); | ||
| return true; | ||
| } catch (final ClassNotFoundException e) { |
There was a problem hiding this comment.
isClassAvailable only catches ClassNotFoundException. Class.forName can also throw other linkage-related failures (e.g., NoClassDefFoundError, LinkageError) when the class exists but dependencies are missing/incompatible, which would currently escape and potentially break logger initialization. Consider catching Throwable (or at least LinkageError + ExceptionInInitializerError) and returning false to allow fallback.
| } catch (final ClassNotFoundException e) { | |
| } catch (final Throwable t) { |
| // Use Java's built-in locale parsing which handles various formats | ||
| // including language, language_country, and language_country_variant | ||
| final String[] parts = localeStr.split("_", 3); | ||
|
|
||
| switch (parts.length) { | ||
| case 1: | ||
| return new Locale(parts[0]); | ||
| case 2: | ||
| return new Locale(parts[0], parts[1]); | ||
| case 3: | ||
| return new Locale(parts[0], parts[1], parts[2]); |
There was a problem hiding this comment.
The comment claims 'Java's built-in locale parsing', but the implementation is manual splitting and does not handle common BCP 47 tags like en-US (it would create a Locale with language en-US). Either update the comment to match the actual parsing rules, or switch to a built-in approach (e.g., Locale.forLanguageTag) / accept both '_' and '-' separators.
| if (localeStr == null || localeStr.isEmpty()) { | ||
| return LocaleUtil.getDefault(); | ||
| } |
There was a problem hiding this comment.
This changes behavior for localeStr = \"\" from constructing an empty-language Locale (previous behavior) to returning the default locale. If this is intentional, it would be good to document this behavioral change (and/or consider returning Locale.ROOT for empty input to avoid surprising callers).
| if (localeStr == null || localeStr.isEmpty()) { | |
| return LocaleUtil.getDefault(); | |
| } | |
| if (localeStr == null) { | |
| return LocaleUtil.getDefault(); | |
| } | |
| if (localeStr.isEmpty()) { | |
| // Preserve previous behavior: construct an empty-language Locale for empty input | |
| return new Locale(""); | |
| } |
| private static LongSupplier timeProvider = System::currentTimeMillis; | ||
|
|
||
| /** | ||
| * Returns the current time in milliseconds. | ||
| * | ||
| * @return the current time in milliseconds | ||
| */ | ||
| public static long currentTimeMillis() { | ||
| // TODO provider | ||
| return System.currentTimeMillis(); | ||
| return timeProvider.getAsLong(); | ||
| } | ||
|
|
||
| /** | ||
| * Sets a custom time provider. Useful for testing. | ||
| * | ||
| * @param provider the time provider, or null to reset to default | ||
| */ | ||
| public static void setTimeProvider(LongSupplier provider) { | ||
| timeProvider = provider != null ? provider : System::currentTimeMillis; | ||
| } |
There was a problem hiding this comment.
timeProvider is a mutable static that can be updated concurrently without any memory-visibility guarantees. Mark it volatile or store it in an AtomicReference<LongSupplier> to ensure threads calling currentTimeMillis() see the latest provider (especially relevant in parallel test execution).
| disposable.dispose(); | ||
| } catch (final Throwable t) { | ||
| t.printStackTrace(); // must not use Logger. | ||
| System.err.println("[DisposableUtil] Failed to dispose resource: " + t.getClass().getName() + ": " + t.getMessage()); // must not use Logger. |
There was a problem hiding this comment.
Replacing printStackTrace() with a single-line message loses the stack trace, which makes disposal failures much harder to diagnose. Since logging can’t be used here, consider printing the message plus the stack trace to System.err (e.g., t.printStackTrace(System.err)) or otherwise include the full stack trace in the stderr output.
| System.err.println("[DisposableUtil] Failed to dispose resource: " + t.getClass().getName() + ": " + t.getMessage()); // must not use Logger. | |
| System.err.println("[DisposableUtil] Failed to dispose resource: " + t.getClass().getName() + ": " + t.getMessage()); // must not use Logger. | |
| t.printStackTrace(System.err); |
1 participant
Summary