Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

📝 [v3 Proposal]: Improve Storage Interface #2839

Open
3 tasks done
efectn opened this issue Feb 8, 2024 · 5 comments · May be fixed by #2300
Open
3 tasks done

📝 [v3 Proposal]: Improve Storage Interface #2839

efectn opened this issue Feb 8, 2024 · 5 comments · May be fixed by #2300

Comments

@efectn
Copy link
Member

efectn commented Feb 8, 2024

Feature Proposal Description

Idea One

Storage interface doesn't allow us to pass context to make operations cancellable. We should add new methods to allow context operations. Something like:

type Storage interface {
	Get(key string) ([]byte, error)
	Set(key string, val []byte, exp time.Duration) error
	Delete(key string) error
	Reset() error
	Close() error
        GetWithContext(ctx context.Context, key string) ([]byte, error)
	SetWithContext(ctx context.Context, key string, val []byte, exp time.Duration) error
	DeleteWithContext(ctx context.Context, key string) error
	ResetWithContext(ctx context.Context) error
}

Idea Two

We can allow using generics to change key and value types.

type Storage[K ~comparable, V any] interface {
	// Get gets the value for the given key.
	// `nil, nil` is returned when the key does not exist
	Get(key K) (V, error)

	// Set stores the given value for the given key along
	// with an expiration value, 0 means no expiration.
	// Empty key or value will be ignored without an error.
	Set(key K, val V, exp time.Duration) error

	// Delete deletes the value for the given key.
	// It returns no error if the storage does not contain the key,
	Delete(key K) error

	// Reset resets the storage and delete all keys.
	Reset() error

	// Close closes the storage and will stop any running garbage
	// collectors and open connections.
	Close() error
}

Alignment with Express API

Express does not have storage interface.

HTTP RFC Standards Compliance

Not related to core.

API Stability

Not related to core.

Feature Examples

.

Checklist:

  • I agree to follow Fiber's Code of Conduct.
  • I have searched for existing issues that describe my proposal before opening this one.
  • I understand that a proposal that does not meet these guidelines may be closed without explanation.
@ReneWerner87
Copy link
Member

Current

fiber/app.go

Lines 43 to 63 in 847a4a9

type Storage interface {
// Get gets the value for the given key.
// `nil, nil` is returned when the key does not exist
Get(key string) ([]byte, error)
// Set stores the given value for the given key along
// with an expiration value, 0 means no expiration.
// Empty key or value will be ignored without an error.
Set(key string, val []byte, exp time.Duration) error
// Delete deletes the value for the given key.
// It returns no error if the storage does not contain the key,
Delete(key string) error
// Reset resets the storage and delete all keys.
Reset() error
// Close closes the storage and will stop any running garbage
// collectors and open connections.
Close() error
}

@ReneWerner87
Copy link
Member

ReneWerner87 commented Feb 9, 2024

related #2300
image

we should then continue

@ReneWerner87
Copy link
Member

ReneWerner87 commented Feb 9, 2024

i like the first idea

// Storage defines the base storage interface.
type StorageV2 interface { // `V2` this allows us to differentiate between old and new interfaces and support at least the last 3, so that we have the possibility to make further adjustments in the future
	Get(key string) ([]byte, error)
	Set(key string, val []byte, exp time.Duration) error
	Delete(key string) error
	Reset() error
	Close() error
	WithContext() ContextAwareStorage // Methode, gives the ContextAwareStorage-Interface
}

// ContextAwareStorage defines the storage interface with context-aware methods.
type ContextAwareStorage interface {
	Get(ctx context.Context, key string) ([]byte, error)
	Set(ctx context.Context, key string, val []byte, exp time.Duration) error
	Delete(ctx context.Context, key string) error
	Reset(ctx context.Context) error
	Close(ctx context.Context) error
}

but maybe we don't need the old methods anymore

2nd idea
for the storages we should have fixed input parameters for key and value, because we have to align everything in the storages of the underlying packages to them
however, we could also create general conversion methods that allow the old data types as output, but this may be too complex

@nickajacks1
Copy link
Member

I feel like having a version that doesn't take a context is redundant because people can just pass context.TODO() if they dont need a context. Just my personal opinion though.

@half2me
Copy link

half2me commented Feb 28, 2024

The benefit here would be more than just the ability to cancel. I'm trying to add open telemetry tracing to my redis storage, but I can't pass the context, so it's not aware of its trace parent. The redis client just has hardcoded context.Background() hardcoded in: https://github.com/gofiber/storage/blob/7479b8518e8feacd7bedd50e6dd6711c1c4c8bbb/redis/redis.go#L108

@ReneWerner87 ReneWerner87 added this to v3 Apr 17, 2024
@ReneWerner87 ReneWerner87 modified the milestones: Next Release, v3 Apr 17, 2024
@efectn efectn moved this to Todo in v3 Apr 28, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
Status: Todo
Development

Successfully merging a pull request may close this issue.

4 participants