From d160aafcdbf935551e3ded73b9bea5d9dbf21d5a Mon Sep 17 00:00:00 2001 From: Kairo Araujo Date: Tue, 30 Jan 2024 04:54:26 +0100 Subject: [PATCH] Fix api responses (#173) * dev: disable vcs from development enviroment -buildvcs Whether to stamp binaries with version control information. By default, version control information is stamped into a binary if the main package and the main module containing it are in the repository containing the current directory (if there is a repository). Use -buildvcs=false to omit version control information. Signed-off-by: Kairo Araujo * docs: swagger methods and parameter for download Improve the swagger documentation including - Fix correct method for /api/v1/download as GET instead POST - Include the parameter which allows users to try the endpoint directly from swagger documentation Signed-off-by: Kairo Araujo * fix: Archivista Download API responses Archivista Download API returned 500 if a GitOID is available while the user calls the `/v1/download/` (`/download/`) endpoint. It is not an Internal Server Error (500) but a not-found for the GitOID endpoint. This PR fixes the HTTP status code, giving the users a more precise response. This commit also includes a better list of return codes to the Swagger documentation. It includes the UT Signed-off-by: Kairo Araujo --------- Signed-off-by: Kairo Araujo --- docs/docs.go | 60 +++++++++++++++-- docs/swagger.json | 62 ++++++++++++++++-- docs/swagger.yaml | 44 +++++++++++-- entrypoint-dev.sh | 2 +- internal/server/server.go | 22 +++++-- internal/server/server_test.go | 114 ++++++++++++++++++++++----------- 6 files changed, 238 insertions(+), 66 deletions(-) diff --git a/docs/docs.go b/docs/docs.go index f33069cd..7dbeabbf 100644 --- a/docs/docs.go +++ b/docs/docs.go @@ -21,20 +21,44 @@ const docTemplate = `{ "host": "{{.Host}}", "basePath": "{{.BasePath}}", "paths": { - "/donwload/{gitoid}": { - "post": { + "/download/{gitoid}": { + "get": { "description": "download an attestation", "produces": [ "application/json" ], "summary": "Download", "deprecated": true, + "parameters": [ + { + "type": "string", + "description": "gitoid", + "name": "gitoid", + "in": "path", + "required": true + } + ], "responses": { "200": { "description": "OK", "schema": { "$ref": "#/definitions/dsse.Envelope" } + }, + "400": { + "description": "Bad Request", + "schema": { + "type": "string" + } + }, + "404": { + "description": "Not Found" + }, + "500": { + "description": "Internal Server Error", + "schema": { + "type": "string" + } } } } @@ -45,7 +69,7 @@ const docTemplate = `{ "produces": [ "application/json" ], - "summary": "Store", + "summary": "Upload", "deprecated": true, "responses": { "200": { @@ -57,8 +81,8 @@ const docTemplate = `{ } } }, - "/v1/donwload/{gitoid}": { - "post": { + "/v1/download/{gitoid}": { + "get": { "description": "download an attestation", "produces": [ "application/json" @@ -67,12 +91,36 @@ const docTemplate = `{ "attestation" ], "summary": "Download", + "parameters": [ + { + "type": "string", + "description": "gitoid", + "name": "gitoid", + "in": "path", + "required": true + } + ], "responses": { "200": { "description": "OK", "schema": { "$ref": "#/definitions/dsse.Envelope" } + }, + "400": { + "description": "Bad Request", + "schema": { + "type": "string" + } + }, + "404": { + "description": "Not Found" + }, + "500": { + "description": "Internal Server Error", + "schema": { + "type": "string" + } } } } @@ -106,7 +154,7 @@ const docTemplate = `{ "tags": [ "attestation" ], - "summary": "Store", + "summary": "Upload", "responses": { "200": { "description": "OK", diff --git a/docs/swagger.json b/docs/swagger.json index 78a95e77..323f0c11 100644 --- a/docs/swagger.json +++ b/docs/swagger.json @@ -13,20 +13,44 @@ "version": "v1" }, "paths": { - "/donwload/{gitoid}": { - "post": { + "/download/{gitoid}": { + "get": { "description": "download an attestation", "produces": [ "application/json" ], "summary": "Download", "deprecated": true, + "parameters": [ + { + "type": "string", + "description": "gitoid", + "name": "gitoid", + "in": "path", + "required": true + } + ], "responses": { "200": { "description": "OK", "schema": { "$ref": "#/definitions/dsse.Envelope" } + }, + "400": { + "description": "Bad Request", + "schema": { + "type": "string" + } + }, + "404": { + "description": "Not Found" + }, + "500": { + "description": "Internal Server Error", + "schema": { + "type": "string" + } } } } @@ -37,7 +61,7 @@ "produces": [ "application/json" ], - "summary": "Store", + "summary": "Upload", "deprecated": true, "responses": { "200": { @@ -49,8 +73,8 @@ } } }, - "/v1/donwload/{gitoid}": { - "post": { + "/v1/download/{gitoid}": { + "get": { "description": "download an attestation", "produces": [ "application/json" @@ -59,12 +83,36 @@ "attestation" ], "summary": "Download", + "parameters": [ + { + "type": "string", + "description": "gitoid", + "name": "gitoid", + "in": "path", + "required": true + } + ], "responses": { "200": { "description": "OK", "schema": { "$ref": "#/definitions/dsse.Envelope" } + }, + "400": { + "description": "Bad Request", + "schema": { + "type": "string" + } + }, + "404": { + "description": "Not Found" + }, + "500": { + "description": "Internal Server Error", + "schema": { + "type": "string" + } } } } @@ -98,7 +146,7 @@ "tags": [ "attestation" ], - "summary": "Store", + "summary": "Upload", "responses": { "200": { "description": "OK", @@ -201,4 +249,4 @@ ] } } -} \ No newline at end of file +} diff --git a/docs/swagger.yaml b/docs/swagger.yaml index 8d100082..9a77dcbc 100644 --- a/docs/swagger.yaml +++ b/docs/swagger.yaml @@ -67,10 +67,16 @@ info: title: Archivista API version: v1 paths: - /donwload/{gitoid}: - post: + /download/{gitoid}: + get: deprecated: true description: download an attestation + parameters: + - description: gitoid + in: path + name: gitoid + required: true + type: string produces: - application/json responses: @@ -78,6 +84,16 @@ paths: description: OK schema: $ref: '#/definitions/dsse.Envelope' + "400": + description: Bad Request + schema: + type: string + "404": + description: Not Found + "500": + description: Internal Server Error + schema: + type: string summary: Download /upload: post: @@ -90,10 +106,16 @@ paths: description: OK schema: $ref: '#/definitions/api.StoreResponse' - summary: Store - /v1/donwload/{gitoid}: - post: + summary: Upload + /v1/download/{gitoid}: + get: description: download an attestation + parameters: + - description: gitoid + in: path + name: gitoid + required: true + type: string produces: - application/json responses: @@ -101,6 +123,16 @@ paths: description: OK schema: $ref: '#/definitions/dsse.Envelope' + "400": + description: Bad Request + schema: + type: string + "404": + description: Not Found + "500": + description: Internal Server Error + schema: + type: string summary: Download tags: - attestation @@ -127,7 +159,7 @@ paths: description: OK schema: $ref: '#/definitions/api.StoreResponse' - summary: Store + summary: Upload tags: - attestation swagger: "2.0" diff --git a/entrypoint-dev.sh b/entrypoint-dev.sh index 493a8c98..d000009d 100644 --- a/entrypoint-dev.sh +++ b/entrypoint-dev.sh @@ -44,4 +44,4 @@ if [[ $atlas_rc -ne 0 ]]; then exit 1 fi -CompileDaemon -log-prefix=false -build="go build -o /out/archivista ./cmd/archivista" -command="/out/archivista" +CompileDaemon -log-prefix=false -build="go build -buildvcs=false -o /out/archivista ./cmd/archivista" -command="/out/archivista" diff --git a/internal/server/server.go b/internal/server/server.go index c207eff4..4cc8347f 100644 --- a/internal/server/server.go +++ b/internal/server/server.go @@ -160,14 +160,14 @@ func (s *Server) UploadHandler(w http.ResponseWriter, r *http.Request) { // @Summary Download // @Description download an attestation // @Produce json +// @Param gitoid path string true "gitoid" // @Success 200 {object} dsse.Envelope +// @Failure 500 {object} string +// @Failure 404 {object} nil +// @Failure 400 {object} string // @Tags attestation -// @Router /v1/download/{gitoid} [post] +// @Router /v1/download/{gitoid} [get] func (s *Server) Download(ctx context.Context, gitoid string) (io.ReadCloser, error) { - if len(strings.TrimSpace(gitoid)) == 0 { - return nil, errors.New("gitoid parameter is required") - } - if s.objectStore == nil { return nil, errors.New("object store unavailable") } @@ -183,9 +183,13 @@ func (s *Server) Download(ctx context.Context, gitoid string) (io.ReadCloser, er // @Summary Download // @Description download an attestation // @Produce json +// @Param gitoid path string true "gitoid" // @Success 200 {object} dsse.Envelope +// @Failure 500 {object} string +// @Failure 404 {object} nil +// @Failure 400 {object} string // @Deprecated -// @Router /download/{gitoid} [post] +// @Router /download/{gitoid} [get] func (s *Server) DownloadHandler(w http.ResponseWriter, r *http.Request) { if r.Method != http.MethodGet { http.Error(w, fmt.Sprintf("%s is an unsupported method", r.Method), http.StatusBadRequest) @@ -197,6 +201,10 @@ func (s *Server) DownloadHandler(w http.ResponseWriter, r *http.Request) { http.Error(w, "gitoid parameter is required", http.StatusBadRequest) return } + if len(strings.TrimSpace(vars["gitoid"])) == 0 { + http.Error(w, "gitoid parameter is required", http.StatusBadRequest) + return + } attestationReader, err := s.Download(r.Context(), vars["gitoid"]) if err != nil { @@ -207,7 +215,7 @@ func (s *Server) DownloadHandler(w http.ResponseWriter, r *http.Request) { defer attestationReader.Close() if _, err := io.Copy(w, attestationReader); err != nil { logrus.Errorf("failed to copy attestation to response: %+v", err) - w.WriteHeader(http.StatusInternalServerError) + w.WriteHeader(http.StatusNotFound) return } diff --git a/internal/server/server_test.go b/internal/server/server_test.go index d573b197..92a40856 100644 --- a/internal/server/server_test.go +++ b/internal/server/server_test.go @@ -31,21 +31,68 @@ import ( "github.com/stretchr/testify/suite" ) +// Mock StorerMock type StorerMock struct { mock.Mock Storer } +func (m *StorerMock) Store(context.Context, string, []byte) error { + args := m.Called() + return args.Error(0) +} + type StorerGetterMock struct { mock.Mock StorerGetter } +// Mock StorerGetter +func (m *StorerGetterMock) Store(context.Context, string, []byte) error { + args := m.Called() + return args.Error(0) +} + +func (m *StorerGetterMock) Get(context.Context, string) (io.ReadCloser, error) { + args := m.Called() + stringReader := strings.NewReader("testData") + stringReadCloser := io.NopCloser(stringReader) + return stringReadCloser, args.Error(0) +} + +// Mock ResponseRecorderMock +type ResponseRecorderMock struct { + mock.Mock + Code int + HeaderMap http.Header + Body *bytes.Buffer + Flushed bool +} + +func (m *ResponseRecorderMock) Write([]byte) (int, error) { + args := m.Called() + return args.Int(0), args.Error(1) +} + +func (rw *ResponseRecorderMock) Header() http.Header { + return http.Header{} +} + +func (rw *ResponseRecorderMock) WriteHeader(code int) { + rw.Code = code +} + +func (rw *ResponseRecorderMock) WriteString(str string) (int, error) { + return 0, nil +} + +// Define test Suite type UTServerSuite struct { suite.Suite - mockedStorer *StorerMock - mockedStorerGetter *StorerGetterMock - testServer Server + mockedStorer *StorerMock + mockedStorerGetter *StorerGetterMock + mockedResposeRecorder *ResponseRecorderMock + testServer Server } func TestUTServerSuite(t *testing.T) { @@ -55,6 +102,7 @@ func TestUTServerSuite(t *testing.T) { func (ut *UTServerSuite) SetupTest() { ut.mockedStorer = new(StorerMock) ut.mockedStorerGetter = new(StorerGetterMock) + ut.mockedResposeRecorder = new(ResponseRecorderMock) ut.testServer = Server{ut.mockedStorer, ut.mockedStorerGetter, nil} } @@ -153,26 +201,6 @@ func (ut *UTServerSuite) Test_New_GraphqlWebClientEnable_False() { ut.Contains(allPaths, "/swagger/") } -// Mock StorerGetter.Store() -func (m *StorerGetterMock) Store(context.Context, string, []byte) error { - args := m.Called() - return args.Error(0) -} - -// Mock StorerGetter.Get() -func (m *StorerGetterMock) Get(context.Context, string) (io.ReadCloser, error) { - args := m.Called() - stringReader := strings.NewReader("testData") - stringReadCloser := io.NopCloser(stringReader) - return stringReadCloser, args.Error(0) -} - -// Mock StorerMock.Store() -func (m *StorerMock) Store(context.Context, string, []byte) error { - args := m.Called() - return args.Error(0) -} - func (ut *UTServerSuite) Test_Upload() { ctx := context.TODO() r := strings.NewReader("fakeTestData") @@ -272,22 +300,6 @@ func (ut *UTServerSuite) Test_Download() { ut.Equal("testData", string(data)) } -func (ut *UTServerSuite) Test_Download_EmptyGitoid() { - ctx := context.TODO() - ut.mockedStorerGetter.On("Get").Return(nil) // mock Get() to return nil - - _, err := ut.testServer.Download(ctx, "") - ut.ErrorContains(err, "gitoid parameter is required") -} - -func (ut *UTServerSuite) Test_Download_EmptyGitoidTrimmed() { - ctx := context.TODO() - ut.mockedStorerGetter.On("Get").Return(nil) // mock Get() to return nil - - _, err := ut.testServer.Download(ctx, " ") - ut.ErrorContains(err, "gitoid parameter is required") -} - func (ut *UTServerSuite) Test_Download_NoObjectStorage() { ctx := context.TODO() ut.testServer.objectStore = nil @@ -339,6 +351,18 @@ func (ut *UTServerSuite) Test_DownloadHandler_MissingGitOID() { ut.Contains(w.Body.String(), "gitoid parameter is required") } +func (ut *UTServerSuite) Test_DownloadHandler_GitOIDEmpty() { + w := httptest.NewRecorder() + request := httptest.NewRequest(http.MethodGet, "/v1/download", nil) + request = mux.SetURLVars(request, map[string]string{"gitoid": ""}) + + ut.mockedStorerGetter.On("Get").Return(nil) // mock Get() to return nil + + ut.testServer.DownloadHandler(w, request) + ut.Equal(http.StatusBadRequest, w.Code) + ut.Contains(w.Body.String(), "gitoid parameter is required") +} + func (ut *UTServerSuite) Test_DownloadHandler_ObjectStorageFailed() { w := httptest.NewRecorder() request := httptest.NewRequest(http.MethodGet, "/v1/download", nil) @@ -350,3 +374,15 @@ func (ut *UTServerSuite) Test_DownloadHandler_ObjectStorageFailed() { ut.Equal(http.StatusInternalServerError, w.Code) ut.Contains(w.Body.String(), "BAD S3") } + +func (ut *UTServerSuite) Test_DownloadHandler_NotFound() { + request := httptest.NewRequest(http.MethodGet, "/v1/download", nil) + request = mux.SetURLVars(request, map[string]string{"gitoid": "fakeGitoid"}) + + ut.mockedStorerGetter.On("Get").Return(nil) // mock Get() to return nil + ut.mockedResposeRecorder.On("Write").Return(404, errors.New("Not Found")) + + ut.testServer.DownloadHandler(ut.mockedResposeRecorder, request) + ut.Equal(http.StatusNotFound, ut.mockedResposeRecorder.Code) + ut.Nil(ut.mockedResposeRecorder.Body) +}