fix: Propagates API errors during delete, read, and wait operations of auto-generated resources - #4676
Conversation
| if notFound(callResult.Body, callResult.Resp) { | ||
| return nil, callResult.Resp, fmt.Errorf("resource not found") | ||
| } |
There was a problem hiding this comment.
The check for processing 404 was not reachable as it was being catched in if statement above (callResult.Err != nil). Only case this if captured was a 200 empty body {} with returned with a resource not found, now it will be hanlded as a regular successful response with no results.
|
will wait on CI execution before merging |
|
APIx bot: a message has been sent to Docs Slack channel |
🤖 Augment PR SummarySummary: This PR corrects error handling for auto-generated resource lifecycle operations.
🤖 Was this summary useful? React with 👍 or 👎 |
![augmentcode[bot]](https://avatars.githubusercontent.com/in/1027498?s=60&v=4){kind=small}
There was a problem hiding this comment.
Pull request overview
This PR fixes a systemic error-handling bug in the auto-generated (serviceapi) resource framework where API failures during read/delete/wait were incorrectly treated as “resource not found”, causing Terraform state to be removed or waits to succeed early. It introduces a clear not-found classification (HTTP 404 or an explicit sentinel) and adds hooks and unit tests to validate behavior across status codes.
Changes:
- Tightens “not found” detection to HTTP 404 or
autogen.ErrNotFound, and propagates all other API errors for read/delete/wait flows. - Adds service-specific hooks for known API quirks (search deployment empty-body “not found”, service account secret missing-from-list) using the new sentinel.
- Adds unit tests using
httptest+ realconfig.MongoDBClientto exercise error propagation and not-found behavior across operations.
Reviewed changes
Copilot reviewed 11 out of 11 changed files in this pull request and generated 2 comments.
Show a summary per file
| File | Description |
|---|---|
| tools/codegen/config.yml | Updates wait-state comments around DELETED semantics (not-found handling) |
| internal/serviceapi/serviceaccountsecret/resource_custom_hooks.go | Wraps missing-secret condition with autogen.ErrNotFound |
| internal/serviceapi/serviceaccountsecret/resource_custom_hooks_internal_test.go | Adds tests for service account secret post-read hook behavior |
| internal/serviceapi/searchdeploymentapi/resource_custom_hooks.go | Adds post-read hook mapping empty JSON bodies to autogen.ErrNotFound |
| internal/serviceapi/searchdeploymentapi/resource_custom_hooks_internal_test.go | Adds tests for search deployment empty-body not-found mapping |
| internal/serviceapi/projectserviceaccountsecret/resource_custom_hooks.go | Wraps missing-secret condition with autogen.ErrNotFound |
| internal/serviceapi/projectserviceaccountsecret/resource_custom_hooks_internal_test.go | Adds tests for project service account secret post-read hook behavior |
| internal/common/autogen/unmarshal.go | Uses exported IsEmptyJSON helper |
| internal/common/autogen/handle_operations.go | Refactors not-found logic to use HTTP 404 or ErrNotFound; exports IsEmptyJSON |
| internal/common/autogen/handle_operations_internal_test.go | Adds unit tests covering delete/read/data source read list/wait refresh error propagation |
| .changelog/4676.txt | Adds release notes for affected resources |
💡 Configure MCP servers for context-aware, tailored reviews. Learn more in the docs.
| // notFound returns if the API result indicates the resource is not found: | ||
| // an HTTP 404 response, or a hook signaling it via ErrNotFound. |
There was a problem hiding this comment.
Rephrased comment slightly. The sentinel gives hooks one consistent way to flag "resource not found" regardless of what the API actually returned, instead of each hook fabricating a fake 404 http.Response that misrepresents the real wire response.
| @@ -0,0 +1,312 @@ | |||
| package autogen | |||
There was a problem hiding this comment.
is this a common pattern? Using tests in the same package?
There was a problem hiding this comment.
Good callout, let me circle back here as this is diverging from common testing pattern in our repo, we have never used *_internal_test.go
EspenAlbert
left a comment
EspenAlbert
left a comment
There was a problem hiding this comment.
nice. Would be great to have a real acceptance test for this also, but understand it might be tricky to trigger
Resolve handle_operations.go conflict: keep both master's waitRefreshResult (#4668) and the ErrNotFound-based notFound. Declare IDLE as a pending state in the refreshFunc test, now required by waitRefreshResult.
|
Merging so we have changes in upcoming release. Will follow up on unit testing structure that was used + assessing feasibility of capturing fixes in an acceptance test (verified fixes locally to be sure). |
Description
Before this fix, auto-generated resources ignored every API error during delete, read, and wait operations and treated the failure as "resource not found". From the user's perspective:
terraform destroyreports a successful destroy even when the Atlas API rejects the delete (400, 401, 409, 500). The resource is removed from Terraform state but still exists in Atlas, with no signal to the operator.terraform planor refresh silently removes the resource from state, so the next apply tries to recreate it. Data sources report "Resource not found" instead of the real API error.Root cause:
callAPIreturns a nil body on any failed call,isEmptyJSON(nil)is true, andcallDelete,handleReadCore, andrefreshFuncconsultednotFoundbeforeErr, so every failure was classified as not-found.Changes:
notFoundnow classifies only genuine not-found: an HTTP 404, or a hook signaling it via the newautogen.ErrNotFoundsentinel. All other errors propagate to diagnostics.PostReadAPICallhook onsearchdeploymentapi, covering both read and delete-wait polling.autogen.ErrNotFoundwhen the secret is absent from the list response, so out-of-band deletion still removes the resource from state.HandleRead,HandleDataSourceRead,HandleDataSourceReadList,callDelete, andrefreshFuncthrough a realconfig.MongoDBClientagainst anhttptestserver across 400/401/404/409/500/204/200 responses.Link to any related issue(s): CLOUDP-437881
Type of change:
Required Checklist: