Skip to content

feat: detect and report all Android resource references in Manifest files #1450

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

Merged
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
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: 7 additions & 12 deletions src/main/kotlin/com/autonomousapps/internal/ManifestParser.kt
Original file line number Diff line number Diff line change
Expand Up @@ -3,10 +3,11 @@
package com.autonomousapps.internal

import com.autonomousapps.internal.utils.buildDocument
import com.autonomousapps.internal.utils.document.attrs
import com.autonomousapps.internal.utils.document.mapToSet
import com.autonomousapps.model.internal.AndroidResSource
import org.w3c.dom.Document
import org.w3c.dom.Element
import org.w3c.dom.Node
import java.io.File

internal class ManifestParser(
Expand All @@ -18,8 +19,8 @@ internal class ManifestParser(
val packageName: String,
/** The value of the `android:name` attribute. May be empty. */
val applicationName: String,
/** The values of the `android:theme` attributes of application and activities. May be empty. */
val themes: Set<String>,
Copy link
Contributor Author

Choose a reason for hiding this comment

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

I merge the themes inside a generic attrRefs.
Also, I parsed it directly as AndroidResSource.AttrRef to make tests more expressive (instead of in explodeAndroidResSource.kt).

/** The resource references (e.g. activity themes, meta-data values, etc.). May be empty. */
val attrRefs: Set<AndroidResSource.AttrRef>,
val components: Map<String, Set<String>>
)

Expand All @@ -34,9 +35,6 @@ internal class ManifestParser(
val application = application(document)
val applicationName = application?.getAttribute("android:name") ?: ""

val applicationTheme = application?.theme()
val activityThemes = application?.getElementsByTagName(Manifest.Component.ACTIVITY.tagName)?.mapToSet { it.theme() }?.filterNotNull() ?: emptySet()

val services = application?.componentNames(Manifest.Component.SERVICE, packageName) ?: emptySet()
val providers = application?.componentNames(Manifest.Component.PROVIDER, packageName) ?: emptySet()
val activities = application?.componentNames(Manifest.Component.ACTIVITY, packageName) ?: emptySet()
Expand All @@ -53,10 +51,12 @@ internal class ManifestParser(
if (receivers.isNotEmpty()) componentsMapping[Manifest.Component.RECEIVER.mapKey] = receivers
}

val attrRefs = document.attrs().mapNotNull(AndroidResSource.AttrRef::from).toSet()

return ParseResult(
packageName = packageName,
applicationName = applicationName,
themes = setOfNotNull(applicationTheme) + activityThemes,
attrRefs = attrRefs,
components = componentsMapping
)
}
Expand Down Expand Up @@ -93,11 +93,6 @@ internal class ManifestParser(
}
}

private fun Node.theme(): String? {
return attributes?.getNamedItem("android:theme")
?.nodeValue?.substringAfter("@style/")
}

