Skip to content

Enable ConstructorInjectionOverFieldInjectionDetector to be configured when using AppComponentFactory #237

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

Draft
WhosNickDoglio wants to merge 1 commit into
base: main
Choose a base branch
from
Draft

Enable ConstructorInjectionOverFieldInjectionDetector to be configured when using AppComponentFactory #237

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
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
52 changes: 24 additions & 28 deletions ...ava/dev/whosnickdoglio/dagger/detectors/ConstructorInjectionOverFieldInjectionDetector.kt
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -3,6 +3,7 @@
package dev.whosnickdoglio.dagger.detectors

import com.android.tools.lint.client.api.UElementHandler
import com.android.tools.lint.detector.api.BooleanOption
import com.android.tools.lint.detector.api.Category
import com.android.tools.lint.detector.api.Detector
import com.android.tools.lint.detector.api.Implementation
Expand All @@ -28,31 +29,28 @@ internal class ConstructorInjectionOverFieldInjectionDetector : Detector(), Sour
override fun getApplicableUastTypes(): List<Class<out UElement>> =
listOf(UAnnotation::class.java)

override fun createUastHandler(context: JavaContext): UElementHandler {
return object : UElementHandler() {
override fun createUastHandler(context: JavaContext): UElementHandler =
object : UElementHandler() {
override fun visitAnnotation(node: UAnnotation) {
if (node.qualifiedName == INJECT) {
val annotatedElement = node.uastParent as? UField ?: return
// val fullAllowList: List<String> =
// if (context.mainProject.minSdk >= MIN_SDK) {
//
// allowList.defaultValue?.split(",").orEmpty()
// } else {
//
// allowList.defaultValue?.split(",").orEmpty() + androidComponents
// }
val fullAllowList: List<String> =
if (usingAppComponentFactory.getValue(context)) {
allowList.getValue(context)?.split(",").orEmpty()
} else {
allowList.getValue(context)?.split(",").orEmpty() + androidComponents
}

val containingClass =
(annotatedElement.javaPsi as? PsiField)?.containingClass ?: return
val isAllowedFieldInjection = androidComponents.any { className ->
val isAllowedFieldInjection = fullAllowList.any { className ->
context.evaluator.extendsClass(
cls = containingClass,
className = className,
strict = true,
)
}

// minSdkLessThan(28)
if (!isAllowedFieldInjection) {
context.report(
Incident(
Expand All @@ -66,13 +64,8 @@ internal class ConstructorInjectionOverFieldInjectionDetector : Detector(), Sour
}
}
}
}

companion object {
// private const val MIN_SDK = 28

// TODO make this configurable
@Suppress("UnusedPrivateProperty")
private val allowList =
StringOption(
name = "allowList",
Expand All @@ -81,8 +74,10 @@ internal class ConstructorInjectionOverFieldInjectionDetector : Detector(), Sour
explanation = "",
)

//
private val androidComponents =
internal val usingAppComponentFactory =
BooleanOption(name = "usingAppComponentFactory", description = "TOOD", explanation = "")

internal val androidComponents =
setOf(
// https://developer.android.com/reference/android/app/AppComponentFactory
"android.app.Activity",
Expand All @@ -103,18 +98,19 @@ internal class ConstructorInjectionOverFieldInjectionDetector : Detector(), Sour

internal val ISSUE =
Issue.create(
id = "ConstructorOverField",
briefDescription = "Class is using field injection over constructor injection",
explanation =
"""
id = "ConstructorOverField",
briefDescription = "Class is using field injection over constructor injection",
explanation =
"""
Constructor injection should be favored over field injection for classes that support it.

See https://whosnickdoglio.dev/dagger-rules/rules/#prefer-constructor-injection-over-field-injection for more information.
""",
category = Category.CORRECTNESS,
priority = 5,
severity = Severity.WARNING,
implementation = implementation,
)
category = Category.CORRECTNESS,
priority = 5,
severity = Severity.WARNING,
implementation = implementation,
)
.setOptions(listOf(usingAppComponentFactory, allowList))
}
}
124 changes: 124 additions & 0 deletions ...dev/whosnickdoglio/dagger/detectors/ConstructorInjectionOverFieldInjectionDetectorTest.kt
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -184,6 +184,130 @@ class ConstructorInjectionOverFieldInjectionDetectorTest {
.expectWarningCount(1)
}

@Test
fun `android subclass in kotlin does triggers member injection warning when usingAppComponentFactory is enabled`() {
TestLintTask.lint()
.files(
javaxAnnotations,
TestFiles.kotlin(
"""
package ${component.classPackage}

class ${component.className}

"""
)
.indented(),
TestFiles.kotlin(
"""
package androidx

import ${component.classImport}

class AndroidX${component.className}: ${component.className}
"""
)
.indented(),
TestFiles.kotlin(
"""
package com.test.android

import javax.inject.Inject
import androidx.AndroidX${component.className}

class Something

class My${component.className}: AndroidX${component.className} {

@Inject lateinit var something: Something

}
"""
)
.indented(),
)
.issues(ConstructorInjectionOverFieldInjectionDetector.ISSUE)
.configureOption(
ConstructorInjectionOverFieldInjectionDetector.usingAppComponentFactory,
true,
)
.run()
.expect(
"""
src/com/test/android/Something.kt:10: Warning: Constructor injection should be favored over field injection for classes that support it.

See https://whosnickdoglio.dev/dagger-rules/rules/#prefer-constructor-injection-over-field-injection for more information. [ConstructorOverField]
@Inject lateinit var something: Something
~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
0 errors, 1 warnings
"""
.trimIndent()
)
.expectWarningCount(1)
}

@Test
fun `android subclass in java does triggers member injection warning when usingAppComponentFactory is enabled`() {
TestLintTask.lint()
.files(
javaxAnnotations,
TestFiles.java(
"""
package ${component.classPackage};

class ${component.className} {}

"""
)
.indented(),
TestFiles.java(
"""
package androidx;

import ${component.classImport};

class AndroidX${component.className} extends ${component.className} {}
"""
)
.indented(),
TestFiles.java(
"""
package com.test.android;

import javax.inject.Inject;
import androidx.AndroidX${component.className};

class Something {}

class My${component.className} extends AndroidX${component.className} {

@Inject Something something;

}
"""
)
.indented(),
)
.issues(ConstructorInjectionOverFieldInjectionDetector.ISSUE)
.configureOption(
ConstructorInjectionOverFieldInjectionDetector.usingAppComponentFactory,
true,
)
.run()
.expect(
"""
src/com/test/android/Something.java:10: Warning: Constructor injection should be favored over field injection for classes that support it.

See https://whosnickdoglio.dev/dagger-rules/rules/#prefer-constructor-injection-over-field-injection for more information. [ConstructorOverField]
@Inject Something something;
~~~~~~~~~~~~~~~~~~~~~~~~~~~~
0 errors, 1 warnings
"""
.trimIndent()
)
.expectWarningCount(1)
}

@Suppress("unused")
enum class AndroidComponentParameters(
val className: String,
Expand Down
Loading