Fix deletion of transactions with attachments

myExpenses/src/androidTest/java/org/totschnig/myexpenses/test/espresso/MyExpensesCabTest.kt

@@ -1,5 +1,6 @@
 package org.totschnig.myexpenses.test.espresso
 
+import android.net.Uri
 import androidx.compose.ui.test.*
 import androidx.test.espresso.Espresso.onView
 import androidx.test.espresso.action.ViewActions.*
@@ -13,6 +14,7 @@ import org.junit.Test
 import org.totschnig.myexpenses.R
 import org.totschnig.myexpenses.compose.TEST_TAG_CONTEXT_MENU
 import org.totschnig.myexpenses.compose.TEST_TAG_LIST
+import org.totschnig.myexpenses.db2.addAttachments
 import org.totschnig.myexpenses.model.ContribFeature
 import org.totschnig.myexpenses.model.Money
 import org.totschnig.myexpenses.model.Transaction
@@ -29,6 +31,7 @@ class MyExpensesCabTest : BaseMyExpensesTest() {
         op0.amount = Money(homeCurrency, -100L)
         op0.save(contentResolver)
         for (i in 2 until 7) {
+            repository.addAttachments(op0.id, listOf(Uri.parse("file:///android_asset/screenshot.jpg")))
             op0.amount = Money(homeCurrency, -100L * i)
             op0.date = op0.date - 10000
             op0.saveAsNew(contentResolver)

myExpenses/src/androidTest/java/org/totschnig/myexpenses/test/provider/AttachmentTest.kt

@@ -1,5 +1,6 @@
 package org.totschnig.myexpenses.test.provider
 
+import android.content.ContentUris
 import android.content.ContentValues
 import android.net.Uri
 import android.os.Bundle
@@ -24,6 +25,7 @@ import java.util.Date
 class AttachmentTest : BaseDbTest() {
 
     private var transactionId: Long = 0
+    private var attachmentId: Long = 0
 
     private val testUri: Uri
         get() = TRANSACTIONS_ATTACHMENTS_URI
@@ -37,6 +39,9 @@ class AttachmentTest : BaseDbTest() {
         val account = AccountInfo("Test account", AccountType.CASH, 0, "USD")
         val accountId = mDb.insert(TABLE_ACCOUNTS, account.contentValues)
         val transaction = TransactionInfo("Transaction 0", Date(), 0, accountId)
+        //We insert two transactions, in order to have transactionId and attachmentId with different values,
+        //so that a bug where they were mixed up, would surface
+        mDb.insert(TABLE_TRANSACTIONS, transaction.contentValues)
         transactionId = mDb.insert(TABLE_TRANSACTIONS, transaction.contentValues)
     }
 
@@ -62,14 +67,22 @@ class AttachmentTest : BaseDbTest() {
         }
     }
 
-    private fun callDelete(uri: String) {
-        assertThat(contentResolver.call(
-            TransactionProvider.DUAL_URI,
-            TransactionProvider.METHOD_DELETE_ATTACHMENTS, null, Bundle(2).apply {
-                putLong(KEY_TRANSACTIONID, transactionId)
-                putStringArray(DatabaseConstants.KEY_URI_LIST, arrayOf(uri))
-            })!!.getBoolean(KEY_RESULT)
-        ).isTrue()
+    private fun callDelete(uri: String, callDeleteMethod: Boolean) {
+        if (callDeleteMethod) {
+            assertThat(contentResolver.call(
+                TransactionProvider.DUAL_URI,
+                TransactionProvider.METHOD_DELETE_ATTACHMENTS, null, Bundle(2).apply {
+                    putLong(KEY_TRANSACTIONID, transactionId)
+                    putStringArray(DatabaseConstants.KEY_URI_LIST, arrayOf(uri))
+                })!!.getBoolean(KEY_RESULT)
+            ).isTrue()
+        } else {
+            assertThat(contentResolver.delete(
+                TransactionProvider.TRANSACTION_ATTACHMENT_SINGLE_URI(
+                    transactionId, attachmentId
+                ),  null, null
+            )).isEqualTo(1)
+        }
     }
 
     private fun expectNoAttachments() {
@@ -78,40 +91,43 @@ class AttachmentTest : BaseDbTest() {
         }
     }
 
-    fun testInsertQueryDeleteInternal() {
-        insertFixture()
-        expectStaleUris(0)
-        expectLinkedAttachment(null)
-        contentResolver.insert(testUri, ContentValues(1).apply {
-            put(KEY_TRANSACTIONID, transactionId)
-            put(KEY_URI, internalAttachmentUri)
-        })
-        expectStaleUris(0)
-        assertThat(persistedPermissions).isEmpty()
-        expectLinkedAttachment(internalAttachmentUri)
-        callDelete(internalAttachmentUri)
-        expectLinkedAttachment(null)
-        //uri should not be deleted but reported as stale
-        expectStaleUris(1)
-        expectNoAttachments()
+    fun testInsertQueryDeleteInternalCall() {
+        doTheTest(withExternalUri = false, withCall = true)
     }
 
-    fun testInsertQueryDeleteExternal() {
+    fun testInsertQueryDeleteExternalCall() {
+        doTheTest(withExternalUri = true, withCall = true)
+    }
+
+    fun testInsertQueryDeleteInternalDelete() {
+        doTheTest(withExternalUri = false, withCall = false)
+    }
+
+    fun testInsertQueryDeleteExternalDelete() {
+        doTheTest(withExternalUri = true, withCall = false)
+    }
+
+    private fun doTheTest(withExternalUri: Boolean, withCall: Boolean) {
+        val uri = if(withExternalUri) externalAttachmentUri else internalAttachmentUri
         insertFixture()
         expectStaleUris(0)
         expectLinkedAttachment(null)
-        contentResolver.insert(testUri, ContentValues(1).apply {
+        attachmentId = ContentUris.parseId(contentResolver.insert(testUri, ContentValues(1).apply {
             put(KEY_TRANSACTIONID, transactionId)
-            put(KEY_URI, externalAttachmentUri)
-        })
+            put(KEY_URI, uri)
+        })!!)
         expectStaleUris(0)
-        assertThat(persistedPermissions).containsExactly(Uri.parse(externalAttachmentUri))
-        expectLinkedAttachment(externalAttachmentUri)
-        callDelete(externalAttachmentUri)
+        if (withExternalUri) {
+            assertThat(persistedPermissions).containsExactly(Uri.parse(externalAttachmentUri))
+        } else {
+            assertThat(persistedPermissions).isEmpty()
+        }
+        expectLinkedAttachment(uri)
+        callDelete(uri, withCall)
         expectLinkedAttachment(null)
         assertThat(persistedPermissions).isEmpty()
         //uri should now be deleted
-        expectStaleUris(0)
+        expectStaleUris(if (withExternalUri) 0 else 1)
         expectNoAttachments()
     }
 }

myExpenses/src/main/java/org/totschnig/myexpenses/db2/Repository.kt

@@ -9,6 +9,7 @@ import androidx.core.database.getLongOrNull
 import org.totschnig.myexpenses.model.CurrencyContext
 import org.totschnig.myexpenses.preference.PrefHandler
 import org.totschnig.myexpenses.provider.DatabaseConstants.KEY_ACCOUNTID
+import org.totschnig.myexpenses.provider.DatabaseConstants.KEY_ATTACHMENT_ID
 import org.totschnig.myexpenses.provider.DatabaseConstants.KEY_CATID
 import org.totschnig.myexpenses.provider.DatabaseConstants.KEY_TRANSACTIONID
 import org.totschnig.myexpenses.provider.DatabaseConstants.KEY_URI
@@ -19,6 +20,7 @@ import org.totschnig.myexpenses.provider.TransactionProvider.DEBTS_URI
 import org.totschnig.myexpenses.provider.TransactionProvider.QUERY_PARAMETER_CALLER_IS_IN_BULK
 import org.totschnig.myexpenses.provider.TransactionProvider.QUERY_PARAMETER_MARK_VOID
 import org.totschnig.myexpenses.provider.TransactionProvider.TRANSACTIONS_URI
+import org.totschnig.myexpenses.provider.TransactionProvider.TRANSACTION_ATTACHMENT_SINGLE_URI
 import org.totschnig.myexpenses.provider.appendBooleanQueryParameter
 import org.totschnig.myexpenses.util.ICurrencyFormatter
 import org.totschnig.myexpenses.viewmodel.data.Debt
@@ -73,20 +75,20 @@ open class Repository @Inject constructor(
 
     fun deleteTransaction(id: Long, markAsVoid: Boolean = false, inBulk: Boolean = false): Boolean {
         val ops = ArrayList<ContentProviderOperation>()
-        loadAttachments(id).forEach {
+        loadAttachmentIds(id).forEach {
             ops.add(
-                ContentProviderOperation
-                    .newDelete(TransactionProvider.TRANSACTIONS_ATTACHMENTS_URI)
-                    .withSelection("$KEY_TRANSACTIONID = ? AND $KEY_URI = ?", arrayOf(id.toString(), it.toString()))
+                ContentProviderOperation.newDelete(TRANSACTION_ATTACHMENT_SINGLE_URI(id, it))
                     .build()
             )
         }
-        ops.add(ContentProviderOperation.newDelete(
-            ContentUris.withAppendedId(TRANSACTIONS_URI, id).buildUpon().apply {
-                if (markAsVoid) appendBooleanQueryParameter(QUERY_PARAMETER_MARK_VOID)
-                if (inBulk) appendBooleanQueryParameter(QUERY_PARAMETER_CALLER_IS_IN_BULK)
-            }.build()
-        ).build())
+        ops.add(
+            ContentProviderOperation.newDelete(
+                ContentUris.withAppendedId(TRANSACTIONS_URI, id).buildUpon().apply {
+                    if (markAsVoid) appendBooleanQueryParameter(QUERY_PARAMETER_MARK_VOID)
+                    if (inBulk) appendBooleanQueryParameter(QUERY_PARAMETER_CALLER_IS_IN_BULK)
+                }.build()
+            ).build()
+        )
         val result = contentResolver.applyBatch(TransactionProvider.AUTHORITY, ops)
         return result.size == ops.size && result.last().count == 1
     }

myExpenses/src/main/java/org/totschnig/myexpenses/db2/RepositoryAttachments.kt

@@ -1,8 +1,10 @@
 package org.totschnig.myexpenses.db2
 
+import android.annotation.SuppressLint
 import android.content.ContentProviderOperation
 import android.net.Uri
 import android.os.Bundle
+import org.totschnig.myexpenses.provider.DatabaseConstants.KEY_ATTACHMENT_ID
 import org.totschnig.myexpenses.provider.DatabaseConstants.KEY_TRANSACTIONID
 import org.totschnig.myexpenses.provider.DatabaseConstants.KEY_URI
 import org.totschnig.myexpenses.provider.DatabaseConstants.KEY_URI_LIST
@@ -13,6 +15,7 @@ import org.totschnig.myexpenses.provider.TransactionProvider.METHOD_DELETE_ATTAC
 import org.totschnig.myexpenses.provider.TransactionProvider.TRANSACTIONS_ATTACHMENTS_URI
 import org.totschnig.myexpenses.provider.asSequence
 import org.totschnig.myexpenses.provider.filter.WhereFilter
+import org.totschnig.myexpenses.provider.useAndMap
 import java.io.IOException
 
 fun Repository.addAttachments(transactionId: Long, attachments: List<Uri>) {
@@ -33,20 +36,19 @@ fun Repository.addAttachments(transactionId: Long, attachments: List<Uri>) {
 
 fun Repository.deleteAttachments(transactionId: Long, attachments: List<Uri>) {
     if (!contentResolver.call(DUAL_URI, METHOD_DELETE_ATTACHMENTS, null, Bundle(2).apply {
-        putLong(KEY_TRANSACTIONID, transactionId)
-        putStringArray(KEY_URI_LIST, attachments.map(Uri::toString).toTypedArray())
-    })!!.getBoolean(KEY_RESULT))  throw IOException("Deleting attachments failed")
+            putLong(KEY_TRANSACTIONID, transactionId)
+            putStringArray(KEY_URI_LIST, attachments.map(Uri::toString).toTypedArray())
+        })!!.getBoolean(KEY_RESULT)) throw IOException("Deleting attachments failed")
 }
 
-fun Repository.loadAttachments(transactionId: Long): ArrayList<Uri> =
-    ArrayList<Uri>().apply {
-        contentResolver.query(
-            TRANSACTIONS_ATTACHMENTS_URI,
-            null, "$KEY_TRANSACTIONID = ?", arrayOf(transactionId.toString()), null
-        )?.use { cursor ->
-            cursor.asSequence.forEach {
-                val uri = Uri.parse(it.getString(0))
-                add(uri)
-            }
-        }
-    }
+//noinspection Recycle
+fun Repository.loadAttachmentIds(transactionId: Long) = contentResolver.query(
+    TRANSACTIONS_ATTACHMENTS_URI,
+    arrayOf(KEY_ATTACHMENT_ID), "$KEY_TRANSACTIONID = ?", arrayOf(transactionId.toString()), null
+)?.useAndMap { it.getLong(0) } ?: emptyList()
+
+//noinspection Recycle
+fun Repository.loadAttachments(transactionId: Long) = contentResolver.query(
+    TRANSACTIONS_ATTACHMENTS_URI,
+    arrayOf(KEY_URI), "$KEY_TRANSACTIONID = ?", arrayOf(transactionId.toString()), null
+)?.useAndMap { Uri.parse(it.getString(0)) } ?: emptyList()

myExpenses/src/main/java/org/totschnig/myexpenses/provider/BaseTransactionProvider.kt

@@ -333,6 +333,7 @@ abstract class BaseTransactionProvider : ContentProvider() {
         protected const val ACCOUNT_ATTRIBUTES = 71
         protected const val TRANSACTION_ATTACHMENTS = 72
         protected const val ATTACHMENTS = 73
+        protected const val TRANSACTION_ID_ATTACHMENT_ID = 74
     }
 
     val homeCurrency: String
@@ -1122,6 +1123,13 @@ abstract class BaseTransactionProvider : ContentProvider() {
         arrayOf(uri)
     ).use { if (it.moveToFirst()) it.getLong(0) else null }
 
+    private fun findAttachment(db: SupportSQLiteDatabase, id: Long) = db.query(
+        TABLE_ATTACHMENTS,
+        arrayOf(KEY_URI),
+        "$KEY_ROWID = ?",
+        arrayOf(id)
+    ).use { if (it.moveToFirst()) it.getString(0) else null }
+
     fun deleteAttachments(
         db: SupportSQLiteDatabase,
         transactionId: Long,
@@ -1139,15 +1147,16 @@ abstract class BaseTransactionProvider : ContentProvider() {
         return true
     }
 
-    private fun deleteAttachment(db: SupportSQLiteDatabase, attachmentId: Long, uriString: String) {
-        val uri = Uri.parse(uriString)
+    fun deleteAttachment(db: SupportSQLiteDatabase, attachmentId: Long, uriString: String?) {
+
+        val uri = Uri.parse(uriString ?: findAttachment(db, attachmentId))
         if (uri.authority != AppDirHelper.getFileProviderAuthority(context!!)) {
             Timber.d("External, releasePersistableUriPermission")
             if (try {
                     db.delete(
                         TABLE_ATTACHMENTS,
-                        "$KEY_ROWID = ? AND $KEY_URI = ?",
-                        arrayOf(attachmentId.toString(), uriString)
+                        "$KEY_ROWID = ?",
+                        arrayOf(attachmentId)
                     )
                 } catch (e: SQLiteConstraintException) {
                     //still in use

myExpenses/src/main/java/org/totschnig/myexpenses/provider/TransactionProvider.java

@@ -217,6 +217,12 @@ public static Uri PLAN_INSTANCE_SINGLE_URI(long templateId, long instanceId) {
         .build();
   }
 
+  public static Uri TRANSACTION_ATTACHMENT_SINGLE_URI(long transactionId, long attachmentId) {
+    return ContentUris.appendId(ContentUris.appendId(
+                    TRANSACTIONS_ATTACHMENTS_URI.buildUpon(), transactionId), attachmentId)
+            .build();
+  }
+
   public static final Uri CURRENCIES_URI =
       Uri.parse("content://" + AUTHORITY + "/currencies");
   public static final Uri TRANSACTIONS_SUM_URI =
@@ -1178,6 +1184,15 @@ public int delete(@NonNull Uri uri, String where, String[] whereArgs) {
         count = db.delete(TABLE_BANKS,
                 KEY_ROWID + " = " + uri.getLastPathSegment() + prefixAnd(where), whereArgs);
       }
+      case TRANSACTION_ID_ATTACHMENT_ID -> {
+        String transactionId = uri.getPathSegments().get(2);
+        String attachmentId = uri.getPathSegments().get(3);
+        count = db.delete(TABLE_TRANSACTION_ATTACHMENTS,
+                KEY_TRANSACTIONID + " = ? AND " + KEY_ATTACHMENT_ID + " = ?",
+                new String[] { transactionId, attachmentId }
+        );
+        deleteAttachment(db, Long.parseLong(attachmentId), null);
+      }
       default -> throw unknownUri(uri);
     }
     if (uriMatch == TRANSACTIONS || (uriMatch == TRANSACTION_ID && callerIsNotInBulkOperation(uri))) {
@@ -1196,7 +1211,7 @@ public int delete(@NonNull Uri uri, String where, String[] whereArgs) {
         notifyChange(PAYEES_URI, false);
       } else if (uriMatch == UNCOMMITTED) {
         notifyChange(DEBTS_URI, false);
-      } else if (uriMatch == TRANSACTION_ATTACHMENTS) {
+      } else if (uriMatch == TRANSACTION_ID_ATTACHMENT_ID) {
         notifyChange(TRANSACTIONS_URI, false);
       }
       notifyChange(uri, uriMatch == TRANSACTION_ID);
@@ -1686,6 +1701,7 @@ public Bundle call(@NonNull String method, @Nullable String arg, @Nullable Bundl
     URI_MATCHER.addURI(AUTHORITY, "accounts/attributes", ACCOUNT_ATTRIBUTES);
     URI_MATCHER.addURI(AUTHORITY, "transactions/attachments", TRANSACTION_ATTACHMENTS);
     URI_MATCHER.addURI(AUTHORITY, "attachments", ATTACHMENTS);
+    URI_MATCHER.addURI(AUTHORITY, "transactions/attachments/#/#", TRANSACTION_ID_ATTACHMENT_ID);
   }
 
   /**

myExpenses/src/main/java/org/totschnig/myexpenses/viewmodel/TransactionEditViewModel.kt

@@ -459,7 +459,7 @@ class TransactionEditViewModel(application: Application, savedStateHandle: Saved
             emit(pair.first)
             pair.second?.takeIf { it.size > 0 }?.let { updateTags(it, false) }
             if (task == InstantiationTask.TRANSACTION) {
-                originalUris = repository.loadAttachments(transactionId)
+                originalUris = ArrayList(repository.loadAttachments(transactionId))
             }
         } ?: run {
             emit(null)