private fun Element.componentNames(
component: Manifest.Component,
packageName: String
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -149,15 +149,10 @@ internal class AndroidManifestParser(
}

private fun Pair<File, ParseResult>.toExplodedManifest(): ExplodedManifest {
val file = first
val parseResult = second
val applicationName = parseResult.applicationName
val themes = parseResult.themes.mapNotNull(AndroidResSource.AttrRef::style).toSet()

return ExplodedManifest(
relativePath = file.toRelativeString(projectDir),
applicationName = applicationName,
themes = themes,
relativePath = first.toRelativeString(projectDir),
applicationName = second.applicationName,
attrRefs = second.attrRefs,
)
}
}
Expand Down Expand Up @@ -197,5 +192,5 @@ internal data class ExplodedRes(
internal data class ExplodedManifest(
val relativePath: String,
val applicationName: String,
val themes: Set<AndroidResSource.AttrRef>,
val attrRefs: Set<AndroidResSource.AttrRef>,
)
7 changes: 1 addition & 6 deletions src/main/kotlin/com/autonomousapps/model/internal/Source.kt
Original file line number Diff line number Diff line change
Expand Up @@ -5,7 +5,6 @@ package com.autonomousapps.model.internal
import com.autonomousapps.internal.parse.AndroidResParser
import com.autonomousapps.internal.utils.LexicographicIterableComparator
import com.autonomousapps.internal.utils.efficient
import com.autonomousapps.model.internal.AndroidResSource.AttrRef.Companion.type
import com.autonomousapps.model.internal.intermediates.consumer.LdcConstant
import com.squareup.moshi.JsonClass
import dev.zacsweers.moshix.sealed.annotations.TypeLabel
Expand Down Expand Up @@ -174,7 +173,7 @@ internal data class AndroidResSource(
*
* @see <a href="https://developer.android.com/guide/topics/resources/providing-resources#ResourcesFromXml">Accessing resources from XML</a>
*/
private val TYPE_REGEX = Regex("""@(\w+:)?(?<type>\w+)/(\w+)""")
private val TYPE_REGEX = Regex("""@(\w+:)?(?<type>\w+)/([\w.]+)""")
Copy link
Contributor Author

Choose a reason for hiding this comment

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

. are actually used for styles/themes, otherwise it breaks detects theme set in manifest and detects theme set in manifest on Activity tests.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I also added a unit test for this.


/**
* TODO(tsr): this regex is too permissive. I only want `@+id/...`, but I lazily just copied the above with a
Expand All @@ -201,10 +200,6 @@ internal data class AndroidResSource(
*/
private val ATTR_REGEX = Regex("""\?(\w+:)?(\w+/)?(?<attr>\w+)""")

fun style(name: String): AttrRef? {
return if (name.isBlank()) null else AttrRef("style", name.toCanonicalResString())
}

/**
* Push [AttrRef]s into the [container], either as external references or as internal "new IDs" (`@+id`). The
* purpose of this approach is to avoid parsing the XML file twice.
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -159,7 +159,7 @@ public abstract class XmlSourceExploderTask @Inject constructor(
if (explodedManifest.applicationName.isNotBlank()) {
usedClasses.add(explodedManifest.applicationName)
}
explodedManifest.themes.forEach(attrRefs::add)
explodedManifest.attrRefs.forEach(attrRefs::add)
},
AndroidResBuilder::concat
)
Expand All @@ -176,7 +176,7 @@ public abstract class XmlSourceExploderTask @Inject constructor(
if (explodedManifest.applicationName.isNotBlank()) {
usedClasses.add(explodedManifest.applicationName)
}
explodedManifest.themes.forEach(attrRefs::add)
explodedManifest.attrRefs.forEach(attrRefs::add)
},
AndroidResBuilder::concat
)
Expand Down
54 changes: 45 additions & 9 deletions src/test/kotlin/com/autonomousapps/internal/ManifestParserTest.kt
Original file line number Diff line number Diff line change
Expand Up @@ -3,7 +3,9 @@
package com.autonomousapps.internal

import com.autonomousapps.internal.ManifestParser.ManifestParseException
import com.autonomousapps.model.internal.AndroidResSource.AttrRef
import com.google.common.truth.Truth.assertThat
import org.intellij.lang.annotations.Language
import org.junit.jupiter.api.Assertions.assertThrows
import org.junit.jupiter.api.Test
import org.junit.jupiter.api.io.TempDir
Expand Down Expand Up @@ -150,26 +152,60 @@ class ManifestParserTest {
assertThat(manifest.applicationName).isEqualTo("mutual.aid.explode")
}

@Test fun `parse themes`() {
@Test fun `parse resource references`() {
Copy link
Contributor Author

Choose a reason for hiding this comment

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

In this test, I added common references that we are using in our own app. This is definitely not an exhaustive list of all places where references are allowed.

val manifest = parse(
manifest = """
<?xml version="1.0" encoding="utf-8"?>
<manifest
xmlns:android="http://schemas.android.com/apk/res/android"
xmlns:tools="http://schemas.android.com/tools" >
<manifest xmlns:android="http://schemas.android.com/apk/res/android"
xmlns:tools="http://schemas.android.com/tools">

<application android:theme="@style/TheEternalVoid">
<activity android:name=".MainActivity" android:theme="@style/TheTwistedLand"/>
</application>
<application android:icon="@mipmap/ic_launcher" android:label="@string/app_name"
android:networkSecurityConfig="@xml/network_security_config"
android:theme="@style/TheEternalVoid">
<activity android:name=".MainActivity" android:theme="@style/TheTwistedLand">
<intent-filter android:autoVerify="true">
<action android:name="android.intent.action.VIEW" />
<category android:name="android.intent.category.DEFAULT" />
<category android:name="android.intent.category.BROWSABLE" />
<data android:scheme="https" />
<data android:host="@string/deeplink_host" />
<data android:path="@string/deeplink_path" />
</intent-filter>
</activity>
<provider android:name="androidx.startup.InitializationProvider"
android:authorities="${'$'}{applicationId}.androidx-startup"
android:exported="false" tools:node="merge">
<meta-data android:name="com.app.MyInitializer"
android:value="@string/androidx_startup" />
</provider>
<meta-data android:name="com.google.android.geo.API_KEY"
android:value="@string/google_maps_api_key" />
<meta-data android:name="com.google.android.gms.version"
android:value="@integer/google_play_services_version" />
<meta-data android:name="google_analytics_default_allow_analytics_storage"
android:value="@bool/google_analytics_enabled" />
</application>
</manifest>
""".trimIndent(),
dslNamespace = "com.app"
)

assertThat(manifest.themes).isEqualTo(setOf("TheEternalVoid", "TheTwistedLand"))
assertThat(manifest.attrRefs).containsExactly(
AttrRef(type = "mipmap", id = "ic_launcher"),
AttrRef(type = "string", id = "app_name"),
AttrRef(type = "xml", id = "network_security_config"),
AttrRef(type = "style", id = "TheEternalVoid"),
AttrRef(type = "style", id = "TheTwistedLand"),
AttrRef(type = "string", id = "deeplink_host"),
AttrRef(type = "string", id = "deeplink_path"),
AttrRef(type = "string", id = "androidx_startup"),
AttrRef(type = "string", id = "google_maps_api_key"),
AttrRef(type = "integer", id = "google_play_services_version"),
AttrRef(type = "bool", id = "google_analytics_enabled"),
)
}

private fun parse(manifest: String, dslNamespace: String = ""): ManifestParser.ParseResult {
private fun parse(@Language("XML") manifest: String, dslNamespace: String = ""): ManifestParser.ParseResult {
Copy link
Contributor Author

Choose a reason for hiding this comment

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

Adding this for syntax highlighting in the IDE

val file = tempFolder.resolve("AndroidManifest.xml").toFile()
file.writeText(manifest)
return ManifestParser(dslNamespace).parse(file)
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -28,4 +28,12 @@ class AttrRefTest {
)
}

@Test
fun `theme with period is parsed as an AttrRef`() {
assertEquals(
AttrRef(type = "style", id = "AppTheme_Dot"),
from("android:theme" to "@style/AppTheme.Dot"),
)
}

}