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)
+    }
+}