From 1fdea37c2b745c72d8c2900c8dfbe540deef226d Mon Sep 17 00:00:00 2001 From: Torsten Grote Date: Thu, 20 Aug 2020 17:23:44 -0300 Subject: [PATCH] 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 --- .../files/services/FileDownloader.java | 3 + .../operations/UploadFileOperation.java | 4 + .../providers/DocumentsStorageProvider.java | 200 ++++++++++-------- 3 files changed, 114 insertions(+), 93 deletions(-) diff --git a/src/main/java/com/owncloud/android/files/services/FileDownloader.java b/src/main/java/com/owncloud/android/files/services/FileDownloader.java index bcc22c83d7..abddc190c9 100644 --- a/src/main/java/com/owncloud/android/files/services/FileDownloader.java +++ b/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()); diff --git a/src/main/java/com/owncloud/android/operations/UploadFileOperation.java b/src/main/java/com/owncloud/android/operations/UploadFileOperation.java index d707e9ccee..2a9d509c78 100644 --- a/src/main/java/com/owncloud/android/operations/UploadFileOperation.java +++ b/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); diff --git a/src/main/java/com/owncloud/android/providers/DocumentsStorageProvider.java b/src/main/java/com/owncloud/android/providers/DocumentsStorageProvider.java index 6359b4da05..0f045e0eba 100644 --- a/src/main/java/com/owncloud/android/providers/DocumentsStorageProvider.java +++ b/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); - } - - 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()); - } 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(); + if (ocFile.isDown()) { + RemoteOperationResult result; try { - syncThread.join(); - } catch (InterruptedException e) { - Log.e(TAG, "Failed to wait for thread to finish", e); + result = new SynchronizeFileOperation(ocFile, null, user, true, context) + .execute(document.getClient(), document.getStorageManager()); + } catch (Exception e) { + throw getFileNotFoundExceptionWithCause("Error synchronizing file: " + ocFile.getFileName(), e); } + 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 { + 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()); + 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); - boolean success = result.isSuccess(); - - if (!success) { - Log_OC.e(TAG, "Failed to update document with id " + documentId); + // 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 findFiles(Document root, String query) { FileDataStorageManager storageManager = root.getStorageManager(); List 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); }