From 35f68c806b10cc0fe4a565293e32e2f5581bfeb5 Mon Sep 17 00:00:00 2001
From: Graham Goh <graham.goh@smartcontract.com>
Date: Fri, 23 Aug 2024 00:06:02 +1000
Subject: [PATCH] fix(RegisterManager): handle error correctly (#14183)

Currently, when something goes wrong in `tx.CreateManager` such as invalid sql, the error returned is ignored. We should handle the error and return accordingly.

Co-authored-by: Ivaylo Novakov <inovakov@gmail.com>
---
 .changeset/smart-pumas-collect.md   |  5 ++++
 core/services/feeds/service.go      |  5 +++-
 core/services/feeds/service_test.go | 46 +++++++++++++++++++++++++++++
 3 files changed, 55 insertions(+), 1 deletion(-)
 create mode 100644 .changeset/smart-pumas-collect.md

diff --git a/.changeset/smart-pumas-collect.md b/.changeset/smart-pumas-collect.md
new file mode 100644
index 00000000000..0748f170b9f
--- /dev/null
+++ b/.changeset/smart-pumas-collect.md
@@ -0,0 +1,5 @@
+---
+"chainlink": patch
+---
+
+#bugfix Fix incorrect error handling when registering a new feed manager
diff --git a/core/services/feeds/service.go b/core/services/feeds/service.go
index 1733d4a7582..5e8e743109a 100644
--- a/core/services/feeds/service.go
+++ b/core/services/feeds/service.go
@@ -209,7 +209,7 @@ func (s *service) RegisterManager(ctx context.Context, params RegisterManagerPar
 		var txerr error
 
 		id, txerr = tx.CreateManager(ctx, &mgr)
-		if err != nil {
+		if txerr != nil {
 			return txerr
 		}
 
@@ -219,6 +219,9 @@ func (s *service) RegisterManager(ctx context.Context, params RegisterManagerPar
 
 		return nil
 	})
+	if err != nil {
+		return 0, err
+	}
 
 	privkey, err := s.getCSAPrivateKey()
 	if err != nil {
diff --git a/core/services/feeds/service_test.go b/core/services/feeds/service_test.go
index 3ee7adc0d70..e98ae984fb7 100644
--- a/core/services/feeds/service_test.go
+++ b/core/services/feeds/service_test.go
@@ -265,6 +265,52 @@ func Test_Service_RegisterManager(t *testing.T) {
 	assert.Equal(t, actual, id)
 }
 
+func Test_Service_RegisterManager_InvalidCreateManager(t *testing.T) {
+	t.Parallel()
+
+	var (
+		id        = int64(1)
+		pubKeyHex = "0f17c3bf72de8beef6e2d17a14c0a972f5d7e0e66e70722373f12b88382d40f9"
+	)
+
+	var pubKey crypto.PublicKey
+	_, err := hex.Decode([]byte(pubKeyHex), pubKey)
+	require.NoError(t, err)
+
+	var (
+		mgr = feeds.FeedsManager{
+			Name:      "FMS",
+			URI:       "localhost:8080",
+			PublicKey: pubKey,
+		}
+		params = feeds.RegisterManagerParams{
+			Name:      "FMS",
+			URI:       "localhost:8080",
+			PublicKey: pubKey,
+		}
+	)
+
+	svc := setupTestService(t)
+
+	svc.orm.On("CountManagers", mock.Anything).Return(int64(0), nil)
+	svc.orm.On("CreateManager", mock.Anything, &mgr, mock.Anything).
+		Return(id, errors.New("orm error"))
+	// ListManagers runs in a goroutine so it might be called.
+	svc.orm.On("ListManagers", testutils.Context(t)).Return([]feeds.FeedsManager{mgr}, nil).Maybe()
+
+	transactCall := svc.orm.On("Transact", mock.Anything, mock.Anything)
+	transactCall.Run(func(args mock.Arguments) {
+		fn := args[1].(func(orm feeds.ORM) error)
+		transactCall.ReturnArguments = mock.Arguments{fn(svc.orm)}
+	})
+	_, err = svc.RegisterManager(testutils.Context(t), params)
+	// We need to stop the service because the manager will attempt to make a
+	// connection
+	svc.Close()
+	require.Error(t, err)
+	assert.Equal(t, "orm error", err.Error())
+}
+
 func Test_Service_ListManagers(t *testing.T) {
 	t.Parallel()
 	ctx := testutils.Context(t)