-
Notifications
You must be signed in to change notification settings - Fork 123
fix: migrations with pg 13 #724
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
Conversation
WalkthroughThe SQL migration script in the file Changes
Sequence Diagram(s)sequenceDiagram
autonumber
participant MScript as "SQL Migration Script"
participant Database as "Database"
MScript->>Database: DROP TRIGGER IF EXISTS "update_transaction_metadata_history_<ledger_id>"
MScript->>Database: CREATE TRIGGER "update_transaction_metadata_history_<ledger_id>" after UPDATE on metadata
sequenceDiagram
autonumber
participant App as "Application"
participant TxTable as "Transactions Table"
participant Trigger as "Trigger"
App->>TxTable: Execute UPDATE on metadata column (ledger matches)
TxTable->>Trigger: Condition check for metadata update
Trigger->>TxTable: Execute trigger action (handle metadata history update)
Poem
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. 🪧 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 (
|
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/18-transactions-fill-inserted-at/up.sql (2)
15-16
: Dynamic SQL for Dropping Triggers
The SQL statement used to drop the trigger is functionally correct. However, building dynamic SQL by simple concatenation may expose subtle issues if_ledger.id
is not inherently text or if there are potential special characters. Consider using PostgreSQL’sformat()
function with the%I
placeholder to safely interpolate identifiers. For example:- _vsql = 'drop trigger if exists "update_transaction_metadata_history_' || _ledger.id || '" on "transactions"'; + _vsql = format('drop trigger if exists %I on %I', 'update_transaction_metadata_history_' || _ledger.id, 'transactions');This change improves safety and readability.
18-18
: Dynamic SQL for Creating Triggers
The new dynamic SQL statement that creates the trigger is correct in its intent. However, similar to the drop statement, concatenating strings manually can lead to issues if_ledger.name
contains single quotes or other special characters. Consider usingformat()
with%I
for identifiers and%L
for literals, which would handle quoting for you. For example:- _vsql = 'create trigger "update_transaction_metadata_history_' || _ledger.id || - '" after update of metadata on "transactions" for each row when (new.ledger = ''' - || _ledger.name || ''') execute procedure update_transaction_metadata_history()'; + _vsql = format( + 'create trigger %I after update of metadata on %I for each row when (new.ledger = %L) execute procedure update_transaction_metadata_history()', + 'update_transaction_metadata_history_' || _ledger.id, + 'transactions', + _ledger.name + );This refactoring will help prevent SQL injection vulnerabilities and potential runtime errors due to unescaped characters.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
⛔ Files ignored due to path filters (4)
go.mod
is excluded by!**/*.mod
go.sum
is excluded by!**/*.sum
,!**/*.sum
tools/generator/go.mod
is excluded by!**/*.mod
tools/generator/go.sum
is excluded by!**/*.sum
,!**/*.sum
📒 Files selected for processing (1)
internal/storage/bucket/migrations/18-transactions-fill-inserted-at/up.sql
(1 hunks)
⏰ Context from checks skipped due to timeout of 90000ms (1)
- GitHub Check: Tests
Codecov ReportAll modified and coverable lines are covered by tests ✅
Additional details and impacted files@@ Coverage Diff @@
## main #724 +/- ##
=======================================
Coverage 81.89% 81.89%
=======================================
Files 135 135
Lines 7291 7291
=======================================
Hits 5971 5971
Misses 1012 1012
Partials 308 308 ☔ View full report in Codecov by Sentry. |
No description provided.