Improve DocumentsProvider implementation

The initial goal was to make all operations completely synchronous,
so failures can be detected by apps using our DocumentsProvider.

However, `#openDocument()` could not be made fully synchronous,
because we need to wait for the ParcelFileDescriptor we are returning to
close before we can upload a file.
Nextcloud works with locally cached files that only get synchronized to
remote storage.

Things I have tried:

* Use the Handler from the calling I/O Thread:
  not guaranteed to have a prepared Looper,
  so can't always create Handler on that Thread
* Extend ParcelFileDescriptor and override its close() methods:
  For some reason they don't get called when the stream gets closed
* notify the content URI when the upload is complete,
  so callers can wait for the notification:
  works, but is non-standard.
  Other DPs are not doing it,
  so it requires Nextcloud specific code on caller side
  and is still hacky:
  what happens if notification doesn't come? Timeout, but for how long?
* use other ways to get a ParcelFileDescriptor on the file:
  couldn't find anything that would interface with Nextcloud's
  current architecture

Signed-off-by: Torsten Grote <t@grobox.de>

src/main/java/com/owncloud/android/files/services/FileDownloader.java

@@ -40,6 +40,7 @@ import android.util.Pair;
 
 import com.nextcloud.client.account.User;
 import com.nextcloud.client.account.UserAccountManager;
+import com.nextcloud.client.files.downloader.DownloadTask;
 import com.owncloud.android.R;
 import com.owncloud.android.authentication.AuthenticatorActivity;
 import com.owncloud.android.datamodel.FileDataStorageManager;
@@ -55,6 +56,7 @@ import com.owncloud.android.lib.common.operations.RemoteOperationResult.ResultCo
 import com.owncloud.android.lib.common.utils.Log_OC;
 import com.owncloud.android.lib.resources.files.FileUtils;
 import com.owncloud.android.operations.DownloadFileOperation;
+import com.owncloud.android.providers.DocumentsStorageProvider;
 import com.owncloud.android.ui.activity.FileActivity;
 import com.owncloud.android.ui.activity.FileDisplayActivity;
 import com.owncloud.android.ui.dialog.SendShareDialog;
@@ -495,6 +497,7 @@ public class FileDownloader extends Service
      * Updates the OC File after a successful download.
      *
      * TODO move to DownloadFileOperation
