fix(purchases): handle empty plan_id on direct-execute approval (closes #395) - #412
Merged
cristim merged 1 commit intoMay 14, 2026
Conversation
#395) PR #373 made approve synchronous, so executePurchase now runs inline from ApproveAndExecute. For direct-execute purchases created via the Opportunities "Purchase" button there is no associated plan and exec.PlanID is "". GetPurchasePlan(ctx, "") hits the UUID-typed purchase_plans.id column and crashes with SQLSTATE 22P02, leaving the execution stuck in approved status without ever running the purchase. executePurchase now short-circuits the plan/accounts fetch when PlanID is empty, synthesises a placeholder PurchasePlan{Name: "Direct purchase"} for the downstream history-record write + confirmation email, and falls through to the single-account path. updatePlanProgress gains a guard at the top so the post-execution progress update doesn't hit the same error. SavePurchaseExecution (store_postgres.go:719) and SavePurchaseHistory (store_postgres.go:1042) already handle empty PlanID by passing NULL to the nullable column, so the persistence layer is untouched. Adds TestManager_ApproveAndExecute_EmptyPlanID as a regression guard that asserts the empty-PlanID approval path never calls GetPurchasePlan / GetPlanAccounts / UpdatePurchasePlan and lands the execution at status "completed".
Contributor
|
Warning Rate limit exceeded
You’ve run out of usage credits. Purchase more in the billing tab. ⌛ How to resolve this issue?After the wait time has elapsed, a review can be triggered using the We recommend that you space out your commits to avoid hitting the rate limit. 🚦 How do rate limits work?CodeRabbit enforces hourly rate limits for each developer per organization. Our paid plans have higher rate limits than the trial, open-source and free plans. In all cases, we re-allow further reviews after a brief timeout. Please see our FAQ for further information. ℹ️ Review info⚙️ Run configurationConfiguration used: defaults Review profile: CHILL Plan: Pro Run ID: 📒 Files selected for processing (2)
✨ Finishing Touches🧪 Generate unit tests (beta)
Comment |
Member
Author
|
@coderabbitai review |
Contributor
✅ Actions performedReview triggered.
|
This was referenced May 14, 2026
cristim
added a commit
that referenced
this pull request
May 19, 2026
#454) * fix(purchases): reconstruct typed Details pointer for every service (closes #453) After #373 made approve synchronous, every AWS purchase from the dashboard failed with `invalid service details for <Service>`. executeSinglePurchase assigned a value-typed common.DatabaseDetails (and only when rec.Engine was non-empty); every AWS findOfferingID type-asserts a *pointer*, so the assertion failed and the error propagated up as "some purchases failed: [<resource>: purchase failed: failed to find offering: invalid service details for <Service>]" for every service. This change preserves the full ServiceDetails payload across the dashboard → DB → executePurchase boundary so per-rec platform, tenancy, AZ-config, plan-type etc. round-trip correctly: - pkg/common/service_details_codec.go: new Marshal/Decode helpers backed by a per-service-slug table. Empty payload on a known slug yields a zero- valued typed pointer (legacy-row graceful degradation); unknown slug returns nil; malformed JSON surfaces as an error. - internal/config/types.go: RecommendationRecord gains a json.RawMessage Details column persisted alongside the existing engine column. Stored as raw JSON because internal/config must not import pkg/common. - internal/scheduler/scheduler.go (convertRecommendations): marshals the per-rec ServiceDetails on collection. Marshal errors are logged and the row is persisted without Details so the graceful-degradation path applies. - internal/purchase/execution.go (executeSinglePurchase): replaces the value-typed DatabaseDetails block with DecodeServiceDetailsFor + applyEngineFallback. The fallback backfills Engine from the persisted column onto DB/Cache Details so legacy rows that lacked the full payload don't silently mis-purchase as the default engine. Per-service map of what Details type is now handed to the cloud client: - EC2 → *common.ComputeDetails (Platform/Tenancy/Scope) - RDS → *common.DatabaseDetails (Engine/AZConfig/InstanceClass) - ElastiCache → *common.CacheDetails (Engine/NodeType) - OpenSearch → *common.SearchDetails (forward-compat; client doesn't assert) - Redshift → *common.DataWarehouseDetails (forward-compat; client doesn't assert) - MemoryDB → nil (no *Details type; client doesn't assert) - Savings Plans (all variants) → *common.SavingsPlanDetails (PlanType/HourlyCommitment) Azure / GCP service clients don't type-assert rec.Details (they read rec.ResourceType / rec.Term / rec.Count directly), so they're unaffected by either the bug or the fix. Independent of #412 (empty plan_id, merged); the two fixes compose to unblock the full dashboard approve flow after #373. Tests: - internal/purchase/coverage_extra_test.go: TestManager_ExecuteSinglePurchase_LegacyEmptyDetails table-drives EC2/RDS/ElastiCache through executePurchase with an empty Details payload, captures the rec.Details handed to the mock service client, and asserts the concrete pointer type plus Engine backfill where applicable. TestApplyEngineFallback pins the helper's per-type behaviour. - pkg/common/service_details_codec_test.go: round-trip + legacy-empty + unknown-service + malformed-JSON + legacy-dash-form SP coverage. Test counts: go test ./... → 4454 pass across 37 packages. go vet ./... clean. * fix(purchases): apply CR nit — bytes.Equal for JSON null check Replace `string(raw) == "null"` with `bytes.Equal(raw, jsonNullBytes)` in the ServiceDetails codec to avoid the string conversion allocation on the read-back path (every purchase routes through DecodeServiceDetailsFor). Same treatment for MarshalServiceDetails' nil-pointer-folding step. CodeRabbit nit on PR #454. No behaviour change.
This was referenced May 19, 2026
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
1 participant
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
executePurchaseshort-circuits the plan/accounts fetch whenPlanIDis empty, synthesises a*config.PurchasePlan{Name: "Direct purchase"}placeholder for the downstream history-record write + confirmation email, and falls through to the single-account legacy path.updatePlanProgressgains a top-levelif planID == "" { return nil }guard so the post-execution hook stops hitting the same SQLSTATE 22P02.SavePurchaseExecution(line 719) andSavePurchaseHistory(line 1042) already passNULLfor empty PlanID on the UUID column, so the persistence layer is unchanged.Root cause
After #373 made approve synchronous,
executePurchaseruns inline fromApproveAndExecute. For purchases created via the Opportunities "Purchase" button there is no plan andexec.PlanID == "".GetPurchasePlan(ctx, "")rejects on the UUID column withERROR: invalid input syntax for type uuid: "" (SQLSTATE 22P02). The execution row is left inapprovedstatus without ever firing.Test plan
go test ./internal/purchase/...— 105 pass, including new regressionTestManager_ApproveAndExecute_EmptyPlanID.go test ./internal/api/...— 1081 pass.go test ./internal/...— 3700 pass across 24 packages.go vet ./...clean.GetPurchasePlan/GetPlanAccounts/UpdatePurchasePlanare never called for empty-PlanIDexecutions, and that the execution lands atstatus=completed.Closes #395