From cc755c027e4f0ca5672164bf5c6ccea215223215 Mon Sep 17 00:00:00 2001 From: tbuchaillot Date: Wed, 31 Jan 2024 18:20:43 +0100 Subject: [PATCH 1/2] Adding SetIfNotExist to KeyValue --- temporal/internal/driver/redisv9/keyvalue.go | 9 ++ temporal/keyvalue/keyvalue_test.go | 93 ++++++++++++++++++++ temporal/model/types.go | 2 + temporal/tempmocks/key_value.go | 28 ++++++ 4 files changed, 132 insertions(+) diff --git a/temporal/internal/driver/redisv9/keyvalue.go b/temporal/internal/driver/redisv9/keyvalue.go index eab39b45..c1402546 100644 --- a/temporal/internal/driver/redisv9/keyvalue.go +++ b/temporal/internal/driver/redisv9/keyvalue.go @@ -479,3 +479,12 @@ func fetchKeys(ctx context.Context, return keys, cursor, nil } + +func (r *RedisV9) SetIfNotExist(ctx context.Context, key, value string, expiration time.Duration) (bool, error) { + if key == "" { + return false, temperr.KeyEmpty + } + + res := r.client.SetNX(ctx, key, value, expiration) + return res.Val(), res.Err() +} diff --git a/temporal/keyvalue/keyvalue_test.go b/temporal/keyvalue/keyvalue_test.go index 019e1340..ae565e70 100644 --- a/temporal/keyvalue/keyvalue_test.go +++ b/temporal/keyvalue/keyvalue_test.go @@ -1261,3 +1261,96 @@ func TestKeyValue_GetKeysWithOpts(t *testing.T) { } } } + +func TestKeyValue_SetIfNotExist(t *testing.T) { + connectors := testutil.TestConnectors(t) + defer testutil.CloseConnectors(t, connectors) + + tcs := []struct { + name string + key string + value string + pre func(kv model.KeyValue) + expiration time.Duration + expectedErr error + expectedSet bool + }{ + { + name: "set_with_valid_key_and_value", + key: "key1", + value: "value1", + expiration: 10 * time.Second, + expectedErr: nil, + expectedSet: true, + }, + { + name: "set_with_empty_key", + key: "", + value: "value2", + expiration: 10 * time.Second, + expectedErr: temperr.KeyEmpty, + }, + { + name: "set_with_empty_value", + key: "key3", + value: "", + expiration: 10 * time.Second, + expectedErr: nil, + expectedSet: true, + }, + { + name: "set_with_no_expiration", + key: "key4", + value: "value4", + expiration: 10 * time.Second, + expectedErr: nil, + expectedSet: true, + }, + { + name: "set_already_existing_key", + key: "key5", + value: "value5", + expiration: 10 * time.Second, + expectedErr: nil, + pre: func(kv model.KeyValue) { + t.Helper() + err := kv.Set(context.Background(), "key5", "value5", 10*time.Second) + assert.Nil(t, err) + }, + expectedSet: false, + }, + } + + for _, connector := range connectors { + for _, tc := range tcs { + t.Run(connector.Type()+"_"+tc.name, func(t *testing.T) { + ctx := context.Background() + + kv, err := NewKeyValue(connector) + assert.Nil(t, err) + + flusher, err := flusher.NewFlusher(connector) + assert.Nil(t, err) + defer assert.Nil(t, flusher.FlushAll(ctx)) + + if tc.pre != nil { + tc.pre(kv) + } + + val, err := kv.SetIfNotExist(ctx, tc.key, tc.value, tc.expiration) + assert.Equal(t, tc.expectedErr, err) + assert.Equal(t, tc.expectedSet, val) + if err == nil { + actualValue, err := kv.Get(ctx, tc.key) + assert.Nil(t, err) + + assert.Equal(t, tc.value, actualValue) + + actualTTL, err := kv.TTL(ctx, tc.key) + assert.Nil(t, err) + assert.True(t, actualTTL <= int64(tc.expiration.Seconds())) + } + }) + } + } +} diff --git a/temporal/model/types.go b/temporal/model/types.go index 9f91f5b4..163472a5 100644 --- a/temporal/model/types.go +++ b/temporal/model/types.go @@ -55,6 +55,8 @@ type KeyValue interface { Get(ctx context.Context, key string) (value string, err error) // Set sets the string value of a key Set(ctx context.Context, key, value string, ttl time.Duration) error + // SetIfNotExist sets the string value of a key if the key does not exist. Returns true if the key was set, false otherwise. + SetIfNotExist(ctx context.Context, key, value string, expiration time.Duration) (bool, error) // Delete removes the specified keys Delete(ctx context.Context, key string) error // Increment atomically increments the integer value of a key by one diff --git a/temporal/tempmocks/key_value.go b/temporal/tempmocks/key_value.go index cbe63130..10ce09ae 100644 --- a/temporal/tempmocks/key_value.go +++ b/temporal/tempmocks/key_value.go @@ -373,6 +373,34 @@ func (_m *KeyValue) Set(ctx context.Context, key string, value string, ttl time. return r0 } +// SetIfNotExist provides a mock function with given fields: ctx, key, value, expiration +func (_m *KeyValue) SetIfNotExist(ctx context.Context, key string, value string, expiration time.Duration) (bool, error) { + ret := _m.Called(ctx, key, value, expiration) + + if len(ret) == 0 { + panic("no return value specified for SetIfNotExist") + } + + var r0 bool + var r1 error + if rf, ok := ret.Get(0).(func(context.Context, string, string, time.Duration) (bool, error)); ok { + return rf(ctx, key, value, expiration) + } + if rf, ok := ret.Get(0).(func(context.Context, string, string, time.Duration) bool); ok { + r0 = rf(ctx, key, value, expiration) + } else { + r0 = ret.Get(0).(bool) + } + + if rf, ok := ret.Get(1).(func(context.Context, string, string, time.Duration) error); ok { + r1 = rf(ctx, key, value, expiration) + } else { + r1 = ret.Error(1) + } + + return r0, r1 +} + // TTL provides a mock function with given fields: ctx, key func (_m *KeyValue) TTL(ctx context.Context, key string) (int64, error) { ret := _m.Called(ctx, key) From 088119d4dc5866a1790d9a63f92c400fc06180b4 Mon Sep 17 00:00:00 2001 From: tbuchaillot Date: Wed, 31 Jan 2024 18:25:31 +0100 Subject: [PATCH 2/2] linting --- persistent/model/object_id_test.go | 1 - temporal/internal/driver/redisv9/keyvalue.go | 8 +++++--- temporal/model/types.go | 3 ++- 3 files changed, 7 insertions(+), 5 deletions(-) diff --git a/persistent/model/object_id_test.go b/persistent/model/object_id_test.go index 779b9d35..032a4eb1 100644 --- a/persistent/model/object_id_test.go +++ b/persistent/model/object_id_test.go @@ -92,7 +92,6 @@ func TestUnmarshalJSON(t *testing.T) { var id2 ObjectID err = id2.UnmarshalJSON(idBytes) - if err != nil { t.Fatal(err) } diff --git a/temporal/internal/driver/redisv9/keyvalue.go b/temporal/internal/driver/redisv9/keyvalue.go index c1402546..adea4f55 100644 --- a/temporal/internal/driver/redisv9/keyvalue.go +++ b/temporal/internal/driver/redisv9/keyvalue.go @@ -195,7 +195,6 @@ func (r *RedisV9) DeleteScanMatch(ctx context.Context, pattern string) (int64, e case *redis.Client: var err error totalDeleted, err = r.deleteScanMatchSingleNode(ctx, client, pattern) - if err != nil { if errors.Is(err, redis.ErrClosed) { return totalDeleted, temperr.ClosedConnection @@ -218,7 +217,6 @@ func (r *RedisV9) deleteScanMatchSingleNode(ctx context.Context, client redis.Cm var keys []string keys, _, err = client.Scan(ctx, cursor, pattern, 0).Result() - if err != nil { return int64(deleted), err } @@ -486,5 +484,9 @@ func (r *RedisV9) SetIfNotExist(ctx context.Context, key, value string, expirati } res := r.client.SetNX(ctx, key, value, expiration) - return res.Val(), res.Err() + if res.Err() != nil { + return false, res.Err() + } + + return res.Val(), nil } diff --git a/temporal/model/types.go b/temporal/model/types.go index 163472a5..95697508 100644 --- a/temporal/model/types.go +++ b/temporal/model/types.go @@ -55,7 +55,8 @@ type KeyValue interface { Get(ctx context.Context, key string) (value string, err error) // Set sets the string value of a key Set(ctx context.Context, key, value string, ttl time.Duration) error - // SetIfNotExist sets the string value of a key if the key does not exist. Returns true if the key was set, false otherwise. + // SetIfNotExist sets the string value of a key if the key does not exist. + // Returns true if the key was set, false otherwise. SetIfNotExist(ctx context.Context, key, value string, expiration time.Duration) (bool, error) // Delete removes the specified keys Delete(ctx context.Context, key string) error