gnodet-bot commented on code in PR #13695: URL: https://github.com/apache/maven/pull/13695#discussion_r4365388427
########## impl/maven-logging/src/main/java/org/apache/maven/logging/impl/DefaultLoggerManager.java: ########## @@ -0,0 +1,299 @@ +/* + * 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.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<>(LogLevel.DEBUG); Review Comment: đ **Bug: `maxReached` initialized to `LogLevel.DEBUG` before any logging occurs.** `hasReached(LogLevel.TRACE)` returns `true` immediately at startup (DEBUG ordinal 1 âĨ TRACE ordinal 0) even though nothing has been logged. This can produce false positives for `--fail-on-severity TRACE`. The initial state should represent "nothing logged yet" â consider using `null` and guarding `hasReached()` with a null check, or introducing a `NONE` sentinel below TRACE. Same issue in `reconfigure()` at line 140. ########## impl/maven-logging/src/main/java/org/apache/maven/logging/impl/DefaultLoggerManager.java: ########## @@ -0,0 +1,299 @@ +/* + * 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.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<>(LogLevel.DEBUG); + private final Properties properties = new Properties(); + private final ConcurrentHashMap<String, Integer> levelCache = new ConcurrentHashMap<>(); + + public DefaultLoggerManager() { + this.logSink = new AccumulatingLogSink(); + LoggerManager.set(this); + 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; + this.logSink = sink; + if (old instanceof AccumulatingLogSink acc && sink != null) { + acc.drainTo(sink); + } + } + + /** + * A sink that buffers events until a real sink is installed. + * Used as the default during early bootstrap. + */ + private static class AccumulatingLogSink implements LogSink { + private final List<LogEvent> events = new CopyOnWriteArrayList<>(); + + @Override + public void accept(LogEvent event) { + events.add(event); + } + + void drainTo(LogSink target) { + for (LogEvent event : events) { + target.accept(event); + } + events.clear(); + } + } + + // -- LogManager: level checking -- + + @Override + public boolean isLevelEnabled(String loggerName, LogLevel level) { + int configuredLevel = getLoggerLevel(loggerName); + return toIntLevel(level) >= configuredLevel; + } + + @Override + public void setRootLoggerLevel(LogLevel level) { + String value = + switch (level) { + case TRACE, DEBUG -> "debug"; Review Comment: đ´ **Bug: `TRACE` level maps to `"debug"` â TRACE logging is silently unreachable.** `stringToLevel("debug")` returns `LOG_LEVEL_DEBUG` (10); `stringToLevel("trace")` would return `LOG_LEVEL_TRACE` (0). A caller invoking `setRootLoggerLevel(LogLevel.TRACE)` intending to enable trace output will silently get debug-level logging instead. ```suggestion case TRACE -> "trace"; case DEBUG -> "debug"; ``` ########## impl/maven-logging/src/main/java/org/apache/maven/logging/impl/DefaultLogEventBuilder.java: ########## @@ -0,0 +1,114 @@ +/* + * 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; + + 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 + public LogEventBuilder withProjectId(String projectId) { + return this; + } + + @Override + public LogEventBuilder withMojoId(String mojoId) { + return this; Review Comment: đĄ **`withMojoId()` is a silent no-op â same issue as `withProjectId()`.** ########## api/maven-api-log/src/main/java/org/apache/maven/api/logging/Logger.java: ########## @@ -0,0 +1,358 @@ +/* + * 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 getName(); Review Comment: đĄ **Naming convention: `@Provider` types use noun-style accessors.** `Logger` is annotated `@Provider`. Maven 4 API convention for provider types avoids `get`-prefixed accessors (see `Session`, `Project`, etc. â all use `name()`, `version()`, etc.). Other methods on this interface (`isEnabled`, `atTrace`, `child`) follow the convention. `getName()` should be `name()` to be consistent â the longer this stays, the more painful the rename. ########## 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 boolean needsMetadata; Review Comment: đ **`needsMetadata` field is not `volatile` but is read from multiple threads.** `setNeedsMetadata()` (line 137) writes this field from one thread; the inner `Sink.needsMetadata()` (line 176) reads it from arbitrary logging threads. Without `volatile`, there is no happens-before guarantee and the write may not be visible. ```suggestion private volatile boolean needsMetadata; ``` ########## impl/maven-logging/src/main/java/org/apache/maven/logging/impl/DefaultLogEventBuilder.java: ########## @@ -0,0 +1,114 @@ +/* + * 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; + + 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 + public LogEventBuilder withProjectId(String projectId) { + return this; Review Comment: đĄ **`withProjectId()` is a silent no-op â contradicts API Javadoc.** The `LogEventBuilder` Javadoc says: *"Overrides the project identifier for this event. Use this method only in the rare cases where the ThreadLocal context is incorrect (e.g. async hand-offs)."* The implementation silently discards the parameter and returns `this`. Callers relying on this for async hand-offs will see wrong project context attached to events. Either implement the override (store the value and pass it to `DefaultLogger.doLog()`), or update the Javadoc to clarify it's a future/deferred feature. ########## 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 Log#atTrace()}, {@link Log#atDebug()}, etc. + * A builder <em>must</em> be consumed in a single chained call; it must not be + * stored in a variable across statements, as implementations may reuse + * thread-local state for zero-allocation dispatch. + * <p> + * When the target level is disabled the {@code Log} returns a shared + * {@code NopLogBuilder} singleton, making the entire fluent call chain a no-op Review Comment: đĄ **Stale Javadoc references.** - Line 27: `{@link Log#atTrace()}` â the type in this module is `Logger`, not `Log`. - Line 33: `NopLogBuilder` â the actual class is `NopLogEventBuilder` (see `DefaultLogger.java`). ########## impl/maven-logging/src/main/java/org/apache/maven/logging/impl/DefaultLoggerManager.java: ########## @@ -0,0 +1,299 @@ +/* + * 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.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<>(LogLevel.DEBUG); + private final Properties properties = new Properties(); + private final ConcurrentHashMap<String, Integer> levelCache = new ConcurrentHashMap<>(); + + public DefaultLoggerManager() { + this.logSink = new AccumulatingLogSink(); + LoggerManager.set(this); + 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; + this.logSink = sink; + if (old instanceof AccumulatingLogSink acc && sink != null) { + acc.drainTo(sink); Review Comment: đ´ **Race condition: new sink becomes visible before old events are drained.** At line 91 `this.logSink = sink` is set, making the new sink immediately visible to concurrent `doLog()` callers. At line 93, `drainTo()` then replays buffered events to the same sink. A concurrent log call between (91) and (93) writes directly to the new sink, arriving *before* buffered events that logically preceded it â breaking ordering. Additionally, `CopyOnWriteArrayList.clear()` in `drainTo()` can silently discard events added concurrently between the iteration and `clear()`. The swap should happen *after* drain completes: ```java if (old instanceof AccumulatingLogSink acc && sink != null) { acc.drainTo(sink); } this.logSink = sink; ``` ########## impl/maven-logging/src/main/java/org/apache/maven/logging/impl/DefaultLogger.java: ########## @@ -0,0 +1,417 @@ +/* + * 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.PrintWriter; +import java.io.StringWriter; +import java.lang.StackWalker.StackFrame; +import java.time.Instant; +import java.util.Map; +import java.util.concurrent.atomic.AtomicLong; +import java.util.function.Supplier; + +import org.apache.maven.api.MonotonicClock; +import org.apache.maven.api.logging.LogEvent; +import org.apache.maven.api.logging.LogEventBuilder; +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; + +import static java.util.Objects.requireNonNull; + +/** + * Default implementation of both {@link Logger} (new fluent API) + * and the deprecated {@link org.apache.maven.api.plugin.Log} (backward compatibility). + * <p> + * Builds fully-structured {@link LogEvent} objects â including {@code projectId}, + * {@code mojoId}, timestamp, and sequence number â and dispatches them to the + * current {@link LogSink} obtained from {@link LoggerManager#get()}. + * Level checks and sink lookup both go through the {@code LoggerManager}, + * so the sink can be changed at runtime. + * + * @since 4.0.0 + */ +@SuppressWarnings("deprecation") +public class DefaultLogger implements Logger, org.apache.maven.api.plugin.Log { + + private static final StackWalker WALKER = StackWalker.getInstance(); + private static final String THIS_CLASS = DefaultLogger.class.getName(); + private static final String BUILDER_CLASS = DefaultLogEventBuilder.class.getName(); + + private static final AtomicLong SEQUENCE = new AtomicLong(); + + private final String name; + private final String projectId; + private final String mojoId; + + public DefaultLogger(String name) { + this(name, null, null); + } + + public DefaultLogger(String name, String projectId, String mojoId) { + this.name = requireNonNull(name, "name"); + this.projectId = projectId; + this.mojoId = mojoId; + } + + // ---- org.apache.maven.api.logging.Logger ---- + + @Override + public String getName() { + return name; + } + + private boolean isLevelEnabled(LogLevel level) { + LoggerManager mgr = LoggerManager.get(); + return mgr != null && mgr.isLevelEnabled(name, level); + } + + @Override + public boolean isEnabled(LogLevel level) { + return isLevelEnabled(level); + } + + @Override + public LogEventBuilder atTrace() { + return isLevelEnabled(LogLevel.TRACE) + ? new DefaultLogEventBuilder(this, LogLevel.TRACE) + : NopLogEventBuilder.INSTANCE; + } + + @Override + public LogEventBuilder atDebug() { + return isLevelEnabled(LogLevel.DEBUG) + ? new DefaultLogEventBuilder(this, LogLevel.DEBUG) + : NopLogEventBuilder.INSTANCE; + } + + @Override + public LogEventBuilder atInfo() { + return isLevelEnabled(LogLevel.INFO) + ? new DefaultLogEventBuilder(this, LogLevel.INFO) + : NopLogEventBuilder.INSTANCE; + } + + @Override + public LogEventBuilder atWarn() { + return isLevelEnabled(LogLevel.WARN) + ? new DefaultLogEventBuilder(this, LogLevel.WARN) + : NopLogEventBuilder.INSTANCE; + } + + @Override + public LogEventBuilder atError() { + return isLevelEnabled(LogLevel.ERROR) + ? new DefaultLogEventBuilder(this, LogLevel.ERROR) + : NopLogEventBuilder.INSTANCE; + } + + @Override + public LogEventBuilder at(LogLevel level) { + return isLevelEnabled(level) ? new DefaultLogEventBuilder(this, level) : NopLogEventBuilder.INSTANCE; + } + + @Override + public DefaultLogger child(String childName) { + requireNonNull(childName, "childName"); + return new DefaultLogger(name + "." + childName, projectId, mojoId); + } + + // ---- Internal dispatch ---- + + void doLog(LogLevel level, String message, Throwable throwable) { + doLog(level, message, null, null, null, throwable); + } + + @SuppressWarnings("deprecation") // Thread.getId() + void doLog( + LogLevel level, + String message, + String messageTemplate, + Object[] messageArgs, + Map<String, String> keyValues, + Throwable throwable) { + LoggerManager mgr = LoggerManager.get(); + if (mgr == null) { + return; + } + LogSink sink = mgr.getLogSink(); + if (sink == null) { + return; + } + + Instant timestamp = MonotonicClock.now(); + long seq = SEQUENCE.getAndIncrement(); + + String sourceClass = null; + String sourceMethod = null; + long threadId = -1; + String threadName = null; + + if (sink.needsMetadata()) { + sourceClass = name; + sourceMethod = resolveCallerMethod(); + threadId = Thread.currentThread().getId(); + threadName = Thread.currentThread().getName(); + } + + String stackTrace = throwable != null ? formatStackTrace(throwable) : null; + + LogEvent event = new DefaultLogEvent( + timestamp, + level, + message, + name, + stackTrace, + throwable, + sourceClass, + sourceMethod, + threadId, + seq, + threadName, + projectId, + mojoId, + keyValues, + messageTemplate, + messageArgs); + + sink.accept(event); Review Comment: đ´ **Bug: `doLog()` never calls `record()` â `--fail-on-severity` is broken for the new `Logger` API.** The old `MavenFailOnSeverityLogger` (deleted in this PR) called `logLevelRecorder.record()` from each SLF4J logging method. The new path dispatches via `sink.accept(event)` but never calls `DefaultLoggerManager.record(level)`. Any plugin that migrates from `org.apache.maven.api.plugin.Log` or SLF4J to the new fluent `Logger` API will stop feeding the severity recorder. `--fail-on-severity WARN` silently stops working for those plugins. Add `if (mgr instanceof DefaultLoggerManager dlm) dlm.record(level);` (or a `LoggerManager.record()` API method) after `sink.accept(event)`. ########## 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; Review Comment: đĄ **Mutable internal array returned from `@Immutable` type.** `LogEvent` is annotated `@Immutable`. Returning the raw `messageArgs` array allows callers to mutate it between dispatch and lazy formatting in `message()`, producing corrupted log output. ```suggestion return messageArgs != null ? messageArgs.clone() : null; ``` ########## 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 Log} API, SLF4J Review Comment: đĄ **Stale Javadoc reference: `{@link Log}` should be `{@link Logger}`.** ########## 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 (Log, LogBuilder, LogLevel, LogEvent, LogSink).</description> Review Comment: đ **Stale `<description>`: lists `Log` and `LogBuilder` â actual types are `Logger` and `LogEventBuilder`.** ```suggestion <description>Maven 4 API - Unified Logging API (Logger, LogEventBuilder, LogLevel, LogEvent, LogSink).</description> ``` ########## impl/maven-core/src/main/java/org/apache/maven/execution/MavenExecutionRequest.java: ########## @@ -319,6 +319,11 @@ public interface MavenExecutionRequest { boolean isInteractiveMode(); + // Transfer progress + MavenExecutionRequest setNoTransferProgress(boolean noTransferProgress); + + boolean isNoTransferProgress(); Review Comment: âšī¸ **Unrelated API addition bundled in this logging PR.** `setNoTransferProgress()`/`isNoTransferProgress()` are new methods on `MavenExecutionRequest` â a core, stable API interface. While used by `LookupInvoker` as part of this refactor, these represent an independent interface change. Consider splitting to a separate PR for clean review history and to make the API evolution explicit. -- 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]
