refactor(broker): explicit transaction boundaries for multi-table writes (#46)#84
Open
ashioyajotham wants to merge 1 commit into
Open
Conversation
…es (Prescott-Data#46) Wrap multi-table write flows in explicit transactions so token upserts and connection status updates are atomic in both OAuth callback exchange and static credential capture paths. Add repository-level InTx support and tests for commit/rollback behavior to prevent partial writes during failures.
Contributor
There was a problem hiding this comment.
Pull request overview
Note
Copilot was unable to run its full agentic suite in this review.
Adds transactional support to ensure token upserts and connection status updates are performed atomically, with repository support for executing writes within a shared DB transaction.
Changes:
- Introduces a transaction helper (
InTx) in the Postgres connection repository and propagates the transaction viacontext. - Updates token/connection Postgres repositories to use the transaction from context when present.
- Adds/updates tests to validate commit/rollback behavior and adjust existing SQL expectations.
Reviewed changes
Copilot reviewed 8 out of 8 changed files in this pull request and generated 4 comments.
Show a summary per file
| File | Description |
|---|---|
| nexus-broker/pkg/handlers/soc2_compliance_test.go | Updates SQL mock expectations to reflect new transactional behavior. |
| nexus-broker/internal/service/credential.go | Wraps credential save + status update in a transaction via inTx. |
| nexus-broker/internal/service/connection.go | Wraps token exchange + status update in a transaction and adds inTx helper. |
| nexus-broker/internal/repository/postgres/tx.go | Adds context helpers to store/retrieve a transaction-backed execer. |
| nexus-broker/internal/repository/postgres/token.go | Uses context-provided transaction execer for token upserts. |
| nexus-broker/internal/repository/postgres/connection.go | Implements InTx and routes write queries through the tx-aware execer. |
| nexus-broker/internal/repository/instrumented/instrumented.go | Adds InTx forwarding + minor formatting fixes. |
| nexus-broker/internal/repository/postgres/tx_test.go | Adds tests validating InTx commit/rollback and tx propagation to repos. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
Comment on lines
+22
to
+35
| // InTx executes fn inside a database transaction. The transaction is stored in | ||
| // context so token/connection repository writes in the same request are atomic. | ||
| func (r *connectionRepository) InTx(ctx context.Context, fn func(context.Context) error) error { | ||
| tx, err := r.db.BeginTxx(ctx, nil) | ||
| if err != nil { | ||
| return err | ||
| } | ||
| ctxWithTx := withTx(ctx, tx) | ||
| if err := fn(ctxWithTx); err != nil { | ||
| _ = tx.Rollback() | ||
| return err | ||
| } | ||
| return tx.Commit() | ||
| } |
Comment on lines
+96
to
+103
| // InTx forwards transactional execution when the wrapped repository supports it. | ||
| func (r *ConnectionRepository) InTx(ctx context.Context, fn func(context.Context) error) error { | ||
| if runner, ok := r.inner.(txRunner); ok { | ||
| defer observe("connection", "InTx", time.Now()) | ||
| return runner.InTx(ctx, fn) | ||
| } | ||
| return fn(ctx) | ||
| } |
Comment on lines
+51
to
+54
| type txRunner interface { | ||
| InTx(ctx context.Context, fn func(context.Context) error) error | ||
| } | ||
|
|
Comment on lines
+33
to
+40
| mock.ExpectExec(regexp.QuoteMeta(` | ||
| INSERT INTO tokens (connection_id, encrypted_data, expires_at) | ||
| VALUES ($1, $2, $3) | ||
| ON CONFLICT (connection_id) | ||
| DO UPDATE SET | ||
| encrypted_data = EXCLUDED.encrypted_data, | ||
| expires_at = EXCLUDED.expires_at, | ||
| created_at = NOW()`)). |
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Summary
Why
Issue #46 identifies risk of partial writes when token persistence and connection status updates are done sequentially without an explicit transaction. This change ensures those operations are atomic.
Test plan