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

🤗 [Question]: Can we add an option to database storages accept a already initialized connection? #967

Open
3 tasks done
mirusky opened this issue Aug 31, 2023 · 8 comments
Assignees
Labels

Comments

@mirusky
Copy link

mirusky commented Aug 31, 2023

Question Description

For example my app already opens a connection with redis and I'd like to reuse the same connection, since it's simple to create a health check endpoint that's only checks one redis connection instead of 2 or more.

Currently I'm opening 3 connections since my code opens 1 and I call redis.New twice, so I need to get the remaining connections using store.Conn and pass it as dependency to my health check controller to check all connections.

I'd like to know if it's possible to add an option to database driven storages like "Conn" that we can pass a already established connection. Something simillar to GORM/Existing database connection

Code Snippet (optional)

package main

import (
	"github.com/redis/go-redis/v9"

	fiberredis "github.com/gofiber/storage/redis"
)

func main() {
	url := "redis://localhost:6379?password=hello&protocol=3"
	opts, err := redis.ParseURL(url)
	if err != nil {
		panic(err)
	}
	rdb := redis.NewClient(opts) // Already connected

	store1 := fiberredis.New(redis.Config{URL: config.RedisURL + "/1"}) // New Conn
	store2 := fiberredis.New(redis.Config{URL: config.RedisURL + "/2"}) // New Conn

	// I'm thinking something like this:
        // Ref: https://github.com/redis/go-redis/issues/1018#issuecomment-517928987
	// db1 := rdb.Conn()
	// db1.Select(context.Background(), 1)
	// db2 := rdb.Conn()
	// db2.Select(context.Background(), 2)
	// store1 := fiberredis.New(redis.Config{Conn: db1})
	// store2 := fiberredis.New(redis.Config{Conn: db2})
}

Checklist:

  • I agree to follow Fiber's Code of Conduct.
  • I have checked for existing issues that describe my questions prior to opening this one.
  • I understand that improperly formatted questions may be closed without explanation.
@mirusky mirusky added the 🤔 Question Further information is requested label Aug 31, 2023
@mirusky
Copy link
Author

mirusky commented Aug 31, 2023

I've checked and some stores already have this, just a few don't allow it.

storage/postgres accepts a conn
storage/mysql accepts too

@gaby
Copy link
Member

gaby commented Sep 1, 2023

@ReneWerner87 We could do this by adding Open() as part of the storage interface. If it sounds good to you I can do the PR for all the drivers

@gaby
Copy link
Member

gaby commented Sep 1, 2023

@mirusky Btw you are using the old Redis Driver. The latest one v2 uses the redis.UniversalClient.

@ReneWerner87
Copy link
Member

@ReneWerner87 We could do this by adding Open() as part of the storage interface. If it sounds good to you I can do the PR for all the drivers

Yes, sounds good

@mirusky
Copy link
Author

mirusky commented Sep 1, 2023

@mirusky Btw you are using the old Redis Driver. The latest one v2 uses the redis.UniversalClient.

I just write the on mobile, I didn't saw I used the previous version, but I checked my code on production and it's using the v2. Thx for pointing out.

@ReneWerner87 We could do this by adding Open() as part of the storage interface. If it sounds good to you I can do the PR for all the drivers

Yes, sounds good

Sounds good

@gaby gaby self-assigned this Sep 3, 2023
@gaby
Copy link
Member

gaby commented Sep 8, 2023

@mirusky Planning PR for this weekend

@gaby
Copy link
Member

gaby commented Sep 10, 2023

@mirusky @ReneWerner87 i'm adding this as an alternative to New() when creating the storage, does that make sense?

Example

conn := UniversalClient(...)
store = redis.Open(conn)

At that point store can be used as if it was created using New()

@ReneWerner87
Copy link
Member

Yes makes sense

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

No branches or pull requests

3 participants