-
-
Notifications
You must be signed in to change notification settings - Fork 1.7k
Feature/2.x/enumerating and exposing named patters #3789
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
base: 2.x
Are you sure you want to change the base?
Feature/2.x/enumerating and exposing named patters #3789
Conversation
Signed-off-by: Roy Ash <[email protected]>
Signed-off-by: Roy Ash <[email protected]>
Signed-off-by: Roy Ash <[email protected]>
Signed-off-by: Roy Ash <[email protected]>
Signed-off-by: Roy Ash <[email protected]>
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Hi @ashr123,
Thanks for the PR!
From my perspective, this refactoring does improve the readability of the decodeNamedPattern
method—cleaner logic and better structure are always welcome. That said, readability can be subjective, so I'd also like to hear what @vy (the original author) thinks about the change.
Regarding the visibility of the new NamedPattern
enum: as I mentioned in #3788 (reply in thread):
I'm not sure there's a strong case for making such an
enum
public. Unless there's a clear user-facing need, we prefer to keep our API surface area minimal.
Would it be possible to make the NamedPattern
enum package-private? Or would that conflict with the purpose of your change?
Currently, the PR triggers an API compatibility check failure:
[ERROR] Name Type Delta New Old Suggest If Prov.
[ERROR] * org.apache.logging.log4j.core.pattern PACKAGE MINOR 2.24.1 2.24.1 2.25.0 -
[ERROR] MINOR PACKAGE org.apache.logging.log4j.core.pattern
[ERROR] ADDED ENUM org.apache.logging.log4j.core.pattern.NamedPattern
This is caused by our BND Baseline mechanism, which flags any additions to the public API. If you do intend to make NamedPattern
public, you'll need to update the @Version
annotation in the package-info.java
file for the org.apache.logging.log4j.core.pattern
package. If not, making the enum package-private would avoid triggering this check.
Looking forward to your thoughts!
@ppkarwasz I see…
I'm also curious to find out what @vy thinks about it and I'll do whatever you decide |
log4j-core/src/main/java/org/apache/logging/log4j/core/pattern/NamedPattern.java
Outdated
Show resolved
Hide resolved
Signed-off-by: Roy Ash <[email protected]>
Signed-off-by: Roy Ash <[email protected]>
Signed-off-by: Roy Ash <[email protected]>
src/changelog/.2.x.x/exported_named_patterns_into_public_enum.xml
Outdated
Show resolved
Hide resolved
@ppkarwasz, I have two questions:
|
Hi @vy,
That's a convincing argument. Making the enum public ensures that any future changes to the
That could be a sensible change. I don't have a strong preference either way, but aligning with our recent naming conventions might improve clarity. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Hi @ashr123,
Thanks for the PR!
Overall, the changes look good. However, I have some concerns regarding the values returned by the public method NamedDatePattern.getPattern()
. Specifically, I think we should:
- Clearly define that this method must always return a pattern compatible with
DateTimeFormatter
. - Document this contract explicitly in the method’s Javadoc.
- If
LEGACY_FORMATTERS_ENABLED
istrue
, fall back to usingFixedDateFormat.FixedFormat
instead ofNamedDatePattern
.
Rationale
I don’t think we should expose legacy FixedFormat
patterns via the new enum, for the following reasons:
- These patterns are not compatible with any standard Java datetime formatters: not
SimpleDateFormat
,DateTimeFormatter
, nor Commons Lang’sFastDateFormat
. - They are also incompatible with our own forked
o.a.l.l.core.util.datetime.FastDateFormat
. - In practice, they only work with
FixedDateFormat
. - We want to discourage third-party usage of
FixedDateFormat
, as Log4j is not intended to serve as a general-purpose date formatting library.
log4j-core/src/main/java/org/apache/logging/log4j/core/pattern/DatePatternConverter.java
Outdated
Show resolved
Hide resolved
log4j-core/src/main/java/org/apache/logging/log4j/core/pattern/NamedDatePattern.java
Outdated
Show resolved
Hide resolved
log4j-core/src/main/java/org/apache/logging/log4j/core/pattern/NamedDatePattern.java
Outdated
Show resolved
Hide resolved
Co-authored-by: Piotr P. Karwasz <[email protected]>
@ashr123, would you mind renaming |
@vy ok, no problem, what else should I do for this PR to be approved? |
Nothing more, AFAICT. (Note that the changelog needs to be updated after the rename.) |
@vy should I use |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM, although I would add some Javadoc to the getPattern()
method.
log4j-core/src/main/java/org/apache/logging/log4j/core/pattern/NamedInstantPattern.java
Show resolved
Hide resolved
Signed-off-by: Roy Ash <[email protected]>
@ppkarwasz can you add the JavaDoc yourself? I'm using another computer and I have a bit of trouble regarding the signature |
@ashr123, no, absolutely not. They should not be used anywhere in Log4j. In |
@vy so what should I do? use |
This ties back to my earlier comment: #3789 (review) I don’t think Lines 142 to 148 in 6b311cd
|
It doesn't. In its current state, deprecated formatters are an island – no programmatic connections from anywhere. We can safely delete them, yet keep the legacy formatters configuration knob functional. Not saying we should, but the current design allows this. Supporting a legacy behavior and internal classes associated with them (of which should not have been exported, but now it is too late) are two different things. In conclusion, deprecated formatters are an island, please keep it that way. |
To decouple `DatePatternConverter` from `FixedFormat`, legacy pattern values are now provided through the `NamedInstantPattern.getLegacyPattern` method. These patterns are specific to `FixedDateFormat` and not intended for general use, so the method is kept package-private.
We can’t remove In commit 9657563, I copied the custom
|
log4j-core/src/main/java/org/apache/logging/log4j/core/pattern/DatePatternConverter.java
Show resolved
Hide resolved
To get the correct precision in the `InstantPatternLegacyFormatter` we need to replace the legacy `n` pattern letter with its `DateTimeFormatter` equivalent `S`.
This change: * Restores detailed documentation explaining the differences between `FixedDateFormat` and `DateTimeFormatter` patterns. * Implements dynamic computation of the legacy pattern based on the documented rules, ensuring consistent behavior and improved maintainability.
log4j-core/src/main/java/org/apache/logging/log4j/core/pattern/NamedInstantPattern.java
Outdated
Show resolved
Hide resolved
.../java/org/apache/logging/log4j/core/util/internal/instant/InstantPatternLegacyFormatter.java
Outdated
Show resolved
Hide resolved
@ppkarwasz, consider users having their custom instant formatters, some might even be relying on |
The fix will be provided separately.
If you're suggesting that For example, code like: DateTimeFormatter formatter = DateTimeFormatter.ofPattern(NamedInstantPattern.getPattern());
formatter.format(Instant.now()); would break if a user enables legacy formatters by setting If someone wants to use the named patterns with public static String convertToLegacyFormat(final String pattern) {
// Replace each consecutive 'S' beyond the first three with literal zeros
final Matcher matcher = Pattern.compile("(?<=SSS)S+").matcher(pattern);
final StringBuilder sb = new StringBuilder();
while (matcher.find()) {
matcher.appendReplacement(sb, "'" + "0".repeat(matcher.group().length()) + "'");
}
matcher.appendTail(sb);
// Replace 'x' with 'X'
return sb.toString().replace('x', 'X');
} This keeps the behavior explicit and avoids magic behind the scenes. If someone wants to use the named pattern with |
I've created an enum for named patterns, that way I think it will be easier to maintain.
In addition the enum modifier is
public
because it lets users to reuse those patterns in their applicationChecklist
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
./mvnw spotless:check
./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.
I got the following error:
bnd-baseline-maven-plugin failure
> ``` > [ERROR] Failed to execute goal biz.aQute.bnd:bnd-baseline-maven-plugin:7.1.0:baseline (check-api-compat) on project log4j-core: An error occurred while calculating the baseline: Baseline problems detected. See the report in /Users/royash/IdeaProjects/logging-log4j2/log4j-core/target/baseline/log4j-core-2.26.0-SNAPSHOT.txt. > [ERROR] =============================================================== > [ERROR] Name Type Delta New Old Suggest > [ERROR] org.apache.logging.log4j.core BUNDLE MAJOR 2.26.0.SNAPSHOT 2.25.0 - > [ERROR] =============================================================== > [ERROR] Name Type Delta New Old Suggest If Prov. > [ERROR] org.apache.logging.log4j.core PACKAGE UNCHANGED 2.24.2 2.24.2 ok - > [ERROR] org.apache.logging.log4j.core.appender PACKAGE MINOR 2.26.0 2.20.3 ok - > [ERROR] org.apache.logging.log4j.core.appender.db PACKAGE UNCHANGED 2.21.0 2.21.0 ok - > [ERROR] org.apache.logging.log4j.core.appender.db.jdbc PACKAGE UNCHANGED 2.20.1 2.20.1 ok - > [ERROR] org.apache.logging.log4j.core.appender.mom PACKAGE UNCHANGED 2.25.0 2.25.0 ok - > [ERROR] org.apache.logging.log4j.core.appender.mom.jeromq PACKAGE UNCHANGED 2.21.0 2.21.0 ok - > [ERROR] org.apache.logging.log4j.core.appender.mom.kafka PACKAGE UNCHANGED 2.20.1 2.20.1 ok - > [ERROR] org.apache.logging.log4j.core.appender.nosql PACKAGE UNCHANGED 2.20.1 2.20.1 ok - > [ERROR] org.apache.logging.log4j.core.appender.rewrite PACKAGE UNCHANGED 2.20.1 2.20.1 ok - > [ERROR] org.apache.logging.log4j.core.appender.rolling PACKAGE MINOR 2.26.0 2.21.1 ok - > [ERROR] org.apache.logging.log4j.core.appender.rolling.action PACKAGE MINOR 2.26.0 2.24.0 ok - > [ERROR] org.apache.logging.log4j.core.appender.routing PACKAGE MINOR 2.26.0 2.20.1 ok - > [ERROR] org.apache.logging.log4j.core.async PACKAGE MINOR 2.26.0 2.25.0 ok - > [ERROR] org.apache.logging.log4j.core.config PACKAGE MINOR 2.26.0 2.25.0 ok - > [ERROR] org.apache.logging.log4j.core.config.arbiters PACKAGE UNCHANGED 2.21.0 2.21.0 ok - > [ERROR] org.apache.logging.log4j.core.config.builder.api PACKAGE UNCHANGED 2.25.0 2.25.0 ok - > [ERROR] org.apache.logging.log4j.core.config.builder.impl PACKAGE UNCHANGED 2.25.0 2.25.0 ok - > [ERROR] org.apache.logging.log4j.core.config.composite PACKAGE UNCHANGED 2.20.1 2.20.1 ok - > [ERROR] org.apache.logging.log4j.core.config.json PACKAGE UNCHANGED 2.20.1 2.20.1 ok - > [ERROR] org.apache.logging.log4j.core.config.plugins PACKAGE UNCHANGED 2.20.1 2.20.1 ok - > [ERROR] org.apache.logging.log4j.core.config.plugins.convert PACKAGE UNCHANGED 2.24.0 2.24.0 ok - > [ERROR] org.apache.logging.log4j.core.config.plugins.processor PACKAGE UNCHANGED 2.25.0 2.25.0 ok - > [ERROR] org.apache.logging.log4j.core.config.plugins.util PACKAGE UNCHANGED 2.20.2 2.20.2 ok - > [ERROR] org.apache.logging.log4j.core.config.plugins.validation PACKAGE UNCHANGED 2.20.1 2.20.1 ok - > [ERROR] org.apache.logging.log4j.core.config.plugins.validation.constraints PACKAGE UNCHANGED 2.20.1 2.20.1 ok - > [ERROR] org.apache.logging.log4j.core.config.plugins.validation.validators PACKAGE UNCHANGED 2.20.1 2.20.1 ok - > [ERROR] org.apache.logging.log4j.core.config.plugins.visitors PACKAGE UNCHANGED 2.20.1 2.20.1 ok - > [ERROR] org.apache.logging.log4j.core.config.properties PACKAGE UNCHANGED 2.20.1 2.20.1 ok - > [ERROR] org.apache.logging.log4j.core.config.status PACKAGE UNCHANGED 2.20.2 2.20.2 ok - > [ERROR] org.apache.logging.log4j.core.config.xml PACKAGE UNCHANGED 2.20.2 2.20.2 ok - > [ERROR] org.apache.logging.log4j.core.config.yaml PACKAGE UNCHANGED 2.20.1 2.20.1 ok - > [ERROR] org.apache.logging.log4j.core.filter PACKAGE UNCHANGED 2.25.0 2.25.0 ok - > [ERROR] org.apache.logging.log4j.core.filter.mutable PACKAGE UNCHANGED 2.20.1 2.20.1 ok - > [ERROR] org.apache.logging.log4j.core.impl PACKAGE UNCHANGED 2.24.1 2.24.1 ok - > [ERROR] org.apache.logging.log4j.core.jackson PACKAGE UNCHANGED 2.24.1 2.24.1 ok - > [ERROR] org.apache.logging.log4j.core.jmx PACKAGE UNCHANGED 2.23.0 2.23.0 ok - > [ERROR] org.apache.logging.log4j.core.layout PACKAGE MINOR 2.26.0 2.25.0 ok - > [ERROR] org.apache.logging.log4j.core.lookup PACKAGE UNCHANGED 2.24.1 2.24.1 ok - > [ERROR] org.apache.logging.log4j.core.net PACKAGE UNCHANGED 2.20.3 2.20.3 ok - > [ERROR] org.apache.logging.log4j.core.net.ssl PACKAGE UNCHANGED 2.20.3 2.20.3 ok - > [ERROR] org.apache.logging.log4j.core.osgi PACKAGE UNCHANGED 2.20.1 2.20.1 ok - > [ERROR] org.apache.logging.log4j.core.parser PACKAGE UNCHANGED 2.20.1 2.20.1 ok - > [ERROR] * org.apache.logging.log4j.core.pattern PACKAGE MINOR 2.24.1 2.24.1 2.25.0 - > [ERROR] MINOR PACKAGE org.apache.logging.log4j.core.pattern > [ERROR] ADDED ENUM org.apache.logging.log4j.core.pattern.NamedPattern > [ERROR] ADDED ACCESS final > [ERROR] ADDED EXTENDS java.lang.Enum > [ERROR] ADDED IMPLEMENTS java.io.Serializable > [ERROR] ADDED IMPLEMENTS java.lang.Comparable > [ERROR] ADDED IMPLEMENTS java.lang.constant.Constable > [ERROR] ADDED FIELD ABSOLUTE > [ERROR] ADDED ACCESS final > [ERROR] ADDED ACCESS static > [ERROR] ADDED RETURN org.apache.logging.log4j.core.pattern.NamedPattern > [ERROR] ADDED FIELD ABSOLUTE_MICROS > [ERROR] ADDED ACCESS final > [ERROR] ADDED ACCESS static > [ERROR] ADDED RETURN org.apache.logging.log4j.core.pattern.NamedPattern > [ERROR] ADDED FIELD ABSOLUTE_NANOS > [ERROR] ADDED ACCESS final > [ERROR] ADDED ACCESS static > [ERROR] ADDED RETURN org.apache.logging.log4j.core.pattern.NamedPattern > [ERROR] ADDED FIELD ABSOLUTE_PERIOD > [ERROR] ADDED ACCESS final > [ERROR] ADDED ACCESS static > [ERROR] ADDED RETURN org.apache.logging.log4j.core.pattern.NamedPattern > [ERROR] ADDED FIELD COMPACT > [ERROR] ADDED ACCESS final > [ERROR] ADDED ACCESS static > [ERROR] ADDED RETURN org.apache.logging.log4j.core.pattern.NamedPattern > [ERROR] ADDED FIELD DATE > [ERROR] ADDED ACCESS final > [ERROR] ADDED ACCESS static > [ERROR] ADDED RETURN org.apache.logging.log4j.core.pattern.NamedPattern > [ERROR] ADDED FIELD DATE_PERIOD > [ERROR] ADDED ACCESS final > [ERROR] ADDED ACCESS static > [ERROR] ADDED RETURN org.apache.logging.log4j.core.pattern.NamedPattern > [ERROR] ADDED FIELD DEFAULT > [ERROR] ADDED ACCESS final > [ERROR] ADDED ACCESS static > [ERROR] ADDED RETURN org.apache.logging.log4j.core.pattern.NamedPattern > [ERROR] ADDED FIELD DEFAULT_MICROS > [ERROR] ADDED ACCESS final > [ERROR] ADDED ACCESS static > [ERROR] ADDED RETURN org.apache.logging.log4j.core.pattern.NamedPattern > [ERROR] ADDED FIELD DEFAULT_NANOS > [ERROR] ADDED ACCESS final > [ERROR] ADDED ACCESS static > [ERROR] ADDED RETURN org.apache.logging.log4j.core.pattern.NamedPattern > [ERROR] ADDED FIELD DEFAULT_PERIOD > [ERROR] ADDED ACCESS final > [ERROR] ADDED ACCESS static > [ERROR] ADDED RETURN org.apache.logging.log4j.core.pattern.NamedPattern > [ERROR] ADDED FIELD ISO8601 > [ERROR] ADDED ACCESS final > [ERROR] ADDED ACCESS static > [ERROR] ADDED RETURN org.apache.logging.log4j.core.pattern.NamedPattern > [ERROR] ADDED FIELD ISO8601_BASIC > [ERROR] ADDED ACCESS final > [ERROR] ADDED ACCESS static > [ERROR] ADDED RETURN org.apache.logging.log4j.core.pattern.NamedPattern > [ERROR] ADDED FIELD ISO8601_BASIC_PERIOD > [ERROR] ADDED ACCESS final > [ERROR] ADDED ACCESS static > [ERROR] ADDED RETURN org.apache.logging.log4j.core.pattern.NamedPattern > [ERROR] ADDED FIELD ISO8601_OFFSET_DATE_TIME_HH > [ERROR] ADDED ACCESS final > [ERROR] ADDED ACCESS static > [ERROR] ADDED RETURN org.apache.logging.log4j.core.pattern.NamedPattern > [ERROR] ADDED FIELD ISO8601_OFFSET_DATE_TIME_HHCMM > [ERROR] ADDED ACCESS final > [ERROR] ADDED ACCESS static > [ERROR] ADDED RETURN org.apache.logging.log4j.core.pattern.NamedPattern > [ERROR] ADDED FIELD ISO8601_OFFSET_DATE_TIME_HHMM > [ERROR] ADDED ACCESS final > [ERROR] ADDED ACCESS static > [ERROR] ADDED RETURN org.apache.logging.log4j.core.pattern.NamedPattern > [ERROR] ADDED FIELD ISO8601_PERIOD > [ERROR] ADDED ACCESS final > [ERROR] ADDED ACCESS static > [ERROR] ADDED RETURN org.apache.logging.log4j.core.pattern.NamedPattern > [ERROR] ADDED FIELD ISO8601_PERIOD_MICROS > [ERROR] ADDED ACCESS final > [ERROR] ADDED ACCESS static > [ERROR] ADDED RETURN org.apache.logging.log4j.core.pattern.NamedPattern > [ERROR] ADDED FIELD US_MONTH_DAY_YEAR2_TIME > [ERROR] ADDED ACCESS final > [ERROR] ADDED ACCESS static > [ERROR] ADDED RETURN org.apache.logging.log4j.core.pattern.NamedPattern > [ERROR] ADDED FIELD US_MONTH_DAY_YEAR4_TIME > [ERROR] ADDED ACCESS final > [ERROR] ADDED ACCESS static > [ERROR] ADDED RETURN org.apache.logging.log4j.core.pattern.NamedPattern > [ERROR] ADDED METHOD clone() > [ERROR] ADDED ACCESS final > [ERROR] ADDED ACCESS protected > [ERROR] ADDED RETURN java.lang.Object > [ERROR] ADDED METHOD compareTo(java.lang.Enum) > [ERROR] ADDED ACCESS final > [ERROR] ADDED RETURN int > [ERROR] ADDED METHOD compareTo(java.lang.Object) > [ERROR] ADDED ACCESS abstract > [ERROR] ADDED RETURN int > [ERROR] ADDED METHOD describeConstable() > [ERROR] ADDED ACCESS final > [ERROR] ADDED RETURN java.util.Optional > [ERROR] ADDED METHOD equals(java.lang.Object) > [ERROR] ADDED ACCESS final > [ERROR] ADDED RETURN boolean > [ERROR] ADDED METHOD finalize() > [ERROR] ADDED ACCESS final > [ERROR] ADDED ACCESS protected > [ERROR] ADDED RETURN void > [ERROR] ADDED METHOD getDeclaringClass() > [ERROR] ADDED ACCESS final > [ERROR] ADDED RETURN java.lang.Class > [ERROR] ADDED METHOD getLegacyPattern() > [ERROR] ADDED RETURN java.lang.String > [ERROR] ADDED METHOD getNonLegacyPattern() > [ERROR] ADDED RETURN java.lang.String > [ERROR] ADDED METHOD hashCode() > [ERROR] ADDED ACCESS final > [ERROR] ADDED RETURN int > [ERROR] ADDED METHOD name() > [ERROR] ADDED ACCESS final > [ERROR] ADDED RETURN java.lang.String > [ERROR] ADDED METHOD ordinal() > [ERROR] ADDED ACCESS final > [ERROR] ADDED RETURN int > [ERROR] ADDED METHOD toString() > [ERROR] ADDED RETURN java.lang.String > [ERROR] ADDED METHOD valueOf(java.lang.String) > [ERROR] ADDED ACCESS static > [ERROR] ADDED RETURN org.apache.logging.log4j.core.pattern.NamedPattern > [ERROR] ADDED METHOD values() > [ERROR] ADDED ACCESS static > [ERROR] ADDED RETURN org.apache.logging.log4j.core.pattern.NamedPattern[] > [ERROR] org.apache.logging.log4j.core.script PACKAGE MINOR 2.26.0 2.20.2 ok - > [ERROR] org.apache.logging.log4j.core.selector PACKAGE UNCHANGED 2.20.1 2.20.1 ok - > [ERROR] org.apache.logging.log4j.core.time PACKAGE UNCHANGED 2.20.1 2.20.1 ok - > [ERROR] org.apache.logging.log4j.core.tools PACKAGE UNCHANGED 2.20.1 2.20.1 ok - > [ERROR] org.apache.logging.log4j.core.util PACKAGE UNCHANGED 2.25.0 2.25.0 ok - > [ERROR] org.apache.logging.log4j.core.util.datetime PACKAGE UNCHANGED 2.21.2 2.21.2 ok - > [ERROR] org.apache.logging.log4j.core.util.internal.instant PACKAGE UNCHANGED 2.25.0 2.25.0 ok - > [ERROR] -> [Help 1] > [ERROR] > [ERROR] To see the full stack trace of the errors, re-run Maven with the -e switch. > [ERROR] Re-run Maven using the -X switch to enable full debug logging. > [ERROR] > [ERROR] For more information about the errors and possible solutions, please read the following articles: > [ERROR] [Help 1] http://cwiki.apache.org/confluence/display/MAVEN/MojoExecutionException > [ERROR] > [ERROR] After correcting the problems, you can resume the build with the command > [ERROR] mvn -rf :log4j-core > ```🧪 Tests (select one)
📝 Changelog (select one)
src/changelog/.2.x.x
. (See Changelog Entry File Guide).