feat: extract compute_hash function #929
Conversation
WalkthroughThis change adds two PostgreSQL PL/pgSQL functions within a migration: Changes
Sequence Diagram(s)sequenceDiagram
participant DB as PostgreSQL
participant Trigger as set_log_hash (trigger)
participant Func as compute_hash (function)
Note over DB: On inserting a new log record
DB->>Trigger: BEFORE INSERT on logs
Trigger->>DB: Query most recent hash for ledger
Trigger->>Func: Call compute_hash(previous_hash, new_record)
Func-->>Trigger: Return computed hash
Trigger->>DB: Assign hash to new record, continue insert
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 16th. To opt out, configure ✨ 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. 🪧 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
|
gfyrag
force-pushed
the
fix/fix-memento-format
branch
from
154f988 to
53e9152
Compare
Codecov ReportAll modified and coverable lines are covered by tests ✅
Additional details and impacted files@@ Coverage Diff @@
## main #929 +/- ##
==========================================
+ Coverage 82.27% 82.35% +0.07%
==========================================
Files 141 141
Lines 7786 7786
==========================================
+ Hits 6406 6412 +6
+ Misses 1055 1051 -4
+ Partials 325 323 -2 ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
gfyrag
force-pushed
the
fix/fix-memento-format
branch
from
53e9152 to
b1cc5c4
Compare
gfyrag
force-pushed
the
fix/compute-hash-fn
branch
2 times, most recently
from
af8b7ff to
8eb0a7d
Compare
* fix: memento format * feat: extract compute_hash function
github-merge-queue
bot
removed this pull request from the merge queue due to failed status checks
github-merge-queue
bot
removed this pull request from the merge queue due to failed status checks
github-merge-queue
bot
removed this pull request from the merge queue due to failed status checks
github-merge-queue
bot
removed this pull request from the merge queue due to failed status checks
* fix: memento format * feat: extract compute_hash function
github-merge-queue
bot
removed this pull request from the merge queue due to failed status checks
github-merge-queue
bot
removed this pull request from the merge queue due to failed status checks
github-merge-queue
bot
removed this pull request from the merge queue due to failed status checks
gfyrag
force-pushed
the
fix/compute-hash-fn
branch
from
8eb0a7d to
2506165
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
🧹 Nitpick comments (5)
internal/storage/bucket/migrations/35-create-compute-hash-function/up.sql (5)
1-4: Markcompute_hashas IMMUTABLE and verifypgcryptoavailability.
Sincecompute_hashis a pure, side-effect-free function, add anIMMUTABLEvolatility modifier afterRETURNS byteato enable planner optimizations. Also confirm that thepgcryptoextension (which providesdigest) is installed in this schema or include aCREATE EXTENSION IF NOT EXISTS pgcryptobefore these definitions to prevent migration failures.create or replace function compute_hash(previous_hash bytea, r logs) - returns bytea + returns bytea + immutable language plpgsql as $$
6-7: Usetextinstead ofvarcharfor unbounded JSON strings.
Even thoughvarcharwithout a length defaults to unlimited in PostgreSQL, usingtextmakes the intent clear and avoids any accidental constraint.declare - marshalledAsJSON varchar; + marshalledAsJSON text;
10-17: Ensure deterministic, valid JSON-like concatenation.
- Wrap the
datapayload in quotes and consider switching to base64 for cross-language consistency.- Guard all concatenated fields (not only
idempotency_key) againstNULLto avoid silent nullification of the entire string.select '{' || - '"type":"' || r.type || '",' || - '"data":' || encode(r.memento, 'escape') || ',' || + '"type":"' || coalesce(r.type, '') || '",' || + '"data":"' || encode(r.memento, 'base64') || '",' || '"date":"' || (to_json(r.date::timestamp)#>>'{}') || 'Z",' || - '"idempotencyKey":"' || coalesce(r.idempotency_key, '') || '",' || + '"idempotencyKey":"' || coalesce(r.idempotency_key, '') || '",' || '"id":0,' || '"hash":null' || '}' into marshalledAsJSON;
19-25: Simplify theRETURNby removing the nestedSELECT.
In PL/pgSQL you can return the result ofpublic.digestdirectly without wrapping it in aSELECT.- return ( - select public.digest( - case - ... - end || E'\n', 'sha256'::text - ) - ); + return public.digest( + case + ... + end || E'\n', + 'sha256' + );
38-42: Add an index to optimize the previous‐hash lookup.
The querySELECT hash FROM logs WHERE ledger = new.ledger ORDER BY seq DESC LIMIT 1;can become slow on large tables. Consider adding an index on
(ledger, seq DESC)if it doesn’t already exist.CREATE INDEX CONCURRENTLY IF NOT EXISTS idx_logs_ledger_seq ON {{ .Schema }}.logs (ledger, seq DESC);
📜 Review details
Configuration used: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
⛔ Files ignored due to path filters (4)
internal/storage/bucket/migrations/35-create-compute-hash-function/notes.yamlis excluded by!**/*.yamlinternal/storage/bucket/migrations/36-accounts-recreate-unique-index/notes.yamlis excluded by!**/*.yamlinternal/storage/bucket/migrations/37-clean-database/notes.yamlis excluded by!**/*.yamlinternal/storage/bucket/migrations/38-logs-async-hash-procedure/notes.yamlis excluded by!**/*.yaml
📒 Files selected for processing (1)
internal/storage/bucket/migrations/35-create-compute-hash-function/up.sql(1 hunks)
🔇 Additional comments (2)
internal/storage/bucket/migrations/35-create-compute-hash-function/up.sql (2)
29-33: Verify trigger installation and timing.
This defines theset_log_hash()function, but ensure you create aBEFORE INSERTtrigger onlogsthat invokes it. Without registering the trigger, new rows won’t get theirhashpopulated.Did you include something like:
CREATE TRIGGER trg_set_log_hash BEFORE INSERT ON {{ .Schema }}.logs FOR EACH ROW EXECUTE FUNCTION set_log_hash();in your migration sequence?
44-46: Confirm correct trigger function execution context.
Assigningnew.hash = compute_hash(previousHash, new)requires this to run in aBEFORE INSERTtrigger so thatnew.seqis available (via default) andnew.hashis persisted. Please verify the trigger’s configuration matches this requirement.
| select '{' || | ||
| '"type":"' || r.type || '",' || | ||
| '"data":' || encode(r.memento, 'escape') || ',' || | ||
| '"date":"' || (to_json(r.date::timestamp)#>>'{}') || 'Z",' || |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Format timestamps in UTC explicitly.
Casting a timestamptz to timestamp applies the server’s timezone. For a stable hash, format the date in UTC, e.g.:
- '"date":"' || (to_json(r.date::timestamp)#>>'{}') || 'Z",' ||
+ '"date":"' || to_char(r.date AT TIME ZONE 'UTC', 'YYYY-MM-DD"T"HH24:MI:SS"Z"') || '",' ||📝 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.
| '"date":"' || (to_json(r.date::timestamp)#>>'{}') || 'Z",' || | |
| '"date":"' || to_char(r.date AT TIME ZONE 'UTC', 'YYYY-MM-DD"T"HH24:MI:SS"Z"') || '",' || |
🤖 Prompt for AI Agents
In internal/storage/bucket/migrations/35-create-compute-hash-function/up.sql at
line 13, the timestamp is cast to timestamp without timezone, which applies the
server's timezone and can cause inconsistent hashes. Modify the code to
explicitly format the timestamp in UTC by converting the timestamptz to UTC
before formatting, ensuring stable and consistent hash values regardless of
server timezone.
No description provided.