CODEWITHSUNDEEP
javafindbugsspotbugs
PatPanda
PatPanda

Reputation: 5018

Spotbugs + Java: may expose internal representation by storing an externally mutable object into QuestionPojo.map

Small question regarding a Spotbugs finding I am having a hard time to fix.

On this super simple POJO:

import java.util.Map;

public class QuestionPojo {

    private final Map<String, String> map;

    public QuestionPojo(Map<String, String> map) {
        this.map = map;
    }

    public Map<String, String> getMap() {
        return map;
    }

    @Override
    public String toString() {
        return "QuestionPojo{" +
                "map=" + map +
                '}';
    }
    
}

I am getting flagged on the map with

may expose internal representation by storing an externally mutable object into QuestionPojo.map

One time on this.map = map;

Another one on the getter return map.

I tried invoking a possible clone() method, but it seems it is not supported in Map.

How do I fix this?

Upvotes: 6

Views: 16271

Answers (1)

Andy Turner
Andy Turner

Reputation: 140329

may expose internal representation by storing an externally mutable object into QuestionPojo.map

What this is telling you is that the internal state of an instance of this class can be changed outside the class.

For example:

Map<String, String> map = new HashMap<>();
map.put("hello", "world");

QuestionPojo qp = new QuestionPojo(map);

// Both of these lines change the map stored inside qp.
map.clear();
qp.getMap().put("silly", "silly");

As to whether this external change of state is important depends on the semantics of your class - in general it is undesirable, however, because it makes it hard to reason about its behavior, because the map can be changed from afar (i.e. anywhere that has a reference to the QuestionPojo, or the map).

The solution to this is defensive copying: take a copy of the map in the constructor:

public QuestionPojo(Map<String, String> map) {
    this.map = new HashMap<>(map);
}

and return a copy of the map in the getter:

public Map<String, String> getMap() {
    return new HashMap<>(map);
}

This means that nothing outside the class has access to the map stored inside the class.

Upvotes: 5

Related Questions