SonarQube: "Store a copy of "productAllergenInfos""

Question

I have the following DTO:

@Data
@RequiredArgsConstructor
public class MenuItemExpandedDTO {
private UUID uuid;
private List<ModifierGroupDTO> modifierGroupDtoList;
private List<AllergenInfo> allergenInfoList;

public MenuItemExpandedDTO(
        PropertiesDTO propertiesDto,
        List<ModifierGroupDTO> modifierGroupDtoList,
        List<AllergenInfo> allergenInfoList
) {
    this.uuid = propertiesDto.getUuid();
    this.modifierGroupDtoList = modifierGroupDtoList;
    this.allergenInfoList = allergenInfoList;
  }
}

In SonarQube analysis, I get a Vulnerability due to allergenInfoList as it is stated

"Message: Store a copy of allergenInfoList"

So, I am not sure what the problem is, but before fixing this error, I am wondering what is wrong with that code? In some pages, it is recommended to initialize the list e.g. private List<AllergenInfo> allergenInfoList = Collections.emptyList(). But it is not a way I follow in my projects. So, what is the problem with this code?

Sonarqube should give you a reference to the rule, providing more detail. This one may be https://rules.sonarsource.com/java/RSPEC-2384 — tgdavies, Oct 05 '21 at 22:41

João Dias · Accepted Answer · 2021-10-05T22:30:01.470

SonarQube is telling you to be cautious when receiving Lists in a constructor. Why? Because the caller holds a reference to that List and it can do the following with it if it is not immutable:

Change the List content by adding or removing elements to it, actually affecting your MenuItemExpandedDTO.
Change the objects contained in the List if they are not immutable. This means that AllergenInfo objects in the List can be changed affecting your MenuItemExpandedDTO object.

To tackle 1., you can simply store a copy of the List as SonarQube suggests:

public MenuItemExpandedDTO(
        PropertiesDTO propertiesDto,
        List<ModifierGroupDTO> modifierGroupDtoList,
        List<AllergenInfo> allergenInfoList
) {
    this.uuid = propertiesDto.getUuid();
    this.modifierGroupDtoList = new ArrayList<>(modifierGroupDtoList);
    this.allergenInfoList = new ArrayList<>(allergenInfoList);
  }
}

Tackling 2. is trickier and the simplest and more reliable solution is to use immutable objects. You can read more about this and how to design your classes so that you have immutable objects at https://www.baeldung.com/java-immutable-object.

public class MenuItemExpandedDTO {
    private final UUID uuid;
    private final List<ModifierGroupDTO> modifierGroupDtoList;
    private final List<AllergenInfo> allergenInfoList;

    public MenuItemExpandedDTO(PropertiesDTO propertiesDto,
                               List<ModifierGroupDTO> modifierGroupDtoList,
                               List<AllergenInfo> allergenInfoList) {
        this.uuid = propertiesDto.getUuid();
        this.modifierGroupDtoList = new ArrayList<>(modifierGroupDtoList);
        this.allergenInfoList = new ArrayList<>(allergenInfoList);
    }

    public UUID getUuid() {
        return UUID;
    }

    public List<ModifierGroupDTO> getModifierGroupDtoList() {
        return new ArrayList<>(modifierGroupDtoList);
    }

    public List<AllergenInfo> getAllergenInfoList() {
        return new ArrayList<>(allergenInfoList);
    }
}

Keep in mind that ModifierGroupDTO and AllergenInfo must also be immutable so that MenuItemExpandedDTO is 100% immutable.

**1.** Thanks a lot for your good explanations. I have never used such a kind of usage e.g. `this.modifierGroupDtoList = new ArrayList<>(modifierGroupDtoList)`, I generaly use `this.modifierGroupDtoList = new ArrayList<>()`. So, what is the meaning of `this.modifierGroupDtoList = new ArrayList<>(modifierGroupDtoList)`? I think it is meaning that, assign `modifierGroupDtoList` to `this.modifierGroupDtoList`. Is that true? — Jack, Oct 04 '21 at 21:05
Would you suggest Tackling 2 over Tackling 1 as mentioned on https://www.baeldung.com/java-immutable-object ? — Jack, Oct 04 '21 at 21:07
It assigns a new `List` containing the content of `modifierGroupDtoList` to `this.modifierGroupDtoList`, so that you have a different list than the one provided (only the list, not the objects inside it). This means that you have to tackle both 1. and 2. if you want to be 100% sure that no one else messes with the `List`. — João Dias, Oct 04 '21 at 23:21
Thanks a lot, voted up. Could you also provide an example by using the code in my answer to show **Tackling 2**? — Jack, Oct 05 '21 at 05:21

SonarQube: "Store a copy of "productAllergenInfos""

1 Answers1