From ac16027edc3b623ab0cd2d8542a738a28f90fb2f Mon Sep 17 00:00:00 2001 From: aliculPix4D Date: Wed, 2 Oct 2024 10:27:59 +0200 Subject: [PATCH] go-concourse: configs: JSON parsing: improve error message given to the end user Signed-off-by: aliculPix4D --- fly/integration/set_pipeline_test.go | 9 ++++ go-concourse/concourse/configs.go | 19 ++++++- go-concourse/concourse/configs_test.go | 75 ++++++++++++++------------ 3 files changed, 68 insertions(+), 35 deletions(-) diff --git a/fly/integration/set_pipeline_test.go b/fly/integration/set_pipeline_test.go index 0150bf8498b..b23d576ee86 100644 --- a/fly/integration/set_pipeline_test.go +++ b/fly/integration/set_pipeline_test.go @@ -55,6 +55,7 @@ var _ = Describe("Fly CLI", func() { Expect(receivedConfig).To(Equal(config)) + w.Header().Set("Content-Type", "application/json") w.WriteHeader(http.StatusOK) w.Write([]byte(`{}`)) }, @@ -341,6 +342,7 @@ var _ = Describe("Fly CLI", func() { Expect(err).NotTo(HaveOccurred()) Expect(receivedConfig).To(Equal(config)) + w.Header().Set("Content-Type", "application/json") w.WriteHeader(http.StatusOK) w.Write([]byte(`{}`)) @@ -885,6 +887,7 @@ this is super secure ghttp.CombineHandlers( ghttp.VerifyHeaderKV(atc.ConfigVersionHeader, "42"), func(w http.ResponseWriter, r *http.Request) { + w.Header().Set("Content-Type", "application/json") config := getConfig(r) Expect(config).To(MatchYAML(payload)) }, @@ -961,6 +964,7 @@ this is super secure ghttp.CombineHandlers( ghttp.VerifyHeaderKV(atc.ConfigVersionHeader, "42"), func(w http.ResponseWriter, r *http.Request) { + w.Header().Set("Content-Type", "application/json") config := getConfig(r) Expect(config).To(MatchYAML(payload)) }, @@ -1268,6 +1272,7 @@ this is super secure atcServer.RouteToHandler("PUT", path, ghttp.CombineHandlers( ghttp.VerifyHeaderKV(atc.ConfigVersionHeader, "42"), func(w http.ResponseWriter, r *http.Request) { + w.Header().Set("Content-Type", "application/json") config := getConfig(r) Expect(config).To(MatchYAML(payload)) }, @@ -1393,8 +1398,10 @@ this is super secure ghttp.VerifyHeaderKV(atc.ConfigVersionHeader, "42"), func(w http.ResponseWriter, r *http.Request) { config := getConfig(r) + w.Header().Set("Content-Type", "application/json") Expect(config).To(MatchYAML(payload)) }, + ghttp.RespondWith(http.StatusOK, "{}"), )) @@ -1419,6 +1426,7 @@ this is super secure ghttp.VerifyHeaderKV(atc.ConfigVersionHeader, "42"), func(w http.ResponseWriter, r *http.Request) { config := getConfig(r) + w.Header().Set("Content-Type", "application/json") Expect(config).To(MatchYAML(payload)) }, ghttp.RespondWith(http.StatusCreated, "{}"), @@ -1443,6 +1451,7 @@ this is super secure ghttp.VerifyHeaderKV(atc.ConfigVersionHeader, "42"), func(w http.ResponseWriter, r *http.Request) { config := getConfig(r) + w.Header().Set("Content-Type", "application/json") Expect(config).To(MatchYAML(payload)) }, ghttp.RespondWith(http.StatusCreated, `{"warnings":[ diff --git a/go-concourse/concourse/configs.go b/go-concourse/concourse/configs.go index 581ad53e3f2..a412e3efafd 100644 --- a/go-concourse/concourse/configs.go +++ b/go-concourse/concourse/configs.go @@ -3,6 +3,7 @@ package concourse import ( "bytes" "encoding/json" + "fmt" "io" "net/http" "net/url" @@ -85,18 +86,32 @@ func (team *team) CreateOrUpdatePipelineConfig(pipelineRef atc.PipelineRef, conf switch response.StatusCode { case http.StatusOK, http.StatusCreated: + if response.Header.Get("Content-Type") != "application/json" { + return false, false, []ConfigWarning{}, internal.UnexpectedResponseError{ + StatusCode: response.StatusCode, + Status: response.Status, + Body: string(body), + } + } configResponse := setConfigResponse{} err = json.Unmarshal(body, &configResponse) if err != nil { - return false, false, []ConfigWarning{}, err + return false, false, []ConfigWarning{}, fmt.Errorf("parsing JSON: %w", err) } created := response.StatusCode == http.StatusCreated return created, !created, configResponse.Warnings, nil case http.StatusBadRequest: + if response.Header.Get("Content-Type") != "application/json" { + return false, false, []ConfigWarning{}, internal.UnexpectedResponseError{ + StatusCode: response.StatusCode, + Status: response.Status, + Body: string(body), + } + } var validationErr atc.SaveConfigResponse err = json.Unmarshal(body, &validationErr) if err != nil { - return false, false, []ConfigWarning{}, err + return false, false, []ConfigWarning{}, fmt.Errorf("parsing JSON: %w", err) } return false, false, []ConfigWarning{}, InvalidConfigError{Errors: validationErr.Errors} case http.StatusForbidden: diff --git a/go-concourse/concourse/configs_test.go b/go-concourse/concourse/configs_test.go index 2de9dc55b35..bc8e7dce82b 100644 --- a/go-concourse/concourse/configs_test.go +++ b/go-concourse/concourse/configs_test.go @@ -204,6 +204,8 @@ var _ = Describe("ATC Handler Configs", func() { Expect(receivedConfig).To(Equal(expectedConfig)) + w.Header().Set("Content-Type", "application/json") + w.WriteHeader(returnHeader) w.Write(returnBody) }, @@ -237,17 +239,6 @@ var _ = Describe("ATC Handler Configs", func() { })) }) - Context("when response contains bad JSON", func() { - BeforeEach(func() { - returnBody = []byte(`bad-json`) - }) - - It("returns an error", func() { - _, _, _, err := team.CreateOrUpdatePipelineConfig(pipelineRef, expectedVersion, expectedConfig, checkCredentials) - Expect(err).To(HaveOccurred()) - }) - }) - Context("when instance vars are specified", func() { BeforeEach(func() { pipelineRef = atc.PipelineRef{ @@ -282,6 +273,26 @@ var _ = Describe("ATC Handler Configs", func() { Expect(atcServer.ReceivedRequests()[0].URL.RawQuery).To(Equal("check_creds=")) }) }) + + Context("when response does not contain application/json header", func() { + BeforeEach(func() { + atcServer.RouteToHandler("PUT", "/api/v1/teams/some-team/pipelines/mypipeline/config", + ghttp.CombineHandlers( + ghttp.VerifyHeaderKV(atc.ConfigVersionHeader, "42"), + func(w http.ResponseWriter, r *http.Request) { + w.WriteHeader(http.StatusOK) + w.Write([]byte(`server error`)) + }, + ), + ) + }) + + It("returns an error", func() { + _, _, _, err := team.CreateOrUpdatePipelineConfig(pipelineRef, expectedVersion, expectedConfig, checkCredentials) + Expect(err).To(HaveOccurred()) + Expect(err.Error()).To(ContainSubstring("server error")) + }) + }) }) Context("when updating a config", func() { @@ -310,17 +321,6 @@ var _ = Describe("ATC Handler Configs", func() { })) }) - Context("when response contains bad JSON", func() { - BeforeEach(func() { - returnBody = []byte(`bad-json`) - }) - - It("returns an error", func() { - _, _, _, err := team.CreateOrUpdatePipelineConfig(pipelineRef, expectedVersion, expectedConfig, checkCredentials) - Expect(err).To(HaveOccurred()) - }) - }) - Context("when instance vars are specified", func() { BeforeEach(func() { pipelineRef = atc.PipelineRef{ @@ -369,17 +369,6 @@ var _ = Describe("ATC Handler Configs", func() { Expect(err.Error()).To(ContainSubstring("invalid pipeline config:\n")) Expect(err.Error()).To(ContainSubstring("fake-error1\nfake-error2")) }) - - Context("when response contains bad JSON", func() { - BeforeEach(func() { - returnBody = []byte(`bad-json`) - }) - - It("returns an error", func() { - _, _, _, err := team.CreateOrUpdatePipelineConfig(pipelineRef, expectedVersion, expectedConfig, checkCredentials) - Expect(err).To(HaveOccurred()) - }) - }) }) Context("when setting config returns forbidden", func() { @@ -394,5 +383,25 @@ var _ = Describe("ATC Handler Configs", func() { Expect(err.Error()).To(ContainSubstring("forbidden: policy check failed: you can't do that")) }) }) + + Context("when response does not contain application/json header", func() { + BeforeEach(func() { + atcServer.RouteToHandler("PUT", "/api/v1/teams/some-team/pipelines/mypipeline/config", + ghttp.CombineHandlers( + ghttp.VerifyHeaderKV(atc.ConfigVersionHeader, "42"), + func(w http.ResponseWriter, r *http.Request) { + w.WriteHeader(http.StatusBadRequest) + w.Write([]byte(`server error`)) + }, + ), + ) + }) + + It("returns an error", func() { + _, _, _, err := team.CreateOrUpdatePipelineConfig(pipelineRef, expectedVersion, expectedConfig, checkCredentials) + Expect(err).To(HaveOccurred()) + Expect(err.Error()).To(ContainSubstring("server error")) + }) + }) }) })