jhaals / yopass

Secure sharing of secrets, passwords and files
https://yopass.se
Apache License 2.0
1.86k stars 290 forks source link

Accept interfaces, return structs #1247

Open zostay opened 2 years ago

zostay commented 2 years ago

The various Database methods currently return interfaces rather than structs, but Golang best practice indicates that returning structs is best. Consider the following situation where I want to wrap the database with a special Pinger in cases where I can use that to help determine the health of the connection. (Unrelated, I am having some weird connectivity issues with memcached and I'm working on trying to determine why using code similar to this.)

type Pingable interface {
    Ping() error
}

type MCPinger struct {
    *server.Memcached
}

func (p *MCPinger) Ping() error {
    return p.Memcached.Client.Ping()
}

If NewMemcached() returned *server.Memcached as convention suggests it should, the code to initialize the MCPinger would read:

var p Pingable = &Pinger{server.NewMemcached(connectString)}

However, right now this code would instead read something like this:

db := server.NewMemcached(connectString)
mdb, ok := db.(*server.Memcached)
if !ok {
    panic("NewMemcached() returns a different object than expected.")
}
var p Pingable = &Pinger{mdb}

Both the memcached and redis databases are implemented this way. The fix is just to change the return type of each New*() function to return the pointer to the struct it constructs rather than the interface.

zostay commented 2 years ago

(And yes I realize I could save myself the if !ok ... panic bit if I skipped the , ok in the type conversion, but in my real code I'm not panicking, but returning an error to be logged and then panicking some place higher up the call frame stack.)

jhaals commented 2 years ago

Hi @zostay! Yes I'd say you're right on that point and I think it make sense to update that. Would you be willing to contribute that suggestion?

I'm currently adding my spare cycles on a reimplementation of the service in typescript and nextjs to both cleanup and make it easier for users to customise as the current react app is not that easy to extend.