Modernize codebase with Java improvements - Use modern Java collections API (toList() instead of Collectors.toList())

[Pankraz76]

Modernize codebase with Java improvements - Use modern Java collections API (toList() instead of Collectors.toList())

• Modernize codebase with Java improvements #2277
• [chore] Modernize codebase with Java improvements - Replace custom null checks with Objects.requireNonNull #2290
• AvoidOutdatedUsage: Use modern Java collections API (toList() instead of collect(Collectors.toList())) checkstyle/checkstyle#17000

[Pankraz76]

issue:

[gnodet]

Please only apply to [root]/api, [root]/impl, but not [api]/compat.

[Pankraz76]

Please only apply to [root]/api, [root]/impl

done.

reverted its, as well.

[elharo]

This is not a drop in replacement. This changes a lot of modifiable lists to immutable lists. In some cases here this is clearly fine — e.g. when all that''s done next is findFirst — but others aren't so obvious. This is really, really hard to debug when it goes wrong. I would limit this PR to cases where the lsist does not escape the method or perhaps the class.

[Pankraz76]

This is not a drop in replacement. This changes a lot of modifiable lists to immutable lists. In some cases here this is clearly fine — e.g. when all that''s done next is findFirst — but others aren't so obvious. This is really, really hard to debug when it goes wrong. I would limit this PR to cases where the lsist does not escape the method or perhaps the class.

Immutable Objects and Thread Safety

If someone wants to deviate from the happy path, they should work with a copy of final and secure state - and by happy with that.

Core Principles:

1. Final/immutable by Default:
   Everything should be final/immutable unless there's a compelling reason not to be.

2. External Manipulation:
   If external modification is absolutely necessary:

   • The requester should receive a defensive copy
   • All modifications must go through our controlled interface
   • We maintain the immutable original as the source of truth

Important Considerations:

• Strict Data Control:
  Every modifiable reference is a potential deviation from our secure, immutable approach.
  We don't allow external code to directly manipulate our internal state.

• Side Effect Prevention:
  Non-final references can lead to unpredictable side effects and thread safety issues.

Reference:

Immutable Objects - Thread Safety

Implementation Strategy:

For production systems:

1. Audit to identify genuine needs for mutability
2. Create exception pathways with proper safeguards:
   • Defensive copying
   • Controlled mutation interfaces
3. Always preserve the immutable source
4. Document all deviation points rigorously

[Pankraz76]

in Checkstyle toUnmodifiableList is enforced - so it seems possible and just one abstraction layer away from having final and secure state in theory.

[elharo]

You're not working in a green field. You can't change the behavior of APIs and assume existing clients will adapt.

[Pankraz76]

You're not working in a green field. You can't change the behavior of APIs and assume existing clients will adapt.

Yes.

Which clients are affected?

When will they get in touch? Do they live inside our JVM — at runtime or even compile-time — like within this code repo's scope?
If so, we should be able to test and adapt accordingly.

If not — for example, clients using the Maven wrapper — they simply execute the .jar file and don’t interact with the API in a way where immutability would matter.

The only case I can imagine is another method using the current API and directly manipulating the result.
In that case, wrapping the result in a simple new ArrayList<>(...) is enough to make the list mutable again — that should do the trick.

Whether we return a mutable list or they create one themselves doesn’t really matter.

The scenario where code relies on mutating a shared reference doesn’t apply here, since toList() already returns a new list instance.
Passing around mutable references would be questionable design and could violate data integrity and the getter/setter principle.

The core change here is introducing immutability — which is a best practice and supports defensive programming.
We also benefit from cleaner code through method argument validation, which is a welcome side effect.

So overall, the change is meaningful in terms of thread safety and data integrity — it's justified.
We just need to align the code accordingly — reverting potential issues or bridging them by explicitly creating a new list instance when needed.

See also: Create mutable list from array - Stack Overflow

Ideally, failing tests that throw UnsupportedOperationException will help identify any breaking points.

https://docs.oracle.com/javase/9/core/creating-immutable-lists-sets-and-maps.htm#JSCOR-GUID-4F3E2B7D-CE90-4862-A78A-414FC08DA6E4

[elharo]

Sorry, no, I have been burned too badly by this change before. It is not OK to change an existing public method from returning a mutable list to an immutable list. It does break things. I have wasted too many hours of my life tracking down problems caused by an unexpected change from mutable to immutable to be OK with this.

[Pankraz76]

of course agree on this.

Then need to make this step by step.

[Pankraz76]

extract internal.

[Pankraz76]

this is api

[Pankraz76]

okay, internal.

[Pankraz76]

Only doing the migration step by step to ensure quick fix, its possible.

As mentioned any change could unpleasant have side effects.