Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Load external denylist entries via ServiceLoader #261

Closed
wants to merge 2 commits into from
Closed

Load external denylist entries via ServiceLoader #261

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
a778e23
Allow consumers to supply extra denylisted entries via ServiceLoader
erawhctim Apr 6, 2024
879cd82
Add test case for externally-loaded denylist entries
erawhctim Apr 6, 2024
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
19 changes: 16 additions & 3 deletions slack-lint-checks/src/main/java/slack/lint/denylistedapis/DenyListedApiDetector.kt
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -28,6 +28,7 @@ import com.android.tools.lint.detector.api.XmlScanner
import com.intellij.psi.PsiField
import com.intellij.psi.PsiMethod
import java.util.EnumSet
import java.util.ServiceLoader
import org.jetbrains.uast.UCallExpression
import org.jetbrains.uast.UElement
import org.jetbrains.uast.UExpression
Expand All @@ -54,7 +55,7 @@ internal class DenyListedApiDetector : Detector(), SourceCodeScanner, XmlScanner
override fun visitElement(context: XmlContext, element: Element) =
CONFIG.visitor(context, element)

private class DenyListConfig(vararg entries: DenyListedEntry) {
private class DenyListConfig(entries: Set<DenyListedEntry>) {
private class TypeConfig(entries: List<DenyListedEntry>) {
@Suppress("UNCHECKED_CAST") // Safe because of filter call.
val functionEntries =
Expand Down Expand Up @@ -213,8 +214,8 @@ internal class DenyListedApiDetector : Detector(), SourceCodeScanner, XmlScanner
val DEFAULT_ISSUE = createIssue("DenyListedApi")
val BLOCKING_ISSUE = createIssue("DenyListedBlockingApi")

private val CONFIG =
DenyListConfig(
private val ENTRIES =
setOf(
DenyListedEntry(
className = "io.reactivex.rxjava3.core.Observable",
functionName = "hide",
Expand Down Expand Up @@ -466,6 +467,10 @@ internal class DenyListedApiDetector : Detector(), SourceCodeScanner, XmlScanner
*rxJavaBlockingCalls().toTypedArray(),
)

private val EXTERNAL_ENTRIES = loadExternalEntries()

private val CONFIG = DenyListConfig(ENTRIES + EXTERNAL_ENTRIES)

val ISSUES = CONFIG.issues

private fun createIssue(
Expand All @@ -491,6 +496,14 @@ internal class DenyListedApiDetector : Detector(), SourceCodeScanner, XmlScanner
),
)
}

private fun loadExternalEntries(): Set<DenyListedEntry> {
val loader = ServiceLoader.load(DenyListedEntryLoader::class.java)

return mutableSetOf<DenyListedEntry>().apply {
loader.iterator().forEachRemaining { addAll(it.entries) }
}
Comment on lines +501 to +505
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

You should be able to do something like ServiceLoader.load(...).iterator().toSet()

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

nice, the more concise the better. I'm not seeing an applicable toSet() call to use here, but I might be missing something.

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

maybe .iterator().asSequence().toSet()

}
}
}

Expand Down
5 changes: 5 additions & 0 deletions slack-lint-checks/src/main/java/slack/lint/denylistedapis/DenyListedEntryLoader.kt
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1,5 @@
package slack.lint.denylistedapis

interface DenyListedEntryLoader {
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Would you mind adding an experimental annotation for this? None of this is a stable API anyway, but want to make it doubly-clear here :). Can call it ExperimentalSlackLintApi

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Let's also name this DenyListedEntryProvider and make it have a function instead (i.e. getEntries()) that takes a JavaContext. Then let's lazily load these in the element handler and provide the available context to it, so we can minimize loading (especially if there are entries provided that don't apply in certain contexts).

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

oh yeah sure, good call.

val entries: Set<DenyListedEntry>
}
43 changes: 43 additions & 0 deletions slack-lint-checks/src/test/java/slack/lint/denylistedapis/DenyListedApiDetectorTest.kt
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -700,6 +700,38 @@ class DenyListedApiDetectorTest : BaseSlackLintTest() {
)
}

@Test
fun externalDenylistEntries() {
lint()
.files(
EXTERNAL_ENTRY_TESTCLASS_STUB,
kotlin(
"""
package foo

import slack.lint.TestClass

class SomeClass {
fun test() {
TestClass.run()
}
}
"""
)
.indented()
)
.run()
.expect(
"""
src/foo/SomeClass.kt:7: Error: TestClass.run() is disallowed in tests via external denylist entry. [DenyListedApi]
TestClass.run()
~~~
1 errors, 0 warnings
"""
.trimIndent()
)
}

companion object {
private val FLOWABLE_STUB =
java(
Expand Down Expand Up @@ -1026,5 +1058,16 @@ class DenyListedApiDetectorTest : BaseSlackLintTest() {
"""
)
.indented()

private val EXTERNAL_ENTRY_TESTCLASS_STUB =
kotlin(
"""
package slack.lint

object TestClass {
fun run() {}
}
""".trimIndent()
)
}
}
12 changes: 12 additions & 0 deletions slack-lint-checks/src/test/java/slack/lint/denylistedapis/TestEntriesLoader.kt
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1,12 @@
package slack.lint.denylistedapis

internal class TestEntriesLoader: DenyListedEntryLoader {
override val entries: Set<DenyListedEntry> =
setOf(
DenyListedEntry(
className = "slack.lint.TestClass",
functionName = "run",
errorMessage = "TestClass.run() is disallowed in tests via external denylist entry."
)
)
}
1 change: 1 addition & 0 deletions ...ecks/src/test/resources/META-INF/services/slack.lint.denylistedapis.DenyListedEntryLoader
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1 @@
slack.lint.denylistedapis.TestEntriesLoader
Loading