java:S2226 False positive: Fails to recognize that servlet init() method intializes field by assignment to this.field =

[nimarukan]

Sonarqube 9.1

---------- src/main/java/pkg/InitInstanceFieldServlet.java ----------

package pkg;

import java.io.IOException;

import javax.servlet.ServletException;
import javax.servlet.http.HttpServlet;
import javax.servlet.http.HttpServletRequest;
import javax.servlet.http.HttpServletResponse;

public class InitInstanceFieldServlet extends HttpServlet {
  // java:S2226 flags this field (false positive, it is initialized in the init() method)
  private int timeoutSeconds;
  // java:S2226 does not flag this field (also initialized in the init() method)
  private long timeoutMilliseconds;

  public @Override void init() throws ServletException {
    String timeoutSecondsString =
      this.getServletContext().getInitParameter("timeoutSeconds");

    // java:S2226 flags this field (false positive)
    this.timeoutSeconds = Integer.parseInt(timeoutSecondsString);

    // java:S2226 does not flag this field
    timeoutMilliseconds = this.timeoutSeconds * 1000L;

    // Apparently:
    // java:S2226 fails to recognize assignment with "this." before field name.
  }

  protected @Override void doGet(HttpServletRequest request,
                                 HttpServletResponse response)
  throws IOException, ServletException {
    try {
      Thread.sleep(this.timeoutMilliseconds);
    } catch (InterruptedException ex) {
      Thread.currentThread().interrupt();
    }
    try {
      response.sendError(HttpServletResponse.SC_INTERNAL_SERVER_ERROR,
                         "timed out after "+this.timeoutSeconds+" seconds");
    } catch (IOException ignore) {
      // connection lost
    }
  }
}

---------- build.gradle ----------

plugins {
  id "org.sonarqube" version "3.3"
}
apply plugin: "java"
apply plugin: "war"

repositories {
  mavenCentral()
}
dependencies {
  providedCompile 'javax.servlet:javax.servlet-api:3.1.0'
}

---------- src/main/webapp/WEB-INF/web.xml --------------------

<web-app xmlns="http://java.sun.com/xml/ns/javaee" version="2.5">
  <display-name>Init Servlet Field</display-name>
  <description>
    Example that initializes a servlet field through its init method.
  </description>
  <context-param>
    <description>Sleep timeout in seconds</description>
    <param-name>timeoutSeconds</param-name>
    <param-value>10</param-value>
  </context-param>
  <servlet>
    <servlet-name>servlet-init-instance-field</servlet-name>
    <servlet-class>pkg.InitInstanceFieldServlet</servlet-class>
  </servlet>
  <servlet-mapping>
    <servlet-name>servlet-init-instance-field</servlet-name>
    <url-pattern></url-pattern>
    <url-pattern>/</url-pattern>
  </servlet-mapping>
</web-app>

[nimarukan]

The above description contains a minimal project that demonstrates that a field intialized in a servlet init() method with “this.field = …” can trigger a false positive of java S2226.

java S2226 says Servlets should not have mutable instance fields, with exceptions for fields initialized in the init() method, or for fields annotated to be initialized by injection.

Servlets should not have mutable instance fields        java:S2226
…
Exceptions

• Fields annotated with @javax.inject.Inject , @javax.ejb.EJB , @org.springframework.beans.factory.annotation.Autowired , @javax.annotation.Resource
• Fields initialized in init() or init(ServletConfig config) methods

[Dorian_Burihabwa]

Hello @nimarukan,
Thanks for reporting the issue. It looks like the rule does not identify instance fields in the init method that are referenced with this in the assignment.
A ticket has been created to solve the issue.

Dorian

[nimarukan]

Thank you. Good to hear there is progress. But an initialization like what appears in the ticket, “this.first = 42”, does not make clear why it must be initialized in the init method.

In case you do not yet have a fix, here is what I found.

Below is a possible addition to the test case for S2226, to test for a field initialized within the init method by “this.field = …”.

diff --git a/java-checks-test-sources/src/main/java/checks/ServletInstanceFieldCheck.java b/java-checks-test-sources/src/main/java/checks/ServletInstanceFieldCheck.java
--- a/java-checks-test-sources/src/main/java/checks/ServletInstanceFieldCheck.java
+++ b/java-checks-test-sources/src/main/java/checks/ServletInstanceFieldCheck.java
@@ -81,3 +81,13 @@ class HttpServletE extends HttpServlet {
     }
   }
 }
+
+class HttpServletF extends HttpServlet {
+  private String storageType; // Compliant, initialized in init() method
+
+  public void init() {
+    // initialized with "this." prefix
+    this.storageType = getServletConfig().getInitParameter("storageType");
+  }
+
+}

[nimarukan]

Below is a possible fix that removes the violation raised by the amended test case.

diff --git a/java-checks/src/main/java/org/sonar/java/checks/ServletInstanceFieldCheck.java b/java-checks/src/main/java/org/sonar/java/checks/ServletInstanceFieldCheck.java
--- a/java-checks/src/main/java/org/sonar/java/checks/ServletInstanceFieldCheck.java
+++ b/java-checks/src/main/java/org/sonar/java/checks/ServletInstanceFieldCheck.java
@@ -30,6 +30,7 @@ import org.sonar.plugins.java.api.semant
 import org.sonar.plugins.java.api.tree.AssignmentExpressionTree;
 import org.sonar.plugins.java.api.tree.BaseTreeVisitor;
 import org.sonar.plugins.java.api.tree.IdentifierTree;
+import org.sonar.plugins.java.api.tree.MemberSelectExpressionTree;
 import org.sonar.plugins.java.api.tree.MethodTree;
 import org.sonar.plugins.java.api.tree.Modifier;
 import org.sonar.plugins.java.api.tree.ModifiersTree;
@@ -106,6 +107,15 @@ public class ServletInstanceFieldCheck e
         if (declaration != null && declaration.is(Kind.VARIABLE)) {
           excludedVariables.add((VariableTree) declaration);
         }
+      } else if (tree.variable().is(Kind.MEMBER_SELECT)) {
+        MemberSelectExpressionTree memberSelectTree = (MemberSelectExpressionTree) tree.variable();
+        if (memberSelectTree.expression().is(Kind.IDENTIFIER) &&
+            "this".equals(((IdentifierTree) memberSelectTree.expression()).name())) {
+          Tree declaration = ((IdentifierTree) memberSelectTree.identifier()).symbol().declaration();
+          if (declaration != null && declaration.is(Kind.VARIABLE)) {
+            excludedVariables.add((VariableTree) declaration);
+          }
+        }
       }
     }
   }

[nimarukan]

Also, the ticket title seems to say there no problem to fix? I suggest renaming it
"S2226 improperly identifies instance variables assigned in init method with this.field = ..."

https://jira.sonarsource.com/browse/SONARJAVA-4061

[Dorian_Burihabwa]

The ticket has been renamed to be more accurate.