-
Notifications
You must be signed in to change notification settings - Fork 108
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
fix: failing queries (GET /accounts?expand=volumes) when migrations are not terminated #723
Conversation
…re not terminated
WalkthroughThe pull request updates several configuration and migration files. In the Changes
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 (
|
Codecov ReportAll modified and coverable lines are covered by tests ✅
Additional details and impacted files@@ Coverage Diff @@
## main #723 +/- ##
==========================================
- Coverage 81.97% 81.82% -0.16%
==========================================
Files 135 135
Lines 7291 7291
==========================================
- Hits 5977 5966 -11
- Misses 1008 1016 +8
- Partials 306 309 +3 ☔ View full report in Codecov by Sentry. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Actionable comments posted: 0
🧹 Nitpick comments (2)
internal/storage/bucket/migrations/11-make-stateless/up.sql (2)
580-601
: Consider adding volatility qualifier to get_all_assets functionThe function is properly implemented with the search path fix, but unlike other functions, it doesn't have an explicit volatility qualifier (like
stable
orimmutable
). Since this function doesn't modify the database and would return the same results for the same parameters, consider adding astable
qualifier.create or replace function get_all_assets(_ledger varchar) returns setof varchar + stable language sql as
603-612
: Consider adding parallel safe attribute for consistencyThe function
get_account_aggregated_volumes
has theparallel safe
attribute, but this one doesn't. If this function can also be safely executed in parallel, consider adding this attribute for consistency and potential performance benefits.create or replace function get_account_aggregated_effective_volumes(_ledger varchar, _account_address varchar, _before timestamp default null) returns jsonb language sql stable + parallel safe as
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (4)
.gitignore
(1 hunks)Earthfile
(1 hunks)internal/storage/bucket/migrations/11-make-stateless/up.sql
(2 hunks)tools/generator/Earthfile
(1 hunks)
✅ Files skipped from review due to trivial changes (1)
- tools/generator/Earthfile
⏰ Context from checks skipped due to timeout of 90000ms (1)
- GitHub Check: Tests
🔇 Additional comments (5)
.gitignore (1)
10-11
: LGTM: Version control configuration updates are appropriateThe changes to the
.gitignore
file look good:
- Retaining
!.envrc
ensures that this environment configuration file remains tracked in the repository- Adding
Pulumi.*.yaml
to ignore Pulumi configuration files is appropriate as these often contain environment-specific settingsEarthfile (1)
4-4
: Dependency update from v0.19.0 to v0.19.1 is appropriateThis is a minor version update of the core dependency, which follows semantic versioning principles and should be backward compatible.
internal/storage/bucket/migrations/11-make-stateless/up.sql (3)
562-564
: Good documentation for the search path fixThe comments clearly explain the purpose of the changes - recreating functions with proper search path configuration for databases created before version 2.2.
565-578
: Function definition looks good with search path fixThe
get_transaction
function is properly defined with theset search_path from current;
statement to fix the search path issue on older databases.
614-623
: Function definition with parallel safe attribute looks goodThe
get_account_aggregated_volumes
function is properly defined with bothstable
andparallel safe
attributes, along with the search path fix.
No description provided.