Skip to content
Open
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
2 changes: 1 addition & 1 deletion maputil/orderedmap.go
Original file line number Diff line number Diff line change
Expand Up @@ -35,7 +35,7 @@ func (om *OrderedMap[K, V]) Set(key K, value V) {
defer om.mu.Unlock()

if elem, ok := om.index[key]; ok {
elem.Value = value
om.data[key] = value
om.order.MoveToBack(elem)

return
Expand Down
4 changes: 4 additions & 0 deletions maputil/orderedmap_test.go
Original file line number Diff line number Diff line change
Expand Up @@ -19,6 +19,10 @@ func TestOrderedMap_Set_Get(t *testing.T) {
assert.Equal(1, val)
assert.Equal(true, ok)

om.Set("a", 2)
val, _ = om.Get("a")
assert.Equal(2, val)

val, ok = om.Get("d")
assert.Equal(false, ok)
assert.Equal(0, val)
Expand Down
134 changes: 134 additions & 0 deletions test.go
Original file line number Diff line number Diff line change
@@ -0,0 +1,134 @@
package cache

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

⚠️ Potential issue

Missing definition of ICache.

The code references an ICache interface at lines 18 and 24, but that interface is not defined within this file or imported from elsewhere. This will result in a compilation error unless ICache is declared in another part of the codebase that’s properly imported.


import (
"context"
"encoding/json"
"time"

"github.com/go-redis/redis/v8"
"github.com/pkg/errors"

"ep-services/pkg/lib/log"
"ep-services/utils"
)

type GetValueFunc[T any] func() (map[string]T, error)

type CacheHandler[T any] struct {
Cache ICache
CacheKey string // 缓存Key
Expiration time.Duration // 过期时间
GetValueFunc GetValueFunc[T] // 获取数据func
}

func NewCacheHandler[T any](cache ICache, cacheKey string, expiration time.Duration, fn GetValueFunc[T]) (*CacheHandler[T], error) {
if fn == nil {
return nil, errors.New("no get entity func")
}
return &CacheHandler[T]{
Cache: cache,
CacheKey: cacheKey,
Expiration: expiration,
GetValueFunc: fn,
}, nil
}

// 获取缓存中的数据,如果没有则设置到缓存中并返回
func (c *CacheHandler[T]) GetValueFromCache(subkeys ...string) ([]T, error) {
value, err := c.getCache(subkeys...)
if err != nil || value == nil {
// 加载数据
values, err := c.setToCacheAndGetValue()
if err != nil {
return nil, err
}
return values, nil
}
return value, nil
}

// 缓存失效存入缓存,这里直接返回所有值
// 后面从缓存获取只返回subkey对应值
func (c *CacheHandler[T]) setToCacheAndGetValue() ([]T, error) {
valueMap, err := c.GetValueFunc()
if err != nil {
return nil, err
}
newMap := make(map[string]string)
for k, v := range valueMap {
v_ := v
b, err := json.Marshal(&v_)
if err != nil {
Comment on lines +59 to +61

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

style: unnecessary variable copy of v_ - can directly marshal v

Suggested change
v_ := v
b, err := json.Marshal(&v_)
if err != nil {
b, err := json.Marshal(&v)
if err != nil {

return nil, err
}
newMap[k] = string(b)
}
c.setCache(newMap)
return utils.MapValues(valueMap), nil
}
Comment on lines +52 to +68

This comment was marked as resolved.

Copy link
Copy Markdown
Owner Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

/korbit-resolve

Copy link
Copy Markdown
Owner Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

/korbit-generate-pr-description

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@congziqi77 I am looking at your pull request. The description will be updated shortly. In the meantime, please do not edit the description until I have finished writing mine.


func (c *CacheHandler[T]) setCache(value map[string]string) {
ctx, cancel := context.WithTimeout(context.Background(), 1*time.Second)
defer cancel()
if err := c.Cache.HashSetEx(ctx, c.CacheKey, value, c.Expiration); err != nil {
log.Error().Msgf("set cache error : %s", err)

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Consider whether silently logging errors in 'setCache' is sufficient. It might be beneficial to propagate the error to the caller for improved error handling.

Copy link
Copy Markdown
Owner Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@ellipsis-dev Can you give me a similar example?

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Here's a simple example of error handling in Go, focusing on logging and propagating errors:

package main

import (
    "fmt"
    "log"
)

// Example function that performs an operation and returns an error
func performOperation() error {
    // Simulate an error
    err := fmt.Errorf("operation failed")
    if err != nil {
        // Log the error with context
        log.Printf("Error in performOperation: %v", err)
        // Return the error to the caller
        return fmt.Errorf("performOperation failed: %w", err)
    }
    return nil
}

func main() {
    if err := performOperation(); err != nil {
        // Handle the error returned from performOperation
        log.Fatalf("Application error: %v", err)
    }
}

In this example, the performOperation function logs the error with context and returns it to the caller, allowing the caller to handle it appropriately. This pattern helps in maintaining clear error propagation and logging.

}
Comment on lines +73 to +75

This comment was marked as resolved.

}
Comment on lines +70 to +76

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The function silently ignores cache set errors, only logging them. This could lead to inconsistent state where callers assume data was cached but it wasn't.

Copy link
Copy Markdown
Owner Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@trag-bot Can you give me a similar example?

Comment on lines +70 to +76

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

logic: setCache silently logs errors without propagating them up. This could mask critical failures in the caching layer.


func (c *CacheHandler[T]) getCache(subkeys ...string) ([]T, error) {
ctx, cancel := context.WithTimeout(context.Background(), 1*time.Second)
defer cancel()
// 如果没有subkeys那么返回所有数据
var values []string
var err error
if len(subkeys) == 0 {
values, err = c.getAllValue(ctx)
} else {
values, err = c.getValue(ctx, subkeys...)
}
if err != nil {
if err == redis.Nil {
return nil, nil
}
log.Error().Msgf("get cache error : %s", err)
return nil, err
}
Comment on lines +89 to +95

This comment was marked as resolved.

Copy link
Copy Markdown
Owner Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@korbit-ai Can you give me a similar example?

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@congziqi77 Sure! Here's a similar example of error logging with the error context. In some codebase, when there is an error when accessing the database, you might see something like this:

if err != nil {  
  log.Error().Msgf("Failed to access database for employee id %s: %s", empId, err)  
  return nil, err  
}```

This way, if you encounter an error, you can quickly identify that it came from the database access operation and the associated employee id. For your case to add context in the error log, the suggestion would be to replace this line of code: 

`log.Error().Msgf("get cache error : %s", err)` 

with 

`log.Error().Msgf("failed to get cache for key %s: %s", c.CacheKey, err)`.
This would make it clear where the error came from when debugging.


if len(values) == 0 {
return nil, nil
}

valMap := []T{}
for _, v := range values {
var t T
err := json.Unmarshal([]byte(v), &t)
if err != nil {
return nil, err
}
valMap = append(valMap, t)
}
return valMap, nil
}

func (c *CacheHandler[T]) getAllValue(ctx context.Context) ([]string, error) {
valueMap, err := c.Cache.HashGetAll(ctx, c.CacheKey)
if err != nil {
return nil, err
}
values := utils.MapValues(valueMap)
return values, nil
}

func (c *CacheHandler[T]) getValue(ctx context.Context, subkeys ...string) ([]string, error) {
values, err := c.Cache.HashMGet(ctx, c.CacheKey, subkeys...)
if err != nil {
return nil, err
}
valArr := []string{}
for _, v := range values {
if v_, ok := v.(string); ok {
valArr = append(valArr, v_)
}
}
return valArr, nil
}