Skip to content

fixed transaction issue in consent db query - #2666

Open
sacrana0 wants to merge 1 commit into
mosip:develop-gofrom
Infosys:ES-2655
Open

sacrana0 wants to merge 1 commit into
mosip:develop-gofrom
Infosys:ES-2655

Conversation

@sacrana0

@sacrana0 sacrana0 commented Oct 1, 2026 •

Copy link
Copy Markdown
Collaborator

Closes #2655

Summary by CodeRabbit

  • Bug Fixes
    • Consent records are now saved more reliably: the related record updates are handled together, so a failure prevents a partial save and leaves the data unchanged.

Signed-off-by: Sachin Rana <sacrana324@gmail.com>
@coderabbitai

coderabbitai Bot commented Oct 1, 2026 •

Copy link
Copy Markdown

Review in Change Stack →

Navigate logical layers of code changes, visualize relationships, and explore their blast radius.

No actionable comments were generated in the recent review. 🎉

ℹ️ Recent review info
⚙️ Run configuration

Configuration used: Repository UI

Review profile: ASSERTIVE

Plan: Advanced

Run ID: cf640801-d219-4c5c-a237-fed8bd40dfc7

📥 Commits

Reviewing files that changed from the base of the PR and between 9fae8e1 and f24183e.

📒 Files selected for processing (2)
  • esignet-service/internal/consentmgmt/service.go
  • esignet-service/internal/consentmgmt/service_test.go

Included review availability: This review used your included allowance. Your plan provides up to 1 included review per hour; 0 remain after this review.


Walkthrough

NewService now stores its database connection in Service.db. Tests verify that SaveRecord writes consent history and detail in one transaction, commits on success, and rolls back when the detail insert fails.

Changes

Consent record transaction

Layer / File(s) Summary
Wire and verify consent record transactions
esignet-service/internal/consentmgmt/service.go, esignet-service/internal/consentmgmt/service_test.go
NewService stores the database connection. Tests verify statement order, transaction commit, and rollback after a detail insert failure.

Priority: ➖ Normal

Estimated code review effort: 2 (Simple) | ~10 minutes

Change: Bug fix · Severity of issue fixed: Medium

Suggested reviewers: anushasunkada

Merge Risk: ⚪ Minimal · up to f2418

Consent history and detail now save atomically, preventing partial writes. The change is mergeable subject to normal test checks.

Security Architecture Review

Security architecture risk: 🔵 Low · up to f2418

The change strengthens consent consistency without an observed expansion of access or privileges. Remaining uncertainty concerns cancellation, ambiguous commit outcomes, and recovery under concurrent or repeated requests.

Retained concerns
No architecture-level concerns identified.

Security review details

Security Blast Radius

  • inferred — The evidenced effect is confined to consent history and current-detail persistence through the existing application database connection. No new caller, cross-service authority, or wider asset access was established by the constructor change.

Trust Boundaries and Controls

  • observed — Submitted consent decisions pass through request-based filtering before the provider constructs the client/user consent record and invokes SaveRecord. The persistence service does not gain a new identity source or authorization decision from retaining the database connection.

Resilience and Maintainability Implications

  • observed — Preparation failures occur before transaction initiation. Begin, write, and commit errors are returned; deferred rollback covers pre-commit exits. The provider returns consent_persist_failed without a local retry. Repeated saves already append fresh history rows, but recovery after an ambiguous commit and external retry behavior are not established by the inspected source or tests.
🚥 Pre-merge checks | ✅ 4 | ❌ 1

❌ Failed checks (1 warning)

Check name Status Explanation Resolution
Docstring Coverage ⚠️ Warning Docstring coverage is 25.00% which is insufficient. The required threshold is 80.00%. Docstring coverage is scoped to functions touched by this diff. Analyzed 4 functions across 2 files. Write docstrings for the functions missing them to satisfy the coverage threshold.
✅ Passed checks (4 passed)
Check name Status Explanation
Description Check ✅ Passed Check skipped - CodeRabbit’s high-level summary is enabled.
Title check ✅ Passed The title clearly describes the main change: fixing the consent database transaction issue. It is concise and directly related to the pull request scope.
Linked Issues check ✅ Passed Issue #2655 requires production NewService(conn) to retain conn in Service.db, while NewServiceWithQuerier must remain unchanged. At the reviewed head, NewService returns Service{db: conn, q: db.New(c…
Out of Scope Changes check ✅ Passed The reviewed changes are limited to internal/consentmgmt. The production constructor change directly implements issue #2655. The added recording-driver tests verify the required transactional behavior…
  • Fix all pre-merge checks with AI
✨ Finishing Touches 💡 1
🧪 Generate unit tests (beta)
  • Create a new PR
🛠️ Fix failing CI checks 💡
  • Commit to this branch
  • Create a new PR
  • Autopilot · Keep fixing CodeRabbit findings and required CI, and resolving merge conflicts

Autopilot is currently an internal CodeRabbit preview.


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.

❤️ Share

History starts inside a transaction
Detail follows in ordered steps
Success closes with commit
Failure rolls the work back
Both records share their path

Comment @coderabbitai help to get the list of available commands.

@codecov-commenter

codecov-commenter commented Oct 1, 2026 •

Copy link
Copy Markdown

⚠️ Please install the 'codecov app svg image' to ensure uploads and comments are reliably processed by Codecov.

Codecov Report

✅ All modified and coverable lines are covered by tests.
⚠️ Please upload report for BASE (develop-go@9fae8e1). Learn more about missing BASE report.
❗ Your organization needs to install the Codecov GitHub app to enable full functionality.

Additional details and impacted files
@@              Coverage Diff              @@
##             develop-go    #2666   +/-   ##
=============================================
  Coverage              ?   70.59%           
=============================================
  Files                 ?      131           
  Lines                 ?     9067           
  Branches              ?      112           
=============================================
  Hits                  ?     6401           
  Misses                ?     2206           
  Partials              ?      460           
Flag Coverage Δ
go 69.47% <100.00%> (?)
npm 92.53% <ø> (?)

Flags with carried forward coverage won't be shown. Click here to find out more.

☔ View full report in Codecov by Harness.
📢 Have feedback on the report? Share it here.

🚀 New features to boost your workflow:
  • ❄️ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.
  • 📦 JS Bundle Analysis: Save yourself from yourself by tracking and limiting bundle sizes in JS merges.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants