Rule S1948 improvement to be able to deal with Java Collections types.

[Michel Pawlak]

Hi I would like to discuss about the rules S1319 and S1948 and more precisely about the following thread and ticket :

[1] http://sonarqube.15.x6.nabble.com/Serializable-classes-with-Java-Collections-get-either-S1319-or-S1948-td5030527.html

[2] http://jira.sonarsource.com/browse/SONARJAVA-808

The ticket has relaxed the rule S1319 in order to avoid false positives due to the declaration of private fields not using a Java Collections interface. However this is not sufficient IMHO.

If we look at the following code, we see that S1948 will raise an issue du to the fact that java.util.Map is not Serializable :

public class AClass implements Serializable {
  private Map<String,String> map = new WeakHashmap<String,String>();  // Noncompliant
  private Map<String,String> map = new Hashmap<String,String>(); // Noncompliant according to S1948... but is there really a problem ?
}

This issue seems legit at first sight as Map, List, Iterable, etc. interfaces do NOT extend Serializable. However this is disputable. First, java.util.WeakHashMap, java.util DelayQueue are the only Collections classes not being Serializable and they are not widely used (I never used them so far). Second, interface based programming and decoupling interfaces from implementations provides great changeability benefits.

Therefore I would like to suggest the following improvement to rule S1948 :

- if the type of the field is a Java Collection interface, then an issue should only be raised if a non Serializable implementation of the interface is assigned to the variable and if the field is not final (which implies that the value of the field will be set at instanciation time).

The result would be :

public class AClass implements Serializable {
  private Map<String,String> map = new WeakHashmap<String,String>();  // Noncompliant : WeakHashMap is non Serializable
  private final Map<String,String> map2 = new WeakHashmap<String,String>();  // Noncompliant : WeakHashMap is non Serializable
  private final Map<String,String> map3 = new Hashmap<String,String>(); // Compliant
  private final Map<String,String> map3;

  public AClass() {
    this.map3 = new HashMap<String, String>(); // Compliant
    this.map3 = new MyCustomSerializableMap<String,String>(); // Maybe noncompliant : we're not sure that this custom Map will be available at deserialization
  }
}

Your opinion ?

Regards,

Michel

[Michel Pawlak]

Code snippet modification (sorry for the mistakes) :

public class AClass implements Serializable {
  private Map<String,String> map = new WeakHashmap<String,String>();  // Noncompliant : WeakHashMap is non Serializable
  private final Map<String,String> map2 = new WeakHashmap<String,String>();  // Noncompliant : WeakHashMap is non Serializable
  private final Map<String,String> map3 = new Hashmap<String,String>(); // Compliant

  private final Map<String,String> map4;
  private final Map<String,String> map5;


  public AClass() {
    this.map4 = new HashMap<String, String>(); // Compliant
    this.map5 = new MyCustomSerializableMap<String,String>(); // Maybe noncompliant : we're not sure that this custom Map will be available at deserialization
  }
}

[Michel Pawlak]

Hi people, does anybody have an opinion about my improvement suggestion ? On our side, the squid:S1948 rule raises 250+ false positives (which is a lot)

[Nicolas Peru]

Hi Michel, 

Sorry for late answer : I tend to agree that rule S1948 makes too much noise with collections. What I would suggest then is the following : 

-  Field is not private : it can be modified outside of the class -> raise an issue : this is actually not safe at all. 

- Field is private : find the assignement(s) and raise an issue if type assigned is not serializable.  This is not that easy to implement but still doable. 

WDYT ? 

Cheers,

Nicolas PERU | SonarSource

Senior Developer
http://sonarsource.com

https://twitter.com/benzonico

2015-08-17 10:55 GMT+02:00 Michel Pawlak <michel...@gmail.com>:

Hi people, does anybody have an opinion about my improvement suggestion ? On our side, the squid:S1948 rule raises 250+ false positives (which is a lot)

--
You received this message because you are subscribed to the Google Groups "SonarQube" group.
To unsubscribe from this group and stop receiving emails from it, send an email to sonarqube+...@googlegroups.com.
To view this discussion on the web visit https://groups.google.com/d/msgid/sonarqube/ccba4814-c046-4ea2-887a-1d705e757ee8%40googlegroups.com.

For more options, visit https://groups.google.com/d/optout.

[Michel Pawlak]

Hi Nicolas,

Looks good, thanks

Cheers,

[Nicolas Peru]

thanks for the feedback, ticket created :https://jira.sonarsource.com/browse/SONARJAVA-1225

Nicolas PERU | SonarSource

Senior Developer
http://sonarsource.com

https://twitter.com/benzonico

To view this discussion on the web visit https://groups.google.com/d/msgid/sonarqube/a0ea6321-0038-4f9a-a375-ce26f3f14602%40googlegroups.com.