From 152db06b615a4f583b7a3881022ff3dc79551dda Mon Sep 17 00:00:00 2001 From: saiflayouni Date: Tue, 16 Jun 2026 14:51:50 +0100 Subject: [PATCH] Suppress error output for silenced HTTP probe calls handleResponse() only honored the ClientPrintHttpResponse toggle on the success path (via PrettyPrint); the error path (status > 399) always printed the response body via clilog.HttpError regardless of the toggle. products.upsert() (and similar probe calls in iam.go, bundles.go, token.go) set ClientPrintHttpResponse.Set(false) before doing a GET to check whether an entity already exists. When the entity doesn't exist, the resulting 404 leaked to stderr even though the overall upsert then succeeds -- this is the behavior reported in #203, where 'products import --upsert' on a new product prints a misleading 404 even though the product is created successfully. Gate the error-path print on the same condition the success path already uses, so intentionally silenced probe calls stay silent on both success and error, while errors are still printed in the default case. Fixes #203 --- internal/apiclient/httpclient.go | 8 +- internal/apiclient/httpclient_test.go | 135 ++++++++++++++++++++++++++ 2 files changed, 142 insertions(+), 1 deletion(-) create mode 100644 internal/apiclient/httpclient_test.go diff --git a/internal/apiclient/httpclient.go b/internal/apiclient/httpclient.go index 97d6b68c..bdc40f54 100644 --- a/internal/apiclient/httpclient.go +++ b/internal/apiclient/httpclient.go @@ -463,7 +463,13 @@ func handleResponse(resp *http.Response) (respBody []byte, err error) { return nil, err } else if resp.StatusCode > 399 { clilog.Debug.Printf("status code %d, error in response: %s\n", resp.StatusCode, string(respBody)) - clilog.HttpError.Println(string(respBody)) + // Honor the same print toggle the success path uses (PrettyPrint). Callers that + // temporarily disable response printing to probe an entity's existence (e.g. + // upsert's pre-check GET) expect the probe's error response to stay silent too; + // otherwise an expected 404 leaks to stderr even though the overall command succeeds. + if GetCmdPrintHttpResponseSetting() && ClientPrintHttpResponse.Get() { + clilog.HttpError.Println(string(respBody)) + } return nil, errors.New(getErrorMessage(resp.StatusCode)) } clilog.Debug.Println("Response: ", string(respBody)) diff --git a/internal/apiclient/httpclient_test.go b/internal/apiclient/httpclient_test.go new file mode 100644 index 00000000..f648a7c2 --- /dev/null +++ b/internal/apiclient/httpclient_test.go @@ -0,0 +1,135 @@ +// Copyright 2020 Google LLC +// +// Licensed under the Apache License, Version 2.0 (the "License"); +// you may not use this file except in compliance with the License. +// You may obtain a copy of the License at +// +// http://www.apache.org/licenses/LICENSE-2.0 +// +// Unless required by applicable law or agreed to in writing, software +// distributed under the License is distributed on an "AS IS" BASIS, +// WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. +// See the License for the specific language governing permissions and +// limitations under the License. + +package apiclient + +import ( + "bytes" + "internal/clilog" + "io" + "log" + "net/http" + "net/http/httptest" + "strings" + "testing" +) + +// newErrorResponse builds a minimal *http.Response carrying an error status +// code, mirroring what Apigee returns for e.g. a "not found" probe. +func newErrorResponse(statusCode int) *http.Response { + return &http.Response{ + StatusCode: statusCode, + Body: io.NopCloser(strings.NewReader(`{"error":{"code":404,"message":"not found"}}`)), + Header: http.Header{}, + } +} + +// TestHandleResponseSuppressesErrorWhenPrintDisabled reproduces +// https://github.com/apigee/apigeecli/issues/203: callers (like the +// products upsert pre-check GET) disable ClientPrintHttpResponse while +// probing whether an entity exists. The expected 404 from that probe must +// stay silent, the same way a successful probe response would. +func TestHandleResponseSuppressesErrorWhenPrintDisabled(t *testing.T) { + clilog.Debug = log.New(io.Discard, "", 0) + var buf bytes.Buffer + clilog.HttpError = log.New(&buf, "", 0) + + ClientPrintHttpResponse.Set(false) + defer ClientPrintHttpResponse.Set(true) + + _, err := handleResponse(newErrorResponse(404)) + if err == nil { + t.Fatal("expected an error for a 404 response") + } + if buf.Len() != 0 { + t.Fatalf("expected no error output to be printed while ClientPrintHttpResponse is disabled, got: %q", buf.String()) + } +} + +// TestHandleResponsePrintsErrorWhenPrintEnabled ensures genuine errors are +// still surfaced when the print toggle is left at its default (enabled). +func TestHandleResponsePrintsErrorWhenPrintEnabled(t *testing.T) { + clilog.Debug = log.New(io.Discard, "", 0) + var buf bytes.Buffer + clilog.HttpError = log.New(&buf, "", 0) + + ClientPrintHttpResponse.Set(true) + + _, err := handleResponse(newErrorResponse(404)) + if err == nil { + t.Fatal("expected an error for a 404 response") + } + if buf.Len() == 0 { + t.Fatal("expected the error response to be printed when ClientPrintHttpResponse is enabled") + } +} + +// TestUpsertExistenceProbeStaysSilent exercises the real upsert pre-check +// flow end-to-end over HTTP: a GET against a product that doesn't exist +// (404), followed by the actual create POST (201). It mirrors exactly what +// products.upsert() does, including the ClientPrintHttpResponse toggling, +// to confirm the probe's expected 404 never reaches output while the +// subsequent create response still does. +func TestUpsertExistenceProbeStaysSilent(t *testing.T) { + NewApigeeClient(ApigeeClientOptions{PrintOutput: true}) + SetApigeeToken("fake-test-token") // bypass the real auth flow + + mux := http.NewServeMux() + mux.HandleFunc("/apiproducts/TestImport", func(w http.ResponseWriter, r *http.Request) { + if r.Method != http.MethodGet { + t.Fatalf("unexpected method %s on existence-check URL", r.Method) + } + w.WriteHeader(http.StatusNotFound) + _, _ = w.Write([]byte(`{"error":{"code":404,"message":"ApiProduct with name TestImport does not exist"}}`)) + }) + mux.HandleFunc("/apiproducts", func(w http.ResponseWriter, r *http.Request) { + if r.Method != http.MethodPost { + t.Fatalf("unexpected method %s on create URL", r.Method) + } + w.Header().Set("Content-Type", "application/json") + w.WriteHeader(http.StatusCreated) + _, _ = w.Write([]byte(`{"name":"TestImport"}`)) + }) + server := httptest.NewServer(mux) + defer server.Close() + + var buf bytes.Buffer + clilog.HttpError = log.New(&buf, "", 0) + clilog.HttpResponse = log.New(&buf, "", 0) + clilog.Debug = log.New(io.Discard, "", 0) + + // --- mirrors products.upsert() for Action == UPSERT --- + ClientPrintHttpResponse.Set(false) + _, err := HttpClient(server.URL + "/apiproducts/TestImport") // existence-check GET -> 404 + productMissing := err != nil + ClientPrintHttpResponse.Set(GetCmdPrintHttpResponseSetting()) + + if !productMissing { + t.Fatal("expected the probe to report the product missing (err != nil)") + } + if buf.Len() != 0 { + t.Fatalf("probe's 404 leaked to output even though ClientPrintHttpResponse was disabled: %q", buf.String()) + } + + respBody, err := HttpClient(server.URL+"/apiproducts", `{"name":"TestImport"}`) // real create POST -> 201 + if err != nil { + t.Fatalf("unexpected error creating product: %v", err) + } + if len(respBody) == 0 { + t.Fatal("expected a response body from the create call") + } + if buf.Len() == 0 { + t.Fatal("expected the successful create response to be printed once the print toggle is restored") + } +}