perf(transactions): add kind to Transaction model and remove expensive Transfer joins in aggregations #2388

2025-06-18T02:57:35+08:00

zachgoll commented

2025-06-18 02:57:35 +08:00

(Migrated from github.com)

This PR primarily aims to improve the performance of /transactions, but serves a dual purpose:

Fix performance issues caused by expensive Transfer join queries by adding kind field to Transaction
Establish the "Transaction Kind" as a pattern to be used for near-term future concepts like transaction fees and splitting transactions

Performance Benchmarks

This PR optimizes IncomeStatement queries by eliminating complex transfer joins and replacing them with simple kind filtering, achieving significant performance improvements.

Summary

Metric	Before	After	Improvement
Production Endpoint (`/transactions`)	[COLD] 0.341 i/s [WARM] 2.180 i/s	[COLD] 0.914 i/s [WARM] 2.139 i/s	2.68x faster
CategoryStats Query (local)	0.040 i/s	34.286 i/s	857x faster
FamilyStats Query (local)	0.039 i/s	45.806 i/s	1,175x faster
Totals Query (local)	0.040 i/s	7.710 i/s	193x faster

Production Endpoint Benchmarks

Measured with derailed_benchmarks in production-like environment

`/transactions` Endpoint

Before Optimization:

Type	IPS	Deviation	Time/Iteration	Iterations	Total Time
COLD	0.341	± 0.00%	2.93 s/i	1.000	2.930439s
WARM	2.180	± 0.00%	458.82 ms/i	22.000	10.097630s

After Optimization:

Type	IPS	Deviation	Time/Iteration	Iterations	Total Time
COLD	0.914	± 0.00%	1.09 s/i	1.000	1.093832s
WARM	2.139	± 0.00%	467.47 ms/i	22.000	10.317139s

Performance issues overview

Prior to this PR, the /transactions endpoint was patched in #2372 to load faster on the "warm" path, but those fixes were temporary by nature and did not address the root cause of the slowness.

By adding a denormalized kind field to the Transaction model, our queries could be drastically simplified and we can defer the complex join logic for detecting various Transfer types to the write side (background jobs) while keeping the read side simple, indexable (was not possible before), and performant.

This PR improves /transactions significantly on the cold path.

Splits and Fees (not in PR scope)

The full enum we'll likely use moving forward:

enum :kind, {
  transfer: "transfer", # A regular, excludable transfer transaction
  loan_payment: "loan_payment", # A transfer, but we include in budgets
  split_parent: "split_parent",
  split_part: "split_part", # This is a "piece" of a transaction split
  fee: "fee", # This is a "fee" that is "linked"/"referenced" to the parent
  one_time: "one_time", # This is a one-time expense / income that should be excluded from analytics
}

The kind field largely answers two questions:

How should we display this transaction in the UI?
Should this transaction go into income statement aggregations (e.g. "currently monthly spending")

This field provides a lot of flexibility and opens us up to start incorporating concepts like splits and fees (future PR).

