From d6c5719e74b7b9b2cc73b49edda02dfd3aa05c0c Mon Sep 17 00:00:00 2001 From: Vincent Boutour Date: Sat, 12 Aug 2023 00:07:45 +0200 Subject: [PATCH] refactor(cache): Changing constructor design to be less chatty Signed-off-by: Vincent Boutour --- cmd/http/port.go | 2 +- pkg/cache/cache.go | 33 +++++++++++++++++---------------- pkg/cache/cache_test.go | 23 +++++++++++++++-------- pkg/cache/list.go | 4 ++-- pkg/redis/redis.go | 10 +++++++++- 5 files changed, 44 insertions(+), 28 deletions(-) diff --git a/cmd/http/port.go b/cmd/http/port.go index ed25a3a7..b313f9c6 100644 --- a/cmd/http/port.go +++ b/cmd/http/port.go @@ -26,7 +26,7 @@ func newPort(config configuration, client client, adapter adapter) port { defer end(nil) return id, nil - }, time.Hour, 4, portTracer) + }, portTracer).WithTTL(time.Hour) output.template = func(w http.ResponseWriter, r *http.Request) (renderer.Page, error) { var err error diff --git a/pkg/cache/cache.go b/pkg/cache/cache.go index 1135a0e6..8bb194ae 100644 --- a/pkg/cache/cache.go +++ b/pkg/cache/cache.go @@ -43,21 +43,18 @@ type App[K comparable, V any] struct { onMissMany fetchMany[K, V] ttl time.Duration concurrency int - extendOnHit bool } -func New[K comparable, V any](client RedisClient, toKey func(K) string, onMiss fetch[K, V], ttl time.Duration, concurrency int, tracer trace.Tracer) *App[K, V] { +func New[K comparable, V any](client RedisClient, toKey func(K) string, onMiss fetch[K, V], tracer trace.Tracer) *App[K, V] { client = getClient(client) return &App[K, V]{ - read: client, - write: client, - toKey: toKey, - serializer: JSONSerializer[V]{}, - onMiss: onMiss, - ttl: ttl, - concurrency: concurrency, - tracer: tracer, + read: client, + write: client, + toKey: toKey, + serializer: JSONSerializer[V]{}, + onMiss: onMiss, + tracer: tracer, } } @@ -79,8 +76,14 @@ func (a *App[K, V]) WithRead(client RedisClient) *App[K, V] { return a } -func (a *App[K, V]) WithExtendOnHit() *App[K, V] { - a.extendOnHit = true +func (a *App[K, V]) WithTTL(ttl time.Duration) *App[K, V] { + a.ttl = ttl + + return a +} + +func (a *App[K, V]) WithMaxConcurrency(concurrency int) *App[K, V] { + a.concurrency = concurrency return a } @@ -117,9 +120,7 @@ func (a *App[K, V]) Get(ctx context.Context, id K) (V, error) { } else if value, ok, err := a.decode([]byte(content)); err != nil { logUnmarshallError(ctx, key, err) } else if ok { - if a.extendOnHit { - a.extendTTL(ctx, key) - } + a.extendTTL(ctx, key) return value, nil } @@ -175,7 +176,7 @@ func (a *App[K, V]) decode(content []byte) (value V, ok bool, err error) { } func (a *App[K, V]) extendTTL(ctx context.Context, keys ...string) { - if a.write == nil || len(keys) == 0 { + if a.write == nil || len(keys) == 0 || a.ttl == 0 { return } diff --git a/pkg/cache/cache_test.go b/pkg/cache/cache_test.go index f04e3530..f46517b6 100644 --- a/pkg/cache/cache_test.go +++ b/pkg/cache/cache_test.go @@ -44,7 +44,6 @@ func TestGet(t *testing.T) { ID: 8000, }, nil }, - duration: time.Minute, }, cacheableItem{ ID: 8000, @@ -59,7 +58,6 @@ func TestGet(t *testing.T) { ID: 8000, }, nil }, - duration: time.Minute, }, cacheableItem{ ID: 8000, @@ -67,6 +65,19 @@ func TestGet(t *testing.T) { nil, }, "cached": { + args{ + key: 8000, + onMiss: func(_ context.Context, id int) (cacheableItem, error) { + return cacheableItem{}, nil + }, + duration: time.Minute, + }, + cacheableItem{ + ID: 8000, + }, + nil, + }, + "cached no extend": { args{ key: 8000, onMiss: func(_ context.Context, id int) (cacheableItem, error) { @@ -86,7 +97,6 @@ func TestGet(t *testing.T) { ID: 8000, }, nil }, - duration: time.Minute, }, cacheableItem{ ID: 8000, @@ -148,10 +158,7 @@ func TestGet(t *testing.T) { mockRedisClient.EXPECT().Store(gomock.Any(), gomock.Any(), gomock.Any(), gomock.Any()).Return(nil) } - instance := New(mockRedisClient, strconv.Itoa, testCase.args.onMiss, testCase.args.duration, -1, nil) - if intention == "cached" { - instance = instance.WithExtendOnHit() - } + instance := New(mockRedisClient, strconv.Itoa, testCase.args.onMiss, nil).WithTTL(testCase.args.duration) got, gotErr := instance.Get(context.Background(), testCase.args.key) @@ -229,7 +236,7 @@ func TestGetError(t *testing.T) { mockRedisClient.EXPECT().Load(gomock.Any(), gomock.Any()).Return(nil, nil) } - instance := New(mockRedisClient, strconv.Itoa, testCase.args.onMiss, testCase.args.duration, -1, nil) + instance := New(mockRedisClient, strconv.Itoa, testCase.args.onMiss, nil).WithTTL(testCase.args.duration) got, gotErr := instance.Get(context.Background(), testCase.args.key) diff --git a/pkg/cache/list.go b/pkg/cache/list.go index 165591ea..4a25d39b 100644 --- a/pkg/cache/list.go +++ b/pkg/cache/list.go @@ -96,7 +96,7 @@ func (a App[K, V]) handleListSingle(ctx context.Context, onMissError func(K, err if ok { output[index] = value - if a.extendOnHit { + if a.ttl != 0 { extendKeys = append(extendKeys, keys[index]) } @@ -131,7 +131,7 @@ func (a App[K, V]) handleListMany(ctx context.Context, items []K, keys, values [ if value, ok, err := a.decode([]byte(values[index])); ok { output[index] = value - if a.extendOnHit { + if a.ttl != 0 { extendKeys = append(extendKeys, keys[index]) } diff --git a/pkg/redis/redis.go b/pkg/redis/redis.go index 163f249a..d5a4043b 100644 --- a/pkg/redis/redis.go +++ b/pkg/redis/redis.go @@ -96,7 +96,7 @@ func (a *App) Ping(ctx context.Context) error { } func (a *App) Store(ctx context.Context, key string, value any, duration time.Duration) error { - return a.client.SetEx(ctx, key, value, duration).Err() + return a.client.Set(ctx, key, value, duration).Err() } func (a *App) Load(ctx context.Context, key string) ([]byte, error) { @@ -162,6 +162,10 @@ func (a *App) pipelinedGet(ctx context.Context, keys ...string) ([]string, error } func (a *App) Expire(ctx context.Context, ttl time.Duration, keys ...string) error { + if len(keys) == 0 { + return nil + } + pipeline := a.client.Pipeline() for _, key := range keys { @@ -172,6 +176,10 @@ func (a *App) Expire(ctx context.Context, ttl time.Duration, keys ...string) err } func (a *App) Delete(ctx context.Context, keys ...string) (err error) { + if len(keys) == 0 { + return nil + } + pipeline := a.client.Pipeline() for _, key := range keys {