How does Collections.unmodifiableList (Java 8 SE) get modified when making a new ArrayList from it?

Question

I have a getter that returns an unmodifiable list, as such:

public List<Product> getProductList() {
    if (productList == null)
        return new ArrayList<>();
    return Collections.unmodifiableList(productList);
}

I am calling this getter as such:

List<Product> productList = new ArrayList<>(report.getProductList());

Then I pass this list to another method that modifies the list as such:

for (Product product : productList) {
    product.addToAdvisoryList(advisory);
}

where addToAdvisoryList(Advisory advisory) is:

public void addToAdvisoryList(Advisory advisory) {
    if (advisoryList == null) {
        setAdvisoryList(Collections.singletonList(advisory));
    } else if (!isContainedAdvisory(advisoryList, advisory)) {
        List<Advisory> newAdvisoryList = new ArrayList<>(advisoryList);
        newAdvisoryList.add(advisory);
        setAdvisoryList(newAdvisoryList);
    }
}

After running these code, the original product list is modified. Can someone please explain what exactly happened? and what can be done to avoid modifying an unmodifiable list?

You're not modifying the list, you're modifying the elements in the list. — shmosel, Jul 09 '18 at 19:58
What do you think `new ArrayList<>(report.getProductList())` does? — Pshemo, Jul 09 '18 at 20:08
@shmosel so does this mean that it does not matter if my getter returns an unmodifiableList, since anybody can make a new ArrayList out of it and modify the list elements? How can I prevent people from modifying the elements of the list? — Griford, Jul 09 '18 at 20:10
@Pshemo I thought that it would make a unrelated copy with the same content — Griford, Jul 09 '18 at 20:11
You don't even need to create a new list. There's no relationship between the mutability of a list and its elements. If you want to prevent the elements from being modified, you need to copy them individually. — shmosel, Jul 09 '18 at 20:11
@shmosel does that mean I have return a new list of each elements of the original list cloned? and how come I have never seen other people using getters like this? Isn't the purpose of a getter to return a read only copy of the object? — Griford, Jul 09 '18 at 20:16
Yes, that's what you need to do. It's not uncommon to see defensive deep copies, but it depends on whether the elements are immutable and what exactly you're trying to protect. — shmosel, Jul 09 '18 at 20:18
@Griford Getters are used to get some *property*. For instance if you have `Square` class you can have getter like `double getArea(){ return a*a;}`. As you see such getter is not letting others have access *any* field (there is no `area` field), it simply returns *value* which *represents* some property. — Pshemo, Jul 09 '18 at 20:41
If you have a reference to a mutable object you can modify it, even if that reference is in a collection or object which is not mutable. If you want the objects to be immutable return either an object or an interface which is immutable. — Peter Lawrey, Jul 09 '18 at 20:52

geco17 · Accepted Answer · 2018-07-09T20:34:57.617

In the code you provided, the original (unmodifiable) list is passed as an argument to the constructor of a java.util.ArrayList, a modifiable list.

List<Advisory> newAdvisoryList = new ArrayList<>(advisoryList);

This constructor creates a new list with all the items of the supplied Collection in the order returned by its iterator (see the documentation).

The same thing happens with the list of products:

List<Product> productList = new ArrayList<>(report.getProductList());

This makes it redundant to return unmodifiable lists in the first place.

In other words, you're not modifying unmodifiable lists. You are creating new lists that are modifiable, with the elements from unmodifiable (immutable) lists, and then modifying the (mutable) lists.

Also see here for immutable lists, from the Java documentation. You will get an UnsupportedOperationException when trying to modify an immutable list.

EDIT

To respond to your question about getters:

"Isn't the purpose of a getter to return a read-only copy of the object?"

A getter is used to provide access to a private or protected class member from the outside.

If you have a class like (in your example) Report

public class Report {

    private List<Product> products;

    public void setProducts(List<Product> products) {
        this.products = products;
    }

    public List<Product> getProducts() {
        return products;
    }

}

The getProducts makes the list accessible from outside the class (so you can add or remove elements if it's mutable). This kind of thing is usually done with Java beans, though its use is debated (consider replacing getters and setters with methods that don't unnecessarily expose state information). In any case, see here for more information on getters and setters according to Oracle.

does that mean I have return a new list of each elements of the original list cloned? and how come I have never seen other people using getters like this? Isn't the purpose of a getter to return a read only copy of the object? — Griford, Jul 09 '18 at 20:17
@Griford I updated my answer and provided a link to Oracle's documentation on getters / setters. — geco17, Jul 09 '18 at 20:35

score 0 · Answer 2 · answered Jul 09 '18 at 20:22

Your lists contain mutable Product objects.

You don't modify any of the lists. The lists hold references to the same mutable objects, and you are making changes to these objects. It's basically the same old problem as

To avoid this, you can make getProductList return a list of copies. Alternatively you can make the Product class immutable, which will force you to rethink your approach.

public List<Product> getProductList() {
    List<Product> copies = new ArrayList<>();
    for (Product product: productList)
        copies.add(new Product(product)); // add this constructor

    return copies;
}

How does Collections.unmodifiableList (Java 8 SE) get modified when making a new ArrayList from it?

2 Answers2