From d36cbed5a99f0157215136ca31b9d427c84512d9 Mon Sep 17 00:00:00 2001 From: Steven Berler Date: Tue, 7 Mar 2023 15:27:35 -0800 Subject: [PATCH 1/3] add JooqBatchWithoutBindArgs check --- README.md | 1 + .../errorprone/JooqBatchWithoutBindArgs.java | 82 +++++++++++++ .../JooqBatchWithoutBindArgsTest.java | 108 ++++++++++++++++++ changelog/@unreleased/pr-2506.v2.yml | 5 + 4 files changed, 196 insertions(+) create mode 100644 baseline-error-prone/src/main/java/com/palantir/baseline/errorprone/JooqBatchWithoutBindArgs.java create mode 100644 baseline-error-prone/src/test/java/com/palantir/baseline/errorprone/JooqBatchWithoutBindArgsTest.java create mode 100644 changelog/@unreleased/pr-2506.v2.yml diff --git a/README.md b/README.md index 373c99ec2..9978f3c2d 100644 --- a/README.md +++ b/README.md @@ -214,6 +214,7 @@ Safe Logging can be found at [github.com/palantir/safe-logging](https://github.c - `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()`. +- `JooqBatchWithoutBindArgs`: Disallow jOOQ batch methods that execute queries without bind args. ### Programmatic Application diff --git a/baseline-error-prone/src/main/java/com/palantir/baseline/errorprone/JooqBatchWithoutBindArgs.java b/baseline-error-prone/src/main/java/com/palantir/baseline/errorprone/JooqBatchWithoutBindArgs.java new file mode 100644 index 000000000..de886a3df --- /dev/null +++ b/baseline-error-prone/src/main/java/com/palantir/baseline/errorprone/JooqBatchWithoutBindArgs.java @@ -0,0 +1,82 @@ +/* + * (c) Copyright 2023 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.common.collect.ImmutableList; +import com.google.errorprone.BugPattern; +import com.google.errorprone.BugPattern.SeverityLevel; +import com.google.errorprone.BugPattern.StandardTags; +import com.google.errorprone.VisitorState; +import com.google.errorprone.bugpatterns.BugChecker; +import com.google.errorprone.bugpatterns.BugChecker.MethodInvocationTreeMatcher; +import com.google.errorprone.matchers.Description; +import com.google.errorprone.matchers.Matcher; +import com.google.errorprone.matchers.Matchers; +import com.google.errorprone.matchers.method.MethodMatchers; +import com.google.errorprone.suppliers.Supplier; +import com.google.errorprone.suppliers.Suppliers; +import com.sun.source.tree.ExpressionTree; +import com.sun.source.tree.MethodInvocationTree; +import com.sun.tools.javac.code.Type; +import java.util.Collection; + +@AutoService(BugChecker.class) +@BugPattern( + link = "https://github.com/palantir/gradle-baseline#baseline-error-prone-checks", + linkType = BugPattern.LinkType.CUSTOM, + severity = SeverityLevel.ERROR, + tags = StandardTags.PERFORMANCE, + summary = "Disallow jOOQ batch methods that execute queries without bind args.") +public final class JooqBatchWithoutBindArgs extends BugChecker implements MethodInvocationTreeMatcher { + + private static final long serialVersionUID = 1L; + + private static final String DSL_CONTEXT = "org.jooq.DSLContext"; + private static final String BATCH = "batch"; + private static final String ERROR_MESSAGE = + "Jooq batch methods that execute queries without bind args cause performance problems."; + + private static final Supplier QUERY_TYPE = + VisitorState.memoize(state -> state.getTypeFromString("org.jooq.Query")); + + private static final Matcher BATCH_WITHOUT_BINDS_MATCHER = Matchers.anyOf( + MethodMatchers.instanceMethod() + .onDescendantOf(DSL_CONTEXT) + .named(BATCH) + .withParameters("org.jooq.Queries"), + MethodMatchers.instanceMethod() + .onDescendantOf(DSL_CONTEXT) + .named(BATCH) + .withParameters(Collection.class.getName()), + MethodMatchers.instanceMethod() + .onDescendantOf(DSL_CONTEXT) + .named(BATCH) + .withParametersOfType(ImmutableList.of(Suppliers.arrayOf(Suppliers.STRING_TYPE))), + MethodMatchers.instanceMethod() + .onDescendantOf(DSL_CONTEXT) + .named(BATCH) + .withParametersOfType(ImmutableList.of(Suppliers.arrayOf(QUERY_TYPE)))); + + @Override + public Description matchMethodInvocation(MethodInvocationTree tree, VisitorState state) { + if (BATCH_WITHOUT_BINDS_MATCHER.matches(tree, state)) { + return buildDescription(tree).setMessage(ERROR_MESSAGE).build(); + } + return Description.NO_MATCH; + } +} diff --git a/baseline-error-prone/src/test/java/com/palantir/baseline/errorprone/JooqBatchWithoutBindArgsTest.java b/baseline-error-prone/src/test/java/com/palantir/baseline/errorprone/JooqBatchWithoutBindArgsTest.java new file mode 100644 index 000000000..92f36c53f --- /dev/null +++ b/baseline-error-prone/src/test/java/com/palantir/baseline/errorprone/JooqBatchWithoutBindArgsTest.java @@ -0,0 +1,108 @@ +/* + * (c) Copyright 2023 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.Test; + +/** + * See {@code org.jooq.DSLContext#batch} docs to see which ones use bind args. + */ +public final class JooqBatchWithoutBindArgsTest { + + private void testFail(String batchArgs) { + test(batchArgs, true); + } + + private void testPass(String batchArgs) { + test(batchArgs, false); + } + + private void test(String batchArgs, boolean fail) { + CompilationTestHelper.newInstance(JooqBatchWithoutBindArgs.class, getClass()) + .addSourceLines( + "Test.java", + "import org.jooq.DSLContext;", + "import org.jooq.Queries;", + "import org.jooq.Query;", + "import org.jooq.Table;", + "import org.jooq.Record;", + "import org.jooq.Field;", + "import org.jooq.impl.DSL;", + "import java.util.ArrayList;", + "", + "class Test {", + "", + " static final ArrayList QUERY_LIST = new ArrayList<>();", + " static final Queries QUERIES = DSL.queries(QUERY_LIST);", + "", + " void f(DSLContext ctx, Table table, Field intField) {", + fail ? " // BUG: Diagnostic contains: Jooq batch methods that execute queries without" : "", + " ctx.batch(" + batchArgs + ").execute();", + " }", + "}") + .doTest(); + } + + @Test + public void testPassSingleQueryString() { + // batch(String) + testPass("\"DELETE FROM table WHERE id = ?\""); + } + + @Test + public void testFailStringArray() { + // batch(String...) + testFail("\"DELETE FROM table WHERE id = 1\", \"DELETE FROM table WHERE id = 2\""); + } + + @Test + public void testPassStringWithBindsArray() { + // batch(String, Object[]...) + testPass("\"DELETE FROM table WHERE id = ?\", new Object[][]{{1}, {2}, {3}}"); + } + + @Test + public void testFailQueryList() { + // batch(Collection) + testFail("QUERY_LIST"); + } + + @Test + public void testFailQueries() { + // batch(Queries) + testFail("QUERIES"); + } + + @Test + public void testPassSingleQuery() { + // batch(Query) + testPass("ctx.deleteFrom(table).where(intField.eq((Integer) null))"); + } + + @Test + public void testFailQueryArray() { + // batch(Query...) + testFail("ctx.deleteFrom(table).where(intField.eq(1)), ctx.selectFrom(table).where(intField.eq(2))"); + } + + @Test + public void testPassQueryWithBindsArray() { + // batch(Query, Object[]...) + testPass("ctx.deleteFrom(table).where(intField.eq((Integer) null)), new Object[][]{{1}, {2}, {3}}"); + } +} diff --git a/changelog/@unreleased/pr-2506.v2.yml b/changelog/@unreleased/pr-2506.v2.yml new file mode 100644 index 000000000..b37aad579 --- /dev/null +++ b/changelog/@unreleased/pr-2506.v2.yml @@ -0,0 +1,5 @@ +type: feature +feature: + description: Add error-prone check JooqBatchWithoutBindArgs + links: + - https://github.com/palantir/gradle-baseline/pull/2506 From ff3bdc8a280f85ade995ebbbf79c973d9eea2f9a Mon Sep 17 00:00:00 2001 From: Steven Berler Date: Tue, 14 Mar 2023 14:45:07 -0700 Subject: [PATCH 2/3] update with pr suggestions --- .../errorprone/JooqBatchWithoutBindArgs.java | 15 ++++++++++----- .../errorprone/JooqBatchWithoutBindArgsTest.java | 2 +- 2 files changed, 11 insertions(+), 6 deletions(-) diff --git a/baseline-error-prone/src/main/java/com/palantir/baseline/errorprone/JooqBatchWithoutBindArgs.java b/baseline-error-prone/src/main/java/com/palantir/baseline/errorprone/JooqBatchWithoutBindArgs.java index de886a3df..5239e1d6e 100644 --- a/baseline-error-prone/src/main/java/com/palantir/baseline/errorprone/JooqBatchWithoutBindArgs.java +++ b/baseline-error-prone/src/main/java/com/palantir/baseline/errorprone/JooqBatchWithoutBindArgs.java @@ -39,17 +39,22 @@ @BugPattern( link = "https://github.com/palantir/gradle-baseline#baseline-error-prone-checks", linkType = BugPattern.LinkType.CUSTOM, - severity = SeverityLevel.ERROR, + severity = SeverityLevel.WARNING, tags = StandardTags.PERFORMANCE, - summary = "Disallow jOOQ batch methods that execute queries without bind args.") + summary = "jOOQ batch methods that execute without bind args can cause performance problems.", + explanation = + "When batch queries execute without bind args, each query is sent to the database as a string with all" + + " variables inline. Inline variables cause each query in the batch to be unique, so the database" + + " uses extra CPU and memory to parse and query plan each query. Instead use one of the other" + + " jOOQ batch methods that is documented as executing queries with bind args, as this allows" + + " parsing and query planning the query once and then executing any number of times with" + + " different bind values.") public final class JooqBatchWithoutBindArgs extends BugChecker implements MethodInvocationTreeMatcher { private static final long serialVersionUID = 1L; private static final String DSL_CONTEXT = "org.jooq.DSLContext"; private static final String BATCH = "batch"; - private static final String ERROR_MESSAGE = - "Jooq batch methods that execute queries without bind args cause performance problems."; private static final Supplier QUERY_TYPE = VisitorState.memoize(state -> state.getTypeFromString("org.jooq.Query")); @@ -75,7 +80,7 @@ public final class JooqBatchWithoutBindArgs extends BugChecker implements Method @Override public Description matchMethodInvocation(MethodInvocationTree tree, VisitorState state) { if (BATCH_WITHOUT_BINDS_MATCHER.matches(tree, state)) { - return buildDescription(tree).setMessage(ERROR_MESSAGE).build(); + return describeMatch(tree); } return Description.NO_MATCH; } diff --git a/baseline-error-prone/src/test/java/com/palantir/baseline/errorprone/JooqBatchWithoutBindArgsTest.java b/baseline-error-prone/src/test/java/com/palantir/baseline/errorprone/JooqBatchWithoutBindArgsTest.java index 92f36c53f..0c60ddfc1 100644 --- a/baseline-error-prone/src/test/java/com/palantir/baseline/errorprone/JooqBatchWithoutBindArgsTest.java +++ b/baseline-error-prone/src/test/java/com/palantir/baseline/errorprone/JooqBatchWithoutBindArgsTest.java @@ -51,7 +51,7 @@ private void test(String batchArgs, boolean fail) { " static final Queries QUERIES = DSL.queries(QUERY_LIST);", "", " void f(DSLContext ctx, Table table, Field intField) {", - fail ? " // BUG: Diagnostic contains: Jooq batch methods that execute queries without" : "", + fail ? " // BUG: Diagnostic contains: jOOQ batch methods that execute without bind" : "", " ctx.batch(" + batchArgs + ").execute();", " }", "}") From 96d21c241845ec3d6463d5d1c61b2b6aa6c1101d Mon Sep 17 00:00:00 2001 From: Steven Berler Date: Tue, 14 Mar 2023 14:46:32 -0700 Subject: [PATCH 3/3] update readme --- README.md | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/README.md b/README.md index 9978f3c2d..d9eb59693 100644 --- a/README.md +++ b/README.md @@ -214,7 +214,7 @@ Safe Logging can be found at [github.com/palantir/safe-logging](https://github.c - `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()`. -- `JooqBatchWithoutBindArgs`: Disallow jOOQ batch methods that execute queries without bind args. +- `JooqBatchWithoutBindArgs`: jOOQ batch methods that execute without bind args can cause performance problems. ### Programmatic Application