Network: return JSON problems instead of text/plain errors

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
@@ -25,6 +28,8 @@ paths:
                 type: array
                 items:
                   type: string
+        default:
+          $ref: '../common/error_response.yaml'
   /internal/network/v1/transaction/{ref}:
     parameters:
       - name: ref
@@ -36,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
@@ -46,8 +58,8 @@ 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:
     parameters:
       - name: ref
@@ -60,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:
@@ -68,14 +87,17 @@ 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:
     get:
       summary: "Visualizes the DAG as a graph"
       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
@@ -85,4 +107,6 @@ paths:
           content:
             text/vnd.graphviz:
               schema:
-                type: string
+                type: string
+        default:
+          $ref: '../common/error_response.yaml'

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,15 @@ 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())
+		log.Logger().Errorf("Error while retrieving transaction payload (hash=%s): %v", hash, err)
+		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 +103,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())

network/api/v1/api_test.go

@@ -18,6 +18,8 @@
 package v1
 
 import (
+	"errors"
+	test2 "github.com/nuts-foundation/nuts-node/test"
 	"net/http"
 	"net/http/httptest"
 	"strings"
@@ -57,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)
@@ -69,9 +88,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 +105,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)
 	})
 }
 
@@ -112,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) {
@@ -137,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)
@@ -150,9 +201,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 +217,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)
 	})
 }
 
@@ -177,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)
 	})
 }