refact: Reworked error handling and unit testing of errors

Signed-off-by: Leonard Goodell <[email protected]>
edgexfoundry · Jul 27, 2022 · 342b219 · 342b219
1 parent 30e9bc2
commit 342b219
Show file tree

Hide file tree

Showing 2 changed files with 23 additions and 42 deletions.
diff --git a/internal/core/metadata/application/device.go b/internal/core/metadata/application/device.go
@@ -64,12 +64,7 @@ func AddDevice(d models.Device, ctx context.Context, dic *di.Container) (id stri
 	device := dtos.FromDeviceModelToDTO(d)
 	go addDeviceCallback(ctx, dic, device)
 
-	go func() {
-		if err := publishDeviceSystemEvent(common.DeviceSystemEventActionAdd, d, ctx, lc, dic); err != nil {
-			// Don't fail request if couldn't publish, just log it. Error message already has context.
-			lc.Error(err.Error())
-		}
-	}()
+	go publishDeviceSystemEvent(common.DeviceSystemEventActionAdd, d.ServiceName, d, ctx, lc, dic)
 
 	return addedDevice.Id, nil
 }
@@ -92,12 +87,7 @@ func DeleteDeviceByName(name string, ctx context.Context, dic *di.Container) err
 	}
 	go deleteDeviceCallback(ctx, dic, device)
 
-	go func() {
-		if err := publishDeviceSystemEvent(common.DeviceSystemEventActionDelete, device, ctx, lc, dic); err != nil {
-			// Don't fail request if couldn't publish, just log it. Error message already has context.
-			lc.Error(err.Error())
-		}
-	}()
+	go publishDeviceSystemEvent(common.DeviceSystemEventActionDelete, device.ServiceName, device, ctx, lc, dic)
 
 	return nil
 }
@@ -187,15 +177,11 @@ func PatchDevice(dto dtos.UpdateDevice, ctx context.Context, dic *di.Container)
 
 	if oldServiceName != "" {
 		go updateDeviceCallback(ctx, dic, oldServiceName, device)
+		go publishDeviceSystemEvent(common.DeviceSystemEventActionUpdate, oldServiceName, device, ctx, lc, dic)
 	}
-	go updateDeviceCallback(ctx, dic, device.ServiceName, device)
 
-	go func() {
-		if err := publishDeviceSystemEvent(common.DeviceSystemEventActionUpdate, device, ctx, lc, dic); err != nil {
-			// Don't fail request if couldn't publish, just log it. Error message already has context.
-			lc.Error(err.Error())
-		}
-	}()
+	go updateDeviceCallback(ctx, dic, device.ServiceName, device)
+	go publishDeviceSystemEvent(common.DeviceSystemEventActionUpdate, device.ServiceName, device, ctx, lc, dic)
 
 	return nil
 }
@@ -270,15 +256,16 @@ func DevicesByProfileName(offset int, limit int, profileName string, dic *di.Con
 	return devices, totalCount, nil
 }
 
-var noMessagingClientError = errors.NewCommonEdgeXWrapper(goErrors.New("unable to publish Device System Event: MessageBus Client not available"))
+var noMessagingClientError = goErrors.New(": MessageBus Client not available")
 
