feat: Download a transfer or folder with api v2#618
Conversation
PR Reviewer Guide 🔍(Review updated until commit 23554ac)Here are some key observations to aid the review process:
|
![github-actions[bot]](https://avatars.githubusercontent.com/in/15368?s=60&v=4){kind=small}
| }.getOrElse { exception -> | ||
| return when (exception) { | ||
| is CancellationException, is NetworkException -> Result.retry() | ||
| is AppDownloadManager.FailureException -> Result.failure() | ||
| else -> { | ||
| SentryLog.e(TAG, "failure", exception) | ||
| Result.failure() | ||
| } | ||
| } | ||
| } |
There was a problem hiding this comment.
Suggestion: Do not retry when a CancellationException is caught, as this indicates the work was cancelled by the user or system. Retrying cancelled work ignores the cancellation request and causes the download to restart unexpectedly. Remove CancellationException from the retry condition and treat it as a failure. [possible issue, importance: 8]
| }.getOrElse { exception -> | |
| return when (exception) { | |
| is CancellationException, is NetworkException -> Result.retry() | |
| is AppDownloadManager.FailureException -> Result.failure() | |
| else -> { | |
| SentryLog.e(TAG, "failure", exception) | |
| Result.failure() | |
| } | |
| } | |
| } | |
| }.getOrElse { exception -> | |
| return when (exception) { | |
| is CancellationException -> Result.failure() | |
| is NetworkException -> Result.retry() | |
| is AppDownloadManager.FailureException -> Result.failure() | |
| else -> { | |
| SentryLog.e(TAG, "failure", exception) | |
| Result.failure() | |
| } | |
| } | |
| } |
| private suspend fun downloadFile( | ||
| url: String, | ||
| output: OutputStream, | ||
| onDownload: suspend (bytesSentTotal: Long, contentLength: Long?) -> Unit, | ||
| ) { | ||
| createHttpClient(HttpClient.okHttpClient).prepareGet(url) { | ||
| accept(ContentType.Any) | ||
| onDownload { bytesSentTotal, contentLength -> | ||
| onDownload(bytesSentTotal, contentLength) | ||
| } | ||
| }.execute { response -> | ||
| val channel = response.bodyAsChannel() | ||
| while (!channel.isClosedForRead) { | ||
| val packet = channel.readRemaining(DEFAULT_BUFFER_SIZE) | ||
| while (!packet.exhausted()) output.write(packet.readByteArray()) | ||
| } | ||
| output.flush() | ||
| } | ||
| } |
There was a problem hiding this comment.
Suggestion: The HttpClient created by createHttpClient() is never closed, causing resource leaks (socket exhaustion, memory growth) when downloading multiple files. Use a single shared HttpClient instance injected into the class, or ensure the client is properly closed after use by wrapping in try-finally or using use {}. [possible issue, importance: 8]
| private suspend fun downloadFile( | |
| url: String, | |
| output: OutputStream, | |
| onDownload: suspend (bytesSentTotal: Long, contentLength: Long?) -> Unit, | |
| ) { | |
| createHttpClient(HttpClient.okHttpClient).prepareGet(url) { | |
| accept(ContentType.Any) | |
| onDownload { bytesSentTotal, contentLength -> | |
| onDownload(bytesSentTotal, contentLength) | |
| } | |
| }.execute { response -> | |
| val channel = response.bodyAsChannel() | |
| while (!channel.isClosedForRead) { | |
| val packet = channel.readRemaining(DEFAULT_BUFFER_SIZE) | |
| while (!packet.exhausted()) output.write(packet.readByteArray()) | |
| } | |
| output.flush() | |
| } | |
| } | |
| private suspend fun downloadFile( | |
| url: String, | |
| output: OutputStream, | |
| onDownload: suspend (bytesSentTotal: Long, contentLength: Long?) -> Unit, | |
| ) { | |
| val client = createHttpClient(HttpClient.okHttpClient) | |
| try { | |
| client.prepareGet(url) { | |
| accept(ContentType.Any) | |
| onDownload { bytesSentTotal, contentLength -> | |
| onDownload(bytesSentTotal, contentLength) | |
| } | |
| }.execute { response -> | |
| val channel = response.bodyAsChannel() | |
| while (!channel.isClosedForRead) { | |
| val packet = channel.readRemaining(DEFAULT_BUFFER_SIZE) | |
| while (!packet.exhausted()) output.write(packet.readByteArray()) | |
| } | |
| output.flush() | |
| } | |
| } finally { | |
| client.close() | |
| } | |
| } |
| private fun hasAvailableSpace(requiredBytes: Long): Boolean { | ||
| val path = when { | ||
| Build.VERSION.SDK_INT >= Build.VERSION_CODES.Q -> applicationContext.getExternalFilesDir(null)?.path ?: return true | ||
| else -> Environment.getExternalStoragePublicDirectory(Environment.DIRECTORY_DOWNLOADS).path | ||
| } | ||
| val availableBytes = StatFs(path).availableBytes | ||
| // Add a safety margin of 10% extra space | ||
| val requiredWithMargin = (requiredBytes * 110) / 100 | ||
| return availableBytes >= requiredWithMargin | ||
| } |
There was a problem hiding this comment.
Suggestion: For API level Q and above, the code incorrectly checks available space in the app's private external directory (getExternalFilesDir) instead of the public Downloads directory where files are actually saved via MediaStore. This can cause incorrect "insufficient space" errors or fail to detect actual lack of space. Use the public Downloads directory path for all API levels. [possible issue, importance: 8]
| private fun hasAvailableSpace(requiredBytes: Long): Boolean { | |
| val path = when { | |
| Build.VERSION.SDK_INT >= Build.VERSION_CODES.Q -> applicationContext.getExternalFilesDir(null)?.path ?: return true | |
| else -> Environment.getExternalStoragePublicDirectory(Environment.DIRECTORY_DOWNLOADS).path | |
| } | |
| val availableBytes = StatFs(path).availableBytes | |
| // Add a safety margin of 10% extra space | |
| val requiredWithMargin = (requiredBytes * 110) / 100 | |
| return availableBytes >= requiredWithMargin | |
| } | |
| private fun hasAvailableSpace(requiredBytes: Long): Boolean { | |
| val path = Environment.getExternalStoragePublicDirectory(Environment.DIRECTORY_DOWNLOADS).path | |
| val availableBytes = StatFs(path).availableBytes | |
| // Add a safety margin of 10% extra space | |
| val requiredWithMargin = (requiredBytes * 110) / 100 | |
| return availableBytes >= requiredWithMargin | |
| } |
|
Persistent review updated to latest commit 23554ac |
| workManager.enqueueUniqueWork(uniqueWorkName, ExistingWorkPolicy.REPLACE, workRequest) | ||
| return UniqueDownloadId(999) |
There was a problem hiding this comment.
Suggestion: Return a unique identifier instead of the hardcoded value 999. Using a constant ID causes collisions when multiple downloads are active, leading to incorrect status tracking and potential conflicts in the database. Generate a unique ID based on the work request or use a counter. [possible issue, importance: 8]
| workManager.enqueueUniqueWork(uniqueWorkName, ExistingWorkPolicy.REPLACE, workRequest) | |
| return UniqueDownloadId(999) | |
| workManager.enqueueUniqueWork(uniqueWorkName, ExistingWorkPolicy.REPLACE, workRequest) | |
| return UniqueDownloadId(workRequest.id.hashCode()) |
| output: OutputStream, | ||
| onDownload: suspend (bytesSentTotal: Long, contentLength: Long?) -> Unit, | ||
| ) { | ||
| createHttpClient(HttpClient.okHttpClient).prepareGet(url) { |
There was a problem hiding this comment.
Suggestion: Reuse a single HttpClient instance instead of creating a new one for each file download. Creating new clients per download wastes resources, causes socket exhaustion under concurrent loads, and bypasses connection pooling benefits. Inject a singleton client or store it as a class property. [possible issue, importance: 6]
| createHttpClient(HttpClient.okHttpClient).prepareGet(url) { | |
| // Inject or define as singleton property | |
| private val httpClient = createHttpClient(HttpClient.okHttpClient) | |
| private suspend fun downloadFile( | |
| url: String, | |
| output: OutputStream, | |
| onDownload: suspend (bytesSentTotal: Long, contentLength: Long?) -> Unit, | |
| ) { | |
| httpClient.prepareGet(url) { |
| fun TransferUi.computeFolderDownloadPathWith(fileUi: FileUi): String { | ||
| val filePath = fileUi.path?.takeIf { it.contains("/") }?.let { | ||
| if (it.startsWith("/")) it.substringAfter("/") else it | ||
| } | ||
| return "$ROOT_FOLDER_NAME/${this.displayTitle}/${filePath?.substringBeforeLast("/") ?: ""}" | ||
| } |
There was a problem hiding this comment.
Suggestion: Sanitize displayTitle and file path components to prevent directory traversal attacks. User-controlled transfer titles containing ".." or path separators can escape the intended download directory on API < 29, potentially overwriting files in arbitrary locations. Remove or escape path separator characters from displayTitle. [security, importance: 9]
| fun TransferUi.computeFolderDownloadPathWith(fileUi: FileUi): String { | |
| val filePath = fileUi.path?.takeIf { it.contains("/") }?.let { | |
| if (it.startsWith("/")) it.substringAfter("/") else it | |
| } | |
| return "$ROOT_FOLDER_NAME/${this.displayTitle}/${filePath?.substringBeforeLast("/") ?: ""}" | |
| } | |
| fun TransferUi.computeFolderDownloadPathWith(fileUi: FileUi): String { | |
| val filePath = fileUi.path?.takeIf { it.contains("/") }?.let { | |
| if (it.startsWith("/")) it.substringAfter("/") else it | |
| }?.substringBeforeLast("/")?.replace("..", "_") | |
| val safeTitle = this.displayTitle.replace("[/\\\\]".toRegex(), "_").replace("..", "_") | |
| return "$ROOT_FOLDER_NAME/$safeTitle/${filePath ?: ""}" | |
| } |
| <string name="buttonEditTransfer">Edit my transfer</string> | ||
| <string name="buttonFeedback">Feedback</string> |
There was a problem hiding this comment.
Do you need this buttonFeedback?
|
|
||
| fun downloadSucceeded(tag: String, title: String) { | ||
| val notification = buildDownloadNotification(title) | ||
| notificationManagerCompat.notifyCompat(tag, 1, notification) |
There was a problem hiding this comment.
This method looks deprecated
|
|
||
| private fun buildDownloadNotification(title: String): Notification { | ||
| return NotificationCompat.Builder(appContext, ChannelIds.downloadChannelId) | ||
| .setTicker(title) |
There was a problem hiding this comment.
Shouldn't the ticker be "download completed" rather than the title of the file? Or if you want to go further maybe a mix of the two like "Download completed: $title" but this requires a new string
| private fun buildDownloadNotification(title: String): Notification { | ||
| return NotificationCompat.Builder(appContext, ChannelIds.downloadChannelId) | ||
| .setTicker(title) | ||
| .setContentTitle(title) | ||
| .setContentText(appContext.getString(R.string.notificationDownloadSuccessNotificationTitle)) | ||
| .setSmallIcon(defaultSmallIcon) | ||
| .setColor(notificationIconColor) | ||
| .setAutoCancel(true) | ||
| .build() | ||
| } |
There was a problem hiding this comment.
Do we not put an intent when the notification is clicked?
| return UniqueDownloadId(999) | ||
| } | ||
|
|
||
| fun downloadStatusFlow(transferId: String, folderId: String?): Flow<DownloadStatus> { |
There was a problem hiding this comment.
Is this really a folderId or rather a fileId?
| } | ||
| }.getOrElse { exception -> | ||
| return when (exception) { | ||
| is CancellationException, is NetworkException -> Result.retry() |
There was a problem hiding this comment.
Why do we want to retry when there's a CancellationException?
| } | ||
| val availableBytes = StatFs(path).availableBytes | ||
| // Add a safety margin of 10% extra space | ||
| val requiredWithMargin = (requiredBytes * 110) / 100 |
There was a problem hiding this comment.
| val requiredWithMargin = (requiredBytes * 110) / 100 | |
| val requiredWithMargin = requiredBytes * 1.1 |
|
|
||
| private fun hasAvailableSpace(requiredBytes: Long): Boolean { | ||
| val path = when { | ||
| Build.VERSION.SDK_INT >= Build.VERSION_CODES.Q -> applicationContext.getExternalFilesDir(null)?.path ?: return true |
There was a problem hiding this comment.
Same writing standard
| Build.VERSION.SDK_INT >= Build.VERSION_CODES.Q -> applicationContext.getExternalFilesDir(null)?.path ?: return true | |
| SDK_INT >= 29 -> applicationContext.getExternalFilesDir(null)?.path ?: return true |
| return workManager.getWorkInfosFlow(workQuery).transform { | ||
| val workInfo = it.firstOrNull() | ||
| if (workInfo == null) { | ||
| emit(DownloadStatus.Complete) | ||
| } else if (workInfo.isPending() && isNetworkAvailableFlow.first().not()) { | ||
| emit(DownloadStatus.Paused(DownloadStatus.Paused.Reason.WaitingForNetwork)) | ||
| } else { | ||
| emit(workInfo.toDownloadStatus()) | ||
| } | ||
| } |
There was a problem hiding this comment.
Why not just use a map instead of a transform?
| onClick: () -> Unit, | ||
| ) { | ||
|
|
||
| val createdDate = transfer.createdDateTimestamp.toDateFromSeconds().format(FORMAT_DATE_TITLE) |
There was a problem hiding this comment.
As sonar pointed out, createdDate is unused
|
2 participants
No description provided.