Skip to content
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

Allow DateTimeFormatter constants to be used for the date pattern. #373

Open
wants to merge 1 commit into
base: main
Choose a base branch
from
Open
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
Original file line number Diff line number Diff line change
Expand Up @@ -477,8 +477,7 @@ public static FormatStep dateFormatStep(final TimeZone timeZone, final String fo
final int minimumWidth,
final boolean truncateBeginning, final int maximumWidth) {
return new JustifyingFormatStep(leftJustify, minimumWidth, truncateBeginning, maximumWidth) {
final DateTimeFormatter dtf = DateTimeFormatter
.ofPattern(formatString == null ? "yyyy-MM-DD HH:mm:ss,SSS" : formatString);
final DateTimeFormatter dtf = StandardDateFormat.resolve(formatString);
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

What if instead of the enum, you did something along these lines in (half-Java, half-pseudocode):

private DateTimeFormatter resolve(String formatString) {
    if (formatString.length() > 1 && formatString.charAt(0) == '#') {
        formatString = formatString.substring(1);
        if (formatString.charAt(0) != '#') { // double-## means pass thru
            switch (formatString) {
                case "ISO_DATE": return DateTimeFormatter.ISO_DATE;
                // ... and one case for each known string that we want to allow ...
                default: return DateTimeFormatter.ofPattern("'<Unknown format " + formatString + ">'");
            }
        }
    }
    return DateTimeFormatter.ofPattern(formatString);
}

This keeps the code minimal and simple, is guaranteed to be compatible with future DateTimeFormatter utilizations of #, and is forward-compatible as well in case we want to support fancier things like putting the format name in mid-string or whatever.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Do you mean the text format would be something like %d{#ISO_DATE}?

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Exactly. This way we never need to be concerned about syntactic conflicts between constant names and format strings.

Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

On the other hand, that character really has no meaningful use from the user standpoint, with the exception of identifying, by convention, date/time pattern formats from constants — something you can achieve performing proper parsing.

I find it more confusing, in the sense that single characters in that particular string (e.g.: %date{ISO8601} |- %highlight(%5level) in %cyan(%logger{32}:%line) %magenta([%t - %m%n%ex) have a particular meaning.

Still it's a minor detail, but it amounts.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@x80486 The main reason for the prefix character is most characters are reserved for a DateTimePattern. While ISO_DATE or ISO08601 is likely not going to bean issue, it could be a valid pattern at some point.


public ItemType getItemType() {
return ItemType.DATE;
Expand Down
Original file line number Diff line number Diff line change
@@ -0,0 +1,77 @@
/*
* JBoss, Home of Professional Open Source.
*
* Copyright 2023 Red Hat, Inc., and individual contributors
* as indicated by the @author tags.
*
* Licensed under the Apache License, Version 2.0 (the "License");
* you may not use this file except in compliance with the License.
* You may obtain a copy of the License at
*
* http://www.apache.org/licenses/LICENSE-2.0
*
* Unless required by applicable law or agreed to in writing, software
* distributed under the License is distributed on an "AS IS" BASIS,
* WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
* See the License for the specific language governing permissions and
* limitations under the License.
*/

package org.jboss.logmanager.formatters;

import java.time.format.DateTimeFormatter;
import java.util.Locale;

/**
* Standard date formats.
*
* @author <a href="mailto:jperkins@redhat.com">James R. Perkins</a>
*/
enum StandardDateFormat {
/**
* Format: {@code HH:mm:ss,SSS}
*/
ABSOLUTE("HH:mm:ss,SSS"),
/**
* Format: {@code yyyyMMddHHmmssSSS}
*/
COMPACT("yyyyMMddHHmmssSSS"),
/**
* Format: {@code yyyy-MM-dd HH:mm:ss,SSS}
*/
DEFAULT("yyyy-MM-dd HH:mm:ss,SSS"),
/**
* Format: {@code yyyy-MM-dd'T'HH:mm:ss,SSS}
*/
ISO8601("yyyy-MM-dd'T'HH:mm:ss,SSS"),
;

private final DateTimeFormatter formatter;
private final String pattern;

StandardDateFormat(final String pattern) {
this.formatter = DateTimeFormatter.ofPattern(pattern);
this.pattern = pattern;
}

/**
* Resolves the requested date format based on the pattern. If the pattern is {@code null}, then the {@link #DEFAULT}
* pattern is used. If the pattern matches one of the constants, that format will be returned. Otherwise, the
* pattern is assumed to be a valid {@link DateTimeFormatter#ofPattern(String)} format pattern}.
*
* @param pattern the pattern to return the formatter for
*
* @return the formatter used to format timestamps
*/
static DateTimeFormatter resolve(final String pattern) {
if (pattern == null) {
return DEFAULT.formatter;
}
for (StandardDateFormat constant : values()) {
if (pattern.toUpperCase(Locale.ROOT).equals(constant.name()) || pattern.equals(constant.pattern)) {
return constant.formatter;
}
}
return DateTimeFormatter.ofPattern(pattern);
}
}
Original file line number Diff line number Diff line change
Expand Up @@ -19,6 +19,10 @@

package org.jboss.logmanager.formatters;

import java.time.Instant;
import java.time.ZoneId;
import java.time.format.DateTimeFormatter;

import org.jboss.logmanager.ExtLogRecord;
import org.jboss.logmanager.MDC;
import org.jboss.logmanager.NDC;
Expand Down Expand Up @@ -269,6 +273,28 @@ public void unqualifiedHost() {
Assert.assertEquals("manager", formatter.format(record));
}

@Test
public void dateFormat() {
final var now = Instant.now();
final var record = createLogRecord("test", now);
var formatter = new PatternFormatter("%d{ISO8601}");
Assert.assertEquals(DateTimeFormatter.ofPattern("yyyy-MM-dd'T'HH:mm:ss,SSS").format(now.atZone(ZoneId.systemDefault())),
formatter.format(record));

formatter = new PatternFormatter("%d");
Assert.assertEquals(DateTimeFormatter.ofPattern("yyyy-MM-dd HH:mm:ss,SSS").format(now.atZone(ZoneId.systemDefault())),
formatter.format(record));

formatter = new PatternFormatter("%d{ABSOLUTE}");
Assert.assertEquals(DateTimeFormatter.ofPattern("HH:mm:ss,SSS").format(now.atZone(ZoneId.systemDefault())),
formatter.format(record));

formatter = new PatternFormatter("%d{compact}");
Assert.assertEquals(DateTimeFormatter.ofPattern("yyyyMMddHHmmssSSS").format(now.atZone(ZoneId.systemDefault())),
formatter.format(record));

}

@Test
public void qualifiedHost() {
final String hostName = "logmanager.jboss.org";
Expand Down Expand Up @@ -330,8 +356,13 @@ private void systemProperties(final String propertyPrefix) throws Exception {
}

protected static ExtLogRecord createLogRecord(final String msg) {
return createLogRecord(msg, Instant.now());
}

protected static ExtLogRecord createLogRecord(final String msg, final Instant instant) {
final ExtLogRecord result = new ExtLogRecord(org.jboss.logmanager.Level.INFO, msg,
PatternFormatterTests.class.getName());
result.setInstant(instant);
result.setSourceClassName(PatternFormatterTests.class.getName());
result.setLoggerName(CATEGORY);
return result;
Expand Down