gnodet-bot commented on code in PR #13695: URL: https://github.com/apache/maven/pull/13695#discussion_r4367291148
########## impl/maven-logging/src/main/java/org/apache/maven/logging/impl/DefaultLoggerManager.java: ########## @@ -0,0 +1,325 @@ +/* + * Licensed to the Apache Software Foundation (ASF) under one + * or more contributor license agreements. See the NOTICE file + * distributed with this work for additional information + * regarding copyright ownership. The ASF licenses this file + * to you 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.apache.maven.logging.impl; + +import java.io.InputStream; +import java.util.List; +import java.util.Properties; +import java.util.concurrent.ConcurrentHashMap; +import java.util.concurrent.ConcurrentLinkedQueue; +import java.util.concurrent.CopyOnWriteArrayList; +import java.util.concurrent.atomic.AtomicReference; + +import org.apache.maven.api.Constants; +import org.apache.maven.api.logging.LogEvent; +import org.apache.maven.api.logging.LogLevel; +import org.apache.maven.api.logging.LogSink; +import org.apache.maven.api.logging.Logger; +import org.apache.maven.api.logging.LoggerManager; + +/** + * Default {@link LoggerManager} implementation. Owns: + * <ul> + * <li>Per-logger level configuration (loaded from properties files)</li> + * <li>The active {@link LogSink}</li> + * <li>Severity tracking for {@code --fail-on-severity}</li> + * </ul> + * <p> + * This class has no SLF4J dependency. The SLF4J adapter layer creates + * an instance and registers it via {@link LoggerManager#set(LoggerManager)}. + */ +public class DefaultLoggerManager implements LoggerManager { + + private static final String CONFIGURATION_FILE = "maven.logger.properties"; + + @Deprecated(since = "4.0.0") + private static final String LEGACY_CONFIGURATION_FILE = "simplelogger.properties"; + + private static final String LEGACY_PREFIX = "org.slf4j.simpleLogger."; + + public static final int LOG_LEVEL_TRACE = 0; + public static final int LOG_LEVEL_DEBUG = 10; + public static final int LOG_LEVEL_INFO = 20; + public static final int LOG_LEVEL_WARN = 30; + public static final int LOG_LEVEL_ERROR = 40; + public static final int LOG_LEVEL_OFF = 50; + + private volatile LogSink logSink; + private volatile int defaultLogLevel = LOG_LEVEL_INFO; + private final AtomicReference<LogLevel> maxReached = new AtomicReference<>(null); + private final Properties properties = new Properties(); + private final ConcurrentHashMap<String, Integer> levelCache = new ConcurrentHashMap<>(); + private final List<Runnable> reconfigurationListeners = new CopyOnWriteArrayList<>(); + + public DefaultLoggerManager() { + this.logSink = new AccumulatingLogSink(); + loadConfiguration(); + } + + // -- LogManager: loggers -- + + @Override + public Logger getLogger(String name) { + return new DefaultLogger(name); + } + + // -- LogManager: sink -- + + @Override + public LogSink getLogSink() { + return logSink; + } + + @Override + public void setLogSink(LogSink sink) { + LogSink old = this.logSink; + // Drain BEFORE making the new sink visible to concurrent doLog() callers. + // If we swapped first, a concurrent logger could write directly to the new sink + // while drainTo() is still replaying older buffered events — breaking ordering. + if (old instanceof AccumulatingLogSink acc && sink != null) { + acc.drainTo(sink); + } + this.logSink = sink; + } + + /** + * A sink that buffers events until a real sink is installed. + * Used as the default during early bootstrap. + * <p> + * Uses {@link ConcurrentLinkedQueue} with {@code poll()}-based draining to avoid + * the TOCTOU race inherent in {@code CopyOnWriteArrayList}: iterating a snapshot + * and then calling {@code clear()} can silently drop events added concurrently + * between the end of the iteration and the {@code clear()} call. + */ + private static class AccumulatingLogSink implements LogSink { + private final ConcurrentLinkedQueue<LogEvent> events = new ConcurrentLinkedQueue<>(); + + @Override + public void accept(LogEvent event) { + events.add(event); + } + + void drainTo(LogSink target) { + LogEvent event; + while ((event = events.poll()) != null) { + target.accept(event); + } + } + } + + // -- LogManager: level checking -- + + @Override + public boolean isLevelEnabled(String loggerName, LogLevel level) { + int configuredLevel = getLoggerLevel(loggerName); + return toIntLevel(level) >= configuredLevel; + } + + @Override Review Comment: 🔴 **[Not addressed] `setRootLoggerLevel()` does not call `reconfigure()` — `levelCache` stays stale.** After writing the system property, `levelCache` is never cleared. Callers who invoke `setRootLoggerLevel()` programmatically (e.g. the CLI `-X` flag processing) will see no change in effective levels until a separate `reconfigure()` call. Add `loadConfiguration()` (or `reconfigure()`) after the `setProperty` call. ```suggestion System.setProperty(Constants.MAVEN_LOGGER_DEFAULT_LOG_LEVEL, value); loadConfiguration(); ``` ########## impl/maven-core/src/main/java/org/apache/maven/logging/ProjectBuildLogAppender.java: ########## @@ -109,29 +116,110 @@ public static void setForkingMojoId(String forkingMojoId) { } } - public static void updateMdc() { - String id = getProjectId(); - if (id != null) { - MDC.put(KEY_PROJECT_ID, id); - } else { - MDC.remove(KEY_PROJECT_ID); - } + private static final AtomicLong SEQUENCE = new AtomicLong(); + + private static volatile Consumer<LogEvent> reportCapture; + + public static void setReportCapture(Consumer<LogEvent> capture) { + ProjectBuildLogAppender.reportCapture = capture; } private final BuildEventListener buildEventListener; + private final ConcurrentLinkedQueue<LogEvent> queue = new ConcurrentLinkedQueue<>(); + private final ReentrantLock drainLock = new ReentrantLock(); + private volatile boolean needsMetadata; Review Comment: 🟠 **[Not addressed] `needsMetadata` field is not `volatile` but is read from multiple threads.** `setNeedsMetadata()` writes from the setup/configuration thread. The inner `Sink` class reads `needsMetadata` from logging threads. Without `volatile`, the JMM does not guarantee the logging threads will ever see the update — the field read may be indefinitely cached in a register or CPU cache. ```suggestion private volatile boolean needsMetadata; ``` ########## impl/maven-logging/src/main/java/org/apache/maven/logging/impl/DefaultLoggerManager.java: ########## @@ -0,0 +1,325 @@ +/* + * Licensed to the Apache Software Foundation (ASF) under one + * or more contributor license agreements. See the NOTICE file + * distributed with this work for additional information + * regarding copyright ownership. The ASF licenses this file + * to you 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.apache.maven.logging.impl; + +import java.io.InputStream; +import java.util.List; +import java.util.Properties; +import java.util.concurrent.ConcurrentHashMap; +import java.util.concurrent.ConcurrentLinkedQueue; +import java.util.concurrent.CopyOnWriteArrayList; +import java.util.concurrent.atomic.AtomicReference; + +import org.apache.maven.api.Constants; +import org.apache.maven.api.logging.LogEvent; +import org.apache.maven.api.logging.LogLevel; +import org.apache.maven.api.logging.LogSink; +import org.apache.maven.api.logging.Logger; +import org.apache.maven.api.logging.LoggerManager; + +/** + * Default {@link LoggerManager} implementation. Owns: + * <ul> + * <li>Per-logger level configuration (loaded from properties files)</li> + * <li>The active {@link LogSink}</li> + * <li>Severity tracking for {@code --fail-on-severity}</li> + * </ul> + * <p> + * This class has no SLF4J dependency. The SLF4J adapter layer creates + * an instance and registers it via {@link LoggerManager#set(LoggerManager)}. + */ +public class DefaultLoggerManager implements LoggerManager { + + private static final String CONFIGURATION_FILE = "maven.logger.properties"; + + @Deprecated(since = "4.0.0") + private static final String LEGACY_CONFIGURATION_FILE = "simplelogger.properties"; + + private static final String LEGACY_PREFIX = "org.slf4j.simpleLogger."; + + public static final int LOG_LEVEL_TRACE = 0; + public static final int LOG_LEVEL_DEBUG = 10; + public static final int LOG_LEVEL_INFO = 20; + public static final int LOG_LEVEL_WARN = 30; + public static final int LOG_LEVEL_ERROR = 40; + public static final int LOG_LEVEL_OFF = 50; + + private volatile LogSink logSink; + private volatile int defaultLogLevel = LOG_LEVEL_INFO; + private final AtomicReference<LogLevel> maxReached = new AtomicReference<>(null); + private final Properties properties = new Properties(); + private final ConcurrentHashMap<String, Integer> levelCache = new ConcurrentHashMap<>(); + private final List<Runnable> reconfigurationListeners = new CopyOnWriteArrayList<>(); + + public DefaultLoggerManager() { + this.logSink = new AccumulatingLogSink(); + loadConfiguration(); + } + + // -- LogManager: loggers -- + + @Override + public Logger getLogger(String name) { + return new DefaultLogger(name); + } + + // -- LogManager: sink -- + + @Override + public LogSink getLogSink() { + return logSink; + } + + @Override + public void setLogSink(LogSink sink) { + LogSink old = this.logSink; + // Drain BEFORE making the new sink visible to concurrent doLog() callers. + // If we swapped first, a concurrent logger could write directly to the new sink + // while drainTo() is still replaying older buffered events — breaking ordering. + if (old instanceof AccumulatingLogSink acc && sink != null) { + acc.drainTo(sink); + } + this.logSink = sink; + } + + /** + * A sink that buffers events until a real sink is installed. + * Used as the default during early bootstrap. + * <p> + * Uses {@link ConcurrentLinkedQueue} with {@code poll()}-based draining to avoid + * the TOCTOU race inherent in {@code CopyOnWriteArrayList}: iterating a snapshot + * and then calling {@code clear()} can silently drop events added concurrently + * between the end of the iteration and the {@code clear()} call. + */ + private static class AccumulatingLogSink implements LogSink { + private final ConcurrentLinkedQueue<LogEvent> events = new ConcurrentLinkedQueue<>(); + + @Override + public void accept(LogEvent event) { + events.add(event); + } + + void drainTo(LogSink target) { + LogEvent event; + while ((event = events.poll()) != null) { + target.accept(event); + } + } + } + + // -- LogManager: level checking -- + + @Override + public boolean isLevelEnabled(String loggerName, LogLevel level) { Review Comment: 🔴 **[Not addressed] Bug: `TRACE` level silently maps to `"debug"` — TRACE logging unreachable.** `case TRACE, DEBUG -> "debug"` passes the string `"debug"` to `stringToLevel()`, which returns `LOG_LEVEL_DEBUG` (10). `LOG_LEVEL_TRACE` (0) is never reachable via `setRootLoggerLevel()`. Any caller using `setRootLoggerLevel(LogLevel.TRACE)` (e.g. `-X` flag) silently gets DEBUG filtering. ```suggestion String value = switch (level) { case TRACE -> "trace"; case DEBUG -> "debug"; case INFO -> "info"; case WARN -> "warn"; case ERROR -> "error"; }; ``` ########## impl/maven-logging/src/main/java/org/apache/maven/logging/impl/DefaultLogEvent.java: ########## @@ -0,0 +1,246 @@ +/* + * Licensed to the Apache Software Foundation (ASF) under one + * or more contributor license agreements. See the NOTICE file + * distributed with this work for additional information + * regarding copyright ownership. The ASF licenses this file + * to you 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.apache.maven.logging.impl; + +import java.time.Instant; +import java.util.Map; + +import org.apache.maven.api.logging.LogEvent; +import org.apache.maven.api.logging.LogLevel; + +/** + * Immutable implementation of {@link LogEvent} with lazy message formatting. + * <p> + * When constructed with a {@code messageTemplate} and {@code messageArgs}, + * the formatted {@link #message()} is computed lazily on first access. + * When constructed with a pre-formatted message, it is returned as-is. + * + * @since 4.1.0 + */ +public final class DefaultLogEvent implements LogEvent { + + private static final String PLACEHOLDER = "{}"; + + private final Instant timestamp; + private final LogLevel level; + private final String message; + private final String loggerName; + private final String stackTrace; + private final Throwable throwable; + private final String sourceClassName; + private final String sourceMethodName; + private final long threadId; + private final long sequenceNumber; + private final String threadName; + private final String projectId; + private final String mojoId; + private final Map<String, String> keyValues; + private final String messageTemplate; + private final Object[] messageArgs; + + private volatile String formattedMessage; + + @SuppressWarnings("checkstyle:ParameterNumber") + public DefaultLogEvent( + Instant timestamp, + LogLevel level, + String message, + String loggerName, + String stackTrace, + Throwable throwable, + String sourceClassName, + String sourceMethodName, + long threadId, + long sequenceNumber, + String threadName, + String projectId, + String mojoId, + Map<String, String> keyValues, + String messageTemplate, + Object[] messageArgs) { + this.timestamp = timestamp; + this.level = level; + this.message = message; + this.loggerName = loggerName; + this.stackTrace = stackTrace; + this.throwable = throwable; + this.sourceClassName = sourceClassName; + this.sourceMethodName = sourceMethodName; + this.threadId = threadId; + this.sequenceNumber = sequenceNumber; + this.threadName = threadName; + this.projectId = projectId; + this.mojoId = mojoId; + this.keyValues = keyValues != null ? keyValues : Map.of(); + this.messageTemplate = messageTemplate; + this.messageArgs = messageArgs; + } + + /** + * Convenience constructor for events without structured data. + */ + @SuppressWarnings("checkstyle:ParameterNumber") + public DefaultLogEvent( + Instant timestamp, + LogLevel level, + String message, + String loggerName, + String stackTrace, + Throwable throwable, + String sourceClassName, + String sourceMethodName, + long threadId, + long sequenceNumber, + String threadName, + String projectId, + String mojoId) { + this( + timestamp, + level, + message, + loggerName, + stackTrace, + throwable, + sourceClassName, + sourceMethodName, + threadId, + sequenceNumber, + threadName, + projectId, + mojoId, + Map.of(), + null, + null); + } + + /** + * Convenience constructor for SLF4J events (no source metadata). + */ + public DefaultLogEvent(Instant timestamp, LogLevel level, String message, String loggerName, String stackTrace) { + this(timestamp, level, message, loggerName, stackTrace, null, null, null, -1, -1, null, null, null); + } + + @Override + public Instant timestamp() { + return timestamp; + } + + @Override + public LogLevel level() { + return level; + } + + @Override + public String message() { + if (message != null) { + return message; + } + String m = formattedMessage; + if (m == null) { + m = formatMessage(messageTemplate, messageArgs); + formattedMessage = m; + } + return m; + } + + @Override + public String loggerName() { + return loggerName; + } + + @Override + public String stackTrace() { + return stackTrace; + } + + @Override + public Throwable throwable() { + return throwable; + } + + @Override + public String sourceClassName() { + return sourceClassName; + } + + @Override + public String sourceMethodName() { + return sourceMethodName; + } + + @Override + public long threadId() { + return threadId; + } + + @Override + public long sequenceNumber() { + return sequenceNumber; + } + + @Override + public String threadName() { + return threadName; + } + + @Override + public String projectId() { + return projectId; + } + + @Override + public String mojoId() { + return mojoId; + } + + @Override + public Map<String, String> keyValues() { + return keyValues; + } + + @Override + public String messageTemplate() { + return messageTemplate; + } + + @Override + public Object[] messageArgs() { + return messageArgs != null ? messageArgs.clone() : null; Review Comment: 🟡 **[Not addressed] Mutable internal array returned from `@Immutable` type.** `LogEvent` is annotated `@Immutable`. `messageArgs()` returns the raw `Object[]` stored in the field — callers can write to it, mutating the event's lazy message formatting in flight (e.g. if the sink formats the message lazily from a different thread than the logger thread). ```suggestion @Override public Object[] messageArgs() { return messageArgs != null ? messageArgs.clone() : null; } ``` ########## api/maven-api-log/pom.xml: ########## @@ -0,0 +1,43 @@ +<?xml version="1.0" encoding="UTF-8"?> +<!-- + Licensed to the Apache Software Foundation (ASF) under one + or more contributor license agreements. See the NOTICE file + distributed with this work for additional information + regarding copyright ownership. The ASF licenses this file + to you 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. +--> +<project xmlns="http://maven.apache.org/POM/4.0.0"; xmlns:xsi="http://www.w3.org/2001/XMLSchema-instance"; xsi:schemaLocation="http://maven.apache.org/POM/4.0.0 http://maven.apache.org/xsd/maven-4.0.0.xsd";> + <modelVersion>4.0.0</modelVersion> + + <parent> + <groupId>org.apache.maven</groupId> + <artifactId>maven-api</artifactId> + <version>4.1.0-SNAPSHOT</version> + </parent> + + <artifactId>maven-api-log</artifactId> + <name>Maven 4 API :: Logging</name> + <description>Maven 4 API - Unified Logging API (Logger, LogEventBuilder, LogLevel, LogEvent, LogSink).</description> Review Comment: 🟡 **[Not addressed] Stale `<description>`: lists `Log, LogBuilder` — actual types are `Logger, LogEventBuilder`.** ```suggestion <description>Maven 4 API - Unified Logging API (Logger, LogEventBuilder, LogLevel, LogEvent, LogSink).</description> ``` ########## api/maven-api-log/src/main/java/org/apache/maven/api/logging/Logger.java: ########## @@ -0,0 +1,395 @@ +/* + * Licensed to the Apache Software Foundation (ASF) under one + * or more contributor license agreements. See the NOTICE file + * distributed with this work for additional information + * regarding copyright ownership. The ASF licenses this file + * to you 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.apache.maven.api.logging; + +import java.util.function.Supplier; + +import org.apache.maven.api.annotations.Experimental; +import org.apache.maven.api.annotations.Nonnull; +import org.apache.maven.api.annotations.Nullable; +import org.apache.maven.api.annotations.Provider; + +/** + * Unified logging interface for Maven plugins and CLI bootstrap code. + * <p> + * Replaces both {@code org.apache.maven.api.plugin.Log} (mojo logging) and + * {@code org.apache.maven.api.cli.Logger} (CLI bootstrap logging) with a single + * interface that supports a fluent builder API, structured key-value pairs, and + * lazy message evaluation. + * <p> + * Instances are obtained via dependency injection. A {@code @Singleton} bean + * may hold a long-lived {@code Logger} instance safely: context fields such as + * {@code projectId} and {@code mojoId} are resolved from a per-thread + * {@code LogContext} at event dispatch time, not at injection time. + * + * <h2>Fluent API (preferred)</h2> + * <pre>{@code + * log.atInfo() + * .withKeyValue("artifact", artifact.getId()) + * .log("Resolved {} files", count); + * }</pre> + * + * <h2>Legacy convenience methods</h2> + * <pre>{@code + * log.info("Compiling {} source files", count); + * log.warn("Deprecated configuration key: {}", key, e); + * }</pre> + * + * @since 4.1.0 + */ +@Experimental +@Provider +public interface Logger { + + /** + * The name of this logger, typically the fully qualified class name of + * the component that owns it. + * + * @return the logger name, never {@code null} + */ + @Nonnull + String name(); Review Comment: 🟡 **[Not addressed] Naming convention: `@Provider` types use noun-style accessors.** `Logger` is annotated `@Provider`. Maven 4 API convention for `@Provider`/`@Immutable` types: noun-style accessors (`name()`, `value()`, `groupId()`) — not the `getX()` JavaBeans prefix. `getName()` should be `name()`. Every new JDK immutable type since Java 8 follows this pattern; see apache/maven#13035 for the full rationale. ```suggestion @Nonnull String name(); ``` ########## impl/maven-logging/src/main/java/org/apache/maven/logging/impl/DefaultLoggerManager.java: ########## @@ -0,0 +1,325 @@ +/* + * Licensed to the Apache Software Foundation (ASF) under one + * or more contributor license agreements. See the NOTICE file + * distributed with this work for additional information + * regarding copyright ownership. The ASF licenses this file + * to you 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.apache.maven.logging.impl; + +import java.io.InputStream; +import java.util.List; +import java.util.Properties; +import java.util.concurrent.ConcurrentHashMap; +import java.util.concurrent.ConcurrentLinkedQueue; +import java.util.concurrent.CopyOnWriteArrayList; +import java.util.concurrent.atomic.AtomicReference; + +import org.apache.maven.api.Constants; +import org.apache.maven.api.logging.LogEvent; +import org.apache.maven.api.logging.LogLevel; +import org.apache.maven.api.logging.LogSink; +import org.apache.maven.api.logging.Logger; +import org.apache.maven.api.logging.LoggerManager; + +/** + * Default {@link LoggerManager} implementation. Owns: + * <ul> + * <li>Per-logger level configuration (loaded from properties files)</li> + * <li>The active {@link LogSink}</li> + * <li>Severity tracking for {@code --fail-on-severity}</li> + * </ul> + * <p> + * This class has no SLF4J dependency. The SLF4J adapter layer creates + * an instance and registers it via {@link LoggerManager#set(LoggerManager)}. + */ +public class DefaultLoggerManager implements LoggerManager { + + private static final String CONFIGURATION_FILE = "maven.logger.properties"; + + @Deprecated(since = "4.0.0") + private static final String LEGACY_CONFIGURATION_FILE = "simplelogger.properties"; + + private static final String LEGACY_PREFIX = "org.slf4j.simpleLogger."; + + public static final int LOG_LEVEL_TRACE = 0; + public static final int LOG_LEVEL_DEBUG = 10; + public static final int LOG_LEVEL_INFO = 20; + public static final int LOG_LEVEL_WARN = 30; + public static final int LOG_LEVEL_ERROR = 40; + public static final int LOG_LEVEL_OFF = 50; + + private volatile LogSink logSink; + private volatile int defaultLogLevel = LOG_LEVEL_INFO; Review Comment: 🟠 **[Not addressed] `maxReached` initialized to `LogLevel.DEBUG` — `hasReached(DEBUG)` is `true` before anything is logged.** `LogLevel.TRACE.ordinal()` (0) < `LogLevel.DEBUG.ordinal()` (1), so `hasReached(LogLevel.TRACE)` returns `true` even before a single log event is emitted. `reconfigure()` resets to `DEBUG` for the same reason — after reset, `hasReached(TRACE)` is immediately true again. Initialize to `TRACE` (the lowest severity) so the tracker starts clean. ```suggestion private final AtomicReference<LogLevel> maxReached = new AtomicReference<>(LogLevel.TRACE); ``` ########## impl/maven-logging/src/main/java/org/apache/maven/logging/impl/DefaultLoggerManager.java: ########## @@ -0,0 +1,325 @@ +/* + * Licensed to the Apache Software Foundation (ASF) under one + * or more contributor license agreements. See the NOTICE file + * distributed with this work for additional information + * regarding copyright ownership. The ASF licenses this file + * to you 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.apache.maven.logging.impl; + +import java.io.InputStream; +import java.util.List; +import java.util.Properties; +import java.util.concurrent.ConcurrentHashMap; +import java.util.concurrent.ConcurrentLinkedQueue; +import java.util.concurrent.CopyOnWriteArrayList; +import java.util.concurrent.atomic.AtomicReference; + +import org.apache.maven.api.Constants; +import org.apache.maven.api.logging.LogEvent; +import org.apache.maven.api.logging.LogLevel; +import org.apache.maven.api.logging.LogSink; +import org.apache.maven.api.logging.Logger; +import org.apache.maven.api.logging.LoggerManager; + +/** + * Default {@link LoggerManager} implementation. Owns: + * <ul> + * <li>Per-logger level configuration (loaded from properties files)</li> + * <li>The active {@link LogSink}</li> + * <li>Severity tracking for {@code --fail-on-severity}</li> + * </ul> + * <p> + * This class has no SLF4J dependency. The SLF4J adapter layer creates + * an instance and registers it via {@link LoggerManager#set(LoggerManager)}. + */ +public class DefaultLoggerManager implements LoggerManager { + + private static final String CONFIGURATION_FILE = "maven.logger.properties"; + + @Deprecated(since = "4.0.0") + private static final String LEGACY_CONFIGURATION_FILE = "simplelogger.properties"; + + private static final String LEGACY_PREFIX = "org.slf4j.simpleLogger."; + + public static final int LOG_LEVEL_TRACE = 0; + public static final int LOG_LEVEL_DEBUG = 10; + public static final int LOG_LEVEL_INFO = 20; + public static final int LOG_LEVEL_WARN = 30; + public static final int LOG_LEVEL_ERROR = 40; + public static final int LOG_LEVEL_OFF = 50; + + private volatile LogSink logSink; + private volatile int defaultLogLevel = LOG_LEVEL_INFO; + private final AtomicReference<LogLevel> maxReached = new AtomicReference<>(null); + private final Properties properties = new Properties(); + private final ConcurrentHashMap<String, Integer> levelCache = new ConcurrentHashMap<>(); + private final List<Runnable> reconfigurationListeners = new CopyOnWriteArrayList<>(); + + public DefaultLoggerManager() { + this.logSink = new AccumulatingLogSink(); + loadConfiguration(); + } + + // -- LogManager: loggers -- + + @Override + public Logger getLogger(String name) { + return new DefaultLogger(name); + } + + // -- LogManager: sink -- + + @Override + public LogSink getLogSink() { + return logSink; + } + + @Override + public void setLogSink(LogSink sink) { + LogSink old = this.logSink; Review Comment: 🔴 **[Not addressed] Race condition: new sink becomes visible before old events are drained.** `this.logSink = sink` (line 91) is visible to all threads via `volatile` as soon as it's written. Between that write and `acc.drainTo(sink)` (line 93), another thread can log new events that go directly to the new sink — these arrive *before* the accumulated bootstrap events are drained. The fix is to drain first, then publish the new sink. ```suggestion @Override public void setLogSink(LogSink sink) { LogSink old = this.logSink; if (old instanceof AccumulatingLogSink acc && sink != null) { acc.drainTo(sink); } this.logSink = sink; } ``` ########## impl/maven-logging/src/main/java/org/apache/maven/logging/impl/DefaultLogEventBuilder.java: ########## @@ -0,0 +1,118 @@ +/* + * Licensed to the Apache Software Foundation (ASF) under one + * or more contributor license agreements. See the NOTICE file + * distributed with this work for additional information + * regarding copyright ownership. The ASF licenses this file + * to you 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.apache.maven.logging.impl; + +import java.util.LinkedHashMap; +import java.util.Map; +import java.util.function.Supplier; + +import org.apache.maven.api.logging.LogEventBuilder; +import org.apache.maven.api.logging.LogLevel; + +/** + * Active {@link LogEventBuilder} returned by {@link DefaultLogger} when the target level is enabled. + * <p> + * Collects key-value pairs, throwable, and override context fields, then dispatches + * the event via {@link DefaultLogger#doLog} on the terminal {@code log()} call. + * Message formatting is deferred to the {@link DefaultLogEvent} — the raw template + * and arguments are passed through so that sinks can access structured data. + * + * @since 4.1.0 + */ +final class DefaultLogEventBuilder implements LogEventBuilder { + + private final DefaultLogger log; + private final LogLevel level; + + private Map<String, String> kvMap; + private Throwable throwable; + private String projectId; + private String mojoId; + + DefaultLogEventBuilder(DefaultLogger log, LogLevel level) { + this.log = log; + this.level = level; + } + + @Override + public LogEventBuilder withKeyValue(String key, String value) { + ensureKv(); + kvMap.put(key, value); + return this; + } + + @Override + public LogEventBuilder withKeyValue(String key, long value) { + return withKeyValue(key, Long.toString(value)); + } + + @Override + public LogEventBuilder withKeyValue(String key, boolean value) { + return withKeyValue(key, Boolean.toString(value)); + } + + @Override + public LogEventBuilder withThrowable(Throwable t) { + this.throwable = t; + return this; + } + + @Override Review Comment: 🟡 **[Not addressed] `withProjectId()` is a silent no-op — contradicts API Javadoc.** The `LogEventBuilder` interface documents `withProjectId()` as "Overrides the projectId for this event." The implementation returns `this` without storing anything. The override is silently discarded when `doLog()` assembles the event. Same issue in `withMojoId()` (line 80). Either store the value and pass it through `doLog()`, or add `@Override` documentation clarifying that `DefaultLogEventBuilder` uses the thread-local context instead and ignores the explicit value. ```suggestion @Override public LogEventBuilder withProjectId(String projectId) { this.projectId = projectId; return this; } ``` (Requires adding a `private String projectId;` field and wiring it into `doLog()`.) ########## api/maven-api-log/src/main/java/org/apache/maven/api/logging/LogEventBuilder.java: ########## @@ -0,0 +1,153 @@ +/* + * Licensed to the Apache Software Foundation (ASF) under one + * or more contributor license agreements. See the NOTICE file + * distributed with this work for additional information + * regarding copyright ownership. The ASF licenses this file + * to you 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.apache.maven.api.logging; + +import org.apache.maven.api.annotations.Experimental; +import org.apache.maven.api.annotations.Nonnull; + +/** + * Fluent builder for a single log statement. + * <p> + * Instances are obtained from {@link Logger#atTrace()}, {@link Logger#atDebug()}, etc. Review Comment: 🟡 **[Not addressed] Stale Javadoc references: `{@link Log#atTrace()}` — the type in this module is `Logger`, not `Log`.** ```suggestion * Instances are obtained from {@link Logger#atTrace()}, {@link Logger#atDebug()}, etc. ``` ########## api/maven-api-log/src/main/java/org/apache/maven/api/logging/LogSink.java: ########## @@ -0,0 +1,61 @@ +/* + * Licensed to the Apache Software Foundation (ASF) under one + * or more contributor license agreements. See the NOTICE file + * distributed with this work for additional information + * regarding copyright ownership. The ASF licenses this file + * to you 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.apache.maven.api.logging; + +import org.apache.maven.api.annotations.Experimental; +import org.apache.maven.api.annotations.Nonnull; + +/** + * Central dispatch point for log events. + * <p> + * All logging paths — the fluent {@link Logger} API, SLF4J Review Comment: 🟡 **[Not addressed] Stale Javadoc reference: `{@link Log}` should be `{@link Logger}`.** ```suggestion * All logging paths — the fluent {@link Logger} API, SLF4J ``` -- 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]
