Skip to content

Commit 63e1812

Browse files
ppkarwaszvy
ppkarwasz
authored and
vy
committed
Fix parsing and merging of literals in InstantPatternDynamicFormatter (#3932)
1 parent 94fc126 commit 63e1812

File tree

3 files changed

+136
-31
lines changed

3 files changed

+136
-31
lines changed

‎log4j-core-test/src/test/java/org/apache/logging/log4j/core/util/internal/instant/InstantPatternDynamicFormatterTest.java‎

Lines changed: 48 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -17,6 +17,7 @@
1717
package org.apache.logging.log4j.core.util.internal.instant;
1818

1919
import static java.util.Arrays.asList;
20+
import static java.util.Collections.emptyList;
2021
import static java.util.Collections.singletonList;
2122
import static org.apache.logging.log4j.core.util.internal.instant.InstantPatternDynamicFormatter.sequencePattern;
2223
import static org.assertj.core.api.Assertions.assertThat;
@@ -37,10 +38,12 @@
3738
import org.apache.logging.log4j.core.util.internal.instant.InstantPatternDynamicFormatter.SecondPatternSequence;
3839
import org.apache.logging.log4j.core.util.internal.instant.InstantPatternDynamicFormatter.StaticPatternSequence;
3940
import org.apache.logging.log4j.util.Constants;
41+
import org.assertj.core.api.Assertions;
4042
import org.junit.jupiter.params.ParameterizedTest;
4143
import org.junit.jupiter.params.provider.Arguments;
4244
import org.junit.jupiter.params.provider.MethodSource;
4345
import org.junit.jupiter.params.provider.ValueSource;
46+
import org.junitpioneer.jupiter.Issue;
4447

4548
class InstantPatternDynamicFormatterTest {
4649

@@ -55,8 +58,15 @@ void sequencing_should_work(
5558
static List<Arguments> sequencingTestCases() {
5659
final List<Arguments> testCases = new ArrayList<>();
5760

61+
// Single literals
62+
testCases.add(Arguments.of("", ChronoUnit.DAYS, emptyList()));
63+
testCases.add(Arguments.of("'foo'", ChronoUnit.DAYS, singletonList(literal("foo"))));
64+
testCases.add(Arguments.of("''", ChronoUnit.DAYS, singletonList(literal("'"))));
65+
testCases.add(Arguments.of("''''", ChronoUnit.DAYS, singletonList(literal("'"))));
66+
testCases.add(Arguments.of("'o''clock'", ChronoUnit.DAYS, singletonList(literal("o'clock"))));
67+
5868
// Merged constants
59-
testCases.add(Arguments.of(":'foo',", ChronoUnit.DAYS, singletonList(new StaticPatternSequence(":foo,"))));
69+
testCases.add(Arguments.of(":'foo',", ChronoUnit.DAYS, singletonList(literal(":foo,"))));
6070

6171
// `SSSX` should be treated constant for daily updates
6272
testCases.add(Arguments.of("SSSX", ChronoUnit.DAYS, asList(pMilliSec(), pDyn("X"))));
@@ -108,6 +118,43 @@ static List<Arguments> sequencingTestCases() {
108118
return testCases;
109119
}
110120

121+
@ParameterizedTest
122+
@ValueSource(strings = {"'", "'''", "'foo", "'foo''bar"})
123+
void sequencing_should_fail_on_unterminated_literal(String pattern) {
124+
Assertions.assertThatThrownBy(() -> sequencePattern(pattern, ChronoUnit.DAYS))
125+
.isInstanceOf(IllegalArgumentException.class)
126+
.hasMessageContaining("incomplete string literal");
127+
}
128+
129+
static Stream<Arguments> merging_of_adjacent_constants_should_work() {
130+
return Stream.of(
131+
Arguments.of(" ", singletonList(literal(" "))),
132+
Arguments.of(" ' ' ", singletonList(literal(" "))),
133+
Arguments.of(" '' ", singletonList(literal(" ' "))),
134+
Arguments.of("d ", singletonList(pDyn("d' '", ChronoUnit.DAYS))),
135+
Arguments.of("d ' ' ", singletonList(pDyn("d' '", ChronoUnit.DAYS))),
136+
Arguments.of("d '' ", singletonList(pDyn("d' '' '", ChronoUnit.DAYS))),
137+
Arguments.of(" d", singletonList(pDyn("' 'd", ChronoUnit.DAYS))),
138+
Arguments.of(" ' ' d", singletonList(pDyn("' 'd", ChronoUnit.DAYS))),
139+
Arguments.of(" '' d", singletonList(pDyn("' '' 'd", ChronoUnit.DAYS))),
140+
Arguments.of("s S", singletonList(pSec(1, " ", 1))),
141+
Arguments.of("s ' ' S", singletonList(pSec(1, " ", 1))),
142+
Arguments.of("s '' S", singletonList(pSec(1, " ' ", 1))));
143+
}
144+
145+
@ParameterizedTest
146+
@MethodSource
147+
@Issue("https://github.com/apache/logging-log4j2/issues/3930")
148+
void merging_of_adjacent_constants_should_work(
149+
final String pattern, final List<PatternSequence> expectedSequences) {
150+
final List<PatternSequence> actualSequences = sequencePattern(pattern, ChronoUnit.DAYS);
151+
assertThat(actualSequences).isEqualTo(expectedSequences);
152+
}
153+
154+
private static StaticPatternSequence literal(final String literal) {
155+
return new StaticPatternSequence(literal);
156+
}
157+
111158
private static DynamicPatternSequence pDyn(final String singlePattern) {
112159
return new DynamicPatternSequence(singlePattern);
113160
}

‎log4j-core/src/main/java/org/apache/logging/log4j/core/util/internal/instant/InstantPatternDynamicFormatter.java‎

Lines changed: 76 additions & 30 deletions
Original file line numberDiff line numberDiff line change
@@ -253,7 +253,23 @@ private static List<PatternSequence> sequencePattern(final String pattern) {
253253

254254
// Handle single-quotes
255255
else if (c == '\'') {
256-
final int endIndex = pattern.indexOf('\'', startIndex + 1);
256+
int endIndex = startIndex + 1;
257+
while (endIndex < pattern.length()) {
258+
if (pattern.charAt(endIndex) == '\'') {
259+
if ((endIndex + 1) < pattern.length() && pattern.charAt(endIndex + 1) == '\'') {
260+
// Escaped apostrophe, skip it
261+
endIndex += 2;
262+
} else {
263+
// Closing quote found
264+
break;
265+
}
266+
} else {
267+
endIndex++;
268+
}
269+
}
270+
if (endIndex >= pattern.length()) {
271+
endIndex = -1; // Signal incomplete literal
272+
}
257273
final PatternSequence sequence = getStaticPatternSequence(pattern, startIndex, endIndex);
258274
sequences.add(sequence);
259275
startIndex = endIndex + 1;
@@ -276,7 +292,10 @@ private static PatternSequence getStaticPatternSequence(String pattern, int star
276292
startIndex, pattern);
277293
throw new IllegalArgumentException(message);
278294
}
279-
final String sequenceLiteral = (startIndex + 1) == endIndex ? "'" : pattern.substring(startIndex + 1, endIndex);
295+
// Extract the literal, replacing escaped apostrophes with a single apostrophe
296+
final String sequenceLiteral = (startIndex + 1) == endIndex
297+
? "'"
298+
: pattern.substring(startIndex + 1, endIndex).replace("''", "'");
280299
return new StaticPatternSequence(sequenceLiteral);
281300
}
282301

@@ -414,8 +433,32 @@ abstract static class PatternSequence {
414433

415434
final ChronoUnit precision;
416435

436+
/**
437+
* Creates a {@code PatternSequence} from a {@link java.time.format.DateTimeFormatter DateTimeFormatter} pattern
438+
* and its precision.
439+
*
440+
* <p><strong>Quoting invariant:</strong> every literal in {@code pattern} must be enclosed in single quotes.
441+
* To include a lone apostrophe as a literal, use {@code "''''"} (open quote, escaped apostrophe {@code ''}, close quote).
442+
* Never use a bare {@code "''"}: while syntactically valid, it becomes ambiguous at concatenation boundaries.
443+
* This contract lets us merge adjacent quoted blocks in a purely context-free way
444+
* (drop the left closing quote and the right opening quote).</p>
445+
*
446+
* <p><b>Examples</b>:
447+
* <pre>{@code
448+
* "yyyy-MM-dd 'at' HH:mm" // OK: 'at' is a quoted literal
449+
* "HH 'o''clock'" // OK: apostrophe inside a quoted block is escaped as ''
450+
* "yyyy''''MM" // OK: emits a literal apostrophe between year and month
451+
* }</pre>
452+
*
453+
* @param pattern a DateTimeFormatter pattern with all literals fully quoted
454+
* @param precision the largest {@link java.time.temporal.ChronoUnit ChronoUnit} interval over which the
455+
* formatted output remains constant for this pattern
456+
* @throws NullPointerException if {@code pattern} or {@code precision} is {@code null}
457+
* @throws IllegalArgumentException if {@code pattern} is not a valid {@code DateTimeFormatter} pattern
458+
*/
417459
@SuppressWarnings("ReturnValueIgnored")
418460
PatternSequence(final String pattern, final ChronoUnit precision) {
461+
assert !"''".equals(pattern);
419462
DateTimeFormatter.ofPattern(pattern); // Validate the pattern
420463
this.pattern = pattern;
421464
this.precision = precision;
@@ -456,37 +499,40 @@ abstract static class PatternSequence {
456499
* @return A merged formatter factory or {@code null} if merging is not possible.
457500
*/
458501
@Nullable
459-
PatternSequence tryMerge(PatternSequence other, ChronoUnit thresholdPrecision) {
460-
return null;
461-
}
502+
abstract PatternSequence tryMerge(PatternSequence other, ChronoUnit thresholdPrecision);
462503

463504
boolean isConstantForDurationOf(final ChronoUnit thresholdPrecision) {
464505
return precision.compareTo(thresholdPrecision) >= 0;
465506
}
466507

467508
static String escapeLiteral(String literal) {
468-
StringBuilder sb = new StringBuilder(literal.length() + 2);
469-
boolean inSingleQuotes = false;
470-
for (int i = 0; i < literal.length(); i++) {
471-
char c = literal.charAt(i);
472-
if (c == '\'') {
473-
if (inSingleQuotes) {
474-
sb.append("'");
475-
}
476-
inSingleQuotes = false;
477-
sb.append("''");
478-
} else {
479-
if (!inSingleQuotes) {
480-
sb.append("'");
481-
}
482-
inSingleQuotes = true;
483-
sb.append(c);
484-
}
485-
}
486-
if (inSingleQuotes) {
487-
sb.append("'");
509+
// Ensure that an empty literal is not quoted as "''",
510+
// which would be interpreted as an apostrophe-escape sequence.
511+
return literal.isEmpty() ? "" : "'" + literal.replace("'", "''") + "'";
512+
}
513+
514+
/**
515+
* Concatenates two DateTimeFormatter pattern fragments.
516+
* <p>
517+
* Precondition (enforced by the caller): every literal is fully quoted.
518+
* Even a lone apostrophe is emitted as the quoted literal block "''''"
519+
* (open quote, escaped apostrophe, and close quote).
520+
* We never use a bare "''".
521+
* </
522+
*/
523+
static String mergePatterns(String left, String right) {
524+
if (left.isEmpty()) return right;
525+
if (right.isEmpty()) return left;
526+
527+
if (left.charAt(left.length() - 1) == '\'' && right.charAt(0) == '\'') {
528+
// Stitch two adjacent quoted-literal blocks into one by removing the
529+
// boundary quotes (close-then-open).
530+
// Without this, concatenation would yield "...''..." at the join, which would change semantics.
531+
//
532+
// See: https://github.com/apache/logging-log4j2/issues/3930
533+
return left.substring(0, left.length() - 1) + right.substring(1);
488534
}
489-
return sb.toString();
535+
return left + right;
490536
}
491537

492538
@Override
@@ -537,12 +583,12 @@ PatternSequence tryMerge(PatternSequence other, ChronoUnit thresholdPrecision) {
537583
// We always merge consecutive static pattern factories
538584
if (other instanceof StaticPatternSequence) {
539585
final StaticPatternSequence otherStatic = (StaticPatternSequence) other;
540-
return new StaticPatternSequence(this.literal + otherStatic.literal);
586+
return new StaticPatternSequence(mergePatterns(this.literal, otherStatic.literal));
541587
}
542588
// We also merge a static pattern factory with a DTF factory
543589
if (other instanceof DynamicPatternSequence) {
544590
final DynamicPatternSequence otherDtf = (DynamicPatternSequence) other;
545-
return new DynamicPatternSequence(this.pattern + otherDtf.pattern, otherDtf.precision);
591+
return new DynamicPatternSequence(mergePatterns(this.pattern, otherDtf.pattern), otherDtf.precision);
546592
}
547593
return null;
548594
}
@@ -591,13 +637,13 @@ PatternSequence tryMerge(PatternSequence other, ChronoUnit thresholdPrecision) {
591637
ChronoUnit precision = this.precision.getDuration().compareTo(otherDtf.precision.getDuration()) < 0
592638
? this.precision
593639
: otherDtf.precision;
594-
return new DynamicPatternSequence(this.pattern + otherDtf.pattern, precision);
640+
return new DynamicPatternSequence(mergePatterns(this.pattern, otherDtf.pattern), precision);
595641
}
596642
}
597643
// We merge a static pattern factory
598644
if (other instanceof StaticPatternSequence) {
599645
final StaticPatternSequence otherStatic = (StaticPatternSequence) other;
600-
return new DynamicPatternSequence(this.pattern + otherStatic.pattern, this.precision);
646+
return new DynamicPatternSequence(mergePatterns(this.pattern, otherStatic.pattern), this.precision);
601647
}
602648
return null;
603649
}

‎src/changelog/.2.x.x/3930_date-converter.xml‎

Lines changed: 12 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,12 @@
1+
<?xml version="1.0" encoding="UTF-8"?>
2+
<entry xmlns="https://logging.apache.org/xml/ns"
3+
xmlns:xsi="http://www.w3.org/2001/XMLSchema-instance"
4+
xsi:schemaLocation="
5+
https://logging.apache.org/xml/ns
6+
https://logging.apache.org/xml/ns/log4j-changelog-0.xsd"
7+
type="fixed">
8+
<issue id="3930" link="https://github.com/apache/logging-log4j2/issues/3930"/>
9+
<description format="asciidoc">
10+
Fix parsing and merging of literals in `InstantPatternDynamicFormatter`.
11+
</description>
12+
</entry>

0 commit comments

Comments
 (0)