From 8a5be782383727d6c703e1a1ed6ed657c509e35a Mon Sep 17 00:00:00 2001 From: a-k-g Date: Wed, 8 Mar 2023 19:59:02 +0530 Subject: [PATCH] Disallow usage of .collapseKeys() in EntryStream(s) (#2291) Added `DangerousCollapseKeysUsage` error prone check to disallow usage of `collapseKeys()` API of `EntryStream`. --- README.md | 1 + .../DangerousCollapseKeysUsage.java | 60 +++++++++++++ .../DangerousCollapseKeysUsageTest.java | 85 +++++++++++++++++++ changelog/@unreleased/pr-2291.v2.yml | 6 ++ 4 files changed, 152 insertions(+) create mode 100644 baseline-error-prone/src/main/java/com/palantir/baseline/errorprone/DangerousCollapseKeysUsage.java create mode 100644 baseline-error-prone/src/test/java/com/palantir/baseline/errorprone/DangerousCollapseKeysUsageTest.java create mode 100644 changelog/@unreleased/pr-2291.v2.yml diff --git a/README.md b/README.md index 90b9d8ea7..373c99ec2 100644 --- a/README.md +++ b/README.md @@ -213,6 +213,7 @@ Safe Logging can be found at [github.com/palantir/safe-logging](https://github.c - `ConsistentOverrides`: Ensure values are bound to the correct variables when overriding methods - `FilterOutputStreamSlowMultibyteWrite`: Subclasses of FilterOutputStream should provide a more efficient implementation of `write(byte[], int, int)` to avoid slow writes. - `BugCheckerAutoService`: Concrete BugChecker implementations should be annotated `@AutoService(BugChecker.class)` for auto registration with error-prone. +- `DangerousCollapseKeysUsage`: Disallow usage of `EntryStream#collapseKeys()`. ### Programmatic Application diff --git a/baseline-error-prone/src/main/java/com/palantir/baseline/errorprone/DangerousCollapseKeysUsage.java b/baseline-error-prone/src/main/java/com/palantir/baseline/errorprone/DangerousCollapseKeysUsage.java new file mode 100644 index 000000000..c0b6bf957 --- /dev/null +++ b/baseline-error-prone/src/main/java/com/palantir/baseline/errorprone/DangerousCollapseKeysUsage.java @@ -0,0 +1,60 @@ +/* + * (c) Copyright 2019 Palantir Technologies Inc. All rights reserved. + * + * Licensed under the Apache License, Version 2.0 (the "License"); + * you may not use this file except in compliance with the License. + * You may obtain a copy of the License at + * + * http://www.apache.org/licenses/LICENSE-2.0 + * + * Unless required by applicable law or agreed to in writing, software + * distributed under the License is distributed on an "AS IS" BASIS, + * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. + * See the License for the specific language governing permissions and + * limitations under the License. + */ + +package com.palantir.baseline.errorprone; + +import com.google.auto.service.AutoService; +import com.google.errorprone.BugPattern; +import com.google.errorprone.BugPattern.SeverityLevel; +import com.google.errorprone.VisitorState; +import com.google.errorprone.bugpatterns.BugChecker; +import com.google.errorprone.matchers.Description; +import com.google.errorprone.matchers.Matcher; +import com.google.errorprone.matchers.method.MethodMatchers; +import com.sun.source.tree.ExpressionTree; +import com.sun.source.tree.MethodInvocationTree; + +@AutoService(BugChecker.class) +@BugPattern( + link = "https://github.com/palantir/gradle-baseline#baseline-error-prone-checks", + linkType = BugPattern.LinkType.CUSTOM, + severity = SeverityLevel.ERROR, + summary = "Disallow usage of .collapseKeys() in EntryStream(s).") +public final class DangerousCollapseKeysUsage extends BugChecker implements BugChecker.MethodInvocationTreeMatcher { + private static final long serialVersionUID = 1L; + private static final String ERROR_MESSAGE = "The collapseKeys API of EntryStream must be avoided. 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.\n" + + "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.\nHence in place of collapseKeys() we recommend using " + + "grouping operations which may require creation of intermediate maps but should avoid surprising code."; + + private static final Matcher COLLAPSE_KEYS_CALL = MethodMatchers.instanceMethod() + .onExactClass("one.util.streamex.EntryStream") + .named("collapseKeys"); + + @Override + public Description matchMethodInvocation(MethodInvocationTree tree, VisitorState state) { + if (!COLLAPSE_KEYS_CALL.matches(tree, state)) { + return Description.NO_MATCH; + } + + // Fail on any 'parallel(...)' implementation, regardless of how many parameters it takes + return buildDescription(tree).setMessage(ERROR_MESSAGE).build(); + } +} diff --git a/baseline-error-prone/src/test/java/com/palantir/baseline/errorprone/DangerousCollapseKeysUsageTest.java b/baseline-error-prone/src/test/java/com/palantir/baseline/errorprone/DangerousCollapseKeysUsageTest.java new file mode 100644 index 000000000..a3a14fa3d --- /dev/null +++ b/baseline-error-prone/src/test/java/com/palantir/baseline/errorprone/DangerousCollapseKeysUsageTest.java @@ -0,0 +1,85 @@ +/* + * (c) Copyright 2022 Palantir Technologies Inc. All rights reserved. + * + * Licensed under the Apache License, Version 2.0 (the "License"); + * you may not use this file except in compliance with the License. + * You may obtain a copy of the License at + * + * http://www.apache.org/licenses/LICENSE-2.0 + * + * Unless required by applicable law or agreed to in writing, software + * distributed under the License is distributed on an "AS IS" BASIS, + * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. + * See the License for the specific language governing permissions and + * limitations under the License. + */ + +package com.palantir.baseline.errorprone; + +import com.google.errorprone.CompilationTestHelper; +import org.junit.jupiter.api.BeforeEach; +import org.junit.jupiter.api.Test; + +class DangerousCollapseKeysUsageTest { + private CompilationTestHelper compilationHelper; + + @BeforeEach + public void before() { + compilationHelper = CompilationTestHelper.newInstance(DangerousCollapseKeysUsage.class, getClass()); + } + + @Test + public void should_error_when_collapse_keys_with_collector_is_used() { + compilationHelper + .addSourceLines( + "Test.java", + "import java.util.List;", + "import java.util.Map;", + "import java.util.Set;", + "import java.util.stream.Collectors;", + "import one.util.streamex.EntryStream;", + "import org.apache.commons.lang3.tuple.Pair;", + "class Test {", + " public static final void main(String[] args) {", + " List> nodesAndPriorities = List.of(", + " Pair.of(1, 5),", + " Pair.of(1, 3),", + " Pair.of(4, 2),", + " Pair.of(1, 9));", + " Map> nodesByPriority = EntryStream.of(nodesAndPriorities)", + " .mapValues(Pair::getRight)", + " // BUG: Diagnostic contains: collapseKeys API of EntryStream must be avoided", + " .collapseKeys(Collectors.toSet())", + " .toMap();", + " }", + "}") + .doTest(); + } + + @Test + public void should_error_when_collapse_keys_is_used_even_without_collector() { + compilationHelper + .addSourceLines( + "Test.java", + "import java.util.List;", + "import java.util.Map;", + "import java.util.stream.Collectors;", + "import one.util.streamex.EntryStream;", + "import org.apache.commons.lang3.tuple.Pair;", + "class Test {", + " public static final void main(String[] args) {", + " List> nodesAndPriorities = List.of(", + " Pair.of(1, 5),", + " Pair.of(1, 3),", + " Pair.of(4, 2),", + " Pair.of(1, 9));", + " Map> nodesByPriority = EntryStream.of(nodesAndPriorities)", + " .mapValues(Pair::getRight)", + " // BUG: Diagnostic contains: collapseKeys API of EntryStream must be avoided", + " .collapseKeys()", + " .toMap();", + " }", + "}") + .doTest(); + } +} diff --git a/changelog/@unreleased/pr-2291.v2.yml b/changelog/@unreleased/pr-2291.v2.yml new file mode 100644 index 000000000..6e895ac6c --- /dev/null +++ b/changelog/@unreleased/pr-2291.v2.yml @@ -0,0 +1,6 @@ +type: feature +feature: + description: Added `DangerousCollapseKeysUsage` error prone check to disallow usage + of `collapseKeys()` API of `EntryStream`. + links: + - https://github.com/palantir/gradle-baseline/pull/2291