SONARJAVA-7163 Implement rule S9436 - Classes should not declare more than one logger

[rombirli]

Implements rule S9436, which raises when a class declares more than one logger field.

A second logger field means part of the class logs under a category nobody configured, so those messages are filtered out or routed to the wrong appender, and the level set for the class governs only half of its output.

The check collects the logger-typed fields declared directly in a class body and reports every one after the first, with a secondary location on the first. Supported facades: SLF4J, Log4j 2, Log4j 1.x, java.util.logging and Commons Logging.

Nested, inner and anonymous classes each count separately, local variables are ignored, and the rule is disabled without a semantic model.

RSPEC: https://github.com/SonarSource/rspec/pull/8338
Jira: https://sonarsource.atlassian.net/browse/SONARJAVA-7163

[hashicorp-vault-sonar-prod]

SONARJAVA-7163

[rombirli]

/pvf

[hashicorp-vault-sonar-prod]

📊 Performance Validation complete
🆕 44 new · 🗑️ 0 lost issues
View dashboard

[github-actions]

⚖️ Ruling update ready for review: #6338

[rombirli]

Both fixed. Derived exemption now needs a getName() concatenation, and final uninitialized loggers count. Tests added.

[gitar-bot]

Code Review ✅ Approved 4 closed / 4 findings

🟡 Medium risk · Adding S9436 changes analyzer findings for Java classes with multiple logger fields; imperfect detection could misreport issues or miss logger configurations.

Adds rule S9436 to flag classes that declare more than one logger field, with a secondary location on the first logger. Supported facades are SLF4J, Log4j 2, Log4j 1.x, java.util.logging and Commons Logging. Resolved this session: the rule description now lists Commons Logging, the multi-declarator test checks the secondary location, the derived-logger exemption no longer skips unrelated initializers that mention a logger, and final loggers set in a block are no longer exempted as uninitialized fields.

✅ 4 closed

✅ Quality: Rule description leaves Commons Logging out of the supported facades

📄 sonar-java-plugin/src/main/resources/org/sonar/l10n/java/rules/java/S9436.html:14 📄 java-checks/src/main/java/org/sonar/java/checks/MoreThanOneLoggerCheck.java:37-42
The rule description says the rule raises "for SLF4J, Log4j, Log4j 2 and java.util.logging". The check also matches org.apache.commons.logging.Log in LOGGER_TYPES, and the sample file has a Noncompliant case for it (CommonsLoggingLoggers). The PR description lists Commons Logging as supported too. Users who read the rule page will not know that Commons Logging fields are flagged. Add Commons Logging to the sentence.

✅ Quality: Multi-declarator test case leaves the secondary location unchecked

📄 java-checks-test-sources/default/src/main/java/checks/MoreThanOneLoggerCheckSample.java:158 📄 java-checks/src/main/java/org/sonar/java/checks/MoreThanOneLoggerCheck.java:58-61
For Logger FIRST = ..., SECOND = ...; the check reports SECOND and adds a secondary location on FIRST, which sits on the same line. Every other noncompliant case in the sample declares [[secondary=...]], but this one doesn't. InternalCheckVerifier.validateAnalyzerMessageAttributes only compares secondaries when SECONDARY_LOCATIONS is present in the expectation. So the secondary location for this case is never checked, and a regression there (missing secondary, or one pointing at the wrong field) would still pass. Add [[secondary=+0]] to the expectation.

✅ Edge Case: Derived-logger exemption skips any initializer that mentions a logger

📄 java-checks/src/main/java/org/sonar/java/checks/MoreThanOneLoggerCheck.java:86-100 📄 java-checks/src/main/java/org/sonar/java/checks/MoreThanOneLoggerCheck.java:120-125 📄 sonar-java-plugin/src/main/resources/org/sonar/l10n/java/rules/java/S9436.html:9 📄 sonar-java-plugin/src/main/resources/org/sonar/l10n/java/rules/java/S9436.html:22-24
withoutDerivedLoggers drops a field when its initializer contains any identifier that resolves to another logger field of the class. It never checks that the field actually builds a child category. So Logger OTHER = LOG;, Logger OTHER = LoggerFactory.getLogger(LOG.getName()); and Logger OTHER = LoggerFactory.getLogger(LOG.getClass()); are all exempt. The first two give the class a second name for the same category, which the rule description (line 9) calls "pure duplication" and treats as part of the problem. The exception text (lines 22-24) only covers a name derived from the parent to create a child category. Fix: exempt a field only when its initializer is a logger factory call whose name argument is a + concatenation that contains <logger>.getName(), ideally followed by a string literal starting with ..

✅ Edge Case: Uninitialized-field exemption also skips final loggers set in a block

📄 java-checks/src/main/java/org/sonar/java/checks/MoreThanOneLoggerCheck.java:76-82
isDeclaredLogger drops every logger field that has no initializer. The Javadoc and the rule description justify this by calling such a field a configurable property that is assigned at startup or injected from outside. That reasoning does not hold for final fields. A field like private static final Logger A; must be assigned in a static initializer, or an instance final field in a constructor, and nothing outside the class can change it. So a class with static final Logger A; static final Logger B; static { A = LoggerFactory.getLogger(X.class); B = LoggerFactory.getLogger("other"); } really has two categories it gives itself, and the rule raises no issue. Fix: apply the exemption only to non-final fields.

Verification Repos 4 inspected

Functional validation No results

Rules No rules evaluated

Cross-repo 4 repositories inspected 0 open impacts

SonarSource/sonarlint-intellij Inspected
SonarSource/sonarlint-eclipse Inspected
SonarSource/sonarlint-vscode Inspected
SonarSource/sonar-java-symbolic-execution Inspected

Auto-approval Not enabled

Set up

Options

Auto-apply is off → Gitar will not commit updates to this branch.
Display: compact → Counting what did not apply, without listing it.

Comment with these commands to change the behavior for this request:

Auto-apply	Compact
gitar auto-apply:on	gitar display:verbose

_{Was this helpful? React with 👍 / 👎 | Gitar}

[sonarqube-next]

Quality Gate passed

Issues
0 New issues
0 Fixed issues
0 Accepted issues

Measures
0 Security Hotspots
0 Dependency risks
93.6% Coverage on New Code
0.0% Duplication on New Code

See analysis details on SonarQube

[romainbrenguier]

Maybe one test-case to check, otherwise looks good. We can check with mega-FPS on peach if it makes sense.

[romainbrenguier]

What about LoggerFactory.getLogger(DerivedChildLogger.class.getName() + ".config")?

[romainbrenguier]

It would be nice to have a compliant/non-compliant example with the Marker case. (to be done on the RSPEC PR)