From 9621df17324aa5ff8a807ad3c8a3aa266ac5caae Mon Sep 17 00:00:00 2001 From: Rein Krul Date: Mon, 3 May 2021 15:51:23 +0200 Subject: [PATCH 1/3] Network: return JSON problems instead of text/plain errors --- docs/_static/network/v1.yaml | 10 +++++++++- network/api/v1/api.go | 21 +++++++++++++-------- network/api/v1/api_test.go | 25 +++++++++++++------------ 3 files changed, 35 insertions(+), 21 deletions(-) diff --git a/docs/_static/network/v1.yaml b/docs/_static/network/v1.yaml index 45123e9b1c..4106f9ce67 100644 --- a/docs/_static/network/v1.yaml +++ b/docs/_static/network/v1.yaml @@ -25,6 +25,8 @@ paths: type: array items: type: string + default: + $ref: '../common/error_response.yaml' /internal/network/v1/transaction/{ref}: parameters: - name: ref @@ -48,6 +50,8 @@ paths: type: string "404": description: "Transaction wasn't found in the transaction log" + default: + $ref: '../common/error_response.yaml' /internal/network/v1/transaction/{ref}/payload: parameters: - name: ref @@ -70,6 +74,8 @@ paths: example: "404": description: "Transaction (or payload) wasn't found" + default: + $ref: '../common/error_response.yaml' /internal/network/v1/diagnostics/graph: get: summary: "Visualizes the DAG as a graph" @@ -85,4 +91,6 @@ paths: content: text/vnd.graphviz: schema: - type: string \ No newline at end of file + type: string + default: + $ref: '../common/error_response.yaml' \ No newline at end of file diff --git a/network/api/v1/api.go b/network/api/v1/api.go index a443345141..897e574024 100644 --- a/network/api/v1/api.go +++ b/network/api/v1/api.go @@ -29,6 +29,11 @@ import ( "github.com/nuts-foundation/nuts-node/network/log" ) +const problemTitleListTransactions = "ListTransactions failed" +const problemTitleGetTransaction = "GetTransaction failed" +const problemTitleGetTransactionPayload = "GetTransactionPayload failed" +const problemTitleRenderGraph = "RenderGraph failed" + // Wrapper implements the ServerInterface for the network API. type Wrapper struct { Service network.Transactions @@ -43,7 +48,7 @@ func (a Wrapper) ListTransactions(ctx echo.Context) error { transactions, err := a.Service.ListTransactions() if err != nil { log.Logger().Errorf("Error while listing transactions: %v", err) - return ctx.String(http.StatusInternalServerError, err.Error()) + return core.NewProblem(problemTitleListTransactions, http.StatusInternalServerError, err.Error()) } results := make([]string, len(transactions)) for i, transaction := range transactions { @@ -56,15 +61,15 @@ func (a Wrapper) ListTransactions(ctx echo.Context) error { func (a Wrapper) GetTransaction(ctx echo.Context, hashAsString string) error { hash, err := hash2.ParseHex(hashAsString) if err != nil { - return ctx.String(http.StatusBadRequest, err.Error()) + return core.NewProblem(problemTitleGetTransaction, http.StatusBadRequest, err.Error()) } transaction, err := a.Service.GetTransaction(hash) if err != nil { log.Logger().Errorf("Error while retrieving transaction (hash=%s): %v", hash, err) - return ctx.String(http.StatusInternalServerError, err.Error()) + return core.NewProblem(problemTitleGetTransaction, http.StatusInternalServerError, err.Error()) } if transaction == nil { - return ctx.String(http.StatusNotFound, "transaction not found") + return core.NewProblem(problemTitleGetTransaction, http.StatusNotFound, "transaction not found") } ctx.Response().Header().Set(echo.HeaderContentType, "application/jose") ctx.Response().WriteHeader(http.StatusOK) @@ -76,14 +81,14 @@ func (a Wrapper) GetTransaction(ctx echo.Context, hashAsString string) error { func (a Wrapper) GetTransactionPayload(ctx echo.Context, hashAsString string) error { hash, err := hash2.ParseHex(hashAsString) if err != nil { - return ctx.String(http.StatusBadRequest, err.Error()) + return core.NewProblem(problemTitleGetTransactionPayload, http.StatusBadRequest, err.Error()) } data, err := a.Service.GetTransactionPayload(hash) if err != nil { - return ctx.String(http.StatusInternalServerError, err.Error()) + return core.NewProblem(problemTitleGetTransactionPayload, http.StatusInternalServerError, err.Error()) } if data == nil { - return ctx.String(http.StatusNotFound, "transaction or contents not found") + return core.NewProblem(problemTitleGetTransactionPayload, http.StatusNotFound, "transaction or contents not found") } ctx.Response().Header().Set(echo.HeaderContentType, "application/octet-stream") ctx.Response().WriteHeader(http.StatusOK) @@ -97,7 +102,7 @@ func (a Wrapper) RenderGraph(ctx echo.Context) error { err := a.Service.Walk(visitor.Accept) if err != nil { log.Logger().Errorf("Error while rendering graph: %v", err) - return ctx.String(http.StatusInternalServerError, err.Error()) + return core.NewProblem(problemTitleRenderGraph, http.StatusInternalServerError, err.Error()) } ctx.Response().Header().Set(echo.HeaderContentType, "text/vnd.graphviz") return ctx.String(http.StatusOK, visitor.Render()) diff --git a/network/api/v1/api_test.go b/network/api/v1/api_test.go index 5dc0e5b7e3..f9e0049982 100644 --- a/network/api/v1/api_test.go +++ b/network/api/v1/api_test.go @@ -18,6 +18,7 @@ package v1 import ( + test2 "github.com/nuts-foundation/nuts-node/test" "net/http" "net/http/httptest" "strings" @@ -69,9 +70,9 @@ func TestApiWrapper_GetTransaction(t *testing.T) { c.SetParamValues("1234") err := wrapper.GetTransaction(c) - assert.NoError(t, err) - assert.Equal(t, http.StatusBadRequest, rec.Code) - assert.Equal(t, "incorrect hash length (2)", rec.Body.String()) + test2.AssertErrProblemTitle(t, problemTitleGetTransaction, err) + test2.AssertErrProblemStatusCode(t, http.StatusBadRequest, err) + test2.AssertErrProblemDetail(t, "incorrect hash length (2)", err) }) t.Run("not found", func(t *testing.T) { var networkClient = network.NewMockTransactions(mockCtrl) @@ -86,9 +87,9 @@ func TestApiWrapper_GetTransaction(t *testing.T) { c.SetParamValues(transaction.Ref().String()) err := wrapper.GetTransaction(c) - assert.NoError(t, err) - assert.Equal(t, http.StatusNotFound, rec.Code) - assert.Equal(t, "transaction not found", rec.Body.String()) + test2.AssertErrProblemTitle(t, problemTitleGetTransaction, err) + test2.AssertErrProblemStatusCode(t, http.StatusNotFound, err) + test2.AssertErrProblemDetail(t, "transaction not found", err) }) } @@ -150,9 +151,9 @@ func TestApiWrapper_GetTransactionPayload(t *testing.T) { c.SetParamValues(transaction.Ref().String()) err := wrapper.GetTransactionPayload(c) - assert.NoError(t, err) - assert.Equal(t, http.StatusNotFound, rec.Code) - assert.Equal(t, "transaction or contents not found", rec.Body.String()) + test2.AssertErrProblemTitle(t, problemTitleGetTransactionPayload, err) + test2.AssertErrProblemStatusCode(t, http.StatusNotFound, err) + test2.AssertErrProblemDetail(t, "transaction or contents not found", err) }) t.Run("invalid hash", func(t *testing.T) { var networkClient = network.NewMockTransactions(mockCtrl) @@ -166,9 +167,9 @@ func TestApiWrapper_GetTransactionPayload(t *testing.T) { c.SetParamValues("1234") err := wrapper.GetTransactionPayload(c) - assert.NoError(t, err) - assert.Equal(t, http.StatusBadRequest, rec.Code) - assert.Equal(t, "incorrect hash length (2)", rec.Body.String()) + test2.AssertErrProblemTitle(t, problemTitleGetTransactionPayload, err) + test2.AssertErrProblemStatusCode(t, http.StatusBadRequest, err) + test2.AssertErrProblemDetail(t, "incorrect hash length (2)", err) }) } From c46d74878f3193c723b3ba66f8ca322b1c5b7cb7 Mon Sep 17 00:00:00 2001 From: Rein Krul Date: Tue, 4 May 2021 09:48:20 +0200 Subject: [PATCH 2/3] More tests --- network/api/v1/api_test.go | 95 +++++++++++++++++++++++++++++++------- 1 file changed, 79 insertions(+), 16 deletions(-) diff --git a/network/api/v1/api_test.go b/network/api/v1/api_test.go index f9e0049982..e9a6339eb6 100644 --- a/network/api/v1/api_test.go +++ b/network/api/v1/api_test.go @@ -18,6 +18,7 @@ package v1 import ( + "errors" test2 "github.com/nuts-foundation/nuts-node/test" "net/http" "net/http/httptest" @@ -58,6 +59,23 @@ func TestApiWrapper_GetTransaction(t *testing.T) { assert.Equal(t, "application/jose", rec.Header().Get("Content-Type")) assert.Equal(t, string(transaction.Data()), rec.Body.String()) }) + t.Run("error", func(t *testing.T) { + var networkClient = network.NewMockTransactions(mockCtrl) + e, wrapper := initMockEcho(networkClient) + networkClient.EXPECT().GetTransaction(gomock.Any()).Return(nil, errors.New("failed")) + + req := httptest.NewRequest(echo.GET, "/", nil) + rec := httptest.NewRecorder() + c := e.NewContext(req, rec) + c.SetPath(path) + c.SetParamNames("ref") + c.SetParamValues(hash.SHA256Sum([]byte{1, 2, 3}).String()) + + err := wrapper.GetTransaction(c) + test2.AssertErrProblemTitle(t, problemTitleGetTransaction, err) + test2.AssertErrProblemStatusCode(t, http.StatusInternalServerError, err) + test2.AssertErrProblemDetail(t, "failed", err) + }) t.Run("invalid hash", func(t *testing.T) { var networkClient = network.NewMockTransactions(mockCtrl) e, wrapper := initMockEcho(networkClient) @@ -113,6 +131,21 @@ func TestApiWrapper_RenderGraph(t *testing.T) { assert.Equal(t, "text/vnd.graphviz", rec.Header().Get("Content-Type")) assert.NotEmpty(t, rec.Body.String()) }) + t.Run("error", func(t *testing.T) { + var networkClient = network.NewMockTransactions(mockCtrl) + e, wrapper := initMockEcho(networkClient) + networkClient.EXPECT().Walk(gomock.Any()).Return(errors.New("failed")) + + req := httptest.NewRequest(echo.GET, "/", nil) + rec := httptest.NewRecorder() + c := e.NewContext(req, rec) + c.SetPath("/graph") + + err := wrapper.RenderGraph(c) + test2.AssertErrProblemTitle(t, problemTitleRenderGraph, err) + test2.AssertErrProblemStatusCode(t, http.StatusInternalServerError, err) + test2.AssertErrProblemDetail(t, "failed", err) + }) } func TestApiWrapper_GetTransactionPayload(t *testing.T) { @@ -138,6 +171,23 @@ func TestApiWrapper_GetTransactionPayload(t *testing.T) { assert.Equal(t, http.StatusOK, rec.Code) assert.Equal(t, string(payload), rec.Body.String()) }) + t.Run("error", func(t *testing.T) { + var networkClient = network.NewMockTransactions(mockCtrl) + e, wrapper := initMockEcho(networkClient) + networkClient.EXPECT().GetTransactionPayload(gomock.Any()).Return(nil, errors.New("failed")) + + req := httptest.NewRequest(echo.GET, "/", nil) + rec := httptest.NewRecorder() + c := e.NewContext(req, rec) + c.SetPath(path) + c.SetParamNames("ref") + c.SetParamValues(transaction.Ref().String()) + + err := wrapper.GetTransactionPayload(c) + test2.AssertErrProblemTitle(t, problemTitleGetTransactionPayload, err) + test2.AssertErrProblemStatusCode(t, http.StatusInternalServerError, err) + test2.AssertErrProblemDetail(t, "failed", err) + }) t.Run("not found", func(t *testing.T) { var networkClient = network.NewMockTransactions(mockCtrl) e, wrapper := initMockEcho(networkClient) @@ -178,22 +228,35 @@ func TestApiWrapper_ListTransactions(t *testing.T) { defer mockCtrl.Finish() transaction := dag.CreateTestTransactionWithJWK(1) - t.Run("list transactions", func(t *testing.T) { - t.Run("200", func(t *testing.T) { - var networkClient = network.NewMockTransactions(mockCtrl) - e, wrapper := initMockEcho(networkClient) - networkClient.EXPECT().ListTransactions().Return([]dag.Transaction{transaction}, nil) - - req := httptest.NewRequest(echo.GET, "/", nil) - rec := httptest.NewRecorder() - c := e.NewContext(req, rec) - c.SetPath("/transaction") - - err := wrapper.ListTransactions(c) - assert.NoError(t, err) - assert.Equal(t, http.StatusOK, rec.Code) - assert.Equal(t, `["`+string(transaction.Data())+`"]`, strings.TrimSpace(rec.Body.String())) - }) + t.Run("200", func(t *testing.T) { + var networkClient = network.NewMockTransactions(mockCtrl) + e, wrapper := initMockEcho(networkClient) + networkClient.EXPECT().ListTransactions().Return([]dag.Transaction{transaction}, nil) + + req := httptest.NewRequest(echo.GET, "/", nil) + rec := httptest.NewRecorder() + c := e.NewContext(req, rec) + c.SetPath("/transaction") + + err := wrapper.ListTransactions(c) + assert.NoError(t, err) + assert.Equal(t, http.StatusOK, rec.Code) + assert.Equal(t, `["`+string(transaction.Data())+`"]`, strings.TrimSpace(rec.Body.String())) + }) + t.Run("error", func(t *testing.T) { + var networkClient = network.NewMockTransactions(mockCtrl) + e, wrapper := initMockEcho(networkClient) + networkClient.EXPECT().ListTransactions().Return(nil, errors.New("failed")) + + req := httptest.NewRequest(echo.GET, "/", nil) + rec := httptest.NewRecorder() + c := e.NewContext(req, rec) + c.SetPath("/transaction") + + err := wrapper.ListTransactions(c) + test2.AssertErrProblemTitle(t, problemTitleListTransactions, err) + test2.AssertErrProblemStatusCode(t, http.StatusInternalServerError, err) + test2.AssertErrProblemDetail(t, "failed", err) }) } From 9fe563f501a7d6a4a1be2d701b062866c43197d7 Mon Sep 17 00:00:00 2001 From: Rein Krul Date: Thu, 6 May 2021 08:05:42 +0200 Subject: [PATCH 3/3] PR feedback --- docs/_static/network/v1.yaml | 24 ++++++++++++++++++++---- network/api/v1/api.go | 1 + 2 files changed, 21 insertions(+), 4 deletions(-) diff --git a/docs/_static/network/v1.yaml b/docs/_static/network/v1.yaml index 4106f9ce67..f01e27ba8e 100644 --- a/docs/_static/network/v1.yaml +++ b/docs/_static/network/v1.yaml @@ -13,6 +13,9 @@ paths: Lists all transactions on the DAG. Since this call returns all transactions on the DAG, care should be taken when there are many of them. TODO: By then we'd need a more elaborate querying interface (ranging over timestamps/hashes, pagination, filtering, etc). + + error returns: + * 500 - internal server error operationId: "listTransactions" tags: - transactions @@ -38,6 +41,13 @@ paths: type: string get: summary: "Retrieves a transaction" + description: | + Retrieves a transaction. + + error returns: + * 400 - invalid transaction reference + * 404 - transaction not found + * 500 - internal server error operationId: "getTransaction" tags: - transactions @@ -48,8 +58,6 @@ paths: application/jose: schema: type: string - "404": - description: "Transaction wasn't found in the transaction log" default: $ref: '../common/error_response.yaml' /internal/network/v1/transaction/{ref}/payload: @@ -64,6 +72,13 @@ paths: get: summary: "Gets the transaction payload" operationId: "getTransactionPayload" + description: | + Gets the transaction payload. + + error returns: + * 400 - invalid transaction reference + * 404 - transaction or payload not found + * 500 - internal server error tags: - transactions responses: @@ -72,8 +87,6 @@ paths: content: application/octet-stream: example: - "404": - description: "Transaction (or payload) wasn't found" default: $ref: '../common/error_response.yaml' /internal/network/v1/diagnostics/graph: @@ -82,6 +95,9 @@ paths: description: > Walks the DAG as subscribers of the DAG do, rendering it as graph. By default it renders in Graphviz format, which can be rendered to an image using `dot`. + + error returns: + * 500 - internal server error operationId: "renderGraph" tags: - transactions diff --git a/network/api/v1/api.go b/network/api/v1/api.go index 897e574024..b920a0e37a 100644 --- a/network/api/v1/api.go +++ b/network/api/v1/api.go @@ -85,6 +85,7 @@ func (a Wrapper) GetTransactionPayload(ctx echo.Context, hashAsString string) er } data, err := a.Service.GetTransactionPayload(hash) if err != nil { + log.Logger().Errorf("Error while retrieving transaction payload (hash=%s): %v", hash, err) return core.NewProblem(problemTitleGetTransactionPayload, http.StatusInternalServerError, err.Error()) } if data == nil {