vy commented on code in PR #3338:
URL: https://github.com/apache/logging-log4j2/pull/3338#discussion_r1898947456
##########
log4j-core-test/src/test/java/org/apache/logging/log4j/core/util/internal/instant/InstantPatternDynamicFormatterTest.java:
##########
@@ -47,112 +45,81 @@ class InstantPatternDynamicFormatterTest {
@ParameterizedTest
@MethodSource("sequencingTestCases")
void sequencing_should_work(
- final String pattern, final ChronoUnit thresholdPrecision, final
List<PatternSequence> expectedSequences) {
- final List<PatternSequence> actualSequences = sequencePattern(pattern,
thresholdPrecision);
+ final String pattern,
+ final ChronoUnit thresholdPrecision,
+ final List<PatternFormatterFactory> expectedSequences) {
+ final List<PatternFormatterFactory> actualSequences =
sequencePattern(pattern, thresholdPrecision);
assertThat(actualSequences).isEqualTo(expectedSequences);
}
static List<Arguments> sequencingTestCases() {
final List<Arguments> testCases = new ArrayList<>();
// `SSSX` should be treated constant for daily updates
- testCases.add(Arguments.of("SSSX", ChronoUnit.DAYS,
singletonList(pCom(pDyn("SSS"), pDyn("X")))));
+ testCases.add(Arguments.of("SSSX", ChronoUnit.DAYS,
asList(pMilliSec(), pDyn("X"))));
// `yyyyMMddHHmmssSSSX` instant cache updated hourly
testCases.add(Arguments.of(
"yyyyMMddHHmmssSSSX",
ChronoUnit.HOURS,
- asList(
- pCom(pDyn("yyyy"), pDyn("MM"), pDyn("dd"), pDyn("HH")),
- pCom(pDyn("mm"), pDyn("ss"), pDyn("SSS")),
- pDyn("X"))));
+ asList(pDyn("yyyyMMddHH", ChronoUnit.HOURS), pDyn("mm"),
pSec("", 3), pDyn("X"))));
// `yyyyMMddHHmmssSSSX` instant cache updated per minute
testCases.add(Arguments.of(
"yyyyMMddHHmmssSSSX",
ChronoUnit.MINUTES,
- asList(
- pCom(pDyn("yyyy"), pDyn("MM"), pDyn("dd"), pDyn("HH"),
pDyn("mm")),
- pCom(pDyn("ss"), pDyn("SSS")),
- pDyn("X"))));
+ asList(pDyn("yyyyMMddHHmm", ChronoUnit.MINUTES), pSec("", 3),
pDyn("X"))));
// ISO9601 instant cache updated daily
final String iso8601InstantPattern = "yyyy-MM-dd'T'HH:mm:ss.SSSX";
testCases.add(Arguments.of(
iso8601InstantPattern,
ChronoUnit.DAYS,
asList(
- pCom(pDyn("yyyy"), pSta("-"), pDyn("MM"), pSta("-"),
pDyn("dd"), pSta("T")),
- pCom(
- pDyn("HH"),
- pSta(":"),
- pDyn("mm"),
- pSta(":"),
- pDyn("ss"),
- pSta("."),
- pDyn("SSS"),
- pDyn("X")))));
+ pDyn("yyyy'-'MM'-'dd'T'", ChronoUnit.DAYS),
+ pDyn("HH':'mm':'", ChronoUnit.MINUTES),
+ pSec(".", 3),
+ pDyn("X"))));
// ISO9601 instant cache updated per minute
testCases.add(Arguments.of(
iso8601InstantPattern,
ChronoUnit.MINUTES,
- asList(
- pCom(
- pDyn("yyyy"),
- pSta("-"),
- pDyn("MM"),
- pSta("-"),
- pDyn("dd"),
- pSta("T"),
- pDyn("HH"),
- pSta(":"),
- pDyn("mm"),
- pSta(":")),
- pCom(pDyn("ss"), pSta("."), pDyn("SSS")),
- pDyn("X"))));
+ asList(pDyn("yyyy'-'MM'-'dd'T'HH':'mm':'",
ChronoUnit.MINUTES), pSec(".", 3), pDyn("X"))));
// ISO9601 instant cache updated per second
testCases.add(Arguments.of(
iso8601InstantPattern,
ChronoUnit.SECONDS,
- asList(
- pCom(
- pDyn("yyyy"),
- pSta("-"),
- pDyn("MM"),
- pSta("-"),
- pDyn("dd"),
- pSta("T"),
- pDyn("HH"),
- pSta(":"),
- pDyn("mm"),
- pSta(":"),
- pDyn("ss"),
- pSta(".")),
- pDyn("SSS"),
- pDyn("X"))));
+ asList(pDyn("yyyy'-'MM'-'dd'T'HH':'mm':'",
ChronoUnit.MINUTES), pSec(".", 3), pDyn("X"))));
+
+ // Seconds and micros
+ testCases.add(Arguments.of(
+ "HH:mm:ss.SSSSSS", ChronoUnit.MINUTES,
asList(pDyn("HH':'mm':'", ChronoUnit.MINUTES), pSec(".", 6))));
return testCases;
}
- private static CompositePatternSequence pCom(final PatternSequence...
sequences) {
- return new CompositePatternSequence(asList(sequences));
+ private static DateTimeFormatterPatternFormatterFactory pDyn(final String
pattern) {
+ return new DateTimeFormatterPatternFormatterFactory(pattern);
+ }
+
+ private static DateTimeFormatterPatternFormatterFactory pDyn(final String
pattern, final ChronoUnit precision) {
+ return new DateTimeFormatterPatternFormatterFactory(pattern,
precision);
}
- private static DynamicPatternSequence pDyn(final String pattern) {
- return new DynamicPatternSequence(pattern);
+ private static SecondPatternFormatterFactory pSec(String separator, int
fractionalDigits) {
+ return new SecondPatternFormatterFactory(true, separator,
fractionalDigits);
}
- private static StaticPatternSequence pSta(final String literal) {
- return new StaticPatternSequence(literal);
+ private static SecondPatternFormatterFactory pMilliSec() {
+ return new SecondPatternFormatterFactory(false, "", 3);
}
@ParameterizedTest
@ValueSource(
strings = {
// Basics
- "S",
Review Comment:
Why did we remove this case?
##########
log4j-core/src/main/java/org/apache/logging/log4j/core/util/internal/instant/InstantPatternDynamicFormatter.java:
##########
@@ -641,32 +411,39 @@ public TimeZone getTimeZone() {
}
}
- abstract static class PatternSequence {
+ /**
+ * Creates an {@link InstantPatternFormatter}.
+ */
+ abstract static class PatternFormatterFactory {
final String pattern;
final ChronoUnit precision;
@SuppressWarnings("ReturnValueIgnored")
- PatternSequence(final String pattern, final ChronoUnit precision) {
+ PatternFormatterFactory(final String pattern, final ChronoUnit
precision) {
DateTimeFormatter.ofPattern(pattern); // Validate the pattern
this.pattern = pattern;
this.precision = precision;
}
- InstantPatternFormatter createFormatter(final Locale locale, final
TimeZone timeZone) {
- final DateTimeFormatter dateTimeFormatter =
- DateTimeFormatter.ofPattern(pattern,
locale).withZone(timeZone.toZoneId());
- return new AbstractFormatter(pattern, locale, timeZone, precision)
{
- @Override
- public void formatTo(final StringBuilder buffer, final Instant
instant) {
- final TemporalAccessor instantAccessor =
toTemporalAccessor(instant);
- dateTimeFormatter.formatTo(instantAccessor, buffer);
- }
- };
+ abstract InstantPatternFormatter createFormatter(final Locale locale,
final TimeZone timeZone);
+
+ /**
+ * Tries to merge two factories.
+ *
+ * @param other A pattern formatter factory.
+ * @param thresholdPrecision The time interval during which the
instant formatters created by this factory will
+ * be used.
Review Comment:
I don't understand what this sentence is saying.
##########
log4j-core/src/main/java/org/apache/logging/log4j/core/util/internal/instant/InstantPatternDynamicFormatter.java:
##########
@@ -711,23 +488,114 @@ public void formatTo(final StringBuilder buffer, final
Instant instant) {
}
};
}
+
+ @Override
+ @Nullable
+ PatternFormatterFactory tryMerge(PatternFormatterFactory other,
ChronoUnit thresholdPrecision) {
+ // We always merge consecutive static pattern factories
+ if (other instanceof StaticPatternFormatterFactory) {
+ final StaticPatternFormatterFactory otherStatic =
(StaticPatternFormatterFactory) other;
+ return new StaticPatternFormatterFactory(this.literal +
otherStatic.literal);
+ }
+ // We also merge a static pattern factory with a DTF factory
+ if (other instanceof DateTimeFormatterPatternFormatterFactory) {
+ final DateTimeFormatterPatternFormatterFactory otherDtf =
+ (DateTimeFormatterPatternFormatterFactory) other;
+ return new DateTimeFormatterPatternFormatterFactory(
+ this.pattern + otherDtf.pattern, otherDtf.precision);
+ }
+ return super.tryMerge(other, thresholdPrecision);
+ }
+
+ static String escapeLiteral(String literal) {
+ StringBuilder sb = new StringBuilder(literal.length() + 2);
+ boolean inSingleQuotes = false;
+ for (int i = 0; i < literal.length(); i++) {
+ char c = literal.charAt(i);
+ if (c == '\'') {
+ if (inSingleQuotes) {
+ sb.append("'");
+ }
+ inSingleQuotes = false;
+ sb.append("''");
+ } else {
+ if (!inSingleQuotes) {
+ sb.append("'");
+ }
+ inSingleQuotes = true;
+ sb.append(c);
+ }
+ }
+ if (inSingleQuotes) {
+ sb.append("'");
+ }
+ return sb.toString();
+ }
}
- static final class DynamicPatternSequence extends PatternSequence {
+ /**
+ * Formats the pattern using {@link DateTimeFormatter}.
+ */
+ static final class DateTimeFormatterPatternFormatterFactory extends
PatternFormatterFactory {
- DynamicPatternSequence(final String content) {
- super(content, contentPrecision(content));
+ /**
+ * @param simplePattern A {@link DateTimeFormatter} pattern containing
a single letter.
Review Comment:
The usage of _simple_ is really dangerous here, IMHO, since it pretty much
references to the `SimpleDateFormat`, which we are trying to abolish here.
##########
log4j-core-test/src/test/java/org/apache/logging/log4j/core/util/internal/instant/InstantPatternDynamicFormatterTest.java:
##########
@@ -47,112 +45,81 @@ class InstantPatternDynamicFormatterTest {
@ParameterizedTest
@MethodSource("sequencingTestCases")
void sequencing_should_work(
- final String pattern, final ChronoUnit thresholdPrecision, final
List<PatternSequence> expectedSequences) {
- final List<PatternSequence> actualSequences = sequencePattern(pattern,
thresholdPrecision);
+ final String pattern,
+ final ChronoUnit thresholdPrecision,
+ final List<PatternFormatterFactory> expectedSequences) {
+ final List<PatternFormatterFactory> actualSequences =
sequencePattern(pattern, thresholdPrecision);
assertThat(actualSequences).isEqualTo(expectedSequences);
}
static List<Arguments> sequencingTestCases() {
Review Comment:
Mind adding a test case only with a literal, please?
##########
log4j-core-test/src/test/java/org/apache/logging/log4j/core/util/internal/instant/InstantPatternDynamicFormatterTest.java:
##########
@@ -163,8 +130,7 @@ private static StaticPatternSequence pSta(final String
literal) {
"yyyy-MM-dd HH:mm:ss,SSSSSSS",
"yyyy-MM-dd HH:mm:ss,SSSSSSSS",
"yyyy-MM-dd HH:mm:ss,SSSSSSSSS",
- "yyyy-MM-dd'T'HH:mm:ss.SSSSSSSSS",
- "yyyy-MM-dd'T'HH:mm:ss.SXXX"
Review Comment:
Why did we remove this case?
##########
log4j-core/src/main/java/org/apache/logging/log4j/core/util/internal/instant/InstantPatternDynamicFormatter.java:
##########
@@ -641,32 +411,39 @@ public TimeZone getTimeZone() {
}
}
- abstract static class PatternSequence {
+ /**
+ * Creates an {@link InstantPatternFormatter}.
+ */
+ abstract static class PatternFormatterFactory {
Review Comment:
@ppkarwasz, I prefer the old name `PatternSequence`, because
* I borrowed the `sequence` term from _DNA sequencing_, which, IMHO, fits
this use case pretty well.
* Sequence captures the intent that the content is a _sequence_ of
formatting patterns. That is, it is not just a _formatter factory_, but more:
`tryMerge()`, `isConstantForDurationOf()`, etc.
* `pattern` and `formatter` keywords are already overused. Sequence stands
out easily. (That is, it will help with avoiding mind-bending terms like
`DateTimeFormatterPatternFormatterFactory`.)
If you prefer to _not_ use the sequence term, please rewrite several
mentions of _sequence_. Right now it is extremely confusing: You removed the
sequence notion, yet you refer to it in docs and method/variable names.
##########
log4j-core/src/main/java/org/apache/logging/log4j/core/util/internal/instant/InstantPatternDynamicFormatter.java:
##########
@@ -218,17 +222,27 @@ public void formatTo(final StringBuilder buffer, final
Instant instant) {
}
}
- static List<PatternSequence> sequencePattern(final String pattern, final
ChronoUnit precisionThreshold) {
- List<PatternSequence> sequences = sequencePattern(pattern);
- final List<PatternSequence> mergedSequences =
mergeDynamicSequences(sequences, precisionThreshold);
- return mergeConsequentEffectivelyConstantSequences(mergedSequences,
precisionThreshold);
+ static List<PatternFormatterFactory> sequencePattern(final String pattern,
final ChronoUnit precisionThreshold) {
+ List<PatternFormatterFactory> sequences = sequencePattern(pattern, (l,
p) -> {
+ switch (l) {
+ case 's':
+ return new SecondPatternFormatterFactory(true, "", 0);
+ case 'S':
+ return new SecondPatternFormatterFactory(false, "",
p.length());
+ default:
+ return new DateTimeFormatterPatternFormatterFactory(p);
+ }
+ });
+ return mergeFactories(sequences, precisionThreshold);
}
- private static List<PatternSequence> sequencePattern(final String pattern)
{
+ static List<PatternFormatterFactory> sequencePattern(
Review Comment:
Can this be `private`? (Couldn't spot usages in the test.)
##########
log4j-core/src/main/java/org/apache/logging/log4j/core/util/internal/instant/InstantPatternDynamicFormatter.java:
##########
Review Comment:
`Implementation note` in the class javadoc (regarding the garbage-free
aspects) needs to be removed.
##########
log4j-core-test/src/test/java/org/apache/logging/log4j/core/util/internal/instant/InstantPatternDynamicFormatterTest.java:
##########
@@ -351,7 +317,9 @@ static Stream<Arguments> formatterInputs(final String
pattern) {
private static MutableInstant randomInstant() {
final MutableInstant instant = new MutableInstant();
- final long epochSecond = RANDOM.nextInt(1_621_280_470); // 2021-05-17
21:41:10
+ // In the 1970's some time zones had sub-minute offsets to UTC, e.g.,
Africa/Monrovia
+ final int startEighties = 315_532_800;
+ final long epochSecond = startEighties + RANDOM.nextInt(1_621_280_470
- startEighties); // 2021-05-17 21:41:10
Review Comment:
```suggestion
// In the 1970's some time zones had sub-minute offsets to UTC,
e.g., Africa/Monrovia.
// We will exclude them for tests:
final long minEpochSecond = 315_532_800; // 1980-01-01 01:00:00
final long maxEpochSecond = 1_621_280_470; // 2021-05-17 21:41:10
final long epochSecond = RANDOM.nextLong(minEpochSecond,
maxEpochSecond);
```
##########
log4j-core/src/main/java/org/apache/logging/log4j/core/util/internal/instant/InstantPatternDynamicFormatter.java:
##########
@@ -689,16 +466,16 @@ public int hashCode() {
@Override
public String toString() {
- return String.format("<%s>%s", pattern, precision);
+ return getClass().getSimpleName() + "[" + "pattern='" + pattern +
'\'' + ", precision=" + precision + ']';
Review Comment:
I would prefer the old shorter version, which was introduced to make the
view comprehensible in the debugger IDE window. I had found it pretty handy
when multiple sequences were in play, which is the case most of the time.
##########
log4j-core/src/main/java/org/apache/logging/log4j/core/util/internal/instant/InstantPatternDynamicFormatter.java:
##########
@@ -218,17 +222,27 @@ public void formatTo(final StringBuilder buffer, final
Instant instant) {
}
}
- static List<PatternSequence> sequencePattern(final String pattern, final
ChronoUnit precisionThreshold) {
- List<PatternSequence> sequences = sequencePattern(pattern);
- final List<PatternSequence> mergedSequences =
mergeDynamicSequences(sequences, precisionThreshold);
- return mergeConsequentEffectivelyConstantSequences(mergedSequences,
precisionThreshold);
+ static List<PatternFormatterFactory> sequencePattern(final String pattern,
final ChronoUnit precisionThreshold) {
+ List<PatternFormatterFactory> sequences = sequencePattern(pattern, (l,
p) -> {
+ switch (l) {
+ case 's':
+ return new SecondPatternFormatterFactory(true, "", 0);
+ case 'S':
+ return new SecondPatternFormatterFactory(false, "",
p.length());
+ default:
+ return new DateTimeFormatterPatternFormatterFactory(p);
Review Comment:
Why do we have this branching here? Shouldn't this be the responsibility of
the (other) `sequencePattern()` method? That is, shouldn't we have this
branching on line 257 instead?
##########
log4j-core/src/main/java/org/apache/logging/log4j/core/util/internal/instant/InstantPatternDynamicFormatter.java:
##########
@@ -256,131 +270,36 @@ else if (c == '\'') {
}
final String sequenceLiteral =
(startIndex + 1) == endIndex ? "'" :
pattern.substring(startIndex + 1, endIndex);
- final PatternSequence sequence = new
StaticPatternSequence(sequenceLiteral);
+ final PatternFormatterFactory sequence = new
StaticPatternFormatterFactory(sequenceLiteral);
sequences.add(sequence);
startIndex = endIndex + 1;
}
// Handle unknown literal
else {
- final PatternSequence sequence = new StaticPatternSequence(""
+ c);
+ final PatternFormatterFactory sequence = new
StaticPatternFormatterFactory("" + c);
sequences.add(sequence);
startIndex++;
}
}
- return mergeConsequentStaticPatternSequences(sequences);
+ return sequences;
}
private static boolean isDynamicPatternLetter(final char c) {
return "GuyDMLdgQqYwWEecFaBhKkHmsSAnNVvzOXxZ".indexOf(c) >= 0;
}
/**
- * Merges consequent static sequences.
+ * Merges sequences of formatter factories using {@link
PatternFormatterFactory#tryMerge}.
*
* <p>
- * For example, the sequencing of the {@code [MM-dd] HH:mm} pattern will
create two static sequences for {@code ]} (right brace) and {@code }
(whitespace) characters.
- * This method will combine such consequent static sequences into one.
+ * For example, given the {@code yyyy-MM-dd'T'HH:mm:ss.SSS} pattern and a
precision threshold of {@link ChronoUnit#MINUTES},
+ * this method will combine sequences associated with {@code
yyyy-MM-dd'T'HH:mm:} into a single sequence,
+ * since these are consequent and effectively constant sequences.
Review Comment:
This patch removes _sequence_, _consequent_, and _effectively constant_
keywords from all definitions, yet uses them in the docs without explaining
what they refer to and sometimes even incorrectly, e.g., `this method will
combine sequences`, though no such thing as a _sequence_ anymore. I think,
wherever possible,
* Old self-explanatory method names be should be restored
* Old docs should be restored
* Old examples shared in the docs should be restored
##########
log4j-core/src/main/java/org/apache/logging/log4j/core/util/internal/instant/InstantPatternDynamicFormatter.java:
##########
@@ -602,7 +372,7 @@ private static TemporalAccessor toTemporalAccessor(final
Instant instant) {
: java.time.Instant.ofEpochSecond(instant.getEpochSecond(),
instant.getNanoOfSecond());
}
- private abstract static class AbstractFormatter implements
InstantPatternFormatter {
+ abstract static class AbstractFormatter implements InstantPatternFormatter
{
Review Comment:
Can't this be `private`?
##########
log4j-core/src/main/java/org/apache/logging/log4j/core/util/internal/instant/InstantPatternDynamicFormatter.java:
##########
@@ -711,23 +488,114 @@ public void formatTo(final StringBuilder buffer, final
Instant instant) {
}
};
}
+
+ @Override
+ @Nullable
+ PatternFormatterFactory tryMerge(PatternFormatterFactory other,
ChronoUnit thresholdPrecision) {
+ // We always merge consecutive static pattern factories
+ if (other instanceof StaticPatternFormatterFactory) {
+ final StaticPatternFormatterFactory otherStatic =
(StaticPatternFormatterFactory) other;
+ return new StaticPatternFormatterFactory(this.literal +
otherStatic.literal);
+ }
+ // We also merge a static pattern factory with a DTF factory
+ if (other instanceof DateTimeFormatterPatternFormatterFactory) {
+ final DateTimeFormatterPatternFormatterFactory otherDtf =
+ (DateTimeFormatterPatternFormatterFactory) other;
+ return new DateTimeFormatterPatternFormatterFactory(
+ this.pattern + otherDtf.pattern, otherDtf.precision);
+ }
+ return super.tryMerge(other, thresholdPrecision);
+ }
+
+ static String escapeLiteral(String literal) {
+ StringBuilder sb = new StringBuilder(literal.length() + 2);
+ boolean inSingleQuotes = false;
+ for (int i = 0; i < literal.length(); i++) {
+ char c = literal.charAt(i);
+ if (c == '\'') {
+ if (inSingleQuotes) {
+ sb.append("'");
+ }
+ inSingleQuotes = false;
+ sb.append("''");
+ } else {
+ if (!inSingleQuotes) {
+ sb.append("'");
+ }
+ inSingleQuotes = true;
+ sb.append(c);
+ }
+ }
+ if (inSingleQuotes) {
+ sb.append("'");
+ }
+ return sb.toString();
+ }
}
- static final class DynamicPatternSequence extends PatternSequence {
+ /**
+ * Formats the pattern using {@link DateTimeFormatter}.
+ */
+ static final class DateTimeFormatterPatternFormatterFactory extends
PatternFormatterFactory {
- DynamicPatternSequence(final String content) {
- super(content, contentPrecision(content));
+ /**
+ * @param simplePattern A {@link DateTimeFormatter} pattern containing
a single letter.
+ */
+ DateTimeFormatterPatternFormatterFactory(final String simplePattern) {
+ this(simplePattern, patternPrecision(simplePattern));
}
/**
- * @param content a single-letter directive content complying (e.g.,
{@code H}, {@code HH}, or {@code pHH})
- * @return the time precision of the directive
+ * @param pattern Any {@link DateTimeFormatter} pattern.
+ * @param precision The maximum interval of time over which this
pattern is constant.
*/
+ DateTimeFormatterPatternFormatterFactory(final String pattern, final
ChronoUnit precision) {
+ super(pattern, precision);
+ }
+
+ InstantPatternFormatter createFormatter(final Locale locale, final
TimeZone timeZone) {
+ final DateTimeFormatter dateTimeFormatter =
+ DateTimeFormatter.ofPattern(pattern,
locale).withZone(timeZone.toZoneId());
+ return new AbstractFormatter(pattern, locale, timeZone, precision)
{
+ @Override
+ public void formatTo(final StringBuilder buffer, final Instant
instant) {
+ final TemporalAccessor instantAccessor =
toTemporalAccessor(instant);
+ dateTimeFormatter.formatTo(instantAccessor, buffer);
+ }
+ };
+ }
+
+ @Override
@Nullable
- private static ChronoUnit contentPrecision(final String content) {
+ PatternFormatterFactory tryMerge(PatternFormatterFactory other,
ChronoUnit thresholdPrecision) {
+ // We merge two DTF factories if they are both above or below the
threshold
+ if (other instanceof DateTimeFormatterPatternFormatterFactory) {
+ final DateTimeFormatterPatternFormatterFactory otherDtf =
+ (DateTimeFormatterPatternFormatterFactory) other;
+ if (isConstantForDurationOf(thresholdPrecision)
+ ==
otherDtf.isConstantForDurationOf(thresholdPrecision)) {
+ ChronoUnit precision =
this.precision.getDuration().compareTo(otherDtf.precision.getDuration()) < 0
+ ? this.precision
+ : otherDtf.precision;
+ return new
DateTimeFormatterPatternFormatterFactory(this.pattern + otherDtf.pattern,
precision);
+ }
+ }
+ // We merge a static pattern factory
+ if (other instanceof StaticPatternFormatterFactory) {
+ final StaticPatternFormatterFactory otherStatic =
(StaticPatternFormatterFactory) other;
+ return new
DateTimeFormatterPatternFormatterFactory(this.pattern + otherStatic.pattern,
this.precision);
+ }
+ return super.tryMerge(other, thresholdPrecision);
+ }
- validateContent(content);
- final String paddingRemovedContent = removePadding(content);
+ /**
+ * @param simplePattern a single-letter directive simplePattern
complying (e.g., {@code H}, {@code HH}, or {@code pHH})
Review Comment:
Please don't use the word _simple_.
##########
log4j-core/src/main/java/org/apache/logging/log4j/core/util/internal/instant/InstantPatternDynamicFormatter.java:
##########
@@ -711,23 +488,114 @@ public void formatTo(final StringBuilder buffer, final
Instant instant) {
}
};
}
+
+ @Override
+ @Nullable
+ PatternFormatterFactory tryMerge(PatternFormatterFactory other,
ChronoUnit thresholdPrecision) {
+ // We always merge consecutive static pattern factories
+ if (other instanceof StaticPatternFormatterFactory) {
+ final StaticPatternFormatterFactory otherStatic =
(StaticPatternFormatterFactory) other;
+ return new StaticPatternFormatterFactory(this.literal +
otherStatic.literal);
+ }
+ // We also merge a static pattern factory with a DTF factory
+ if (other instanceof DateTimeFormatterPatternFormatterFactory) {
+ final DateTimeFormatterPatternFormatterFactory otherDtf =
+ (DateTimeFormatterPatternFormatterFactory) other;
+ return new DateTimeFormatterPatternFormatterFactory(
+ this.pattern + otherDtf.pattern, otherDtf.precision);
+ }
+ return super.tryMerge(other, thresholdPrecision);
+ }
+
+ static String escapeLiteral(String literal) {
+ StringBuilder sb = new StringBuilder(literal.length() + 2);
+ boolean inSingleQuotes = false;
+ for (int i = 0; i < literal.length(); i++) {
+ char c = literal.charAt(i);
+ if (c == '\'') {
+ if (inSingleQuotes) {
+ sb.append("'");
+ }
+ inSingleQuotes = false;
+ sb.append("''");
+ } else {
+ if (!inSingleQuotes) {
+ sb.append("'");
+ }
+ inSingleQuotes = true;
+ sb.append(c);
+ }
+ }
+ if (inSingleQuotes) {
+ sb.append("'");
+ }
+ return sb.toString();
+ }
}
- static final class DynamicPatternSequence extends PatternSequence {
+ /**
+ * Formats the pattern using {@link DateTimeFormatter}.
Review Comment:
`Formats...` No, it doesn't – it _creates_ the formatter.
##########
log4j-core/src/main/java/org/apache/logging/log4j/core/util/internal/instant/InstantPatternDynamicFormatter.java:
##########
@@ -515,83 +326,42 @@ private static List<PatternSequence>
mergeDynamicSequences(
*
* <pre>{@code
* [
- * composite(
- * sequences=[
- * dynamic(pattern="yyyy", precision=YEARS),
- * static(literal="-"),
- * dynamic(pattern="MM", precision=MONTHS),
- * static(literal="-"),
- * dynamic(pattern="dd", precision=DAYS),
- * static(literal="T"),
- * dynamic(pattern="HH", precision=HOURS),
- * static(literal=":"),
- * dynamic(pattern="mm", precision=MINUTES),
- * static(literal=":")
- * ],
- * precision=MINUTES),
- * composite(
- * sequences=[
- * dynamic(pattern="ss", precision=SECONDS),
- * static(literal="."),
- * dynamic(pattern="SSS", precision=MILLISECONDS)
- * ],
- * precision=MILLISECONDS),
- * dynamic(pattern="X", precision=HOURS),
+ * dynamic(pattern="yyyy-MM-dd'T'HH:mm", precision=MINUTES),
Review Comment:
Since you moved the monolithic merging logic to individual class methods in
`IpfFactory`, you cannot make any claims on the merging behavior anymore. This
voids the entire javadoc of this method.
##########
log4j-core/src/main/java/org/apache/logging/log4j/core/util/internal/instant/InstantPatternDynamicFormatter.java:
##########
@@ -711,23 +488,114 @@ public void formatTo(final StringBuilder buffer, final
Instant instant) {
}
};
}
+
+ @Override
+ @Nullable
+ PatternFormatterFactory tryMerge(PatternFormatterFactory other,
ChronoUnit thresholdPrecision) {
Review Comment:
Docs are missing.
##########
log4j-core/src/main/java/org/apache/logging/log4j/core/util/internal/instant/InstantPatternDynamicFormatter.java:
##########
@@ -711,23 +488,114 @@ public void formatTo(final StringBuilder buffer, final
Instant instant) {
}
};
}
+
+ @Override
+ @Nullable
+ PatternFormatterFactory tryMerge(PatternFormatterFactory other,
ChronoUnit thresholdPrecision) {
+ // We always merge consecutive static pattern factories
+ if (other instanceof StaticPatternFormatterFactory) {
+ final StaticPatternFormatterFactory otherStatic =
(StaticPatternFormatterFactory) other;
+ return new StaticPatternFormatterFactory(this.literal +
otherStatic.literal);
+ }
+ // We also merge a static pattern factory with a DTF factory
+ if (other instanceof DateTimeFormatterPatternFormatterFactory) {
+ final DateTimeFormatterPatternFormatterFactory otherDtf =
+ (DateTimeFormatterPatternFormatterFactory) other;
+ return new DateTimeFormatterPatternFormatterFactory(
+ this.pattern + otherDtf.pattern, otherDtf.precision);
+ }
+ return super.tryMerge(other, thresholdPrecision);
+ }
+
+ static String escapeLiteral(String literal) {
+ StringBuilder sb = new StringBuilder(literal.length() + 2);
+ boolean inSingleQuotes = false;
+ for (int i = 0; i < literal.length(); i++) {
+ char c = literal.charAt(i);
+ if (c == '\'') {
+ if (inSingleQuotes) {
+ sb.append("'");
+ }
+ inSingleQuotes = false;
+ sb.append("''");
+ } else {
+ if (!inSingleQuotes) {
+ sb.append("'");
+ }
+ inSingleQuotes = true;
+ sb.append(c);
+ }
+ }
+ if (inSingleQuotes) {
+ sb.append("'");
+ }
+ return sb.toString();
+ }
}
- static final class DynamicPatternSequence extends PatternSequence {
+ /**
+ * Formats the pattern using {@link DateTimeFormatter}.
+ */
+ static final class DateTimeFormatterPatternFormatterFactory extends
PatternFormatterFactory {
- DynamicPatternSequence(final String content) {
- super(content, contentPrecision(content));
+ /**
+ * @param simplePattern A {@link DateTimeFormatter} pattern containing
a single letter.
+ */
+ DateTimeFormatterPatternFormatterFactory(final String simplePattern) {
+ this(simplePattern, patternPrecision(simplePattern));
}
/**
- * @param content a single-letter directive content complying (e.g.,
{@code H}, {@code HH}, or {@code pHH})
- * @return the time precision of the directive
+ * @param pattern Any {@link DateTimeFormatter} pattern.
+ * @param precision The maximum interval of time over which this
pattern is constant.
*/
+ DateTimeFormatterPatternFormatterFactory(final String pattern, final
ChronoUnit precision) {
+ super(pattern, precision);
+ }
+
+ InstantPatternFormatter createFormatter(final Locale locale, final
TimeZone timeZone) {
+ final DateTimeFormatter dateTimeFormatter =
+ DateTimeFormatter.ofPattern(pattern,
locale).withZone(timeZone.toZoneId());
+ return new AbstractFormatter(pattern, locale, timeZone, precision)
{
+ @Override
+ public void formatTo(final StringBuilder buffer, final Instant
instant) {
+ final TemporalAccessor instantAccessor =
toTemporalAccessor(instant);
+ dateTimeFormatter.formatTo(instantAccessor, buffer);
+ }
+ };
+ }
+
+ @Override
@Nullable
- private static ChronoUnit contentPrecision(final String content) {
+ PatternFormatterFactory tryMerge(PatternFormatterFactory other,
ChronoUnit thresholdPrecision) {
Review Comment:
Docs are missing
##########
log4j-core/src/main/java/org/apache/logging/log4j/core/util/internal/instant/InstantPatternDynamicFormatter.java:
##########
@@ -711,23 +488,114 @@ public void formatTo(final StringBuilder buffer, final
Instant instant) {
}
};
}
+
+ @Override
+ @Nullable
+ PatternFormatterFactory tryMerge(PatternFormatterFactory other,
ChronoUnit thresholdPrecision) {
+ // We always merge consecutive static pattern factories
+ if (other instanceof StaticPatternFormatterFactory) {
+ final StaticPatternFormatterFactory otherStatic =
(StaticPatternFormatterFactory) other;
+ return new StaticPatternFormatterFactory(this.literal +
otherStatic.literal);
+ }
+ // We also merge a static pattern factory with a DTF factory
+ if (other instanceof DateTimeFormatterPatternFormatterFactory) {
+ final DateTimeFormatterPatternFormatterFactory otherDtf =
+ (DateTimeFormatterPatternFormatterFactory) other;
+ return new DateTimeFormatterPatternFormatterFactory(
+ this.pattern + otherDtf.pattern, otherDtf.precision);
+ }
+ return super.tryMerge(other, thresholdPrecision);
+ }
+
+ static String escapeLiteral(String literal) {
Review Comment:
This method used in multiple implementations, I think we should move this
one level above.
##########
log4j-core/src/main/java/org/apache/logging/log4j/core/util/internal/instant/InstantPatternDynamicFormatter.java:
##########
@@ -711,23 +488,114 @@ public void formatTo(final StringBuilder buffer, final
Instant instant) {
}
};
}
+
+ @Override
+ @Nullable
+ PatternFormatterFactory tryMerge(PatternFormatterFactory other,
ChronoUnit thresholdPrecision) {
+ // We always merge consecutive static pattern factories
+ if (other instanceof StaticPatternFormatterFactory) {
+ final StaticPatternFormatterFactory otherStatic =
(StaticPatternFormatterFactory) other;
+ return new StaticPatternFormatterFactory(this.literal +
otherStatic.literal);
+ }
+ // We also merge a static pattern factory with a DTF factory
+ if (other instanceof DateTimeFormatterPatternFormatterFactory) {
+ final DateTimeFormatterPatternFormatterFactory otherDtf =
+ (DateTimeFormatterPatternFormatterFactory) other;
+ return new DateTimeFormatterPatternFormatterFactory(
+ this.pattern + otherDtf.pattern, otherDtf.precision);
+ }
+ return super.tryMerge(other, thresholdPrecision);
+ }
+
+ static String escapeLiteral(String literal) {
+ StringBuilder sb = new StringBuilder(literal.length() + 2);
+ boolean inSingleQuotes = false;
+ for (int i = 0; i < literal.length(); i++) {
+ char c = literal.charAt(i);
+ if (c == '\'') {
+ if (inSingleQuotes) {
+ sb.append("'");
+ }
+ inSingleQuotes = false;
+ sb.append("''");
+ } else {
+ if (!inSingleQuotes) {
+ sb.append("'");
+ }
+ inSingleQuotes = true;
+ sb.append(c);
+ }
+ }
+ if (inSingleQuotes) {
+ sb.append("'");
+ }
+ return sb.toString();
+ }
}
- static final class DynamicPatternSequence extends PatternSequence {
+ /**
+ * Formats the pattern using {@link DateTimeFormatter}.
+ */
+ static final class DateTimeFormatterPatternFormatterFactory extends
PatternFormatterFactory {
- DynamicPatternSequence(final String content) {
- super(content, contentPrecision(content));
+ /**
+ * @param simplePattern A {@link DateTimeFormatter} pattern containing
a single letter.
+ */
+ DateTimeFormatterPatternFormatterFactory(final String simplePattern) {
+ this(simplePattern, patternPrecision(simplePattern));
}
/**
- * @param content a single-letter directive content complying (e.g.,
{@code H}, {@code HH}, or {@code pHH})
- * @return the time precision of the directive
+ * @param pattern Any {@link DateTimeFormatter} pattern.
+ * @param precision The maximum interval of time over which this
pattern is constant.
*/
+ DateTimeFormatterPatternFormatterFactory(final String pattern, final
ChronoUnit precision) {
+ super(pattern, precision);
+ }
+
+ InstantPatternFormatter createFormatter(final Locale locale, final
TimeZone timeZone) {
+ final DateTimeFormatter dateTimeFormatter =
+ DateTimeFormatter.ofPattern(pattern,
locale).withZone(timeZone.toZoneId());
+ return new AbstractFormatter(pattern, locale, timeZone, precision)
{
+ @Override
+ public void formatTo(final StringBuilder buffer, final Instant
instant) {
+ final TemporalAccessor instantAccessor =
toTemporalAccessor(instant);
+ dateTimeFormatter.formatTo(instantAccessor, buffer);
+ }
+ };
+ }
+
+ @Override
@Nullable
- private static ChronoUnit contentPrecision(final String content) {
+ PatternFormatterFactory tryMerge(PatternFormatterFactory other,
ChronoUnit thresholdPrecision) {
+ // We merge two DTF factories if they are both above or below the
threshold
+ if (other instanceof DateTimeFormatterPatternFormatterFactory) {
+ final DateTimeFormatterPatternFormatterFactory otherDtf =
+ (DateTimeFormatterPatternFormatterFactory) other;
+ if (isConstantForDurationOf(thresholdPrecision)
+ ==
otherDtf.isConstantForDurationOf(thresholdPrecision)) {
+ ChronoUnit precision =
this.precision.getDuration().compareTo(otherDtf.precision.getDuration()) < 0
+ ? this.precision
+ : otherDtf.precision;
+ return new
DateTimeFormatterPatternFormatterFactory(this.pattern + otherDtf.pattern,
precision);
+ }
+ }
+ // We merge a static pattern factory
+ if (other instanceof StaticPatternFormatterFactory) {
+ final StaticPatternFormatterFactory otherStatic =
(StaticPatternFormatterFactory) other;
+ return new
DateTimeFormatterPatternFormatterFactory(this.pattern + otherStatic.pattern,
this.precision);
+ }
+ return super.tryMerge(other, thresholdPrecision);
Review Comment:
One less cognitive hop:
```suggestion
return null;
```
##########
log4j-core/src/main/java/org/apache/logging/log4j/core/util/internal/instant/InstantPatternDynamicFormatter.java:
##########
@@ -806,26 +674,114 @@ private static String removePadding(final String
content) {
}
}
- static final class CompositePatternSequence extends PatternSequence {
+ static class SecondPatternFormatterFactory extends PatternFormatterFactory
{
+
+ private final boolean printSeconds;
+ private final String separator;
+ private final int fractionalDigits;
+
+ SecondPatternFormatterFactory(boolean printSeconds, String separator,
int fractionalDigits) {
+ super(
+ createPattern(printSeconds, separator, fractionalDigits),
+ determinePrecision(printSeconds, fractionalDigits));
+ this.printSeconds = printSeconds;
+ this.separator = separator;
+ this.fractionalDigits = fractionalDigits;
+ }
- CompositePatternSequence(final List<PatternSequence> sequences) {
- super(concatSequencePatterns(sequences),
findSequenceMaxPrecision(sequences));
- // Only allow two or more sequences
- if (sequences.size() < 2) {
- throw new IllegalArgumentException("was expecting two or more
sequences: " + sequences);
+ private static String createPattern(boolean printSeconds, String
separator, int fractionalDigits) {
+ StringBuilder builder = new StringBuilder();
+ if (printSeconds) {
+ builder.append("ss");
}
+
builder.append(StaticPatternFormatterFactory.escapeLiteral(separator));
+ if (fractionalDigits > 0) {
+ builder.append(Strings.repeat("S", fractionalDigits));
+ }
+ return builder.toString();
+ }
+
+ private static ChronoUnit determinePrecision(boolean printSeconds, int
digits) {
+ return digits > 6
+ ? ChronoUnit.NANOS
+ : digits > 3
+ ? ChronoUnit.MICROS
+ : digits > 0 ? ChronoUnit.MILLIS : printSeconds ?
ChronoUnit.SECONDS : ChronoUnit.FOREVER;
Review Comment:
Would you mind breaking this entire method down to multiple `if` blocks,
please?
##########
log4j-core/src/main/java/org/apache/logging/log4j/core/util/internal/instant/InstantPatternDynamicFormatter.java:
##########
@@ -711,23 +488,114 @@ public void formatTo(final StringBuilder buffer, final
Instant instant) {
}
};
}
+
+ @Override
+ @Nullable
+ PatternFormatterFactory tryMerge(PatternFormatterFactory other,
ChronoUnit thresholdPrecision) {
+ // We always merge consecutive static pattern factories
+ if (other instanceof StaticPatternFormatterFactory) {
+ final StaticPatternFormatterFactory otherStatic =
(StaticPatternFormatterFactory) other;
+ return new StaticPatternFormatterFactory(this.literal +
otherStatic.literal);
+ }
+ // We also merge a static pattern factory with a DTF factory
+ if (other instanceof DateTimeFormatterPatternFormatterFactory) {
+ final DateTimeFormatterPatternFormatterFactory otherDtf =
+ (DateTimeFormatterPatternFormatterFactory) other;
+ return new DateTimeFormatterPatternFormatterFactory(
+ this.pattern + otherDtf.pattern, otherDtf.precision);
+ }
+ return super.tryMerge(other, thresholdPrecision);
Review Comment:
One less cognitive hop:
```suggestion
return null;
```
##########
log4j-core/src/main/java/org/apache/logging/log4j/core/util/internal/instant/InstantPatternDynamicFormatter.java:
##########
@@ -711,23 +488,114 @@ public void formatTo(final StringBuilder buffer, final
Instant instant) {
}
};
}
+
+ @Override
+ @Nullable
+ PatternFormatterFactory tryMerge(PatternFormatterFactory other,
ChronoUnit thresholdPrecision) {
+ // We always merge consecutive static pattern factories
+ if (other instanceof StaticPatternFormatterFactory) {
+ final StaticPatternFormatterFactory otherStatic =
(StaticPatternFormatterFactory) other;
+ return new StaticPatternFormatterFactory(this.literal +
otherStatic.literal);
+ }
+ // We also merge a static pattern factory with a DTF factory
+ if (other instanceof DateTimeFormatterPatternFormatterFactory) {
+ final DateTimeFormatterPatternFormatterFactory otherDtf =
+ (DateTimeFormatterPatternFormatterFactory) other;
+ return new DateTimeFormatterPatternFormatterFactory(
+ this.pattern + otherDtf.pattern, otherDtf.precision);
+ }
+ return super.tryMerge(other, thresholdPrecision);
+ }
+
+ static String escapeLiteral(String literal) {
+ StringBuilder sb = new StringBuilder(literal.length() + 2);
+ boolean inSingleQuotes = false;
+ for (int i = 0; i < literal.length(); i++) {
+ char c = literal.charAt(i);
+ if (c == '\'') {
+ if (inSingleQuotes) {
+ sb.append("'");
+ }
+ inSingleQuotes = false;
+ sb.append("''");
+ } else {
+ if (!inSingleQuotes) {
+ sb.append("'");
+ }
+ inSingleQuotes = true;
+ sb.append(c);
+ }
+ }
+ if (inSingleQuotes) {
+ sb.append("'");
+ }
+ return sb.toString();
+ }
}
- static final class DynamicPatternSequence extends PatternSequence {
+ /**
+ * Formats the pattern using {@link DateTimeFormatter}.
+ */
+ static final class DateTimeFormatterPatternFormatterFactory extends
PatternFormatterFactory {
Review Comment:
*Nit:* I prefer
1. renaming this from `DateTimeFormatter...` to `Dynamic...` (since the
usage of `DTF` has nothing to do with its function, which is being a dynamic
date & time pattern ~sequence~ formatter factory)
2. extending `SecondPattern...` from this `Dynamic...`
##########
log4j-core/src/main/java/org/apache/logging/log4j/core/util/internal/instant/InstantPatternDynamicFormatter.java:
##########
@@ -806,26 +674,114 @@ private static String removePadding(final String
content) {
}
}
- static final class CompositePatternSequence extends PatternSequence {
+ static class SecondPatternFormatterFactory extends PatternFormatterFactory
{
+
+ private final boolean printSeconds;
+ private final String separator;
+ private final int fractionalDigits;
+
+ SecondPatternFormatterFactory(boolean printSeconds, String separator,
int fractionalDigits) {
+ super(
+ createPattern(printSeconds, separator, fractionalDigits),
+ determinePrecision(printSeconds, fractionalDigits));
+ this.printSeconds = printSeconds;
+ this.separator = separator;
+ this.fractionalDigits = fractionalDigits;
+ }
- CompositePatternSequence(final List<PatternSequence> sequences) {
- super(concatSequencePatterns(sequences),
findSequenceMaxPrecision(sequences));
- // Only allow two or more sequences
- if (sequences.size() < 2) {
- throw new IllegalArgumentException("was expecting two or more
sequences: " + sequences);
+ private static String createPattern(boolean printSeconds, String
separator, int fractionalDigits) {
+ StringBuilder builder = new StringBuilder();
+ if (printSeconds) {
+ builder.append("ss");
}
+
builder.append(StaticPatternFormatterFactory.escapeLiteral(separator));
+ if (fractionalDigits > 0) {
+ builder.append(Strings.repeat("S", fractionalDigits));
+ }
+ return builder.toString();
+ }
+
+ private static ChronoUnit determinePrecision(boolean printSeconds, int
digits) {
+ return digits > 6
+ ? ChronoUnit.NANOS
+ : digits > 3
+ ? ChronoUnit.MICROS
+ : digits > 0 ? ChronoUnit.MILLIS : printSeconds ?
ChronoUnit.SECONDS : ChronoUnit.FOREVER;
}
- @SuppressWarnings("OptionalGetWithoutIsPresent")
- private static ChronoUnit
findSequenceMaxPrecision(List<PatternSequence> sequences) {
- return sequences.stream()
- .map(sequence -> sequence.precision)
- .min(Comparator.comparing(ChronoUnit::getDuration))
- .get();
+ private static void formatSeconds(StringBuilder buffer, Instant
instant) {
+ long secondsInMinute = instant.getEpochSecond() % 60L;
+ buffer.append((char) ((secondsInMinute / 10L) + '0'));
+ buffer.append((char) ((secondsInMinute % 10L) + '0'));
}
- private static String concatSequencePatterns(List<PatternSequence>
sequences) {
- return sequences.stream().map(sequence ->
sequence.pattern).collect(Collectors.joining());
+ private void formatFractionalDigits(StringBuilder buffer, Instant
instant) {
+ final int offset = buffer.length();
+ buffer.setLength(offset + fractionalDigits);
+ long value = instant.getNanoOfSecond();
+ int valuePrecision = 9;
+ // Skip digits beyond the requested precision
+ while (fractionalDigits < valuePrecision) {
+ valuePrecision--;
+ value = value / 10L;
+ }
+ // Print the digits
+ while (0 < valuePrecision--) {
+ buffer.setCharAt(offset + valuePrecision, (char) ('0' + value
% 10L));
+ value = value / 10L;
+ }
+ }
+
+ @Override
+ InstantPatternFormatter createFormatter(Locale locale, TimeZone
timeZone) {
+ if (!printSeconds) {
+ return new AbstractFormatter(pattern, locale, timeZone,
precision) {
+ @Override
+ public void formatTo(StringBuilder buffer, Instant
instant) {
+ buffer.append(separator);
+ formatFractionalDigits(buffer, instant);
+ }
+ };
+ }
+ if (fractionalDigits == 0) {
+ return new AbstractFormatter(pattern, locale, timeZone,
precision) {
+ @Override
+ public void formatTo(StringBuilder buffer, Instant
instant) {
+ formatSeconds(buffer, instant);
+ buffer.append(separator);
+ }
+ };
+ }
+ return new AbstractFormatter(pattern, locale, timeZone, precision)
{
+ @Override
+ public void formatTo(StringBuilder buffer, Instant instant) {
+ formatSeconds(buffer, instant);
+ buffer.append(separator);
+ formatFractionalDigits(buffer, instant);
+ }
+ };
+ }
+
+ @Override
+ @Nullable
+ PatternFormatterFactory tryMerge(PatternFormatterFactory other,
ChronoUnit thresholdPrecision) {
Review Comment:
Docs are missing
##########
log4j-core-test/src/test/java/org/apache/logging/log4j/core/util/internal/instant/InstantPatternDynamicFormatterTest.java:
##########
@@ -47,112 +45,81 @@ class InstantPatternDynamicFormatterTest {
@ParameterizedTest
@MethodSource("sequencingTestCases")
void sequencing_should_work(
- final String pattern, final ChronoUnit thresholdPrecision, final
List<PatternSequence> expectedSequences) {
- final List<PatternSequence> actualSequences = sequencePattern(pattern,
thresholdPrecision);
+ final String pattern,
+ final ChronoUnit thresholdPrecision,
+ final List<PatternFormatterFactory> expectedSequences) {
+ final List<PatternFormatterFactory> actualSequences =
sequencePattern(pattern, thresholdPrecision);
assertThat(actualSequences).isEqualTo(expectedSequences);
}
static List<Arguments> sequencingTestCases() {
final List<Arguments> testCases = new ArrayList<>();
// `SSSX` should be treated constant for daily updates
- testCases.add(Arguments.of("SSSX", ChronoUnit.DAYS,
singletonList(pCom(pDyn("SSS"), pDyn("X")))));
+ testCases.add(Arguments.of("SSSX", ChronoUnit.DAYS,
asList(pMilliSec(), pDyn("X"))));
// `yyyyMMddHHmmssSSSX` instant cache updated hourly
testCases.add(Arguments.of(
"yyyyMMddHHmmssSSSX",
ChronoUnit.HOURS,
- asList(
- pCom(pDyn("yyyy"), pDyn("MM"), pDyn("dd"), pDyn("HH")),
- pCom(pDyn("mm"), pDyn("ss"), pDyn("SSS")),
- pDyn("X"))));
+ asList(pDyn("yyyyMMddHH", ChronoUnit.HOURS), pDyn("mm"),
pSec("", 3), pDyn("X"))));
// `yyyyMMddHHmmssSSSX` instant cache updated per minute
testCases.add(Arguments.of(
"yyyyMMddHHmmssSSSX",
ChronoUnit.MINUTES,
- asList(
- pCom(pDyn("yyyy"), pDyn("MM"), pDyn("dd"), pDyn("HH"),
pDyn("mm")),
- pCom(pDyn("ss"), pDyn("SSS")),
- pDyn("X"))));
+ asList(pDyn("yyyyMMddHHmm", ChronoUnit.MINUTES), pSec("", 3),
pDyn("X"))));
// ISO9601 instant cache updated daily
final String iso8601InstantPattern = "yyyy-MM-dd'T'HH:mm:ss.SSSX";
testCases.add(Arguments.of(
iso8601InstantPattern,
ChronoUnit.DAYS,
asList(
- pCom(pDyn("yyyy"), pSta("-"), pDyn("MM"), pSta("-"),
pDyn("dd"), pSta("T")),
- pCom(
- pDyn("HH"),
- pSta(":"),
- pDyn("mm"),
- pSta(":"),
- pDyn("ss"),
- pSta("."),
- pDyn("SSS"),
- pDyn("X")))));
+ pDyn("yyyy'-'MM'-'dd'T'", ChronoUnit.DAYS),
+ pDyn("HH':'mm':'", ChronoUnit.MINUTES),
+ pSec(".", 3),
+ pDyn("X"))));
// ISO9601 instant cache updated per minute
testCases.add(Arguments.of(
iso8601InstantPattern,
ChronoUnit.MINUTES,
- asList(
- pCom(
- pDyn("yyyy"),
- pSta("-"),
- pDyn("MM"),
- pSta("-"),
- pDyn("dd"),
- pSta("T"),
- pDyn("HH"),
- pSta(":"),
- pDyn("mm"),
- pSta(":")),
- pCom(pDyn("ss"), pSta("."), pDyn("SSS")),
- pDyn("X"))));
+ asList(pDyn("yyyy'-'MM'-'dd'T'HH':'mm':'",
ChronoUnit.MINUTES), pSec(".", 3), pDyn("X"))));
// ISO9601 instant cache updated per second
testCases.add(Arguments.of(
iso8601InstantPattern,
ChronoUnit.SECONDS,
- asList(
- pCom(
- pDyn("yyyy"),
- pSta("-"),
- pDyn("MM"),
- pSta("-"),
- pDyn("dd"),
- pSta("T"),
- pDyn("HH"),
- pSta(":"),
- pDyn("mm"),
- pSta(":"),
- pDyn("ss"),
- pSta(".")),
- pDyn("SSS"),
- pDyn("X"))));
+ asList(pDyn("yyyy'-'MM'-'dd'T'HH':'mm':'",
ChronoUnit.MINUTES), pSec(".", 3), pDyn("X"))));
+
+ // Seconds and micros
+ testCases.add(Arguments.of(
+ "HH:mm:ss.SSSSSS", ChronoUnit.MINUTES,
asList(pDyn("HH':'mm':'", ChronoUnit.MINUTES), pSec(".", 6))));
return testCases;
}
- private static CompositePatternSequence pCom(final PatternSequence...
sequences) {
- return new CompositePatternSequence(asList(sequences));
+ private static DateTimeFormatterPatternFormatterFactory pDyn(final String
pattern) {
Review Comment:
`pDyn()` is not correct anymore
##########
log4j-core/src/main/java/org/apache/logging/log4j/core/util/internal/instant/InstantPatternDynamicFormatter.java:
##########
@@ -806,26 +674,114 @@ private static String removePadding(final String
content) {
}
}
- static final class CompositePatternSequence extends PatternSequence {
+ static class SecondPatternFormatterFactory extends PatternFormatterFactory
{
+
+ private final boolean printSeconds;
+ private final String separator;
+ private final int fractionalDigits;
+
+ SecondPatternFormatterFactory(boolean printSeconds, String separator,
int fractionalDigits) {
+ super(
+ createPattern(printSeconds, separator, fractionalDigits),
+ determinePrecision(printSeconds, fractionalDigits));
+ this.printSeconds = printSeconds;
+ this.separator = separator;
+ this.fractionalDigits = fractionalDigits;
+ }
- CompositePatternSequence(final List<PatternSequence> sequences) {
- super(concatSequencePatterns(sequences),
findSequenceMaxPrecision(sequences));
- // Only allow two or more sequences
- if (sequences.size() < 2) {
- throw new IllegalArgumentException("was expecting two or more
sequences: " + sequences);
+ private static String createPattern(boolean printSeconds, String
separator, int fractionalDigits) {
+ StringBuilder builder = new StringBuilder();
+ if (printSeconds) {
+ builder.append("ss");
}
+
builder.append(StaticPatternFormatterFactory.escapeLiteral(separator));
+ if (fractionalDigits > 0) {
+ builder.append(Strings.repeat("S", fractionalDigits));
+ }
+ return builder.toString();
+ }
+
+ private static ChronoUnit determinePrecision(boolean printSeconds, int
digits) {
+ return digits > 6
+ ? ChronoUnit.NANOS
+ : digits > 3
+ ? ChronoUnit.MICROS
+ : digits > 0 ? ChronoUnit.MILLIS : printSeconds ?
ChronoUnit.SECONDS : ChronoUnit.FOREVER;
}
- @SuppressWarnings("OptionalGetWithoutIsPresent")
- private static ChronoUnit
findSequenceMaxPrecision(List<PatternSequence> sequences) {
- return sequences.stream()
- .map(sequence -> sequence.precision)
- .min(Comparator.comparing(ChronoUnit::getDuration))
- .get();
+ private static void formatSeconds(StringBuilder buffer, Instant
instant) {
+ long secondsInMinute = instant.getEpochSecond() % 60L;
+ buffer.append((char) ((secondsInMinute / 10L) + '0'));
+ buffer.append((char) ((secondsInMinute % 10L) + '0'));
}
- private static String concatSequencePatterns(List<PatternSequence>
sequences) {
- return sequences.stream().map(sequence ->
sequence.pattern).collect(Collectors.joining());
+ private void formatFractionalDigits(StringBuilder buffer, Instant
instant) {
+ final int offset = buffer.length();
+ buffer.setLength(offset + fractionalDigits);
+ long value = instant.getNanoOfSecond();
+ int valuePrecision = 9;
+ // Skip digits beyond the requested precision
+ while (fractionalDigits < valuePrecision) {
+ valuePrecision--;
+ value = value / 10L;
+ }
+ // Print the digits
+ while (0 < valuePrecision--) {
+ buffer.setCharAt(offset + valuePrecision, (char) ('0' + value
% 10L));
+ value = value / 10L;
+ }
+ }
+
+ @Override
+ InstantPatternFormatter createFormatter(Locale locale, TimeZone
timeZone) {
+ if (!printSeconds) {
+ return new AbstractFormatter(pattern, locale, timeZone,
precision) {
+ @Override
+ public void formatTo(StringBuilder buffer, Instant
instant) {
+ buffer.append(separator);
+ formatFractionalDigits(buffer, instant);
+ }
+ };
+ }
+ if (fractionalDigits == 0) {
+ return new AbstractFormatter(pattern, locale, timeZone,
precision) {
+ @Override
+ public void formatTo(StringBuilder buffer, Instant
instant) {
+ formatSeconds(buffer, instant);
+ buffer.append(separator);
+ }
+ };
+ }
+ return new AbstractFormatter(pattern, locale, timeZone, precision)
{
+ @Override
+ public void formatTo(StringBuilder buffer, Instant instant) {
+ formatSeconds(buffer, instant);
+ buffer.append(separator);
+ formatFractionalDigits(buffer, instant);
+ }
+ };
+ }
+
+ @Override
+ @Nullable
+ PatternFormatterFactory tryMerge(PatternFormatterFactory other,
ChronoUnit thresholdPrecision) {
+ // If we don't have a fractional part, we can merge a literal
separator
+ if (other instanceof StaticPatternFormatterFactory) {
+ StaticPatternFormatterFactory staticOther =
(StaticPatternFormatterFactory) other;
+ if (fractionalDigits == 0) {
+ return new SecondPatternFormatterFactory(
+ printSeconds, this.separator +
staticOther.literal, fractionalDigits);
+ }
+ }
+ // We can always append more fractional digits
+ if (other instanceof SecondPatternFormatterFactory) {
+ SecondPatternFormatterFactory secondOther =
(SecondPatternFormatterFactory) other;
+ if (!secondOther.printSeconds &&
secondOther.separator.isEmpty()) {
+ return new SecondPatternFormatterFactory(
+ printSeconds, this.separator,
this.fractionalDigits + secondOther.fractionalDigits);
+ }
+ }
+ return super.tryMerge(other, thresholdPrecision);
Review Comment:
One less cognitive hop:
```suggestion
return null;
```
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: notifications-unsubscr...@logging.apache.org
For queries about this service, please contact Infrastructure at:
us...@infra.apache.org
