Does it lead to data corruption when we access an outside object in a Java 8 map function?

Question

I have object customerSummary at line #2 and accessing it at lines #11 & #12. Does it lead to data corruption in production?

private CustomerSummary enrichCustomerIdentifiers(CustomerSummaryDTO customerSummaryDTO) {
    CustomerSummary customerSummary = customerSummaryDTO.getCustomerSummary();

    List<CustomerIdentifier> customerIdentifiers = customerSummary
            .getCustomerIdentifiers().stream()
            .peek(customerIdentifier -> {
                if (getCustomerReferenceTypes().contains(customerIdentifier.getIdentifierType())) {
                    customerIdentifier.setRefType(RefType.REF.toString());
                } else {
                    customerIdentifier.setRefType(RefType.TAX.toString());
                    Country country = new Country();
                    country.setIsoCountryCode(customerSummary.getCustomerAddresses().get(0).getIsoCountryCode());
                    country.setCountryName(customerSummary.getCustomerAddresses().get(0).getCountryName());
                    customerIdentifier.setCountry(country);
                }
            }).collect(Collectors.toList());
    customerSummary.setCustomerIdentifiers(customerIdentifiers);

    return customerSummary;
}

Answer 1

The direct answer to your question is a resounding 'no', but you're misusing streams, which presumably is part of why you are even asking this question. You're operating on mutables in stream code, which you shouldn't be doing: It's why I'm saying 'misusing' - this code compiles and works but leads to hard to read and had to maintain code that will fail in weird ways as you use more and more of the stream API. The solution is not to go against the grain so much.

You're also engaging in stringly based typing which is another style mistake.

Finally, your collect call is misleading.

So, to answer the question:

Does it lead to data corruption in production?

No. How would you imagine it would?

Style mistake #1: mutables

Streams don't work nearly as well when you're working with mutables. The general idea is that you have immutable classes (classes without any setters; the instances of these classes cannot change after construction. String is immutable, so is Integer, and so is BigDecimal. There is no .setValue() on an integer instance, there is no setChar() on a string, or even a clear() or an append() - all operations on immutables that appear to modify things actually return a new instance that contains the result of the operation. someBigDecimal.add() doesn't change what someBigDecimal is pointing at; it constructs a new bigDecimal instance and returns that.

With immutables, if you want to change things, Stream's map method is the right one to use: For example, if you have a stream of BigDecimal objects and you want to, say, print them all, but with 2.5 added to them, you'd be calling map: You want to map each input BigDecimal into an output BD by asking the BD instance to make a new BD instance by adding 2.5 to itself.

With mutables, both map and peek are more relevant. Style debates are rife on what to do. peek just lets you witness what's going through a stream pipeline. It can be misleading because stream pipelines dont process anything until you stick a terminator on the end (something like collect, or max() or whatnot, those are 'terminators'). When talking about mutables, peek in theory works just as well as map does and some (evidently, including intellij's auto-suggest authors) are of the belief that a map operation that really just mutates the underlying object in the stream and returns the same reference is a style violation and should be replaced with a peek operation instead.

But the far more relevant observation is that stream operations should not be mutating anything at all. Do not call setters.

You have 2 options:

1. Massively refactor this code, make CustomIdentifier immutable (get rid of the setters, make all fields final, consider adding with-ers and builders and the like), change your peek code to something like:

.map(identifier -> {
    if (....) return customerIdentifier.with(RefType.REF);
    return identifier.withCountry(new Country(summary.get..., summary.get...));
})

Note that Country also needs this treatment.

2. Do not use streams.

This is much simpler. This code is vastly less confusing and better style if you just write a foreach loop. I have no idea why you thought streams were appropriate here. Streams are not 'better'. A problem is that adherents of functional style are so incredibly convinced they are correct they spread copious FUD (Fear, Uncertainty, Doubt) about non-functional approaches and strongly insinuate that functional style is 'just better'. This is not true - it's merely a different style that is more suitable to some domains and less to others. This style goes a lot further than just 'turn for loops into streams', and unawareness of what 'functional style' really means just leads to hard to maintain, hard to read, weird code like what you pasted.

I really, really want to use streams here

This is just a bad idea here (unless you do the full rewrite to immutables), but if you MUST, the actual right answer is not what intellij said, it's to use forEach. This is peek and the terminal in one package. It gets rid of the pointless collect (which just recreates a list that is 100% identical to what customerSummary.getCustomerIdentifiers() returns) call and properly represents what is actually happening (which is NOT that you're writing code that witnesses what is flowing through the stream pipe, you're writing code that you intend to execute on each element in the stream).

But that's still much worse than this:

CustomerSummary summary = customerSummaryDTO.getCustomerSummary();
for (CustomerIdentifier identifier : summary.getCustomerIdentifiers()) {
    if (getCustomerReferenceTypes().contains(customerIdentifier.getIdentifierType())) {
        customerIdentifier.setRefType(RefType.REF.toString());
    } else {
        customerIdentifier.setRefType(RefType.TAX.toString());
        Country country = new Country();
        country.setIsoCountryCode(customerSummary.getCustomerAddresses().get(0).getIsoCountryCode());
        country.setCountryName(customerSummary.getCustomerAddresses().get(0).getCountryName());
        customerIdentifier.setCountry(country);
    }
}

return customerSummary;

Style mistake #2: stringly typing

Why isn't the refType field in CustomerIdentifier just RefType? Why are you converting RefType instances to strings and back?

DB engines support enums and if they don't, the in-between layer (your DTO) should support marshalling enums into strings and back.

Answer 2

The literal answer to your question is No ... assuming that the access is thread-safe.

---

But your code probably doesn't do what you think it does.

The peek() method returns the precise stream of objects that it is called on. So your code is effectively doing this:

 summary.setCustomerIdentifiers(
        new SomeListClass<>(summary.getCustomerIdentifiers()));

... while doing some operations on the identifier objects.

You are (AFAIK unnecessarily) copying the list and reassigning it to the field of the summary object.

---

It would be simpler AND more efficient to write it as:

for (CustomerIdentifier id: summary.getCustomerIdentifiers()) {
    if (getCustomerReferenceTypes().contains(id.getIdentifierType())) {
        id.setRefType(RefType.REF.toString());
    } else {
        id.setRefType(RefType.TAX.toString());
        Country country = new Country();
        Address address = summary.getCustomerAddresses().get(0);
        country.setIsoCountryCode(address.getIsoCountryCode());
        country.setCountryName(address.getCountryName());
        id.setCountry(country);
    }
}

You could do the above using a list.stream().forEach(), or a list.forEach(), but the code is (IMO) neither simpler or substantially more concise than a plain loop.

summary.getCustomerIdentifiers().forEach(
    id -> {
        if (getCustomerReferenceTypes().contains(id.getIdentifierType())) {
            id.setRefType(RefType.REF.toString());
        } else {
            id.setRefType(RefType.TAX.toString());
            Country country = new Country();
            Address address = summary.getCustomerAddresses().get(0);
            country.setIsoCountryCode(address.getIsoCountryCode());
            country.setCountryName(address.getCountryName());
            id.setCountry(country);
        }
    }
);

(A final micro-optimization would be to declare and initialize address outside of the loop.)

Java 8 streams are not the solution to all problems.