gradle-baseline icon indicating copy to clipboard operation
gradle-baseline copied to clipboard

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

Open a-k-g opened this issue 3 years ago • 1 comments

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.

a-k-g avatar Jun 06 '22 17:06 a-k-g

Generate changelog in changelog/@unreleased

Type

  • [x] 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)

  • [x] Generate changelog entry

changelog-app[bot] avatar Jun 06 '22 17:06 changelog-app[bot]

Released 4.190.0

svc-autorelease avatar Mar 08 '23 14:03 svc-autorelease