handle error in operations' result from transact method correctly (eBay#96)

the results of the operations specified in the transact method is not
correctly verified. we need to always walk through the operations'
results to check for null error to claim a transaction has succeded.

furthermore, per RFC 7047 section 4.1.3, if all of the operations
succeed, but the results cannot be committed, then "result" will have
one more element than "params", with the additional element being an
<error>. this case was not handled as well.

with the fix in place, errors related to "constraint violation",
 "referential integrity violation", "resources exhausted", and
 "I/O error" are being caught.

Signed-off-by: Girish Moodalbail <[email protected]>

Author: girishmg

chassis_test.go

@@ -21,9 +21,11 @@ import (
 )
 
 const (
-	CHASSIS_HOSTNAME = "fakehost"
-	CHASSIS_NAME     = "fakechassis"
-	IP               = "10.0.0.11"
+	CHASSIS_HOSTNAME  = "fakehost"
+	CHASSIS_NAME      = "fakechassis"
+	IP                = "10.0.0.11"
+	CHASSIS2_HOSTNAME = "fakehost2"
+	CHASSIS2_NAME     = "fakechassis2"
 )
 
 // can be one or many encap_types similar to chassis-add of sbctl

ovnimp.go

@@ -25,6 +25,10 @@ import (
 	"github.com/ebay/libovsdb"
 )
 
+const (
+	commitTransactionText = "commiting transaction"
+)
+
 var (
 	// ErrorOption used when invalid args specified
 	ErrorOption = errors.New("invalid option specified")
@@ -149,12 +153,23 @@ func (odbi *ovndb) transact(db string, ops ...libovsdb.Operation) ([]libovsdb.Op
 		return reply, err
 	}
 
-	if len(reply) < len(ops) {
-		for i, o := range reply {
-			if o.Error != "" && i < len(ops) {
-				return nil, fmt.Errorf("Transaction Failed due to an error : %v details: %v in %v", o.Error, o.Details, ops[i])
+	// Per RFC 7047 Section 4.1.3, the operation result array in the transact response object
+	// maps one-to-one with operations array in the transact request object. We need to check
+	// each of the operation result for null error to ensure that the transaction has succeeded.
+	for i, o := range reply {
+		if o.Error != "" {
+			// Per RFC 7047 Section 4.1.3, if all of the operations succeed, but the results
+			// cannot be committed, then "result" will have one more element than "params",
+			// with the additional element being an <error>.
+			opsInfo := commitTransactionText
+			if i < len(ops) {
+				opsInfo = fmt.Sprintf("%v", ops[i])
 			}
+			return nil, fmt.Errorf("Transaction Failed due to an error: %v details: %v in %s",
+				o.Error, o.Details, opsInfo)
 		}
+	}
+	if len(reply) < len(ops) {
 		return reply, fmt.Errorf("Number of Replies should be atleast equal to number of operations")
 	}
 	return reply, nil

ovnimp_test.go

@@ -0,0 +1,46 @@
+package goovn
+
+import (
+	"testing"
+
+	"github.com/stretchr/testify/assert"
+)
+
+func TestBadTransact(t *testing.T) {
+	ovndbapi := getOVNClient(DBSB)
+	t.Logf("Adding Chassis to OVN SB DB")
+	ocmd, err := ovndbapi.ChassisAdd(CHASSIS_NAME, CHASSIS_HOSTNAME, ENCAP_TYPES, IP, nil, nil, nil)
+	if err != nil {
+		t.Fatal(err)
+	}
+	err = ovndbapi.Execute(ocmd)
+	if err != nil {
+		t.Fatalf("Adding Chassis to OVN failed with err %v", err)
+	}
+	t.Logf("Adding Chassis to OVN Done")
+
+	t.Logf("Adding second Chassis to OVN SB DB but with same ENCAP_TYPES and IP")
+	ocmd, err = ovndbapi.ChassisAdd(CHASSIS2_NAME, CHASSIS2_HOSTNAME, ENCAP_TYPES, IP, nil, nil, nil)
+	if err != nil {
+		t.Fatal(err)
+	}
+
+	// expecting constraint violation error with following details -- "Transaction causes multiple
+	// rows in \"Encap\" table to have identical values (stt and \"10.0.0.11\") for index on columns
+	// \"type\" and \"ip\".  First row, with UUID 9860cf40-bd82-4c24-9514-05b225434934, existed in
+	// the database before this transaction and was not modified by the transaction.  Second row,
+	// with UUID 10d7d018-7444-48de-89fc-cb062f88e520, was inserted by this transaction."
+	err = ovndbapi.Execute(ocmd)
+	assert.Error(t, err)
+
+	t.Logf("Deleting Chassis:%v", CHASSIS_NAME)
+	ocmd, err = ovndbapi.ChassisDel(CHASSIS_NAME)
+	if err != nil && err != ErrorNotFound {
+		t.Fatal(err)
+	}
+
+	err = ovndbapi.Execute(ocmd)
+	if err != nil {
+		t.Fatalf("err executing command:%v", err)
+	}
+}