Fix formatting of s pattern (#3469) (#3866)

Fixes the formatting of the single `s` pattern.

Co-authored-by: Piotr P. Karwasz <[email protected]>
Co-authored-by: Volkan Yazıcı <[email protected]>

Author: 3 people

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

@@ -65,13 +65,13 @@ static List<Arguments> sequencingTestCases() {
         testCases.add(Arguments.of(
                 "yyyyMMddHHmmssSSSX",
                 ChronoUnit.HOURS,
-                asList(pDyn("yyyyMMddHH", ChronoUnit.HOURS), pDyn("mm"), pSec("", 3), pDyn("X"))));
+                asList(pDyn("yyyyMMddHH", ChronoUnit.HOURS), pDyn("mm"), pSec(2, "", 3), pDyn("X"))));
 
         // `yyyyMMddHHmmssSSSX` instant cache updated per minute
         testCases.add(Arguments.of(
                 "yyyyMMddHHmmssSSSX",
                 ChronoUnit.MINUTES,
-                asList(pDyn("yyyyMMddHHmm", ChronoUnit.MINUTES), pSec("", 3), pDyn("X"))));
+                asList(pDyn("yyyyMMddHHmm", ChronoUnit.MINUTES), pSec(2, "", 3), pDyn("X"))));
 
         // ISO9601 instant cache updated daily
         final String iso8601InstantPattern = "yyyy-MM-dd'T'HH:mm:ss.SSSX";
@@ -81,24 +81,29 @@ static List<Arguments> sequencingTestCases() {
                 asList(
                         pDyn("yyyy'-'MM'-'dd'T'", ChronoUnit.DAYS),
                         pDyn("HH':'mm':'", ChronoUnit.MINUTES),
-                        pSec(".", 3),
+                        pSec(2, ".", 3),
                         pDyn("X"))));
 
         // ISO9601 instant cache updated per minute
         testCases.add(Arguments.of(
                 iso8601InstantPattern,
                 ChronoUnit.MINUTES,
-                asList(pDyn("yyyy'-'MM'-'dd'T'HH':'mm':'", ChronoUnit.MINUTES), pSec(".", 3), pDyn("X"))));
+                asList(pDyn("yyyy'-'MM'-'dd'T'HH':'mm':'", ChronoUnit.MINUTES), pSec(2, ".", 3), pDyn("X"))));
 
         // ISO9601 instant cache updated per second
         testCases.add(Arguments.of(
                 iso8601InstantPattern,
                 ChronoUnit.SECONDS,
-                asList(pDyn("yyyy'-'MM'-'dd'T'HH':'mm':'", ChronoUnit.MINUTES), pSec(".", 3), pDyn("X"))));
+                asList(pDyn("yyyy'-'MM'-'dd'T'HH':'mm':'", ChronoUnit.MINUTES), pSec(2, ".", 3), pDyn("X"))));
 
         // Seconds and micros
         testCases.add(Arguments.of(
-                "HH:mm:ss.SSSSSS", ChronoUnit.MINUTES, asList(pDyn("HH':'mm':'", ChronoUnit.MINUTES), pSec(".", 6))));
+                "HH:mm:ss.SSSSSS",
+                ChronoUnit.MINUTES,
+                asList(pDyn("HH':'mm':'", ChronoUnit.MINUTES), pSec(2, ".", 6))));
+
+        // Seconds without padding
+        testCases.add(Arguments.of("s.SSS", ChronoUnit.SECONDS, singletonList(pSec(1, ".", 3))));
 
         return testCases;
     }
@@ -111,12 +116,12 @@ private static DynamicPatternSequence pDyn(final String pattern, final ChronoUni
         return new DynamicPatternSequence(pattern, precision);
     }
 
