vy commented on code in PR #2424:
URL: https://github.com/apache/logging-log4j2/pull/2424#discussion_r1543618080
##########
log4j-api/src/main/java/org/apache/logging/log4j/util/PropertiesUtil.java:
##########
@@ -649,45 +674,57 @@ public static Map<String, Properties>
partitionOnCommonPrefixes(
* @return true if system properties tell us we are running on Windows.
*/
public boolean isOsWindows() {
- return getStringProperty("os.name", "").startsWith("Windows");
+ return SystemPropertiesPropertySource.getSystemProperty("os.name",
"").startsWith("Windows");
+ }
+
+ static Duration parseDuration(final String value) {
+ final Matcher matcher = DURATION_PATTERN.matcher(value);
+ if (matcher.matches()) {
+ return Duration.of(TimeUnit.parseAmount(matcher),
TimeUnit.parseUnit(matcher));
+ }
+ throw new DateTimeParseException("Invalid duration value: should be in
the format nnn[unit].", value, 0);
}
private enum TimeUnit {
- NANOS("ns,nano,nanos,nanosecond,nanoseconds", ChronoUnit.NANOS),
- MICROS("us,micro,micros,microsecond,microseconds", ChronoUnit.MICROS),
- MILLIS("ms,milli,millis,millsecond,milliseconds", ChronoUnit.MILLIS),
- SECONDS("s,second,seconds", ChronoUnit.SECONDS),
- MINUTES("m,minute,minutes", ChronoUnit.MINUTES),
- HOURS("h,hour,hours", ChronoUnit.HOURS),
- DAYS("d,day,days", ChronoUnit.DAYS);
-
- /*
- * https://errorprone.info/bugpattern/ImmutableEnumChecker
- * This field is effectively immutable.
- */
- @SuppressWarnings("ImmutableEnumChecker")
- private final String[] descriptions;
+ NANOS(new String[] {"ns", "nano", "nanos", "nanosecond",
"nanoseconds"}, ChronoUnit.NANOS),
+ MICROS(new String[] {"us", "micro", "micros", "microsecond",
"microseconds"}, ChronoUnit.MICROS),
+ MILLIS(new String[] {"ms", "milli", "millis", "millisecond",
"milliseconds"}, ChronoUnit.MILLIS),
+ SECONDS(new String[] {"s", "second", "seconds"}, ChronoUnit.SECONDS),
+ MINUTES(new String[] {"m", "minute", "minutes"}, ChronoUnit.MINUTES),
+ HOURS(new String[] {"h", "hour", "hours"}, ChronoUnit.HOURS),
+ DAYS(new String[] {"d", "day", "days"}, ChronoUnit.DAYS);
- private final ChronoUnit timeUnit;
+ private final String[] descriptions;
+ private final TemporalUnit timeUnit;
- TimeUnit(final String descriptions, final ChronoUnit timeUnit) {
- this.descriptions = descriptions.split(",");
+ TimeUnit(final String[] descriptions, final TemporalUnit timeUnit) {
+ this.descriptions = descriptions;
this.timeUnit = timeUnit;
}
- static Duration getDuration(final String time) {
- final String value = time.trim();
- TemporalUnit temporalUnit = ChronoUnit.MILLIS;
- long timeVal = 0;
- for (TimeUnit timeUnit : values()) {
- for (String suffix : timeUnit.descriptions) {
- if (value.endsWith(suffix)) {
- temporalUnit = timeUnit.timeUnit;
- timeVal = Long.parseLong(value.substring(0,
value.length() - suffix.length()));
+ private static long parseAmount(final MatchResult matcher) {
Review Comment:
This method has nothing to do with `TimeUnit`. I would rather place it right
after `parseDuration` and rename it to `parseDurationAmount`.
##########
log4j-api/src/main/java/org/apache/logging/log4j/util/PropertiesUtil.java:
##########
@@ -649,45 +674,57 @@ public static Map<String, Properties>
partitionOnCommonPrefixes(
* @return true if system properties tell us we are running on Windows.
*/
public boolean isOsWindows() {
- return getStringProperty("os.name", "").startsWith("Windows");
+ return SystemPropertiesPropertySource.getSystemProperty("os.name",
"").startsWith("Windows");
+ }
+
+ static Duration parseDuration(final String value) {
+ final Matcher matcher = DURATION_PATTERN.matcher(value);
+ if (matcher.matches()) {
+ return Duration.of(TimeUnit.parseAmount(matcher),
TimeUnit.parseUnit(matcher));
+ }
+ throw new DateTimeParseException("Invalid duration value: should be in
the format nnn[unit].", value, 0);
}
private enum TimeUnit {
- NANOS("ns,nano,nanos,nanosecond,nanoseconds", ChronoUnit.NANOS),
- MICROS("us,micro,micros,microsecond,microseconds", ChronoUnit.MICROS),
- MILLIS("ms,milli,millis,millsecond,milliseconds", ChronoUnit.MILLIS),
- SECONDS("s,second,seconds", ChronoUnit.SECONDS),
- MINUTES("m,minute,minutes", ChronoUnit.MINUTES),
- HOURS("h,hour,hours", ChronoUnit.HOURS),
- DAYS("d,day,days", ChronoUnit.DAYS);
-
- /*
- * https://errorprone.info/bugpattern/ImmutableEnumChecker
- * This field is effectively immutable.
- */
- @SuppressWarnings("ImmutableEnumChecker")
- private final String[] descriptions;
+ NANOS(new String[] {"ns", "nano", "nanos", "nanosecond",
"nanoseconds"}, ChronoUnit.NANOS),
+ MICROS(new String[] {"us", "micro", "micros", "microsecond",
"microseconds"}, ChronoUnit.MICROS),
+ MILLIS(new String[] {"ms", "milli", "millis", "millisecond",
"milliseconds"}, ChronoUnit.MILLIS),
+ SECONDS(new String[] {"s", "second", "seconds"}, ChronoUnit.SECONDS),
+ MINUTES(new String[] {"m", "minute", "minutes"}, ChronoUnit.MINUTES),
+ HOURS(new String[] {"h", "hour", "hours"}, ChronoUnit.HOURS),
+ DAYS(new String[] {"d", "day", "days"}, ChronoUnit.DAYS);
- private final ChronoUnit timeUnit;
+ private final String[] descriptions;
+ private final TemporalUnit timeUnit;
- TimeUnit(final String descriptions, final ChronoUnit timeUnit) {
- this.descriptions = descriptions.split(",");
+ TimeUnit(final String[] descriptions, final TemporalUnit timeUnit) {
+ this.descriptions = descriptions;
this.timeUnit = timeUnit;
}
- static Duration getDuration(final String time) {
- final String value = time.trim();
- TemporalUnit temporalUnit = ChronoUnit.MILLIS;
- long timeVal = 0;
- for (TimeUnit timeUnit : values()) {
- for (String suffix : timeUnit.descriptions) {
- if (value.endsWith(suffix)) {
- temporalUnit = timeUnit.timeUnit;
- timeVal = Long.parseLong(value.substring(0,
value.length() - suffix.length()));
+ private static long parseAmount(final MatchResult matcher) {
+ final String amount = matcher.group(1);
+ try {
+ return Long.parseLong(amount);
+ } catch (final NumberFormatException e) {
+ throw new DateTimeParseException(
+ "Invalid time amount '" + amount + "'",
matcher.group(), matcher.start(1), e);
+ }
+ }
+
+ private static TemporalUnit parseUnit(final MatchResult matcher) {
Review Comment:
Why does `TimeUnit#parseUnit()` receive a `MatchResult` instead of a
`String`?
##########
log4j-api/src/main/java/org/apache/logging/log4j/util/PropertiesUtil.java:
##########
@@ -23,6 +23,7 @@
import java.io.InputStream;
import java.nio.charset.Charset;
import java.time.Duration;
+import java.time.format.DateTimeParseException;
Review Comment:
`DateTimeParseException` doesn't display any of its input arguments when
printed. Could you instead use `IllegalArgumentException` with self-explanatory
messages, please?
For instance, consider the following _example_:
```
final String message = String.format("provided duration `%s` doesn't match
the expected `%s` pattern", input, pattern);
throw new IllegalArgumentException(message, cause);
```
##########
log4j-api/src/main/java/org/apache/logging/log4j/util/PropertiesUtil.java:
##########
@@ -64,6 +68,8 @@ public final class PropertiesUtil {
private static final Lazy<PropertiesUtil> COMPONENT_PROPERTIES =
Lazy.lazy(() -> new PropertiesUtil(LOG4J_PROPERTIES_FILE_NAME,
false));
+ private static final Pattern DURATION_PATTERN =
Pattern.compile("([+-]?\\d+)\\s*(\\w+)?", Pattern.CASE_INSENSITIVE);
Review Comment:
*Nit:* I would rather dynamically build this pattern using the exact list of
supported values from `TimeUnit`.
##########
log4j-api/src/main/java/org/apache/logging/log4j/util/PropertiesUtil.java:
##########
@@ -649,45 +674,57 @@ public static Map<String, Properties>
partitionOnCommonPrefixes(
* @return true if system properties tell us we are running on Windows.
*/
public boolean isOsWindows() {
- return getStringProperty("os.name", "").startsWith("Windows");
+ return SystemPropertiesPropertySource.getSystemProperty("os.name",
"").startsWith("Windows");
+ }
+
+ static Duration parseDuration(final String value) {
+ final Matcher matcher = DURATION_PATTERN.matcher(value);
+ if (matcher.matches()) {
+ return Duration.of(TimeUnit.parseAmount(matcher),
TimeUnit.parseUnit(matcher));
+ }
+ throw new DateTimeParseException("Invalid duration value: should be in
the format nnn[unit].", value, 0);
Review Comment:
I don't think someone who is not able to correctly type a duration value can
understand what is expected from them by looking at the hinted `nnn[unit]`
format. Can we make the example/pattern a little bit more human-friendly?
##########
log4j-api/src/main/java/org/apache/logging/log4j/util/SystemPropertiesPropertySource.java:
##########
@@ -32,19 +34,32 @@
@ServiceProvider(value = PropertySource.class, resolution =
Resolution.OPTIONAL)
public class SystemPropertiesPropertySource implements PropertySource {
+ private static final Logger LOGGER = StatusLogger.getLogger();
private static final int DEFAULT_PRIORITY = 0;
private static final String PREFIX = "log4j2.";
+ private static final PropertySource INSTANCE = new
SystemPropertiesPropertySource();
+
/**
- * Used by bootstrap code to get system properties without loading
PropertiesUtil.
+ * Method used by Java 9+ to instantiate providers
+ * @since 2.24.0
+ * @see java.util.ServiceLoader
*/
+ public static PropertySource provider() {
+ return INSTANCE;
+ }
Review Comment:
1. What do these methods do?
2. What do they have to do with this PR?
--
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: [email protected]
For queries about this service, please contact Infrastructure at:
[email protected]
