From c6e3e19806b58d0550c3dee656d8efd94517df00 Mon Sep 17 00:00:00 2001 From: Michael Totschnig Date: Tue, 26 Sep 2023 12:05:47 +0200 Subject: [PATCH] Fix deletion of transactions with attachments --- .../test/espresso/MyExpensesCabTest.kt | 3 + .../test/provider/AttachmentTest.kt | 80 +++++++++++-------- .../totschnig/myexpenses/db2/Repository.kt | 22 ++--- .../myexpenses/db2/RepositoryAttachments.kt | 32 ++++---- .../provider/BaseTransactionProvider.kt | 17 +++- .../provider/TransactionProvider.java | 18 ++++- .../viewmodel/TransactionEditViewModel.kt | 2 +- 7 files changed, 111 insertions(+), 63 deletions(-) diff --git a/myExpenses/src/androidTest/java/org/totschnig/myexpenses/test/espresso/MyExpensesCabTest.kt b/myExpenses/src/androidTest/java/org/totschnig/myexpenses/test/espresso/MyExpensesCabTest.kt index aab4607979..39a718bd8b 100644 --- a/myExpenses/src/androidTest/java/org/totschnig/myexpenses/test/espresso/MyExpensesCabTest.kt +++ b/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) diff --git a/myExpenses/src/androidTest/java/org/totschnig/myexpenses/test/provider/AttachmentTest.kt b/myExpenses/src/androidTest/java/org/totschnig/myexpenses/test/provider/AttachmentTest.kt index 5f9ddd56ac..c3e086971d 100644 --- a/myExpenses/src/androidTest/java/org/totschnig/myexpenses/test/provider/AttachmentTest.kt +++ b/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() } } \ No newline at end of file diff --git a/myExpenses/src/main/java/org/totschnig/myexpenses/db2/Repository.kt b/myExpenses/src/main/java/org/totschnig/myexpenses/db2/Repository.kt index c102da34b3..1100c66170 100644 --- a/myExpenses/src/main/java/org/totschnig/myexpenses/db2/Repository.kt +++ b/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() - 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 } diff --git a/myExpenses/src/main/java/org/totschnig/myexpenses/db2/RepositoryAttachments.kt b/myExpenses/src/main/java/org/totschnig/myexpenses/db2/RepositoryAttachments.kt index 07e5500e61..3493b2ce23 100644 --- a/myExpenses/src/main/java/org/totschnig/myexpenses/db2/RepositoryAttachments.kt +++ b/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) { @@ -33,20 +36,19 @@ fun Repository.addAttachments(transactionId: Long, attachments: List) { fun Repository.deleteAttachments(transactionId: Long, attachments: List) { 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 = - ArrayList().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) - } - } - } \ No newline at end of file +//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() \ No newline at end of file diff --git a/myExpenses/src/main/java/org/totschnig/myexpenses/provider/BaseTransactionProvider.kt b/myExpenses/src/main/java/org/totschnig/myexpenses/provider/BaseTransactionProvider.kt index 73b4dd5225..b19c7a21ba 100644 --- a/myExpenses/src/main/java/org/totschnig/myexpenses/provider/BaseTransactionProvider.kt +++ b/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 diff --git a/myExpenses/src/main/java/org/totschnig/myexpenses/provider/TransactionProvider.java b/myExpenses/src/main/java/org/totschnig/myexpenses/provider/TransactionProvider.java index b7ae01b9d0..0594f22eb0 100644 --- a/myExpenses/src/main/java/org/totschnig/myexpenses/provider/TransactionProvider.java +++ b/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); } /** diff --git a/myExpenses/src/main/java/org/totschnig/myexpenses/viewmodel/TransactionEditViewModel.kt b/myExpenses/src/main/java/org/totschnig/myexpenses/viewmodel/TransactionEditViewModel.kt index a001ecfee1..7bf86e8369 100644 --- a/myExpenses/src/main/java/org/totschnig/myexpenses/viewmodel/TransactionEditViewModel.kt +++ b/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)