fix: Use big.Int in query filters #1042
Conversation
|
Important Review skippedReview was skipped due to path filters ⛔ Files ignored due to path filters (6)
CodeRabbit blocks several paths by default. You can override this behavior by explicitly including those paths in the path filters. For example, including You can disable this status message by setting the WalkthroughTests for account balance filters were updated to use big.Int values instead of float64. Schema validation now accepts big.Int and *big.Int as valid numeric types. An end-to-end test was added to verify balance filtering with large big integers. Changes
Sequence Diagram(s)sequenceDiagram
autonumber
participant C as Client
participant API as API (Accounts List)
participant Q as Query Builder
participant S as Storage Schema
participant DB as Database
C->>API: GET /accounts?filter=balance[CCY] $gte/$lte (big.Int)
API->>Q: Build filters with big.Int bounds
Q->>S: Validate numeric values
Note over S: Accept int/float/big.Int
S-->>Q: OK
Q->>DB: Execute query with balance predicates
DB-->>API: Matching accounts
API-->>C: 200 OK with result set
Estimated code review effort🎯 2 (Simple) | ⏱️ ~10 minutes 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 |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (2)
internal/storage/common/schema.go (1)
217-225: No pointer numeric usages found in query builders; nil-pointer checks remain optional
- Ran ripgrep against all Go files for patterns like
query.Lt(..., &x)and literal pointer types (*big.Int,*int, etc.); no matches were found in the repo.- Since pointer numerics aren’t currently passed into the query builders, adding nil-pointer rejection in
ValidateValuewon’t break existing behavior—this remains an optional hardening step.You may choose to implement the suggested diff to guard against typed-nil pointers in the future, but there are no immediate downstream impacts.
test/e2e/api_accounts_list_test.go (1)
120-143: Valuable e2e for big.Int range filters; tighten assertion to check the actual account.The $and with $gte/$lte around the same bigInt exercises range handling for huge values. To make the test more robust, assert which account is returned (not just count). Example change:
It("should properly filter big ints", func(specContext SpecContext) { 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[0].Address).To(Equal("foo:foo")) })If
$eqis supported by the query language, you could alternatively use it here; the current form is fine if you specifically want to cover both comparison paths.
📜 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 (7)
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_volumes_test.go(2 hunks)internal/storage/common/schema.go(2 hunks)test/e2e/api_accounts_list_test.go(1 hunks)
🧰 Additional context used
🧬 Code graph analysis (1)
test/e2e/api_accounts_list_test.go (1)
pkg/client/models/operations/v2listaccounts.go (1)
V2ListAccountsRequest(11-30)
🔇 Additional comments (12)
internal/storage/common/schema.go (2)
5-6: Import of math/big is appropriate for big.Int acceptance.No issues. This aligns with the extended numeric validation.
5-6: Unable to locate dependency management files to verify go-libs versionI wasn’t able to find a go.mod, go.work, Gopkg.toml/lock, glide.yaml/lock, or vendor/modules.txt in the repo—so I can’t confirm that you’ve pinned github.com/formancehq/go-libs/v3 to the version containing PR 505 (which adds the required big.Int support). Please ensure:
- Your module’s go.mod (or equivalent) at the project root includes
require github.com/formancehq/go-libs/v3 <version-with-PR-505>- No conflicting
replacedirective overrides that version- If you manage dependencies elsewhere (e.g. in a parent module), point me to the file/location where go-libs is declared
File under review: internal/storage/common/schema.go (imports “math/big” and relies on go-libs big.Int support)
internal/api/v2/common.go (1)
90-91: Whitespace-only change.No behavioral impact. Fine to keep.
internal/api/v2/controllers_accounts_count_test.go (3)
5-5: Import math/big to exercise big.Int filters is correct.Matches the updated schema validation and query expectations.
11-14: Import adjustments look fine.No functional changes; keeps tests readable and organized.
76-77: Switch to big.NewInt(100) in expected Builder aligns with body payload.This validates big.Int flow end-to-end for Lt on balance[USD/2].
internal/api/v2/controllers_volumes_test.go (1)
38-38: Formatting-only blank line.No action needed.
internal/api/v2/controllers_accounts_list.go (1)
4-5: Import reorder only.No behavioral impact; goimports-compatible.
internal/api/v2/controllers_accounts_list_test.go (4)
5-5: Good move: use big.Int to avoid float precision pitfalls in filters.Switching tests to math/big ensures deterministic comparisons for balances.
11-15: Imports look correct and necessary.pointer, common, storagecommon, and ledgerstore are all used below; aliasing storagecommon keeps call sites readable.
42-42: Whitespace-only change.No action needed.
151-151: Confirm big.Int usage and parser parity for JSON numbers vs. strings
All existing balance-filter tests now usebig.NewInt(no float64/json.Number left inquery.Lt|Lte|Gt|Gte|Eqforbalance[...]) andTypeNumeric.ValidateValueaccepts bothbig.Intand*big.Intper schema logic.• File/internal/api/v2/controllers_accounts_list_test.go:151
- The change to
query.Lt("balance[USD/2]", big.NewInt(100))is correct to avoid float rounding.• Depedency check
- Please manually verify that your
go-libs/v3module is upgraded to the version including PR 505 (which ensures JSON numbers and string‐encoded big ints are handled identically).• Optional test enhancement
- Consider adding a sibling test using a string literal filter, e.g.:
and assert that the parser still produces aBuilder: query.Lt("balance[USD/2]", "100"),big.Intof value 100.
| Builder: query.Gte("balance[EUR]", big.NewInt(100)), | ||
| Expand: make([]string, 0), |
There was a problem hiding this comment.
Mismatch: body uses 50 but expected Builder uses big.NewInt(100).
Test will fail as the parsed query compares against 50 while expectation asserts 100. Align the expected Builder to 50.
Apply this diff:
- Builder: query.Gte("balance[EUR]", big.NewInt(100)),
+ Builder: query.Gte("balance[EUR]", big.NewInt(50)),📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| Builder: query.Gte("balance[EUR]", big.NewInt(100)), | |
| Expand: make([]string, 0), | |
| @@ internal/api/v2/controllers_volumes_test.go | |
| Builder: query.Gte("balance[EUR]", big.NewInt(50)), | |
| Expand: make([]string, 0), |
🤖 Prompt for AI Agents
In internal/api/v2/controllers_volumes_test.go around lines 128 to 129, the test
expectation uses Builder: query.Gte("balance[EUR]", big.NewInt(100)) while the
test body parses a query with value 50; update the expected Builder value to
big.NewInt(50) so the parsed query and the expectation match, and run tests to
verify the failure is resolved.
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## main #1042 +/- ##
==========================================
- Coverage 80.75% 80.67% -0.08%
==========================================
Files 186 186
Lines 10309 10309
==========================================
- Hits 8325 8317 -8
- Misses 1563 1569 +6
- Partials 421 423 +2 ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
Azorlogh
force-pushed
the
fix/query-filters-with-big-ints
branch
from
be1ba1d to
f690471
Compare
There was a problem hiding this comment.
Actionable comments posted: 0
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
internal/storage/common/schema.go (1)
217-224: Validation accepts nil numeric pointers — reject nils to avoid invalid queries slipping throughCurrently,
*int/*float/*big.Intvalues pass validation even when nil. That can admit malformed filters and defer failures downstream. Explicitly reject nil pointers.-func (t TypeNumeric) ValidateValue(value any) error { - switch value.(type) { - case int, int8, int16, int32, int64, uint, uint8, uint16, uint32, uint64, float64, float32, big.Int, - *int, *int8, *int16, *int32, *int64, *uint, *uint8, *uint16, *uint32, *uint64, *float64, *float32, *big.Int: - return nil - default: - return fmt.Errorf("expected numeric value, got %T", value) - } -} +func (t TypeNumeric) ValidateValue(value any) error { + switch v := value.(type) { + case int, int8, int16, int32, int64, uint, uint8, uint16, uint32, uint64, float64, float32, big.Int: + return nil + case *int, *int8, *int16, *int32, *int64, + *uint, *uint8, *uint16, *uint32, *uint64, + *float64, *float32, *big.Int: + if v == nil { + return fmt.Errorf("expected non-nil numeric pointer, got %T(nil)", value) + } + return nil + default: + return fmt.Errorf("expected numeric value, got %T", value) + } +}Consider adding a small unit test set asserting nil pointers are rejected for completeness.
♻️ Duplicate comments (1)
internal/api/v2/controllers_volumes_test.go (1)
127-128: Aligned expected bound with request body (50) — resolvedThe expected Builder now matches the body’s 50 value. This addresses the earlier mismatch noted in prior review.
🧹 Nitpick comments (2)
test/e2e/api_accounts_list_test.go (2)
120-143: Strengthen the big-int filter assertionSince the filter should return exactly the “foo:foo” account, assert the address for extra confidence.
Expect(err).ToNot(HaveOccurred()) - Expect(len(response.V2AccountsCursorResponse.Cursor.Data)).To(Equal(1)) + Expect(len(response.V2AccountsCursorResponse.Cursor.Data)).To(Equal(1)) + Expect(response.V2AccountsCursorResponse.Cursor.Data[0].Address).To(Equal("foo:foo"))
61-62: Check SetString conversion success to avoid silent test flakinessGuard against accidental invalid literals by asserting the
okflag from SetString.- bigInt, _ = big.NewInt(0).SetString("98EWM95ct8tBYWroCxXYN9vCgN7NTcR6nUsvCx1mEdLZ999999999999999999999999999999999999999999", 10) + var ok bool + bigInt, ok = big.NewInt(0).SetString("98EWM95ct8tBYWroCxXYN9vCgN7NTcR6nUsvCx1mEdLZ999999999999999999999999999999999999999999", 10) + Expect(ok).To(BeTrue())Also applies to: 120-143
📜 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 (2)
go.modis excluded by!**/*.modgo.sumis excluded by!**/*.sum,!**/*.sum
📒 Files selected for processing (5)
internal/api/v2/controllers_accounts_count_test.go(2 hunks)internal/api/v2/controllers_accounts_list_test.go(3 hunks)internal/api/v2/controllers_volumes_test.go(1 hunks)internal/storage/common/schema.go(2 hunks)test/e2e/api_accounts_list_test.go(1 hunks)
⏰ 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: Tests
- GitHub Check: Dirty
🔇 Additional comments (2)
internal/api/v2/controllers_accounts_count_test.go (1)
12-12: Switch to big.Int in balance filter expectation — looks correctUsing big.NewInt(100) aligns the test with the new numeric handling in query filters. No issues spotted.
Also applies to: 73-75
internal/api/v2/controllers_accounts_list_test.go (1)
13-13: big.Int adoption in balance filter expectation — good updateThe expectation now matches big-int aware parsing. All good.
Also applies to: 149-151
Relies on formancehq/go-libs#505