Add JooqResultStreamLeak ErrorProne rule

README.md

@@ -179,6 +179,7 @@ Safe Logging can be found at [github.com/palantir/safe-logging](https://github.c
 - `StrictCollectionIncompatibleType`: Likely programming error due to using the wrong type in a method that accepts Object.
 - `InvocationHandlerDelegation`: InvocationHandlers which delegate to another object must catch and unwrap InvocationTargetException.
 - `Slf4jThrowable`: Slf4j loggers require throwables to be the last parameter otherwise a stack trace is not produced.
+- `JooqResultStreamLeak`: Autocloseable streams and cursors from jOOQ results should be obtained in a try-with-resources statement.
 
 ### Programmatic Application
 

baseline-error-prone/build.gradle

@@ -18,6 +18,7 @@ dependencies {
     testCompile 'org.apache.commons:commons-lang3'
     testCompile 'commons-lang:commons-lang'
     testCompile 'org.assertj:assertj-core'
+    testCompile 'org.jooq:jooq'
     testImplementation 'org.junit.jupiter:junit-jupiter'
     testRuntimeOnly 'org.junit.jupiter:junit-jupiter-migrationsupport'
 

baseline-error-prone/src/main/java/com/palantir/baseline/errorprone/JooqResultStreamLeak.java

@@ -0,0 +1,78 @@
+/*
+ * (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.VisitorState;
+import com.google.errorprone.bugpatterns.BugChecker;
+import com.google.errorprone.bugpatterns.StreamResourceLeak;
+import com.google.errorprone.matchers.Description;
+import com.google.errorprone.matchers.Matcher;
+import com.google.errorprone.matchers.method.MethodMatchers;
+import com.google.errorprone.util.ASTHelpers;
+import com.sun.source.tree.ExpressionTree;
+import com.sun.source.tree.MethodInvocationTree;
+import com.sun.tools.javac.code.Type;
+
+@AutoService(BugChecker.class)
+@BugPattern(
+        name = "JooqResultStreamLeak",
+        link = "https://github.com/palantir/gradle-baseline#baseline-error-prone-checks",
+        linkType = BugPattern.LinkType.CUSTOM,
+        providesFix = BugPattern.ProvidesFix.REQUIRES_HUMAN_ATTENTION,
+        severity = BugPattern.SeverityLevel.WARNING,
+        summary = "Methods that return an autocloseable resource on jOOQ's ResultQuery should be closed using "
+                + "try-with-resources. Not doing so can result in leaked database resources (such as connections or "
+                + "cursors) in code paths that throw an exception or fail to call #close().")
+public final class JooqResultStreamLeak extends StreamResourceLeak {
+    private static final Matcher<ExpressionTree> MATCHER =
+            MethodMatchers.instanceMethod()
+                .onDescendantOf("org.jooq.ResultQuery")
+                .withAnyName();
+
+    @Override
+    public Description matchMethodInvocation(
+            MethodInvocationTree tree, VisitorState state) {
+        if (!MATCHER.matches(tree, state)) {
+            return Description.NO_MATCH;
+        }
+
+        if (!shouldBeAutoClosed(tree, state)) {
+            return Description.NO_MATCH;
+        }
+
+        return matchNewClassOrMethodInvocation(tree, state);
+    }
+
+    private static boolean shouldBeAutoClosed(MethodInvocationTree tree, VisitorState state) {
+        Type returnType = ASTHelpers.getReturnType(tree);
+
+        // Most auto-closeable returns should be auto-closed.
+        boolean isAutoCloseable = ASTHelpers.isSubtype(returnType,
+                state.getTypeFromString(AutoCloseable.class.getName()),
+                state);
+
+        // QueryParts can hold resources but usually don't, so auto-tripping and trying to fix on things
+        // like SelectConditionStep is unnecessary.
+        boolean isJooqQuery = ASTHelpers.isSubtype(returnType,
+                state.getTypeFromString("org.jooq.QueryPart"),
+                state);
+
+        return isAutoCloseable && !isJooqQuery;
+    }
+}

baseline-error-prone/src/test/java/com/palantir/baseline/errorprone/JooqResultStreamLeakTest.java

@@ -0,0 +1,144 @@
+/*
+ * (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.errorprone.CompilationTestHelper;
+import org.junit.Test;
+import org.junit.jupiter.api.parallel.Execution;
+import org.junit.jupiter.api.parallel.ExecutionMode;
+
+@Execution(ExecutionMode.CONCURRENT)
+public final class JooqResultStreamLeakTest {
+
+    private final CompilationTestHelper testHelper =
+            CompilationTestHelper.newInstance(JooqResultStreamLeak.class, getClass());
+
+    private final RefactoringValidator refactoringValidator =
+            RefactoringValidator.of(new JooqResultStreamLeak(), getClass());
+
+    @Test
+    public void test_positive() {
+        testHelper.addSourceLines("JooqStream.java",
+            "import java.util.stream.Collectors;",
+            "import java.util.stream.Stream;",
+            "import org.jooq.Record;",
+            "import org.jooq.ResultQuery;",
+            "class Test {",
+            "  void f(ResultQuery<Record> rq) {",
+            "    // BUG: Diagnostic contains: should be closed",
+            "    rq.stream().map(r -> r.getValue(0));",
+            "    // BUG: Diagnostic contains: should be closed",
+            "    rq.fetchStream().map(r -> r.getValue(0));",
+            "    // BUG: Diagnostic contains: should be closed",
+            "    rq.fetchLazy();",
+            "    try (Stream<String> stream = rq.stream().collect(Collectors.toList()).stream()) {",
+            "      stream.collect(Collectors.joining(\", \"));",
+            "    }",
+            "  }",
+            "}")
+            .doTest();
+    }
+
+    @Test
+    public void test_query_steps_ignored() {
+        testHelper.addSourceLines("JooqStream.java",
+                "import java.util.stream.Collectors;",
+                "import java.util.stream.Stream;",
+                "import org.jooq.Record;",
+                "import org.jooq.SelectConditionStep;",
+                "class Test {",
+                "  SelectConditionStep<Record> f(SelectConditionStep<Record> select) {",
+                "    return select.and(\"some_field <> 3\");",
+                "  }",
+                "}")
+                .doTest();
+    }
+
+    @Test
+    public void test_negative() {
+        testHelper.addSourceLines("JooqStream.java",
+            "import java.util.stream.Stream;",
+            "import org.jooq.Record;",
+            "import org.jooq.ResultQuery;",
+            "class Test {",
+            "  void f(ResultQuery rq) {",
+            "    try (Stream<Record> stream = rq.stream()) {",
+            "      Stream<Field<?>> newStream = stream.map(r -> r.getValue(0));",
+            "    }",
+            "    try (Stream<Record> stream = rq.fetchStream()) {",
+            "      Stream<Field<?>> newStream = stream.map(r -> r.getValue(0));",
+            "    }",
+            "  }",
+            "}")
+            .doTest();
+    }
+
+    @Test
+    public void test_fix() {
+        refactoringValidator
+                .addInputLines("in/JooqStream.java",
+                    "import java.util.stream.Stream;",
+                    "import org.jooq.Record;",
+                    "import org.jooq.ResultQuery;",
+                    "class Test {",
+                    "  void f(ResultQuery<Record> rq) {",
+                    "    rq.stream().map(r -> r.getValue(0));",
+                    "  }",
+                    "}")
+            .addOutputLines("out/JooqStream.java",
+                    "import java.util.stream.Stream;",
+                    "import org.jooq.Record;",
+                    "import org.jooq.ResultQuery;",
+                    "class Test {",
+                    "  void f(ResultQuery<Record> rq) {",
+                    "    try (Stream<Record> stream = rq.stream()) {",
+                    "      stream.map(r -> r.getValue(0));",
+                    "    }",
+                    "  }",
+                    "}")
+            .doTest();
+    }
+
+    @Test
+    public void test_fix_variable() {
+        refactoringValidator
+                .addInputLines("in/JooqStream.java",
+                        "import java.util.stream.Collectors;",
+                        "import java.util.stream.Stream;",
+                        "import org.jooq.Record;",
+                        "import org.jooq.ResultQuery;",
+                        "class Test {",
+                        "  void f(ResultQuery<Record> rq) {",
+                        "    String res = rq.stream().map(r -> r.toString()).collect(Collectors.joining(\", \"));",
+                        "  }",
+                        "}")
+                .addOutputLines("out/JooqStream.java",
+                        "import java.util.stream.Collectors;",
+                        "import java.util.stream.Stream;",
+                        "import org.jooq.Record;",
+                        "import org.jooq.ResultQuery;",
+                        "class Test {",
+                        "  void f(ResultQuery<Record> rq) {",
+                        "    String res;",
+                        "    try (Stream<Record> stream = rq.stream()) {",
+                        "      res = stream.map(r -> r.toString()).collect(Collectors.joining(\", \"));",
+                        "    }",
+                        "  }",
+                        "}")
+                .doTest();
+    }
+}

changelog/@unreleased/pr-1055.v2.yml

@@ -0,0 +1,7 @@
+type: improvement
+improvement:
+  description: Adds an ErrorProne rule, `JooqResultStreamLeak`, which ensures that
+    result streams and cursors returned from jOOQ results are closed in a try-with-resources
+    block.
+  links:
+  - https://github.com/palantir/gradle-baseline/pull/1055

versions.props

@@ -12,6 +12,7 @@ com.palantir.safe-logging:* = 1.12.0
 org.apache.maven.shared:maven-dependency-analyzer = 1.11.1
 org.github.ngbinh.scalastyle:gradle-scalastyle-plugin_2.11 = 1.0.1
 org.inferred:freebuilder = 1.14.6
+org.jooq:jooq = 3.11.12
 org.slf4j:slf4j-api = 1.7.25
 
 # test deps