Merge pull request #1130 from hyperledger/or-nil

Add strong nil checking on orchestrator and redress test coverage

.golangci.yml

@@ -30,8 +30,9 @@ linters-settings:
       limitations under the License.
 linters:
   disable-all: false
+  disable:
+    - structcheck
   enable:
-    - bodyclose
     - deadcode
     - depguard
     - dogsled
@@ -51,10 +52,8 @@ linters:
     - nakedret
     - revive
     - staticcheck
-    - structcheck
     - stylecheck
     - typecheck
     - unconvert
-    - unparam
     - unused
     - varcheck

internal/apiserver/route_get_namespace_test.go

@@ -18,6 +18,7 @@ package apiserver
 
 import (
 	"context"
+	"fmt"
 	"net/http/httptest"
 	"testing"
 
@@ -48,8 +49,8 @@ func TestGetNamespaceInvalid(t *testing.T) {
 	req.Header.Set("Content-Type", "application/json; charset=utf-8")
 	res := httptest.NewRecorder()
 
-	mgr.On("Orchestrator", "BAD").Return(nil, nil)
+	mgr.On("Orchestrator", mock.Anything, "BAD").Return(nil, fmt.Errorf("pop"))
 	r.ServeHTTP(res, req)
 
-	assert.Equal(t, 404, res.Result().StatusCode)
+	assert.Equal(t, 500, res.Result().StatusCode)
 }

internal/apiserver/server.go

@@ -1,4 +1,4 @@
-// Copyright © 2022 Kaleido, Inc.
+// Copyright © 2023 Kaleido, Inc.
 //
 // SPDX-License-Identifier: Apache-2.0
 //
@@ -188,9 +188,9 @@ func (as *apiServer) swaggerGenerator(routes []*ffapi.Route, apiBaseURL string)
 func (as *apiServer) contractSwaggerGenerator(mgr namespace.Manager, apiBaseURL string) func(req *http.Request) (*openapi3.T, error) {
 	return func(req *http.Request) (*openapi3.T, error) {
 		vars := mux.Vars(req)
-		or := mgr.Orchestrator(vars["ns"])
-		if or == nil {
-			return nil, i18n.NewError(req.Context(), coremsgs.MsgNamespaceDoesNotExist)
+		or, err := mgr.Orchestrator(req.Context(), vars["ns"])
+		if err != nil {
+			return nil, err
 		}
 		cm := or.Contracts()
 		api, err := cm.GetContractAPI(req.Context(), apiBaseURL, vars["apiName"])
@@ -213,19 +213,16 @@ func (as *apiServer) contractSwaggerGenerator(mgr namespace.Manager, apiBaseURL
 func getOrchestrator(ctx context.Context, mgr namespace.Manager, tag string, r *ffapi.APIRequest) (or orchestrator.Orchestrator, err error) {
 	switch tag {
 	case routeTagDefaultNamespace:
-		or = mgr.Orchestrator(config.GetString(coreconfig.NamespacesDefault))
+		return mgr.Orchestrator(ctx, config.GetString(coreconfig.NamespacesDefault))
 	case routeTagNonDefaultNamespace:
 		vars := mux.Vars(r.Req)
 		if ns, ok := vars["ns"]; ok {
-			or = mgr.Orchestrator(ns)
+			return mgr.Orchestrator(ctx, ns)
 		}
-	default:
+	case routeTagGlobal:
 		return nil, nil
 	}
-	if or == nil {
-		return nil, i18n.NewError(ctx, coremsgs.MsgNamespaceDoesNotExist)
-	}
-	return or, nil
+	return nil, i18n.NewError(ctx, coremsgs.MsgMissingNamespace)
 }
 
 func (as *apiServer) routeHandler(hf *ffapi.HandlerFactory, mgr namespace.Manager, apiBaseURL string, route *ffapi.Route) http.HandlerFunc {

internal/apiserver/server_test.go

@@ -35,6 +35,7 @@ import (
 	"github.com/hyperledger/firefly-common/pkg/httpserver"
 	"github.com/hyperledger/firefly-common/pkg/i18n"
 	"github.com/hyperledger/firefly/internal/coreconfig"
+	"github.com/hyperledger/firefly/internal/coremsgs"
 	"github.com/hyperledger/firefly/internal/metrics"
 	"github.com/hyperledger/firefly/mocks/apiservermocks"
 	"github.com/hyperledger/firefly/mocks/contractmocks"
@@ -53,9 +54,9 @@ func newTestServer() (*namespacemocks.Manager, *orchestratormocks.Orchestrator,
 	InitConfig()
 	mgr := &namespacemocks.Manager{}
 	o := &orchestratormocks.Orchestrator{}
-	mgr.On("Orchestrator", "default").Return(o).Maybe()
-	mgr.On("Orchestrator", "mynamespace").Return(o).Maybe()
-	mgr.On("Orchestrator", "ns1").Return(o).Maybe()
+	mgr.On("Orchestrator", mock.Anything, "default").Return(o, nil).Maybe()
+	mgr.On("Orchestrator", mock.Anything, "mynamespace").Return(o, nil).Maybe()
+	mgr.On("Orchestrator", mock.Anything, "ns1").Return(o, nil).Maybe()
 	config.Set(coreconfig.APIMaxFilterLimit, 100)
 	as := &apiServer{
 		apiTimeout:    5 * time.Second,
@@ -369,7 +370,7 @@ func TestContractAPISwaggerJSONBadNamespace(t *testing.T) {
 	s := httptest.NewServer(r)
 	defer s.Close()
 
-	mgr.On("Orchestrator", "BAD").Return(nil)
+	mgr.On("Orchestrator", mock.Anything, "BAD").Return(nil, i18n.NewError(context.Background(), coremsgs.MsgUnknownNamespace))
 
 	res, err := http.Get(fmt.Sprintf("http://%s/api/v1/namespaces/BAD/apis/my-api/api/swagger.json", s.Listener.Addr()))
 	assert.NoError(t, err)
@@ -395,7 +396,7 @@ func TestJSONBadNamespace(t *testing.T) {
 	s := httptest.NewServer(r)
 	defer s.Close()
 
-	mgr.On("Orchestrator", "BAD").Return(nil)
+	mgr.On("Orchestrator", mock.Anything, "BAD").Return(nil, i18n.NewError(context.Background(), coremsgs.MsgUnknownNamespace))
 
 	var b bytes.Buffer
 	req := httptest.NewRequest("GET", "/api/v1/namespaces/BAD/apis", &b)
@@ -413,7 +414,7 @@ func TestFormDataBadNamespace(t *testing.T) {
 	s := httptest.NewServer(r)
 	defer s.Close()
 
-	mgr.On("Orchestrator", "BAD").Return(nil)
+	mgr.On("Orchestrator", mock.Anything, "BAD").Return(nil, i18n.NewError(context.Background(), coremsgs.MsgUnknownNamespace))
 
 	var b bytes.Buffer
 	w := multipart.NewWriter(&b)
@@ -471,3 +472,8 @@ func TestFormDataDisabledRoute(t *testing.T) {
 
 	assert.Equal(t, 400, res.Result().StatusCode)
 }
+
+func TestGetOrchestratorMissingTag(t *testing.T) {
+	_, err := getOrchestrator(context.Background(), &namespacemocks.Manager{}, "", nil)
+	assert.Regexp(t, "FF10437", err)
+}

internal/apiserver/spi_routes.go

@@ -1,4 +1,4 @@
-// Copyright © 2022 Kaleido, Inc.
+// Copyright © 2023 Kaleido, Inc.
 //
 // SPDX-License-Identifier: Apache-2.0
 //
@@ -20,11 +20,14 @@ import "github.com/hyperledger/firefly-common/pkg/ffapi"
 
 // The Service Provider Interface (SPI) allows external microservices (such as the FireFly Transaction Manager)
 // to act as augmented components to the core.
-var spiRoutes = []*ffapi.Route{
+var spiRoutes = append(globalRoutes([]*ffapi.Route{
 	spiGetNamespaceByName,
 	spiGetNamespaces,
 	spiGetOpByID,
-	spiGetOps,
 	spiPatchOpByID,
 	spiPostReset,
-}
+}),
+	namespacedRoutes([]*ffapi.Route{
+		spiGetOps,
+	})...,
+)

internal/blockchain/ethereum/ethereum_test.go

@@ -296,6 +296,7 @@ func TestInitAndStartWithFFTM(t *testing.T) {
 	assert.Equal(t, `{"type":"listen","topic":"topic1"}`, startupMessage)
 	startupMessage = <-toServer
 	assert.Equal(t, `{"type":"listenreplies"}`, startupMessage)
+	fromServer <- `{"bad":"receipt"}`                 // bad receipt that cannot be handled - will be swallowed
 	fromServer <- `{"batchNumber":12345,"events":[]}` // empty batch, will be ignored, but acked
 	reply := <-toServer
 	assert.Equal(t, `{"type":"ack","topic":"topic1","batchNumber":12345}`, reply)

internal/blockchain/fabric/fabric_test.go

@@ -135,6 +135,14 @@ func TestInitMissingURL(t *testing.T) {
 	assert.Regexp(t, "FF10138.*url", err)
 }
 
+func TestGenerateErrorSignatureNoOp(t *testing.T) {
+	e, cancel := newTestFabric()
+	defer cancel()
+	resetConf(e)
+
+	assert.Empty(t, e.GenerateErrorSignature(context.Background(), &fftypes.FFIErrorDefinition{}))
+}
+
 func TestInitMissingTopic(t *testing.T) {
 	e, cancel := newTestFabric()
 	defer cancel()
@@ -205,7 +213,8 @@ func TestInitAllNewStreamsAndWSEvent(t *testing.T) {
 	assert.Equal(t, `{"type":"listen","topic":"topic1"}`, startupMessage)
 	startupMessage = <-toServer
 	assert.Equal(t, `{"type":"listenreplies"}`, startupMessage)
-	fromServer <- `[]` // empty batch, will be ignored, but acked
+	fromServer <- `{"bad":"receipt"}` // will be ignored - no ack\
+	fromServer <- `[]`                // empty batch, will be ignored, but acked
 	reply := <-toServer
 	assert.Equal(t, `{"topic":"topic1","type":"ack"}`, reply)
 

internal/contracts/manager_test.go

@@ -138,6 +138,13 @@ func TestResolveFFI(t *testing.T) {
 				},
 			},
 		},
+		Errors: []*fftypes.FFIError{
+			{
+				FFIErrorDefinition: fftypes.FFIErrorDefinition{
+					Name: "changed",
+				},
+			},
+		},
 	}
 
 	err := cm.ResolveFFI(context.Background(), ffi)
@@ -419,6 +426,19 @@ func TestValidateFFI(t *testing.T) {
 				},
 			},
 		},
+		Errors: []*fftypes.FFIError{
+			{
+				FFIErrorDefinition: fftypes.FFIErrorDefinition{
+					Name: "sum",
+					Params: []*fftypes.FFIParam{
+						{
+							Name:   "z",
+							Schema: fftypes.JSONAnyPtr(`{"type": "integer", "details": {"type": "uint256"}}`),
+						},
+					},
+				},
+			},
+		},
 	}
 
 	mdi.On("GetFFI", context.Background(), "ns1", "math", "1.0.0").Return(nil, nil)
@@ -593,6 +613,64 @@ func TestValidateFFIBadEventParam(t *testing.T) {
 	assert.Regexp(t, "FF10319", err)
 }
 
+func TestValidateFFIBadError(t *testing.T) {
+	cm := newTestContractManager()
+	mdi := cm.database.(*databasemocks.Plugin)
+
+	ffi := &fftypes.FFI{
+		Name:      "math",
+		Version:   "1.0.0",
+		Namespace: "default",
+		Errors: []*fftypes.FFIError{
+			{
+				FFIErrorDefinition: fftypes.FFIErrorDefinition{
+					Name: "",
+					Params: []*fftypes.FFIParam{
+						{
+							Name:   "z",
+							Schema: fftypes.JSONAnyPtr(`{"type": "integer", "details": {"type": "uint256"}}`),
+						},
+					},
+				},
+			},
+		},
+	}
+
+	mdi.On("GetFFI", context.Background(), "ns1", "math", "1.0.0").Return(nil, nil)
+
+	err := cm.ResolveFFI(context.Background(), ffi)
+	assert.Regexp(t, "FF10433", err)
+}
+
+func TestValidateFFIBadErrorParam(t *testing.T) {
+	cm := newTestContractManager()
+	mdi := cm.database.(*databasemocks.Plugin)
+
+	ffi := &fftypes.FFI{
+		Name:      "math",
+		Version:   "1.0.0",
+		Namespace: "default",
+		Errors: []*fftypes.FFIError{
+			{
+				FFIErrorDefinition: fftypes.FFIErrorDefinition{
+					Name: "pop",
+					Params: []*fftypes.FFIParam{
+						{
+							Name:   "z",
+							Schema: fftypes.JSONAnyPtr(`{"type": "wrongness"`),
+						},
+					},
+				},
+			},
+		},
+	}
+
+	mdi.On("GetFFI", context.Background(), "ns1", "math", "1.0.0").Return(nil, nil)
+
+	err := cm.ResolveFFI(context.Background(), ffi)
+	assert.Regexp(t, "FF10332", err)
+}
+
 func TestAddContractListenerInline(t *testing.T) {
 	cm := newTestContractManager()
 	mbi := cm.blockchain.(*blockchainmocks.Plugin)
@@ -1260,6 +1338,26 @@ func TestGetFFIByIDWithChildren(t *testing.T) {
 	assert.Equal(t, "event1", ffi.Events[0].Name)
 }
 
+func TestGetFFIByIDWithChildrenErrorsFail(t *testing.T) {
+	cm := newTestContractManager()
+	mdb := cm.database.(*databasemocks.Plugin)
+
+	cid := fftypes.NewUUID()
+	mdb.On("GetFFIByID", mock.Anything, "ns1", cid).Return(&fftypes.FFI{
+		ID: cid,
+	}, nil)
+	mdb.On("GetFFIMethods", mock.Anything, "ns1", mock.Anything).Return([]*fftypes.FFIMethod{
+		{ID: fftypes.NewUUID(), Name: "method1"},
+	}, nil, nil)
+	mdb.On("GetFFIEvents", mock.Anything, "ns1", mock.Anything).Return([]*fftypes.FFIEvent{}, nil, nil)
+	mdb.On("GetFFIErrors", mock.Anything, "ns1", mock.Anything).Return(nil, fmt.Errorf("pop"))
+
+	_, err := cm.GetFFIByIDWithChildren(context.Background(), cid)
+
+	assert.EqualError(t, err, "pop")
+	mdb.AssertExpectations(t)
+}
+
 func TestGetFFIByIDWithChildrenEventsFail(t *testing.T) {
 	cm := newTestContractManager()
 	mdb := cm.database.(*databasemocks.Plugin)
@@ -1663,6 +1761,27 @@ func TestInvokeContractMethodNotFound(t *testing.T) {
 	assert.Regexp(t, "FF10315", err)
 }
 
+func TestInvokeContractErrorsFail(t *testing.T) {
+	cm := newTestContractManager()
+	mdb := cm.database.(*databasemocks.Plugin)
+	mim := cm.identity.(*identitymanagermocks.Manager)
+
+	req := &core.ContractCallRequest{
+		Type:       core.CallTypeInvoke,
+		Interface:  fftypes.NewUUID(),
+		Location:   fftypes.JSONAnyPtr(""),
+		MethodPath: "set",
+	}
+
+	mim.On("NormalizeSigningKey", mock.Anything, "", identity.KeyNormalizationBlockchainPlugin).Return("key-resolved", nil)
+	mdb.On("GetFFIMethod", mock.Anything, "ns1", req.Interface, req.MethodPath).Return(&fftypes.FFIMethod{Name: "set"}, nil)
+	mdb.On("GetFFIErrors", mock.Anything, "ns1", req.Interface).Return(nil, fmt.Errorf("pop"))
+
+	_, err := cm.InvokeContract(context.Background(), req, false)
+
+	assert.Regexp(t, "FF10434", err)
+}
+
 func TestInvokeContractMethodBadInput(t *testing.T) {
 	cm := newTestContractManager()
 	mim := cm.identity.(*identitymanagermocks.Manager)

internal/coremsgs/en_error_messages.go

@@ -274,4 +274,6 @@ var (
 	MsgErrorNameMustBeSet                 = ffe("FF10433", "The name of the error must be set", 400)
 	MsgContractErrorsResolveError         = ffe("FF10434", "Unable to resolve contract errors: %s", 400)
 	MsgUnknownInterfaceFormat             = ffe("FF10435", "Unknown interface format: %s", 400)
+	MsgUnknownNamespace                   = ffe("FF10436", "Unknown namespace '%s'", 404)
+	MsgMissingNamespace                   = ffe("FF10437", "Missing namespace in request", 400)
 )