Merge pull request #6806 from nextcloud/grote/improve-documentsprovider-add-tests

Improve DocumentsProvider and add tests

scripts/analysis/findbugs-results.txt

@@ -1 +1 @@
-329
+326

src/androidTest/java/com/owncloud/android/providers/DocumentsProviderUtils.kt

@@ -0,0 +1,198 @@
+package com.owncloud.android.providers
+
+import android.content.Context
+import android.database.ContentObserver
+import android.database.Cursor
+import android.net.Uri
+import android.provider.DocumentsContract.Document.COLUMN_DOCUMENT_ID
+import android.provider.DocumentsContract.Document.COLUMN_MIME_TYPE
+import android.provider.DocumentsContract.Document.MIME_TYPE_DIR
+import android.provider.DocumentsContract.EXTRA_LOADING
+import android.provider.DocumentsContract.buildChildDocumentsUriUsingTree
+import android.provider.DocumentsContract.buildDocumentUriUsingTree
+import android.provider.DocumentsContract.buildTreeDocumentUri
+import android.provider.DocumentsContract.getDocumentId
+import android.util.Log
+import androidx.annotation.VisibleForTesting
+import androidx.documentfile.provider.DocumentFile
+import com.owncloud.android.datamodel.FileDataStorageManager
+import com.owncloud.android.datamodel.OCFile
+import com.owncloud.android.lib.common.OwnCloudClient
+import com.owncloud.android.lib.resources.files.ExistenceCheckRemoteOperation
+import com.owncloud.android.providers.DocumentsStorageProvider.DOCUMENTID_SEPARATOR
+import kotlinx.coroutines.Dispatchers
+import kotlinx.coroutines.TimeoutCancellationException
+import kotlinx.coroutines.suspendCancellableCoroutine
+import kotlinx.coroutines.withContext
+import kotlinx.coroutines.withTimeout
+import org.junit.Assert.assertArrayEquals
+import org.junit.Assert.assertEquals
+import org.junit.Assert.assertFalse
+import org.junit.Assert.assertNotNull
+import org.junit.Assert.assertTrue
+import java.io.IOException
+import java.io.InputStream
+import kotlin.coroutines.resume
+
+// Uploads can sometimes take a bit of time, so 15sec is still considered recent enough
+private const val RECENT_MILLISECONDS = 15_000
+
+object DocumentsProviderUtils {
+
+    internal fun DocumentFile.getOCFile(storageManager: FileDataStorageManager): OCFile? {
+        val id = getDocumentId(uri)
+        val separated: List<String> = id.split(DOCUMENTID_SEPARATOR.toRegex())
+        return storageManager.getFileById(separated[1].toLong())
+    }
+
+    internal fun DocumentFile.assertRegularFile(
+        name: String? = null,
+        size: Long? = null,
+        mimeType: String? = null,
+        parent: DocumentFile? = null
+    ) {
+        name?.let { assertEquals(it, this.name) }
+        assertTrue(exists())
+        assertTrue(isFile)
+        assertFalse(isDirectory)
+        assertFalse(isVirtual)
+        size?.let { assertEquals(it, length()) }
+        mimeType?.let { assertEquals(it, type) }
+        parent?.let { assertEquals(it.uri.toString(), parentFile!!.uri.toString()) }
+    }
+
+    internal fun DocumentFile.assertRegularFolder(name: String? = null, parent: DocumentFile? = null) {
+        name?.let { assertEquals(it, this.name) }
+        assertTrue(exists())
+        assertFalse(isFile)
+        assertTrue(isDirectory)
+        assertFalse(isVirtual)
+        parent?.let { assertEquals(it.uri.toString(), parentFile!!.uri.toString()) }
+    }
+
+    internal fun DocumentFile.assertRecentlyModified() {
+        val diff = System.currentTimeMillis() - lastModified()
+        assertTrue("File $name older than expected: $diff", diff < RECENT_MILLISECONDS)
+    }
+
+    internal fun assertExistsOnServer(client: OwnCloudClient, remotePath: String, shouldExit: Boolean) {
+        val result = ExistenceCheckRemoteOperation(remotePath, !shouldExit).execute(client)
+        assertTrue("$result", result.isSuccess)
+    }
+
+    internal fun assertListFilesEquals(expected: Collection<DocumentFile>, actual: Collection<DocumentFile>) {
+//        assertEquals(
+//            "Actual: ${actual.map { it.name.toString() }}",
+//            expected.map { it.uri.toString() }.apply { sorted() },
+//            actual.map { it.uri.toString() }.apply { sorted() },
+//        )
+        // FIXME replace with commented out stronger assertion above
+        //  when parallel [UploadFileOperation]s don't bring back deleted files
+        val expectedSet = HashSet<String>(expected.map { it.uri.toString() })
+        val actualSet = HashSet<String>(actual.map { it.uri.toString() })
+        assertTrue(actualSet.containsAll(expectedSet))
+        actualSet.removeAll(expectedSet)
+        actualSet.forEach {
+            Log.e("TEST", "Error: Found unexpected file: $it")
+        }
+    }
+
+    internal fun assertReadEquals(data: ByteArray, inputStream: InputStream?) {
+        assertNotNull(inputStream)
+        inputStream!!.use {
+            assertArrayEquals(data, it.readBytes())
+        }
+    }
+
+    /**
+     * Same as [DocumentFile.findFile] only that it re-queries when the first result was stale.
+     *
+     * Most documents providers including Nextcloud are listing the full directory content
+     * when querying for a specific file in a directory,
+     * so there is no point in trying to optimize the query by not listing all children.
+     */
+    suspend fun DocumentFile.findFileBlocking(context: Context, displayName: String): DocumentFile? {
+        val files = listFilesBlocking(context)
+        for (doc in files) {
+            if (displayName == doc.name) return doc
+        }
+        return null
+    }
+
+    /**
+     * Works like [DocumentFile.listFiles] except that it waits until the DocumentProvider has a result.
+     * This prevents getting an empty list even though there are children to be listed.
+     */
+    suspend fun DocumentFile.listFilesBlocking(context: Context) = withContext(Dispatchers.IO) {
+        val resolver = context.contentResolver
+        val childrenUri = buildChildDocumentsUriUsingTree(uri, getDocumentId(uri))
+        val projection = arrayOf(COLUMN_DOCUMENT_ID, COLUMN_MIME_TYPE)
+        val result = ArrayList<DocumentFile>()
+
+        try {
+            getLoadedCursor {
+                resolver.query(childrenUri, projection, null, null, null)
+            }
+        } catch (e: TimeoutCancellationException) {
+            throw IOException(e)
+        }.use { cursor ->
+            while (cursor.moveToNext()) {
+                val documentId = cursor.getString(0)
+                val isDirectory = cursor.getString(1) == MIME_TYPE_DIR
+                val file = if (isDirectory) {
+                    val treeUri = buildTreeDocumentUri(uri.authority, documentId)
+                    DocumentFile.fromTreeUri(context, treeUri)!!
+                } else {
+                    val documentUri = buildDocumentUriUsingTree(uri, documentId)
+                    DocumentFile.fromSingleUri(context, documentUri)!!
+                }
+                result.add(file)
+            }
+        }
+        result
+    }
+
+    /**
+     * Returns a cursor for the given query while ensuring that the cursor was loaded.
+     *
+     * When the SAF backend is a cloud storage provider (e.g. Nextcloud),
+     * it can happen that the query returns an outdated (e.g. empty) cursor
+     * which will only be updated in response to this query.
+     *
+     * See: https://commonsware.com/blog/2019/12/14/scoped-storage-stories-listfiles-woe.html
+     *
+     * This method uses a [suspendCancellableCoroutine] to wait for the result of a [ContentObserver]
+     * registered on the cursor in case the cursor is still loading ([EXTRA_LOADING]).
+     * If the cursor is not loading, it will be returned right away.
+     *
+     * @param timeout an optional time-out in milliseconds
+     * @throws TimeoutCancellationException if there was no result before the time-out
+     * @throws IOException if the query returns null
+     */
+    @Suppress("EXPERIMENTAL_API_USAGE")
+    @VisibleForTesting
+    internal suspend fun getLoadedCursor(timeout: Long = 15_000, query: () -> Cursor?) =
+        withTimeout(timeout) {
+            suspendCancellableCoroutine<Cursor> { cont ->
+                val cursor = query() ?: throw IOException("Initial query returned no results")
+                cont.invokeOnCancellation { cursor.close() }
+                val loading = cursor.extras.getBoolean(EXTRA_LOADING, false)
+                if (loading) {
+                    Log.e("TEST", "Cursor was loading, wait for update...")
+                    cursor.registerContentObserver(
+                        object : ContentObserver(null) {
+                            override fun onChange(selfChange: Boolean, uri: Uri?) {
+                                cursor.close()
+                                val newCursor = query()
+                                if (newCursor == null) cont.cancel(IOException("Re-query returned no results"))
+                                else cont.resume(newCursor)
+                            }
+                        }
+                    )
+                } else {
+                    // not loading, return cursor right away
+                    cont.resume(cursor)
+                }
+            }
+        }
+}

