Handle null URI while uploading picture (#2262)

* Handle null URI while uploading picture

* Modify cache method to never return null path

* Fix tests

app/src/main/java/fr/free/nrw/commons/upload/FileUtils.java

@@ -85,7 +85,7 @@ public class FileUtils {
     static String getGeolocationOfFile(String filePath) {
 
         try {
-            ExifInterface exifInterface=new ExifInterface(filePath);
+            ExifInterface exifInterface = new ExifInterface(filePath);
             GPSExtractor imageObj = new GPSExtractor(exifInterface);
             if (imageObj.imageCoordsExists) { // If image has geolocation information in its EXIF
                 return imageObj.getCoords();
@@ -98,14 +98,27 @@ public class FileUtils {
         }
     }
 
+    static String createCopyPathAndCopy(boolean useExternalStorage,
+                                        Uri uri,
+                                        ContentResolver contentResolver,
+                                        Context context) throws IOException {
+        return useExternalStorage ? createExternalCopyPathAndCopy(uri, contentResolver) :
+                createCopyPathAndCopy(uri, context);
+    }
+
     /**
      * In older devices getPath() may fail depending on the source URI. Creating and using a copy of the file seems to work instead.
      *
      * @return path of copy
      */
-    @NonNull
+    @Nullable
-    static String createExternalCopyPathAndCopy(Uri uri, ContentResolver contentResolver) throws IOException {
+    private static String createExternalCopyPathAndCopy(Uri uri, ContentResolver contentResolver) throws IOException {
-        FileDescriptor fileDescriptor = contentResolver.openFileDescriptor(uri, "r").getFileDescriptor();
+        ParcelFileDescriptor parcelFileDescriptor = contentResolver.openFileDescriptor(uri, "r");
+        if (parcelFileDescriptor == null) {
+            return null;
+        }
+
+        FileDescriptor fileDescriptor = parcelFileDescriptor.getFileDescriptor();
         String copyPath = Environment.getExternalStorageDirectory().toString() + "/CommonsApp/" + new Date().getTime() + "." + getFileExt(uri, contentResolver);
         File newFile = new File(Environment.getExternalStorageDirectory().toString() + "/CommonsApp");
         newFile.mkdir();
@@ -119,8 +132,8 @@ public class FileUtils {
      *
      * @return path of copy
      */
-    @NonNull
+    @Nullable
-    static String createCopyPathAndCopy(Uri uri, Context context) throws IOException {
+    private static String createCopyPathAndCopy(Uri uri, Context context) throws IOException {
         FileDescriptor fileDescriptor = context.getContentResolver().openFileDescriptor(uri, "r").getFileDescriptor();
         String copyPath = context.getCacheDir().getAbsolutePath() + "/" + new Date().getTime() + "." + getFileExt(uri, context.getContentResolver());
         FileUtils.copy(fileDescriptor, copyPath);
@@ -435,13 +448,13 @@ public class FileUtils {
         return result;
     }
 
-    static String getFileExt(String fileName){
+    static String getFileExt(String fileName) {
         //Default file extension
-        String extension=".jpg";
+        String extension = ".jpg";
 
         int i = fileName.lastIndexOf('.');
         if (i > 0) {
-            extension = fileName.substring(i+1);
+            extension = fileName.substring(i + 1);
         }
         return extension;
     }

app/src/main/java/fr/free/nrw/commons/upload/FileUtilsWrapper.java

@@ -20,12 +20,14 @@ public class FileUtilsWrapper {
 
     }
 
-    public String createExternalCopyPathAndCopy(Uri uri, ContentResolver contentResolver) throws IOException {
+    public String createCopyPathAndCopy(boolean useExtStorage,
-        return FileUtils.createExternalCopyPathAndCopy(uri, contentResolver);
+                                        Uri uri,
-    }
+                                        ContentResolver contentResolver,
-
+                                        Context context) throws IOException {
-    public String createCopyPathAndCopy(Uri uri, Context context) throws IOException {
+        return FileUtils.createCopyPathAndCopy(useExtStorage,
-        return FileUtils.createCopyPathAndCopy(uri, context);
+                uri,
+                contentResolver,
+                context);
     }
 
     public String getFileExt(String fileName) {

app/src/main/java/fr/free/nrw/commons/upload/UploadActivity.java

@@ -71,7 +71,6 @@ import static fr.free.nrw.commons.wikidata.WikidataConstants.WIKIDATA_ENTITY_ID_
 import static fr.free.nrw.commons.wikidata.WikidataConstants.WIKIDATA_ITEM_LOCATION;
 
 public class UploadActivity extends AuthenticatedActivity implements UploadView, SimilarImageInterface {
-    @Inject InputMethodManager inputMethodManager;
     @Inject MediaWikiApi mwApi;
     @Inject @Named("direct_nearby_upload_prefs") SharedPreferences directPrefs;
     @Inject UploadPresenter presenter;
@@ -613,6 +612,10 @@ public class UploadActivity extends AuthenticatedActivity implements UploadView,
 
         if (Intent.ACTION_SEND.equals(intent.getAction())) {
             Uri mediaUri = intent.getParcelableExtra(Intent.EXTRA_STREAM);
+            if(mediaUri == null) {
+                handleNullMedia();
+                return;
+            }
             if (intent.getBooleanExtra("isDirectUpload", false)) {
                 String imageTitle = directPrefs.getString("Title", "");
                 String imageDesc = directPrefs.getString("Desc", "");
@@ -627,10 +630,24 @@ public class UploadActivity extends AuthenticatedActivity implements UploadView,
         } else if (Intent.ACTION_SEND_MULTIPLE.equals(intent.getAction())) {
             ArrayList<Uri> urisList = intent.getParcelableArrayListExtra(Intent.EXTRA_STREAM);
             Timber.i("Received multiple upload %s", urisList.size());
+
+            if(urisList.isEmpty()) {
+                handleNullMedia();
+                return;
+            }
             presenter.receive(urisList, mimeType, source);
         }
     }
 
+    /**
+     * Handle null URI from the received intent.
+     * Current implementation will simply show a toast and finish the upload activity.
+     */
+    private void handleNullMedia() {
+        ViewUtil.showLongToast(this, R.string.error_processing_image);
+        finish();
+    }
+
     private void updateCardState(boolean state, ImageView button, View... content) {
         button.animate().rotation(button.getRotation() + (state ? 180 : -180)).start();
         if (content != null) {

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

@@ -27,6 +27,7 @@ import fr.free.nrw.commons.settings.Prefs;
 import fr.free.nrw.commons.utils.BitmapRegionDecoderWrapper;
 import fr.free.nrw.commons.utils.ImageUtils;
 import fr.free.nrw.commons.utils.ImageUtilsWrapper;
+import fr.free.nrw.commons.utils.StringUtils;
 import io.reactivex.Observable;
 import io.reactivex.Single;
 import io.reactivex.disposables.Disposable;
@@ -99,7 +100,7 @@ public class UploadModel {
         initDefaultValues();
         Observable<UploadItem> itemObservable = Observable.fromIterable(mediaUri)
                 .map(media -> {
-                    currentMediaUri=media;
+                    currentMediaUri = media;
                     return cacheFileUpload(media);
                 })
                 .map(filePath -> {
@@ -107,7 +108,7 @@ public class UploadModel {
                     Uri uri = Uri.fromFile(new File(filePath));
                     fileProcessor.initFileDetails(filePath, context.getContentResolver());
                     UploadItem item = new UploadItem(uri, mimeType, source, fileProcessor.processFileCoordinates(similarImageInterface),
-                            fileUtilsWrapper.getFileExt(filePath), null,fileCreatedDate);
+                            fileUtilsWrapper.getFileExt(filePath), null, fileCreatedDate);
                     Single.zip(
                             Single.fromCallable(() ->
                                     fileUtilsWrapper.getFileInputStream(filePath))
@@ -136,7 +137,7 @@ public class UploadModel {
         Uri uri = Uri.fromFile(new File(filePath));
         fileProcessor.initFileDetails(filePath, context.getContentResolver());
         UploadItem item = new UploadItem(uri, mimeType, source, fileProcessor.processFileCoordinates(similarImageInterface),
-                fileUtilsWrapper.getFileExt(filePath), wikidataEntityIdPref,fileCreatedDate);
+                fileUtilsWrapper.getFileExt(filePath), wikidataEntityIdPref, fileCreatedDate);
         item.title.setTitleText(title);
         item.descriptions.get(0).setDescriptionText(desc);
         //TODO figure out if default descriptions in other languages exist
@@ -149,7 +150,7 @@ public class UploadModel {
                         .map(b -> b ? ImageUtils.IMAGE_DUPLICATE : ImageUtils.IMAGE_OK),
                 Single.fromCallable(() -> filePath)
                         .map(fileUtilsWrapper::getGeolocationOfFile)
-                        .map(geoLocation -> imageUtilsWrapper.checkImageGeolocationIsDifferent(geoLocation,wikidataItemLocation))
+                        .map(geoLocation -> imageUtilsWrapper.checkImageGeolocationIsDifferent(geoLocation, wikidataItemLocation))
                         .map(r -> r ? ImageUtils.IMAGE_GEOLOCATION_DIFFERENT : ImageUtils.IMAGE_OK),
                 Single.fromCallable(() ->
                         fileUtilsWrapper.getFileInputStream(filePath))
@@ -171,6 +172,7 @@ public class UploadModel {
 
     /**
      * Get file creation date from uri from all possible content providers
+     *
      * @param media
      * @return
      */
@@ -354,23 +356,25 @@ public class UploadModel {
 
     /**
      * Copy files into local storage and return file path
-     *
+     * If somehow copy fails, it returns the original path
      * @param media Uri of the file
      * @return path of the enw file
      */
     private String cacheFileUpload(Uri media) {
+        String finalFilePath;
         try {
-            String copyPath;
+            String copyFilePath = fileUtilsWrapper.createCopyPathAndCopy(useExtStorage, media, contentResolver, context);
-            if (useExtStorage)
+            Timber.i("Copied file path is %s", copyFilePath);
-                copyPath = fileUtilsWrapper.createExternalCopyPathAndCopy(media, contentResolver);
+            finalFilePath = copyFilePath;
-            else
+        } catch (Exception e) {
-                copyPath = fileUtilsWrapper.createCopyPathAndCopy(media, context);
+            Timber.w(e, "Error in copying URI %s. Using original file path instead", media.getPath());
-            Timber.i("File path is " + copyPath);
+            finalFilePath = media.getPath();
-            return copyPath;
-        } catch (IOException e) {
-            Timber.w(e, "Error in copying URI " + media.getPath());
-            return null;
         }
+
+        if (StringUtils.isNullOrWhiteSpace(finalFilePath)) {
+            finalFilePath = media.getPath();
+        }
+        return finalFilePath;
     }
 
     void keepPicture() {
@@ -421,7 +425,7 @@ public class UploadModel {
             this.gpsCoords = gpsCoords;
             this.fileExt = fileExt;
             imageQuality = BehaviorSubject.createDefault(ImageUtils.IMAGE_WAIT);
-            this.createdTimestamp=createdTimestamp;
+            this.createdTimestamp = createdTimestamp;
         }
     }
 

app/src/main/res/values/strings.xml

@@ -450,6 +450,7 @@ Upload your first media by touching the camera or gallery icon above.</string>
 
   <string name="this_function_needs_network_connection">This function requires network connection, please check your connection settings.</string>
   <string name="bad_token_error_proposed_solution">Upload failed due to issues with edit token. Please try logging out and in again. </string>
+    <string name="error_processing_image">Error occurred while processing the image. Please try again!</string>
 
   <plurals name="receiving_shared_content">
     <item quantity="one">Receiving shared content. Processing the image might take some time depending on the size of the image and your device</item>

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

@@ -64,10 +64,8 @@ class UploadModelTest {
 
         `when`(context!!.applicationContext)
                 .thenReturn(mock(Application::class.java))
-        `when`(fileUtilsWrapper!!.createCopyPathAndCopy(any(Uri::class.java), any(Context::class.java)))
+        `when`(fileUtilsWrapper!!.createCopyPathAndCopy(anyBoolean(), any(Uri::class.java), nullable(ContentResolver::class.java), any(Context::class.java)))
                 .thenReturn("file.jpg")
-        `when`(fileUtilsWrapper!!.createExternalCopyPathAndCopy(any(Uri::class.java), any(ContentResolver::class.java)))
-                .thenReturn("extFile.jpg")
         `when`(fileUtilsWrapper!!.getFileExt(anyString()))
                 .thenReturn("jpg")
         `when`(fileUtilsWrapper!!.getSHA1(any(InputStream::class.java)))
@@ -111,7 +109,7 @@ class UploadModelTest {
     fun verifyPreviousNotAvailableForDirectUpload() {
         val element = mock(Uri::class.java)
         uploadModel!!.receiveDirect(element, "image/jpeg", "external", "Q1", "Test", "Test", { _, _ -> }
-        , "")
+                , "")
         assertFalse(uploadModel!!.isPreviousAvailable)
     }