fix(listutil): gate optional-resource swallow behind explicit flag#52
Conversation
FetchPaginated previously converted any IsOptionalResourceError (400/404)
into (nil, nil) for every caller. The lenience was inherited from
configutil's private helper, where it was intentional for stream_routes,
protos and secret_providers in non-stream deployments; promoting the
helper to a shared package generalized it to consumers that need strict
behavior:
* route list aggregation across services silently dropped a deleted
service's 404 and rendered a short table with no error.
* config dump / config sync trusted (nil, nil) on services /
consumers / ssl / global_rules, producing partial output that the
diff engine would then treat as remote deletions or duplicate
creates against an out-of-sync gateway.
Add an explicit allowOptional bool to FetchPaginated and thread it
through FetchRoutesForServices. Set true at the three configutil call
sites that genuinely tolerate the endpoint being absent
(stream_routes, protos, secret_providers); set false everywhere else.
Closes #50
📝 WalkthroughWalkthroughThis PR adds an ChangesOptional resource error handling refactor
🎯 3 (Moderate) | ⏱️ ~20 minutes Possibly related PRs
🚥 Pre-merge checks | ✅ 5 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (5 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches📝 Generate docstrings
🧪 Generate unit tests (beta)
Comment |
There was a problem hiding this comment.
Pull request overview
This PR updates the shared pkg/listutil pagination/aggregation helpers so that swallowing “optional resource” API errors (HTTP 400/404) is only possible when explicitly opted into, preventing silent partial results in commands like route list and config dump/sync.
Changes:
- Added an
allowOptional boolparameter toFetchPaginatedand threaded it throughFetchRoutesForServices. - Updated
route listandconfigutil.FetchRemoteConfigcall sites to passallowOptional=falsefor required resources and per-service route fetches; kepttrueonly for genuinely optional resources (stream_routes,protos,secret_providers). - Added unit tests covering strict vs. optional behavior for both helpers.
Reviewed changes
Copilot reviewed 4 out of 4 changed files in this pull request and generated 2 comments.
| File | Description |
|---|---|
| pkg/listutil/listutil.go | Adds allowOptional gating for optional-resource error swallowing and updates helper contracts. |
| pkg/listutil/listutil_test.go | Introduces unit tests for strict/lenient behavior of pagination and per-service route aggregation. |
| pkg/cmd/route/list/list.go | Updates route listing to use strict behavior (allowOptional=false) for both direct and aggregated list paths. |
| pkg/cmd/config/configutil/configutil.go | Ensures config dump/sync uses strict behavior for required resources and explicit leniency only for optional endpoints. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| // in use on the target gateway (stream_routes, protos, secret_providers, | ||
| // plugin_metadata). All other callers must pass false so transient failures |
| func TestFetchPaginated_StrictPropagates404(t *testing.T) { | ||
| reg := &httpmock.Registry{} | ||
| reg.Register(http.MethodGet, "/apisix/admin/services", httpmock.StringResponse(http.StatusNotFound, `{"error_msg":"not found"}`)) | ||
|
|
||
| client := newTestClient(reg) | ||
| items, err := FetchPaginated[api.Service](client, "/apisix/admin/services", nil, false) |
- Drop plugin_metadata from the FetchPaginated doc comment; it never
goes through FetchPaginated (configutil.fetchPluginMetadata hand-rolls
/plugins/list + /plugin_metadata/{name}).
- Call reg.Verify(t) in every listutil test so a regression that
short-circuits before issuing the HTTP request can't pass silently.
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (1)
pkg/cmd/route/list/list.go (1)
128-136: ⚡ Quick winAdd a command-path test for strict error propagation in
fetchRoutes.
allowOptional=falseis now part of this command’s contract. Please add a test that verifies/routesor/services400/404 errors are surfaced (not swallowed) in the route-list path.As per coding guidelines:
**/*.go: Write tests for every code change.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@pkg/cmd/route/list/list.go` around lines 128 - 136, Add a unit test that ensures fetchRoutes (the code path invoking listutil.FetchPaginated[api.Route] and listutil.FetchPaginated[api.Service] and then listutil.FetchRoutesForServices) does not swallow HTTP 400/404 errors when allowOptional=false; mock the HTTP client to return 400 or 404 from the /apisix/admin/routes and /apisix/admin/services endpoints and assert that the command returns an error (propagates the error) instead of succeeding or returning empty results, using the same routeQuery/baseQuery parameters used in the function under test so the test exercises the exact command-path behavior.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@pkg/cmd/config/configutil/configutil.go`:
- Around line 115-147: FetchRemoteConfig is not covered by tests to ensure the
boolean allowOptional mapping is passed correctly to listutil functions; add a
unit test for FetchRemoteConfig that stubs/mocks listutil.FetchPaginated and
listutil.FetchRoutesForServices and asserts the boolean argument values: ensure
calls for services, consumers, ssl, global_rules pass allowOptional=false while
calls for stream_routes, protos, secret_providers pass allowOptional=true (and
routes are fetched via FetchRoutesForServices with false), and verify the
aggregated return value matches expected output to lock the contract.
---
Nitpick comments:
In `@pkg/cmd/route/list/list.go`:
- Around line 128-136: Add a unit test that ensures fetchRoutes (the code path
invoking listutil.FetchPaginated[api.Route] and
listutil.FetchPaginated[api.Service] and then listutil.FetchRoutesForServices)
does not swallow HTTP 400/404 errors when allowOptional=false; mock the HTTP
client to return 400 or 404 from the /apisix/admin/routes and
/apisix/admin/services endpoints and assert that the command returns an error
(propagates the error) instead of succeeding or returning empty results, using
the same routeQuery/baseQuery parameters used in the function under test so the
test exercises the exact command-path behavior.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: 1ae4a49b-cca9-4306-b050-13424ebd8cd4
📒 Files selected for processing (4)
pkg/cmd/config/configutil/configutil.gopkg/cmd/route/list/list.gopkg/listutil/listutil.gopkg/listutil/listutil_test.go
| services, err := listutil.FetchPaginated[api.Service](client, "/apisix/admin/services", query, false) | ||
| if err != nil { | ||
| return nil, err | ||
| } | ||
|
|
||
| // API7 EE requires service_id when listing routes with access tokens. | ||
| // Fetch routes per service and aggregate. | ||
| routes, err := listutil.FetchRoutesForServices(client, services, query) | ||
| routes, err := listutil.FetchRoutesForServices(client, services, query, false) | ||
| if err != nil { | ||
| return nil, err | ||
| } | ||
| consumers, err := listutil.FetchPaginated[api.Consumer](client, "/apisix/admin/consumers", query) | ||
| consumers, err := listutil.FetchPaginated[api.Consumer](client, "/apisix/admin/consumers", query, false) | ||
| if err != nil { | ||
| return nil, err | ||
| } | ||
| ssl, err := listutil.FetchPaginated[api.SSL](client, "/apisix/admin/ssls", query) | ||
| ssl, err := listutil.FetchPaginated[api.SSL](client, "/apisix/admin/ssls", query, false) | ||
| if err != nil { | ||
| return nil, err | ||
| } | ||
| globalRules, err := listutil.FetchPaginated[api.GlobalRule](client, "/apisix/admin/global_rules", query) | ||
| globalRules, err := listutil.FetchPaginated[api.GlobalRule](client, "/apisix/admin/global_rules", query, false) | ||
| if err != nil { | ||
| return nil, err | ||
| } | ||
| streamRoutes, err := listutil.FetchPaginated[api.StreamRoute](client, "/apisix/admin/stream_routes", query) | ||
| streamRoutes, err := listutil.FetchPaginated[api.StreamRoute](client, "/apisix/admin/stream_routes", query, true) | ||
| if err != nil { | ||
| return nil, err | ||
| } | ||
| protos, err := listutil.FetchPaginated[api.Proto](client, "/apisix/admin/protos", query) | ||
| protos, err := listutil.FetchPaginated[api.Proto](client, "/apisix/admin/protos", query, true) | ||
| if err != nil { | ||
| return nil, err | ||
| } | ||
| secrets, err := listutil.FetchPaginated[api.Secret](client, "/apisix/admin/secret_providers", query) | ||
| secrets, err := listutil.FetchPaginated[api.Secret](client, "/apisix/admin/secret_providers", query, true) | ||
| if err != nil { |
There was a problem hiding this comment.
🛠️ Refactor suggestion | 🟠 Major | ⚡ Quick win
Add call-site tests for allowOptional mapping in FetchRemoteConfig.
This wiring is correctness-critical, but the provided tests only validate listutil behavior, not this mapping (false for required resources, true for optional ones). Please add a focused FetchRemoteConfig test to lock this contract and prevent silent regressions.
As per coding guidelines: **/*.go: Write tests for every code change.
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@pkg/cmd/config/configutil/configutil.go` around lines 115 - 147,
FetchRemoteConfig is not covered by tests to ensure the boolean allowOptional
mapping is passed correctly to listutil functions; add a unit test for
FetchRemoteConfig that stubs/mocks listutil.FetchPaginated and
listutil.FetchRoutesForServices and asserts the boolean argument values: ensure
calls for services, consumers, ssl, global_rules pass allowOptional=false while
calls for stream_routes, protos, secret_providers pass allowOptional=true (and
routes are fetched via FetchRoutesForServices with false), and verify the
aggregated return value matches expected output to lock the contract.
There was a problem hiding this comment.
Holding off on this one. The mapping is exercised indirectly by the existing pkg/cmd/config/dump and pkg/cmd/config/sync test suites (both call FetchRemoteConfig, both still pass), and the new pkg/listutil tests lock the helper's strict/lenient contract on each side. The flag table is also documented in #50, the PR body, and the commit message.
To assert the literal boolean argument at each call site we'd need to either inject a fetcher interface into configutil or introduce a test seam over the package-level listutil.FetchPaginated. That's a non-trivial refactor for an assertion that catches one failure mode (an accidental boolean flip), so I'd rather leave it out of this PR. Happy to revisit if the mapping becomes more dynamic in the future.
There was a problem hiding this comment.
@shreemaan-abhishek, that's a reasonable trade-off — the indirect coverage from the dump/sync suites plus the listutil unit tests gives a solid safety net, and the refactor cost for injecting a fetcher seam isn't justified for a single boolean-flip guard right now. I'll drop this suggestion.
Would you like me to open a follow-up issue to track the potential future refactor (interface injection into configutil) for when the mapping becomes more dynamic or the call-site count grows?
🧠 Learnings used
Learnt from: shreemaan-abhishek
Repo: api7/a7 PR: 43
File: pkg/cmdutil/exporter_test.go:1-1
Timestamp: 2026-05-26T06:17:24.351Z
Learning: In the api7/a7 repository, Apache 2.0 license headers are intentionally not required on Go source files, including *_test.go files. During code reviews, do not flag issues for missing Apache 2.0 license headers in any .go files.
Closes #50. Stacked on top of #47; please merge #47 first.
Problem
`FetchPaginated` converted every `IsOptionalResourceError` (400/404) into `(nil, nil)`. The leniency was inherited from `configutil`'s original private helper, where it made sense for `stream_routes` / `protos` / `secret_providers` on gateways that don't expose those subsystems. Promoting the helper into a shared package in #42 generalized it to callers that should fail loudly:
Also violates AGENTS.md rule 3 (never suppress errors), and the existing doc comment lied — it told callers to "inspect the returned error" on a path where there was no error to inspect.
Change
`fetchPluginMetadata` doesn't go through `FetchPaginated` (it hand-rolls calls to `/apisix/admin/plugins/list` and `/apisix/admin/plugin_metadata/{name}`), so it already handles its own leniency locally and needs no change.
Tests
New file `pkg/listutil/listutil_test.go` with 7 cases:
Test plan
Summary by CodeRabbit