-
-
Notifications
You must be signed in to change notification settings - Fork 2.6k
lint: Rule for openInputStreamSafe #19682
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.
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
base: main
Are you sure you want to change the base?
Conversation
|
First PR! 🚀 We sincerely appreciate that you have taken the time to propose a change to AnkiDroid! Please have patience with us as we are all volunteers - we will get to this as soon as possible. |
| ) { | ||
| // Check if this is ContentResolver.openInputStream() | ||
| val evaluator = context.evaluator | ||
| if (!evaluator.isMemberInClass(method, "android.content.ContentResolver")) { |
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.
I don't understand this, can you explain it with a comment?
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.
I still don't understand it
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.
I removed it and lint still passed
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.
Well, I added it just in case someone later adds another class with an openInputStream() method in the codebase. The lint rule won't incorrectly flag it. Should I remove it to make the code more readable?
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.
Ahh. This would be clearer if tested
Index: lint-rules/src/main/java/com/ichi2/anki/lint/rules/OpenInputStreamSafeDetector.kt
IDEA additional info:
Subsystem: com.intellij.openapi.diff.impl.patch.CharsetEP
<+>UTF-8
===================================================================
diff --git a/lint-rules/src/main/java/com/ichi2/anki/lint/rules/OpenInputStreamSafeDetector.kt b/lint-rules/src/main/java/com/ichi2/anki/lint/rules/OpenInputStreamSafeDetector.kt
--- a/lint-rules/src/main/java/com/ichi2/anki/lint/rules/OpenInputStreamSafeDetector.kt (revision 4fe93f1558f3ce22717c9dc5c72973c15ad68fec)
+++ b/lint-rules/src/main/java/com/ichi2/anki/lint/rules/OpenInputStreamSafeDetector.kt (date 1765125080450)
@@ -50,13 +50,8 @@
node: UCallExpression,
method: PsiMethod,
) {
- // Check if this is ContentResolver.openInputStream()
- // The evaluator checks if this method is a member of "android.content.ContentResolver".
- // If it's not (e.g., it's some custom class's openInputStream), we ignore it and return early.
- val evaluator = context.evaluator
- if (!evaluator.isMemberInClass(method, "android.content.ContentResolver")) {
- return
- }
+ // Only warn on ContentResolver.openInputStream()
+ if (!context.evaluator.isMemberInClass(method, "android.content.ContentResolver")) return
context.report(
issue = ISSUE,
lint-rules/src/main/java/com/ichi2/anki/lint/rules/OpenInputStreamSafeDetector.kt
Outdated
Show resolved
Hide resolved
lint-rules/src/main/java/com/ichi2/anki/lint/rules/OpenInputStreamSafeDetector.kt
Outdated
Show resolved
Hide resolved
lint-rules/src/test/java/com/ichi2/anki/lint/rules/OpenInputStreamSafeDetectorTest.kt
Outdated
Show resolved
Hide resolved
lint-rules/src/test/java/com/ichi2/anki/lint/rules/OpenInputStreamSafeDetectorTest.kt
Outdated
Show resolved
Hide resolved
lint-rules/src/test/java/com/ichi2/anki/lint/rules/OpenInputStreamSafeDetectorTest.kt
Outdated
Show resolved
Hide resolved
| contentResolverStub, | ||
| uriStub, |
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.
What is this doing?
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.
These stubs provide fake Android classes for the test environment. Without them, the lint detector wouldn't know what ContentResolver or Uri are, and couldn't check if openInputStream() belongs to the android.content.ContentResolver class.
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 passes
Index: lint-rules/src/test/java/com/ichi2/anki/lint/rules/OpenInputStreamSafeDetectorTest.kt
IDEA additional info:
Subsystem: com.intellij.openapi.diff.impl.patch.CharsetEP
<+>UTF-8
===================================================================
diff --git a/lint-rules/src/test/java/com/ichi2/anki/lint/rules/OpenInputStreamSafeDetectorTest.kt b/lint-rules/src/test/java/com/ichi2/anki/lint/rules/OpenInputStreamSafeDetectorTest.kt
--- a/lint-rules/src/test/java/com/ichi2/anki/lint/rules/OpenInputStreamSafeDetectorTest.kt (revision a7e7533b8af81829e5d940559c77442272219534)
+++ b/lint-rules/src/test/java/com/ichi2/anki/lint/rules/OpenInputStreamSafeDetectorTest.kt (date 1764850400435)
@@ -9,40 +9,12 @@
@RunWith(JUnit4::class)
class OpenInputStreamSafeDetectorTest {
- // Stub files for Android classes needed by the tests
- private val contentResolverStub =
- java(
- """
- package android.content;
-
- import android.net.Uri;
- import java.io.InputStream;
-
- public abstract class ContentResolver {
- public final InputStream openInputStream(Uri uri) {
- return null;
- }
- }
- """,
- ).indented()
-
- private val uriStub =
- java(
- """
- package android.net;
-
- public abstract class Uri {
- }
- """,
- ).indented()
@Test
fun testDirectOpenInputStreamCall() {
lint()
.allowMissingSdk()
.files(
- contentResolverStub,
- uriStub,
kotlin(
"""
class MyClass {
@@ -62,8 +34,6 @@
lint()
.allowMissingSdk()
.files(
- contentResolverStub,
- uriStub,
kotlin(
"""
class MyClass {
@@ -83,8 +53,6 @@
lint()
.allowMissingSdk()
.files(
- contentResolverStub,
- uriStub,
java(
"""
public class MyClass {
@@ -104,8 +72,6 @@
lint()
.allowMissingSdk()
.files(
- contentResolverStub,
- uriStub,
kotlin(
"""
class MyClass {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 is still pending
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.
The tests fail without those lines
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.
Works fine here, how are you testing?
Index: lint-rules/src/test/java/com/ichi2/anki/lint/rules/OpenInputStreamSafeDetectorTest.kt
IDEA additional info:
Subsystem: com.intellij.openapi.diff.impl.patch.CharsetEP
<+>UTF-8
===================================================================
diff --git a/lint-rules/src/test/java/com/ichi2/anki/lint/rules/OpenInputStreamSafeDetectorTest.kt b/lint-rules/src/test/java/com/ichi2/anki/lint/rules/OpenInputStreamSafeDetectorTest.kt
--- a/lint-rules/src/test/java/com/ichi2/anki/lint/rules/OpenInputStreamSafeDetectorTest.kt (revision 5f313280ebc06bb23f1bc3419d6bd0577458147a)
+++ b/lint-rules/src/test/java/com/ichi2/anki/lint/rules/OpenInputStreamSafeDetectorTest.kt (date 1765262787110)
@@ -9,40 +9,12 @@
@RunWith(JUnit4::class)
class OpenInputStreamSafeDetectorTest {
- // Stub files for Android classes needed by the tests
- private val contentResolverStub =
- java(
- """
- package android.content;
-
- import android.net.Uri;
- import java.io.InputStream;
-
- public abstract class ContentResolver {
- public final InputStream openInputStream(Uri uri) {
- return null;
- }
- }
- """,
- ).indented()
-
- private val uriStub =
- java(
- """
- package android.net;
-
- public abstract class Uri {
- }
- """,
- ).indented()
@Test
fun testDirectOpenInputStreamCall() {
lint()
.allowMissingSdk()
.files(
- contentResolverStub,
- uriStub,
kotlin(
"""
class MyClass {
@@ -62,8 +34,6 @@
lint()
.allowMissingSdk()
.files(
- contentResolverStub,
- uriStub,
kotlin(
"""
class MyClass {
@@ -83,8 +53,6 @@
lint()
.allowMissingSdk()
.files(
- contentResolverStub,
- uriStub,
java(
"""
public class MyClass {
dcc1ad5 to
9bc9410
Compare
|
CI is failing. Comments haven't been answered |
9bc9410 to
a7e7533
Compare
a7e7533 to
4fe93f1
Compare
lint-rules/src/main/java/com/ichi2/anki/lint/rules/OpenInputStreamSafeDetector.kt
Outdated
Show resolved
Hide resolved
4fe93f1 to
5f31328
Compare
david-allison
left a comment
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.
The cleanup commits need work, and the merge commit needs removing, but I can do this post-approval.
One improvement with the test, then good to go
Thanks so much!!!!
5f31328 to
69973b7
Compare
Purpose / Description
Add a new Lint rule (OpenInputStreamSafeDetector) that enforces the use of openInputStreamSafe().
Fixes
openInputStreamSafe#19663Approach
Reports the UnsafeOpenInputStream issue and provides a quick-fix to replace the call with openInputStreamSafe().
How Has This Been Tested?
Added unit tests for the same
Checklist
Please, go through these checks before submitting the PR.