[Bug fix] Check if duplicate exist using both original and modified file's checksum (#6169)

* check original file's SHA too along with modified one

Signed-off-by: parneet-guraya <gurayaparneet@gmail.com>

* fix tests

Signed-off-by: parneet-guraya <gurayaparneet@gmail.com>

---------

Signed-off-by: parneet-guraya <gurayaparneet@gmail.com>

app/src/main/java/fr/free/nrw/commons/contributions/ContributionsPresenter.java

@@ -62,7 +62,10 @@ public class ContributionsPresenter implements UserActionListener {
      */
     public void checkDuplicateImageAndRestartContribution(Contribution contribution) {
         compositeDisposable.add(uploadRepository
-            .checkDuplicateImage(contribution.getLocalUriPath().getPath())
+            .checkDuplicateImage(
+                contribution.getContentUri(),
+                contribution.getLocalUri()
+            )
             .subscribeOn(ioThreadScheduler)
             .subscribe(imageCheckResult -> {
                 if (imageCheckResult == IMAGE_OK) {

app/src/main/java/fr/free/nrw/commons/repository/UploadRepository.kt

@@ -1,5 +1,6 @@
 package fr.free.nrw.commons.repository
 
+import android.net.Uri
 import fr.free.nrw.commons.Media
 import fr.free.nrw.commons.category.CategoriesModel
 import fr.free.nrw.commons.category.CategoryItem
@@ -203,8 +204,8 @@ class UploadRepository @Inject constructor(
      * @param filePath file to be checked
      * @return IMAGE_DUPLICATE or IMAGE_OK
      */
-    fun checkDuplicateImage(filePath: String): Single<Int> {
+    fun checkDuplicateImage(originalFilePath: Uri, modifiedFilePath: Uri): Single<Int> {
-        return uploadModel.checkDuplicateImage(filePath)
+        return uploadModel.checkDuplicateImage(originalFilePath, modifiedFilePath)
     }
 
     /**

app/src/main/java/fr/free/nrw/commons/upload/ImageProcessingService.kt

@@ -1,5 +1,7 @@
 package fr.free.nrw.commons.upload
 
+import android.content.Context
+import android.net.Uri
 import fr.free.nrw.commons.location.LatLng
 import fr.free.nrw.commons.media.MediaClient
 import fr.free.nrw.commons.nearby.Place
@@ -13,7 +15,7 @@ import io.reactivex.Single
 import io.reactivex.schedulers.Schedulers
 import org.apache.commons.lang3.StringUtils
 import timber.log.Timber
-import java.io.FileInputStream
+import java.io.InputStream
 import javax.inject.Inject
 import javax.inject.Singleton
 
@@ -26,7 +28,8 @@ class ImageProcessingService @Inject constructor(
     private val imageUtilsWrapper: ImageUtilsWrapper,
     private val readFBMD: ReadFBMD,
     private val exifReader: EXIFReader,
-    private val mediaClient: MediaClient
+    private val mediaClient: MediaClient,
+    private val appContext: Context
 ) {
     /**
      * Check image quality before upload - checks duplicate image - checks dark image - checks
@@ -47,7 +50,10 @@ class ImageProcessingService @Inject constructor(
         val filePath = uploadItem.mediaUri?.path
 
         return Single.zip(
-            checkDuplicateImage(filePath),
+            checkIfFileAlreadyExists(
+                originalFilePath = uploadItem.contentUri!!,
+                modifiedFilePath = uploadItem.mediaUri!!
+            ),
             checkImageGeoLocation(uploadItem.place, filePath, inAppPictureLocation),
             checkDarkImage(filePath!!),
             checkFBMD(filePath),
@@ -114,18 +120,31 @@ class ImageProcessingService @Inject constructor(
             .subscribeOn(Schedulers.io())
     }
 
+    /**
+     * Checks if file already exists using original & modified file's SHA
+     *
+     * @param originalFilePath original file to be checked
+     * @param modifiedFilePath modified (after exif modifications) file to be checked
+     * @return IMAGE_DUPLICATE or IMAGE_OK
+     */
+    fun checkIfFileAlreadyExists(originalFilePath: Uri, modifiedFilePath: Uri): Single<Int> {
+        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
+        }
+    }
+
     /**
      * Checks for duplicate image
      *
      * @param filePath file to be checked
      * @return IMAGE_DUPLICATE or IMAGE_OK
      */
-    fun checkDuplicateImage(filePath: String?): Single<Int> {
+    private fun checkDuplicateImage(inputStream: InputStream): Single<Int> {
-        Timber.d("Checking for duplicate image %s", filePath)
+        return Single.fromCallable { fileUtilsWrapper.getSHA1(inputStream) }
-        return Single.fromCallable { fileUtilsWrapper.getFileInputStream(filePath) }
-            .map { stream: FileInputStream? ->
-                fileUtilsWrapper.getSHA1(stream)
-            }
             .flatMap { fileSha: String? ->
                 mediaClient.checkFileExistsUsingSha(fileSha)
             }

app/src/main/java/fr/free/nrw/commons/upload/PendingUploadsPresenter.kt

@@ -149,7 +149,10 @@ class PendingUploadsPresenter @Inject internal constructor(
             }
             compositeDisposable.add(
                 uploadRepository
-                    .checkDuplicateImage(contribution.localUriPath!!.path)
+                    .checkDuplicateImage(
+                        originalFilePath = contribution.contentUri!!,
+                        modifiedFilePath = contribution.localUri!!
+                    )
                     .subscribeOn(ioThreadScheduler)
                     .subscribe({ imageCheckResult: Int ->
                         if (imageCheckResult == IMAGE_OK) {
@@ -218,7 +221,10 @@ class PendingUploadsPresenter @Inject internal constructor(
             }
             compositeDisposable.add(
                 uploadRepository
-                    .checkDuplicateImage(contribution.localUriPath!!.path)
+                    .checkDuplicateImage(
+                        originalFilePath = contribution.contentUri!!,
+                        modifiedFilePath = contribution.localUri!!
+                    )
                     .subscribeOn(ioThreadScheduler)
                     .subscribe { imageCheckResult: Int ->
                         if (imageCheckResult == IMAGE_OK) {

app/src/main/java/fr/free/nrw/commons/upload/UploadModel.kt

@@ -103,8 +103,11 @@ class UploadModel @Inject internal constructor(
      * @param filePath file to be checked
      * @return IMAGE_DUPLICATE or IMAGE_OK
      */
-    fun checkDuplicateImage(filePath: String?): Single<Int> =
+    fun checkDuplicateImage(originalFilePath: Uri?, modifiedFilePath: Uri?): Single<Int> =
-        imageProcessingService.checkDuplicateImage(filePath)
+        imageProcessingService.checkIfFileAlreadyExists(
+            originalFilePath = originalFilePath!!,
+            modifiedFilePath = modifiedFilePath!!
+        )
 
     /**
      * Calls validateCaption() of ImageProcessingService to check caption of image

app/src/test/kotlin/fr/free/nrw/commons/upload/ImageProcessingServiceTest.kt

@@ -1,5 +1,7 @@
 package fr.free.nrw.commons.upload
 
+import android.content.ContentResolver
+import android.content.Context
 import android.net.Uri
 import fr.free.nrw.commons.location.LatLng
 import fr.free.nrw.commons.media.MediaClient
@@ -18,6 +20,7 @@ import org.mockito.Mockito.mock
 import org.mockito.Mockito.`when`
 import org.mockito.MockitoAnnotations
 import java.io.FileInputStream
+import java.io.InputStream
 
 class ImageProcessingServiceTest {
     @Mock
@@ -38,6 +41,9 @@ class ImageProcessingServiceTest {
     @Mock
     internal var location: LatLng? = null
 
+    @Mock
+    internal lateinit var appContext: Context
+
     @InjectMocks
     var imageProcessingService: ImageProcessingService? = null
 
@@ -49,8 +55,10 @@ class ImageProcessingServiceTest {
     fun setUp() {
         MockitoAnnotations.openMocks(this)
         val mediaUri = mock(Uri::class.java)
+        val contentUri = mock(Uri::class.java)
         val mockPlace = mock(Place::class.java)
         val mockTitle = mock(List::class.java)
+        val contentResolver = mock(ContentResolver::class.java)
 
         `when`(mockPlace.wikiDataEntityId).thenReturn("Q1")
         `when`(mockPlace.getLocation()).thenReturn(mock(LatLng::class.java))
@@ -59,6 +67,8 @@ class ImageProcessingServiceTest {
         `when`(mockTitle.isSet).thenReturn(true)*/
 
         `when`(uploadItem.mediaUri).thenReturn(mediaUri)
+        `when`(uploadItem.contentUri).thenReturn(contentUri)
+        `when`(appContext.contentResolver).thenReturn(contentResolver)
         `when`(uploadItem.imageQuality).thenReturn(ImageUtils.IMAGE_WAIT)
 
         `when`(uploadItem.uploadMediaDetails).thenReturn(mockTitle as MutableList<UploadMediaDetail>?)
@@ -68,7 +78,9 @@ class ImageProcessingServiceTest {
 
         `when`(fileUtilsWrapper!!.getFileInputStream(ArgumentMatchers.anyString()))
             .thenReturn(mock(FileInputStream::class.java))
-        `when`(fileUtilsWrapper!!.getSHA1(any(FileInputStream::class.java)))
+        `when`(appContext.contentResolver.openInputStream(ArgumentMatchers.any()))
+            .thenReturn(mock(InputStream::class.java))
+        `when`(fileUtilsWrapper!!.getSHA1(any(InputStream::class.java)))
             .thenReturn("fileSha")
 
         `when`(fileUtilsWrapper!!.getGeolocationOfFile(ArgumentMatchers.anyString(), any(LatLng::class.java)))