From df8f5853e85cc1be42658ecaa51c537cc0eccf60 Mon Sep 17 00:00:00 2001
From: James Long <longster@gmail.com>
Date: Sun, 12 Mar 2023 19:00:15 -0400
Subject: [PATCH] Route aggregate queries in transaction grouped mode through
the correct layer to remove deleted transactions (#247)
I recently migrated my personal usage of Actual over to the open-source
version and imported a bunch of transactions. I have a _lot_ of history
in Actual, including a lot of weird edge cases like deleted split
transactions. While reconciling I noticed that my account balance shown
at the top was incorrect, even though the running balance was current.
Digging into this, I discovered that we aren't correctly handling
aggregate queries when querying transactions in the "grouped" mode.
Aggregate queries don't make sense in the "grouped" mode. Grouped means
that you want a list of transactions that include both the parent and
child transactions (when they are split). If you are summing up all the
amount, you only want to consider non-parent transactions. So we switch
it back to "inline" mode, but the way we did this previously was to
manually stitch the query together.
Even though was add SQL to ignore deleted transactions, we still
possibly include them. A child transaction may not be marked as deleted,
even though the parent transaction is deleted. When a parent transaction
is deleted, all child transactions should be considered deleted as well,
regardless of their tombstone status. This is what the
`v_transactions_internal_alive` view does. Previously we weren't going
through this view though, so we could still potentially include split
transactions even though they've been deleted.
This is little hacky, but it fixes the immediate problem. We fall back
to the inline mode by modifying the where clause, and we also adjust the
view that it queries to use the correct one.
---
packages/loot-core/src/server/aql/compiler.js | 4 +--
.../src/server/aql/schema/executors.js | 32 ++++++++++++-------
.../src/server/aql/schema/executors.test.js | 2 +-
3 files changed, 24 insertions(+), 14 deletions(-)
diff --git a/packages/loot-core/src/server/aql/compiler.js b/packages/loot-core/src/server/aql/compiler.js
index a5ad99f43..48e076ad6 100644
--- a/packages/loot-core/src/server/aql/compiler.js
+++ b/packages/loot-core/src/server/aql/compiler.js
@@ -927,7 +927,7 @@ function isAggregateFunction(expr) {
return true;
}
- return argExprs.find(ex => isAggregateFunction(ex));
+ return !!argExprs.find(ex => isAggregateFunction(ex));
}
export function isAggregateQuery(queryState) {
@@ -939,7 +939,7 @@ export function isAggregateQuery(queryState) {
return true;
}
- return queryState.selectExpressions.find(expr => {
+ return !!queryState.selectExpressions.find(expr => {
if (typeof expr !== 'string') {
let [_, value] = Object.entries(expr)[0];
return isAggregateFunction(value);
diff --git a/packages/loot-core/src/server/aql/schema/executors.js b/packages/loot-core/src/server/aql/schema/executors.js
index df3c229a9..2be475e7f 100644
--- a/packages/loot-core/src/server/aql/schema/executors.js
+++ b/packages/loot-core/src/server/aql/schema/executors.js
@@ -85,18 +85,28 @@ async function execTransactionsGrouped(
let { withDead } = queryState;
let whereDead = withDead ? '' : `AND ${sql.from}.tombstone = 0`;
+ // Aggregate queries don't make sense for a grouped transactions
+ // query. We never should include both parent and children
+ // transactions as it would duplicate amounts and the final number
+ // would never make sense. In this case, switch back to the "inline"
+ // type where only non-parent transactions are considered
if (isAggregateQuery(queryState)) {
- let allSql = `
- SELECT ${sql.select}
- FROM ${sql.from}
- ${sql.joins}
- ${sql.where} AND is_parent = 0 ${whereDead}
- ${sql.groupBy}
- ${sql.orderBy}
- ${sql.limit != null ? `LIMIT ${sql.limit}` : ''}
- ${sql.offset != null ? `OFFSET ${sql.offset}` : ''}
- `;
- return db.all(allSql);
+ let s = { ...sql };
+
+ // Modify the where to only include non-parents
+ s.where = `${s.where} AND ${s.from}.is_parent = 0`;
+
+ // We also want to exclude deleted transactions. Normally we
+ // handle this manually down below, but now that we are doing a
+ // normal query we want to rely on the view. Unfortunately, SQL
+ // has already been generated so we can't easily change the view
+ // name here; instead, we change it and map it back to the name
+ // used elsewhere in the query. Ideally we'd improve this
+ if (!withDead) {
+ s.from = 'v_transactions_internal_alive v_transactions_internal';
+ }
+
+ return execQuery(queryState, state, s, params, outputTypes);
}
let rows;
diff --git a/packages/loot-core/src/server/aql/schema/executors.test.js b/packages/loot-core/src/server/aql/schema/executors.test.js
index 655918680..1df6cf607 100644
--- a/packages/loot-core/src/server/aql/schema/executors.test.js
+++ b/packages/loot-core/src/server/aql/schema/executors.test.js
@@ -207,7 +207,7 @@ describe('transaction executors', () => {
let { data } = await runQuery(aggQuery.serialize());
- let sum = arr.reduce((sum, trans) => {
+ let sum = aliveTransactions(arr).reduce((sum, trans) => {
let amount = trans.amount || 0;
let matched = (amount < -5 || amount > -2) && trans.payee != null;
if (!trans.tombstone && !trans.is_parent && matched) {
--
GitLab