feat: add transactional repository methods (InsertWithTx, CheckLimitsWithTx)#73
feat: add transactional repository methods (InsertWithTx, CheckLimitsWithTx)#73alexgarzao wants to merge 15 commits intodevelopfrom
Conversation
…WithTx) Add WithTx variants alongside existing methods for transactional usage: - InsertWithTx accepts pgdb.DB parameter for atomic operations - CheckLimitsWithTx accepts pgdb.DB for API consistency - Extract insertInternal/checkLimitsInternal to eliminate duplication - Add nil db validation to InsertWithTx - Add forbidden practice rule: no task/ticket IDs in code - Regenerate mocks for updated interfaces
|
This PR is very large (27 files, 1359 lines changed). Consider breaking it into smaller PRs for easier review. |
|
Consider updating CHANGELOG.md to document this change. If this change doesn't need a changelog entry, add the |
|
Consider updating CHANGELOG.md to document this change. If this change doesn't need a changelog entry, add the |
|
This PR is very large (27 files, 1360 lines changed). Consider breaking it into smaller PRs for easier review. |
|
Note Reviews pausedIt looks like this branch is under active development. To avoid overwhelming you with review comments due to an influx of new commits, CodeRabbit has automatically paused this review. You can configure this behavior by changing the Use the following commands to manage reviews:
Use the checkboxes below for quick actions:
WalkthroughAdds transactional entry points and shared internal helpers: Sequence Diagram(s)sequenceDiagram
participant Client
participant Repo as TransactionValidationRepository
participant DB
participant Tracer
Client->>Repo: InsertWithTx(ctx, db, validation)
Repo->>Tracer: Start span "repository.transaction_validation.insert_with_tx"
Repo->>Repo: insertInternal(ctx, db, validation, logger, span, operationName)
Repo->>DB: ExecContext("INSERT INTO transaction_validations", args)
DB-->>Repo: Exec result / error
Repo->>Tracer: HandleSpanError(span, err)
Repo-->>Client: return error | nil
sequenceDiagram
participant Client
participant Service as LimitCheckerService
participant LimitRepo as LimitRepository
participant UsageRepo as UsageCounterRepository
participant DB
participant Tracer
Client->>Service: CheckLimitsWithTx(ctx, db, input)
Service->>Tracer: Start span "service.limit_checker.check_limits_with_tx"
Service->>Service: checkLimitsInternal(ctx, input, logger, span, operationName)
Service->>LimitRepo: List(applicable limits)
LimitRepo-->>Service: limits
Service->>UsageRepo: UpsertAndIncrementAtomic(..., /* may use db */)
UsageRepo-->>Service: updated usage / error
Service->>Tracer: HandleSpanError(span, err)
Service-->>Client: CheckLimitsOutput | error
🚥 Pre-merge checks | ✅ 1 | ❌ 1❌ Failed checks (1 inconclusive)
✅ Passed checks (1 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. Comment |
alexgarzao
changed the base branch from
develop
to
feat/idempotency-key-support--foundation--schema-and-query-and-input-validation
🔒 Security Scan Results —
|
| Policy | Status |
|---|---|
| Default non-root user | ✅ Passed |
| No fixable critical/high CVEs | ✅ Passed |
| No high-profile vulnerabilities | ✅ Passed |
| No AGPL v3 licenses | ✅ Passed |
Shallow copy of LimitUsageDetails left the internal Scopes slice aliased with the original entity. Replace copy() with per-element iteration that creates independent Scopes slices.
alexgarzao
changed the base branch from
feat/idempotency-key-support--foundation--schema-and-query-and-input-validation
to
develop
✅ Actions performedReview triggered.
|
This comment was marked as resolved.
This comment was marked as resolved.
Sorry, something went wrong.
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
This comment was marked as resolved.
This comment was marked as resolved.
Sorry, something went wrong.
📊 Unit Test Coverage Report:
|
| Metric | Value |
|---|---|
| Overall Coverage | 82.8% |
| Threshold | 85% |
Coverage by Package
| Package | Coverage |
|---|---|
tracer/internal/adapters/cel |
81.9% |
tracer/internal/adapters/http/in/middleware |
62.0% |
tracer/internal/adapters/http/in |
81.5% |
tracer/internal/adapters/postgres/db |
0.0% |
tracer/internal/adapters/postgres |
74.8% |
tracer/internal/services/cache |
95.6% |
tracer/internal/services/command |
81.5% |
tracer/internal/services/query |
80.6% |
tracer/internal/services/workers |
79.7% |
tracer/internal/services |
40.2% |
tracer/internal/testhelper |
0.0% |
tracer/pkg/clock |
50.0% |
tracer/pkg/contextutil |
100.0% |
tracer/pkg/logging |
100.0% |
tracer/pkg/migration |
89.0% |
tracer/pkg/model |
95.0% |
tracer/pkg/net/http |
88.3% |
tracer/pkg/resilience |
97.8% |
tracer/pkg/sanitize |
87.1% |
tracer/pkg/validation |
50.0% |
tracer/pkg |
96.6% |
Generated by Go PR Analysis workflow
There was a problem hiding this comment.
Actionable comments posted: 1
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
internal/adapters/postgres/transaction_validation_repository.go (1)
109-124:⚠️ Potential issue | 🟠 MajorReject
nilvalidation before acquiring the connection.
Insertnow reachesr.conn.GetDB()before the shared helper can rejectvalidation == nil. If connection acquisition fails first, a caller bug is surfaced as an infrastructure error instead of the deterministic validation error, and we also do unnecessary DB work on a request that should fail fast. Split the nil-validation check into a small preflight step that runs beforeGetDB()and reuse it from both entrypoints.Also applies to: 147-163
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@internal/adapters/postgres/transaction_validation_repository.go` around lines 109 - 124, Reject nil validation before acquiring the DB connection by adding a small preflight check and reusing it from both entrypoints: move the nil check for *model.TransactionValidation into a helper (e.g., validateNotNilValidation or preflightValidation) and call it at the top of Insert (before r.conn.GetDB()) and in the other public entrypoint that currently runs at lines ~147-163; keep insertInternal unchanged (it can still assume a non-nil validation) and return a deterministic error when validation is nil instead of proceeding to GetDB().
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@internal/adapters/postgres/transaction_validation_repository_test.go`:
- Around line 1497-1547: The test
TestTransactionValidationPostgresRepository_InsertWithTx_UsesProvidedDB should
use a real sql transaction rather than *sql.DB: add sqlMock.ExpectBegin() and
call tx, err := db.Begin() (or db.BeginTx(ctx, ...)) then pass that tx into
repo.InsertWithTx(ctx, tx, tv) instead of repo.InsertWithTx(ctx, db, tv); ensure
you still ExpectExec on the mock and commit/rollback the tx and verify
sqlMock.ExpectationsWereMet() so the test asserts compatibility with *sql.Tx
used by InsertWithTx.
---
Outside diff comments:
In `@internal/adapters/postgres/transaction_validation_repository.go`:
- Around line 109-124: Reject nil validation before acquiring the DB connection
by adding a small preflight check and reusing it from both entrypoints: move the
nil check for *model.TransactionValidation into a helper (e.g.,
validateNotNilValidation or preflightValidation) and call it at the top of
Insert (before r.conn.GetDB()) and in the other public entrypoint that currently
runs at lines ~147-163; keep insertInternal unchanged (it can still assume a
non-nil validation) and return a deterministic error when validation is nil
instead of proceeding to GetDB().
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: ASSERTIVE
Plan: Pro
Run ID: 16357a18-b9bb-4e45-8dcf-dbdcd855cad9
📒 Files selected for processing (8)
.github/workflows/build.yml.github/workflows/go-combined-analysis.yml.github/workflows/gptchangelog.yml.github/workflows/pr-security-scan.yml.github/workflows/pr-validation.yml.github/workflows/release.ymlinternal/adapters/postgres/transaction_validation_repository.gointernal/adapters/postgres/transaction_validation_repository_test.go
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In @.github/workflows/build.yml:
- Line 10: Replace the mutable tag for the reusable workflow reference
"LerianStudio/github-actions-shared-workflows/.github/workflows/build.yml@v1.18.1"
with the immutable commit SHA "09f3e9acbe91c58bac6997b4f6f7f2584a6200c5" in both
places it appears (the current occurrences using `@v1.18.1`); update the string to
"LerianStudio/github-actions-shared-workflows/.github/workflows/build.yml@09f3e9acbe91c58bac6997b4f6f7f2584a6200c5"
so the workflow is pinned to the exact commit.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: ASSERTIVE
Plan: Pro
Run ID: aa391465-582a-4ffd-b9b8-863f2e3d7dae
📒 Files selected for processing (2)
.github/workflows/build.ymlDockerfile
| jobs: | ||
| build: | ||
| uses: LerianStudio/github-actions-shared-workflows/.github/workflows/build.yml@v1.15.0 | ||
| uses: LerianStudio/github-actions-shared-workflows/.github/workflows/build.yml@v1.18.1 |
There was a problem hiding this comment.
🧹 Nitpick | 🔵 Trivial
🧩 Analysis chain
🌐 Web query:
What does GitHub Actions security guidance recommend for referencing external reusable workflows: mutable tags (e.g., @v1.18.1) or immutable commit SHAs?
💡 Result:
GitHub Actions security guidance recommends referencing external reusable workflows by an immutable full commit SHA, not by a mutable ref (branch or tag like @v1 or @v1.18.1). GitHub’s reusable-workflows documentation explicitly says {ref} can be a SHA, tag, or branch, but that “using the commit SHA is the safest option for stability and security.” [1]. GitHub also recommends SHA pinning more generally to reduce supply-chain risk from compromised/moved refs [2].
Answer: immutable commit SHAs. [1][2]
🏁 Script executed:
# Check if the file exists and read lines 1-30 to see context
if [ -f .github/workflows/build.yml ]; then
echo "=== File exists, showing lines 1-30 ==="
head -30 .github/workflows/build.yml | cat -n
else
echo "File .github/workflows/build.yml not found"
fiRepository: LerianStudio/tracer
Length of output: 1071
🌐 Web query:
GitHub LerianStudio/github-actions-shared-workflows v1.18.1 commit SHA
💡 Result:
LerianStudio/github-actions-shared-workflows v1.18.1 points to commit SHA:
09f3e9acbe91c58bac6997b4f6f7f2584a6200c5 [1]
Pin reusable workflows to immutable commit SHAs.
Lines 10 and 27 use mutable tags (@v1.18.1). GitHub security guidance recommends pinning to exact commit SHAs to reduce supply-chain risk and prevent unexpected behavior changes. For v1.18.1, use commit SHA 09f3e9acbe91c58bac6997b4f6f7f2584a6200c5.
🔒 Proposed hardening
- uses: LerianStudio/github-actions-shared-workflows/.github/workflows/build.yml@v1.18.1
+ uses: LerianStudio/github-actions-shared-workflows/.github/workflows/build.yml@09f3e9acbe91c58bac6997b4f6f7f2584a6200c5
...
- uses: LerianStudio/github-actions-shared-workflows/.github/workflows/gitops-update.yml@v1.18.1
+ uses: LerianStudio/github-actions-shared-workflows/.github/workflows/gitops-update.yml@09f3e9acbe91c58bac6997b4f6f7f2584a6200c5📝 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.
| uses: LerianStudio/github-actions-shared-workflows/.github/workflows/build.yml@v1.18.1 | |
| uses: LerianStudio/github-actions-shared-workflows/.github/workflows/build.yml@09f3e9acbe91c58bac6997b4f6f7f2584a6200c5 |
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In @.github/workflows/build.yml at line 10, Replace the mutable tag for the
reusable workflow reference
"LerianStudio/github-actions-shared-workflows/.github/workflows/build.yml@v1.18.1"
with the immutable commit SHA "09f3e9acbe91c58bac6997b4f6f7f2584a6200c5" in both
places it appears (the current occurrences using `@v1.18.1`); update the string to
"LerianStudio/github-actions-shared-workflows/.github/workflows/build.yml@09f3e9acbe91c58bac6997b4f6f7f2584a6200c5"
so the workflow is pinned to the exact commit.
- Add span trade-off documentation comment in InsertWithTx nil check - Add span error annotation for FromEntity failure in insertInternal - Add TODO(T-003) markers for future db parameter wiring in CheckLimitsWithTx - Add TODO(T-003) markers in limit_checker_test.go for future expectations
…methods' of github.com:LerianStudio/tracer into feat/idempotency-key-support--transactional-repository-methods
There was a problem hiding this comment.
Actionable comments posted: 1
♻️ Duplicate comments (1)
internal/services/query/limit_checker.go (1)
80-92:⚠️ Potential issue | 🔴 Critical
CheckLimitsWithTxstill isn't transactional.
dbis discarded and this method delegates into the same repository path asCheckLimits, so counter mutations can still commit even if the caller rolls back its surrounding transaction. The inline TODO documents the gap, but shipping aWithTxAPI before the repositories honor that handle is still unsafe. Please wiredbthrough repositoryWithTxmethods before exposing this entrypoint, or fail fast/remove it for now.Also applies to: 143-164
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@internal/services/query/limit_checker.go` around lines 80 - 92, The CheckLimitsWithTx entrypoint is not actually transactional because it discards the db parameter and delegates to the same repository path as CheckLimits; either thread the provided db (pgdb.DB) through the underlying repositories using their WithTx (or equivalent) methods so all counter mutations use the caller's transaction, or remove/fail-fast this API until repositories accept the transactional handle; update CheckLimitsWithTx to accept *model.CheckLimitsInput and return *model.CheckLimitsOutput as before but call repo.WithTx(db).<method>…</method> (use the repository names and WithTx methods found in the same package) so that all calls that mutate counters use the transactional repo handle instead of the non-transactional path.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@internal/services/query/limit_checker_test.go`:
- Around line 5304-5305: Replace any TODO comments that include task/ticket IDs
(e.g., "TODO(T-003)") with plain "TODO" so they comply with the new no
task/ticket IDs rule; specifically update the comment containing "TODO(T-003):
When repos support WithTx, add gomock.Eq(mockDB) expectations..." to remove
"(T-003)" and do the same for the other occurrence of "TODO(T-003)" around the
later comment (previously noted at the other location), leaving the rest of the
comment text unchanged.
---
Duplicate comments:
In `@internal/services/query/limit_checker.go`:
- Around line 80-92: The CheckLimitsWithTx entrypoint is not actually
transactional because it discards the db parameter and delegates to the same
repository path as CheckLimits; either thread the provided db (pgdb.DB) through
the underlying repositories using their WithTx (or equivalent) methods so all
counter mutations use the caller's transaction, or remove/fail-fast this API
until repositories accept the transactional handle; update CheckLimitsWithTx to
accept *model.CheckLimitsInput and return *model.CheckLimitsOutput as before but
call repo.WithTx(db).<method>…</method> (use the repository names and WithTx
methods found in the same package) so that all calls that mutate counters use
the transactional repo handle instead of the non-transactional path.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: ASSERTIVE
Plan: Pro
Run ID: af93c69c-5958-409d-9825-ef43563eb596
📒 Files selected for processing (3)
internal/adapters/postgres/transaction_validation_repository.gointernal/services/query/limit_checker.gointernal/services/query/limit_checker_test.go
| // TODO(T-003): When repos support WithTx, add gomock.Eq(mockDB) expectations | ||
| // to verify db parameter is forwarded correctly. |
There was a problem hiding this comment.
Remove task IDs from these TODO comments.
TODO(T-003) conflicts with the new no task/ticket IDs rule introduced in this PR. Use plain TODO text here and keep the work-item reference in PR/issue metadata instead.
Also applies to: 5487-5487
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@internal/services/query/limit_checker_test.go` around lines 5304 - 5305,
Replace any TODO comments that include task/ticket IDs (e.g., "TODO(T-003)")
with plain "TODO" so they comply with the new no task/ticket IDs rule;
specifically update the comment containing "TODO(T-003): When repos support
WithTx, add gomock.Eq(mockDB) expectations..." to remove "(T-003)" and do the
same for the other occurrence of "TODO(T-003)" around the later comment
(previously noted at the other location), leaving the rest of the comment text
unchanged.
4 participants
Summary
Add
WithTxvariants alongside existing repository methods, enabling transactional usage without breaking existing non-transactional callers. This is the foundation for atomic deduplication in the next PR.Changes
Repository Layer
InsertWithTx(ctx, db, entity): Inserts transaction validation using providedpgdb.DBparameterinsertInternal: Extracted shared logic betweenInsertandInsertWithTx(DRY)dbis nilService Layer
CheckLimitsWithTx(ctx, db, input): Acceptspgdb.DBfor API consistency (db param reserved for future transactional repo support)checkLimitsInternal: Extracted shared logic betweenCheckLimitsandCheckLimitsWithTx(DRY)Interfaces
command.TransactionValidationRepository: AddedInsertWithTxquery.LimitChecker: AddedCheckLimitsWithTxCode Quality
Design Decisions
txpassed aspgdb.DBparameter (no TxManager)WithTxmethods are additive -- existing methods unchanged, backward compatibleCheckLimitsWithTxacceptsdbfor API consistency; underlying repos will use it when they support transactional operationsBackward Compatibility
InsertandCheckLimitsmethods unchanged