Skip to content

GCI1066 [Team TREE][2025] - For logging format, prefer using %s with kwargs instead of builtin formatter \"\".format() or f\"\"" #84

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Open
wants to merge 1 commit into
base: main
Choose a base branch
from
Open

GCI1066 [Team TREE][2025] - For logging format, prefer using %s with kwargs instead of builtin formatter \"\".format() or f\"\"" #84

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
34 changes: 34 additions & 0 deletions .../org/greencodeinitiative/creedengo/python/checks/DetectBadLoggingFormatInterpolation.java
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1,34 @@
package org.greencodeinitiative.creedengo.python.checks;
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Add corresponding header :
/*

  • creedengo - Python language - Provides rules to reduce the environmental footprint of your Python programs
  • Copyright © 2024 Green Code Initiative (https://green-code-initiative.org)
  • This program is free software: you can redistribute it and/or modify
  • it under the terms of the GNU General Public License as published by
  • the Free Software Foundation, either version 3 of the License, or
  • (at your option) any later version.
  • 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
  • GNU General Public License for more details.
  • You should have received a copy of the GNU General Public License
  • along with this program. If not, see http://www.gnu.org/licenses/.
    */


import org.sonar.check.Rule;
import org.sonar.plugins.python.api.PythonSubscriptionCheck;
import org.sonar.plugins.python.api.SubscriptionContext;
import org.sonar.plugins.python.api.tree.*;
import java.util.regex.Pattern;

@Rule(key = "GCI1066")
public class DetectBadLoggingFormatInterpolation extends PythonSubscriptionCheck {

protected static final String MESSAGE_RULE = "For logging format, prefer using %s with kwargs instead of builtin formatter \"\".format() or f\"\"";
private static final Pattern PATTERN_FORMAT = Pattern.compile("f['\"].*['\"]");
private static final Pattern PATTERN_LOGGER = Pattern.compile("logging|logger");
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I suggest making a version that is insensitive to the variable name because the user can define it, so decteion won't work:

here's what I suggest:
public class DetectBadLoggingFormatInterpolation extends PythonSubscriptionCheck {

protected static final String MESSAGE_RULE = "For logging format, prefer using %s with kwargs instead of builtin formatter \"\".format() or f\"\"";
private static final Pattern PATTERN_FORMAT = Pattern.compile("f['\"].*['\"]");
private static final Pattern PATTERN_LOGGING_METHOD = Pattern.compile("info|debug|error");
private static final String LOGGER_FUNCCTION = "logging.getLogger";
private final Set<String> loggerNames = new HashSet<>();

@Override
public void initialize(Context context) {
    context.registerSyntaxNodeConsumer(Tree.Kind.CALL_EXPR, this::visitNodeString);
    context.registerSyntaxNodeConsumer(ASSIGNMENT_STMT, this::handleLoggerAssignment);
}

public void visitNodeString(SubscriptionContext ctx) {
    CallExpression callExpression = (CallExpression) ctx.syntaxNode();
    Expression callee = callExpression.callee();

    if (!(callee instanceof QualifiedExpression)) {
        return;
    }

    QualifiedExpression qualifiedExpression = (QualifiedExpression) callee;
    Expression qualifier = qualifiedExpression.qualifier();

    if (!(qualifier instanceof Name)) {
        return;
    }

    Name qualifierName = (Name) qualifier;

    if (PATTERN_LOGGING_METHOD.matcher(qualifiedExpression.name().name()).matches()
            && loggerNames.contains(qualifierName.name())
            && !callExpression.arguments().isEmpty()
            && callExpression.arguments().get(0) instanceof RegularArgument) {

        Expression argExpr = ((RegularArgument) callExpression.arguments().get(0)).expression();

        if (argExpr.firstToken() != null && PATTERN_FORMAT.matcher(argExpr.firstToken().value()).matches()) {
            ctx.addIssue(callExpression, MESSAGE_RULE);
        }
    }
}

public void handleLoggerAssignment(SubscriptionContext ctx) {
    var assignmentStmt = (AssignmentStatement) ctx.syntaxNode();
    var value = assignmentStmt.assignedValue();

    if (value.is(CALL_EXPR) && isLoggerCreation((CallExpression) value)) {
        String variableName = Utils.getVariableName(ctx);
        if (variableName != null) {
            loggerNames.add(variableName);
            System.out.println("Logger created with name: " + variableName);
        }
    }
}

private boolean isLoggerCreation(CallExpression callExpression) {
    return LOGGER_FUNCCTION.equals(Utils.getQualifiedName(callExpression));
}

}

private static final Pattern PATTERN_LOGGING_METHOD = Pattern.compile("info|debug|error");

@Override
public void initialize(Context context) {
context.registerSyntaxNodeConsumer(Tree.Kind.CALL_EXPR, this::visitNodeString);
}

public void visitNodeString(SubscriptionContext ctx) {
CallExpression callExpression = (CallExpression) ctx.syntaxNode();

QualifiedExpression qualifiedExpression = (QualifiedExpression)((CallExpression)ctx.syntaxNode()).callee();

if(PATTERN_LOGGING_METHOD.matcher(qualifiedExpression.name().name()).matches()
&& PATTERN_LOGGER.matcher(((Name)qualifiedExpression.qualifier()).name()).matches()
&& PATTERN_FORMAT.matcher(((RegularArgument)callExpression.arguments().get(0)).expression().firstToken().value()).matches()) {
ctx.addIssue(callExpression, MESSAGE_RULE);
}
}

}
14 changes: 14 additions & 0 deletions .../greencodeinitiative/creedengo/python/checks/DetectBadLoggingFormatInterpolationTest.java
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1,14 @@
package org.greencodeinitiative.creedengo.python.checks;
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Add corresponding header :
/*

  • creedengo - Python language - Provides rules to reduce the environmental footprint of your Python programs
  • Copyright © 2024 Green Code Initiative (https://green-code-initiative.org)
  • This program is free software: you can redistribute it and/or modify
  • it under the terms of the GNU General Public License as published by
  • the Free Software Foundation, either version 3 of the License, or
  • (at your option) any later version.
  • 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
  • GNU General Public License for more details.
  • You should have received a copy of the GNU General Public License
  • along with this program. If not, see http://www.gnu.org/licenses/.
    */


