diff --git a/java-checks-test-sources/default/src/main/java/checks/MoreThanOneLoggerCheckSample.java b/java-checks-test-sources/default/src/main/java/checks/MoreThanOneLoggerCheckSample.java new file mode 100644 index 00000000000..5bb356f9199 --- /dev/null +++ b/java-checks-test-sources/default/src/main/java/checks/MoreThanOneLoggerCheckSample.java @@ -0,0 +1,245 @@ +package checks; + +import org.apache.commons.logging.Log; +import org.apache.commons.logging.LogFactory; +import org.apache.logging.log4j.LogManager; +import org.slf4j.Logger; +import org.slf4j.LoggerFactory; +import org.slf4j.Marker; +import org.slf4j.MarkerFactory; + +class MoreThanOneLoggerCheckSample { + + private static final Logger LOG = LoggerFactory.getLogger(MoreThanOneLoggerCheckSample.class); +// ^^^> {{Logger already declared here.}} + private static final Logger LOGGER = LoggerFactory.getLogger(MoreThanOneLoggerCheckSample.class); // Noncompliant {{Remove this logger and use the one already declared in this class.}} +// ^^^^^^ + + void log() { + LOG.info("one"); + LOGGER.info("two"); + } +} + +class SingleLogger { + + private static final Logger LOG = LoggerFactory.getLogger(SingleLogger.class); // Compliant + + void log() { + LOG.info("one"); + } +} + +class NoLogger { + + private static final String NAME = "no logger here"; // Compliant + + private final Object delegate = new Object(); // Compliant +} + +class ThreeLoggers { + + private static final Logger FIRST = LoggerFactory.getLogger(ThreeLoggers.class); +// ^^^^^> {{Logger already declared here.}} + private static final Logger SECOND = LoggerFactory.getLogger(ThreeLoggers.class); // Noncompliant {{Remove this logger and use the one already declared in this class.}} +// ^^^^^^ + private static final Logger THIRD = LoggerFactory.getLogger(ThreeLoggers.class); // Noncompliant {{Remove this logger and use the one already declared in this class.}} +// ^^^^^ +// ^^^^^@-5< {{Logger already declared here.}} +} + +class MixedFacades { + + private static final Logger SLF4J = LoggerFactory.getLogger(MixedFacades.class); +// ^^^^^> {{Logger already declared here.}} + private static final org.apache.logging.log4j.Logger LOG4J2 = LogManager.getLogger(MixedFacades.class); // Noncompliant {{Remove this logger and use the one already declared in this class.}} +// ^^^^^^ +} + +class JulLoggers { + + private static final java.util.logging.Logger LOG = java.util.logging.Logger.getLogger("first"); +// ^^^> {{Logger already declared here.}} + private static final java.util.logging.Logger OTHER = java.util.logging.Logger.getLogger("first"); // Noncompliant {{Remove this logger and use the one already declared in this class.}} +// ^^^^^ +} + +class CommonsLoggingLoggers { + + private static final Log LOG = LogFactory.getLog(CommonsLoggingLoggers.class); +// ^^^> {{Logger already declared here.}} + private static final Log OTHER = LogFactory.getLog(CommonsLoggingLoggers.class); // Noncompliant {{Remove this logger and use the one already declared in this class.}} +// ^^^^^ +} + +class NonStaticNextToStatic { + + private static final Logger LOG = LoggerFactory.getLogger(NonStaticNextToStatic.class); +// ^^^> {{Logger already declared here.}} + private final Logger instanceLogger = LoggerFactory.getLogger(NonStaticNextToStatic.class); // Noncompliant {{Remove this logger and use the one already declared in this class.}} +// ^^^^^^^^^^^^^^ +} + +class LoggerWithMarker { + + private static final Logger LOG = LoggerFactory.getLogger(LoggerWithMarker.class); // Compliant + private static final Marker AUDIT = MarkerFactory.getMarker("AUDIT"); // Compliant, a marker is not a logger + + void log() { + LOG.info(AUDIT, "audited"); + } +} + +class OuterWithNestedClasses { + + private static final Logger LOG = LoggerFactory.getLogger(OuterWithNestedClasses.class); // Compliant + + static class Nested { + private static final Logger NESTED_LOG = LoggerFactory.getLogger(Nested.class); // Compliant, a nested class is a class of its own + } + + class Inner { + private final Logger innerLog = LoggerFactory.getLogger(Inner.class); // Compliant, an inner class is a class of its own + } +} + +class NestedClassWithTwoLoggers { + + private static final Logger LOG = LoggerFactory.getLogger(NestedClassWithTwoLoggers.class); // Compliant + + static class Nested { + private static final Logger FIRST = LoggerFactory.getLogger(Nested.class); +// ^^^^^> {{Logger already declared here.}} + private static final Logger SECOND = LoggerFactory.getLogger(Nested.class); // Noncompliant {{Remove this logger and use the one already declared in this class.}} +// ^^^^^^ + } +} + +class AnonymousClassLogger { + + private static final Logger LOG = LoggerFactory.getLogger(AnonymousClassLogger.class); // Compliant + + Runnable task() { + return new Runnable() { + private final Logger anonymousLog = LoggerFactory.getLogger(Runnable.class); // Compliant, the anonymous class body is its own class + + @Override + public void run() { + anonymousLog.info("run"); + } + }; + } +} + +class LocalLoggerVariables { + + private static final Logger LOG = LoggerFactory.getLogger(LocalLoggerVariables.class); // Compliant + + void log() { + Logger local = LoggerFactory.getLogger(Object.class); // Compliant, a local variable is not a field + Logger otherLocal = LoggerFactory.getLogger(String.class); // Compliant + local.info("one"); + otherLocal.info("two"); + LOG.info("three"); + } +} + +enum EnumWithTwoLoggers { + INSTANCE; + + private static final Logger LOG = LoggerFactory.getLogger(EnumWithTwoLoggers.class); +// ^^^> {{Logger already declared here.}} + private static final Logger OTHER = LoggerFactory.getLogger(EnumWithTwoLoggers.class); // Noncompliant {{Remove this logger and use the one already declared in this class.}} +// ^^^^^ +} + +record RecordWithTwoLoggers(String name) { + + private static final Logger LOG = LoggerFactory.getLogger(RecordWithTwoLoggers.class); +// ^^^> {{Logger already declared here.}} + private static final Logger OTHER = LoggerFactory.getLogger(RecordWithTwoLoggers.class); // Noncompliant {{Remove this logger and use the one already declared in this class.}} +// ^^^^^ +} + +interface InterfaceWithTwoLoggers { + + Logger LOG = LoggerFactory.getLogger(InterfaceWithTwoLoggers.class); +// ^^^> {{Logger already declared here.}} + Logger OTHER = LoggerFactory.getLogger(InterfaceWithTwoLoggers.class); // Noncompliant {{Remove this logger and use the one already declared in this class.}} +// ^^^^^ +} + +class ConfigurableLoggerProperty { + + private static final Logger LOG = LoggerFactory.getLogger(ConfigurableLoggerProperty.class); // Compliant + private Logger contextLogger; // Compliant, not initialized here, it is a configurable property + + Logger getContextLogger() { + return contextLogger; + } + + void setContextLogger(Logger logger) { + contextLogger = logger; + } + + void start() { + if (contextLogger == null) { + contextLogger = LoggerFactory.getLogger(ConfigurableLoggerProperty.class.getName() + ".context"); + } + LOG.info("started"); + } +} + +class DerivedChildLogger { + + private static final Logger LOG = LoggerFactory.getLogger(DerivedChildLogger.class); // Compliant + private static final Logger LOG_CONFIG = LoggerFactory.getLogger(LOG.getName() + ".config"); // Compliant, a child category inherits the parent configuration + + void log() { + LOG.info("one"); + LOG_CONFIG.debug("configuration detail"); + } +} + +class AliasedLogger { + + private static final Logger LOG = LoggerFactory.getLogger(AliasedLogger.class); +// ^^^> {{Logger already declared here.}} + private static final Logger OTHER = LOG; // Noncompliant {{Remove this logger and use the one already declared in this class.}} +// ^^^^^ +} + +class SameCategoryLogger { + + private static final Logger LOG = LoggerFactory.getLogger(SameCategoryLogger.class); +// ^^^> {{Logger already declared here.}} + private static final Logger OTHER = LoggerFactory.getLogger(LOG.getName()); // Noncompliant {{Remove this logger and use the one already declared in this class.}} +// ^^^^^ +} + +class FinalLoggersAssignedInStaticBlock { + + private static final Logger FIRST; +// ^^^^^> {{Logger already declared here.}} + private static final Logger SECOND; // Noncompliant {{Remove this logger and use the one already declared in this class.}} +// ^^^^^^ + + static { + FIRST = LoggerFactory.getLogger(FinalLoggersAssignedInStaticBlock.class); + SECOND = LoggerFactory.getLogger("other"); + } +} + +@interface AnnotationTypeWithTwoLoggers { + + Logger LOG = LoggerFactory.getLogger(AnnotationTypeWithTwoLoggers.class); +// ^^^> {{Logger already declared here.}} + Logger OTHER = LoggerFactory.getLogger(AnnotationTypeWithTwoLoggers.class); // Noncompliant {{Remove this logger and use the one already declared in this class.}} +// ^^^^^ +} + +class MultipleLoggerFieldsInOneDeclaration { + + private static final Logger FIRST = LoggerFactory.getLogger(MultipleLoggerFieldsInOneDeclaration.class), SECOND = LoggerFactory.getLogger(MultipleLoggerFieldsInOneDeclaration.class); // Noncompliant {{Remove this logger and use the one already declared in this class.}} +// ^^^^^> {{Logger already declared here.}} ^^^^^^ +} diff --git a/java-checks/src/main/java/org/sonar/java/checks/MoreThanOneLoggerCheck.java b/java-checks/src/main/java/org/sonar/java/checks/MoreThanOneLoggerCheck.java new file mode 100644 index 00000000000..37db248b05e --- /dev/null +++ b/java-checks/src/main/java/org/sonar/java/checks/MoreThanOneLoggerCheck.java @@ -0,0 +1,150 @@ +/* + * SonarQube Java + * Copyright (C) SonarSource Sàrl + * mailto:info AT sonarsource DOT com + * + * You can redistribute and/or modify this program under the terms of + * the Sonar Source-Available License Version 1, as published by SonarSource Sàrl. + * + * This program is distributed in the hope that it will be useful, + * but WITHOUT ANY WARRANTY; without even the implied warranty of + * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE. + * See the Sonar Source-Available License for more details. + * + * You should have received a copy of the Sonar Source-Available License + * along with this program; if not, see https://sonarsource.com/license/ssal/ + */ +package org.sonar.java.checks; + +import java.util.Collections; +import java.util.List; +import java.util.Set; +import java.util.stream.Collectors; +import org.sonar.check.Rule; +import org.sonar.plugins.java.api.IssuableSubscriptionVisitor; +import org.sonar.plugins.java.api.JavaFileScannerContext; +import org.sonar.plugins.java.api.semantic.Symbol; +import org.sonar.plugins.java.api.semantic.Type; +import org.sonar.plugins.java.api.tree.BaseTreeVisitor; +import org.sonar.plugins.java.api.tree.BinaryExpressionTree; +import org.sonar.plugins.java.api.tree.ClassTree; +import org.sonar.plugins.java.api.tree.ExpressionTree; +import org.sonar.plugins.java.api.tree.IdentifierTree; +import org.sonar.plugins.java.api.tree.MemberSelectExpressionTree; +import org.sonar.plugins.java.api.tree.MethodInvocationTree; +import org.sonar.plugins.java.api.tree.Tree; +import org.sonar.plugins.java.api.tree.VariableTree; + +@Rule(key = "S9436") +public class MoreThanOneLoggerCheck extends IssuableSubscriptionVisitor { + + private static final String MESSAGE = "Remove this logger and use the one already declared in this class."; + private static final String SECONDARY_MESSAGE = "Logger already declared here."; + + private static final Set LOGGER_TYPES = Set.of( + "org.slf4j.Logger", + "org.apache.logging.log4j.Logger", + "org.apache.log4j.Logger", + "java.util.logging.Logger", + "org.apache.commons.logging.Log"); + + @Override + public List nodesToVisit() { + return Tree.Kind.CLASS_KINDS; + } + + @Override + public void visitNode(Tree tree) { + if (context.getSemanticModel() == null) { + return; + } + List loggerFields = loggerFields((ClassTree) tree); + if (loggerFields.size() < 2) { + return; + } + List secondary = + Collections.singletonList(new JavaFileScannerContext.Location(SECONDARY_MESSAGE, loggerFields.get(0).simpleName())); + loggerFields.subList(1, loggerFields.size()) + .forEach(extraLogger -> reportIssue(extraLogger.simpleName(), MESSAGE, secondary, null)); + } + + private static List loggerFields(ClassTree classTree) { + List loggerFields = classTree.members().stream() + .filter(member -> member.is(Tree.Kind.VARIABLE)) + .map(VariableTree.class::cast) + .filter(MoreThanOneLoggerCheck::isDeclaredLogger) + .toList(); + return withoutDerivedLoggers(loggerFields); + } + + /** + * A non-final logger field without an initializer is a configurable property, assigned at startup + * or injected from outside, rather than a category the class gives itself. A final one has to be + * assigned in an initializer or a constructor, so it is a category of the class. + */ + private static boolean isDeclaredLogger(VariableTree field) { + return (field.initializer() != null || field.symbol().isFinal()) && isLoggerType(field.type().symbolType()); + } + + /** + * Drops the loggers whose name is derived from another logger of the same class, such as + * {@code LoggerFactory.getLogger(LOG.getName() + ".config")}. Those create a child category, + * which inherits the level and the appenders configured for the parent. An initializer that + * merely reuses another logger, as in {@code LoggerFactory.getLogger(LOG.getName())}, is not a + * child category and is still reported. + */ + private static List withoutDerivedLoggers(List loggerFields) { + Set loggerSymbols = loggerFields.stream() + .map(VariableTree::symbol) + .collect(Collectors.toSet()); + return loggerFields.stream() + .filter(field -> !buildsChildCategory(field, loggerSymbols)) + .toList(); + } + + private static boolean buildsChildCategory(VariableTree field, Set loggerSymbols) { + if (field.initializer() == null) { + return false; + } + ChildCategoryVisitor visitor = new ChildCategoryVisitor(loggerSymbols); + field.initializer().accept(visitor); + return visitor.derivesLoggerName && visitor.concatenates; + } + + private static boolean isLoggerType(Type type) { + return LOGGER_TYPES.stream().anyMatch(type::is); + } + + private static class ChildCategoryVisitor extends BaseTreeVisitor { + + private final Set loggerSymbols; + private boolean derivesLoggerName = false; + private boolean concatenates = false; + + private ChildCategoryVisitor(Set loggerSymbols) { + this.loggerSymbols = loggerSymbols; + } + + @Override + public void visitBinaryExpression(BinaryExpressionTree tree) { + if (tree.is(Tree.Kind.PLUS)) { + concatenates = true; + } + super.visitBinaryExpression(tree); + } + + @Override + public void visitMethodInvocation(MethodInvocationTree tree) { + if (tree.methodSelect() instanceof MemberSelectExpressionTree select + && "getName".equals(select.identifier().name()) + && isLoggerField(select.expression())) { + derivesLoggerName = true; + } + super.visitMethodInvocation(tree); + } + + private boolean isLoggerField(ExpressionTree expression) { + return expression instanceof IdentifierTree identifier && loggerSymbols.contains(identifier.symbol()); + } + } +} diff --git a/java-checks/src/test/java/org/sonar/java/checks/MoreThanOneLoggerCheckTest.java b/java-checks/src/test/java/org/sonar/java/checks/MoreThanOneLoggerCheckTest.java new file mode 100644 index 00000000000..8b76cc4256b --- /dev/null +++ b/java-checks/src/test/java/org/sonar/java/checks/MoreThanOneLoggerCheckTest.java @@ -0,0 +1,42 @@ +/* + * SonarQube Java + * Copyright (C) SonarSource Sàrl + * mailto:info AT sonarsource DOT com + * + * You can redistribute and/or modify this program under the terms of + * the Sonar Source-Available License Version 1, as published by SonarSource Sàrl. + * + * This program is distributed in the hope that it will be useful, + * but WITHOUT ANY WARRANTY; without even the implied warranty of + * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE. + * See the Sonar Source-Available License for more details. + * + * You should have received a copy of the Sonar Source-Available License + * along with this program; if not, see https://sonarsource.com/license/ssal/ + */ +package org.sonar.java.checks; + +import org.junit.jupiter.api.Test; +import org.sonar.java.checks.verifier.CheckVerifier; + +import static org.sonar.java.checks.verifier.TestUtils.mainCodeSourcesPath; + +class MoreThanOneLoggerCheckTest { + + @Test + void test() { + CheckVerifier.newVerifier() + .onFile(mainCodeSourcesPath("checks/MoreThanOneLoggerCheckSample.java")) + .withCheck(new MoreThanOneLoggerCheck()) + .verifyIssues(); + } + + @Test + void test_without_semantic() { + CheckVerifier.newVerifier() + .onFile(mainCodeSourcesPath("checks/MoreThanOneLoggerCheckSample.java")) + .withCheck(new MoreThanOneLoggerCheck()) + .withoutSemantic() + .verifyNoIssues(); + } +} diff --git a/sonar-java-plugin/src/main/resources/org/sonar/l10n/java/rules/java/S9436.html b/sonar-java-plugin/src/main/resources/org/sonar/l10n/java/rules/java/S9436.html new file mode 100644 index 00000000000..933283bf328 --- /dev/null +++ b/sonar-java-plugin/src/main/resources/org/sonar/l10n/java/rules/java/S9436.html @@ -0,0 +1,84 @@ +

A class that declares more than one logger splits its own log output across several loggers, so the logging configuration that applies to the class +governs only part of what it logs.

+

Why is this an issue?

+

A class is expected to log through a single logger. That logger defines one category for the whole class, and the logging configuration attaches a +level and appenders to it, so the output of the class is filtered and routed as one unit.

+

Declaring a second logger field breaks that. The reader now has to work out which logger each call site uses before being able to tell where a +message goes, and the two loggers do not necessarily resolve to the same category. When they do not, the level configured for the class governs only +part of its output: raising it to DEBUG during an incident produces a partial trace, with no indication that anything is missing. When +they do resolve to the same category, the second field is pure duplication and the class ends up with two names for one thing.

+

More than one logger in a class is almost always accidental: a field left behind by a refactoring that split or merged classes, a declaration +copied along with the code that used it, or two logging facades that ended up in the same file.

+

This rule raises an issue when a class declares more than one logger field, for SLF4J, Log4j, Log4j 2, Commons Logging and +java.util.logging.

+

Exceptions

+

A nested or inner class is a class of its own, so a logger it declares is not counted against the logger of the enclosing class. Only the fields +declared directly in the same class body are counted together.

+

A non-static logger field declared next to a static one still raises an issue, because the class still ends up with two loggers, regardless of the +modifiers. Rule {rule:java:S1312} covers the modifiers a logger field should have.

+

A non-final logger field that is not initialized where it is declared does not count. Such a field is a configurable property, assigned at startup +or injected from outside, often exposed through a getter and a setter. It is not a second category that the class gives itself, and it usually cannot +be removed without changing the API of the class. A final field is different: it can only be assigned in an initializer or a constructor +of the class, so it does count.

+

A logger whose name is derived from another logger of the same class, as in LoggerFactory.getLogger(LOG.getName() + ".config"), does +not count either. That call creates a child category, which inherits the level and the appenders configured for the parent, so the output of the class +is still governed as one unit and the problem described above does not arise. Reusing the name as it is, as in +LoggerFactory.getLogger(LOG.getName()), builds the same category rather than a child one and is still reported.

+

How to fix it

+

Keep one logger and remove the other field, then point the calls that used the removed logger at the remaining one.

+

The second logger is sometimes deliberate: a separate stream is wanted for audit or metrics messages, so that they can be routed to their own file +and kept at their own level. In that case, do not simply merge the calls, as that would lose the distinction. Tag the messages with an SLF4J +Marker instead: create the marker once with MarkerFactory.getMarker("AUDIT"), pass it as the first argument of the logging +call, and route it in the logging configuration with a marker filter. A marker keeps the distinction and the routing without giving the class a second +logger.

+

With Log4j 2, MarkerManager.getMarker plays the same role. With java.util.logging, which has no marker concept, keep the +single logger and either use a dedicated level or a Filter on the handler.

+

Code examples

+

Noncompliant code example

+
+public class PaymentService {
+
+  private static final Logger LOG = LoggerFactory.getLogger(PaymentService.class);
+  private static final Logger LOGGER = LoggerFactory.getLogger(PaymentService.class); // Noncompliant
+
+  public void capture(Payment payment) {
+    LOG.debug("Capturing payment {}", payment.id());
+    gateway.capture(payment);
+    LOGGER.info("Payment {} captured for {}", payment.id(), payment.amount());
+  }
+}
+
+

Compliant solution

+
+public class PaymentService {
+
+  private static final Logger LOG = LoggerFactory.getLogger(PaymentService.class);
+
+  public void capture(Payment payment) {
+    LOG.debug("Capturing payment {}", payment.id());
+    gateway.capture(payment);
+    LOG.info("Payment {} captured for {}", payment.id(), payment.amount());
+  }
+}
+
+

How does this work?

+

The class now has a single logger, so everything it logs is filtered and routed by the configuration that targets its category, and a call site no +longer says anything about where its message goes beyond the level it uses.

+

The refactoring preserves behavior as long as every call moved to the remaining logger keeps its level and its arguments. When the removed logger +used a different category, check the logging configuration before deleting the field, otherwise messages that used to be written to a dedicated +appender silently move to the main log. If that separation has to be kept, use a marker as described above rather than a second logger.

+

Resources

+

Documentation

+ +

Related rules

+
    +
  • {rule:java:S1312} - Loggers should be "private static final" and should share a naming convention
    • +
  • {rule:java:S3416} - Loggers should be named for their enclosing classes
    • +
+

Those two rules constrain the form and the name of a logger field. This rule is about their number: a class should declare only one.

+ diff --git a/sonar-java-plugin/src/main/resources/org/sonar/l10n/java/rules/java/S9436.json b/sonar-java-plugin/src/main/resources/org/sonar/l10n/java/rules/java/S9436.json new file mode 100644 index 00000000000..24d7e43c067 --- /dev/null +++ b/sonar-java-plugin/src/main/resources/org/sonar/l10n/java/rules/java/S9436.json @@ -0,0 +1,25 @@ +{ + "title": "Classes should not declare more than one logger", + "type": "BUG", + "status": "ready", + "remediation": { + "func": "Constant\/Issue", + "constantCost": "10min" + }, + "tags": [ + "logging", + "confusing" + ], + "defaultSeverity": "Major", + "ruleSpecification": "RSPEC-9436", + "sqKey": "S9436", + "scope": "All", + "quickfix": "infeasible", + "code": { + "impacts": { + "RELIABILITY": "MEDIUM", + "MAINTAINABILITY": "LOW" + }, + "attribute": "LOGICAL" + } +} diff --git a/sonar-java-plugin/src/main/resources/profiles/Sonar_way/S9436 b/sonar-java-plugin/src/main/resources/profiles/Sonar_way/S9436 new file mode 100644 index 00000000000..e69de29bb2d