src/androidTest/java/com/owncloud/android/providers/DocumentsStorageProviderIT.kt

@@ -0,0 +1,175 @@
+package com.owncloud.android.providers
+
+import android.provider.DocumentsContract
+import android.util.Log
+import androidx.documentfile.provider.DocumentFile
+import com.owncloud.android.AbstractOnServerIT
+import com.owncloud.android.R
+import com.owncloud.android.datamodel.OCFile.ROOT_PATH
+import com.owncloud.android.providers.DocumentsProviderUtils.assertExistsOnServer
+import com.owncloud.android.providers.DocumentsProviderUtils.assertListFilesEquals
+import com.owncloud.android.providers.DocumentsProviderUtils.assertReadEquals
+import com.owncloud.android.providers.DocumentsProviderUtils.assertRecentlyModified
+import com.owncloud.android.providers.DocumentsProviderUtils.assertRegularFile
+import com.owncloud.android.providers.DocumentsProviderUtils.assertRegularFolder
+import com.owncloud.android.providers.DocumentsProviderUtils.findFileBlocking
+import com.owncloud.android.providers.DocumentsProviderUtils.getOCFile
+import com.owncloud.android.providers.DocumentsProviderUtils.listFilesBlocking
+import com.owncloud.android.providers.DocumentsStorageProvider.DOCUMENTID_SEPARATOR
+import kotlinx.coroutines.runBlocking
+import net.bytebuddy.utility.RandomString
+import org.junit.After
+import org.junit.Assert.assertEquals
+import org.junit.Assert.assertFalse
+import org.junit.Assert.assertTrue
+import org.junit.Before
+import org.junit.Test
+import kotlin.random.Random
+
+private const val MAX_FILE_NAME_LENGTH = 225
+
+class DocumentsStorageProviderIT : AbstractOnServerIT() {
+
+    private val context = targetContext
+    private val contentResolver = context.contentResolver
+    private val authority = context.getString(R.string.document_provider_authority)
+
+    private val rootFileId = storageManager.getFileByEncryptedRemotePath(ROOT_PATH).fileId
+    private val documentId = "${account.hashCode()}${DOCUMENTID_SEPARATOR}$rootFileId"
+    private val uri = DocumentsContract.buildTreeDocumentUri(authority, documentId)
+    private val rootDir get() = DocumentFile.fromTreeUri(context, uri)!!
+
+    @Before
+    fun before() {
+        // DocumentsProvider#onCreate() is called when the application is started
+        // which is *after* AbstractOnServerIT adds the accounts (when the app is freshly installed).
+        // So we need to query our roots here to ensure that the internal storage map is initialized.
+        contentResolver.query(DocumentsContract.buildRootsUri(authority), null, null, null)
+        assertTrue("Storage root does not exist", rootDir.exists())
+        assertTrue(rootDir.isDirectory)
+    }
+
+    /**
+     * Delete all files in [rootDir] after each test.
+     *
+     * We can't use [AbstractOnServerIT.after] as this is only deleting remote files.
+     */
+    @After
+    override fun after() = runBlocking {
+        rootDir.listFilesBlocking(context).forEach {
+            Log.e("TEST", "Deleting ${it.name}...")
+            it.delete()
+        }
+    }
+
+    @Test
+    fun testCreateDeleteFiles() = runBlocking {
+        // no files in root initially
+        assertListFilesEquals(emptyList(), rootDir.listFilesBlocking(context))
+
+        // create first file
+        val name1 = RandomString.make()
+        val type1 = "text/html"
+        val file1 = rootDir.createFile(type1, name1)!!
+
+        // check assumptions
+        @Suppress("ForbiddenComment")
+        file1.assertRegularFile(name1, 0L, null/* FIXME: type1 */, rootDir)
+        file1.assertRecentlyModified()
+
+        // file1 is found in root
+        assertListFilesEquals(listOf(file1), rootDir.listFilesBlocking(context).toList())
+
+        // file1 was uploaded
+        val ocFile1 = file1.getOCFile(storageManager)!!
+        assertExistsOnServer(client, ocFile1.remotePath, true)
+
+        // create second long file with long file name
+        val name2 = RandomString.make(MAX_FILE_NAME_LENGTH)
+        val type2 = "application/octet-stream"
+        val file2 = rootDir.createFile(type2, name2)!!
+
+        // file2 was uploaded
+        val ocFile2 = file2.getOCFile(storageManager)!!
+        assertExistsOnServer(client, ocFile2.remotePath, true)
+
+        // check assumptions
+        file2.assertRegularFile(name2, 0L, type2, rootDir)
+        file2.assertRecentlyModified()
+
+        // both files get listed in root
+        assertListFilesEquals(listOf(file1, file2), rootDir.listFiles().toList())
+
+        // delete first file
+        assertTrue(file1.delete())
+        assertFalse(file1.exists())
+        assertExistsOnServer(client, ocFile1.remotePath, false)
+
+        // only second file gets listed in root
+        assertListFilesEquals(listOf(file2), rootDir.listFiles().toList())
+
+        // delete also second file
+        assertTrue(file2.delete())
+        assertFalse(file2.exists())
+        assertExistsOnServer(client, ocFile2.remotePath, false)
+
+        // no more files in root
+        assertListFilesEquals(emptyList(), rootDir.listFilesBlocking(context))
+    }
+
+    @Test
+    fun testReadWriteFiles() {
+        // create random file
+        val file1 = rootDir.createFile("application/octet-stream", RandomString.make())!!
+        file1.assertRegularFile(size = 0L)
+
+        // write random bytes to file
+        @Suppress("MagicNumber")
+        val dataSize = Random.nextInt(1, 99) * 1024
+        val data1 = Random.nextBytes(dataSize)
+        contentResolver.openOutputStream(file1.uri, "wt").use {
+            it!!.write(data1)
+        }
+
+        // read back random bytes
+        assertReadEquals(data1, contentResolver.openInputStream(file1.uri))
+
+        // file size was updated correctly
+        file1.assertRegularFile(size = data1.size.toLong())
+    }
+
+    @Test
+    fun testCreateDeleteFolders() = runBlocking {
+        // create a new folder
+        val dirName1 = RandomString.make()
+        val dir1 = rootDir.createDirectory(dirName1)!!
+        dir1.assertRegularFolder(dirName1, rootDir)
+        // FIXME about a minute gets lost somewhere after CFO sets the correct time
+        @Suppress("MagicNumber")
+        assertTrue(System.currentTimeMillis() - dir1.lastModified() < 60_000)
+//        dir1.assertRecentlyModified()
+
+        // ensure folder was uploaded to server
+        val ocDir1 = dir1.getOCFile(storageManager)!!
+        assertExistsOnServer(client, ocDir1.remotePath, true)
+
+        // create file in folder
+        val file1 = dir1.createFile("text/html", RandomString.make())!!
+        file1.assertRegularFile(parent = dir1)
+        val ocFile1 = file1.getOCFile(storageManager)!!
+        assertExistsOnServer(client, ocFile1.remotePath, true)
+
+        // we find the new file in the created folder and get it in the list
+        assertEquals(file1.uri.toString(), dir1.findFileBlocking(context, file1.name!!)!!.uri.toString())
+        assertListFilesEquals(listOf(file1), dir1.listFilesBlocking(context))
+
+        // delete folder
+        dir1.delete()
+        assertFalse(dir1.exists())
+        assertExistsOnServer(client, ocDir1.remotePath, false)
+
+        // ensure file got deleted with it
+        assertFalse(file1.exists())
+        assertExistsOnServer(client, ocFile1.remotePath, false)
+    }
+}

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;
@@ -497,6 +499,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,42 +38,44 @@ 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;
 import com.owncloud.android.R;
