Disallow usage of .collapseKeys() in EntryStream(s)

[a-k-g]

Before this PR

The collapseKeys() API of EntryStream is dangerous and should be avoided. In internal products the API is frequently used as a grouping operation but its not suitable for that use case. The contract requires duplicate keys to be adjacent to each other in the stream, which is rarely the case in production code paths. When this constraint is violated, it leads to a duplicate key error at runtime.

A work around for the issue is to sort the keys prior to running the collapse operation. Since the sort operation is surprising, a comment is often added to explain. Overall the usage of collapseKeys() leads to code that is error prone or surprising.

Hence in place of collapseKeys() we should just use grouping operations which may require creation of intermediate maps but should avoid surprising code.

Notional example of buggy code:

List<Pair<Integer, Integer>> nodesAndPriorities = List.of
        Pair.of(1, 5),
        Pair.of(1, 3),
        Pair.of(4, 2),
        Pair.of(1, 9));

Map<Integer, Set<Integer>> nodesByPriority = EntryStream.of(nodesAndPriorities)
        .mapValues(Pair::getRight)
        .collapseKeys(Collectors.toSet())
        .toMap();

Notional example of applying sort work around:

Map<Integer, Set<Integer>> nodesByPriority = EntryStream.of(nodesAndPriorities)
        .mapValues(Pair::getRight)
        // collapseKeys() requires duplicate keys to be adjacent
        .sorted()
        .collapseKeys(Collectors.toSet())
        .toMap();

Notional example with proposed alternative of grouping:

Map<Integer, Set<Integer>> nodesByPriority = EntryStream.of(nodesAndPriorities)
        .mapValues(Pair::getRight)
        .grouping(Collectors.toSet());

After this PR

==COMMIT_MSG==
Added DangerousCollapseKeysUsage error prone check to disallow usage of collapseKeys() API of EntryStream.
==COMMIT_MSG==

Possible downsides?

Will need manual action for existing usages.

[changelog-app]

Generate changelog in changelog/@unreleased

Type

• Feature
• Improvement
• Fix
• Break
• Deprecation
• Manual task
• Migration

Description

Added DangerousCollapseKeysUsage error prone check to disallow usage of collapseKeys() API of EntryStream.

Check the box to generate changelog(s)

• Generate changelog entry

[carterkozak]

"should" implies info/warning, not SeverityLevel.ERROR.

[a-k-g]

Would you suggest changing the error level, or the language? I'm inclined to change this to The collapseKeys API of EntryStream must be avoided.

[carterkozak]

I'd prefer setting this to WARNING to reduce friction of rollout. I might suggest adding a check for a sort called prior to this method, to avoid flagging cases that are safe.

[a-k-g]

I've removed most of the internal usage so rollout should be friction free and we can keep it at ERROR. I've changed the should to must.

[carterkozak]

collapseKeys, not parallel

[a-k-g]

Ah, fixing in #2510.

[svc-autorelease]

Released 4.190.0