From 667d9f95b42cbee73dcb12cbae33b4d2c9f6e94b Mon Sep 17 00:00:00 2001 From: Aakash Goenka Date: Mon, 6 Jun 2022 18:28:59 +0100 Subject: [PATCH 1/4] Disallow usage of .collapseKeys() in EntryStream(s) --- README.md | 1 + .../DangerousCollapseKeysUsage.java | 60 +++++++++++++ .../DangerousCollapseKeysUsageTest.java | 85 +++++++++++++++++++ 3 files changed, 146 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 diff --git a/README.md b/README.md index e1dd72637..32dd8cf2b 100644 --- a/README.md +++ b/README.md @@ -211,6 +211,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..41fe83f3a --- /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 should 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..2eaca7874 --- /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 should 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 should be avoided", + " .collapseKeys()", + " .toMap();", + " }", + "}") + .doTest(); + } +} From 5fe9d2e170c493a98a783ae94738a68997b50d03 Mon Sep 17 00:00:00 2001 From: svc-changelog Date: Mon, 6 Jun 2022 17:48:18 +0000 Subject: [PATCH 2/4] Add generated changelog entries --- changelog/@unreleased/pr-2291.v2.yml | 6 ++++++ 1 file changed, 6 insertions(+) create mode 100644 changelog/@unreleased/pr-2291.v2.yml 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 From b80cbd54370f22c17d5edbc3f9a797cb6bbd9953 Mon Sep 17 00:00:00 2001 From: a-k-g Date: Mon, 4 Jul 2022 15:48:46 +0100 Subject: [PATCH 3/4] Update DangerousCollapseKeysUsage.java --- .../baseline/errorprone/DangerousCollapseKeysUsage.java | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) 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 index 41fe83f3a..c0b6bf957 100644 --- 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 @@ -35,7 +35,7 @@ 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 should be avoided. The " + 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" From a4597ae42ef4882d9cfb9b11bad48d6c5cf2d7ec Mon Sep 17 00:00:00 2001 From: a-k-g Date: Mon, 4 Jul 2022 15:56:46 +0100 Subject: [PATCH 4/4] Update DangerousCollapseKeysUsageTest.java --- .../baseline/errorprone/DangerousCollapseKeysUsageTest.java | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) 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 index 2eaca7874..a3a14fa3d 100644 --- 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 @@ -48,7 +48,7 @@ public void should_error_when_collapse_keys_with_collector_is_used() { " Pair.of(1, 9));", " Map> nodesByPriority = EntryStream.of(nodesAndPriorities)", " .mapValues(Pair::getRight)", - " // BUG: Diagnostic contains: collapseKeys API of EntryStream should be avoided", + " // BUG: Diagnostic contains: collapseKeys API of EntryStream must be avoided", " .collapseKeys(Collectors.toSet())", " .toMap();", " }", @@ -75,7 +75,7 @@ public void should_error_when_collapse_keys_is_used_even_without_collector() { " Pair.of(1, 9));", " Map> nodesByPriority = EntryStream.of(nodesAndPriorities)", " .mapValues(Pair::getRight)", - " // BUG: Diagnostic contains: collapseKeys API of EntryStream should be avoided", + " // BUG: Diagnostic contains: collapseKeys API of EntryStream must be avoided", " .collapseKeys()", " .toMap();", " }",