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

Question

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

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 flag 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 please?

Thank you

Orison

this.map = new HashMap<>(map); and return new HashMap<>(map);. – Strunk

This is the correct answer, thanks – Orison

Answer 1

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.

Strunk

Understood, thanks! – Orison