Skip to content

listutil: FetchPaginated silently swallows 400/404 instead of propagating #50

@shreemaan-abhishek

Description

@shreemaan-abhishek

Split out from PR #47 review (CodeRabbit + Copilot both flagged it).

Problem

`pkg/listutil/listutil.go` `FetchPaginated` returns `(nil, nil)` whenever the underlying GET fails with a `cmdutil.IsOptionalResourceError` (HTTP 400 or 404):

```go
body, err := client.Get(path, query)
if err != nil {
if cmdutil.IsOptionalResourceError(err) {
return nil, nil
}
return nil, err
}
```

This was inherited from `configutil`'s original unexported helper, where the lenience is intentional: in non-stream deployments `stream_routes` (and similarly `protos`, `secrets`) return 400/404, and `config dump` should treat those as "this resource is not in use here" rather than fail outright.

Promoting that helper into a shared `pkg/listutil` package as part of #42 generalized the lenience to every caller, which is wrong:

  1. `route list` aggregation can hide real errors. If a service is deleted between the `/services` enumeration and the per-service `/routes` fetch, the 404 is silently dropped and the user sees fewer routes with no indication.
  2. `config dump` of required resources can produce partial output. A 400 on `/services` or `/consumers` would currently return `nil` instead of failing — `config sync` could then diff an empty remote against a populated local and propose mass deletions.
  3. Violates AGENTS.md rule test: migrate runtime coverage toward ginkgo e2e #3: "Never suppress errors. Always handle and propagate."
  4. The doc comment on `FetchPaginated` claims callers can "inspect the returned error", but there is no error to inspect when the swallow path fires.

Proposed fix

Add an explicit `allowOptional bool` parameter:

```go
func FetchPaginated[T any](
client *api.Client,
path string,
extraQuery map[string]string,
allowOptional bool,
) ([]T, error)
```

Only convert optional-resource errors into `(nil, nil)` when `allowOptional == true`. Pass `true` only at the call sites where the resource is genuinely optional:

Call site allowOptional
`configutil` `stream_routes` true
`configutil` `protos` true
`configutil` `secrets` (secret_providers) true
`configutil` `plugin_metadata` (the inner fetch) true
`configutil` `services` / `consumers` / `ssl` / `global_rules` false
`route list` aggregation (`FetchRoutesForServices`) false
All `pkg/cmd//list` consumers (if/when migrated) false by default

Thread the same flag through `FetchRoutesForServices` (default `false` so 404 on a per-service `/routes` fetch propagates).

Update the doc comment to describe the actual contract.

Tests

  • Unit: `FetchPaginated` with a 404 mock + `allowOptional=false` should return the error; same with `allowOptional=true` should return `(nil, nil)`.
  • Unit: `FetchRoutesForServices` where one service's `/routes` returns 404 should surface the error (not silently drop).
  • Existing `config dump`/`config sync` tests must still pass with the appropriate `true` flags wired in.

Scope

This is a refactor across:

  • `pkg/listutil/listutil.go`
  • `pkg/cmd/config/configutil/configutil.go` (8 call sites)
  • Any future `pkg/cmd//list` migrations (currently only `route list` consumes `listutil`, and that one wants `false`).

Filed as a follow-up to PR #47 so the original change can land focused on the route-list UX fix.

Metadata

Metadata

Assignees

No one assigned

    Labels

    No labels
    No labels

    Type

    No type
    No fields configured for issues without a type.

    Projects

    No projects

    Milestone

    No milestone

    Relationships

    None yet

    Development

    No branches or pull requests

    Issue actions