-func publishDeviceSystemEvent(action string, d models.Device, ctx context.Context, lc logger.LoggingClient, dic *di.Container) errors.EdgeX {
+func publishDeviceSystemEvent(action string, owner string, d models.Device, ctx context.Context, lc logger.LoggingClient, dic *di.Container) {
 	device := dtos.FromDeviceModelToDTO(d)
-	systemEvent := dtos.NewSystemEvent(common.DeviceSystemEventType, action, common.CoreMetaDataServiceKey, device.ServiceName, nil, device)
+	systemEvent := dtos.NewSystemEvent(common.DeviceSystemEventType, action, common.CoreMetaDataServiceKey, owner, nil, device)
 
 	messagingClient := bootstrapContainer.MessagingClientFrom(dic.Get)
 	if messagingClient == nil {
-		return &noMessagingClientError
+		lc.Errorf("unable to publish Device System Event: %v", noMessagingClientError)
+		return
 	}
 
 	config := container.ConfigurationFrom(dic.Get)
@@ -298,11 +285,9 @@ func publishDeviceSystemEvent(action string, d models.Device, ctx context.Contex
 	envelope.ContentType = common.ContentTypeJSON
 
 	if err := messagingClient.Publish(envelope, publishTopic); err != nil {
-		msg := fmt.Sprintf("unable to publish '%s' Device System Event for device '%s' to topic '%s'", action, device.Name, publishTopic)
-		return errors.NewCommonEdgeX(errors.KindCommunicationError, msg, err)
+		lc.Errorf("unable to publish '%s' Device System Event for device '%s' to topic '%s': %v", action, device.Name, publishTopic, err)
+		return
 	}
 
 	lc.Debugf("Published the '%s' Device System Event for device '%s' to topic '%s'", action, device.Name, publishTopic)
-
-	return nil
 }
diff --git a/internal/core/metadata/application/device_test.go b/internal/core/metadata/application/device_test.go
@@ -15,6 +15,7 @@ import (
 	"github.com/edgexfoundry/edgex-go/internal/core/metadata/config"
 	"github.com/edgexfoundry/edgex-go/internal/core/metadata/container"
 	bootstrapConfig "github.com/edgexfoundry/go-mod-bootstrap/v2/config"
+	mocks2 "github.com/edgexfoundry/go-mod-core-contracts/v2/clients/logger/mocks"
 	"github.com/edgexfoundry/go-mod-core-contracts/v2/errors"
 	"github.com/edgexfoundry/go-mod-messaging/v2/messaging/mocks"
 	"github.com/edgexfoundry/go-mod-messaging/v2/pkg/types"
@@ -25,15 +26,12 @@ import (
 
 	bootstrapContainer "github.com/edgexfoundry/go-mod-bootstrap/v2/bootstrap/container"
 	"github.com/edgexfoundry/go-mod-bootstrap/v2/di"
-	"github.com/edgexfoundry/go-mod-core-contracts/v2/clients/logger"
 	"github.com/edgexfoundry/go-mod-core-contracts/v2/common"
 	"github.com/edgexfoundry/go-mod-core-contracts/v2/dtos"
 	"github.com/edgexfoundry/go-mod-core-contracts/v2/models"
 )
 
 func TestPublishDeviceSystemEvent(t *testing.T) {
-	lc := logger.NewMockClient()
-
 	expectedDevice := models.Device{
 		Name:        "Camera-Device",
 		Id:          uuid.NewString(),
@@ -97,14 +95,18 @@ func TestPublishDeviceSystemEvent(t *testing.T) {
 			}
 
 			mockClient := &mocks.MessageClient{}
+			mockLogger := &mocks2.LoggingClient{}
+			mockLogger.On("Debugf", mock.Anything, mock.Anything, mock.Anything, mock.Anything).Return()
 
 			if test.PubError {
 				mockClient.On("Publish", mock.Anything, mock.Anything).Return(pubErrMsg)
+				mockLogger.On("Errorf", mock.Anything, mock.Anything, mock.Anything, mock.Anything, mock.Anything).Return()
 			} else {
 				mockClient.On("Publish", mock.Anything, mock.Anything).Return(validatePublishCallFunc)
 			}
 
 			if test.ClientMissing {
+				mockLogger.On("Errorf", mock.Anything, mock.Anything).Return()
 				dic.Update(di.ServiceConstructorMap{
 					bootstrapContainer.MessagingClientName: func(get di.Get) interface{} {
 						return nil
@@ -118,21 +120,14 @@ func TestPublishDeviceSystemEvent(t *testing.T) {
 				})
 			}
 
-			// Use CBOR to make sure publisher override with JSON properly
+			// Use CBOR to make sure publisher overrides with JSON properly
 			ctx := context.WithValue(context.Background(), common.ContentType, common.ContentTypeCBOR)
 			ctx = context.WithValue(ctx, common.CorrelationHeader, expectedCorrelationID)
 
-			err := publishDeviceSystemEvent(test.Action, expectedDevice, ctx, lc, dic)
-
-			if test.PubError {
-				require.Error(t, err)
-				assert.Contains(t, err.Error(), pubErrMsg.Error())
-				return
-			}
+			publishDeviceSystemEvent(test.Action, expectedDevice.ServiceName, expectedDevice, ctx, mockLogger, dic)
 
 			if test.ClientMissing {
-				require.Error(t, err)
-				assert.Equal(t, &noMessagingClientError, err)
+				mockLogger.AssertCalled(t, "Errorf", mock.Anything, noMessagingClientError)
 				return
 			}
 
@@ -143,9 +138,10 @@ func TestPublishDeviceSystemEvent(t *testing.T) {
 				test.Action,
 				expectedDevice.ServiceName,
 				expectedDevice.ProfileName)
+			mockClient.AssertCalled(t, "Publish", mock.Anything, expectedTopic)
 
-			if !test.ClientMissing {
-				mockClient.AssertCalled(t, "Publish", mock.Anything, expectedTopic)
+			if test.PubError {
+				mockLogger.AssertCalled(t, "Errorf", mock.Anything, mock.Anything, mock.Anything, mock.Anything, pubErrMsg)
 			}
 		})
 	}