+     *  unify with code from {@link DocumentsStorageProvider} and {@link DownloadTask}.
      */
     private void saveDownloadedFile() {
         OCFile file = mStorageManager.getFileById(mCurrentDownload.getFile().getFileId());

src/main/java/com/owncloud/android/operations/UploadFileOperation.java

@@ -1287,6 +1287,10 @@ public class UploadFileOperation extends SyncOperation {
         if (file.fileExists()) {
             file = getStorageManager().getFileById(file.getFileId());
         }
+        if (file == null) {
+            // this can happen e.g. when the file gets deleted during upload
+            return;
+        }
         long syncDate = System.currentTimeMillis();
         file.setLastSyncDateForData(syncDate);
 

src/main/java/com/owncloud/android/providers/DocumentsStorageProvider.java

@@ -29,7 +29,6 @@ import android.annotation.SuppressLint;
 import android.annotation.TargetApi;
 import android.content.ContentResolver;
 import android.content.Context;
-import android.content.Intent;
 import android.content.res.AssetFileDescriptor;
 import android.database.Cursor;
 import android.graphics.Point;
@@ -39,17 +38,16 @@ import android.os.Build;
 import android.os.Bundle;
 import android.os.CancellationSignal;
 import android.os.Handler;
-import android.os.Looper;
 import android.os.ParcelFileDescriptor;
 import android.provider.DocumentsContract;
 import android.provider.DocumentsProvider;
 import android.util.Log;
 import android.util.SparseArray;
-import android.widget.Toast;
 
 import com.nextcloud.client.account.User;
 import com.nextcloud.client.account.UserAccountManager;
 import com.nextcloud.client.account.UserAccountManagerImpl;
+import com.nextcloud.client.files.downloader.DownloadTask;
 import com.nextcloud.client.preferences.AppPreferences;
 import com.nextcloud.client.preferences.AppPreferencesImpl;
 import com.owncloud.android.MainApp;
@@ -59,6 +57,8 @@ import com.owncloud.android.datamodel.FileDataStorageManager;
 import com.owncloud.android.datamodel.OCFile;
 import com.owncloud.android.datamodel.ThumbnailsCacheManager;
 import com.owncloud.android.files.services.FileDownloader;
+import com.owncloud.android.files.services.FileUploader;
+import com.owncloud.android.files.services.FileUploader.NameCollisionPolicy;
 import com.owncloud.android.lib.common.OwnCloudAccount;
 import com.owncloud.android.lib.common.OwnCloudClient;
 import com.owncloud.android.lib.common.OwnCloudClientManagerFactory;
@@ -67,14 +67,15 @@ import com.owncloud.android.lib.common.utils.Log_OC;
 import com.owncloud.android.lib.resources.files.UploadFileRemoteOperation;
 import com.owncloud.android.operations.CopyFileOperation;
 import com.owncloud.android.operations.CreateFolderOperation;
+import com.owncloud.android.operations.DownloadFileOperation;
 import com.owncloud.android.operations.MoveFileOperation;
 import com.owncloud.android.operations.RefreshFolderOperation;
 import com.owncloud.android.operations.RemoveFileOperation;
 import com.owncloud.android.operations.RenameFileOperation;
 import com.owncloud.android.operations.SynchronizeFileOperation;
-import com.owncloud.android.ui.activity.ConflictsResolveActivity;
 import com.owncloud.android.ui.activity.SettingsActivity;
 import com.owncloud.android.utils.FileStorageUtils;
+import com.owncloud.android.utils.MimeTypeUtil;
 import com.owncloud.android.utils.UriUtils;
 
 import org.nextcloud.providers.cursors.FileCursor;
@@ -90,8 +91,11 @@ import java.util.concurrent.Executor;
 import java.util.concurrent.Executors;
 import java.util.concurrent.TimeUnit;
 
+import androidx.annotation.NonNull;
+
 import static com.owncloud.android.datamodel.OCFile.PATH_SEPARATOR;
 import static com.owncloud.android.datamodel.OCFile.ROOT_PATH;
+import static com.owncloud.android.files.services.FileUploader.LOCAL_BEHAVIOUR_MOVE;
 
 @TargetApi(Build.VERSION_CODES.KITKAT)
 public class DocumentsStorageProvider extends DocumentsProvider {
@@ -192,108 +196,100 @@ public class DocumentsStorageProvider extends DocumentsProvider {
 
         Document document = toDocument(documentId);
 
-        Context context = getContext();
-        if (context == null) {
-            throw new FileNotFoundException("Context may not be null!");
-        }
+        Context context = getNonNullContext();
 
         OCFile ocFile = document.getFile();
         Account account = document.getAccount();
         final User user = accountManager.getUser(account.name).orElseThrow(RuntimeException::new); // should exist
 
-        if (!ocFile.isDown()) {
-            Intent i = new Intent(getContext(), FileDownloader.class);
-            i.putExtra(FileDownloader.EXTRA_USER, user);
-            i.putExtra(FileDownloader.EXTRA_FILE, ocFile);
-            if (android.os.Build.VERSION.SDK_INT >= android.os.Build.VERSION_CODES.O) {
-                context.startForegroundService(i);
-            } else {
-                context.startService(i);
+        if (ocFile.isDown()) {
+            RemoteOperationResult result;
+            try {
+                result = new SynchronizeFileOperation(ocFile, null, user, true, context)
+                    .execute(document.getClient(), document.getStorageManager());
+            } catch (Exception e) {
+                throw getFileNotFoundExceptionWithCause("Error synchronizing file: " + ocFile.getFileName(), e);
             }
-
-            do {
-                if (!waitOrGetCancelled(cancellationSignal)) {
-                    throw new FileNotFoundException("File with id " + documentId + " not found!");
-                }
-                ocFile = document.getFile();
-
-                if (ocFile == null) {
-                    throw new FileNotFoundException("File with id " + documentId + " not found!");
-                }
-            } while (!ocFile.isDown());
+            if (result.getCode() == RemoteOperationResult.ResultCode.SYNC_CONFLICT) {
+                // TODO show a conflict notification with a pending intent that shows a ConflictResolveDialog
+                Log_OC.w(TAG, "Conflict found: " + result);
+            } else if (!result.isSuccess()) {
+                Log_OC.e(TAG, result.toString());
+                throw new FileNotFoundException("Error synchronizing file: " + ocFile.getFileName());
+            }
+            // TODO test if this needed here
+            // block thread until file is saved
+            FileStorageUtils.checkIfFileFinishedSaving(ocFile);
         } else {
-            OCFile finalFile = ocFile;
-            Thread syncThread = new Thread(() -> {
-                try {
-                    FileDataStorageManager storageManager = new FileDataStorageManager(user.toPlatformAccount(),
-                                                                                       context.getContentResolver());
-                    RemoteOperationResult result = new SynchronizeFileOperation(finalFile, null, user,
-                                                                                true, context)
-                        .execute(storageManager, context);
-                    if (result.getCode() == RemoteOperationResult.ResultCode.SYNC_CONFLICT) {
-                        // ISSUE 5: if the user is not running the app (this is a service!),
-                        // this can be very intrusive; a notification should be preferred
-                        Intent intent = ConflictsResolveActivity.createIntent(finalFile,
-                                                                              user.toPlatformAccount(),
-                                                                              Intent.FLAG_ACTIVITY_NEW_TASK,
-                                                                              context);
-                        context.startActivity(intent);
-                    } else {
-                        FileStorageUtils.checkIfFileFinishedSaving(finalFile);
-                        if (!result.isSuccess()) {
-                            showToast();
-                        }
-                    }
-                } catch (Exception e) {
-                    Log_OC.e(TAG, "Error syncing file", e);
-                    showToast();
-                }
-            });
-
-            syncThread.start();
-            try {
-                syncThread.join();
-            } catch (InterruptedException e) {
-                Log.e(TAG, "Failed to wait for thread to finish", e);
+            DownloadFileOperation downloadFileOperation = new DownloadFileOperation(account, ocFile, context);
+            RemoteOperationResult result = downloadFileOperation.execute(document.getClient());
+            if (!result.isSuccess()) {
+                Log_OC.e(TAG, result.toString());
+                throw new FileNotFoundException("Error downloading file: " + ocFile.getFileName());
             }
+            saveDownloadedFile(document.getStorageManager(), downloadFileOperation, ocFile);
         }
 
         File file = new File(ocFile.getStoragePath());
         int accessMode = ParcelFileDescriptor.parseMode(mode);
-        boolean isWrite = mode.indexOf('w') != -1;
-
-        final OCFile oldFile = ocFile;
-        final OCFile newFile = ocFile;
+        boolean isWrite = accessMode != ParcelFileDescriptor.MODE_READ_ONLY;
 
         if (isWrite) {
+            // The calling thread is not guaranteed to have a Looper, so we can't block it with the OnCloseListener.
+            // Thus, we are unable to do a synchronous upload and have to start an asynchronous one.
+            Handler handler = new Handler(context.getMainLooper());
             try {
-                // reset last sync date to ensure we will be syncing this write to the server
-                ocFile.setLastSyncDateForData(0);
-                Handler handler = new Handler(context.getMainLooper());
-                return ParcelFileDescriptor.open(file, accessMode, handler, l -> {
-                    RemoteOperationResult result = new SynchronizeFileOperation(newFile, oldFile, user, true,
-                                                                                context)
-                        .execute(document.getClient(), document.getStorageManager());
-
-                    boolean success = result.isSuccess();
-
-                    if (!success) {
-                        Log_OC.e(TAG, "Failed to update document with id " + documentId);
+                return ParcelFileDescriptor.open(file, accessMode, handler, error -> {
+                    if (error == null) { // no error
+                        // As we can't upload the file synchronously, let's at least update its metadata here already.
+                        ocFile.setFileLength(file.length());
+                        ocFile.setModificationTimestamp(System.currentTimeMillis());
+                        document.getStorageManager().saveFile(ocFile);
+
+                        // TODO disable upload notifications as DocumentsProvider users already show them
+                        // upload file with FileUploader service (off main thread)
+                        FileUploader.uploadUpdateFile(
+                            context,
+                            account,
+                            ocFile,
+                            LOCAL_BEHAVIOUR_MOVE,
+                            NameCollisionPolicy.OVERWRITE
+                                                     );
+                    } else { // error, no upload needed
+                        Log_OC.e(TAG, "File was closed with an error: " + ocFile.getFileName(), error);
                     }
                 });
             } catch (IOException e) {
-                throw new FileNotFoundException("Failed to open/edit document with id " + documentId);
+                throw new FileNotFoundException("Failed to open document for writing " + ocFile.getFileName());
             }
         } else {
             return ParcelFileDescriptor.open(file, accessMode);
         }
     }
 
-    private void showToast() {
-        Handler handler = new Handler(Looper.getMainLooper());
-        handler.post(() -> Toast.makeText(MainApp.getAppContext(),
-                R.string.file_not_synced,
-                Toast.LENGTH_SHORT).show());
+    /**
+     * Updates the OC File after a successful download.
+     *
+     * TODO unify with code from {@link FileDownloader} and {@link DownloadTask}.
+     */
+    private void saveDownloadedFile(FileDataStorageManager storageManager, DownloadFileOperation dfo, OCFile file) {
+        long syncDate = System.currentTimeMillis();
+        file.setLastSyncDateForProperties(syncDate);
+        file.setLastSyncDateForData(syncDate);
+        file.setUpdateThumbnailNeeded(true);
+        file.setModificationTimestamp(dfo.getModificationTimestamp());
+        file.setModificationTimestampAtLastSyncForData(dfo.getModificationTimestamp());
+        file.setEtag(dfo.getEtag());
+        file.setMimeType(dfo.getMimeType());
+        String savePath = dfo.getSavePath();
+        file.setStoragePath(savePath);
+        file.setFileLength(new File(savePath).length());
+        file.setRemoteId(dfo.getFile().getRemoteId());
+        storageManager.saveFile(file);
+        if (MimeTypeUtil.isMedia(dfo.getMimeType())) {
+            FileDataStorageManager.triggerMediaScan(file.getStoragePath(), file);
+        }
+        storageManager.saveConflict(file, null);
     }
 
     @Override
@@ -349,6 +345,7 @@ public class DocumentsStorageProvider extends DocumentsProvider {
             .execute(document.getClient(), document.getStorageManager());
 
         if (!result.isSuccess()) {
+            Log_OC.e(TAG, result.toString());
             throw new FileNotFoundException("Failed to rename document with documentId " + documentId + ": " +
                                                 result.getException());
         }
@@ -376,6 +373,7 @@ public class DocumentsStorageProvider extends DocumentsProvider {
             .execute(document.getClient(), storageManager);
 
         if (!result.isSuccess()) {
+            Log_OC.e(TAG, result.toString());
             throw new FileNotFoundException("Failed to copy document with documentId " + sourceDocumentId
                                                 + " to " + targetParentDocumentId);
         }
@@ -388,6 +386,7 @@ public class DocumentsStorageProvider extends DocumentsProvider {
             .execute(targetFolder.getClient());
 
         if (!updateParent.isSuccess()) {
+            Log_OC.e(TAG, updateParent.toString());
             throw new FileNotFoundException("Failed to copy document with documentId " + sourceDocumentId
                                                 + " to " + targetParentDocumentId);
         }
@@ -421,6 +420,7 @@ public class DocumentsStorageProvider extends DocumentsProvider {
             .execute(document.getClient(), document.getStorageManager());
 
         if (!result.isSuccess()) {
+            Log_OC.e(TAG, result.toString());
             throw new FileNotFoundException("Failed to move document with documentId " + sourceDocumentId
                                                 + " to " + targetParentDocumentId);
         }
@@ -481,6 +481,7 @@ public class DocumentsStorageProvider extends DocumentsProvider {
             .execute(targetFolder.getClient(), storageManager);
 
         if (!result.isSuccess()) {
+            Log_OC.e(TAG, result.toString());
             throw new FileNotFoundException("Failed to create document with name " +
                                                 displayName + " and documentId " + targetFolder.getDocumentId());
         }
@@ -491,6 +492,7 @@ public class DocumentsStorageProvider extends DocumentsProvider {
             .execute(targetFolder.getClient());
 
         if (!updateParent.isSuccess()) {
+            Log_OC.e(TAG, updateParent.toString());
             throw new FileNotFoundException("Failed to create document with documentId " + targetFolder.getDocumentId());
         }
 
@@ -527,7 +529,7 @@ public class DocumentsStorageProvider extends DocumentsProvider {
                 throw new FileNotFoundException("File could not be created");
             }
         } catch (IOException e) {
-            throw new FileNotFoundException("File could not be created");
+            throw getFileNotFoundExceptionWithCause("File could not be created", e);
         }
 
         String newFilePath = targetFolder.getRemotePath() + displayName;
@@ -542,6 +544,7 @@ public class DocumentsStorageProvider extends DocumentsProvider {
             .execute(client);
 
         if (!result.isSuccess()) {
+            Log_OC.e(TAG, result.toString());
             throw new FileNotFoundException("Failed to upload document with path " + newFilePath);
         }
 
@@ -556,6 +559,7 @@ public class DocumentsStorageProvider extends DocumentsProvider {
             .execute(client);
 
         if (!updateParent.isSuccess()) {
+            Log_OC.e(TAG, updateParent.toString());
             throw new FileNotFoundException("Failed to create document with documentId " + targetFolder.getDocumentId());
         }
 
@@ -649,6 +653,12 @@ public class DocumentsStorageProvider extends DocumentsProvider {
         return false;
     }
 
+    private FileNotFoundException getFileNotFoundExceptionWithCause(String msg, Exception cause) {
+        FileNotFoundException e = new FileNotFoundException(msg);
+        e.initCause(cause);
+        return e;
+    }
+
     private FileDataStorageManager getStorageManager(String rootId) {
         for(int i = 0; i < rootIdToStorageManager.size(); i++) {
             FileDataStorageManager storageManager = rootIdToStorageManager.valueAt(i);
@@ -672,16 +682,6 @@ public class DocumentsStorageProvider extends DocumentsProvider {
         }
     }
 
-    private boolean waitOrGetCancelled(CancellationSignal cancellationSignal) {
-        try {
-            Thread.sleep(1000);
-        } catch (InterruptedException e) {
-            return false;
-        }
-
-        return !(cancellationSignal != null && cancellationSignal.isCanceled());
-    }
-
     private List<Document> findFiles(Document root, String query) {
         FileDataStorageManager storageManager = root.getStorageManager();
         List<Document> result = new ArrayList<>();
@@ -715,6 +715,20 @@ public class DocumentsStorageProvider extends DocumentsProvider {
         return new Document(storageManager, Long.parseLong(separated[1]));
     }
 
+    /**
+     * Returns a {@link Context} guaranteed to be non-null.
+     *
+     * @throws IllegalStateException if called before {@link #onCreate()}.
+     */
+    @NonNull
+    private Context getNonNullContext() {
+        Context context = getContext();
+        if (context == null) {
+            throw new IllegalStateException();
+        }
+        return context;
+    }
+
     public interface OnTaskFinishedCallback {
         void onTaskFinished(RemoteOperationResult result);
     }