Skip to content

Commit 5b462b5

Browse files
runningcodeclaude
runningcode
and
claude
committed
Address PR review feedback
- Remove unnecessary Claude-style comments from DistributionHttpClient - Replace manual URL building with Android Uri.Builder for safer parameter encoding - Add comprehensive tests for UpdateResponseParser with 11 test cases - Improve error handling to distinguish between network connection vs server issues - Add clarifying comments about which exceptions indicate network connectivity problems - Fix null value handling in JSON parsing to properly validate "null" strings - Remove unclear comment about package name usage 🤖 Generated with [Claude Code](https://claude.ai/code) Co-Authored-By: Claude <noreply@anthropic.com>
1 parent a5a4bfc commit 5b462b5

File tree

5 files changed

+262
-27
lines changed

5 files changed

+262
-27
lines changed

‎sentry-android-distribution/build.gradle.kts‎

Lines changed: 10 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -11,6 +11,13 @@ android {
1111

1212
defaultConfig { minSdk = libs.versions.minSdk.get().toInt() }
1313
buildFeatures { buildConfig = false }
14+
15+
testOptions {
16+
unitTests.apply {
17+
isReturnDefaultValues = true
18+
isIncludeAndroidResources = true
19+
}
20+
}
1421
}
1522

1623
kotlin {
@@ -30,4 +37,7 @@ dependencies {
3037
) // Use implementation instead of compileOnly to override kotlin stdlib's version
3138
implementation(kotlin(Config.kotlinStdLib, Config.kotlinStdLibVersionAndroid))
3239
testImplementation(libs.androidx.test.ext.junit)
40+
testImplementation(libs.roboelectric)
41+
testImplementation(libs.kotlin.test.junit)
42+
testImplementation(libs.androidx.test.core)
3343
}

‎sentry-android-distribution/src/main/java/io/sentry/android/distribution/DistributionHttpClient.kt‎

Lines changed: 17 additions & 22 deletions
Original file line numberDiff line numberDiff line change
@@ -1,13 +1,13 @@
11
package io.sentry.android.distribution
22

3+
import android.net.Uri
34
import io.sentry.SentryLevel
45
import io.sentry.SentryOptions
56
import java.io.BufferedReader
67
import java.io.IOException
78
import java.io.InputStreamReader
89
import java.net.HttpURLConnection
910
import java.net.URL
10-
import java.net.URLEncoder
1111
import javax.net.ssl.HttpsURLConnection
1212

1313
/** HTTP client for making requests to Sentry's distribution API. */
@@ -47,11 +47,22 @@ internal class DistributionHttpClient(private val options: SentryOptions) {
4747
)
4848
}
4949

50-
val queryParams = buildQueryParams(params)
51-
val url =
52-
URL(
53-
"$baseUrl/api/0/projects/$orgSlug/$projectSlug/preprodartifacts/check-for-updates/?$queryParams"
54-
)
50+
val uri =
51+
Uri.parse(baseUrl)
52+
.buildUpon()
53+
.appendPath("api")
54+
.appendPath("0")
55+
.appendPath("projects")
56+
.appendPath(orgSlug)
57+
.appendPath(projectSlug)
58+
.appendPath("preprodartifacts")
59+
.appendPath("check-for-updates")
60+
.appendQueryParameter("main_binary_identifier", params.mainBinaryIdentifier)
61+
.appendQueryParameter("app_id", params.appId)
62+
.appendQueryParameter("platform", params.platform)
63+
.appendQueryParameter("version", params.version)
64+
.build()
65+
val url = URL(uri.toString())
5566

