Skip to content

Commit

Permalink
Extend AvoidUnguardedAssignmentToNonFinalFieldsInSharedObjects rule f…
Browse files Browse the repository at this point in the history 
…or inherited fields #18
  • Loading branch information
@jborgers
jborgers committed Apr 16, 2020
1 parent e9e0a27 commit 9d9c64d
Show file tree
Hide file tree
Showing 3 changed files with 70 additions and 20 deletions.
26 changes: 18 additions & 8 deletions rulesets/java/jpinpoint-rules.xml
View file
Original file line number Diff line number Diff line change
Expand Up @@ -701,23 +701,25 @@ ClassOrInterfaceBodyDeclaration//MethodDeclaration/Block//PrimaryExpression/Prim
<example>
<![CDATA[
class Bad {
PortletSession session; // same for HttpSession
public void setAttribute(String name, Object obj) {
getPortletSession().setAttribute(name, obj); // bad
session.setAttribute(name, obj); // bad
}
public Object getAttribute(String sessionAttributeName) {
return getPortletSession().getAttribute(sessionAttributeName);
public Object getAttribute(String name) {
return session.getAttribute(name);
}
}
class Good {
PortletSession session; // same for HttpSession
public void setAttribute(String name, final Object obj) {
getPortletSession().setAttribute(name, obj);
session.setAttribute(name, obj);
}
public Object getAttribute(String sessionAttributeName) {
return getPortletSession().getAttribute(sessionAttributeName);
public Object getAttribute(String name) {
return session.getAttribute(name);
}
public void removeAttribute(String name) {
getPortletSession().removeAttribute(name);
session.removeAttribute(name);
}
}
]]>
Expand Down Expand Up @@ -1314,6 +1316,7 @@ or (ancestor::ClassOrInterfaceBodyDeclaration/Annotation//Name[@Image='VisibleFo
1. Autowiring/injection is thread safe, yet make sure no other thread-unsafe assignment is made to that field.&#13;
2. In case you are sure the Component is used in single threaded context only (e.g. a Tasklet), annotate the class with @NotThreadSafe to make this explicit. &#13;
3. Use package-private and @VisibleForTesting for methods (e.g. setters) used for JUnit only.
4. Use synchronized for accessors to inherited fields, or better: make field private and use proper accessors on base class level using @GuardedBy.
(jpinpoint-rules)</description>
<priority>2</priority>
<properties>
Expand All @@ -1331,9 +1334,16 @@ and not (ancestor::TypeDeclaration/Annotation/MarkerAnnotation/Name[@Image='NotT
and not ((ancestor::TypeDeclaration/Annotation//Name[@Image='ConfigurationProperties'])
and not (ancestor::TypeDeclaration/Annotation/MarkerAnnotation/Name[@Image='Setter']))]
(: assignment to a field :)
/../../..//ClassOrInterfaceDeclaration[@Static=false()]/ClassOrInterfaceBody/ClassOrInterfaceBodyDeclaration/MethodDeclaration/Block/BlockStatement/Statement//StatementExpression/AssignmentOperator/../PrimaryExpression//*[@Image=
/../../..//ClassOrInterfaceDeclaration[@Static=false()]/ClassOrInterfaceBody/ClassOrInterfaceBodyDeclaration/MethodDeclaration/Block/BlockStatement/Statement//StatementExpression/AssignmentOperator/../PrimaryExpression//*[(@Image=
(: non-final, non-volatile, non-GuardedBy fields :)
ancestor::ClassOrInterfaceBody/ClassOrInterfaceBodyDeclaration/FieldDeclaration[@Final=false() and @Volatile=false() and not (../Annotation//Name[@Image='GuardedBy'])]/VariableDeclarator/VariableDeclaratorId/@Image
or
(: not 'this' and extends a base class :)
((@Image != '') and ancestor::ClassOrInterfaceDeclaration/ExtendsList and
(: not a field of this class, param or local :)
not (exists(index-of((ancestor::ClassOrInterfaceBody//VariableDeclaratorId/@Image), @Image))))
and not (ancestor::SynchronizedStatement or ancestor::MethodDeclaration[@Synchronized = true()])
)
(: field not on accessor method with assignment, annotated with framework annotation :)
and not (ancestor::ClassOrInterfaceBodyDeclaration/Annotation//Name[@Image='Autowired' or @Image='PostConstruct' or @Image='BeforeStep' or @Image='Value' or @Image='Inject']
(: field not assigned in non-public accessor method annotated with VisibleForTesting :)
Expand Down
10 changes: 9 additions & 1 deletion src/main/resources/category/java/concurrent.xml
View file
Original file line number Diff line number Diff line change
Expand Up @@ -182,6 +182,7 @@ ancestor::StatementExpression/PrimaryExpression/PrimaryPrefix/Name/@Image = ance
1. Autowiring/injection is thread safe, yet make sure no other thread-unsafe assignment is made to that field.&#13;
2. In case you are sure the Component is used in single threaded context only (e.g. a Tasklet), annotate the class with @NotThreadSafe to make this explicit. &#13;
3. Use package-private and @VisibleForTesting for methods (e.g. setters) used for JUnit only.
4. Use synchronized for accessors to inherited fields, or better: make field private and use proper accessors on base class level using @GuardedBy.
(jpinpoint-rules)</description>
<priority>2</priority>
<properties>
Expand All @@ -199,9 +200,16 @@ and not (ancestor::TypeDeclaration/Annotation/MarkerAnnotation/Name[@Image='NotT
and not ((ancestor::TypeDeclaration/Annotation//Name[@Image='ConfigurationProperties'])
and not (ancestor::TypeDeclaration/Annotation/MarkerAnnotation/Name[@Image='Setter']))]
(: assignment to a field :)
/../../..//ClassOrInterfaceDeclaration[@Static=false()]/ClassOrInterfaceBody/ClassOrInterfaceBodyDeclaration/MethodDeclaration/Block/BlockStatement/Statement//StatementExpression/AssignmentOperator/../PrimaryExpression//*[@Image=
/../../..//ClassOrInterfaceDeclaration[@Static=false()]/ClassOrInterfaceBody/ClassOrInterfaceBodyDeclaration/MethodDeclaration/Block/BlockStatement/Statement//StatementExpression/AssignmentOperator/../PrimaryExpression//*[(@Image=
(: non-final, non-volatile, non-GuardedBy fields :)
ancestor::ClassOrInterfaceBody/ClassOrInterfaceBodyDeclaration/FieldDeclaration[@Final=false() and @Volatile=false() and not (../Annotation//Name[@Image='GuardedBy'])]/VariableDeclarator/VariableDeclaratorId/@Image
or
(: not 'this' and extends a base class :)
((@Image != '') and ancestor::ClassOrInterfaceDeclaration/ExtendsList and
(: not a field of this class, param or local :)
not (exists(index-of((ancestor::ClassOrInterfaceBody//VariableDeclaratorId/@Image), @Image))))
and not (ancestor::SynchronizedStatement or ancestor::MethodDeclaration[@Synchronized = true()])
)
(: field not on accessor method with assignment, annotated with framework annotation :)
and not (ancestor::ClassOrInterfaceBodyDeclaration/Annotation//Name[@Image='Autowired' or @Image='PostConstruct' or @Image='BeforeStep' or @Image='Value' or @Image='Inject']
(: field not assigned in non-public accessor method annotated with VisibleForTesting :)
Expand Down
54 changes: 43 additions & 11 deletions ...g/java/ruleset/concurrent/xml/AvoidUnguardedAssignmentToNonFinalFieldsInSharedObjects.xml
View file
Original file line number Diff line number Diff line change
Expand Up @@ -5,7 +5,8 @@
xsi:schemaLocation="http://pmd.sourceforge.net/rule-tests http://pmd.sourceforge.net/rule-tests_1_0_0.xsd">
<test-code>
<description>Avoid unguarded assignment to non-final fields in shared objects</description>
<expected-problems>10</expected-problems>
<expected-problems>11</expected-problems>
<expected-linenumbers>55, 87, 96, 137, 138, 157, 198, 223, 328, 338, 395</expected-linenumbers>
<code><![CDATA[
import com.google.common.annotations.VisibleForTesting;
import lombok.Data;
Expand Down Expand Up @@ -61,7 +62,7 @@ class AService1 {
private String url; // violation on next line, of additional unguarded accessor method
public void setUrlViolate(final String url) {
this.url = url;
this.url = url; // bad
} // violation: unguarded accessor method
}
Expand Down Expand Up @@ -93,7 +94,7 @@ class ASingletonBean {
private String url;
public void setUrlViolate(final String url) {
this.url = url;
this.url = url; // bad
} // violation: unguarded accessor method
}
Expand All @@ -102,7 +103,7 @@ class ASingletonBean {
class AComponent2 {
private String _url;
public void setUrlViolate(final String url) {
_url = url;
_url = url; // bad
} // violation: unguarded accessor method
}
Expand Down Expand Up @@ -143,8 +144,8 @@ class AnotherComponent {
private String wiredOk;
public void setUrlViolate(String url) {
url += "/";
this.url = url;
url += "/"; // bad
this.url = url; // bad
} // violation: unguarded accessor method
public void setVolatileOk(String vol) {
Expand All @@ -163,7 +164,7 @@ class AnotherComponent {
class ASessionScopedComponent {
private String _url;
public void setUrlViolate(final String url) {
_url = url;
_url = url; // bad
} // violation: unguarded accessor method
}
Expand Down Expand Up @@ -204,7 +205,7 @@ class ASyncedSingletonConcurrencyBean {
@VisibleForTesting
public void setUrlForTestingOnlyViolate(String urlForTestingOnly) {
this.url = urlForTestingOnly;
this.url = urlForTestingOnly; // bad
}
}
Expand All @@ -229,7 +230,7 @@ class AComponentMaoValidation {
}
public void doRestTemplateViolate() {
restTemplateViolate = null;
restTemplateViolate = null; // bad
}
@Data
Expand Down Expand Up @@ -334,7 +335,7 @@ class APayRequestOutSender {
}
private void setMax() {
try {
databaseQueryMaxResultsViolation = 5000;
databaseQueryMaxResultsViolation = 5000; // bad
} catch (RuntimeException e) {}
}
}
Expand All @@ -344,7 +345,7 @@ abstract class AInitializeTaskletViolate implements Tasklet, InitializingBean {
// Work directory for this instance only.
protected String workDirectoryViolate;
public void setWorkDirectory(final String workDirectory) {
this.workDirectoryViolate = workDirectory;
this.workDirectoryViolate = workDirectory; // bad
}
}
Expand Down Expand Up @@ -392,6 +393,37 @@ class Foo {
String CONTEXT_ATTRIBUTES_BEAN_NAME = "contextAttributes";
*/
// inherited field used
@Component
class SubClassBad extends Base {
SubClassBad() {
baseField = ""; // good, could be final
}
public void setBaseField(final String val) {
baseField = val; // bad
} // violation: unguarded accessor method
}
// inherited field used
@Component
class SubClassGood extends Base {
private final Object LOCK = new Object();
SubClassGood() {
baseField = ""; // good, could be final
}
public synchronized void setBaseField1(final String val) {
baseField = val; // assumed good
}
public void setBaseField2(final String val) {
synchronized(LOCK) {
baseField = val; // assumed good
}
}
}
]]></code>
</test-code>
</test-data>

0 comments on commit 9d9c64d

Please sign in to comment.