This PR primarily aims to improve the performance of `/transactions`, but serves a dual purpose: 1. Fix performance issues caused by expensive `Transfer` join queries by adding `kind` field to `Transaction` 2. Establish the "Transaction Kind" as a pattern to be used for near-term future concepts like transaction fees and splitting transactions ## Performance Benchmarks This PR optimizes IncomeStatement queries by eliminating complex transfer joins and replacing them with simple `kind` filtering, achieving significant performance improvements. ### Summary | Metric | Before | After | Improvement | |--------|--------|-------|-------------| | **Production Endpoint** (`/transactions`) | [COLD] 0.341 i/s<br>[WARM] 2.180 i/s | [COLD] 0.914 i/s<br>[WARM] 2.139 i/s | 2.68x faster | | **CategoryStats Query** (local) | 0.040 i/s | 34.286 i/s | 857x faster | | **FamilyStats Query** (local) | 0.039 i/s | 45.806 i/s | 1,175x faster | | **Totals Query** (local) | 0.040 i/s | 7.710 i/s | 193x faster | --- ### Production Endpoint Benchmarks *Measured with `derailed_benchmarks` in production-like environment* #### `/transactions` Endpoint **Before Optimization:** | Type | IPS | Deviation | Time/Iteration | Iterations | Total Time | |------|-----|-----------|----------------|------------|------------| | COLD | 0.341 | ± 0.00% | 2.93 s/i | 1.000 | 2.930439s | | WARM | 2.180 | ± 0.00% | 458.82 ms/i | 22.000 | 10.097630s | **After Optimization:** | Type | IPS | Deviation | Time/Iteration | Iterations | Total Time | |------|-----|-----------|----------------|------------|------------| | COLD | 0.914 | ± 0.00% | 1.09 s/i | 1.000 | 1.093832s | | WARM | 2.139 | ± 0.00% | 467.47 ms/i | 22.000 | 10.317139s | --- ## Performance issues overview Prior to this PR, the `/transactions` endpoint was patched in #2372 to load faster on the "warm" path, but those fixes were temporary by nature and did not address the root cause of the slowness. By adding a denormalized `kind` field to the `Transaction` model, our queries could be drastically simplified and we can defer the complex join logic for detecting various Transfer types to the _write side_ (background jobs) while keeping the read side simple, _indexable_ (was not possible before), and performant. This PR improves `/transactions` significantly on the cold path. ## Splits and Fees (not in PR scope) The full enum we'll likely use moving forward: ```rb enum :kind, { transfer: "transfer", # A regular, excludable transfer transaction loan_payment: "loan_payment", # A transfer, but we include in budgets split_parent: "split_parent", split_part: "split_part", # This is a "piece" of a transaction split fee: "fee", # This is a "fee" that is "linked"/"referenced" to the parent one_time: "one_time", # This is a one-time expense / income that should be excluded from analytics } ``` The `kind` field largely answers two questions: - How should we display this transaction in the UI? - Should this transaction go into income statement aggregations (e.g. "currently monthly spending") This field provides a lot of flexibility and opens us up to start incorporating concepts like splits and fees (future PR).

cursor[bot] (Migrated from github.com) reviewed 2025-06-21 00:59:07 +08:00

cursor[bot] (Migrated from github.com) left a comment

Bug: Transfer Status Inconsistency

The template inconsistently uses transaction.transfer? (enum-based kind) and transaction.transfer.present? (association presence) to determine transfer status. This can lead to UI inconsistencies such as incorrect checkbox disabled states, misdirected links (e.g., linking to entry_path instead of a transfer path), and inconsistent display of transfer-specific UI elements. The discrepancy arises when a transaction's kind indicates a transfer but no Transfer record is associated, or vice-versa, potentially causing nil errors.

app/views/transactions/_transaction.html.erb#L12-L73

ec7882b09c/app/views/transactions/_transaction.html.erb (L12-L73)

Fix in Cursor

Was this report helpful? Give feedback by reacting with 👍 or 👎

<details open> <summary><h3>Bug: Transfer Status Inconsistency</h3></summary> The template inconsistently uses `transaction.transfer?` (enum-based `kind`) and `transaction.transfer.present?` (association presence) to determine transfer status. This can lead to UI inconsistencies such as incorrect checkbox `disabled` states, misdirected links (e.g., linking to `entry_path` instead of a transfer path), and inconsistent display of transfer-specific UI elements. The discrepancy arises when a transaction's `kind` indicates a transfer but no `Transfer` record is associated, or vice-versa, potentially causing nil errors. <p></p> <details> <summary><code>app/views/transactions/_transaction.html.erb#L12-L73</code></summary> https://github.com/maybe-finance/maybe/blob/ec7882b09cea6f5afa856a5a89e5c8f321a2f2b7/app/views/transactions/_transaction.html.erb#L12-L73 </details> <a href="https://cursor.com/open?data=eyJhbGciOiJSUzI1NiIsInR5cCI6IkpXVCIsImtpZCI6ImJ1Z2JvdC12MSJ9.eyJ2ZXJzaW9uIjoxLCJ0eXBlIjoiQlVHQk9UX0ZJWF9JTl9DVVJTT1IiLCJkYXRhIjp7InJlZGlzS2V5IjoiYnVnYm90OmY0MDRhMmUyLTVmOTEtNDQ3MS1hOWFjLTVhYTA3Y2Y5YWY3OCIsImVuY3J5cHRpb25LZXkiOiJSTTFiSEpoN0RmbFJLVC10MnFQelpsMktHcnVSQ1pHOHJIcTZhcERvRWpZIiwiYnJhbmNoIjoiemFjaGdvbGwvcGVyZi10cmFuc2FjdGlvbnMtaW5kZXgifSwiaWF0IjoxNzUwNDM4NzQ3LCJleHAiOjE3NTEwNDM1NDd9.flXlP6N_4njzrJL-hmwPoaeYtQyWayFRMs3gL2UQzHUrxkifdO_HnYE328ULj3H2EEMCTU-bQ8T-aUxslhOYrDW2GVL8l3I7ZgdokGIN-WS-DWeaRWkHVL3sZ8fn5lHSpCqHjBngFia-yxWh8dtyNxlp3AJU1IKoNFO7gzrSThKuEJrAenMtkshugzxqne-EK10wM3kr_JEawde4vPByyPdmKR6eyjDdzd6WdAKxXJ2Gv5lfB9zGi8AtjwLMkykR7Gb26QS7zeLvQqj8Fl2XwezBsiSqxB5DI1-P6i7q5sdYnEn1IXmLlY24d3ikFxj1EypfPf0E9gcSePVrXDeH6g">Fix in Cursor</a> </details> --- _Was this report helpful? Give feedback by reacting with 👍 or 👎_

