feat: filter tx by amount #1041
Conversation
|
Important Review skippedAuto reviews are disabled on base/target branches other than the default branch. 🗂️ Base branches to auto review (2)
Please check the settings in the CodeRabbit UI or the You can disable this status message by setting the WalkthroughAdds an asset-specific numeric Changes
Sequence Diagram(s)sequenceDiagram
autonumber
participant Client
participant API as API v2 Controller
participant Builder as Query Builder
participant DB as Database
Client->>API: ListTransactions(filter: amount[ASSET] {$gt|$lt}, PIT, page)
API->>Builder: Translate filter to builder form
Builder->>Builder: Detect amount[...] via amountRegex
Builder->>DB: Correlated subquery: SELECT SUM((posting->>'amount')::numeric) FROM jsonb_array_elements(postings) WHERE (posting->>'asset')=ASSET AND postings->>'tx_id'=transactions.id
DB-->>Builder: SUM(...) expression
Builder-->>API: Final SQL with amount comparison, PIT, sort, limit
API->>DB: Execute SQL
DB-->>API: Rows + cursor
API-->>Client: Results + cursor
Estimated code review effort🎯 4 (Complex) | ⏱️ ~60 minutes Possibly related PRs
Suggested reviewers
Poem
✨ Finishing Touches🧪 Generate unit tests
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
🚨 Bugbot Trial ExpiredYour team's Bugbot trial has expired. Please contact your team administrator to turn on the paid plan to continue using Bugbot. A team admin can activate the plan in the Cursor dashboard. |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 2
🧹 Nitpick comments (3)
internal/storage/ledger/transactions.go (1)
32-34: Anchor and harden the amount[] regex.The current pattern
amount\[(.*)]is greedy and unanchored. It will match partial strings and allows empty assets. Prefer a stricter, anchored pattern to avoid accidental matches and edge cases.Apply this diff:
-var ( - amountRegex = regexp.MustCompile(`amount\[(.*)]`) -) +var ( + // Match exactly "amount[ASSET]" and capture ASSET (any chars except a closing bracket) + amountRegex = regexp.MustCompile(`^amount\[([^\]]+)\]$`) +)internal/storage/ledger/transactions_test.go (1)
879-886: Great: first storage-level assertion for amount[]; add a few more cases.This proves
$gtonamount[EUR]with expected rows{tx6, tx1}. To harden behavior and semantics, please add:
$ltand$ltevariants (e.g.,Lt("amount[EUR]", 50)should exclude tx without EUR if that’s intended).- A non-existent asset (e.g.,
amount[JPY]) to confirm no matches.- An asset with a slash (
USD/2) to validate regex capture for complex asset codes.I can push a follow-up test block covering these cases if you want.
test/e2e/api_transactions_list_test.go (1)
447-480: Nice e2e for amount[]; consider mirroring $lt and composing with $and.This verifies the API-layer builder captures
query.Gt("amount[EUR]", 10.0)and preserves PIT. For parity with unit/integration:
- Add a
$ltscenario.- Add an
$andcomposition mixingamount[EUR]with another predicate (e.g.,source=world) to ensure composition works as intended.
📜 Review details
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
💡 Knowledge Base configuration:
- MCP integration is disabled by default for public repositories
- Jira integration is disabled by default for public repositories
- Linear integration is disabled by default for public repositories
You can enable these sources in your CodeRabbit configuration.
📒 Files selected for processing (5)
internal/api/v2/controllers_transactions_list_test.go(1 hunks)internal/storage/ledger/resource_transactions.go(2 hunks)internal/storage/ledger/transactions.go(2 hunks)internal/storage/ledger/transactions_test.go(6 hunks)test/e2e/api_transactions_list_test.go(2 hunks)
🧰 Additional context used
🧬 Code graph analysis (4)
internal/api/v2/controllers_transactions_list_test.go (2)
internal/storage/common/pagination.go (1)
InitialPaginatedQuery(9-14)internal/storage/common/resource.go (1)
ResourceQuery(408-414)
internal/storage/ledger/transactions_test.go (4)
internal/transaction.go (3)
NewTransaction(227-231)Transaction(36-50)Transaction(52-59)internal/posting.go (1)
NewPosting(26-33)internal/storage/common/pagination.go (1)
InitialPaginatedQuery(9-14)internal/storage/common/resource.go (1)
ResourceQuery(408-414)
test/e2e/api_transactions_list_test.go (4)
pkg/client/models/operations/v2listtransactions.go (1)
V2ListTransactionsRequest(37-61)pkg/client/models/components/v2transactionscursorresponse.go (1)
V2TransactionsCursorResponse(48-50)internal/storage/common/pagination.go (1)
ColumnPaginatedQuery(19-24)internal/storage/common/resource.go (1)
ResourceQuery(408-414)
internal/storage/ledger/resource_transactions.go (2)
internal/storage/common/schema.go (1)
NewNumericMapField(92-94)internal/storage/common/resource.go (1)
ConvertOperatorToSQL(17-33)
🔇 Additional comments (9)
internal/storage/ledger/transactions.go (1)
12-12: New import looks fine, but regex stability warrants a quick tweak next.internal/storage/ledger/transactions_test.go (6)
733-741: Good edge case: intra-tx opposing postings.tx6 exercises the sum semantics across both directions of EUR postings within a single transaction. This is valuable to ensure the filter aggregates all postings, not just net effects.
742-750: Complementary mixed-asset case is useful.tx7 validates that non-target assets are ignored in the sum. This helps catch accidental cross-asset aggregation.
761-761: Result set updates look consistent with new fixtures.Prepending tx7/tx6 in expectations aligns with insert order and default id-desc sorting.
788-798: Segment-based address expectations updated correctly.Inclusion of tx7/tx6 under “users:” filters is correct given the new postings.
867-868: “not exists metadata” expectation update is consistent.
876-877: Timestamp equality matching multiple tx at identical instant is correct.Four transactions share the exact timestamp; updated expectation matches that behavior.
test/e2e/api_transactions_list_test.go (1)
402-446: Rename to “multiple matches” improves clarity; builder expectation LGTM.internal/storage/ledger/resource_transactions.go (1)
28-28: Schema addition for amount map is appropriate.Declaring
amountas a numeric map aligns with theamount[ASSET]filter shape. No issues here.
|
@Azorlogh In case you have not seen, tests are red. |
| Order: pointer.For(bunpaginate.Order(bunpaginate.OrderDesc)), | ||
| Options: storagecommon.ResourceQuery[any]{ | ||
| PIT: &now, | ||
| Builder: query.Lt("amount[EUR]", 20.0), |
There was a problem hiding this comment.
It looks like the query numbers currently get parsed to floats only? Wouldn't I want something like a big int here?
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## fix/query-filters-with-big-ints #1041 +/- ##
===================================================================
+ Coverage 80.67% 80.79% +0.11%
===================================================================
Files 186 186
Lines 10309 10326 +17
===================================================================
+ Hits 8317 8343 +26
+ Misses 1569 1563 -6
+ Partials 423 420 -3 ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
| ) | ||
| Expect(err).To(BeNil()) | ||
| }) | ||
| _ = response |
Azorlogh
force-pushed
the
feat/filter-tx-by-amount
branch
from
ce0cc5f to
8548c87
Compare
Azorlogh
changed the base branch from
main
to
fix/query-filters-with-big-ints
Azorlogh
force-pushed
the
feat/filter-tx-by-amount
branch
from
8548c87 to
cf2fd5a
Compare
There was a problem hiding this comment.
Actionable comments posted: 0
♻️ Duplicate comments (1)
internal/storage/ledger/resource_transactions.go (1)
105-118: Fix JSON amount casting and wrap boolean subselect (current SQL can fail).
Casting(posting->'amount')::numericis invalid if the JSON encodes amounts as strings (common with big.Int); also safer to wrap the boolean subselect. Use->>before casting.Apply this diff:
case amountRegex.Match([]byte(property)): asset := amountRegex.FindStringSubmatch(property)[1] selectAmount := h.store.db.NewSelect(). ModelTableExpr(fmt.Sprintf("%[1]s, jsonb_array_elements(%[1]s.postings::jsonb) posting", h.store.GetPrefixedRelationName("transactions"))). Where("id = dataset.id"). Where("ledger = ?", h.store.ledger.Name). Where("posting->>'asset' = ?", asset). - ColumnExpr("sum((posting->'amount')::numeric) amount") + -- Extract as text then cast to numeric to support string-encoded amounts + ColumnExpr("sum((posting->>'amount')::numeric) AS amount") - return h.store.db.NewSelect(). - TableExpr("(?) amount", selectAmount). - ColumnExpr(fmt.Sprintf("amount %s ?", common.ConvertOperatorToSQL(operator)), value). - String(), nil, nil + q := h.store.db.NewSelect(). + TableExpr("(?) amt", selectAmount). + ColumnExpr(fmt.Sprintf("amount %s ?", common.ConvertOperatorToSQL(operator)), value). + String() + return fmt.Sprintf("(%s)", q), nil, nilOptional: If you want transactions without that asset to participate in $lt/$lte as 0, wrap the SUM with
coalesce(..., 0). If you do, adjust tests accordingly.
🧹 Nitpick comments (1)
test/e2e/api_accounts_list_test.go (1)
120-143: Harden the big-int filter test (assert parsed bigInt and use HaveLen).
Ensure SetString produced a non-nil bigInt and prefer Gomega’s HaveLen matcher.Apply this diff:
It("should properly filter big ints", func(specContext SpecContext) { - response, err := Wait(specContext, DeferClient(testServer)).Ledger.V2.ListAccounts( + Expect(bigInt).ToNot(BeNil()) + response, err := Wait(specContext, DeferClient(testServer)).Ledger V2.ListAccounts( ctx, operations.V2ListAccountsRequest{ Ledger: "default", RequestBody: map[string]any{ "$and": []map[string]any{ { "$gte": map[string]any{ "balance[USD]": bigInt, }, }, { "$lte": map[string]any{ "balance[USD]": bigInt, }, }, }, }, }, ) Expect(err).ToNot(HaveOccurred()) - Expect(len(response.V2AccountsCursorResponse.Cursor.Data)).To(Equal(1)) + Expect(response.V2AccountsCursorResponse.Cursor.Data).To(HaveLen(1)) })
📜 Review details
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
💡 Knowledge Base configuration:
- Linear integration is disabled by default for public repositories
You can enable these sources in your CodeRabbit configuration.
⛔ Files ignored due to path filters (6)
go.modis excluded by!**/*.modgo.sumis excluded by!**/*.sum,!**/*.sumtools/generator/go.modis excluded by!**/*.modtools/generator/go.sumis excluded by!**/*.sum,!**/*.sumtools/provisioner/go.modis excluded by!**/*.modtools/provisioner/go.sumis excluded by!**/*.sum,!**/*.sum
📒 Files selected for processing (12)
internal/api/v2/common.go(1 hunks)internal/api/v2/controllers_accounts_count_test.go(3 hunks)internal/api/v2/controllers_accounts_list.go(1 hunks)internal/api/v2/controllers_accounts_list_test.go(3 hunks)internal/api/v2/controllers_transactions_list_test.go(1 hunks)internal/api/v2/controllers_volumes_test.go(2 hunks)internal/storage/common/schema.go(2 hunks)internal/storage/ledger/resource_transactions.go(2 hunks)internal/storage/ledger/transactions.go(2 hunks)internal/storage/ledger/transactions_test.go(6 hunks)test/e2e/api_accounts_list_test.go(1 hunks)test/e2e/api_transactions_list_test.go(2 hunks)
✅ Files skipped from review due to trivial changes (2)
- internal/api/v2/common.go
- internal/api/v2/controllers_accounts_list.go
🚧 Files skipped from review as they are similar to previous changes (4)
- internal/api/v2/controllers_transactions_list_test.go
- internal/storage/ledger/transactions_test.go
- internal/storage/ledger/transactions.go
- test/e2e/api_transactions_list_test.go
🧰 Additional context used
🧬 Code graph analysis (2)
test/e2e/api_accounts_list_test.go (2)
pkg/client/v2.go (1)
V2(20-24)pkg/client/models/operations/v2listaccounts.go (1)
V2ListAccountsRequest(11-30)
internal/storage/ledger/resource_transactions.go (2)
internal/storage/common/schema.go (1)
NewNumericMapField(94-96)internal/storage/common/resource.go (1)
ConvertOperatorToSQL(17-33)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (2)
- GitHub Check: Dirty
- GitHub Check: Tests
🔇 Additional comments (9)
internal/storage/common/schema.go (2)
5-6: BigInt import addition is appropriate.
Enables numeric validation for big integers used across filters.
219-221: *TypeNumeric: add big.Int and big.Int support — LGTM.
Matches new usage in tests and filters; no issues spotted.internal/api/v2/controllers_volumes_test.go (2)
38-38: No-op formatting change.
Nothing to do.
128-129: *Expect query now uses big.Int — correct.
Aligns with schema’s numeric handling.internal/api/v2/controllers_accounts_list_test.go (2)
5-5: Import math/big for test values — OK.
Matches updated expectations using *big.Int.
151-152: Switch to big.NewInt in Lt predicate — correct.
Consistent with numeric schema and other tests.internal/api/v2/controllers_accounts_count_test.go (2)
5-5: Import math/big — OK.
Needed for BigInt predicates.
76-77: *Use big.Int in balance Lt predicate — correct.
Keeps type consistency with controllers and storage.internal/storage/ledger/resource_transactions.go (1)
28-29: Expose amount as a numeric map field — LGTM.
Enables amount[ASSET] filtering at the schema level.
Azorlogh
force-pushed
the
feat/filter-tx-by-amount
branch
from
cf2fd5a to
aeaa55f
Compare
Adds filtering of transactions by amount, e.g.
will only show transactions where the sum of amounts of all EUR postings is greater than 100