From fe3b90a938a1a408e07520de3bd3fbd08328c48a Mon Sep 17 00:00:00 2001 From: Sonal Yadav Date: Thu, 14 Aug 2025 13:27:28 +0530 Subject: [PATCH 1/6] Fix nearby uploads by detecting retry scenarios and skipping re-uploads to Commons while preserving Wikidata P18 linking with added testing functionality. --- .../contributions/ContributionsPresenter.kt | 8 +++ .../nrw/commons/upload/worker/UploadWorker.kt | 72 ++++++++++++++++++- 2 files changed, 79 insertions(+), 1 deletion(-) diff --git a/app/src/main/java/fr/free/nrw/commons/contributions/ContributionsPresenter.kt b/app/src/main/java/fr/free/nrw/commons/contributions/ContributionsPresenter.kt index 2a36101b9..8f9dd4be4 100644 --- a/app/src/main/java/fr/free/nrw/commons/contributions/ContributionsPresenter.kt +++ b/app/src/main/java/fr/free/nrw/commons/contributions/ContributionsPresenter.kt @@ -58,6 +58,13 @@ class ContributionsPresenter @Inject internal constructor( contribution.state = Contribution.STATE_QUEUED saveContribution(contribution) } else { + // For nearby uploads, if image already exists but Wikidata linking failed, + // we should retry just the Wikidata operation instead of deleting + if (contribution.wikidataPlace != null) { + Timber.d("Nearby upload: Image exists on Commons, retrying Wikidata P18 linking") + contribution.state = Contribution.STATE_QUEUED + saveContribution(contribution) + } else { Timber.e("Contribution already exists") compositeDisposable!!.add( contributionsRepository @@ -65,6 +72,7 @@ class ContributionsPresenter @Inject internal constructor( .subscribeOn(ioThreadScheduler) .subscribe() ) + } } }) } diff --git a/app/src/main/java/fr/free/nrw/commons/upload/worker/UploadWorker.kt b/app/src/main/java/fr/free/nrw/commons/upload/worker/UploadWorker.kt index c8a1d9b98..feed1fdac 100644 --- a/app/src/main/java/fr/free/nrw/commons/upload/worker/UploadWorker.kt +++ b/app/src/main/java/fr/free/nrw/commons/upload/worker/UploadWorker.kt @@ -9,6 +9,9 @@ import android.content.Intent import android.content.pm.ServiceInfo import android.graphics.BitmapFactory import android.os.Build +import android.os.Handler +import android.os.Looper +import android.widget.Toast import androidx.core.app.NotificationCompat import androidx.core.app.NotificationManagerCompat import androidx.multidex.BuildConfig @@ -340,6 +343,38 @@ class UploadWorker( ) try { + // Check if this is a retry for a nearby upload that already succeeded on Commons + // but failed on Wikidata - identified by having retries > 0, wikidataPlace != null, + // and non-empty filename (meaning it was previously uploaded) + if (contribution.retries > 0 && contribution.wikidataPlace != null && + !contribution.media.filename.isNullOrEmpty()) { + + Timber.d("Nearby upload retry detected - filename already exists: ${contribution.media.filename}") + + // Create a minimal UploadResult directly from the saved filename + val uploadResult = UploadResult( + result = "Success", + filekey = "", + offset = 0, + filename = contribution.media.filename!! + ) + + // Skip the Commons upload and proceed directly to Wikidata operation + Timber.d("Skipping Commons upload, proceeding directly to Wikidata P18 linking") + try { + Timber.d("Retry: Directly attempting Wikidata P18 linking with existing filename") + makeWikiDataEdit(uploadResult, contribution) + Timber.d("Retry: Wikidata P18 linking succeeded!") + showSuccessNotification(contribution) + return + } catch (exception: Exception) { + Timber.e(exception, "Retry: Wikidata P18 linking failed") + contribution.state = Contribution.STATE_FAILED + contributionDao.saveSynchronous(contribution) + showFailedNotification(contribution) + throw exception // Let WorkManager handle retry + } + } // Upload the file to stash val stashUploadResult = uploadClient @@ -390,6 +425,12 @@ class UploadWorker( Timber.d( "WikiDataEdit required, making wikidata edit", ) + + // Save the filename for retry in case Wikidata operation fails + Timber.d("Saving Commons filename to media: ${uploadResult.filename}") + contribution.media.filename = uploadResult.filename + contributionDao.saveSynchronous(contribution) + makeWikiDataEdit(uploadResult, contribution) } showSuccessNotification(contribution) @@ -461,6 +502,9 @@ class UploadWorker( contributionDao.saveSynchronous(contribution) } + // For testing - set to true to enable 10-second delay before Wikidata operations + private val ENABLE_TESTING_DELAY = false + /** * Make the WikiData Edit, if applicable */ @@ -473,6 +517,20 @@ class UploadWorker( if (!contribution.hasInvalidLocation()) { var revisionID: Long? = null val p18WasSkipped = !wikiDataPlace.imageValue.isNullOrBlank() + + // Testing mechanism: Add 10-second delay before Wikidata operations to simulate network issues + if (ENABLE_TESTING_DELAY) { + Timber.d("TESTING: Adding 10-second delay before Wikidata operation") + Handler(Looper.getMainLooper()).post { + Toast.makeText(applicationContext, "Starting Wikidata operation - testing delay", Toast.LENGTH_LONG).show() + } + Thread.sleep(10000) // 10 seconds delay for testing + + Handler(Looper.getMainLooper()).post { + Toast.makeText(applicationContext, "Proceeding with Wikidata after delay", Toast.LENGTH_LONG).show() + } + } + try { if (!p18WasSkipped) { // Only set P18 if the place does not already have a picture @@ -498,7 +556,19 @@ class UploadWorker( // Always show success notification, whether P18 was set or skipped showSuccessNotification(contribution) } catch (exception: Exception) { - Timber.e(exception) + Timber.e(exception, "Wikidata P18 linking failed") + + // Mark contribution as failed so it can be retried + contribution.state = Contribution.STATE_FAILED + contributionDao.saveSynchronous(contribution) + + // Show failed notification + showFailedNotification(contribution) + + // Re-throw to allow WorkManager to retry + // If it's a network error, WorkManager will automatically retry + // Network errors: UnknownHostException, SocketException, SocketTimeoutException, etc. + throw exception } withContext(Dispatchers.Main) { From 5110c0a7c866406c4cf01eeb7a74f1183db96826 Mon Sep 17 00:00:00 2001 From: Sonal Yadav Date: Sat, 16 Aug 2025 18:29:34 +0530 Subject: [PATCH 2/6] Fix SecurityException crash when accessing picker URIs after app restart --- .../commons/upload/ImageProcessingService.kt | 31 +++++++++++++++++-- 1 file changed, 29 insertions(+), 2 deletions(-) diff --git a/app/src/main/java/fr/free/nrw/commons/upload/ImageProcessingService.kt b/app/src/main/java/fr/free/nrw/commons/upload/ImageProcessingService.kt index cbec7559f..476c926c8 100644 --- a/app/src/main/java/fr/free/nrw/commons/upload/ImageProcessingService.kt +++ b/app/src/main/java/fr/free/nrw/commons/upload/ImageProcessingService.kt @@ -129,11 +129,38 @@ class ImageProcessingService @Inject constructor( */ fun checkIfFileAlreadyExists(originalFilePath: Uri, modifiedFilePath: Uri): Single { return Single.zip( - checkDuplicateImage(inputStream = appContext.contentResolver.openInputStream(originalFilePath)!!), - checkDuplicateImage(inputStream = fileUtilsWrapper.getFileInputStream(modifiedFilePath.path)) + // Handle SecurityException for picker URIs that have lost permission + Single.defer { + try { + val inputStream = appContext.contentResolver.openInputStream(originalFilePath) + if (inputStream != null) { + checkDuplicateImage(inputStream = inputStream) + } else { + Single.just(IMAGE_OK) + } + } catch (e: SecurityException) { + Timber.w(e, "Security exception accessing picker URI - permission lost after app restart") + Single.just(IMAGE_OK) + } catch (e: Exception) { + Timber.e(e, "Error accessing original file URI") + Single.just(IMAGE_OK) + } + }, + // Handle exceptions for modified file path + Single.defer { + try { + checkDuplicateImage(inputStream = fileUtilsWrapper.getFileInputStream(modifiedFilePath.path)) + } catch (e: Exception) { + Timber.e(e, "Error accessing modified file") + Single.just(IMAGE_OK) + } + } ) { resultForOriginal, resultForDuplicate -> return@zip if (resultForOriginal == IMAGE_DUPLICATE || resultForDuplicate == IMAGE_DUPLICATE) IMAGE_DUPLICATE else IMAGE_OK + }.onErrorReturn { error -> + Timber.e(error, "Error during duplicate file check") + IMAGE_OK } } From 239bd72cf945e2433c06b21ba8dc861ebe5a31ad Mon Sep 17 00:00:00 2001 From: Sonal Yadav Date: Sun, 17 Aug 2025 10:36:08 +0530 Subject: [PATCH 3/6] Fix Crash --- .../commons/upload/ImageProcessingService.kt | 54 +++++++++---------- .../nrw/commons/upload/worker/UploadWorker.kt | 12 ++++- 2 files changed, 36 insertions(+), 30 deletions(-) diff --git a/app/src/main/java/fr/free/nrw/commons/upload/ImageProcessingService.kt b/app/src/main/java/fr/free/nrw/commons/upload/ImageProcessingService.kt index 476c926c8..98f5042a7 100644 --- a/app/src/main/java/fr/free/nrw/commons/upload/ImageProcessingService.kt +++ b/app/src/main/java/fr/free/nrw/commons/upload/ImageProcessingService.kt @@ -128,39 +128,35 @@ class ImageProcessingService @Inject constructor( * @return IMAGE_DUPLICATE or IMAGE_OK */ fun checkIfFileAlreadyExists(originalFilePath: Uri, modifiedFilePath: Uri): Single { - return Single.zip( - // Handle SecurityException for picker URIs that have lost permission - Single.defer { - try { - val inputStream = appContext.contentResolver.openInputStream(originalFilePath) - if (inputStream != null) { - checkDuplicateImage(inputStream = inputStream) - } else { - Single.just(IMAGE_OK) - } - } catch (e: SecurityException) { - Timber.w(e, "Security exception accessing picker URI - permission lost after app restart") - Single.just(IMAGE_OK) - } catch (e: Exception) { - Timber.e(e, "Error accessing original file URI") - Single.just(IMAGE_OK) + // Safely open the original (picker) URI. If permission is gone after app restart, + // skip this leg and proceed with the modified file check to avoid crashing. + val originalCheck: Single = + Single.fromCallable { appContext.contentResolver.openInputStream(originalFilePath) } + .flatMap { inputStream -> + if (inputStream != null) checkDuplicateImage(inputStream) else Single.just(IMAGE_OK) } - }, - // Handle exceptions for modified file path - Single.defer { - try { - checkDuplicateImage(inputStream = fileUtilsWrapper.getFileInputStream(modifiedFilePath.path)) - } catch (e: Exception) { - Timber.e(e, "Error accessing modified file") - Single.just(IMAGE_OK) + .onErrorReturn { t -> + Timber.w(t, "Skipping original URI duplicate check (no permission or not found)") + IMAGE_OK } - } - ) { resultForOriginal, resultForDuplicate -> + .subscribeOn(Schedulers.io()) + + // Safely open the modified file stream as well; be defensive in case the temp/cached file + // was cleaned up while the app was backgrounded. + val modifiedCheck: Single = + Single.fromCallable { fileUtilsWrapper.getFileInputStream(modifiedFilePath.path) } + .flatMap { inputStream -> + if (inputStream != null) checkDuplicateImage(inputStream) else Single.just(IMAGE_OK) + } + .onErrorReturn { t -> + Timber.w(t, "Skipping modified file duplicate check (file missing)") + IMAGE_OK + } + .subscribeOn(Schedulers.io()) + + return Single.zip(originalCheck, modifiedCheck) { resultForOriginal, resultForDuplicate -> return@zip if (resultForOriginal == IMAGE_DUPLICATE || resultForDuplicate == IMAGE_DUPLICATE) IMAGE_DUPLICATE else IMAGE_OK - }.onErrorReturn { error -> - Timber.e(error, "Error during duplicate file check") - IMAGE_OK } } diff --git a/app/src/main/java/fr/free/nrw/commons/upload/worker/UploadWorker.kt b/app/src/main/java/fr/free/nrw/commons/upload/worker/UploadWorker.kt index feed1fdac..587f05a1f 100644 --- a/app/src/main/java/fr/free/nrw/commons/upload/worker/UploadWorker.kt +++ b/app/src/main/java/fr/free/nrw/commons/upload/worker/UploadWorker.kt @@ -612,7 +612,17 @@ class UploadWorker( private fun saveIntoUploadedStatus(contribution: Contribution) { contribution.contentUri?.let { val imageSha1 = contribution.imageSHA1.toString() - val modifiedSha1 = fileUtilsWrapper.getSHA1(fileUtilsWrapper.getFileInputStream(contribution.localUri?.path)) + + // The local modified file might be gone after app restart. + // Be defensive: if the file is missing or cannot be opened, fall back to imageSha1. + val modifiedSha1: String = try { + val modifiedStream = fileUtilsWrapper.getFileInputStream(contribution.localUri?.path) + if (modifiedStream != null) fileUtilsWrapper.getSHA1(modifiedStream) else imageSha1 + } catch (e: Exception) { + Timber.w(e, "Skipping modified SHA1 calculation (file missing/unavailable)") + imageSha1 + } + CoroutineScope(Dispatchers.IO).launch { uploadedStatusDao.insertUploaded( UploadedStatus( From 8fd8dcee8eea6034a6c5c3b2c29204f3b8de7136 Mon Sep 17 00:00:00 2001 From: Sonal Yadav Date: Sun, 17 Aug 2025 18:36:24 +0530 Subject: [PATCH 4/6] Reverted the previous commit and pulled PR-6402 --- .../free/nrw/commons/filepicker/FilePicker.kt | 18 ++++++++--- .../commons/upload/ImageProcessingService.kt | 31 +++---------------- .../nrw/commons/upload/worker/UploadWorker.kt | 6 ++++ 3 files changed, 24 insertions(+), 31 deletions(-) diff --git a/app/src/main/java/fr/free/nrw/commons/filepicker/FilePicker.kt b/app/src/main/java/fr/free/nrw/commons/filepicker/FilePicker.kt index bb0a371e1..a7e3a671d 100644 --- a/app/src/main/java/fr/free/nrw/commons/filepicker/FilePicker.kt +++ b/app/src/main/java/fr/free/nrw/commons/filepicker/FilePicker.kt @@ -296,10 +296,19 @@ object FilePicker : Constants { * https://github.com/commons-app/apps-android-commons/issues/6357 */ private fun takePersistableUriPermissions(context: Context, result: ActivityResult) { - result.data?.data?.also { uri -> - val takeFlags: Int = (Intent.FLAG_GRANT_READ_URI_PERMISSION - or Intent.FLAG_GRANT_WRITE_URI_PERMISSION) - context.contentResolver.takePersistableUriPermission(uri, takeFlags) + result.data?.let { intentData -> + val takeFlags: Int = (Intent.FLAG_GRANT_READ_URI_PERMISSION) + // Persist the URI permission for all URIs in the clip data + // if multiple images are selected, + // or for the single URI if only one image is selected + intentData.clipData?.let { clipData -> + for (i in 0 until clipData.itemCount) { + context.contentResolver.takePersistableUriPermission( + clipData.getItemAt(i).uri, takeFlags) + } + } ?: intentData.data?.let { uri -> + context.contentResolver.takePersistableUriPermission(uri, takeFlags) + } } } @@ -358,6 +367,7 @@ object FilePicker : Constants { callbacks: Callbacks ) { if (result.resultCode == Activity.RESULT_OK && !isPhoto(result.data)) { + takePersistableUriPermissions(activity, result) try { val files = getFilesFromGalleryPictures(result.data, activity) callbacks.onImagesPicked(files, ImageSource.GALLERY, restoreType(activity)) diff --git a/app/src/main/java/fr/free/nrw/commons/upload/ImageProcessingService.kt b/app/src/main/java/fr/free/nrw/commons/upload/ImageProcessingService.kt index 98f5042a7..cbec7559f 100644 --- a/app/src/main/java/fr/free/nrw/commons/upload/ImageProcessingService.kt +++ b/app/src/main/java/fr/free/nrw/commons/upload/ImageProcessingService.kt @@ -128,33 +128,10 @@ class ImageProcessingService @Inject constructor( * @return IMAGE_DUPLICATE or IMAGE_OK */ fun checkIfFileAlreadyExists(originalFilePath: Uri, modifiedFilePath: Uri): Single { - // Safely open the original (picker) URI. If permission is gone after app restart, - // skip this leg and proceed with the modified file check to avoid crashing. - val originalCheck: Single = - Single.fromCallable { appContext.contentResolver.openInputStream(originalFilePath) } - .flatMap { inputStream -> - if (inputStream != null) checkDuplicateImage(inputStream) else Single.just(IMAGE_OK) - } - .onErrorReturn { t -> - Timber.w(t, "Skipping original URI duplicate check (no permission or not found)") - IMAGE_OK - } - .subscribeOn(Schedulers.io()) - - // Safely open the modified file stream as well; be defensive in case the temp/cached file - // was cleaned up while the app was backgrounded. - val modifiedCheck: Single = - Single.fromCallable { fileUtilsWrapper.getFileInputStream(modifiedFilePath.path) } - .flatMap { inputStream -> - if (inputStream != null) checkDuplicateImage(inputStream) else Single.just(IMAGE_OK) - } - .onErrorReturn { t -> - Timber.w(t, "Skipping modified file duplicate check (file missing)") - IMAGE_OK - } - .subscribeOn(Schedulers.io()) - - return Single.zip(originalCheck, modifiedCheck) { resultForOriginal, resultForDuplicate -> + return Single.zip( + checkDuplicateImage(inputStream = appContext.contentResolver.openInputStream(originalFilePath)!!), + checkDuplicateImage(inputStream = fileUtilsWrapper.getFileInputStream(modifiedFilePath.path)) + ) { resultForOriginal, resultForDuplicate -> return@zip if (resultForOriginal == IMAGE_DUPLICATE || resultForDuplicate == IMAGE_DUPLICATE) IMAGE_DUPLICATE else IMAGE_OK } diff --git a/app/src/main/java/fr/free/nrw/commons/upload/worker/UploadWorker.kt b/app/src/main/java/fr/free/nrw/commons/upload/worker/UploadWorker.kt index 587f05a1f..58ffefe73 100644 --- a/app/src/main/java/fr/free/nrw/commons/upload/worker/UploadWorker.kt +++ b/app/src/main/java/fr/free/nrw/commons/upload/worker/UploadWorker.kt @@ -434,6 +434,12 @@ class UploadWorker( makeWikiDataEdit(uploadResult, contribution) } showSuccessNotification(contribution) + if (appContext.contentResolver.persistedUriPermissions.any { + it.uri == contribution.contentUri }) { + appContext.contentResolver.releasePersistableUriPermission( + contribution.contentUri!!, Intent.FLAG_GRANT_READ_URI_PERMISSION + ) + } } else { Timber.e("Stash Upload failed") showFailedNotification(contribution) From 5eeddbd9a32e46cd94ba56c70357a635f56d71b0 Mon Sep 17 00:00:00 2001 From: Sonal Yadav Date: Sun, 17 Aug 2025 18:45:44 +0530 Subject: [PATCH 5/6] revert changes --- .../free/nrw/commons/upload/worker/UploadWorker.kt | 12 +----------- 1 file changed, 1 insertion(+), 11 deletions(-) diff --git a/app/src/main/java/fr/free/nrw/commons/upload/worker/UploadWorker.kt b/app/src/main/java/fr/free/nrw/commons/upload/worker/UploadWorker.kt index 58ffefe73..0bf54de2d 100644 --- a/app/src/main/java/fr/free/nrw/commons/upload/worker/UploadWorker.kt +++ b/app/src/main/java/fr/free/nrw/commons/upload/worker/UploadWorker.kt @@ -618,17 +618,7 @@ class UploadWorker( private fun saveIntoUploadedStatus(contribution: Contribution) { contribution.contentUri?.let { val imageSha1 = contribution.imageSHA1.toString() - - // The local modified file might be gone after app restart. - // Be defensive: if the file is missing or cannot be opened, fall back to imageSha1. - val modifiedSha1: String = try { - val modifiedStream = fileUtilsWrapper.getFileInputStream(contribution.localUri?.path) - if (modifiedStream != null) fileUtilsWrapper.getSHA1(modifiedStream) else imageSha1 - } catch (e: Exception) { - Timber.w(e, "Skipping modified SHA1 calculation (file missing/unavailable)") - imageSha1 - } - + val modifiedSha1 = fileUtilsWrapper.getSHA1(fileUtilsWrapper.getFileInputStream(contribution.localUri?.path)) CoroutineScope(Dispatchers.IO).launch { uploadedStatusDao.insertUploaded( UploadedStatus( From 7f0aabf368784a48a4358cc98fd5746a863122e3 Mon Sep 17 00:00:00 2001 From: Sonal Yadav Date: Fri, 22 Aug 2025 07:47:47 +0530 Subject: [PATCH 6/6] Guard getMedia finalize path from ids=M0 and make uploaded status save resilient on retry --- .../fr/free/nrw/commons/media/MediaClient.kt | 16 ++++++++++++++-- .../nrw/commons/upload/worker/UploadWorker.kt | 13 ++++++++++++- 2 files changed, 26 insertions(+), 3 deletions(-) diff --git a/app/src/main/java/fr/free/nrw/commons/media/MediaClient.kt b/app/src/main/java/fr/free/nrw/commons/media/MediaClient.kt index 3728a7202..f3789ded9 100644 --- a/app/src/main/java/fr/free/nrw/commons/media/MediaClient.kt +++ b/app/src/main/java/fr/free/nrw/commons/media/MediaClient.kt @@ -207,13 +207,25 @@ class MediaClient if (pages.isEmpty()) { Single.just(emptyList()) } else { + // If any pageId is invalid (0 or negative), avoid fetching entities like M0 + // and convert using a blank entity to prevent wbgetentities no-such-entity errors. + if (pages.any { it.pageId() <= 0 }) { + Single.just( + pages.mapNotNull { page -> + page.imageInfo()?.let { imageInfo -> + mediaConverter.convert(page, Entities.Entity(), imageInfo) + } + }, + ) + } else { getEntities(pages.map { "$PAGE_ID_PREFIX${it.pageId()}" }) .map { pages .zip(it.entities().values) .mapNotNull { (page, entity) -> - page.imageInfo()?.let { - mediaConverter.convert(page, entity, it) + page.imageInfo()?.let { imageInfo -> + mediaConverter.convert(page, entity, imageInfo) + } } } } diff --git a/app/src/main/java/fr/free/nrw/commons/upload/worker/UploadWorker.kt b/app/src/main/java/fr/free/nrw/commons/upload/worker/UploadWorker.kt index 0bf54de2d..ad9fb0bd4 100644 --- a/app/src/main/java/fr/free/nrw/commons/upload/worker/UploadWorker.kt +++ b/app/src/main/java/fr/free/nrw/commons/upload/worker/UploadWorker.kt @@ -618,8 +618,16 @@ class UploadWorker( private fun saveIntoUploadedStatus(contribution: Contribution) { contribution.contentUri?.let { val imageSha1 = contribution.imageSHA1.toString() - val modifiedSha1 = fileUtilsWrapper.getSHA1(fileUtilsWrapper.getFileInputStream(contribution.localUri?.path)) + val modifiedSha1 = try { + fileUtilsWrapper.getSHA1( + fileUtilsWrapper.getFileInputStream(contribution.localUri?.path), + ) + } catch (e: Exception) { + Timber.w(e, "UploadedStatus: modified file missing/unreadable; falling back to original SHA1") + imageSha1 + } CoroutineScope(Dispatchers.IO).launch { + try { uploadedStatusDao.insertUploaded( UploadedStatus( imageSha1, @@ -628,6 +636,9 @@ class UploadWorker( true, ), ) + } catch (e: Exception) { + Timber.w(e, "UploadedStatus: insert failed; continuing") + } } } }