import org.junit.Test;
import org.sonar.python.checks.utils.PythonCheckVerifier;

public class DetectBadLoggingFormatInterpolationTest {

@Test
public void test_bad_logging_format_interpolation() {
PythonCheckVerifier.verify("src/test/resources/checks/detectBadLoggingFormatInterpolationNonCompliant.py", new DetectBadLoggingFormatInterpolation());
PythonCheckVerifier.verifyNoIssue("src/test/resources/checks/detectBadLoggingFormatInterpolationCompliant.py", new DetectBadLoggingFormatInterpolation());
}

}
6 changes: 6 additions & 0 deletions src/test/resources/checks/detectBadLoggingFormatInterpolationCompliant.py
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1,6 @@
import logging
logger = logging.getLogger()
name = "world"

logging.info("Hello %s", name) # Correct
logger.debug("Hello %s", name) # Correct
7 changes: 7 additions & 0 deletions src/test/resources/checks/detectBadLoggingFormatInterpolationNonCompliant.py
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1,7 @@
import logging
logger = logging.getLogger()
name = "world"

#logging.info("Hello {}".format(name)) # Noncompliant {{For logging format, prefer using %s with kwargs instead of builtin formatter "".format() or f""}}
logger.debug(f'Hello {name}') # Noncompliant {{For logging format, prefer using %s with kwargs instead of builtin formatter "".format() or f""}}
logger.error(f"Hello {name}") # Noncompliant {{For logging format, prefer using %s with kwargs instead of builtin formatter "".format() or f""}}
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

If you modify the variable name so that it is not modifiable, you will have to add the corresponding tests, for example :

my_logger = logging.getLogger()
user = "admin"
ip = "192.168.0.1"

my_logger.debug(f"User {user} attempted login") # Noncompliant {{For logging format, prefer using %s with kwargs instead of builtin formatter "".format() or f""}}
my_logger.error(f"Blocked IP: {ip}") # Noncompliant {{For logging format, prefer using %s with kwargs instead of builtin formatter "".format() or f""}}

my_logger.info("Static log with no variables")

my_logger.info("User %s connected", user)
my_logger.warning("IP %s is blocked", ip)
my_logger.debug("User %s attempted login", user)
my_logger.error("Blocked IP: %s", ip)
my_logger.critical("CRITICAL issue with user %s on IP %s", user, ip)

if True:
my_logger.debug(f"Conditional debug for {user}") # Noncompliant {{For logging format, prefer using %s with kwargs instead of builtin formatter "".format() or f""}}

with open("log.txt", "w") as f:
my_logger.info("Opened file for logging")

class MyService:
def init(self):
self.logger = logging.getLogger("service")

def report(self, user):
    self.logger.info("Reporting user %s", user)

Loading