-    private static SecondPatternSequence pSec(String separator, int fractionalDigits) {
-        return new SecondPatternSequence(true, separator, fractionalDigits);
+    private static SecondPatternSequence pSec(int secondDigits, String separator, int fractionalDigits) {
+        return new SecondPatternSequence(secondDigits, separator, fractionalDigits);
     }
 
     private static SecondPatternSequence pMilliSec() {
-        return new SecondPatternSequence(false, "", 3);
+        return new SecondPatternSequence(0, "", 3);
     }
 
     @ParameterizedTest
@@ -352,7 +357,9 @@ static Stream<Arguments> formatterInputs() {
                         "yyyy-MM-dd'T'HH:mm:ss.SSS",
                         "yyyy-MM-dd'T'HH:mm:ss.SSSSSS",
                         "dd/MM/yy HH:mm:ss.SSS",
-                        "dd/MM/yyyy HH:mm:ss.SSS")
+                        "dd/MM/yyyy HH:mm:ss.SSS",
+                        // seconds without padding
+                        "s.SSS")
                 .flatMap(InstantPatternDynamicFormatterTest::formatterInputs);
     }
 
@@ -364,7 +371,7 @@ static Stream<Arguments> formatterInputs() {
             Arrays.stream(TimeZone.getAvailableIDs()).map(TimeZone::getTimeZone).toArray(TimeZone[]::new);
 
     static Stream<Arguments> formatterInputs(final String pattern) {
-        return IntStream.range(0, 500).mapToObj(ignoredIndex -> {
+        return IntStream.range(0, 100).mapToObj(ignoredIndex -> {
             final Locale locale = LOCALES[RANDOM.nextInt(LOCALES.length)];
             final TimeZone timeZone = TIME_ZONES[RANDOM.nextInt(TIME_ZONES.length)];
             final MutableInstant instant = randomInstant();

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

@@ -239,10 +239,10 @@ private static List<PatternSequence> sequencePattern(final String pattern) {
                 final PatternSequence sequence;
                 switch (c) {
                     case 's':
-                        sequence = new SecondPatternSequence(true, "", 0);
+                        sequence = new SecondPatternSequence(sequenceContent.length(), "", 0);
                         break;
                     case 'S':
-                        sequence = new SecondPatternSequence(false, "", sequenceContent.length());
+                        sequence = new SecondPatternSequence(0, "", sequenceContent.length());
                         break;
                     default:
                         sequence = new DynamicPatternSequence(sequenceContent);
@@ -694,39 +694,50 @@ static class SecondPatternSequence extends PatternSequence {
             100_000_000, 10_000_000, 1_000_000, 100_000, 10_000, 1_000, 100, 10, 1
         };
 
-        private final boolean printSeconds;
+        private final int secondDigits;
         private final String separator;
         private final int fractionalDigits;
 
-        SecondPatternSequence(boolean printSeconds, String separator, int fractionalDigits) {
+        SecondPatternSequence(int secondDigits, String separator, int fractionalDigits) {
             super(
-                    createPattern(printSeconds, separator, fractionalDigits),
-                    determinePrecision(printSeconds, fractionalDigits));
-            this.printSeconds = printSeconds;
+                    createPattern(secondDigits, separator, fractionalDigits),
+                    determinePrecision(secondDigits, fractionalDigits));
+            final int maxSecondDigits = 2;
+            if (secondDigits > maxSecondDigits) {
+                final String message = String.format(
+                        "More than %d `s` pattern letters are not supported, found: %d", maxSecondDigits, secondDigits);
+                throw new IllegalArgumentException(message);
+            }
+            final int maxFractionalDigits = 9;
+            if (fractionalDigits > maxFractionalDigits) {
+                final String message = String.format(
+                        "More than %d `S` pattern letters are not supported, found: %d",
+                        maxFractionalDigits, fractionalDigits);
+                throw new IllegalArgumentException(message);
+            }
+            this.secondDigits = secondDigits;
             this.separator = separator;
             this.fractionalDigits = fractionalDigits;
         }
 
-        private static String createPattern(boolean printSeconds, String separator, int fractionalDigits) {
-            StringBuilder builder = new StringBuilder();
-            if (printSeconds) {
-                builder.append("ss");
-            }
-            builder.append(StaticPatternSequence.escapeLiteral(separator));
-            if (fractionalDigits > 0) {
-                builder.append(Strings.repeat("S", fractionalDigits));
-            }
-            return builder.toString();
+        private static String createPattern(int secondDigits, String separator, int fractionalDigits) {
+            return Strings.repeat("s", secondDigits)
+                    + StaticPatternSequence.escapeLiteral(separator)
+                    + Strings.repeat("S", fractionalDigits);
         }
 
-        private static ChronoUnit determinePrecision(boolean printSeconds, int digits) {
+        private static ChronoUnit determinePrecision(int secondDigits, int digits) {
             if (digits > 6) return ChronoUnit.NANOS;
             if (digits > 3) return ChronoUnit.MICROS;
             if (digits > 0) return ChronoUnit.MILLIS;
-            return printSeconds ? ChronoUnit.SECONDS : ChronoUnit.FOREVER;
+            return secondDigits > 0 ? ChronoUnit.SECONDS : ChronoUnit.FOREVER;
+        }
+
+        private static void formatUnpaddedSeconds(StringBuilder buffer, Instant instant) {
+            buffer.append(instant.getEpochSecond() % 60L);
         }
 
-        private static void formatSeconds(StringBuilder buffer, Instant instant) {
+        private static void formatPaddedSeconds(StringBuilder buffer, Instant instant) {
             long secondsInMinute = instant.getEpochSecond() % 60L;
             buffer.append((char) ((secondsInMinute / 10L) + '0'));
             buffer.append((char) ((secondsInMinute % 10L) + '0'));
@@ -757,9 +768,12 @@ private static void formatMillis(StringBuilder buffer, Instant instant) {
 
         @Override
         InstantPatternFormatter createFormatter(Locale locale, TimeZone timeZone) {
+            final BiConsumer<StringBuilder, Instant> secondDigitsFormatter = secondDigits == 2
+                    ? SecondPatternSequence::formatPaddedSeconds
+                    : SecondPatternSequence::formatUnpaddedSeconds;
             final BiConsumer<StringBuilder, Instant> fractionDigitsFormatter =
                     fractionalDigits == 3 ? SecondPatternSequence::formatMillis : this::formatFractionalDigits;
-            if (!printSeconds) {
+            if (secondDigits == 0) {
                 return new AbstractFormatter(pattern, locale, timeZone, precision) {
                     @Override
                     public void formatTo(StringBuilder buffer, Instant instant) {
@@ -772,15 +786,15 @@ public void formatTo(StringBuilder buffer, Instant instant) {
                 return new AbstractFormatter(pattern, locale, timeZone, precision) {
                     @Override
                     public void formatTo(StringBuilder buffer, Instant instant) {
-                        formatSeconds(buffer, instant);
+                        secondDigitsFormatter.accept(buffer, instant);
                         buffer.append(separator);
                     }
                 };
             }
             return new AbstractFormatter(pattern, locale, timeZone, precision) {
                 @Override
                 public void formatTo(StringBuilder buffer, Instant instant) {
-                    formatSeconds(buffer, instant);
+                    secondDigitsFormatter.accept(buffer, instant);
                     buffer.append(separator);
                     fractionDigitsFormatter.accept(buffer, instant);
                 }
@@ -795,15 +809,15 @@ PatternSequence tryMerge(PatternSequence other, ChronoUnit thresholdPrecision) {
                 StaticPatternSequence staticOther = (StaticPatternSequence) other;
                 if (fractionalDigits == 0) {
                     return new SecondPatternSequence(
-                            printSeconds, this.separator + staticOther.literal, fractionalDigits);
+                            this.secondDigits, this.separator + staticOther.literal, fractionalDigits);
                 }
             }
             // We can always append more fractional digits
             if (other instanceof SecondPatternSequence) {
                 SecondPatternSequence secondOther = (SecondPatternSequence) other;
-                if (!secondOther.printSeconds && secondOther.separator.isEmpty()) {
+                if (secondOther.secondDigits == 0 && secondOther.separator.isEmpty()) {
                     return new SecondPatternSequence(
-                            printSeconds, this.separator, this.fractionalDigits + secondOther.fractionalDigits);
+                            this.secondDigits, this.separator, this.fractionalDigits + secondOther.fractionalDigits);
                 }
             }
             return null;