elastic / elastic/terraform-provider-elasticstack
[duplicate-code] repeated API-call err-check/dispatch triplet across internal/clients/kibanaoapi
- Dominant language
- Go
- Stars
- 209
- Forks
- 151
- Avg merge
- 23h 11m
- Merged PRs (30d)
- 169
Description
## Summary
Nearly every Get/Create/Update/Delete function in `internal/clients/kibanaoapi` repeats the identical 3-statement shape: call the generated API client, check `err != nil` and wrap it with `diagutil.FrameworkDiagFromError`, then dispatch the response through one of `HandleGetTypedResponse` / `HandleMutateTypedResponse` / `HandleGetRawResponse` / `HandleMutateRawResponse` / `diagutil.HandleStatusResponse`. Only the endpoint call, the request/response types, and the accepted status codes vary.
## Duplication Details
### Pattern: err-check + response-dispatch boilerplate copy-pasted per CRUD function
- **Severity**: Medium
- **Occurrences**: 30+ functions across at least 10 files
- **Locations** (representative, not exhaustive):
- `internal/clients/kibanaoapi/agentbuilder_agent.go:31-65` (`GetAgent`, `CreateAgent`, `UpdateAgent`, `DeleteAgent`)
- `internal/clients/kibanaoapi/agentbuilder_skill.go:32-68`
- `internal/clients/kibanaoapi/agentbuilder_tool.go:32-65`
- `internal/clients/kibanaoapi/agentbuilder_workflow.go:42-77`
- `internal/clients/kibanaoapi/exceptions.go:30-113` (8 functions: `GetExceptionList`, `CreateExceptionList`, `UpdateExceptionList`, `DeleteExceptionList`, `GetExceptionListItem`, `CreateExceptionListItem`, `UpdateExceptionListItem`, `DeleteExceptionListItem`)
- `internal/clients/kibanaoapi/maintenance_window.go:30-70` (`GetMaintenanceWindow`, `CreateMaintenanceWindow`, `UpdateMaintenanceWindow`, `DeleteMaintenanceWindow`)
- Similar shapes also present in `dashboards.go`, `data_views.go`, `security_lists.go`, `spaces.go`, `synthetics_monitor.go`, `streams.go`, `slo.go`, `prebuilt_rules.go`
- **Code Sample** (from `agentbuilder_agent.go`, repeated with only the call/type changed):
```go
func GetAgent(ctx context.Context, client *Client, spaceID, agentID string) (*models.Agent, diag.Diagnostics) {
resp, err := client.API.GetAgentBuilderAgentsIdWithResponse(ctx, agentID, kibanautil.SpaceAwarePathRequestEditor(spaceID))
if err != nil {
return nil, diagutil.FrameworkDiagFromError(err)
}
return HandleGetRawResponse[models.Agent](resp.StatusCode(), resp.Body)
}
func DeleteAgent(ctx context.Context, client *Client, spaceID, agentID string) diag.Diagnostics {
resp, err := client.API.DeleteAgentBuilderAgentsIdWithResponse(ctx, agentID, kibanautil.SpaceAwarePathRequestEditor(spaceID))
if err != nil {
return diagutil.FrameworkDiagFromError(err)
}
return diagutil.HandleStatusResponse(resp.StatusCode(), resp.Body, http.StatusOK, http.StatusNotFound)
}
```
The same `if err != nil { return ..., diagutil.FrameworkDiagFromError(err) }` guard, followed by a one-line dispatch call, recurs with zero logic variation in every one of the 30+ functions listed above.
## Impact Analysis
- **Maintainability**: Any future change to error wrapping (e.g. attaching request context, endpoint name, or retry hints to the diagnostic) requires editing 30+ call sites by hand.
- **Bug Risk**: The copy-paste has already started to drift — e.g. `tag.go` uses a different `diagutil.ErrDiag(fmt.Sprintf(...))` variant instead of `diagutil.FrameworkDiagFromError`, showing inconsistency creeping in as new files are added by copying an existing one.
- **Code Bloat**: Dozens of near-identical 4-6 line blocks inflate the package without adding behavior.
## Refactoring Recommendations
1. **Extract a small generic invoke helper in `kibanaoapi` (or `diagutil`)**
- Something like:
```go
func Invoke[R any](call func() (R, error)) (R, diag.Diagnostics) {
resp, err := call()
if err != nil {
var zero R
return zero, diagutil.FrameworkDiagFromError(err)
}
return resp, nil
}
```
and have each Get/Create/Update/Delete function pass only its endpoint call as a closure, then apply the existing `HandleGetTypedResponse`/`HandleMutateTypedResponse`/`HandleStatusResponse` call to the result.
- Estimated effort: medium (mechanical but touches ~30 call sites across ~10 files)
- Benefits: single place to change error-wrapping behavior; removes the risk of new files copying the wrong variant (like `tag.go` already did).
## Implementation Checklist
- [ ] Review duplication findings
- [ ] Prioritize refactoring tasks
- [ ] Create refactoring plan
- [ ] Implement changes
- [ ] Update tests
- [ ] Verify no functionality broken
## Analysis Metadata
- **Analyzed Files**: `internal/clients/kibanaoapi/*.go` (non-test)
- **Detection Method**: Semantic code analysis (grep + manual read confirmation)
- **Commit**: ba7e1649ce8cd374e086de06f3bd872c6fd888cc
> [!WARNING]
>
> Firewall blocked 1 domain
>
> The following domain was blocked by the firewall during workflow execution:
>
> - `api.anthropic.com`
>
> To allow these domains, add them to the `network.allowed` list in your workflow frontmatter:
>
> ```yaml
> network:
> allowed:
> - defaults
> - "api.anthropic.com"
> ```
>
> See [Network Configuration](https://github.github.com/gh-aw/reference/network/) for more information.
>
>
> Generated by [Duplicate Code Detector](https://github.com/elastic/terraform-provider-elasticstack/actions/runs/34823156042) · claude · sonnet50 · 206.9 AIC · ⌖ 36.6 AIC · ⊞ 9K · [◷](https://github.com/search?q=repo%3Aelastic%2Fterraform-provider-elasticstack+is%3Aissue+%22gh-aw-workflow-call-id%3A+elastic%2Fterraform-provider-elasticstack%2Fduplicate-code-detector%22&type=issues)
Contributor guide
No contributing guide indexed for this repository
Research direction
Start by reading the representative CRUD functions in internal/clients/kibanaoapi/agentbuilder_agent.go and exceptions.go, then inspect the existing response helpers and related files such as maintenance_window.go and tag.go. Run the package tests before changing the 30+ call sites. Done means the duplicated invocation handling is consolidated without changing response behavior, tests are updated as needed, and the package tests pass.
Written by the indexing model from the issue text.
Assessment
- Tech stack
- go
- Domain
- backend-api-design
- Issue type
- Refactor
- Difficulty
- 4/5
- Estimated time
- 3-5 days
- Activity status
- Active
- Clarity
- Mostly clear
- Newbie friendliness
- 48/100