This repository was archived by the owner on Aug 17, 2020. It is now read-only.
-
Notifications
You must be signed in to change notification settings - Fork 412
Question marks in a query don't match number of arguments lint check #220
Merged
Merged
Changes from all commits
Commits
Show all changes
9 commits
Select commit
Hold shift + click to select a range
c4f9c3f
Question marks in a query don't match number of arguments lint check
geralt-encore d1a3999
Fix PR comments
geralt-encore a9439c9
Fix more PR comments
geralt-encore 3b88397
Remove unnecessary detector call in tests
geralt-encore 244a9b7
Fix comments
geralt-encore 2b506b3
Fix tests
geralt-encore 37be373
Fix star import
geralt-encore 26c4ae5
Fix comment
geralt-encore 8a7ac86
Remove lint checks dependency
geralt-encore File filter
Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
There are no files selected for viewing
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -1,5 +1,6 @@ | ||
| include ':sqlbrite' | ||
| include ':sqlbrite-kotlin' | ||
| include ':sqlbrite-lint' | ||
| include ':sample' | ||
|
|
||
| rootProject.name = 'sqlbrite-root' |
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,16 @@ | ||
| apply plugin: 'kotlin' | ||
|
|
||
| dependencies { | ||
| compileOnly rootProject.ext.kotlinStdLib | ||
| compileOnly rootProject.ext.lintApi | ||
|
|
||
| testImplementation rootProject.ext.junit | ||
| testImplementation rootProject.ext.lint | ||
| testImplementation rootProject.ext.lintTests | ||
| } | ||
|
|
||
| jar { | ||
| manifest { | ||
| attributes("Lint-Registry-v2": "com.squareup.sqlbrite3.BriteIssueRegistry") | ||
| } | ||
| } | ||
23 changes: 23 additions & 0 deletions
23
sqlbrite-lint/src/main/java/com/squareup/sqlbrite3/BriteIssueRegistry.kt
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,23 @@ | ||
| /* | ||
| * Copyright (C) 2017 Square, Inc. | ||
| * | ||
| * 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.squareup.sqlbrite3 | ||
|
|
||
| import com.android.tools.lint.client.api.IssueRegistry | ||
|
|
||
| class BriteIssueRegistry : IssueRegistry() { | ||
|
|
||
| override fun getIssues() = listOf(SqlBriteArgCountDetector.ISSUE) | ||
| } |
79 changes: 79 additions & 0 deletions
79
sqlbrite-lint/src/main/java/com/squareup/sqlbrite3/SqlBriteArgCountDetector.kt
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,79 @@ | ||
| /* | ||
| * Copyright (C) 2017 Square, Inc. | ||
| * | ||
| * 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.squareup.sqlbrite3 | ||
|
|
||
| import com.android.tools.lint.detector.api.Category | ||
| import com.android.tools.lint.detector.api.ConstantEvaluator.evaluateString | ||
| import com.android.tools.lint.detector.api.Detector | ||
| import com.android.tools.lint.detector.api.Implementation | ||
| import com.android.tools.lint.detector.api.Issue | ||
| import com.android.tools.lint.detector.api.JavaContext | ||
| import com.android.tools.lint.detector.api.Scope.JAVA_FILE | ||
| import com.android.tools.lint.detector.api.Scope.TEST_SOURCES | ||
| import com.android.tools.lint.detector.api.Severity | ||
| import com.intellij.psi.PsiMethod | ||
| import org.jetbrains.uast.UCallExpression | ||
| import java.util.EnumSet | ||
|
|
||
| private const val BRITE_DATABASE = "com.squareup.sqlbrite3.BriteDatabase" | ||
| private const val QUERY_METHOD_NAME = "query" | ||
| private const val CREATE_QUERY_METHOD_NAME = "createQuery" | ||
|
|
||
| class SqlBriteArgCountDetector : Detector(), Detector.UastScanner { | ||
|
|
||
| companion object { | ||
|
|
||
| val ISSUE: Issue = Issue.create( | ||
| "SqlBriteArgCount", | ||
| "Number of provided arguments doesn't match number " + | ||
| "of arguments specified in query", | ||
| "When providing arguments to query you need to provide the same amount of " + | ||
| "arguments that is specified in query.", | ||
| Category.MESSAGES, | ||
| 9, | ||
| Severity.ERROR, | ||
| Implementation(SqlBriteArgCountDetector::class.java, EnumSet.of(JAVA_FILE, TEST_SOURCES))) | ||
| } | ||
|
|
||
| override fun getApplicableMethodNames() = listOf(CREATE_QUERY_METHOD_NAME, QUERY_METHOD_NAME) | ||
|
|
||
| override fun visitMethod(context: JavaContext, call: UCallExpression, method: PsiMethod) { | ||
| val evaluator = context.evaluator | ||
|
|
||
| if (evaluator.isMemberInClass(method, BRITE_DATABASE)) { | ||
| // Skip non varargs overloads. | ||
| if (!method.isVarArgs) return | ||
|
|
||
| // Position of sql parameter depends on method. | ||
| val sql = evaluateString(context, | ||
| call.valueArguments[if (call.isQueryMethod()) 0 else 1], true) ?: return | ||
|
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. nice |
||
|
|
||
| // Count only vararg arguments. | ||
| val argumentsCount = call.valueArgumentCount - if (call.isQueryMethod()) 1 else 2 | ||
| val questionMarksCount = sql.count { it == '?' } | ||
| if (argumentsCount != questionMarksCount) { | ||
| val requiredArguments = "$questionMarksCount ${"argument".pluralize(questionMarksCount)}" | ||
| val actualArguments = "$argumentsCount ${"argument".pluralize(argumentsCount)}" | ||
| context.report(ISSUE, call, context.getLocation(call), "Wrong argument count, " + | ||
| "query $sql requires $requiredArguments, but was provided $actualArguments") | ||
| } | ||
| } | ||
| } | ||
|
|
||
| private fun UCallExpression.isQueryMethod() = methodName == QUERY_METHOD_NAME | ||
|
|
||
| private fun String.pluralize(count: Int) = if (count == 1) this else this + "s" | ||
|
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. There's already this function in LintUtils.pluralize but unfortuantely it's private :( we could request it to be made public |
||
| } | ||
207 changes: 207 additions & 0 deletions
207
sqlbrite-lint/src/test/java/com/squareup/sqlbrite3/SqlBriteArgCountDetectorTest.kt
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,207 @@ | ||
| /* | ||
| * Copyright (C) 2017 Square, Inc. | ||
| * | ||
| * 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.squareup.sqlbrite3 | ||
|
|
||
| import com.android.tools.lint.checks.infrastructure.TestFiles.java | ||
| import com.android.tools.lint.checks.infrastructure.TestLintTask.lint | ||
| import org.junit.Test | ||
|
|
||
| class SqlBriteArgCountDetectorTest { | ||
|
|
||
| companion object { | ||
| private val BRITE_DATABASE_STUB = java( | ||
| """ | ||
| package com.squareup.sqlbrite3; | ||
|
|
||
| public final class BriteDatabase { | ||
|
|
||
| public void query(String sql, Object... args) { | ||
| } | ||
|
|
||
| public void createQuery(String table, String sql, Object... args) { | ||
| } | ||
|
|
||
| // simulate createQuery with SupportSQLiteQuery query parameter | ||
| public void createQuery(String table, int something) { | ||
| } | ||
| } | ||
| """.trimIndent() | ||
| ) | ||
| } | ||
|
|
||
| @Test | ||
| fun cleanCaseWithWithQueryAsLiteral() { | ||
| lint().files( | ||
| BRITE_DATABASE_STUB, | ||
| java( | ||
| """ | ||
| package test.pkg; | ||
|
|
||
| import com.squareup.sqlbrite3.BriteDatabase; | ||
|
|
||
| public class Test { | ||
| private static final String QUERY = "SELECT name FROM table WHERE id = ?"; | ||
|
|
||
| public void test() { | ||
| BriteDatabase db = new BriteDatabase(); | ||
| db.query(QUERY, "id"); | ||
| } | ||
|
|
||
| } | ||
| """.trimIndent())) | ||
| .issues(SqlBriteArgCountDetector.ISSUE) | ||
| .run() | ||
| .expectClean() | ||
| } | ||
|
|
||
| @Test | ||
| fun cleanCaseWithQueryAsBinaryExpression() { | ||
| lint().files( | ||
| BRITE_DATABASE_STUB, | ||
| java( | ||
| """ | ||
| package test.pkg; | ||
|
|
||
| import com.squareup.sqlbrite3.BriteDatabase; | ||
|
|
||
| public class Test { | ||
| private static final String QUERY = "SELECT name FROM table WHERE "; | ||
|
|
||
| public void test() { | ||
| BriteDatabase db = new BriteDatabase(); | ||
| db.query(QUERY + "id = ?", "id"); | ||
| } | ||
|
|
||
| } | ||
| """.trimIndent())) | ||
| .issues(SqlBriteArgCountDetector.ISSUE) | ||
| .run() | ||
| .expectClean() | ||
| } | ||
|
|
||
| @Test | ||
| fun cleanCaseWithQueryThatCantBeEvaluated() { | ||
| lint().files( | ||
| BRITE_DATABASE_STUB, | ||
| java( | ||
| """ | ||
| package test.pkg; | ||
|
|
||
| import com.squareup.sqlbrite3.BriteDatabase; | ||
|
|
||
| public class Test { | ||
| private static final String QUERY = "SELECT name FROM table WHERE id = ?"; | ||
|
|
||
| public void test() { | ||
| BriteDatabase db = new BriteDatabase(); | ||
| db.query(query(), "id"); | ||
| } | ||
|
|
||
| private String query() { | ||
| return QUERY + " age = ?"; | ||
| } | ||
|
|
||
| } | ||
| """.trimIndent())) | ||
| .issues(SqlBriteArgCountDetector.ISSUE) | ||
| .run() | ||
| .expectClean() | ||
| } | ||
|
|
||
| @Test | ||
| fun cleanCaseWithNonVarargMethodCall() { | ||
| lint().files( | ||
| BRITE_DATABASE_STUB, | ||
| java( | ||
| """ | ||
| package test.pkg; | ||
|
|
||
| import com.squareup.sqlbrite3.BriteDatabase; | ||
|
|
||
| public class Test { | ||
|
|
||
| public void test() { | ||
| BriteDatabase db = new BriteDatabase(); | ||
| db.createQuery("table", 42); | ||
| } | ||
|
|
||
| } | ||
| """.trimIndent())) | ||
| .issues(SqlBriteArgCountDetector.ISSUE) | ||
| .run() | ||
| .expectClean() | ||
| } | ||
|
|
||
| @Test | ||
| fun queryMethodWithWrongNumberOfArguments() { | ||
| lint().files( | ||
| BRITE_DATABASE_STUB, | ||
| java( | ||
| """ | ||
| package test.pkg; | ||
|
|
||
| import com.squareup.sqlbrite3.BriteDatabase; | ||
|
|
||
| public class Test { | ||
| private static final String QUERY = "SELECT name FROM table WHERE id = ?"; | ||
|
|
||
| public void test() { | ||
| BriteDatabase db = new BriteDatabase(); | ||
| db.query(QUERY); | ||
| } | ||
|
|
||
| } | ||
| """.trimIndent())) | ||
| .issues(SqlBriteArgCountDetector.ISSUE) | ||
| .run() | ||
| .expect("src/test/pkg/Test.java:10: " + | ||
| "Error: Wrong argument count, query SELECT name FROM table WHERE id = ?" + | ||
| " requires 1 argument, but was provided 0 arguments [SqlBriteArgCount]\n" + | ||
| " db.query(QUERY);\n" + | ||
| " ~~~~~~~~~~~~~~~\n" + | ||
| "1 errors, 0 warnings") | ||
| } | ||
|
|
||
| @Test | ||
| fun createQueryMethodWithWrongNumberOfArguments() { | ||
| lint().files( | ||
| BRITE_DATABASE_STUB, | ||
| java( | ||
| """ | ||
| package test.pkg; | ||
|
|
||
| import com.squareup.sqlbrite3.BriteDatabase; | ||
|
|
||
| public class Test { | ||
| private static final String QUERY = "SELECT name FROM table WHERE id = ?"; | ||
|
|
||
| public void test() { | ||
| BriteDatabase db = new BriteDatabase(); | ||
| db.createQuery("table", QUERY); | ||
| } | ||
|
|
||
| } | ||
| """.trimIndent())) | ||
| .issues(SqlBriteArgCountDetector.ISSUE) | ||
| .run() | ||
| .expect("src/test/pkg/Test.java:10: " + | ||
| "Error: Wrong argument count, query SELECT name FROM table WHERE id = ?" + | ||
| " requires 1 argument, but was provided 0 arguments [SqlBriteArgCount]\n" + | ||
| " db.createQuery(\"table\", QUERY);\n" + | ||
| " ~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~\n" + | ||
| "1 errors, 0 warnings") | ||
| } | ||
| } |
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This should be
implementationsince, in theory, future versions of lint could not be based on Kotlin and thus not have the stdlib already on the classpath.There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
If I mark it as
implementationinstead ofcompileOnlyI have this error:There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
That's... interesting. I mean it makes sense, but I guess we have to assume that lint will always contain the Kotlin stdlib as a result.