gnodet-bot commented on code in PR #13695: URL: https://github.com/apache/maven/pull/13695#discussion_r4365530540
########## 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: 🔴 **[Not addressed] Bug: `doLog()` never calls `record()` — `--fail-on-severity` broken for new `Logger` API.** `sink.accept(event)` dispatches the event but never calls `DefaultLoggerManager.record(level)`. The old `MavenFailOnSeverityLogger` (deleted in this PR) called `logLevelRecorder.record()` from the SLF4J dispatch path. The new `MavenFailOnSeverityLogger` in `maven-logging-slf4j` still calls `dlm.record(level)` but only covers the SLF4J path. Any plugin using the new fluent `Logger` API will silently bypass `--fail-on-severity`. ```suggestion sink.accept(event); if (mgr instanceof DefaultLoggerManager dlm) { dlm.record(level); } ``` ########## 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: 🔴 **[Not addressed] Race condition: new sink becomes visible before old events are drained.** `this.logSink = sink` at L91 makes the new sink immediately visible to concurrent `doLog()` callers. `drainTo()` at L93 then replays buffered events. Between L91 and L93, concurrent loggers write directly to the real sink while the drain is still replaying older events — breaking ordering guarantees. Additionally, `CopyOnWriteArrayList.clear()` in `drainTo()` can discard events added concurrently between the iteration completing and `clear()` executing. Fix: drain first, then swap: ```suggestion 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/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"; + case INFO -> "info"; + case WARN -> "warn"; + case ERROR -> "error"; + }; + System.setProperty(Constants.MAVEN_LOGGER_DEFAULT_LOG_LEVEL, value); Review Comment: 🔴 **New: `setRootLoggerLevel()` does not call `reconfigure()` — `levelCache` stays stale.** `setRootLoggerLevel()` writes the new level to a system property (L132) but does not clear `levelCache`. All loggers that already resolved their level continue at the stale cached value until the next `reconfigure()` call (or JVM restart). In practice, `-X` and `-q` are applied at startup before any logger levels are cached, so this may not manifest immediately — but it will break any runtime level changes. The Javadoc on the interface says "Takes effect after `reconfigure()`" which acknowledges the issue, but that contract puts the burden on callers who can't know the internals. `setRootLoggerLevel()` should be self-contained: ```suggestion @Override public void setRootLoggerLevel(LogLevel level) { String value = switch (level) { case TRACE -> "trace"; case DEBUG -> "debug"; case INFO -> "info"; case WARN -> "warn"; case ERROR -> "error"; }; System.setProperty(Constants.MAVEN_LOGGER_DEFAULT_LOG_LEVEL, value); reconfigure(); } ``` ########## 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: 🔴 **[Not addressed] `setRootLoggerLevel(TRACE)` silently maps to `"debug"` — TRACE logging unreachable.** `case TRACE, DEBUG -> "debug"` means `setRootLoggerLevel(LogLevel.TRACE)` writes `"debug"` to the system property. `stringToLevel("debug")` returns `LOG_LEVEL_DEBUG` (10), but `toIntLevel(LogLevel.TRACE)` returns `LOG_LEVEL_TRACE` (0). The check `isLevelEnabled(TRACE)` evaluates `0 >= 10 = false`. TRACE logging is therefore never reachable via `setRootLoggerLevel()`. ```suggestion case TRACE -> "trace"; case DEBUG -> "debug"; ``` ########## impl/maven-core/src/main/java/org/apache/maven/logging/ProjectBuildLogAppender.java: ########## @@ -109,29 +117,118 @@ 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: 🟠 **[Not addressed] `needsMetadata` field is not `volatile` but is read from multiple threads.** `setNeedsMetadata()` (L143) writes this field; the inner `Sink.needsMetadata()` (L182) reads it from arbitrary logging threads. Without `volatile` (or another happens-before guarantee), the write may not be visible to the reader thread. This is the same concern raised in the prior review. ```suggestion private volatile boolean needsMetadata; ``` ########## 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: 🟠 **[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> ``` ########## 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: 🟡 **[Not addressed] `withProjectId()` is a silent no-op — contradicts API Javadoc.** The `LogEventBuilder` Javadoc says: *"Overrides the project identifier for this event."* The implementation discards the value. Callers using the fluent builder for async context hand-offs (the stated use case) will see wrong context on events. ```suggestion @Override public LogEventBuilder withProjectId(String projectId) { this.projectId = projectId; return this; } ``` Add `private String projectId;` / `private String mojoId;` fields, and pass them to `doLog()`. If the intent is that context is always resolved from the `DefaultLogger` fields, document that explicitly instead. ########## 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: 🟡 **[Not addressed] Stale Javadoc references.** - L27: `{@link Log#atTrace()}` — the type in this module is `Logger`, not `Log`. Should be `{@link Logger#atTrace()}`. - L32: `{@code Log}` — same, should be `{@code Logger}`. - L33: `NopLogBuilder` — the actual class is `NopLogEventBuilder`. ########## 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: 🟡 **[Not addressed] Stale Javadoc: `{@link Log}` should be `{@link Logger}`.** ```suggestion * All logging paths — the fluent {@link Logger} API, SLF4J ``` ########## 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: 🟠 **[Not addressed] `maxReached` initialized to `LogLevel.DEBUG` — `hasReached(DEBUG)` is `true` before anything is logged.** `LogLevel.DEBUG.ordinal()` is 1; `hasReached(LogLevel.TRACE)` returns `true` (1 ≥ 0) and `hasReached(LogLevel.DEBUG)` returns `true` (1 ≥ 1) immediately at construction, before any log event has been dispatched. `reconfigure()` also resets to `LogLevel.DEBUG`. The initial state should represent "nothing reached yet": ```suggestion private final AtomicReference<LogLevel> maxReached = new AtomicReference<>(null); ``` And update `hasReached()`: ```java public boolean hasReached(LogLevel level) { LogLevel max = maxReached.get(); return max != null && max.ordinal() >= level.ordinal(); } ``` And `reconfigure()`: `maxReached.set(null);` ########## 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: 🟡 **[Not addressed] Mutable internal array returned from `@Immutable` type.** `LogEvent` is annotated `@Immutable`. `messageArgs()` returns the raw `Object[]` array. A sink could mutate the array between dispatch and the point lazy formatting in `message()` runs, corrupting the formatted output. ```suggestion return messageArgs != null ? messageArgs.clone() : null; ``` ########## 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: 🟡 **[Not addressed] `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: 🟡 **[Not addressed] Naming convention: `@Provider` types use noun-style accessors.** `Logger` is annotated `@Provider`. Maven 4 API convention for provider/immutable types uses `name()`, not `getName()`. Every new JDK immutable type since Java 8 uses noun-style; `getX()` is the JavaBeans mutable-bean prefix. Other methods on this interface (`isEnabled`, `atTrace`, `child`) follow the convention — `getName()` is the outlier. ```suggestion String name(); ``` -- 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]
