Merge pull request #1561 from dwertent/fix-retry-operations

chore: Fix Cache Mismatch by Deep Copying Operation

internal/operations/manager.go

@@ -1,4 +1,4 @@
-// Copyright © 2023 Kaleido, Inc.
+// Copyright © 2024 Kaleido, Inc.
 //
 // SPDX-License-Identifier: Apache-2.0
 //
@@ -234,10 +234,12 @@ func (om *operationsManager) RetryOperation(ctx context.Context, opID *fftypes.U
 	var po *core.PreparedOperation
 	var idempotencyKey core.IdempotencyKey
 	err = om.database.RunAsGroup(ctx, func(ctx context.Context) error {
-		op, err = om.findLatestRetry(ctx, opID)
+		parent, err := om.findLatestRetry(ctx, opID)
 		if err != nil {
 			return err
 		}
+		// Deep copy the operation so the parent ID will not get overwritten
+		op = parent.DeepCopy()
 
 		tx, err := om.updater.txHelper.GetTransactionByIDCached(ctx, op.Transaction)
 		if err != nil {

internal/operations/manager_test.go

@@ -1,4 +1,4 @@
-// Copyright © 2022 Kaleido, Inc.
+// Copyright © 2024 Kaleido, Inc.
 //
 // SPDX-License-Identifier: Apache-2.0
 //
@@ -346,9 +346,12 @@ func TestRetryOperationSuccess(t *testing.T) {
 		assert.NoError(t, err)
 		assert.Equal(t, 1, len(info.SetOperations))
 		assert.Equal(t, "retry", info.SetOperations[0].Field)
-		val, err := info.SetOperations[0].Value.Value()
+		retryVal, err := info.SetOperations[0].Value.Value()
 		assert.NoError(t, err)
-		assert.Equal(t, op.ID.String(), val)
+		// The retry value of the parent operation should be the new operation ID
+		assert.Equal(t, op.Retry.String(), retryVal.(string))
+		// The parent ID should not change
+		assert.Equal(t, op.ID.String(), opID.String())
 		return true
 	})).Return(true, nil)
 	mdi.On("GetTransactionByID", mock.Anything, "ns1", txID).Return(&core.Transaction{

pkg/core/operation.go

@@ -1,4 +1,4 @@
-// Copyright © 2023 Kaleido, Inc.
+// Copyright © 2024 Kaleido, Inc.
 //
 // SPDX-License-Identifier: Apache-2.0
 //
@@ -74,6 +74,79 @@ func (op *Operation) IsTokenOperation() bool {
 	return op.Type == OpTypeTokenActivatePool || op.Type == OpTypeTokenApproval || op.Type == OpTypeTokenCreatePool || op.Type == OpTypeTokenTransfer
 }
 
+func (op *Operation) DeepCopy() *Operation {
+	cop := &Operation{
+		Namespace: op.Namespace,
+		Type:      op.Type,
+		Status:    op.Status,
+		Plugin:    op.Plugin,
+		Error:     op.Error,
+	}
+	if op.ID != nil {
+		idCopy := *op.ID
+		cop.ID = &idCopy
+	}
+	if op.Transaction != nil {
+		txCopy := *op.Transaction
+		cop.Transaction = &txCopy
+	}
+	if op.Created != nil {
+		createdCopy := *op.Created
+		cop.Created = &createdCopy
+	}
+	if op.Updated != nil {
+		updatedCopy := *op.Updated
+		cop.Updated = &updatedCopy
+	}
+	if op.Retry != nil {
+		retryCopy := *op.Retry
+		cop.Retry = &retryCopy
+	}
+	if op.Input != nil {
+		cop.Input = deepCopyMap(op.Input)
+	}
+	if op.Output != nil {
+		cop.Output = deepCopyMap(op.Output)
+	}
+	return cop
+}
+
+func deepCopyMap(original map[string]interface{}) map[string]interface{} {
+	if original == nil {
+		return nil
+	}
+	copy := make(map[string]interface{}, len(original))
+	for key, value := range original {
+		switch v := value.(type) {
+		case map[string]interface{}:
+			copy[key] = deepCopyMap(v)
+		case []interface{}:
+			copy[key] = deepCopySlice(v)
+		default:
+			copy[key] = v
+		}
+	}
+	return copy
+}
+
+func deepCopySlice(original []interface{}) []interface{} {
+	if original == nil {
+		return nil
+	}
+	copy := make([]interface{}, len(original))
+	for i, value := range original {
+		switch v := value.(type) {
+		case map[string]interface{}:
+			copy[i] = deepCopyMap(v)
+		case []interface{}:
+			copy[i] = deepCopySlice(v)
+		default:
+			copy[i] = v
+		}
+	}
+	return copy
+}
+
 // OpStatus is the current status of an operation
 type OpStatus string
 

pkg/core/operation_test.go

@@ -1,4 +1,4 @@
-// Copyright © 2021 Kaleido, Inc.
+// Copyright © 2024 Kaleido, Inc.
 //
 // SPDX-License-Identifier: Apache-2.0
 //
@@ -18,6 +18,7 @@ package core
 
 import (
 	"context"
+	"reflect"
 	"testing"
 
 	"github.com/hyperledger/firefly-common/pkg/fftypes"
@@ -97,6 +98,63 @@ func TestOperationTypes(t *testing.T) {
 	assert.False(t, op.IsBlockchainOperation())
 }
 
+func TestOperationDeepCopy(t *testing.T) {
+	op := &Operation{
+		ID:          fftypes.NewUUID(),
+		Namespace:   "ns1",
+		Transaction: fftypes.NewUUID(),
+		Type:        OpTypeBlockchainInvoke,
+		Status:      OpStatusInitialized,
+		Plugin:      "fake",
+		Input:       fftypes.JSONObject{"key": "value"},
+		Output:      fftypes.JSONObject{"result": "success"},
+		Error:       "error message",
+		Created:     fftypes.Now(),
+		Updated:     fftypes.Now(),
+		Retry:       fftypes.NewUUID(),
+	}
+
+	copyOp := op.DeepCopy()
+	shallowCopy := op // Shallow copy for showcasing that DeepCopy is a deep copy
+
+	// Ensure the data was copied correctly
+	assert.Equal(t, op.ID, copyOp.ID)
+	assert.Equal(t, op.Namespace, copyOp.Namespace)
+	assert.Equal(t, op.Transaction, copyOp.Transaction)
+	assert.Equal(t, op.Type, copyOp.Type)
+	assert.Equal(t, op.Status, copyOp.Status)
+	assert.Equal(t, op.Plugin, copyOp.Plugin)
+	assert.Equal(t, op.Input, copyOp.Input)
+	assert.Equal(t, op.Output, copyOp.Output)
+	assert.Equal(t, op.Error, copyOp.Error)
+	assert.Equal(t, op.Created, copyOp.Created)
+	assert.Equal(t, op.Updated, copyOp.Updated)
+	assert.Equal(t, op.Retry, copyOp.Retry)
+
+	// Modify the original and ensure the copy is not modified
+	*op.ID = *fftypes.NewUUID()
+	assert.NotEqual(t, copyOp.ID, op.ID)
+
+	*op.Created = *fftypes.Now()
+	assert.NotEqual(t, copyOp.Created, op.Created)
+
+	// Ensure the copy is a deep copy by comparing the pointers of the fields
+	assert.NotSame(t, copyOp.ID, op.ID)
+	assert.NotSame(t, copyOp.Created, op.Created)
+	assert.NotSame(t, copyOp.Updated, op.Updated)
+	assert.NotSame(t, copyOp.Transaction, op.Transaction)
+	assert.NotSame(t, copyOp.Retry, op.Retry)
+	assert.NotSame(t, copyOp.Input, op.Input)
+	assert.NotSame(t, copyOp.Output, op.Output)
+
+	// showcasing that the shallow copy is a shallow copy and the copied object value changed as well the pointer has the same address as the original
+	assert.Equal(t, shallowCopy.ID, op.ID)
+	assert.Same(t, shallowCopy.ID, op.ID)
+
+	// Ensure no new fields are added to the Operation struct
+	// If a new field is added, this test will fail and the DeepCopy function should be updated
+	assert.Equal(t, 12, reflect.TypeOf(Operation{}).NumField())
+}
 func TestParseNamespacedOpID(t *testing.T) {
 
 	ctx := context.Background()
@@ -124,3 +182,113 @@ func TestParseNamespacedOpID(t *testing.T) {
 	assert.Equal(t, "ns1", ns)
 
 }
+
+func TestDeepCopyMapNil(t *testing.T) {
+	original := map[string]interface{}(nil)
+	copy := deepCopyMap(original)
+	assert.Nil(t, copy)
+}
+
+func TestDeepCopyMapEmpty(t *testing.T) {
+	original := map[string]interface{}{}
+	copy := deepCopyMap(original)
+	assert.NotNil(t, copy)
+	assert.Empty(t, copy)
+}
+
+func TestDeepCopyMapSimple(t *testing.T) {
+	original := map[string]interface{}{
+		"key1": "value1",
+		"key2": 42,
+	}
+	copy := deepCopyMap(original)
+	assert.Equal(t, original, copy)
+}
+
+func TestDeepCopyMapNestedMap(t *testing.T) {
+	original := map[string]interface{}{
+		"key1": map[string]interface{}{
+			"nestedKey1": "nestedValue1",
+		},
+	}
+	copy := deepCopyMap(original)
+	assert.Equal(t, original, copy)
+	assert.NotSame(t, original["key1"], copy["key1"])
+}
+
+func TestDeepCopyMapNestedSlice(t *testing.T) {
+	original := map[string]interface{}{
+		"key1": []interface{}{"value1", 42},
+	}
+	copy := deepCopyMap(original)
+	assert.Equal(t, original, copy)
+	assert.NotSame(t, original["key1"], copy["key1"])
+}
+
+func TestDeepCopyMapMixed(t *testing.T) {
+	original := map[string]interface{}{
+		"key1": "value1",
+		"key2": map[string]interface{}{
+			"nestedKey1": "nestedValue1",
+		},
+		"key3": []interface{}{"value1", 42},
+	}
+	copy := deepCopyMap(original)
+	assert.Equal(t, original, copy)
+	assert.NotSame(t, original["key2"], copy["key2"])
+	assert.NotSame(t, original["key3"], copy["key3"])
+}
+
+func TestDeepCopySliceNil(t *testing.T) {
+	original := []interface{}(nil)
+	copy := deepCopySlice(original)
+	assert.Nil(t, copy)
+}
+
+func TestDeepCopySliceEmpty(t *testing.T) {
+	original := []interface{}{}
+	copy := deepCopySlice(original)
+	assert.NotNil(t, copy)
+	assert.Empty(t, copy)
+}
+
+func TestDeepCopySliceSimple(t *testing.T) {
+	original := []interface{}{"value1", 42}
+	copy := deepCopySlice(original)
+	assert.Equal(t, original, copy)
+}
+
+func TestDeepCopySliceNestedMap(t *testing.T) {
+	original := []interface{}{
+		map[string]interface{}{
+			"nestedKey1": "nestedValue1",
+		},
+	}
+	copy := deepCopySlice(original)
+	assert.Equal(t, original, copy)
+	assert.NotSame(t, original[0], copy[0])
+}
+
+func TestDeepCopySliceNestedSlice(t *testing.T) {
+	original := []interface{}{
+		[]interface{}{"value1", 42},
+	}
+	copy := deepCopySlice(original)
+	assert.Equal(t, original, copy)
+	assert.NotSame(t, original[0], copy[0])
+}
+
+func TestDeepCopySliceMixed(t *testing.T) {
+	original := []interface{}{
+		"value1",
+		42,
+		map[string]interface{}{
+			"nestedKey1": "nestedValue1",
+		},
+		[]interface{}{"value2", 43},
+	}
+	copy := deepCopySlice(original)
+	assert.Equal(t, original, copy)
+	assert.NotSame(t, original[2], copy[2])
+	assert.NotSame(t, original[3], copy[3])
+}