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

database/sql: add Queryer interface #14674

Closed
james-lawrence opened this issue Mar 6, 2016 · 16 comments
Closed

database/sql: add Queryer interface #14674

james-lawrence opened this issue Mar 6, 2016 · 16 comments
Assignees
Milestone

Comments

@james-lawrence
Copy link
Contributor

@james-lawrence james-lawrence commented Mar 6, 2016

seems like 90% of the time I'm interacting with a database most of my DB code doesn't care if i'm within a TX or not. often there are queries I'm executing in various places with different contexts, some of which require a transaction, some which don't. But I can't share the code without writing my own Queryer style interface. now granted its only a few lines, but it seems like this is something that should just be in the sql package itself.

I'm happy to do the work, submit the PR etc. just curious if there is any object to adding it and why.

@bradfitz bradfitz changed the title database/sql - add Queryer interface database/sql: add Queryer interface Apr 10, 2016
@bradfitz
Copy link
Contributor

@bradfitz bradfitz commented Apr 10, 2016

In Go, interfaces are defined only when they're accepted, not when they're provided. You can define your own Querier interface if you need one, no?

@bradfitz bradfitz closed this Apr 10, 2016
@james-lawrence
Copy link
Contributor Author

@james-lawrence james-lawrence commented Apr 10, 2016

correct but seems like everyone defines it seems kind of silly. but fair point.

@bradfitz
Copy link
Contributor

@bradfitz bradfitz commented Apr 10, 2016

Does everybody define it? Got examples? If there's evidence that it's widely used, we can add it.

@james-lawrence
Copy link
Contributor Author

@james-lawrence james-lawrence commented Apr 10, 2016

off the top of my head:

  1. https://github.com/Masterminds/squirrel/blob/master/squirrel.go#L31
  2. https://github.com/jmoiron/sqlx/blob/master/sqlx.go#L75
  3. https://github.com/knq/xo - generates it for you.
// XODB is the common interface for database operations that can be used with
// types from public.
//
// This should work with database/sql.DB and database/sql.Tx.
type XODB interface {
    Exec(string, ...interface{}) (sql.Result, error)
    Query(string, ...interface{}) (*sql.Rows, error)
    QueryRow(string, ...interface{}) *sql.Row
}
  1. https://bitbucket.org/jatone/genieql (my project) would love to have it available in the runtime, if it isn't added here, I'll just make it something genieql will auto generate the code for people. similar to what xo is doing.
  2. https://github.com/gocraft/dbr/blob/master/dbr.go#L85
  3. https://github.com/doug-martin/goqu/blob/master/database.go#L12

I'm sure I could dig up more examples potentially. squirrel has a few forks that most likely also use them.
Basically in some form all of these libraries are abstracting out those common methods between TX and DB.

@bradfitz
Copy link
Contributor

@bradfitz bradfitz commented Apr 10, 2016

All those interfaces look different, though. It doesn't look like there's necessarily consensus.

I think it's fine if people writing funcs like QueryRowWith (https://github.com/Masterminds/squirrel/blob/master/squirrel.go#L110) define the exact interface they want.

@james-lawrence
Copy link
Contributor Author

@james-lawrence james-lawrence commented Apr 10, 2016

I disagree, almost all of them roll up to allow you to use TX and DB interchangably when executing a query, I think the differences are superficial. but as I said pretty indifferent on whether its included. just thought I'd point it out.

@bradfitz
Copy link
Contributor

@bradfitz bradfitz commented Apr 10, 2016

Well, we have testing.TB already, so there's precedent for something like this. I'm less worried if we define it like that and rather than have N small interfaces, we have 1 larger interface which is the intersection of all TX and DB methods, plus a private method so nobody else can implement that type, preventing types other than TX or DB (like we did with testing.TB).

@bradfitz bradfitz reopened this Apr 10, 2016
@bradfitz bradfitz self-assigned this Apr 10, 2016
@bradfitz bradfitz added this to the Go1.7 milestone Apr 10, 2016
@danilobuerger
Copy link

@danilobuerger danilobuerger commented Apr 10, 2016

A case i frequently run into is if i want to use DB and TX (created from that DB) interchangeably on prepared statements:

stmt, err := db.Prepare("SELECT ...")

If i want to use that with a TX from db.Begin, i will have to pass it through tx.Stmt. It would be super nice if such an interface would include Stmt(stmt *Stmt) *Stmt so that something like this becomes possible:

func Foo(q Querier) {
    q.Stmt(stmt).Query()
}
@james-lawrence
Copy link
Contributor Author

@james-lawrence james-lawrence commented Apr 10, 2016

@danilobuerger Stmt would likely satisify the querier interface.

@bradfitz
Copy link
Contributor

@bradfitz bradfitz commented Apr 10, 2016

@james-lawrence, Stmt doesn't take the query. It only takes args ...interface{}.

@danilobuerger, there is only one thing in the database/sql package with a Stmt(stmt *Stmt) *Stmt method. Why would it go into an interface?

@james-lawrence
Copy link
Contributor Author

@james-lawrence james-lawrence commented Apr 10, 2016

crap missed that part =)

@bradfitz
Copy link
Contributor

@bradfitz bradfitz commented Apr 10, 2016

The fact that Stmt is different from DB and Tx gives me pause. If we add an interface which is the intersection of DB and Tx, it seems to discourage use of prepared statements, if things in the ecosystem start requiring a Querier interface.

It almost makes me think the interface should be of Stmt's signature instead, but then we'd need something like a WithQuery(query string) QueryExecer method on DB and Tx instead. (at which point the interface is named QueryExecer instead of Querier because Exec is also in the intersection of methods).

Anybody want to try that route and send a CL?

@bradfitz bradfitz removed their assignment Apr 10, 2016
@bradfitz bradfitz modified the milestones: Go1.7, Unplanned Apr 10, 2016
@danilobuerger
Copy link

@danilobuerger danilobuerger commented Apr 10, 2016

@bradfitz sorry, i didn't add that: Stmt should be on DB as well in that case. Otherwise it would be cumbersome to use:

func Foo(q Querier) {
    s := stmt
    if tx, ok := q.(*sql.Tx); ok {
        s = tx.Stmt(s)
    }
    q.Stmt(s).Query()
}
@james-lawrence
Copy link
Contributor Author

@james-lawrence james-lawrence commented Apr 10, 2016

@bradfitz I'll be happy to take a stab at it and see how it works in practice.

@kardianos
Copy link
Contributor

@kardianos kardianos commented Oct 8, 2016

We should be careful here. We just defined a bunch of new Context methods and there is debate if Tx should have a nesting Tx or RollbackTo and SavePoint methods. This would strongly flavor such an interface.

@kardianos
Copy link
Contributor

@kardianos kardianos commented Nov 28, 2016

This is a duplicate of #14468.

@kardianos kardianos closed this Nov 28, 2016
@golang golang locked and limited conversation to collaborators Nov 28, 2017
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Projects
None yet
Linked pull requests

Successfully merging a pull request may close this issue.

None yet
5 participants
You can’t perform that action at this time.