fix: set not null on some columns #930
Conversation
WalkthroughThis pull request updates two SQL migration scripts. The first script changes trigger creation on the Changes
Sequence Diagram(s)sequenceDiagram
participant Migration
participant DB
Migration->>DB: Set search_path to target schema
loop For each ledger
Migration->>DB: CREATE OR REPLACE TRIGGER on transactions/accounts
end
Note over Migration,DB: Triggers created or replaced as needed
sequenceDiagram
participant Migration
participant DB
Migration->>DB: BEGIN TRANSACTION
loop For each column in [sources, destinations, sources_arrays, destinations_arrays]
Migration->>DB: ADD CHECK constraint (NOT VALID)
Migration->>DB: VALIDATE constraint
Migration->>DB: ALTER COLUMN SET NOT NULL
Migration->>DB: DROP CHECK constraint
end
Migration->>DB: COMMIT
Assessment against linked issues
Possibly related PRs
Suggested reviewers
Poem
Note ⚡️ AI Code Reviews for VS Code, Cursor, WindsurfCodeRabbit now has a plugin for VS Code, Cursor and Windsurf. This brings AI code reviews directly in the code editor. Each commit is reviewed immediately, finding bugs before the PR is raised. Seamless context handoff to your AI code agent ensures that you can easily incorporate review feedback. Note ⚡️ Faster reviews with cachingCodeRabbit now supports caching for code and dependencies, helping speed up reviews. This means quicker feedback, reduced wait times, and a smoother review experience overall. Cached data is encrypted and stored securely. This feature will be automatically enabled for all accounts on May 30th. To opt out, configure 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:
SupportNeed help? Create a ticket on our support page for assistance with any issues or questions. 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
Documentation and Community
|
Codecov ReportAll modified and coverable lines are covered by tests ✅
Additional details and impacted files@@ Coverage Diff @@
## main #930 +/- ##
==========================================
+ Coverage 82.34% 82.38% +0.03%
==========================================
Files 141 141
Lines 7823 7823
==========================================
+ Hits 6442 6445 +3
+ Misses 1056 1054 -2
+ Partials 325 324 -1 ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
gfyrag
force-pushed
the
chore/remove-useless-features
branch
from
9fb2656 to
89f2939
Compare
gfyrag
force-pushed
the
feat/set-not-null-columns
branch
from
6bffbda to
982a5b7
Compare
gfyrag
force-pushed
the
chore/remove-useless-features
branch
from
89f2939 to
13c599c
Compare
gfyrag
force-pushed
the
feat/set-not-null-columns
branch
2 times, most recently
from
949c34f to
e6d8e93
Compare
gfyrag
force-pushed
the
chore/remove-useless-features
branch
from
13c599c to
692d742
Compare
gfyrag
force-pushed
the
feat/set-not-null-columns
branch
from
e6d8e93 to
71f9762
Compare
gfyrag
force-pushed
the
feat/set-not-null-columns
branch
from
1f2ac29 to
ffc3cf8
Compare
gfyrag
force-pushed
the
chore/remove-useless-features
branch
from
692d742 to
f30eb39
Compare
gfyrag
force-pushed
the
feat/set-not-null-columns
branch
from
ffc3cf8 to
66d9fea
Compare
gfyrag
force-pushed
the
feat/set-not-null-columns
branch
from
66d9fea to
051f884
Compare
![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
♻️ Duplicate comments (5)
internal/storage/bucket/migrations/39-clean-useless-features/up.sql (2)
15-15: Duplicate: apply safe dynamic SQL assembly here as well
Same pattern applies to theaccounts_set_address_arraytrigger—useformat()with%I/%Linstead of manual concatenation.
18-18: Duplicate: apply safe dynamic SQL assembly here too
Also update thetransaction_set_addresses_segmentstrigger to useformat()and proper quoting.internal/storage/bucket/migrations/40-refine-columns-types/up.sql (3)
19-31: Duplicate: consider the same DRY refactor fordestinationsblock
Reapply the loop-based approach andIF EXISTSdrop recommendation here.
33-45: Duplicate: consider the same DRY refactor forsources_arraysblock
Reapply the loop-based approach andIF EXISTSdrop recommendation here.
47-59: Duplicate: consider the same DRY refactor fordestinations_arraysblock
Reapply the loop-based approach andIF EXISTSdrop recommendation here.
🧹 Nitpick comments (1)
internal/storage/bucket/migrations/40-refine-columns-types/up.sql (1)
5-17: DRY repetitive NOT NULL enforcement with a loop and addIF EXISTSon drops
Each column follows the same 4-step pattern. You can reduce duplication by iterating over an array of column names and usingformat(). Also, make theDROP CONSTRAINTidempotent withIF EXISTS.Example refactor snippet:
-do $$ -begin - set search_path = '{{ .Schema }}'; - -- short-time exclusive lock - alter table transactions - add constraint sources_not_null - check (sources is not null) not valid; - ... - alter table transactions drop constraint sources_not_null; -end -$$; +do $$ +declare + col text; +begin + set search_path = '{{ .Schema }}'; + for col in array['sources','destinations','sources_arrays','destinations_arrays'] loop + execute format($f$ + ALTER TABLE transactions ADD CONSTRAINT %I_not_null CHECK (%I IS NOT NULL) NOT VALID; + ALTER TABLE transactions VALIDATE CONSTRAINT %I_not_null; + ALTER TABLE transactions ALTER COLUMN %I SET NOT NULL; + ALTER TABLE transactions DROP CONSTRAINT IF EXISTS %I_not_null; + $f$, col, col, col, col, col); + end loop; +end +$$;
📜 Review details
Configuration used: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
⛔ Files ignored due to path filters (1)
internal/storage/bucket/migrations/40-refine-columns-types/notes.yamlis excluded by!**/*.yaml
📒 Files selected for processing (2)
internal/storage/bucket/migrations/39-clean-useless-features/up.sql(1 hunks)internal/storage/bucket/migrations/40-refine-columns-types/up.sql(1 hunks)
⏰ Context from checks skipped due to timeout of 90000ms (2)
- GitHub Check: Dirty
- GitHub Check: Tests
| -- recreate trigger but with a check on sources is null for transactions and address_array is null for accounts | ||
| -- this way, the 2.2 (which use triggers) continuer to work, and the 2.3 which does not need them can work too | ||
| -- todo(next minor / 2.4): remove triggers and associated functions | ||
| for ledger in select * from _system.ledgers where bucket = current_schema loop | ||
| vsql = 'create trigger "transaction_set_addresses_' || ledger.id || '" before insert on transactions for each row when (new.ledger = ''' || ledger.name || ''' and new.sources is null) execute procedure set_transaction_addresses()'; | ||
| vsql = 'create or replace trigger "transaction_set_addresses_' || ledger.id || '" before insert on transactions for each row when (new.ledger = ''' || ledger.name || ''' and new.sources is null) execute procedure set_transaction_addresses()'; |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Use format() with quote_ident/quote_literal for safer dynamic SQL
Concatenating ledger.name directly can break or allow SQL injection if the ledger name contains quotes or special characters. Prefer format() and the %I/%L specifiers:
- vsql = 'create or replace trigger "transaction_set_addresses_' || ledger.id ||
- '" before insert on transactions for each row when (new.ledger = ''' ||
- ledger.name || ''' and new.sources is null) execute procedure set_transaction_addresses()';
+ vsql := format(
+ $f$CREATE OR REPLACE TRIGGER "transaction_set_addresses_%s"
+ BEFORE INSERT ON transactions
+ FOR EACH ROW
+ WHEN (NEW.ledger = %L AND NEW.sources IS NULL)
+ EXECUTE PROCEDURE set_transaction_addresses()$f$,
+ ledger.id,
+ ledger.name
+ );📝 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.
| vsql = 'create or replace trigger "transaction_set_addresses_' || ledger.id || '" before insert on transactions for each row when (new.ledger = ''' || ledger.name || ''' and new.sources is null) execute procedure set_transaction_addresses()'; | |
| -- replace manual concatenation with format() for safe quoting | |
| vsql := format( | |
| $f$CREATE OR REPLACE TRIGGER "transaction_set_addresses_%s" | |
| BEFORE INSERT ON transactions | |
| FOR EACH ROW | |
| WHEN (NEW.ledger = %L AND NEW.sources IS NULL) | |
| EXECUTE PROCEDURE set_transaction_addresses()$f$, | |
| ledger.id, | |
| ledger.name | |
| ); |
🤖 Prompt for AI Agents
In internal/storage/bucket/migrations/39-clean-useless-features/up.sql at line
12, the dynamic SQL concatenates ledger.name directly, risking SQL injection or
syntax errors with special characters. Replace the string concatenation with the
use of the format() function, applying %I for identifiers and %L for literals to
safely quote ledger.id and ledger.name within the trigger creation statement.
Fixes LX-42