-import com.owncloud.android.datamodel.ArbitraryDataProvider;
 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;
+import com.owncloud.android.lib.common.accounts.AccountUtils.AccountNotFoundException;
 import com.owncloud.android.lib.common.operations.RemoteOperationResult;
 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,12 @@ import java.util.concurrent.Executor;
 import java.util.concurrent.Executors;
 import java.util.concurrent.TimeUnit;
 
+import androidx.annotation.NonNull;
+import androidx.annotation.VisibleForTesting;
+
 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 {
@@ -102,7 +107,8 @@ public class DocumentsStorageProvider extends DocumentsProvider {
 
     UserAccountManager accountManager;
 
-    private static final String DOCUMENTID_SEPARATOR = "/";
+    @VisibleForTesting
+    static final String DOCUMENTID_SEPARATOR = "/";
     private static final int DOCUMENTID_PARTS = 2;
     private final SparseArray<FileDataStorageManager> rootIdToStorageManager = new SparseArray<>();
 
@@ -165,13 +171,8 @@ public class DocumentsStorageProvider extends DocumentsProvider {
 
         boolean isLoading = false;
         if (parentFolder.isExpired()) {
-            ArbitraryDataProvider arbitraryDataProvider = new ArbitraryDataProvider(getContext().getContentResolver());
-
-            final ReloadFolderDocumentTask task = new ReloadFolderDocumentTask(arbitraryDataProvider,
-                                                                               parentFolder,
-                                                                               result -> {
-                getContext().getContentResolver().notifyChange(toNotifyUri(parentFolder), null, false);
-            });
+            final ReloadFolderDocumentTask task = new ReloadFolderDocumentTask(parentFolder, result ->
+                getContext().getContentResolver().notifyChange(toNotifyUri(parentFolder), null, false));
             task.executeOnExecutor(executor);
             resultCursor.setLoadingTask(task);
             isLoading = true;
@@ -192,108 +193,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(() -> {
+        if (ocFile.isDown()) {
+            RemoteOperationResult result;
             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();
-                        }
-                    }
+                result = new SynchronizeFileOperation(ocFile, null, user, true, context)
+                    .execute(document.getClient(), document.getStorageManager());
             } catch (Exception e) {
-                    Log_OC.e(TAG, "Error syncing file", e);
-                    showToast();
+                throw getFileNotFoundExceptionWithCause("Error synchronizing file: " + ocFile.getFileName(), e);
             }
-            });
-
-            syncThread.start();
-            try {
-                syncThread.join();
-            } catch (InterruptedException e) {
-                Log.e(TAG, "Failed to wait for thread to finish", 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) {
-            try {
-                // reset last sync date to ensure we will be syncing this write to the server
-                ocFile.setLastSyncDateForData(0);
+            // 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());
-                return ParcelFileDescriptor.open(file, accessMode, handler, l -> {
-                    RemoteOperationResult result = new SynchronizeFileOperation(newFile, oldFile, user, true,
-                                                                                context)
-                        .execute(document.getClient(), document.getStorageManager());
+            try {
+                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 +342,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 +370,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 +383,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 +417,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);
         }
@@ -460,7 +457,7 @@ public class DocumentsStorageProvider extends DocumentsProvider {
         if (DocumentsContract.Document.MIME_TYPE_DIR.equalsIgnoreCase(mimeType)) {
             return createFolder(folderDocument, displayName);
         } else {
-            return createFile(folderDocument, displayName);
+            return createFile(folderDocument, displayName, mimeType);
         }
     }
 
@@ -481,6 +478,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 +489,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());
         }
 
@@ -501,7 +500,7 @@ public class DocumentsStorageProvider extends DocumentsProvider {
         return newFolder.getDocumentId();
     }
 
-    private String createFile(Document targetFolder, String displayName) throws FileNotFoundException {
+    private String createFile(Document targetFolder, String displayName, String mimeType) throws FileNotFoundException {
         Context context = getContext();
         if (context == null) {
             throw new FileNotFoundException("Context may not be null!");
@@ -527,21 +526,24 @@ 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;
 
+        // FIXME we need to update the mimeType somewhere else as well
+
         // perform the upload, no need for chunked operation as we have a empty file
         OwnCloudClient client = targetFolder.getClient();
         RemoteOperationResult result = new UploadFileRemoteOperation(emptyFile.getAbsolutePath(),
                                                                      newFilePath,
-                                                                     null,
+                                                                     mimeType,
                                                                      "",
                                                                      String.valueOf(System.currentTimeMillis() / 1000))
             .execute(client);
 
         if (!result.isSuccess()) {
+            Log_OC.e(TAG, result.toString());
             throw new FileNotFoundException("Failed to upload document with path " + newFilePath);
         }
 
@@ -556,6 +558,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 +652,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 +681,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 +714,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);
     }
@@ -723,14 +736,10 @@ public class DocumentsStorageProvider extends DocumentsProvider {
 
         private final Document folder;
         private final OnTaskFinishedCallback callback;
-        private final ArbitraryDataProvider arbitraryDataProvider;
 
-        ReloadFolderDocumentTask(ArbitraryDataProvider arbitraryDataProvider,
-                                 Document folder,
-                                 OnTaskFinishedCallback callback) {
+        ReloadFolderDocumentTask(Document folder, OnTaskFinishedCallback callback) {
             this.folder = folder;
             this.callback = callback;
-            this.arbitraryDataProvider = arbitraryDataProvider;
         }
 
         @Override
@@ -798,8 +807,7 @@ public class DocumentsStorageProvider extends DocumentsProvider {
             try {
                 OwnCloudAccount ocAccount = new OwnCloudAccount(getAccount(), MainApp.getAppContext());
                 return OwnCloudClientManagerFactory.getDefaultSingleton().getClientFor(ocAccount, getContext());
-            } catch (OperationCanceledException | IOException | AuthenticatorException |
-                com.owncloud.android.lib.common.accounts.AccountUtils.AccountNotFoundException e) {
+            } catch (OperationCanceledException | IOException | AuthenticatorException | AccountNotFoundException e) {
                 Log_OC.e(TAG, "Failed to set client", e);
             }
             return null;