`InstantPatternLegacyFormatter` computes precision incorrectly

[vy]

InstantPatternLegacyFormatter uses InstantPatternDynamicFormatter to determine the precision of the legacy date & time pattern it receives. As reported by @ppkarwasz (see the review discussion), this results in incorrect precision (i.e., microseconds are currently classified as nanoseconds), and hence, causes incorrect cache invalidation.

In 3799799, @ppkarwasz fixes this by converting the legacy pattern back to DateTimeFormatter-compatible (i.e., modern) variant before passing it to InstantPatternDynamicFormatter. I am personally in favor of copying InstantPatternDynamicFormatter::patternPrecision to InstantPatternLegacyFormatter, and adapting it, since this will ensure legacy code remains an island.

[ppkarwasz]

Hi @vy,

As reported by @ppkarwasz (see the review discussion), this results in incorrect precision (i.e., microseconds are currently classified as nanoseconds), and hence, causes incorrect cache invalidation.

This is something we should fix to ensure all implementations adhere to the InstantPattern contract. However, the issue currently has no observable effect:

• InstantPatternLegacyFormatter results are not cached.
• Even if they were, we only cache timestamps with millisecond precision, so higher-resolution discrepancies (microseconds vs nanoseconds) don’t matter in practice.

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

Lines 125 to 152 in 8ec5703

	// Return legacy formatters, if requested
	if (legacyFormattersEnabled) {
	return new InstantPatternLegacyFormatter(pattern, locale, timeZone);
	}
	// Create the formatter, and return it, if caching is disabled
	final InstantPatternDynamicFormatter formatter =
	new InstantPatternDynamicFormatter(pattern, locale, timeZone);
	if (!cachingEnabled) {
	return formatter;
	}
	// Wrap the formatter with caching, if necessary
	switch (formatter.getPrecision()) {
	// It is not worth caching when a precision equal to or higher than microsecond is requested
	case NANOS:
	case MICROS:
	return formatter;
	// Millisecond precision cache
	case MILLIS:
	return InstantPatternThreadLocalCachedFormatter.ofMilliPrecision(formatter);
	// Cache everything else with second precision
	default:
	return InstantPatternThreadLocalCachedFormatter.ofSecondPrecision(formatter);
	}

In 3799799, @ppkarwasz fixes this by converting the legacy pattern back to DateTimeFormatter-compatible (i.e., modern) variant before passing it to InstantPatternDynamicFormatter. I am personally in favor of copying InstantPatternDynamicFormatter::patternPrecision to InstantPatternLegacyFormatter, and adapting it, since this will ensure legacy code remains an island.

I don't think duplicating code is necessary here: InstantPatternLegacyFormatter has been destined for deprecation and subsequent removal from the start.

Once FixedDateFormat is removed, we should retire both InstantPatternLegacyFormatter and support for legacy patterns. There's no reason to continue supporting legacy patterns with the new implementation. Since enabling them requires a manual configuration change, users might as well migrate to the modern DTF-style patterns.