Skip to content

Commit 43dca1d

Browse files
parneet-gurayaparneet-guraya
authored
[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 <[email protected]> * fix tests Signed-off-by: parneet-guraya <[email protected]> --------- Signed-off-by: parneet-guraya <[email protected]>
1 parent 30a7f70 commit 43dca1d

File tree

6 files changed

+61
-17
lines changed

6 files changed

+61
-17
lines changed

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

Lines changed: 4 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -62,7 +62,10 @@ public Contribution getContributionsWithTitle(String title) {
6262
*/
6363
public void checkDuplicateImageAndRestartContribution(Contribution contribution) {
6464
compositeDisposable.add(uploadRepository
65-
.checkDuplicateImage(contribution.getLocalUriPath().getPath())
65+
.checkDuplicateImage(
66+
contribution.getContentUri(),
67+
contribution.getLocalUri()
68+
)
6669
.subscribeOn(ioThreadScheduler)
6770
.subscribe(imageCheckResult -> {
6871
if (imageCheckResult == IMAGE_OK) {

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

Lines changed: 3 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -1,5 +1,6 @@
11
package fr.free.nrw.commons.repository
22

3+
import android.net.Uri
34
import fr.free.nrw.commons.Media
45
import fr.free.nrw.commons.category.CategoriesModel
56
import fr.free.nrw.commons.category.CategoryItem
@@ -203,8 +204,8 @@ class UploadRepository @Inject constructor(
203204
* @param filePath file to be checked
204205
* @return IMAGE_DUPLICATE or IMAGE_OK
205206
*/
206-
fun checkDuplicateImage(filePath: String): Single<Int> {
207-
return uploadModel.checkDuplicateImage(filePath)
207+
fun checkDuplicateImage(originalFilePath: Uri, modifiedFilePath: Uri): Single<Int> {
208+
return uploadModel.checkDuplicateImage(originalFilePath, modifiedFilePath)
208209
}
209210

210211
/**

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

Lines changed: 28 additions & 9 deletions
Original file line numberDiff line numberDiff line change
@@ -1,5 +1,7 @@
11
package fr.free.nrw.commons.upload
22

3+
import android.content.Context
4+
import android.net.Uri
35
import fr.free.nrw.commons.location.LatLng
46
import fr.free.nrw.commons.media.MediaClient
57
import fr.free.nrw.commons.nearby.Place
@@ -13,7 +15,7 @@ import io.reactivex.Single
1315
import io.reactivex.schedulers.Schedulers
1416
import org.apache.commons.lang3.StringUtils
1517
import timber.log.Timber
16-
import java.io.FileInputStream
18+
import java.io.InputStream
1719
import javax.inject.Inject
1820
import javax.inject.Singleton
1921

@@ -26,7 +28,8 @@ class ImageProcessingService @Inject constructor(
2628
private val imageUtilsWrapper: ImageUtilsWrapper,
2729
private val readFBMD: ReadFBMD,
2830
private val exifReader: EXIFReader,
29-
private val mediaClient: MediaClient
31+
private val mediaClient: MediaClient,
32+
private val appContext: Context
3033
) {
3134
/**
3235
* Check image quality before upload - checks duplicate image - checks dark image - checks
@@ -47,7 +50,10 @@ class ImageProcessingService @Inject constructor(
4750
val filePath = uploadItem.mediaUri?.path
4851

4952
return Single.zip(
50-
checkDuplicateImage(filePath),
53+
checkIfFileAlreadyExists(
54+
originalFilePath = uploadItem.contentUri!!,
55+
modifiedFilePath = uploadItem.mediaUri!!
56+
),
5157
checkImageGeoLocation(uploadItem.place, filePath, inAppPictureLocation),
5258
checkDarkImage(filePath!!),
5359
checkFBMD(filePath),
@@ -114,18 +120,31 @@ class ImageProcessingService @Inject constructor(
114120
.subscribeOn(Schedulers.io())
115121
}
116122

123+
/**
124+
* Checks if file already exists using original & modified file's SHA
125+
*
126+
* @param originalFilePath original file to be checked
127+
* @param modifiedFilePath modified (after exif modifications) file to be checked
128+
* @return IMAGE_DUPLICATE or IMAGE_OK
129+
*/
130+
fun checkIfFileAlreadyExists(originalFilePath: Uri, modifiedFilePath: Uri): Single<Int> {
131+
return Single.zip(
132+
checkDuplicateImage(inputStream = appContext.contentResolver.openInputStream(originalFilePath)!!),
133+
checkDuplicateImage(inputStream = fileUtilsWrapper.getFileInputStream(modifiedFilePath.path))
134+
) { resultForOriginal, resultForDuplicate ->
135+
return@zip if (resultForOriginal == IMAGE_DUPLICATE || resultForDuplicate == IMAGE_DUPLICATE)
136+
IMAGE_DUPLICATE else IMAGE_OK
137+
}
138+
}
139+
117140
/**
118141
* Checks for duplicate image
119142
*
120143
* @param filePath file to be checked
121144
* @return IMAGE_DUPLICATE or IMAGE_OK
122145
*/
123-
fun checkDuplicateImage(filePath: String?): Single<Int> {
124-
Timber.d("Checking for duplicate image %s", filePath)
125-
return Single.fromCallable { fileUtilsWrapper.getFileInputStream(filePath) }
126-
.map { stream: FileInputStream? ->
127-
fileUtilsWrapper.getSHA1(stream)
128-
}
146+
private fun checkDuplicateImage(inputStream: InputStream): Single<Int> {
147+
return Single.fromCallable { fileUtilsWrapper.getSHA1(inputStream) }
129148
.flatMap { fileSha: String? ->
130149
mediaClient.checkFileExistsUsingSha(fileSha)
131150
}

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

Lines changed: 8 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -149,7 +149,10 @@ class PendingUploadsPresenter @Inject internal constructor(
149149
}
150150
compositeDisposable.add(
151151
uploadRepository
152-
.checkDuplicateImage(contribution.localUriPath!!.path)
152+
.checkDuplicateImage(
153+
originalFilePath = contribution.contentUri!!,
154+
modifiedFilePath = contribution.localUri!!
155+
)
153156
.subscribeOn(ioThreadScheduler)
154157
.subscribe({ imageCheckResult: Int ->
155158
if (imageCheckResult == IMAGE_OK) {
@@ -218,7 +221,10 @@ class PendingUploadsPresenter @Inject internal constructor(
218221
}
219222
compositeDisposable.add(
220223
uploadRepository
221-
.checkDuplicateImage(contribution.localUriPath!!.path)
224+
.checkDuplicateImage(
225+
originalFilePath = contribution.contentUri!!,
226+
modifiedFilePath = contribution.localUri!!
227+
)
222228
.subscribeOn(ioThreadScheduler)
223229
.subscribe { imageCheckResult: Int ->
224230
if (imageCheckResult == IMAGE_OK) {

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

Lines changed: 5 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -103,8 +103,11 @@ class UploadModel @Inject internal constructor(
103103
* @param filePath file to be checked
104104
* @return IMAGE_DUPLICATE or IMAGE_OK
105105
*/
106-
fun checkDuplicateImage(filePath: String?): Single<Int> =
107-
imageProcessingService.checkDuplicateImage(filePath)
106+
fun checkDuplicateImage(originalFilePath: Uri?, modifiedFilePath: Uri?): Single<Int> =
107+
imageProcessingService.checkIfFileAlreadyExists(
108+
originalFilePath = originalFilePath!!,
109+
modifiedFilePath = modifiedFilePath!!
110+
)
108111

109112
/**
110113
* Calls validateCaption() of ImageProcessingService to check caption of image

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

Lines changed: 13 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -1,5 +1,7 @@
11
package fr.free.nrw.commons.upload
22

3+
import android.content.ContentResolver
4+
import android.content.Context
35
import android.net.Uri
46
import fr.free.nrw.commons.location.LatLng
57
import fr.free.nrw.commons.media.MediaClient
@@ -18,6 +20,7 @@ import org.mockito.Mockito.mock
1820
import org.mockito.Mockito.`when`
1921
import org.mockito.MockitoAnnotations
2022
import java.io.FileInputStream
23+
import java.io.InputStream
2124

2225
class ImageProcessingServiceTest {
2326
@Mock
@@ -38,6 +41,9 @@ class ImageProcessingServiceTest {
3841
@Mock
3942
internal var location: LatLng? = null
4043

44+
@Mock
45+
internal lateinit var appContext: Context
46+
4147
@InjectMocks
4248
var imageProcessingService: ImageProcessingService? = null
4349

@@ -49,8 +55,10 @@ class ImageProcessingServiceTest {
4955
fun setUp() {
5056
MockitoAnnotations.openMocks(this)
5157
val mediaUri = mock(Uri::class.java)
58+
val contentUri = mock(Uri::class.java)
5259
val mockPlace = mock(Place::class.java)
5360
val mockTitle = mock(List::class.java)
61+
val contentResolver = mock(ContentResolver::class.java)
5462

5563
`when`(mockPlace.wikiDataEntityId).thenReturn("Q1")
5664
`when`(mockPlace.getLocation()).thenReturn(mock(LatLng::class.java))
@@ -59,6 +67,8 @@ class ImageProcessingServiceTest {
5967
`when`(mockTitle.isSet).thenReturn(true)*/
6068

6169
`when`(uploadItem.mediaUri).thenReturn(mediaUri)
70+
`when`(uploadItem.contentUri).thenReturn(contentUri)
71+
`when`(appContext.contentResolver).thenReturn(contentResolver)
6272
`when`(uploadItem.imageQuality).thenReturn(ImageUtils.IMAGE_WAIT)
6373

6474
`when`(uploadItem.uploadMediaDetails).thenReturn(mockTitle as MutableList<UploadMediaDetail>?)
@@ -68,7 +78,9 @@ class ImageProcessingServiceTest {
6878

6979
`when`(fileUtilsWrapper!!.getFileInputStream(ArgumentMatchers.anyString()))
7080
.thenReturn(mock(FileInputStream::class.java))
71-
`when`(fileUtilsWrapper!!.getSHA1(any(FileInputStream::class.java)))
81+
`when`(appContext.contentResolver.openInputStream(ArgumentMatchers.any()))
82+
.thenReturn(mock(InputStream::class.java))
83+
`when`(fileUtilsWrapper!!.getSHA1(any(InputStream::class.java)))
7284
.thenReturn("fileSha")
7385

7486
`when`(fileUtilsWrapper!!.getGeolocationOfFile(ArgumentMatchers.anyString(), any(LatLng::class.java)))

0 commit comments

Comments
 (0)