-
Notifications
You must be signed in to change notification settings - Fork 0
fix(listutil): gate optional-resource swallow behind explicit flag #52
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Merged
Merged
Changes from all commits
Commits
Show all changes
4 commits
Select commit
Hold shift + click to select a range
3bd64c3
fix(listutil): gate optional-resource swallow behind explicit flag
shreemaan-abhishek c0d56f4
fixup: address review on PR #52
shreemaan-abhishek 603125b
trigger ci
shreemaan-abhishek 11363b4
chore: trigger CI after base retarget to master
shreemaan-abhishek File filter
Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
There are no files selected for viewing
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,159 @@ | ||
| package listutil | ||
|
|
||
| import ( | ||
| "errors" | ||
| "net/http" | ||
| "testing" | ||
|
|
||
| "github.com/api7/a7/pkg/api" | ||
| "github.com/api7/a7/pkg/httpmock" | ||
| ) | ||
|
|
||
| func newTestClient(reg *httpmock.Registry) *api.Client { | ||
| return api.NewClient(reg.GetClient(), "http://api.local") | ||
| } | ||
|
|
||
| // TestFetchPaginated_StrictPropagates404 confirms that with allowOptional=false | ||
| // a 404 from the upstream surfaces as an error instead of being silently | ||
| // converted to (nil, nil). | ||
| 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) | ||
|
Comment on lines
+19
to
+24
|
||
| if err == nil { | ||
| t.Fatalf("expected error, got items=%v", items) | ||
| } | ||
| if items != nil { | ||
| t.Errorf("expected nil items on error, got %v", items) | ||
| } | ||
| var apiErr *api.APIError | ||
| if !errors.As(err, &apiErr) || apiErr.StatusCode != http.StatusNotFound { | ||
| t.Errorf("expected *api.APIError with status 404, got %T: %v", err, err) | ||
| } | ||
| reg.Verify(t) | ||
| } | ||
|
|
||
| // TestFetchPaginated_StrictPropagates400 confirms 400 is also propagated when | ||
| // allowOptional=false. A transient 400 on services/consumers must not be | ||
| // swallowed because config sync would then plan destructive operations | ||
| // against the unintentionally-empty result. | ||
| func TestFetchPaginated_StrictPropagates400(t *testing.T) { | ||
| reg := &httpmock.Registry{} | ||
| reg.Register(http.MethodGet, "/apisix/admin/consumers", httpmock.StringResponse(http.StatusBadRequest, `{"error_msg":"bad request"}`)) | ||
|
|
||
| client := newTestClient(reg) | ||
| if _, err := FetchPaginated[api.Consumer](client, "/apisix/admin/consumers", nil, false); err == nil { | ||
| t.Fatal("expected error to propagate when allowOptional=false") | ||
| } | ||
| reg.Verify(t) | ||
| } | ||
|
|
||
| // TestFetchPaginated_OptionalSwallows404 confirms the opt-in lenient path | ||
| // still works: stream_routes / protos / secret_providers callers pass | ||
| // allowOptional=true and expect (nil, nil) when the endpoint signals the | ||
| // resource is not in use. | ||
| func TestFetchPaginated_OptionalSwallows404(t *testing.T) { | ||
| reg := &httpmock.Registry{} | ||
| reg.Register(http.MethodGet, "/apisix/admin/stream_routes", httpmock.StringResponse(http.StatusNotFound, `{"error_msg":"not found"}`)) | ||
|
|
||
| client := newTestClient(reg) | ||
| items, err := FetchPaginated[api.StreamRoute](client, "/apisix/admin/stream_routes", nil, true) | ||
| if err != nil { | ||
| t.Fatalf("expected nil error with allowOptional=true, got %v", err) | ||
| } | ||
| if items != nil { | ||
| t.Errorf("expected nil items, got %v", items) | ||
| } | ||
| reg.Verify(t) | ||
| } | ||
|
|
||
| // TestFetchPaginated_OptionalSwallows400 mirrors the 404 case: stream mode | ||
| // disabled commonly returns 400, and allowOptional=true callers expect it | ||
| // suppressed. | ||
| func TestFetchPaginated_OptionalSwallows400(t *testing.T) { | ||
| reg := &httpmock.Registry{} | ||
| reg.Register(http.MethodGet, "/apisix/admin/protos", httpmock.StringResponse(http.StatusBadRequest, `{"error_msg":"stream disabled"}`)) | ||
|
|
||
| client := newTestClient(reg) | ||
| items, err := FetchPaginated[api.Proto](client, "/apisix/admin/protos", nil, true) | ||
| if err != nil { | ||
| t.Fatalf("expected nil error with allowOptional=true, got %v", err) | ||
| } | ||
| if items != nil { | ||
| t.Errorf("expected nil items, got %v", items) | ||
| } | ||
| reg.Verify(t) | ||
| } | ||
|
|
||
| // TestFetchPaginated_OptionalStillPropagatesOther5xx confirms allowOptional | ||
| // only relaxes the 400/404 contract; a 500 must still surface. | ||
| func TestFetchPaginated_OptionalStillPropagatesOther5xx(t *testing.T) { | ||
| reg := &httpmock.Registry{} | ||
| reg.Register(http.MethodGet, "/apisix/admin/protos", httpmock.StringResponse(http.StatusInternalServerError, `{"error_msg":"boom"}`)) | ||
|
|
||
| client := newTestClient(reg) | ||
| if _, err := FetchPaginated[api.Proto](client, "/apisix/admin/protos", nil, true); err == nil { | ||
| t.Fatal("expected 500 to propagate even with allowOptional=true") | ||
| } | ||
| reg.Verify(t) | ||
| } | ||
|
|
||
| // TestFetchRoutesForServices_StrictPropagatesPerServiceError confirms that a | ||
| // 404 on one service's /routes is not silently dropped when allowOptional is | ||
| // false. This is the race scenario from issue #50: if a service is deleted | ||
| // between enumeration and the per-service /routes fetch, the user must see | ||
| // an error rather than a quietly-shortened list. | ||
| func TestFetchRoutesForServices_StrictPropagatesPerServiceError(t *testing.T) { | ||
| reg := &httpmock.Registry{} | ||
| reg.RegisterResponder(http.MethodGet, "/apisix/admin/routes", func(r *http.Request) (httpmock.Response, error) { | ||
| switch r.URL.Query().Get("service_id") { | ||
| case "svc-a": | ||
| return httpmock.JSONResponse(`{"total":1,"list":[{"id":"r-a","service_id":"svc-a"}]}`), nil | ||
| case "svc-gone": | ||
| return httpmock.StringResponse(http.StatusNotFound, `{"error_msg":"not found"}`), nil | ||
| default: | ||
| return httpmock.JSONResponse(`{"total":0,"list":[]}`), nil | ||
| } | ||
| }) | ||
|
|
||
| client := newTestClient(reg) | ||
| services := []api.Service{{ID: "svc-a"}, {ID: "svc-gone"}} | ||
| routes, err := FetchRoutesForServices(client, services, nil, false) | ||
| if err == nil { | ||
| t.Fatalf("expected error, got routes=%v", routes) | ||
| } | ||
| var apiErr *api.APIError | ||
| if !errors.As(err, &apiErr) || apiErr.StatusCode != http.StatusNotFound { | ||
| t.Errorf("expected *api.APIError with status 404, got %T: %v", err, err) | ||
| } | ||
| reg.Verify(t) | ||
| } | ||
|
|
||
| // TestFetchRoutesForServices_OptionalSkipsPerServiceError documents that the | ||
| // lenient path is still wired through for callers that explicitly opt in. | ||
| func TestFetchRoutesForServices_OptionalSkipsPerServiceError(t *testing.T) { | ||
| reg := &httpmock.Registry{} | ||
| reg.RegisterResponder(http.MethodGet, "/apisix/admin/routes", func(r *http.Request) (httpmock.Response, error) { | ||
| switch r.URL.Query().Get("service_id") { | ||
| case "svc-a": | ||
| return httpmock.JSONResponse(`{"total":1,"list":[{"id":"r-a","service_id":"svc-a"}]}`), nil | ||
| case "svc-gone": | ||
| return httpmock.StringResponse(http.StatusNotFound, `{"error_msg":"not found"}`), nil | ||
| default: | ||
| return httpmock.JSONResponse(`{"total":0,"list":[]}`), nil | ||
| } | ||
| }) | ||
|
|
||
| client := newTestClient(reg) | ||
| services := []api.Service{{ID: "svc-a"}, {ID: "svc-gone"}} | ||
| routes, err := FetchRoutesForServices(client, services, nil, true) | ||
| if err != nil { | ||
| t.Fatalf("expected no error with allowOptional=true, got %v", err) | ||
| } | ||
| if len(routes) != 1 || routes[0].ID != "r-a" { | ||
| t.Errorf("expected only svc-a's routes, got %+v", routes) | ||
| } | ||
| reg.Verify(t) | ||
| } | ||
Oops, something went wrong.
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Uh oh!
There was an error while loading. Please reload this page.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
🛠️ Refactor suggestion | 🟠 Major | ⚡ Quick win
Add call-site tests for
allowOptionalmapping inFetchRemoteConfig.This wiring is correctness-critical, but the provided tests only validate
listutilbehavior, not this mapping (falsefor required resources,truefor optional ones). Please add a focusedFetchRemoteConfigtest to lock this contract and prevent silent regressions.As per coding guidelines:
**/*.go: Write tests for every code change.🤖 Prompt for AI Agents
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Holding off on this one. The mapping is exercised indirectly by the existing
pkg/cmd/config/dumpandpkg/cmd/config/synctest suites (both callFetchRemoteConfig, both still pass), and the newpkg/listutiltests 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
configutilor introduce a test seam over the package-levellistutil.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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@shreemaan-abhishek, that's a reasonable trade-off — the indirect coverage from the dump/sync suites plus thelistutilunit 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