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;
}
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 ), then you can:
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
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?
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:
If you are unsure about how getEpsBearerIds() result will be used, then it’s safer to return Collections.unmodifiableSet(epsIds) instead of epsIds.
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.