fix: batch resource apply and fix variable sync status code #57
fix: batch resource apply and fix variable sync status code #57zacharyblasczyk merged 2 commits intomainfrom
Conversation
…r and would overwrite eachother with the apply command
📝 WalkthroughWalkthroughThe apply command now partitions specs into resource-item specs and others; it batch-upserts resource-item specs per provider via a new BatchUpsertResources, syncs each resource's variables, then batch-applies remaining specs and aggregates all results for reporting and error checking. Changes
Sequence DiagramsequenceDiagram
participant CLI as Apply Command
participant Split as Partition Specs
participant Batch as BatchUpsertResources
participant Provider as Provider API
participant Sync as Variable Sync
participant Engine as DefaultProviderEngine
CLI->>Split: sort & partition specs
Split->>Batch: resource-item specs
Batch->>Provider: SetResourceProviderResourcesWithResponse (per provider)
Provider-->>Batch: 202 Accepted + resource results
Batch->>Sync: sync variables for each resource
Sync-->>Batch: per-resource sync results
Batch-->>CLI: aggregated resource results
Split->>Engine: non-resource specs
Engine-->>CLI: batch apply results
CLI->>CLI: aggregate all results & report
Estimated Code Review Effort🎯 3 (Moderate) | ⏱️ ~20 minutes Possibly Related PRs
Suggested Reviewers
Poem
🚥 Pre-merge checks | ✅ 2 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (2 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches
🧪 Generate unit tests (beta)
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. Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 2
🧹 Nitpick comments (2)
cmd/ctrlc/root/apply/cmd.go (2)
109-115: Consider exportingresourceTypeNameto avoid a fragile cross-package string literal.The
"Resource"string at Line 110 must stay in sync with the unexportedresourceTypeNameconstant ininternal/api/providers/resource.go. A silent mismatch would cause all resource specs to fall through toBatchApplyinstead ofBatchUpsertResources, reverting the bug this PR fixes.Exporting the constant (e.g.,
ResourceTypeName) or adding a type-assertion helper eliminates this risk.♻️ Suggested approach — export the constant
In
internal/api/providers/resource.go:-const resourceTypeName = "Resource" +const ResourceTypeName = "Resource"In
cmd/ctrlc/root/apply/cmd.go:- if ts.Type == "Resource" { + if ts.Type == providers.ResourceTypeName {🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@cmd/ctrlc/root/apply/cmd.go` around lines 109 - 115, The code is using a hard-coded "Resource" string to detect resource items which must match the unexported constant resourceTypeName in internal/api/providers/resource.go; export that constant (e.g., ResourceTypeName) or add a helper function to avoid fragile string literals, then update the check in cmd/ctrlc/root/apply/cmd.go (the loop over sortedSpecs that currently compares ts.Type == "Resource") to use the exported ResourceTypeName (or the helper) so resource specs are correctly routed to BatchUpsertResources instead of falling through to BatchApply; ensure providers.ResourceItemSpec type-assertion remains and tests still cover BatchUpsertResources behavior.
107-131: Document the intentional ordering choice or refactor to preserve sort semantics.
sortSpecsByOrdersorts specs in descending order byOrder(), with Resource specs at 300. Currently, only theResourceProvider(Order 300) is registered, so no ordering violation occurs. However, the code unconditionally processes all Resource specs before other specs, which would silently break the sort contract if a future provider is added withOrder() > 300.Either document that Resource specs always apply first regardless of sort order, or refactor to interleave the two groups while respecting their relative priority.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@cmd/ctrlc/root/apply/cmd.go` around lines 107 - 131, The current code extracts all Resource specs and applies them before other specs, which breaks the descending sort semantics from sortSpecsByOrder and could mis-order future providers; instead iterate the sortedSpecs slice in order and flush contiguous runs: when you encounter a Resource (providers.ResourceItemSpec) accumulate it into a resource batch and when the next item is non-Resource call providers.BatchUpsertResources(applyCtx, resourceBatch) and clear the batch; likewise accumulate non-Resource typed specs and flush them via providers.DefaultProviderEngine.BatchApply(applyCtx, otherBatch, providers.BatchApplyOptions{}) when the next item is a Resource or at the end—this preserves the original sort order while keeping the existing BatchUpsertResources and BatchApply calls.
🤖 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/api/providers/resource.go`:
- Around line 234-245: The Result.Action is incorrectly hardcoded to "updated"
in the loop that builds per-resource results; change the action value to
"upserted" so SetResourceProviderResources (the upsert operation) accurately
reports creations and matches the documented Result.Action values. Locate the
loop that constructs Result objects (the Result struct instantiation in the for
_, spec := range group block) and replace Action: "updated" with Action:
"upserted" so results reflect upsert semantics.
- Around line 150-153: Check createResp.JSON202 for nil before accessing its Id:
if createResp.StatusCode() != http.StatusAccepted return the existing error,
else if createResp.JSON202 == nil return a clear error (e.g., "received 202 but
response body empty or malformed") instead of dereferencing; update the return
site that currently uses createResp.JSON202.Id to first guard with a nil check
and then return createResp.JSON202.Id when non-nil.
---
Nitpick comments:
In `@cmd/ctrlc/root/apply/cmd.go`:
- Around line 109-115: The code is using a hard-coded "Resource" string to
detect resource items which must match the unexported constant resourceTypeName
in internal/api/providers/resource.go; export that constant (e.g.,
ResourceTypeName) or add a helper function to avoid fragile string literals,
then update the check in cmd/ctrlc/root/apply/cmd.go (the loop over sortedSpecs
that currently compares ts.Type == "Resource") to use the exported
ResourceTypeName (or the helper) so resource specs are correctly routed to
BatchUpsertResources instead of falling through to BatchApply; ensure
providers.ResourceItemSpec type-assertion remains and tests still cover
BatchUpsertResources behavior.
- Around line 107-131: The current code extracts all Resource specs and applies
them before other specs, which breaks the descending sort semantics from
sortSpecsByOrder and could mis-order future providers; instead iterate the
sortedSpecs slice in order and flush contiguous runs: when you encounter a
Resource (providers.ResourceItemSpec) accumulate it into a resource batch and
when the next item is non-Resource call providers.BatchUpsertResources(applyCtx,
resourceBatch) and clear the batch; likewise accumulate non-Resource typed specs
and flush them via providers.DefaultProviderEngine.BatchApply(applyCtx,
otherBatch, providers.BatchApplyOptions{}) when the next item is a Resource or
at the end—this preserves the original sort order while keeping the existing
BatchUpsertResources and BatchApply calls.
ℹ️ Review info
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Disabled knowledge base sources:
- Linear integration is disabled
You can enable these sources in your CodeRabbit configuration.
📒 Files selected for processing (2)
cmd/ctrlc/root/apply/cmd.gointernal/api/providers/resource.go
| if createResp.StatusCode() != http.StatusAccepted { | ||
| return "", fmt.Errorf("failed to create resource provider: %s", createResp.Status()) | ||
| } | ||
| return createResp.JSON202.Id, nil |
There was a problem hiding this comment.
Guard against nil createResp.JSON202 before dereferencing.
The status check at Line 150 (200→202 fix) is correct. However, createResp.JSON202 is populated by the generated client only when the response body deserializes successfully. A 202 with an empty or malformed body leaves JSON202 nil, causing a nil-pointer panic at Line 153.
🛡️ Proposed fix
if createResp.StatusCode() != http.StatusAccepted {
return "", fmt.Errorf("failed to create resource provider: %s", createResp.Status())
}
+if createResp.JSON202 == nil {
+ return "", fmt.Errorf("failed to create resource provider: empty response body")
+}
return createResp.JSON202.Id, nil📝 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.
| if createResp.StatusCode() != http.StatusAccepted { | |
| return "", fmt.Errorf("failed to create resource provider: %s", createResp.Status()) | |
| } | |
| return createResp.JSON202.Id, nil | |
| if createResp.StatusCode() != http.StatusAccepted { | |
| return "", fmt.Errorf("failed to create resource provider: %s", createResp.Status()) | |
| } | |
| if createResp.JSON202 == nil { | |
| return "", fmt.Errorf("failed to create resource provider: empty response body") | |
| } | |
| return createResp.JSON202.Id, nil |
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@internal/api/providers/resource.go` around lines 150 - 153, Check
createResp.JSON202 for nil before accessing its Id: if createResp.StatusCode()
!= http.StatusAccepted return the existing error, else if createResp.JSON202 ==
nil return a clear error (e.g., "received 202 but response body empty or
malformed") instead of dereferencing; update the return site that currently uses
createResp.JSON202.Id to first guard with a nil check and then return
createResp.JSON202.Id when non-nil.
There was a problem hiding this comment.
This is a pre-existing pattern throughout the codebase — not something I introduced.
Please create an issue on this gh repo to come back and fix this later and more broadly.
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
# Search for patterns where JSON response fields are dereferenced without nil checks
# Looking for patterns like .JSON200, .JSON201, .JSON202 being accessed directly
echo "=== Searching for potential nil dereference patterns ==="
rg -n "\.JSON\d{3}\." --type go -A 1 -B 2
echo ""
echo "=== Count of direct JSON response field accesses ==="
rg "\.JSON\d{3}\." --type go | wc -l
Length of output: 9882
🏁 Script executed:
#!/bin/bash
# Create GitHub issue for tracking the nil pointer dereference pattern
gh issue create \
--repo ctrlplanedev/cli \
--title "Add nil checks for generated API client response bodies" \
--body "## Description
Throughout the codebase, generated API client response fields (e.g., \`JSON200\`, \`JSON202\`, \`JSON201\`) are accessed directly without nil checks. These fields are only populated when the response body deserializes successfully. A response with the expected status code but an empty or malformed body will leave these fields nil, causing a nil pointer panic.
## Context
This pattern was identified during review of PR `#57` (specifically in \`internal/api/providers/resource.go\` around line 153 where \`createResp.JSON202.Id\` is accessed), but it appears to be a widespread pattern throughout the codebase.
## Proposed Solution
Add nil checks before dereferencing any generated client response body fields, for example:
- Check if \`resp.JSON200 == nil\` before accessing \`resp.JSON200.Field\`
- Return a descriptive error when the response body is nil despite receiving the expected status code
## Related
- PR: https://github.com/ctrlplanedev/cli/pull/57
- Comment: https://github.com/ctrlplanedev/cli/pull/57#discussion_r2854507891
- Requested by: `@zacharyblasczyk`" \
--assignee zacharyblasczyk
Length of output: 45
@zacharyblasczyk Understood — I've created issue #58 to track adding nil checks for generated API client response bodies across the codebase. The search found ~48 instances of this pattern, confirming it's a broader issue that makes sense to address systematically in a separate PR.
| func (r *ResourceItemSpec) getProviderID(ctx Context) (string, error) { | ||
| providerName := r.Provider | ||
| if providerName == "" { | ||
| providerName = "ctrlc-apply" |
There was a problem hiding this comment.
There was a problem hiding this comment.
♻️ Duplicate comments (1)
internal/api/providers/resource.go (1)
150-153:⚠️ Potential issue | 🟡 MinorAvoid a nil-pointer panic when reading provider create response.
Line 153 dereferences
createResp.JSON202.Idwithout guardingcreateResp.JSON202. A202 Acceptedwith an empty/malformed body can still makeJSON202nil.🛠️ Proposed fix
if createResp.StatusCode() != http.StatusAccepted { return "", fmt.Errorf("failed to create resource provider: %s", createResp.Status()) } +if createResp.JSON202 == nil { + return "", fmt.Errorf("failed to create resource provider: empty response body") +} return createResp.JSON202.Id, nil#!/bin/bash set -euo pipefail echo "Inspect the call site in getProviderID:" rg -n -C3 'RequestResourceProviderUpsertWithResponse|createResp\.StatusCode\(\)|createResp\.JSON202\.Id' echo echo "Inspect generated response type to confirm JSON202 is a pointer field:" rg -n -C3 'type .*RequestResourceProviderUpsert.*Response|JSON202\s+\*'🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@internal/api/providers/resource.go` around lines 150 - 153, getProviderID currently dereferences createResp.JSON202.Id without checking if createResp.JSON202 is nil, which can panic on an empty/malformed 202 body; fix by verifying createResp.JSON202 != nil before accessing Id (in the code path handling RequestResourceProviderUpsertWithResponse), and if it's nil return a clear error (e.g. fmt.Errorf("failed to create resource provider: %s - empty response body", createResp.Status())) or include raw response body/status if available to aid debugging instead of dereferencing NULL.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Duplicate comments:
In `@internal/api/providers/resource.go`:
- Around line 150-153: getProviderID currently dereferences
createResp.JSON202.Id without checking if createResp.JSON202 is nil, which can
panic on an empty/malformed 202 body; fix by verifying createResp.JSON202 != nil
before accessing Id (in the code path handling
RequestResourceProviderUpsertWithResponse), and if it's nil return a clear error
(e.g. fmt.Errorf("failed to create resource provider: %s - empty response body",
createResp.Status())) or include raw response body/status if available to aid
debugging instead of dereferencing NULL.
ℹ️ Review info
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Disabled knowledge base sources:
- Linear integration is disabled
You can enable these sources in your CodeRabbit configuration.
📒 Files selected for processing (1)
internal/api/providers/resource.go
|
Approved by @adityachoudhari26 verbally. |
1 participant
Summary
ctrlc applyglob patterns to batch resources by provider, so all resources in a single apply are sent together instead of one-at-a-time (which caused each call to overwrite the previous)200 OKwhen the API returns202 AcceptedChanges
Batch resource upsert (
internal/api/providers/resource.go)BatchUpsertResources— groups resources by provider name and makes oneSetResourceProviderResourcescall per provider with all resources in that group, then syncs variables individuallyctrlc apply -f "instances/**/instance.yaml") called the API once per resource. SinceSetResourceProviderResourcesis a replace-all operation, each call overwrote the previous — only the last resource survived per provider.Wire up batch path (
cmd/ctrlc/root/apply/cmd.go)Resourcespecs from other types before applyingResourcespecs throughBatchUpsertResources; other types continue through the normalBatchApplyframeworkFix provider creation status check (
internal/api/providers/resource.go)getProviderIDchecked forhttp.StatusOK(200) butRequestResourceProviderUpsertreturns202 Accepted— changed tohttp.StatusAcceptedSummary by CodeRabbit
Refactor
Bug Fixes