cursor[bot] (Migrated from github.com) reviewed 2025-06-21 01:07:18 +08:00

cursor[bot] (Migrated from github.com) left a comment

Bug: Inconsistent Transfer Detection Causes UI Issues

The view uses inconsistent logic for detecting transfers. It mixes transaction.transfer.present? (which checks the association) with transaction.transfer? (which checks the kind enum). This affects elements like the checkbox disabling and other display logic, potentially causing divergent UI behavior and negating the performance benefit of the transaction.transfer? method.

app/views/transactions/_transaction.html.erb#L11-L96

af0a2b9e92/app/views/transactions/_transaction.html.erb (L11-L96)

Fix in Cursor

Bug: Family Ownership Check Fails, Returns Nil

The set_transfer method in TransfersController incorrectly determines family ownership by only checking the inflow_transaction_id, making transfers with family-owned outflow_transaction inaccessible. Furthermore, it returns nil when a transfer is not found or not owned, causing NoMethodError in show, update, and destroy actions instead of the expected ActiveRecord::RecordNotFound (404).

app/controllers/transfers_controller.rb#L52-L58

af0a2b9e92/app/controllers/transfers_controller.rb (L52-L58)

Fix in Cursor

Was this report helpful? Give feedback by reacting with 👍 or 👎

<details open> <summary><h3>Bug: Inconsistent Transfer Detection Causes UI Issues</h3></summary> The view uses inconsistent logic for detecting transfers. It mixes `transaction.transfer.present?` (which checks the association) with `transaction.transfer?` (which checks the `kind` enum). This affects elements like the checkbox disabling and other display logic, potentially causing divergent UI behavior and negating the performance benefit of the `transaction.transfer?` method. <p></p> <details> <summary><code>app/views/transactions/_transaction.html.erb#L11-L96</code></summary> https://github.com/maybe-finance/maybe/blob/af0a2b9e92aba1c17a77c2764db1a52dba6182b1/app/views/transactions/_transaction.html.erb#L11-L96 </details> <a href="https://cursor.com/open?data=eyJhbGciOiJSUzI1NiIsInR5cCI6IkpXVCIsImtpZCI6ImJ1Z2JvdC12MSJ9.eyJ2ZXJzaW9uIjoxLCJ0eXBlIjoiQlVHQk9UX0ZJWF9JTl9DVVJTT1IiLCJkYXRhIjp7InJlZGlzS2V5IjoiYnVnYm90OjhlNzYxZWI4LTIyZDUtNDkwMS1iMGEwLTg1OTE0YzAxYWIxYyIsImVuY3J5cHRpb25LZXkiOiJ1OWVFZHlzdGFzX2w0Q2RTdE9mZktyOFpjajdVaXl2Y0I3OGc2anFYSktRIiwiYnJhbmNoIjoiemFjaGdvbGwvcGVyZi10cmFuc2FjdGlvbnMtaW5kZXgifSwiaWF0IjoxNzUwNDM5MjM4LCJleHAiOjE3NTEwNDQwMzh9.c341htTgtGG9YznExdQgZnLy7P_dAdRiAHM7_bWQHE1SM5GpvFH6LKKUiqHp9rXLKg3kFfmPPp7F5lE_BHq5BsLMITRDQJbarFEM5xDMvE-6e2UGryX7mxndmPTALCn8Msk1HmMxIoqnEvhjenHx_lKwvIeIQzEQyh0gl3Mxd57zdC98aTnFIW0KfRbkhJLCnsyl00tUhl2X2-dw2cQCr8Cs0j8Nflo0P82sQ-koypdFPHtH6EE1Xtqse0Bhye1uqndq6EMzsf4pYrhF7l3i_scdF-XYKc7NlfEBzV9CHyP_DUk53XAsJvwc7y0-qyJJy9qBwG6FVZgAz3I0EpOacQ">Fix in Cursor</a> </details> --- <details open> <summary><h3>Bug: Family Ownership Check Fails, Returns Nil</h3></summary> The `set_transfer` method in `TransfersController` incorrectly determines family ownership by only checking the `inflow_transaction_id`, making transfers with family-owned `outflow_transaction` inaccessible. Furthermore, it returns `nil` when a transfer is not found or not owned, causing `NoMethodError` in `show`, `update`, and `destroy` actions instead of the expected `ActiveRecord::RecordNotFound` (404). <p></p> <details> <summary><code>app/controllers/transfers_controller.rb#L52-L58</code></summary> https://github.com/maybe-finance/maybe/blob/af0a2b9e92aba1c17a77c2764db1a52dba6182b1/app/controllers/transfers_controller.rb#L52-L58 </details> <a href="https://cursor.com/open?data=eyJhbGciOiJSUzI1NiIsInR5cCI6IkpXVCIsImtpZCI6ImJ1Z2JvdC12MSJ9.eyJ2ZXJzaW9uIjoxLCJ0eXBlIjoiQlVHQk9UX0ZJWF9JTl9DVVJTT1IiLCJkYXRhIjp7InJlZGlzS2V5IjoiYnVnYm90OjYwODVmYTZiLTI1ZDAtNGJhMC05MzNiLWU2MGEwMjhmZjYwZiIsImVuY3J5cHRpb25LZXkiOiI3ZzJlRENmendQYndGQzlhUExXb3ZtMUlQSzdCOC1hVUtvT29BaVlUSUd3IiwiYnJhbmNoIjoiemFjaGdvbGwvcGVyZi10cmFuc2FjdGlvbnMtaW5kZXgifSwiaWF0IjoxNzUwNDM5MjM4LCJleHAiOjE3NTEwNDQwMzh9.OQsQsYP9uUreVMIP-OK4BeUq6RYjE87WUQpXZY26jvbgw2J5TGy1lOg5sK5ZPIQ8pMZjPcel1wmQeSTF5Jn44prak8NykTt6kRw7oniUpIITTYctcV4foGpxyVGYa-6QWuF54OOR-vbriydxoE4QC9n3pUmU5J3xtkRKbgu-Z8vT-GTVnCVuwOz6UULP_pkSlFKBAHPyrwi-wqQK0zkSK-UomfaikJaxrq4k_b01Jyslr8RcxPrxNakfMHiX9vGyInnRGByA-ft-3Q2_-ylI0Slqcm-6jaLJi0I9p6APyTjd37hze-3HU5972HJ6UQgEsMdBIjEP2os4paVSxTkuMg">Fix in Cursor</a> </details> --- _Was this report helpful? Give feedback by reacting with 👍 or 👎_

Sign in to join this conversation.

1 Participants

Notifications

Due Date

No due date set.

Dependencies

No dependencies set.

Reference: gavin/maybe#2388

perf(transactions): add kind to Transaction model and remove expensive Transfer joins in aggregations #2388

Performance Benchmarks

Summary

Production Endpoint Benchmarks

/transactions Endpoint

Performance issues overview

Splits and Fees (not in PR scope)

Bug: Transfer Status Inconsistency

Bug: Inconsistent Transfer Detection Causes UI Issues

Bug: Family Ownership Check Fails, Returns Nil

perf(transactions): add `kind` to `Transaction` model and remove expensive Transfer joins in aggregations #2388

`/transactions` Endpoint