fix: server crash when accepting improper resource create request (#374)

* fix: server crash when accepting improper resource create request

* fix: add recovery interceptor to handle unexpected possible panic

api/handler/v1beta1/adapter.go

@@ -1,6 +1,7 @@
 package v1beta1
 
 import (
+	"errors"
 	"fmt"
 	"strings"
 	"time"
@@ -442,14 +443,24 @@ func ToResourceProto(spec models.ResourceSpec) (*pb.ResourceSpecification, error
 }
 
 func FromResourceProto(spec *pb.ResourceSpecification, storeName string, datastoreRepo models.DatastoreRepo) (models.ResourceSpec, error) {
+	if spec == nil {
+		return models.ResourceSpec{}, errors.New("spec is nil")
+	}
+	if storeName == "" {
+		return models.ResourceSpec{}, errors.New("store name is empty")
+	}
+	if datastoreRepo == nil {
+		return models.ResourceSpec{}, errors.New("datastore repo is nil")
+	}
+
 	storer, err := datastoreRepo.GetByName(storeName)
 	if err != nil {
 		return models.ResourceSpec{}, err
 	}
 
 	typeController, ok := storer.Types()[models.ResourceType(spec.GetType())]
 	if !ok {
-		return models.ResourceSpec{}, fmt.Errorf("unsupported type %s for datastore %s", spec.Type, storeName)
+		return models.ResourceSpec{}, fmt.Errorf("unsupported type [%s] for datastore [%s]", spec.Type, storeName)
 	}
 	buf, err := proto.Marshal(spec)
 	if err != nil {

api/handler/v1beta1/adapter_test.go

@@ -1,11 +1,13 @@
 package v1beta1_test
 
 import (
+	"errors"
 	"reflect"
 	"testing"
 	"time"
 
 	"github.com/stretchr/testify/assert"
+	tMock "github.com/stretchr/testify/mock"
 	"google.golang.org/protobuf/types/known/timestamppb"
 
 	v1 "github.com/odpf/optimus/api/handler/v1beta1"
@@ -141,6 +143,93 @@ func TestAdapter(t *testing.T) {
 	})
 }
 
+func TestAdapter_FromResourceProto(t *testing.T) {
+	t.Run("should return empty result and error if spec is nil", func(t *testing.T) {
+		var spec *pb.ResourceSpecification
+		storeName := "table"
+		datastoreRepo := &mock.SupportedDatastoreRepo{}
+
+		actualResource, actualError := v1.FromResourceProto(spec, storeName, datastoreRepo)
+
+		assert.Empty(t, actualResource)
+		assert.Error(t, actualError)
+	})
+
+	t.Run("should return empty result and error if store name is empty", func(t *testing.T) {
+		spec := &pb.ResourceSpecification{}
+		var storeName string
+		datastoreRepo := &mock.SupportedDatastoreRepo{}
+
+		actualResource, actualError := v1.FromResourceProto(spec, storeName, datastoreRepo)
+
+		assert.Empty(t, actualResource)
+		assert.Error(t, actualError)
+	})
+
+	t.Run("should return empty result and error if datastore repo is nil", func(t *testing.T) {
+		spec := &pb.ResourceSpecification{}
+		storeName := "table"
+		var datastoreRepo models.DatastoreRepo
+
+		actualResource, actualError := v1.FromResourceProto(spec, storeName, datastoreRepo)
+
+		assert.Empty(t, actualResource)
+		assert.Error(t, actualError)
+	})
+
+	t.Run("should return empty result and error if error encountered when getting storer", func(t *testing.T) {
+		spec := &pb.ResourceSpecification{}
+		storeName := "table"
+		datastoreRepo := &mock.SupportedDatastoreRepo{}
+		datastoreRepo.On("GetByName", tMock.Anything).Return(nil, errors.New("random error"))
+
+		actualResource, actualError := v1.FromResourceProto(spec, storeName, datastoreRepo)
+
+		assert.Empty(t, actualResource)
+		assert.Error(t, actualError)
+	})
+
+	t.Run("should return empty result and error if cannot find spec type from storer", func(t *testing.T) {
+		spec := &pb.ResourceSpecification{}
+		storeName := "table"
+		types := map[models.ResourceType]models.DatastoreTypeController{}
+		datastorer := &mock.Datastorer{}
+		datastorer.On("Types").Return(types)
+		datastoreRepo := &mock.SupportedDatastoreRepo{}
+		datastoreRepo.On("GetByName", tMock.Anything).Return(datastorer, nil)
+
+		actualResource, actualError := v1.FromResourceProto(spec, storeName, datastoreRepo)
+
+		assert.Empty(t, actualResource)
+		assert.Error(t, actualError)
+	})
+
+	t.Run("should return spec and nil if no error is encountered", func(t *testing.T) {
+		spec := &pb.ResourceSpecification{
+			Type: "table",
+		}
+		storeName := "table"
+		specAdapter := &mock.DatastoreTypeAdapter{}
+		specAdapter.On("FromProtobuf", tMock.Anything).Return(models.ResourceSpec{
+			Version: 1,
+		}, nil)
+		typeController := &mock.DatastoreTypeController{}
+		typeController.On("Adapter").Return(specAdapter)
+		types := map[models.ResourceType]models.DatastoreTypeController{
+			"table": typeController,
+		}
+		datastorer := &mock.Datastorer{}
+		datastorer.On("Types").Return(types)
+		datastoreRepo := &mock.SupportedDatastoreRepo{}
+		datastoreRepo.On("GetByName", tMock.Anything).Return(datastorer, nil)
+
+		actualResource, actualError := v1.FromResourceProto(spec, storeName, datastoreRepo)
+
+		assert.NotEmpty(t, actualResource)
+		assert.NoError(t, actualError)
+	})
+}
+
 func TestAdapter_FromInstanceProto(t *testing.T) {
 	type args struct {
 		conf *pb.InstanceSpec

cmd/server/server.go

@@ -11,6 +11,7 @@ import (
 
 	grpc_middleware "github.com/grpc-ecosystem/go-grpc-middleware"
 	grpc_logrus "github.com/grpc-ecosystem/go-grpc-middleware/logging/logrus"
+	grpc_recovery "github.com/grpc-ecosystem/go-grpc-middleware/recovery"
 	grpctags "github.com/grpc-ecosystem/go-grpc-middleware/tags"
 	grpc_prometheus "github.com/grpc-ecosystem/go-grpc-prometheus"
 	"github.com/grpc-ecosystem/grpc-gateway/v2/runtime"
@@ -21,7 +22,9 @@ import (
 	"golang.org/x/net/http2"
 	"golang.org/x/net/http2/h2c"
 	"google.golang.org/grpc"
+	"google.golang.org/grpc/codes"
 	"google.golang.org/grpc/reflection"
+	"google.golang.org/grpc/status"
 
 	pb "github.com/odpf/optimus/api/proto/odpf/optimus/core/v1beta1"
 	"github.com/odpf/optimus/config"
@@ -73,16 +76,21 @@ func setupGRPCServer(l log.Logger) (*grpc.Server, error) {
 	// Make sure that log statements internal to gRPC library are logged using the logrus logger as well.
 	grpc_logrus.ReplaceGrpcLogger(grpcLogrusEntry)
 
+	recoverPanic := func(p interface{}) (err error) {
+		return status.Error(codes.Unknown, fmt.Sprintf("panic is triggered: %v", p))
+	}
 	grpcOpts := []grpc.ServerOption{
 		grpc_middleware.WithUnaryServerChain(
 			grpctags.UnaryServerInterceptor(grpctags.WithFieldExtractor(grpctags.CodeGenRequestFieldExtractor)),
 			grpc_logrus.UnaryServerInterceptor(grpcLogrusEntry, opts...),
 			otelgrpc.UnaryServerInterceptor(),
 			grpc_prometheus.UnaryServerInterceptor,
+			grpc_recovery.UnaryServerInterceptor(grpc_recovery.WithRecoveryHandler(recoverPanic)),
 		),
 		grpc_middleware.WithStreamServerChain(
 			otelgrpc.StreamServerInterceptor(),
 			grpc_prometheus.StreamServerInterceptor,
+			grpc_recovery.StreamServerInterceptor(grpc_recovery.WithRecoveryHandler(recoverPanic)),
 		),
 		grpc.MaxRecvMsgSize(GRPCMaxRecvMsgSize),
 		grpc.MaxSendMsgSize(GRPCMaxSendMsgSize),

mock/datastore.go

@@ -127,8 +127,23 @@ type SupportedDatastoreRepo struct {
 }
 
 func (repo *SupportedDatastoreRepo) GetByName(name string) (models.Datastorer, error) {
-	args := repo.Called(name)
-	return args.Get(0).(models.Datastorer), args.Error(1)
+	ret := repo.Called(name)
+
+	var r0 models.Datastorer
+	if rf, ok := ret.Get(0).(func(string) models.Datastorer); ok {
+		r0 = rf(name)
+	} else if ret.Get(0) != nil {
+		r0 = ret.Get(0).(models.Datastorer)
+	}
+
+	var r1 error
+	if rf, ok := ret.Get(1).(func(string) error); ok {
+		r1 = rf(name)
+	} else if ret.Get(1) != nil {
+		r1 = ret.Get(1).(error)
+	}
+
+	return r0, r1
 }
 
 func (repo *SupportedDatastoreRepo) GetAll() []models.Datastorer {