From 951c7da4b7576751dcac19d6fe3f249c9953f764 Mon Sep 17 00:00:00 2001 From: Antonio Aversa Date: Thu, 14 Mar 2024 14:22:26 +0100 Subject: [PATCH] Modify rule S3416: add C# and VB.NET (#2528) * Modify rule S3416: add C# and VB.NET * Add logging frameworks to the list of allowed frameworks * Reverse adding logging frameworks * Fix issues from adoc validation * Review 1 * Fix tabbing * Review 2 * Add list of supported frameworks * Missed renames * Add 'logging' tag * Remove VB.NET * Fix a minor typo --------- Co-authored-by: Gregory Paidis Co-authored-by: Gregory Paidis <115458417+gregory-paidis-sonarsource@users.noreply.github.com> --- rules/S3416/comments-and-links.adoc | 10 ++++ rules/S3416/csharp/how.adoc | 58 +++++++++++++++++++ rules/S3416/csharp/metadata.json | 4 ++ rules/S3416/csharp/rule.adoc | 3 + rules/S3416/csharp/why-code-example.adoc | 13 +++++ rules/S3416/csharp/why-exception-example.adoc | 13 +++++ rules/S3416/java/metadata.json | 34 +---------- rules/S3416/java/rule.adoc | 10 +--- rules/S3416/metadata.json | 31 ++++++++++ rules/S3416/rspecator-dotnet.adoc | 21 +++++++ rules/S3416/rule-dotnet.adoc | 57 ++++++++++++++++++ 11 files changed, 212 insertions(+), 42 deletions(-) create mode 100644 rules/S3416/comments-and-links.adoc create mode 100644 rules/S3416/csharp/how.adoc create mode 100644 rules/S3416/csharp/metadata.json create mode 100644 rules/S3416/csharp/rule.adoc create mode 100644 rules/S3416/csharp/why-code-example.adoc create mode 100644 rules/S3416/csharp/why-exception-example.adoc create mode 100644 rules/S3416/rspecator-dotnet.adoc create mode 100644 rules/S3416/rule-dotnet.adoc diff --git a/rules/S3416/comments-and-links.adoc b/rules/S3416/comments-and-links.adoc new file mode 100644 index 0000000000..5a538060b9 --- /dev/null +++ b/rules/S3416/comments-and-links.adoc @@ -0,0 +1,10 @@ +== Comments And Links +(visible only on this page) + +=== on 25 Nov 2015, 09:22:38 Freddy Mallet wrote: +See my comment on relating Google Group thread [~ann.campbell.2]: \https://groups.google.com/forum/?utm_medium=email&utm_source=footer#!msg/sonarqube/7DGhtbqRsHk/j2rDIp6dAwAJ + +=== on 7 Jun 2018, 14:17:52 Andrei Epure wrote: +Also requested in \https://groups.google.com/forum/?utm_source=digest&utm_medium=email#!topic/sonarqube/qCK_pZJ7G_Q + + diff --git a/rules/S3416/csharp/how.adoc b/rules/S3416/csharp/how.adoc new file mode 100644 index 0000000000..65036aac96 --- /dev/null +++ b/rules/S3416/csharp/how.adoc @@ -0,0 +1,58 @@ +== How to fix it + +When the logger name is defined by a generic type parameter: + +[source,csharp] +---- +class EnclosingType +{ + private readonly ILogger logger; + + public EnclosingType(ILoggerFactory loggerFactory) + { + logger = loggerFactory.CreateLogger(); // Noncompliant + logger = loggerFactory.CreateLogger(); // Compliant + } +} +---- + +When the logger name is defined by an input parameter of type `Type`: + +[source,csharp] +---- +class EnclosingType +{ + private readonly ILogger logger; + + public EnclosingType(ILoggerFactory loggerFactory) + { + logger = loggerFactory.CreateLogger(typeof(AnotherType)); // Noncompliant + logger = loggerFactory.CreateLogger(typeof(EnclosingType)); // Compliant + logger = loggerFactory.CreateLogger(GetType()); // Compliant + } +} +---- + +When the logger name is a string, derived from a `Type`: + +[source,csharp] +---- +class EnclosingType +{ + private readonly ILogger logger; + + public EnclosingType(ILoggerFactory loggerFactory) + { + logger = loggerFactory.CreateLogger(typeof(AnotherType).Name); // Noncompliant + logger = loggerFactory.CreateLogger(typeof(AnotherType).FullName); // Noncompliant + logger = loggerFactory.CreateLogger(nameof(AnotherType)); // Noncompliant + // Fix by referring to the right type + logger = loggerFactory.CreateLogger(typeof(EnclosingType).Name); // Compliant + logger = loggerFactory.CreateLogger(typeof(EnclosingType).FullName); // Compliant + logger = loggerFactory.CreateLogger(nameof(EnclosingType)); // Compliant + // or by retrieving the right type dynamically + logger = loggerFactory.CreateLogger(GetType().FullName); // Compliant + } +} +---- + diff --git a/rules/S3416/csharp/metadata.json b/rules/S3416/csharp/metadata.json new file mode 100644 index 0000000000..589c6c0bc0 --- /dev/null +++ b/rules/S3416/csharp/metadata.json @@ -0,0 +1,4 @@ +{ + "title": "Loggers should be named for their enclosing types", + "quickfix": "targeted" +} diff --git a/rules/S3416/csharp/rule.adoc b/rules/S3416/csharp/rule.adoc new file mode 100644 index 0000000000..aaccfa20c2 --- /dev/null +++ b/rules/S3416/csharp/rule.adoc @@ -0,0 +1,3 @@ +:language: csharp + +include::../rule-dotnet.adoc[] \ No newline at end of file diff --git a/rules/S3416/csharp/why-code-example.adoc b/rules/S3416/csharp/why-code-example.adoc new file mode 100644 index 0000000000..37fa2bed44 --- /dev/null +++ b/rules/S3416/csharp/why-code-example.adoc @@ -0,0 +1,13 @@ +[source,csharp] +---- +class EnclosingType +{ + private readonly ILogger logger; + + public EnclosingType(ILoggerFactory loggerFactory) + { + logger = loggerFactory.CreateLogger(); // Noncompliant + logger = loggerFactory.CreateLogger(); // Compliant + } +} +---- diff --git a/rules/S3416/csharp/why-exception-example.adoc b/rules/S3416/csharp/why-exception-example.adoc new file mode 100644 index 0000000000..acbfd5010e --- /dev/null +++ b/rules/S3416/csharp/why-exception-example.adoc @@ -0,0 +1,13 @@ +[source,csharp] +---- +class EnclosingType +{ + private readonly ILogger logger; + + EnclosingType(ILoggerFactory loggerFactory) + { + logger = loggerFactory.CreateLogger("My cross-type logging category"); // Compliant + logger = loggerFactory.CreateLogger(AComplexLogicToFindTheRightType()); // Compliant + } +} +---- diff --git a/rules/S3416/java/metadata.json b/rules/S3416/java/metadata.json index 2b0cc9d363..7a73a41bfd 100644 --- a/rules/S3416/java/metadata.json +++ b/rules/S3416/java/metadata.json @@ -1,34 +1,2 @@ { - "title": "Loggers should be named for their enclosing classes", - "type": "CODE_SMELL", - "code": { - "impacts": { - "MAINTAINABILITY": "LOW" - }, - "attribute": "IDENTIFIABLE" - }, - "status": "ready", - "remediation": { - "func": "Constant\/Issue", - "constantCost": "5min" - }, - "tags": [ - "confusing" - ], - "extra": { - "replacementRules": [ - - ], - "legacyKeys": [ - - ] - }, - "defaultSeverity": "Minor", - "ruleSpecification": "RSPEC-3416", - "sqKey": "S3416", - "scope": "Main", - "defaultQualityProfiles": [ - "Sonar way" - ], - "quickfix": "unknown" -} +} \ No newline at end of file diff --git a/rules/S3416/java/rule.adoc b/rules/S3416/java/rule.adoc index fee789af99..8a4d8db02d 100644 --- a/rules/S3416/java/rule.adoc +++ b/rules/S3416/java/rule.adoc @@ -36,20 +36,12 @@ ifdef::env-github,rspecator-view[] Update this logger to use the current class. - === Highlighting Xxx.class - ''' -== Comments And Links -(visible only on this page) -=== on 25 Nov 2015, 09:22:38 Freddy Mallet wrote: -See my comment on relating Google Group thread [~ann.campbell.2]: \https://groups.google.com/forum/?utm_medium=email&utm_source=footer#!msg/sonarqube/7DGhtbqRsHk/j2rDIp6dAwAJ - -=== on 7 Jun 2018, 14:17:52 Andrei Epure wrote: -Also requested in \https://groups.google.com/forum/?utm_source=digest&utm_medium=email#!topic/sonarqube/qCK_pZJ7G_Q +include::../comments-and-links.adoc[] endif::env-github,rspecator-view[] diff --git a/rules/S3416/metadata.json b/rules/S3416/metadata.json index 2c63c08510..c1b83882f3 100644 --- a/rules/S3416/metadata.json +++ b/rules/S3416/metadata.json @@ -1,2 +1,33 @@ { + "title": "Loggers should be named for their enclosing classes", + "type": "CODE_SMELL", + "status": "ready", + "code": { + "impacts": { + "MAINTAINABILITY": "LOW" + }, + "attribute": "IDENTIFIABLE" + }, + "remediation": { + "func": "Constant\/Issue", + "constantCost": "5min" + }, + "tags": [ + "confusing", + "logging" + ], + "extra": { + "replacementRules": [ + ], + "legacyKeys": [ + ] + }, + "defaultSeverity": "Minor", + "ruleSpecification": "RSPEC-3416", + "sqKey": "S3416", + "scope": "Main", + "defaultQualityProfiles": [ + "Sonar way" + ], + "quickfix": "unknown" } diff --git a/rules/S3416/rspecator-dotnet.adoc b/rules/S3416/rspecator-dotnet.adoc new file mode 100644 index 0000000000..d79825bd3a --- /dev/null +++ b/rules/S3416/rspecator-dotnet.adoc @@ -0,0 +1,21 @@ +ifdef::env-github,rspecator-view[] + +''' +== Implementation Specification +(visible only on this page) + +=== Message + +Update this logger to use its enclosing type. + +=== Highlighting + +XXX | typeof(XXX) | typeof(XXX).FullName + +''' +== Comments And Links +(visible only on this page) + +include::comments-and-links.adoc[] + +endif::env-github,rspecator-view[] diff --git a/rules/S3416/rule-dotnet.adoc b/rules/S3416/rule-dotnet.adoc new file mode 100644 index 0000000000..a345405e5e --- /dev/null +++ b/rules/S3416/rule-dotnet.adoc @@ -0,0 +1,57 @@ +== Why is this an issue? + +It is a well-established convention to name each logger after its enclosing type. This rule raises an issue when the convention is not respected. + +include::{language}/why-code-example.adoc[] + +Not following such a convention can result in confusion and logging misconfiguration. + +For example, the person configuring the log may attempt to change the logging behavior for the `MyNamespace.EnclosingType` type, by overriding defaults for the logger named after that type. + +[source,json] +---- +{ + "Logging": { + "LogLevel": { + "Default": "Error", + "MyNamespace.EnclosingType": "Debug" + } + } +} +---- + +However, if the convention is not in place, the override would not affect logs from `MyNamespace.EnclosingType`, since they are made via a logger with a different name. + +Moreover, using the same logger name for multiple types prevents the granular configuration of each type's logger, since there is no way to distinguish them in configuration. + +The rule targets the following logging frameworks: +* https://learn.microsoft.com/en-us/dotnet/core/extensions/logging[Microsoft Extensions Logging] +* https://logging.apache.org/log4net/[Apache log4net] +* https://nlog-project.org/[NLog] + +=== Exceptions + +The rule doesn't raise issues when custom handling of logging names is in place, and the logger name is not derived from a `Type`. + +include::{language}/why-exception-example.adoc[] + +include::{language}/how.adoc[] + +== Resources + +=== Documentation + +* Microsoft Learn - https://learn.microsoft.com/en-us/dotnet/core/diagnostics/logging-tracing[.NET logging and tracing] +* Microsoft Learn - https://learn.microsoft.com/en-us/dotnet/core/extensions/logging?tabs=command-line#log-category[Logging in C# and .NET - Log category] +* Microsoft Learn - https://learn.microsoft.com/en-us/dotnet/core/extensions/logging?tabs=command-line#configure-logging[Logging in C# and .NET - Configure logging] +* Microsoft Learn - https://learn.microsoft.com/en-us/dotnet/api/microsoft.extensions.logging.ilogger-1[ILogger Interface] +* Apache Logging - https://logging.apache.org/log4net/[Apache log4net] +* NLog - https://nlog-project.org/[Flexible & free open-source logging for .NET] + +=== Articles & blog posts + +* Raygun Blog - https://raygun.com/blog/c-sharp-logging-best-practices/[C# logging: Best practices in 2023 with examples and tools] +* Apache Logging - https://logging.apache.org/log4net/release/manual/configuration.html[Apache log4net Manual - Configuration] +* GitHub NLog repository - https://github.com/nlog/nlog/wiki/Tutorial#best-practices-for-using-nlog[Best practices for using NLog] + +include::rspecator-dotnet.adoc[] \ No newline at end of file