fix: missing filter while listing logs #707
Conversation
|
Important Review skippedAuto reviews are disabled on base/target branches other than the default branch. 🗂️ Base branches to auto review (1)
Please check the settings in the CodeRabbit UI or the You can disable this status message by setting the WalkthroughThis pull request makes changes to several components of the codebase. In the Earthfile, the build step for the client generation is commented out, leaving the openapi build active for the pre-commit target. In the API package and scripting logic, variable handling is adjusted to use pointers and type switches for improved data formatting. Several SQL query functions in the ledgerstore have been modified to include point-in-time filters, new conditions, and enhanced error handling. Additionally, multiple new test cases have been added across packages to verify the updated functionality and ensure correct handling of edge cases. Changes
Sequence Diagram(s)sequenceDiagram
participant User
participant Earthfile
participant BuildEngine
User->>Earthfile: Trigger pre-commit
Earthfile->>BuildEngine: Execute BUILD +openapi
Earthfile-->>BuildEngine: (generate-client step is skipped)
BuildEngine-->>Earthfile: Return build results
Earthfile->>User: Pre-commit complete
Possibly related PRs
Suggested reviewers
Poem
Thank you for using CodeRabbit. We offer it for free to the OSS community and would appreciate your support in helping us grow. If you find it useful, would you consider giving us a shout-out on your favorite social media? 🪧 TipsChatThere are 3 ways to chat with CodeRabbit:
Note: Be mindful of the bot's finite context window. It's strongly recommended to break down tasks such as reading entire modules into smaller chunks. For a focused discussion, use review comments to chat about specific files and their changes, instead of using the PR comments. CodeRabbit Commands (Invoked using PR comments)
Other keywords and placeholders
CodeRabbit Configuration File (
|
![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 (3)
internal/script.go (1)
35-40: Improved type handling for numeric values, but potential issues with precision.The changes to
ToCoremethod add proper type-specific handling for theamountfield, which aligns with the PR objective to resolve incompatibility issues when handling large numbers. However, there are two potential concerns:
- Converting
float64tointwill truncate decimal places, potentially causing data loss- Very large
float64values might exceed the range ofinttype (which is platform-dependent), leading to overflowConsider handling decimal places and potential overflow:
case float64: - s.Script.Vars[k] = fmt.Sprintf("%s %d", v["asset"], int(amount)) + // Preserve precision and handle potential overflow + s.Script.Vars[k] = fmt.Sprintf("%s %.0f", v["asset"], amount)internal/script_test.go (1)
19-47: Good test coverage, but missing potential edge cases.The test cases effectively verify the conversion functionality for both large float64 and string-based big integers, which is great. This helps ensure the PR objective of fixing incompatibility with large numbers is met.
Consider adding these additional test cases:
- A number with decimal places to verify truncation behavior
- A negative number to ensure sign is preserved
- Edge cases like zero, extremely large values, or invalid inputs
testCases := []testCase{ // existing test cases... + { + name: "decimal number conversion", + inputVars: map[string]any{ + "amount": map[string]any{ + "asset": "USD", + "amount": 123.45, + }, + }, + expected: map[string]string{ + "amount": "USD 123", + }, + }, + { + name: "negative number conversion", + inputVars: map[string]any{ + "amount": map[string]any{ + "asset": "USD", + "amount": -500, + }, + }, + expected: map[string]string{ + "amount": "USD -500", + }, + }, }internal/storage/ledgerstore/accounts.go (1)
128-141: DRY opportunity for balance-based filtering.
The logic here mirrors the approach in thebalanceRegexcase. If these two paths share the same concept, consider consolidating them to reduce duplication and facilitate maintenance.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
⛔ Files ignored due to path filters (1)
docker-compose.ymlis excluded by!**/*.yml
📒 Files selected for processing (10)
Earthfile(1 hunks)internal/api/v2/utils.go(1 hunks)internal/script.go(1 hunks)internal/script_test.go(1 hunks)internal/storage/ledgerstore/accounts.go(2 hunks)internal/storage/ledgerstore/accounts_test.go(3 hunks)internal/storage/ledgerstore/balances.go(1 hunks)internal/storage/ledgerstore/balances_test.go(1 hunks)internal/storage/ledgerstore/logs.go(2 hunks)internal/storage/ledgerstore/logs_test.go(1 hunks)
✅ Files skipped from review due to trivial changes (1)
- Earthfile
🔇 Additional comments (10)
internal/script_test.go (1)
10-64: Well-structured test with parallel execution.The test is well-structured using table-driven testing patterns, with clear test cases and good use of parallel test execution. The approach of testing both float64 and big integer string conversions is thorough.
internal/storage/ledgerstore/balances_test.go (1)
21-32: Good addition of test for zero balance on non-existent accounts.This test properly validates that the store returns a zero balance instead of an error when querying for a non-existent account, aligning with the PR objective to "fix the ledger store to return a zero balance for non-existing accounts".
internal/storage/ledgerstore/logs_test.go (1)
293-298: Good addition of test case for filtering logs by ID.This test case properly validates the functionality of retrieving logs with ID less than 3, which helps ensure the filtering capability works as expected. This aligns with the PR objective to fix the missing filter while listing logs.
internal/storage/ledgerstore/logs.go (1)
75-76: Good enhancement to support filtering by ID.The addition of
idas a valid filter key properly extends the query building functionality to support filtering logs by their ID, which was mentioned in the PR objectives.internal/storage/ledgerstore/accounts_test.go (2)
10-11: Appropriate addition of pointer package import.Good addition of the pointer package import, which is used in the new test case.
190-201: Good test case for balance filtering with PIT.This test case properly validates that filtering accounts by balance works correctly when combined with a point-in-time (PIT) filter, which addresses the PR objective about "invalid balance filter when using point-in-time (PIT) queries".
internal/api/v2/utils.go (1)
50-62:✅ Verification successful
Use caution with nil pointer usage in PIT.
Now that the function storespitas a pointer, there may be scenarios wherepitremainsnil. Although the downstream code often checksif q.PIT != nil && !q.PIT.IsZero(), please verify that every place referencingPITis robust against a potentialnilvalue and won’t cause unexpected panics or logic errors.Feel free to run the script below to search for all references to
PITand usage patterns across the codebase for potential nil-dereferences:
🏁 Script executed:
#!/bin/bash # Description: Locate references to PIT and check for nil handling rg -A 5 "q\.PIT"Length of output: 4668
PIT nil pointer handling confirmed
After searching across the codebase, all references to
q.PIT—notably ininternal/storage/ledgerstore/balances.goandinternal/storage/ledgerstore/accounts.go—consistently check for anilvalue (using patterns likeif q.PIT != nilandif q.PIT != nil && !q.PIT.IsZero()). This indicates that the new usage of a pointer forpitis properly guarded and should not cause unexpected panics or logic errors downstream.No further modifications are necessary.
internal/storage/ledgerstore/balances.go (1)
149-165: Inline SQL query for balance retrieval looks good.
Switching from the stored function to a direct inline SQL query makes the logic more explicit and predictable. Returning zero when no record is found (line 164-165) also clearly fulfills the requirement of avoiding errors for non-existent accounts.internal/storage/ledgerstore/accounts.go (2)
110-126: Verify the usage ofinsertion_datefor PIT filtering.
The addeddateFiltercondition usesinsertion_date < ?for filtering whenPITis provided. Ensure that you indeed want to referenceinsertion_daterather thaneffective_date, since other parts of the code sometimes rely oneffective_dateas a point-in-time reference.
152-153: Newfirst_usagefilter support.
This addition helps filter queries by thefirst_usagecolumn. It looks good. However, confirm that the chosen operator is compatible with all comparison modes you intend to allow (for instance$match,$exists, etc., if relevant).
internal/storage/ledgerstore/logs.go
Outdated
| fmt.Println(selectQuery.String()) | ||
|
|
There was a problem hiding this comment.
🛠️ Refactor suggestion
Remove debugging print statement.
This fmt.Println statement appears to be a debugging leftover and should be removed from the production code.
- fmt.Println(selectQuery.String())📝 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.
| fmt.Println(selectQuery.String()) |
internal/storage/ledgerstore/logs.go
Outdated
| @@ -83,6 +83,7 @@ func (store *Store) logsQueryBuilder(q PaginatedQueryOptions[any]) func(*bun.Sel | |||
| } | |||
| selectQuery = selectQuery.Where(subQuery, args...) | |||
| } | |||
| fmt.Println(selectQuery.String()) | |||
There was a problem hiding this comment.
coderabbit is right on this one, we should remove the query print !
gfyrag
force-pushed
the
hotfix/list-logs-missing-filter
branch
from
4355a93 to
64dd9a4
Compare
No description provided.