feat: idempotency — transactional deduplication (flip the switch)#74
Conversation
FNV-1a 32-bit hash converts UUID to int32 for use with pg_advisory_xact_lock. Deterministic, fast, and well-distributed across the int32 range.
- Add ValidateResult struct with Response and IsDuplicate fields - Add transactionValidationQueryRepo dependency for FindByRequestID - Check for existing record before processing (DD-3: Stripe model) - Duplicate requests return cached response, skip rule/limit eval - Add ErrNilTransactionValidationQueryRepo sentinel error - Update constructor to accept and validate query repo dependency
- Handler uses ValidateResult.IsDuplicate to select status code - HTTP 201 Created for new requests (DD-9) - HTTP 200 OK for duplicate requests returning cached response - Wire transactionValidationQueryRepo in bootstrap config
- TestValidate_DuplicateRequestID_ReturnsOriginal - TestValidate_DuplicateRequestID_NoDoubleCount - TestValidate_DuplicateRequestID_NoAudit - TestValidate_DenyByRule_RetryTolerant - TestValidate_DenyByLimit_RollbackAtomic - TestValidate_Review_RollbackAtomic - TestValidationHandler_Validate_ReturnsCorrectStatusCodes (201/200)
Adapt all existing validation service and handler tests to use the new 6-parameter constructor and ValidateResult return type. Add FindByRequestID mock expectations for non-duplicate flows.
Replace TODO(T-003) markers with NOTE comments per PROJECT_RULES.md rule against task IDs in source code.
|
Important Review skippedAuto reviews are disabled on base/target branches other than the default branch. 🗂️ Base branches to auto review (1)
Please check the settings in the CodeRabbit UI or the ⚙️ Run configurationConfiguration used: Organization UI Review profile: ASSERTIVE Plan: Pro Run ID: You can disable this status message by setting the Use the checkbox below for a quick retry:
WalkthroughThis pull request introduces idempotency support for validation requests alongside infrastructure updates. The validation service now checks for duplicate requests by Sequence DiagramsequenceDiagram
participant Client
participant Handler as ValidationHandler
participant Service as ValidationService
participant QueryRepo as TransactionValidationQueryRepository
participant RuleEval as RuleEvaluator
participant LimitChecker
participant CommandRepo as TransactionValidationRepository
Client->>Handler: POST /v1/validations (request_id)
Handler->>Service: Validate(ctx, request)
Service->>QueryRepo: FindByRequestID(ctx, request_id)
alt Duplicate Request Found
QueryRepo-->>Service: existing validation
Service-->>Handler: ValidateResult{Response: cached, IsDuplicate: true}
Handler-->>Client: HTTP 200 OK (cached response)
else New Request (Not Found)
QueryRepo-->>Service: nil (not found)
Service->>RuleEval: Evaluate(ctx, rules)
RuleEval-->>Service: decision (Approve/Deny/Review)
Service->>LimitChecker: CheckLimits(ctx, input)
LimitChecker-->>Service: CheckLimitsOutput{Allowed, Details}
Service->>CommandRepo: Insert(ctx, validation)
CommandRepo-->>Service: ✓ inserted
Service-->>Handler: ValidateResult{Response: new, IsDuplicate: false}
Handler-->>Client: HTTP 201 Created (new response)
end
🚥 Pre-merge checks | ❌ 2❌ Failed checks (1 warning, 1 inconclusive)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. Comment |
|
Consider updating CHANGELOG.md to document this change. If this change doesn't need a changelog entry, add the |
|
This PR is very large (35 files, 2333 lines changed). Consider breaking it into smaller PRs for easier review. |
📊 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.1% |
tracer/internal/testhelper |
0.0% |
tracer/pkg/clock |
50.0% |
tracer/pkg/contextutil |
100.0% |
tracer/pkg/hash |
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
🔒 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 |
| // Write UUID bytes directly - uuid.UUID is a [16]byte array | ||
| h.Write(id[:]) | ||
|
|
||
| return int32(h.Sum32()) |
Check failure
Code scanning / gosec
integer overflow conversion uint32 -> int32 Error
| func HashUUIDToInt32(id uuid.UUID) int32 { | ||
| h := fnv.New32a() | ||
| // Write UUID bytes directly - uuid.UUID is a [16]byte array | ||
| h.Write(id[:]) |
Check warning
Code scanning / gosec
Errors unhandled Warning
|
No description provided. |
The Validate endpoint now returns 201 for new requests and 200 for duplicates, but Swagger only documented 200. Add the 201 response entry and clarify both descriptions.
Add mutation of AccountID pointer inside Scopes to catch shallow copy of nested pointer fields. The append-only check would miss shared pointer references between original and copy.
These 4 comments were scaffolding from the TDD-RED phase and should have been removed when the implementation was added in TDD-GREEN.
Replace time.Now().UTC() with testutil.DefaultTestTime for stable test data across runs. Remove unused time import.
7423e7c to
efdda00
Compare
All POST /v1/validations now return 201 Created (new) or 200 OK (duplicate). Update 16 integration test files: - Change StatusOK to StatusCreated for POST validation assertions - Preserve StatusOK for GET/PUT/PATCH endpoints (limits, rules, etc.) - Update tests 1_1_52 and 1_1_53 for idempotent behavior: duplicate requestId returns cached response (200) instead of re-processing - Use uuid.New() in auth and limit tests to avoid request_id collisions between tests sharing the same database
Summary
Implements idempotent transaction validation: retrying
POST /v1/validationswith the samerequest_idreturns the original result without re-processing.What Changed
ValidateResultstruct: New return type withResponseandIsDuplicatefieldsFindByRequestIDcheck before any processing (Stripe model, DD-3)HashUUIDToInt32: FNV-1a utility for advisory lock keys (DD-8)transactionValidationQueryRepodependency addedDesign Decisions Implemented
request_idonly (Stripe model)txpassed aspgdb.DBparameterrequest_idviapg_advisory_xact_lockON CONFLICT DO NOTHINGBehavior Change
request_idTest Coverage
ValidateResultreturn typeCommits
feat: add HashUUIDToInt32 utility for advisory lock keysfeat: add ValidateResult and duplicate detection in Validate flowfeat: return HTTP 201 for new validations, 200 for duplicatestest: add idempotency unit tests for deduplication flowtest: update existing tests for ValidateResult return typestyle: replace task ID references with descriptive commentsBreaking Changes
ValidationService.Validate()now returns*ValidateResultinstead of*ValidationResponseNewValidationServiceconstructor takes 6 parameters (addedtransactionValidationQueryRepo)POST /v1/validationsreturns HTTP 201 for new requests (was 200)Integration tests note
Integration tests need to be updated to expect HTTP 201 for new validation requests (was 200). This is a deliberate behavior change per DD-9.
Depends On