Repository: hyperledger/firefly (go)

Issue:
Aggregator stuck on environment in latest mainline
Aggregator stuck on this:
```
[2022-04-12T19:52:09.498Z] ERROR node_0: SQL insert failed: UNIQUE constraint failed: contractapis.namespace, contractapis.name sql=[ INSERT INTO contractapis (id,interface_id,location,name,namespace,message_id) VALUES ($1,$2,$3,$4,$5,$6) ]: UNIQUE constraint failed: contractapis.namespace, contractapis.name dbtx=HMQh6mZQ role=aggregator
[2022-04-12T19:52:09.498Z]  INFO node_0: Result of definition broadcast 'ff_define_contract_api' [ce2f2857-963d-49b1-a774-fe30c2dc67d7]: retry dbtx=HMQh6mZQ role=aggregator
[2022-04-12T19:52:09.499Z]  WARN node_0: SQL! transaction rollback dbtx=HMQh6mZQ role=aggregator
[2022-04-12T19:52:09.499Z] ERROR node_0: process events attempt 25: FF10116: Database insert failed: UNIQUE constraint failed: contractapis.namespace, contractapis.name role=ep[ff_system:ff_aggregator]
```
[output.log](https://github.com/hyperledger/firefly/files/8476365/output.log)



Pull request:
Reject Contract API updates with an ID mismatch
Fixes #703

More work can be done to reject upfront for a better user experience, but this fixes the important bug that blocks event processing.

Patch:
diff --git a/internal/database/sqlcommon/contractapis_sql.go b/internal/database/sqlcommon/contractapis_sql.go
index a8d4283c1..9dc6f1c1d 100644
--- a/internal/database/sqlcommon/contractapis_sql.go
+++ b/internal/database/sqlcommon/contractapis_sql.go
@@ -53,12 +53,22 @@ func (s *SQLCommon) UpsertContractAPI(ctx context.Context, api *fftypes.Contract
 	rows, _, err := s.queryTx(ctx, tx,
 		sq.Select("id").
 			From("contractapis").
-			Where(sq.And{sq.Eq{"id": api.ID}}),
+			Where(sq.And{sq.Eq{"namespace": api.Namespace}, sq.Eq{"name": api.Name}}),
 	)
 	if err != nil {
 		return err
 	}
 	existing := rows.Next()
+
+	if existing {
+		var id fftypes.UUID
+		_ = rows.Scan(&id)
+		if api.ID != nil && *api.ID != id {
+			rows.Close()
+			return database.IDMismatch
+		}
+		api.ID = &id // Update on returned object
+	}
 	rows.Close()
 
 	if existing {
diff --git a/internal/definitions/definition_handler_contracts.go b/internal/definitions/definition_handler_contracts.go
index 9c1e2497f..72f850cad 100644
--- a/internal/definitions/definition_handler_contracts.go
+++ b/internal/definitions/definition_handler_contracts.go
@@ -19,6 +19,7 @@ package definitions
 import (
 	"context"
 
+	"github.com/hyperledger/firefly/pkg/database"
 	"github.com/hyperledger/firefly/pkg/fftypes"
 	"github.com/hyperledger/firefly/pkg/log"
 )
@@ -54,15 +55,23 @@ func (dh *definitionHandlers) persistFFI(ctx context.Context, ffi *fftypes.FFI)
 func (dh *definitionHandlers) persistContractAPI(ctx context.Context, api *fftypes.ContractAPI) (valid bool, err error) {
 	existing, err := dh.database.GetContractAPIByName(ctx, api.Namespace, api.Name)
 	if err != nil {
-		return false, err
+		return false, err // retryable
 	}
 	if existing != nil {
 		if !api.LocationAndLedgerEquals(existing) {
-			return false, nil
+			return false, nil // not retryable
 		}
 	}
 	err = dh.database.UpsertContractAPI(ctx, api)
-	return err == nil, err
+	if err != nil {
+		if err == database.IDMismatch {
+			log.L(ctx).Errorf("Invalid contract API '%s'. ID mismatch with existing record", api.ID)
+			return false, nil // not retryable
+		}
+		log.L(ctx).Errorf("Failed to insert contract API '%s': %s", api.ID, err)
+		return false, err // retryable
+	}
+	return true, nil
 }
 
 func (dh *definitionHandlers) handleFFIBroadcast(ctx context.Context, state DefinitionBatchState, msg *fftypes.Message, data fftypes.DataArray, tx *fftypes.UUID) (HandlerResult, error) {


Test patch:
diff --git a/internal/database/sqlcommon/contractapis_sql_test.go b/internal/database/sqlcommon/contractapis_sql_test.go
index 79caa9ec6..2fb5979ff 100644
--- a/internal/database/sqlcommon/contractapis_sql_test.go
+++ b/internal/database/sqlcommon/contractapis_sql_test.go
@@ -22,6 +22,7 @@ import (
 	"testing"
 
 	"github.com/DATA-DOG/go-sqlmock"
+	"github.com/hyperledger/firefly/mocks/databasemocks"
 	"github.com/hyperledger/firefly/pkg/database"
 	"github.com/hyperledger/firefly/pkg/fftypes"
 	"github.com/hyperledger/firefly/pkg/log"
@@ -121,6 +122,24 @@ func TestContractAPIDBFailUpdate(t *testing.T) {
 	assert.Regexp(t, "pop", err)
 }
 
+func TestUpsertContractAPIIDMismatch(t *testing.T) {
+	s, db := newMockProvider().init()
+	callbacks := &databasemocks.Callbacks{}
+	s.SQLCommon.callbacks = callbacks
+	apiID := fftypes.NewUUID()
+	api := &fftypes.ContractAPI{
+		ID:        apiID,
+		Namespace: "ns1",
+	}
+
+	db.ExpectBegin()
+	db.ExpectQuery("SELECT .*").WillReturnRows(sqlmock.NewRows([]string{"id"}).AddRow("1"))
+	db.ExpectRollback()
+	err := s.UpsertContractAPI(context.Background(), api)
+	assert.Equal(t, database.IDMismatch, err)
+	assert.NoError(t, db.ExpectationsWereMet())
+}
+
 func TestContractAPIDBFailScan(t *testing.T) {
 	s, mock := newMockProvider().init()
 	apiID := fftypes.NewUUID()
diff --git a/internal/definitions/definition_handler_contracts_test.go b/internal/definitions/definition_handler_contracts_test.go
index 8b0886a42..fb95d6a67 100644
--- a/internal/definitions/definition_handler_contracts_test.go
+++ b/internal/definitions/definition_handler_contracts_test.go
@@ -24,6 +24,7 @@ import (
 
 	"github.com/hyperledger/firefly/mocks/contractmocks"
 	"github.com/hyperledger/firefly/mocks/databasemocks"
+	"github.com/hyperledger/firefly/pkg/database"
 	"github.com/hyperledger/firefly/pkg/fftypes"
 	"github.com/stretchr/testify/assert"
 	"github.com/stretchr/testify/mock"
@@ -279,6 +280,17 @@ func TestPersistContractAPIUpsertFail(t *testing.T) {
 	mbi.AssertExpectations(t)
 }
 
+func TestPersistContractAPIIDMismatch(t *testing.T) {
+	dh, _ := newTestDefinitionHandlers(t)
+	mbi := dh.database.(*databasemocks.Plugin)
+	mbi.On("GetContractAPIByName", mock.Anything, mock.Anything, mock.Anything).Return(nil, nil)
+	mbi.On("UpsertContractAPI", mock.Anything, mock.Anything, mock.Anything).Return(database.IDMismatch)
+	valid, err := dh.persistContractAPI(context.Background(), testContractAPI())
+	assert.False(t, valid)
+	assert.NoError(t, err)
+	mbi.AssertExpectations(t)
+}
+
 func TestHandleContractAPIBroadcastValidateFail(t *testing.T) {
 	dh, bs := newTestDefinitionHandlers(t)
 	api := testContractAPI()