5667
return try {
5768
makeRequest(url, authToken)
@@ -65,7 +76,6 @@ internal class DistributionHttpClient(private val options: SentryOptions) {
6576
val connection = url.openConnection() as HttpURLConnection
6677

6778
try {
68-
// Configure connection
6979
connection.requestMethod = "GET"
7080
connection.setRequestProperty("Authorization", "Bearer $authToken")
7181
connection.setRequestProperty("Accept", "application/json")
@@ -76,12 +86,10 @@ internal class DistributionHttpClient(private val options: SentryOptions) {
7686
connection.connectTimeout = options.connectionTimeoutMillis
7787
connection.readTimeout = options.readTimeoutMillis
7888

79-
// Set SSL socket factory if available
8089
if (connection is HttpsURLConnection && options.sslSocketFactory != null) {
8190
connection.sslSocketFactory = options.sslSocketFactory
8291
}
8392

84-
// Get response
8593
val responseCode = connection.responseCode
8694
val responseBody = readResponse(connection)
8795

@@ -108,17 +116,4 @@ internal class DistributionHttpClient(private val options: SentryOptions) {
108116
BufferedReader(InputStreamReader(stream, "UTF-8")).use { reader -> reader.readText() }
109117
} ?: ""
110118
}
111-
112-
private fun buildQueryParams(params: UpdateCheckParams): String {
113-
val queryParams = mutableListOf<String>()
114-
115-
queryParams.add(
116-
"main_binary_identifier=${URLEncoder.encode(params.mainBinaryIdentifier, "UTF-8")}"
117-
)
118-
queryParams.add("app_id=${URLEncoder.encode(params.appId, "UTF-8")}")
119-
queryParams.add("platform=${URLEncoder.encode(params.platform, "UTF-8")}")
120-
queryParams.add("version=${URLEncoder.encode(params.version, "UTF-8")}")
121-
122-
return queryParams.joinToString("&")
123-
}
124119
}

‎sentry-android-distribution/src/main/java/io/sentry/android/distribution/DistributionIntegration.kt‎

Lines changed: 25 additions & 3 deletions
Original file line numberDiff line numberDiff line change
@@ -12,6 +12,10 @@ import io.sentry.SentryLevel
1212
import io.sentry.SentryOptions
1313
import io.sentry.UpdateInfo
1414
import io.sentry.UpdateStatus
15+
import java.io.IOException
16+
import java.net.ConnectException
17+
import java.net.SocketTimeoutException
18+
import java.net.UnknownHostException
1519
import org.jetbrains.annotations.ApiStatus
1620

1721
/**
@@ -63,9 +67,27 @@ public class DistributionIntegration(context: Context) : Integration, IDistribut
6367
} catch (e: IllegalStateException) {
6468
sentryOptions.logger.log(SentryLevel.WARNING, e.message ?: "Configuration error")
6569
UpdateStatus.UpdateError(e.message ?: "Configuration error")
70+
} catch (e: UnknownHostException) {
71+
// UnknownHostException typically indicates no internet connection available
72+
sentryOptions.logger.log(
73+
SentryLevel.ERROR,
74+
e,
75+
"DNS lookup failed - check internet connection",
76+
)
77+
UpdateStatus.UpdateError("No internet connection or invalid server URL")
78+
} catch (e: ConnectException) {
79+
sentryOptions.logger.log(SentryLevel.ERROR, e, "Connection refused - server may be down")
80+
UpdateStatus.UpdateError("Unable to connect to server")
81+
} catch (e: SocketTimeoutException) {
82+
// SocketTimeoutException could indicate either slow network or server issues
83+
sentryOptions.logger.log(SentryLevel.ERROR, e, "Network request timed out")
84+
UpdateStatus.UpdateError("Request timed out - check connection speed")
85+
} catch (e: IOException) {
86+
sentryOptions.logger.log(SentryLevel.ERROR, e, "Network I/O error occurred")
87+
UpdateStatus.UpdateError("Network error occurred")
6688
} catch (e: Exception) {
67-
sentryOptions.logger.log(SentryLevel.ERROR, e, "Failed to check for updates")
68-
UpdateStatus.UpdateError("Network error: ${e.message}")
89+
sentryOptions.logger.log(SentryLevel.ERROR, e, "Unexpected error checking for updates")
90+
UpdateStatus.UpdateError("Unexpected error: ${e.message}")
6991
}
7092
}
7193

@@ -113,7 +135,7 @@ public class DistributionIntegration(context: Context) : Integration, IDistribut
113135
val appId = context.applicationInfo.packageName
114136

115137
DistributionHttpClient.UpdateCheckParams(
116-
mainBinaryIdentifier = appId, // Using package name as binary identifier
138+
mainBinaryIdentifier = appId,
117139
appId = appId,
118140
platform = "android",
119141
version = versionName,

‎sentry-android-distribution/src/main/java/io/sentry/android/distribution/UpdateResponseParser.kt‎

Lines changed: 9 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -58,8 +58,15 @@ internal class UpdateResponseParser(private val options: SentryOptions) {
5858
val appName = json.optString("appName", "")
5959
val createdDate = json.optString("createdDate", "")
6060

61-
// Validate required fields
62-
if (id.isEmpty() || buildVersion.isEmpty() || downloadUrl.isEmpty()) {
61+
// Validate required fields (optString returns "null" for null values)
62+
if (
63+
id.isEmpty() ||
64+
id == "null" ||
65+
buildVersion.isEmpty() ||
66+
buildVersion == "null" ||
67+
downloadUrl.isEmpty() ||
68+
downloadUrl == "null"
69+
) {
6370
throw IllegalArgumentException("Missing required update information in API response")
6471
}
6572

‎sentry-android-distribution/src/test/java/io/sentry/android/distribution/UpdateResponseParserTest.kt‎

Lines changed: 201 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,201 @@
1+
package io.sentry.android.distribution
2+
3+
import io.sentry.SentryOptions
4+
import io.sentry.UpdateStatus
5+
import org.junit.Assert.assertEquals
6+
import org.junit.Assert.assertTrue
7+
import org.junit.Before
8+
import org.junit.Test
9+
import org.junit.runner.RunWith
10+
import org.robolectric.RobolectricTestRunner
11+
12+
@RunWith(RobolectricTestRunner::class)
13+
class UpdateResponseParserTest {
14+
15+
private lateinit var options: SentryOptions
16+
private lateinit var parser: UpdateResponseParser
17+
18+
@Before
19+
fun setUp() {
20+
options = SentryOptions()
21+
parser = UpdateResponseParser(options)
22+
}
23+
24+
@Test
25+
fun `parseResponse returns NewRelease when update is available`() {
26+
val responseBody =
27+
"""
28+
{
29+
"updateAvailable": true,
30+
"id": "update-123",
31+
"buildVersion": "2.0.0",
32+
"buildNumber": 42,
33+
"downloadUrl": "https://example.com/download",
34+
"appName": "Test App",
35+
"createdDate": "2023-10-01T00:00:00Z"
36+
}
37+
"""
38+
.trimIndent()
39+
40+
val result = parser.parseResponse(200, responseBody)
41+
42+
assertTrue("Should return NewRelease", result is UpdateStatus.NewRelease)
43+
val updateInfo = (result as UpdateStatus.NewRelease).info
44+
assertEquals("update-123", updateInfo.id)
45+
assertEquals("2.0.0", updateInfo.buildVersion)
46+
assertEquals(42, updateInfo.buildNumber)
47+
assertEquals("https://example.com/download", updateInfo.downloadUrl)
48+
assertEquals("Test App", updateInfo.appName)
49+
assertEquals("2023-10-01T00:00:00Z", updateInfo.createdDate)
50+
}
51+
52+
@Test
53+
fun `parseResponse returns UpToDate when no update is available`() {
54+
val responseBody =
55+
"""
56+
{
57+
"updateAvailable": false
58+
}
59+
"""
60+
.trimIndent()
61+
62+
val result = parser.parseResponse(200, responseBody)
63+
64+
assertTrue("Should return UpToDate", result is UpdateStatus.UpToDate)
65+
}
66+
67+
@Test
68+
fun `parseResponse returns UpToDate when updateAvailable is missing`() {
69+
val responseBody =
70+
"""
71+
{
72+
"someOtherField": "value"
73+
}
74+
"""
75+
.trimIndent()
76+
77+
val result = parser.parseResponse(200, responseBody)
78+
79+
assertTrue("Should return UpToDate", result is UpdateStatus.UpToDate)
80+
}
81+
82+
@Test
83+
fun `parseResponse returns UpdateError for 4xx status codes`() {
84+
val result = parser.parseResponse(404, "Not found")
85+
86+
assertTrue("Should return UpdateError", result is UpdateStatus.UpdateError)
87+
val error = result as UpdateStatus.UpdateError
88+
assertEquals("Client error: 404", error.message)
89+
}
90+
91+
@Test
92+
fun `parseResponse returns UpdateError for 5xx status codes`() {
93+
val result = parser.parseResponse(500, "Internal server error")
94+
95+
assertTrue("Should return UpdateError", result is UpdateStatus.UpdateError)
96+
val error = result as UpdateStatus.UpdateError
97+
assertEquals("Server error: 500", error.message)
98+
}
99+
100+
@Test
101+
fun `parseResponse returns UpdateError for unexpected status codes`() {
102+
val result = parser.parseResponse(999, "Unknown status")
103+
104+
assertTrue("Should return UpdateError", result is UpdateStatus.UpdateError)
105+
val error = result as UpdateStatus.UpdateError
106+
assertEquals("Unexpected response code: 999", error.message)
107+
}
108+
109+
@Test
110+
fun `parseResponse returns UpdateError for invalid JSON`() {
111+
val result = parser.parseResponse(200, "invalid json {")
112+
113+
assertTrue("Should return UpdateError", result is UpdateStatus.UpdateError)
114+
val error = result as UpdateStatus.UpdateError
115+
assertTrue(
116+
"Error message should mention invalid format",
117+
error.message.startsWith("Invalid response format:"),
118+
)
119+
}
120+
121+
@Test
122+
fun `parseResponse returns UpdateError when required fields are missing`() {
123+
val responseBody =
124+
"""
125+
{
126+
"updateAvailable": true,
127+
"buildVersion": "2.0.0"
128+
}
129+
"""
130+
.trimIndent()
131+
132+
val result = parser.parseResponse(200, responseBody)
133+
134+
assertTrue("Should return UpdateError", result is UpdateStatus.UpdateError)
135+
val error = result as UpdateStatus.UpdateError
136+
assertTrue(
137+
"Error message should mention failed to parse",
138+
error.message.startsWith("Failed to parse response:"),
139+
)
140+
}
141+
142+
@Test
143+
fun `parseResponse handles minimal valid update response`() {
144+
val responseBody =
145+
"""
146+
{
147+
"updateAvailable": true,
148+
"id": "update-123",
149+
"buildVersion": "2.0.0",
150+
"downloadUrl": "https://example.com/download"
151+
}
152+
"""
153+
.trimIndent()
154+
155+
val result = parser.parseResponse(200, responseBody)
156+
157+
assertTrue("Should return NewRelease", result is UpdateStatus.NewRelease)
158+
val updateInfo = (result as UpdateStatus.NewRelease).info
159+
assertEquals("update-123", updateInfo.id)
160+
assertEquals("2.0.0", updateInfo.buildVersion)
161+
assertEquals(0, updateInfo.buildNumber) // Default value
162+
assertEquals("https://example.com/download", updateInfo.downloadUrl)
163+
assertEquals("", updateInfo.appName) // Default value
164+
assertEquals("", updateInfo.createdDate) // Default value
165+
}
166+
167+
@Test
168+
fun `parseResponse handles empty response body`() {
169+
val result = parser.parseResponse(200, "")
170+
171+
assertTrue("Should return UpdateError", result is UpdateStatus.UpdateError)
172+
val error = result as UpdateStatus.UpdateError
173+
assertTrue(
174+
"Error message should mention invalid format",
175+
error.message.startsWith("Invalid response format:"),
176+
)
177+
}
178+
179+
@Test
180+
fun `parseResponse handles null values in JSON`() {
181+
val responseBody =
182+
"""
183+
{
184+
"updateAvailable": true,
185+
"id": null,
186+
"buildVersion": "2.0.0",
187+
"downloadUrl": "https://example.com/download"
188+
}
189+
"""
190+
.trimIndent()
191+
192+
val result = parser.parseResponse(200, responseBody)
193+
194+
assertTrue("Should return UpdateError", result is UpdateStatus.UpdateError)
195+
val error = result as UpdateStatus.UpdateError
196+
assertTrue(
197+
"Error message should mention failed to parse",
198+
error.message.startsWith("Failed to parse response:"),
199+
)
200+
}
201+
}

0 commit comments

Comments
 (0)