Merge pull request #5483 from adrianclay/use-ktor-client

Switch from java.net.URL to Ktor client

app/build.gradle.kts

@@ -153,6 +153,11 @@ dependencies {
     // scheduling background jobs
     implementation("androidx.work:work-runtime-ktx:2.9.0")
 
+    // HTTP Client
+    implementation("io.ktor:ktor-client-core:2.3.8")
+    implementation("io.ktor:ktor-client-cio:2.3.8")
+    testImplementation("io.ktor:ktor-client-mock:2.3.8")
+
     // finding in which country we are for country-specific logic
     implementation("de.westnordost:countryboundaries:2.1")
     // finding a name for a feature without a name tag

app/proguard-rules.pro

@@ -5,7 +5,6 @@
 # let's just keep everything
 -keep class com.mapzen.tangram.** { *; }
 -keep class com.mapzen.tangram.* { *; }
-
 # tangram end --------------------------------------------------------------------------------------
 
 # Lifecycle
@@ -38,6 +37,9 @@
     kotlinx.serialization.KSerializer serializer(...);
 }
 
+# ktor client, see https://youtrack.jetbrains.com/issue/KTOR-5528
+-dontwarn org.slf4j.impl.StaticLoggerBinder
+
 # Change here com.yourcompany.yourpackage
 -keep,includedescriptorclasses class de.westnordost.streetcomplete.**$$serializer { *; }
 -keepclassmembers class de.westnordost.streetcomplete.** {

app/src/main/java/de/westnordost/streetcomplete/ApplicationModule.kt

@@ -5,6 +5,9 @@ import android.content.res.Resources
 import de.westnordost.streetcomplete.util.CrashReportExceptionHandler
 import de.westnordost.streetcomplete.util.SoundFx
 import de.westnordost.streetcomplete.util.logs.DatabaseLogger
+import io.ktor.client.HttpClient
+import io.ktor.client.plugins.defaultRequest
+import io.ktor.http.userAgent
 import org.koin.android.ext.koin.androidContext
 import org.koin.dsl.module
 
@@ -15,4 +18,9 @@ val appModule = module {
     single { CrashReportExceptionHandler(androidContext(), get(), "streetcomplete_errors@westnordost.de", "crashreport.txt") }
     single { DatabaseLogger(get()) }
     single { SoundFx(androidContext()) }
+    single { HttpClient {
+        defaultRequest {
+            userAgent(ApplicationConstants.USER_AGENT)
+        }
+    } }
 }

app/src/main/java/de/westnordost/streetcomplete/data/osmnotes/AvatarsDownloader.kt

@@ -3,19 +3,23 @@ package de.westnordost.streetcomplete.data.osmnotes
 import de.westnordost.osmapi.user.UserApi
 import de.westnordost.streetcomplete.util.ktx.format
 import de.westnordost.streetcomplete.util.ktx.nowAsEpochMilliseconds
-import de.westnordost.streetcomplete.util.ktx.saveToFile
 import de.westnordost.streetcomplete.util.logs.Log
+import io.ktor.client.HttpClient
+import io.ktor.client.plugins.expectSuccess
+import io.ktor.client.request.get
+import io.ktor.client.statement.bodyAsChannel
+import io.ktor.util.cio.writeChannel
+import io.ktor.utils.io.copyAndClose
 import java.io.File
-import java.io.IOException
-import java.net.URL
 
 /** Downloads and stores the OSM avatars of users */
 class AvatarsDownloader(
+    private val httpClient: HttpClient,
     private val userApi: UserApi,
     private val cacheDir: File
 ) {
 
-    fun download(userIds: Collection<Long>) {
+    suspend fun download(userIds: Collection<Long>) {
         if (!ensureCacheDirExists()) {
             Log.w(TAG, "Unable to create directories for avatars")
             return
@@ -41,13 +45,16 @@ class AvatarsDownloader(
         }
 
     /** download avatar for the given user and a known avatar url */
-    fun download(userId: Long, avatarUrl: String) {
+    suspend fun download(userId: Long, avatarUrl: String) {
         if (!ensureCacheDirExists()) return
+        val avatarFile = File(cacheDir, "$userId")
         try {
-            val avatarFile = File(cacheDir, "$userId")
-            URL(avatarUrl).saveToFile(avatarFile)
+            val response = httpClient.get(avatarUrl) {
+                expectSuccess = true
+            }
+            response.bodyAsChannel().copyAndClose(avatarFile.writeChannel())
             Log.d(TAG, "Downloaded file: ${avatarFile.path}")
-        } catch (e: IOException) {
+        } catch (e: Exception) {
             Log.w(TAG, "Unable to download avatar for user id $userId")
         }
     }

app/src/main/java/de/westnordost/streetcomplete/data/osmnotes/AvatarsInNotesUpdater.kt

@@ -1,4 +1,5 @@
 package de.westnordost.streetcomplete.data.osmnotes
+import kotlinx.coroutines.runBlocking
 
 class AvatarsInNotesUpdater(private val downloader: AvatarsDownloader) :
     NoteController.Listener {
@@ -7,7 +8,7 @@ class AvatarsInNotesUpdater(private val downloader: AvatarsDownloader) :
         if (added.isEmpty() && updated.isEmpty()) return
 
         val noteCommentUserIds = (added + updated).flatMap { it.userIds }.toSet()
-        downloader.download(noteCommentUserIds)
+        runBlocking { downloader.download(noteCommentUserIds) }
     }
 
     override fun onCleared() {}

app/src/main/java/de/westnordost/streetcomplete/data/osmnotes/NotesModule.kt

@@ -8,11 +8,11 @@ import java.io.File
 
 val notesModule = module {
     factory(named("AvatarsCacheDirectory")) { File(get<Context>().cacheDir, ApplicationConstants.AVATARS_CACHE_DIRECTORY) }
-    factory { AvatarsDownloader(get(), get(named("AvatarsCacheDirectory"))) }
+    factory { AvatarsDownloader(get(), get(), get(named("AvatarsCacheDirectory"))) }
     factory { AvatarsInNotesUpdater(get()) }
     factory { NoteDao(get()) }
     factory { NotesDownloader(get(), get()) }
-    factory { StreetCompleteImageUploader(ApplicationConstants.SC_PHOTO_SERVICE_URL) }
+    factory { StreetCompleteImageUploader(get(), ApplicationConstants.SC_PHOTO_SERVICE_URL) }
 
     single {
         NoteController(get()).apply {

app/src/main/java/de/westnordost/streetcomplete/data/osmnotes/StreetCompleteImageUploader.kt

@@ -1,16 +1,22 @@
 package de.westnordost.streetcomplete.data.osmnotes
 
-import de.westnordost.streetcomplete.ApplicationConstants
 import de.westnordost.streetcomplete.data.download.ConnectionException
+import io.ktor.client.HttpClient
+import io.ktor.client.call.body
+import io.ktor.client.request.header
+import io.ktor.client.request.post
+import io.ktor.client.request.setBody
+import io.ktor.http.ContentType
+import io.ktor.http.HttpStatusCode
+import io.ktor.http.contentType
+import io.ktor.http.defaultForFile
+import io.ktor.util.cio.readChannel
+import io.ktor.utils.io.errors.IOException
 import kotlinx.serialization.SerialName
 import kotlinx.serialization.Serializable
 import kotlinx.serialization.SerializationException
 import kotlinx.serialization.json.Json
 import java.io.File
-import java.io.IOException
-import java.net.HttpURLConnection
-import java.net.URL
-import java.net.URLConnection
 
 @Serializable
 private data class PhotoUploadResponse(
@@ -19,9 +25,12 @@ private data class PhotoUploadResponse(
 )
 
 /** Upload and activate a list of image paths to an instance of the
- * <a href="https://github.com/exploide/sc-photo-service">StreetComplete image hosting service</a>
+ * <a href="https://github.com/streetcomplete/sc-photo-service">StreetComplete image hosting service</a>
  */
-class StreetCompleteImageUploader(private val baseUrl: String) {
+class StreetCompleteImageUploader(
+    private val httpClient: HttpClient,
+    private val baseUrl: String
+) {
     private val json = Json {
         ignoreUnknownKeys = true
     }
@@ -31,43 +40,36 @@ class StreetCompleteImageUploader(private val baseUrl: String) {
      *  @throws ImageUploadServerException when there was a server error on upload (server error)
      *  @throws ImageUploadClientException when the server rejected the upload request (client error)
      *  @throws ConnectionException if it is currently not reachable (no internet etc) */
-    fun upload(imagePaths: List<String>): List<String> {
+    suspend fun upload(imagePaths: List<String>): List<String> {
         val imageLinks = ArrayList<String>()
 
         for (path in imagePaths) {
             val file = File(path)
             if (!file.exists()) continue
 
             try {
-                val connection = createConnection("upload.php")
-                connection.requestMethod = "POST"
-                connection.setRequestProperty("Content-Type", URLConnection.guessContentTypeFromName(file.path))
-                connection.setRequestProperty("Content-Transfer-Encoding", "binary")
-                connection.setRequestProperty("Content-Length", file.length().toString())
-                connection.outputStream.use { output ->
-                    file.inputStream().use { input ->
-                        input.copyTo(output)
-                    }
+                val response = httpClient.post(baseUrl + "upload.php") {
+                    contentType(ContentType.defaultForFile(file))
+                    header("Content-Transfer-Encoding", "binary")
+                    setBody(file.readChannel())
                 }
 
-                val status = connection.responseCode
-                if (status == HttpURLConnection.HTTP_OK) {
-                    val response = connection.inputStream.bufferedReader().use { it.readText() }
+                val status = response.status
+                val body = response.body<String>()
+                if (status == HttpStatusCode.OK) {
                     try {
-                        val parsedResponse = json.decodeFromString<PhotoUploadResponse>(response)
+                        val parsedResponse = json.decodeFromString<PhotoUploadResponse>(body)
                         imageLinks.add(parsedResponse.futureUrl)
                     } catch (e: SerializationException) {
-                        throw ImageUploadServerException("Upload Failed: Unexpected response \"$response\"")
+                        throw ImageUploadServerException("Upload Failed: Unexpected response \"$body\"")
                     }
                 } else {
-                    val error = connection.errorStream.bufferedReader().use { it.readText() }
-                    if (status / 100 == 5) {
-                        throw ImageUploadServerException("Upload failed: Error code $status, Message: \"$error\"")
+                    if (status.value / 100 == 5) {
+                        throw ImageUploadServerException("Upload failed: Error code $status, Message: \"$body\"")
                     } else {
-                        throw ImageUploadClientException("Upload failed: Error code $status, Message: \"$error\"")
+                        throw ImageUploadClientException("Upload failed: Error code $status, Message: \"$body\"")
                     }
                 }
-                connection.disconnect()
             } catch (e: IOException) {
                 throw ConnectionException("Upload failed", e)
             }
@@ -80,39 +82,29 @@ class StreetCompleteImageUploader(private val baseUrl: String) {
      *  @throws ImageUploadServerException when there was a server error on upload (server error)
      *  @throws ImageUploadClientException when the server rejected the upload request (client error)
      *  @throws ConnectionException if it is currently not reachable (no internet etc)  */
-    fun activate(noteId: Long) {
+    suspend fun activate(noteId: Long) {
         try {
-            val connection = createConnection("activate.php")
-            connection.requestMethod = "POST"
-            connection.setRequestProperty("Content-Type", "Content-Type: application/json")
-            connection.outputStream.bufferedWriter().use { it.write("{\"osm_note_id\": $noteId}") }
+            val response = httpClient.post(baseUrl + "activate.php") {
+                contentType(ContentType.Application.Json)
+                setBody("{\"osm_note_id\": $noteId}")
+            }
 
-            val status = connection.responseCode
-            if (status == HttpURLConnection.HTTP_GONE) {
+            val status = response.status
+            if (status == HttpStatusCode.Gone) {
                 // it's gone if the note does not exist anymore. That's okay, it should only fail
                 // if we might want to try again later.
-            } else if (status != HttpURLConnection.HTTP_OK) {
-                val error = connection.errorStream.bufferedReader().use { it.readText() }
-                if (status / 100 == 5) {
+            } else if (status != HttpStatusCode.OK) {
+                val error = response.body<String>()
+                if (status.value / 100 == 5) {
                     throw ImageUploadServerException("Error code $status, Message: \"$error\"")
                 } else {
                     throw ImageUploadClientException("Error code $status, Message: \"$error\"")
                 }
             }
-            connection.disconnect()
         } catch (e: IOException) {
             throw ConnectionException("", e)
         }
     }
-
-    private fun createConnection(url: String): HttpURLConnection {
-        val connection = URL(baseUrl + url).openConnection() as HttpURLConnection
-        connection.useCaches = false
-        connection.doOutput = true
-        connection.doInput = true
-        connection.setRequestProperty("User-Agent", ApplicationConstants.USER_AGENT)
-        return connection
-    }
 }
 
 class ImageUploadServerException(message: String? = null, cause: Throwable? = null) :

app/src/main/java/de/westnordost/streetcomplete/data/osmnotes/edits/NoteEditsUploader.kt

@@ -13,14 +13,14 @@ import de.westnordost.streetcomplete.data.upload.OnUploadedChangeListener
 import de.westnordost.streetcomplete.data.user.UserDataSource
 import de.westnordost.streetcomplete.util.ktx.truncate
 import de.westnordost.streetcomplete.util.logs.Log
+import io.ktor.http.encodeURLPathPart
 import kotlinx.coroutines.CoroutineName
 import kotlinx.coroutines.CoroutineScope
 import kotlinx.coroutines.Dispatchers
 import kotlinx.coroutines.SupervisorJob
 import kotlinx.coroutines.sync.Mutex
 import kotlinx.coroutines.sync.withLock
 import kotlinx.coroutines.withContext
-import java.net.URLEncoder
 
 class NoteEditsUploader(
     private val noteEditsController: NoteEditsController,
@@ -68,7 +68,7 @@ class NoteEditsUploader(
         }
     }
 
-    private fun uploadEdit(edit: NoteEdit) {
+    private suspend fun uploadEdit(edit: NoteEdit) {
         // try to upload the image and track if we have them
         val imageText = uploadAndGetAttachedPhotosText(edit.imagePaths)
         val trackText = uploadAndGetAttachedTrackText(edit.track, edit.text)
@@ -116,7 +116,7 @@ class NoteEditsUploader(
         }
     }
 
-    private fun uploadAndGetAttachedPhotosText(imagePaths: List<String>): String {
+    private suspend fun uploadAndGetAttachedPhotosText(imagePaths: List<String>): String {
         if (imagePaths.isNotEmpty()) {
             val urls = imageUploader.upload(imagePaths)
             if (urls.isNotEmpty()) {
@@ -132,7 +132,7 @@ class NoteEditsUploader(
     ): String {
         if (trackpoints.isEmpty()) return ""
         val trackId = tracksApi.create(trackpoints, noteText?.truncate(255))
-        val encodedUsername = URLEncoder.encode(userDataSource.userName, "utf-8").replace("+", "%20")
+        val encodedUsername = userDataSource.userName!!.encodeURLPathPart()
         return "\n\nGPS Trace: https://www.openstreetmap.org/user/$encodedUsername/traces/$trackId\n"
     }
 

app/src/main/java/de/westnordost/streetcomplete/data/upload/UploadModule.kt

@@ -8,7 +8,7 @@ import org.koin.core.qualifier.named
 import org.koin.dsl.module
 
 val uploadModule = module {
-    factory { VersionIsBannedChecker("https://www.westnordost.de/streetcomplete/banned_versions.txt", ApplicationConstants.USER_AGENT) }
+    factory { VersionIsBannedChecker(get(), "https://www.westnordost.de/streetcomplete/banned_versions.txt", ApplicationConstants.USER_AGENT) }
 
     single { Uploader(get(), get(), get(), get(), get(), get(named("SerializeSync"))) }
     /* uploading and downloading should be serialized, i.e. may not run in parallel, to avoid

app/src/main/java/de/westnordost/streetcomplete/data/upload/Uploader.kt

@@ -11,7 +11,10 @@ import de.westnordost.streetcomplete.data.user.UserLoginStatusSource
 import de.westnordost.streetcomplete.util.Listeners
 import de.westnordost.streetcomplete.util.logs.Log
 import kotlinx.coroutines.CancellationException
+import kotlinx.coroutines.CoroutineStart
 import kotlinx.coroutines.Dispatchers
+import kotlinx.coroutines.GlobalScope
+import kotlinx.coroutines.async
 import kotlinx.coroutines.sync.Mutex
 import kotlinx.coroutines.sync.withLock
 import kotlinx.coroutines.withContext
@@ -27,7 +30,7 @@ class Uploader(
 
     private val listeners = Listeners<UploadProgressSource.Listener>()
 
-    private val bannedInfo by lazy { versionIsBannedChecker.get() }
+    private lateinit var bannedInfo: BannedInfo
 
     private val uploadedChangeRelay = object : OnUploadedChangeListener {
         override fun onUploaded(questType: String, at: LatLon) {
@@ -52,7 +55,11 @@ class Uploader(
         try {
             isUploadInProgress = true
             listeners.forEach { it.onStarted() }
-            val banned = withContext(Dispatchers.IO) { bannedInfo }
+
+            if (!::bannedInfo.isInitialized) {
+                bannedInfo = withContext(Dispatchers.IO) { versionIsBannedChecker.get() }
+            }
+            val banned = bannedInfo
             if (banned is IsBanned) {
                 throw VersionBannedException(banned.reason)
             }