fix incorrect merge conflict

sourcenetwork · Jun 17, 2024 · 749499b · 749499b
1 parent 2b70154
commit 749499b
Show file tree

Hide file tree

Showing 4 changed files with 32 additions and 28 deletions.
diff --git a/datastore/badger/v4/datastore_test.go b/datastore/badger/v4/datastore_test.go
@@ -1130,7 +1130,7 @@ func TestTxnWithConflict(t *testing.T) {
 	require.ErrorIs(t, err, ErrTxnConflict)
 }
 
-func TestTxnWithConflictAfterDelete(t *testing.T) {
+func TestTxnWithNoConflictAfterDelete(t *testing.T) {
 	ctx := context.Background()
 	s := newLoadedDatastore(ctx, t)
 	defer func() {
@@ -1144,9 +1144,6 @@ func TestTxnWithConflictAfterDelete(t *testing.T) {
 	tx2, err := s.NewTransaction(ctx, false)
 	require.NoError(t, err)
 
-	_, err = tx.GetSize(ctx, testKey2)
-	require.NoError(t, err)
-
 	err = tx.Put(ctx, testKey2, testValue3)
 	require.NoError(t, err)
 
@@ -1157,7 +1154,7 @@ func TestTxnWithConflictAfterDelete(t *testing.T) {
 	require.NoError(t, err)
 
 	err = tx.Commit(ctx)
-	require.ErrorIs(t, err, ErrTxnConflict)
+	require.NoError(t, err)
 }
 
 func TestTxnWithNoConflictAfterGet(t *testing.T) {

diff --git a/datastore/memory/txn.go b/datastore/memory/txn.go
@@ -80,6 +80,11 @@ func (t *basicTxn) get(ctx context.Context, key ds.Key) dsItem {
 	if result.key == "" {
 		result = t.ds.get(ctx, key, t.getDSVersion())
 		result.isGet = true
+		if result.key == "" {
+			// If the datastore doesn't have the item, we still need to track it
+			// to check for merge conflicts.
+			result.key = key.String()
+		}
 		t.ops.Set(result)
 	}
 	return result
@@ -97,7 +102,7 @@ func (t *basicTxn) Get(ctx context.Context, key ds.Key) ([]byte, error) {
 		return nil, ErrTxnDiscarded
 	}
 	result := t.get(ctx, key)
-	if result.key == "" || result.isDeleted {
+	if result.version == 0 || result.isDeleted {
 		return nil, ds.ErrNotFound
 	}
 	return result.val, nil
@@ -115,7 +120,7 @@ func (t *basicTxn) GetSize(ctx context.Context, key ds.Key) (size int, err error
 		return 0, ErrTxnDiscarded
 	}
 	result := t.get(ctx, key)
-	if result.key == "" || result.isDeleted {
+	if result.version == 0 || result.isDeleted {
 		return 0, ds.ErrNotFound
 	}
 	return len(result.val), nil
@@ -133,7 +138,7 @@ func (t *basicTxn) Has(ctx context.Context, key ds.Key) (exists bool, err error)
 		return false, ErrTxnDiscarded
 	}
 	result := t.get(ctx, key)
-	if result.key == "" || result.isDeleted {
+	if result.version == 0 || result.isDeleted {
 		return false, nil
 	}
 	return true, nil
@@ -270,8 +275,14 @@ func (t *basicTxn) checkForConflicts(ctx context.Context) error {
 	iter := t.ops.Iter()
 	defer iter.Release()
 	for iter.Next() {
-		expectedItem := t.ds.get(ctx, ds.NewKey(iter.Item().key), t.getDSVersion())
-		latestItem := t.ds.get(ctx, ds.NewKey(iter.Item().key), t.ds.getVersion())
+		item := iter.Item()
+		if !item.isGet {
+			// Conflict should only ocure if an item has been updated
+			// after we've read it within the transaction.
+			continue
+		}
+		expectedItem := t.ds.get(ctx, ds.NewKey(item.key), t.getDSVersion())
+		latestItem := t.ds.get(ctx, ds.NewKey(item.key), t.ds.getVersion())
 		if latestItem.version != expectedItem.version {
 			return ErrTxnConflict
 		}

diff --git a/datastore/memory/txn_test.go b/datastore/memory/txn_test.go
@@ -707,11 +707,16 @@ func TestTxnWithConflict(t *testing.T) {
 		require.NoError(t, err)
 	}()
 
-	tx := s.newTransaction(false)
+	tx, err := s.NewTransaction(ctx, false)
+	require.NoError(t, err)
 
-	tx2 := s.newTransaction(false)
+	tx2, err := s.NewTransaction(ctx, false)
+	require.NoError(t, err)
 
-	err := tx.Put(ctx, testKey3, testValue3)
+	_, err = tx.GetSize(ctx, testKey3)
+	require.ErrorIs(t, err, ds.ErrNotFound)
+
+	err = tx.Put(ctx, testKey3, testValue3)
 	require.NoError(t, err)
 
 	err = tx2.Put(ctx, testKey3, testValue4)
@@ -724,7 +729,7 @@ func TestTxnWithConflict(t *testing.T) {
 	require.ErrorIs(t, err, ErrTxnConflict)
 }
 
-func TestTxnWithConflictAfterDelete(t *testing.T) {
+func TestTxnWithNoConflictAfterDelete(t *testing.T) {
 	ctx := context.Background()
 	s := newLoadedDatastore(ctx)
 	defer func() {
@@ -746,7 +751,7 @@ func TestTxnWithConflictAfterDelete(t *testing.T) {
 	require.NoError(t, err)
 
 	err = tx.Commit(ctx)
-	require.ErrorIs(t, err, ErrTxnConflict)
+	require.NoError(t, err)
 }
 
 func TestTxnWithConflictAfterGet(t *testing.T) {

diff --git a/datastore/txn_test.go b/datastore/txn_test.go
@@ -201,8 +201,7 @@ func TestShimTxnStoreClose(t *testing.T) {
 	require.NoError(t, err)
 }
 
-// This test documents https://github.com/sourcenetwork/defradb/issues/2673
-func TestMemoryStoreTxn_TwoTransactionsWithPutConflict_ShouldErrorWithConflict(t *testing.T) {
+func TestMemoryStoreTxn_TwoTransactionsWithPutConflict_ShouldSucceed(t *testing.T) {
 	ctx := context.Background()
 	rootstore := memory.NewDatastore(ctx)
 
@@ -223,7 +222,7 @@ func TestMemoryStoreTxn_TwoTransactionsWithPutConflict_ShouldErrorWithConflict(t
 	require.NoError(t, err)
 
 	err = txn1.Commit(ctx)
-	require.ErrorIs(t, err, badger.ErrConflict)
+	require.NoError(t, err)
 }
 
 func TestMemoryStoreTxn_TwoTransactionsWithGetPutConflict_ShouldErrorWithConflict(t *testing.T) {
@@ -284,8 +283,7 @@ func TestMemoryStoreTxn_TwoTransactionsWithHasPutConflict_ShouldErrorWithConflic
 	require.ErrorIs(t, err, badger.ErrConflict)
 }
 
-// This test documents https://github.com/sourcenetwork/defradb/issues/2673
-func TestBadgerMemoryStoreTxn_TwoTransactionsWithPutConflict_ShouldErrorWithConflict(t *testing.T) {
+func TestBadgerMemoryStoreTxn_TwoTransactionsWithPutConflict_ShouldSucceed(t *testing.T) {
 	ctx := context.Background()
 	opts := badgerds.Options{Options: badger.DefaultOptions("").WithInMemory(true)}
 	rootstore, err := badgerds.NewDatastore("", &opts)
@@ -308,9 +306,6 @@ func TestBadgerMemoryStoreTxn_TwoTransactionsWithPutConflict_ShouldErrorWithConf
 	require.NoError(t, err)
 
 	err = txn1.Commit(ctx)
-	// We are expecting this to fail because of the conflict but badger does not return an error.
-	// Conflicts in badger only occurs when the value of a key was changed between the time you read and you rewrite it.
-	// require.ErrorIs(t, err, badger.ErrConflict)
 	require.NoError(t, err)
 }
 
@@ -376,8 +371,7 @@ func TestBadgerMemoryStoreTxn_TwoTransactionsWithHasPutConflict_ShouldErrorWithC
 	require.ErrorIs(t, err, badger.ErrConflict)
 }
 
-// This test documents https://github.com/sourcenetwork/defradb/issues/2673
-func TestBadgerFileStoreTxn_TwoTransactionsWithPutConflict_ShouldErrorWithConflict(t *testing.T) {
+func TestBadgerFileStoreTxn_TwoTransactionsWithPutConflict_ShouldSucceed(t *testing.T) {
 	ctx := context.Background()
 	opts := badgerds.Options{Options: badger.DefaultOptions("")}
 	rootstore, err := badgerds.NewDatastore(t.TempDir(), &opts)
@@ -400,9 +394,6 @@ func TestBadgerFileStoreTxn_TwoTransactionsWithPutConflict_ShouldErrorWithConfli
 	require.NoError(t, err)
 
 	err = txn1.Commit(ctx)
-	// We are expecting this to fail because of the conflict but badger does not return an error.
-	// Conflicts in badger only occurs when the value of a key was changed between the time you read and you rewrite it.
-	// require.ErrorIs(t, err, badger.ErrConflict)
 	require.NoError(t, err)
 }