From 66264414b7377a670b78208cf24a95124cd065d3 Mon Sep 17 00:00:00 2001 From: Fred Carle Date: Mon, 17 Jun 2024 12:45:08 -0400 Subject: [PATCH] fix: Resolve incorrect merge conflict (#2723) ## Relevant issue(s) Resolves #2673 ## Description This PR fixes the incorrect merge conflict in our memory store. --- datastore/badger/v4/datastore_test.go | 7 ++----- datastore/memory/txn.go | 21 ++++++++++++++++----- datastore/memory/txn_test.go | 15 ++++++++++----- datastore/txn_test.go | 17 ++++------------- 4 files changed, 32 insertions(+), 28 deletions(-) diff --git a/datastore/badger/v4/datastore_test.go b/datastore/badger/v4/datastore_test.go index 69a24981df..c72ff988db 100644 --- 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 index 7430077e46..f1086332b6 100644 --- 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 occur 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 index 0dae48f2dc..5a0d1a2d8c 100644 --- 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 index 1a8623600f..b11ca3acfe 100644 --- 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) }