Rule "Mutable members should not be stored or returned directly" correct?

Hi,
We are running an old version, Version 6.7.7 (build 38951) .

This rule is complaining about the following code:

public final class ProfileData implements BaseProfileData {

private final Set<Integer> epsIds;

private ProfileData (ProfileDataBuilder builder) {
    epsIds= builder.epsIds;
}

public static ProfileDataBuilder getBuilder() {
    return new ProfileDataBuilder();
}



public Set<Integer> getEpsBearerIds() {
    return epsIds;
}

}

The above code gives:

MINOR SonarQube violation:

Return a copy of “epsIds”.

Read more: https:///coding_rules#rule_key=squid%3AS2384

The very important question:

is anybody modifying the set?

If yes, then you have to mark it as Won't Fix or refactor the code.

If no you have two options:

  • make SonarQube happy
  • make code very safe

If you just want to make SonarQube happy, you can add this:

private ProfileData(ProfileDataBuilder builder) {
    epsIds = Collections.unmodifiableSet(builder.epsIds);
}

Method java.util.Collections#unmodifiableSet(Set) creates a view which doesn’t allow to modify the set. It means that such operations will fail:

profileData.getEpsBearerIds().add(Integer.valueOf(12));

but it is still possible to modify the data if anybody has access to ProfileDataBuilder:

Set<Integer> ids = new HashSet<>();
BaseProfileData builder = ProfileData.getBuilder().epsIds(ids).build();
// I don't know how you create a new instance of ProfileData,
// so I'll make a constructor public
ProfileData data = new ProfileData(builder);
data.getEpsBearerIds().size(); // 0
builder.getEpsIds().add(Integer.valueOf(10));
data.getEpsBearerIds().size(); // 1
data.getEpsBearerIds().add(Integer.valueOf(13)); // throws exception

If you want to make code very safe (usually not needed :wink: ), then you can:

private ProfileData(ProfileDataBuilder builder) {
    epsIds = Collections.unmodifiableSet(new HashSet<>(builder.epsIds));
}

Now all data is copied to a new set for which we create a read-only view:

Set<Integer> ids = new HashSet<>();
BaseProfileData builder = ProfileData.getBuilder().epsIds(ids).build();
// I don't know how you create a new instance of ProfileData,
// so I'll make a constructor public
ProfileData data = new ProfileData(builder);
data.getEpsBearerIds().size(); // 0
builder.getEpsIds().add(Integer.valueOf(10));
data.getEpsBearerIds().size(); // 0
data.getEpsBearerIds().add(Integer.valueOf(13)); // throws exception

Hi Adam,

So we are basically forced to use:

Collections.unmodifiableSet

even if

epsIds

is not modified?

br,

//mikael

Also the documentation does not state it:

class A {
  private String [] strings;

  public A () {
    strings = new String[]{"first", "second"};
  }

  public String [] getStrings() {
    return strings.clone();
  }

  public void setStrings(String [] strings) {
    this.strings = strings.clone();
  }
}

public class B {

  private A a = new A();  // At this point a.strings = {"first", "second"};

  public void wreakHavoc() {
    a.getStrings()[0] = "yellow";  // a.strings = {"first", "second"};
  }
}

Could your example be a candidate for making into sonarqube?

br,

//mikael

The rule protects against returning mutable members. It doesn’t care if you modify them or not. The idea is that nobody is allowed to modify the object state by using getters or non-private fields.

Hi Mikael,
I agree with the issue raised by the rule S2384 on your code sample. And I agree with Adam on the fact that exposing the mutable set epsIds through the public method getEpsBearerIds() could be unsafe, because it’s possible to change the internal state of a ProfileData instance using profileData.getEpsBearerIds().add(injectedValue);

You have two options:

  1. If you are unsure about how getEpsBearerIds() result will be used, then it’s safer to return Collections.unmodifiableSet(epsIds) instead of epsIds.
  2. If you are 100% sure that getEpsBearerIds() result will never be modified, then mark this issue as Won't Fix. Or disable the rule S2384 if you favor performance over safty.