From d5a56394044043c332b0aec3898de415b445f8d1 Mon Sep 17 00:00:00 2001 From: aliculPix4D Date: Thu, 3 Oct 2024 12:59:55 +0200 Subject: [PATCH] address review comments Signed-off-by: aliculPix4D --- fly/integration/set_pipeline_test.go | 9 ++ go-concourse/concourse/configs.go | 32 ++++--- go-concourse/concourse/configs_test.go | 110 +++++++++---------------- 3 files changed, 65 insertions(+), 86 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 2bea7707f5e..a412e3efafd 100644 --- a/go-concourse/concourse/configs.go +++ b/go-concourse/concourse/configs.go @@ -86,36 +86,34 @@ func (team *team) CreateOrUpdatePipelineConfig(pipelineRef atc.PipelineRef, conf switch response.StatusCode { case http.StatusOK, http.StatusCreated: - if response.Header.Get("Content-Type") == "application/json" { - configResponse := setConfigResponse{} - err = json.Unmarshal(body, &configResponse) - if err != nil { - return false, false, []ConfigWarning{}, fmt.Errorf("client: unable to JSON parse the response body: %w", err) - } - created := response.StatusCode == http.StatusCreated - return created, !created, configResponse.Warnings, nil - } else { + 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{}, 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" { - var validationErr atc.SaveConfigResponse - err = json.Unmarshal(body, &validationErr) - if err != nil { - return false, false, []ConfigWarning{}, fmt.Errorf("client: unable to JSON parse the response body: %w", err) - } - return false, false, []ConfigWarning{}, InvalidConfigError{Errors: validationErr.Errors} - } else { + 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{}, fmt.Errorf("parsing JSON: %w", err) + } + return false, false, []ConfigWarning{}, InvalidConfigError{Errors: validationErr.Errors} case http.StatusForbidden: return false, false, []ConfigWarning{}, internal.ForbiddenError{ Reason: string(body), diff --git a/go-concourse/concourse/configs_test.go b/go-concourse/concourse/configs_test.go index 22d77660a8b..bc8e7dce82b 100644 --- a/go-concourse/concourse/configs_test.go +++ b/go-concourse/concourse/configs_test.go @@ -175,7 +175,6 @@ var _ = Describe("ATC Handler Configs", func() { expectedVersion string expectedConfig []byte - contentType string returnHeader int returnBody []byte @@ -205,9 +204,7 @@ var _ = Describe("ATC Handler Configs", func() { Expect(receivedConfig).To(Equal(expectedConfig)) - if contentType != "" { - w.Header().Set("Content-Type", contentType) - } + w.Header().Set("Content-Type", "application/json") w.WriteHeader(returnHeader) w.Write(returnBody) @@ -218,7 +215,6 @@ var _ = Describe("ATC Handler Configs", func() { Context("when creating a new config", func() { BeforeEach(func() { - contentType = "application/json" returnHeader = http.StatusCreated returnBody = []byte(`{"warnings":[ {"type": "warning-1-type", "message": "fake-warning1"}, @@ -243,31 +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()) - Expect(err.Error()).To(ContainSubstring("client: unable to JSON parse the response body")) - }) - }) - - Context("when response does not contain application/json header", func() { - BeforeEach(func() { - contentType = "text/plain" - returnBody = []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 instance vars are specified", func() { BeforeEach(func() { pipelineRef = atc.PipelineRef{ @@ -302,11 +273,30 @@ 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() { BeforeEach(func() { - contentType = "application/json" returnHeader = http.StatusOK returnBody = []byte(`{"warnings":[ {"type": "warning-1-type", "message": "fake-warning1"}, @@ -331,18 +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()) - Expect(err.Error()).To(ContainSubstring("client: unable to JSON parse the response body")) - }) - }) - Context("when instance vars are specified", func() { BeforeEach(func() { pipelineRef = atc.PipelineRef{ @@ -381,7 +359,6 @@ var _ = Describe("ATC Handler Configs", func() { Context("when setting config returns bad request", func() { BeforeEach(func() { - contentType = "application/json" returnHeader = http.StatusBadRequest returnBody = []byte(`{"errors":["fake-error1","fake-error2"]}`) }) @@ -392,31 +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()) - Expect(err.Error()).To(ContainSubstring("client: unable to JSON parse the response body")) - }) - }) - - Context("when response does not contain application/json header", func() { - BeforeEach(func() { - contentType = "text/plain" - returnBody = []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 setting config returns forbidden", func() { @@ -431,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")) + }) + }) }) })