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

WithTx is not part of the method set of interface generated #383

Open
dharmjit opened this issue Mar 6, 2020 · 10 comments · May be fixed by #3125
Open

WithTx is not part of the method set of interface generated #383

dharmjit opened this issue Mar 6, 2020 · 10 comments · May be fixed by #3125
Labels
bug Something isn't working 🔧 golang

Comments

@dharmjit
Copy link

dharmjit commented Mar 6, 2020

Hi, I have interface embedding where my db interface is embedding sqlc generated interface. As db interface is then injected to service layer, I am not able to use this. Is it possible to include WithTX merhod in emitted interface also.

@kyleconroy kyleconroy added the bug Something isn't working label Mar 7, 2020
@kyleconroy
Copy link
Collaborator

Is it possible to include WithTX method in emitted interface also.

I actually don't think it's possible to include the WithTx method. The reason is that WithTx currently returns a concrete type, *Queries. I think we'd need to change the signature of WithTx to return a Querier and then always emit the interface. I'm not sure it's worth a breaking change.

There may be a better way to do this. I'll need to think about it a bit more.

golang/go#7512

@dharmjit
Copy link
Author

dharmjit commented Mar 7, 2020

I guess most project which will adoptsqlc, will try to keep flexibility for some custom methods which will not come under code genration. Then interface embedding will be required.

@dharmjit
Copy link
Author

Hi @kyleconroy , what do you suggest meanwhile to handle transactions.

@earthboundkid
Copy link
Contributor

I have a similar problem. I wanted to add a BeginTx call to my app, but I was using the interface everywhere, so I would have to tear all that out and replace it with a concrete type to do that.

I think it would make sense to add BeginTx(ctx context.Context, opts *db.TxOptions) (Querier, error) to the interface to simplify usage. If you're worried about backwards compatibility, it could be a config option that defaults to false.

@philipjscott
Copy link

philipjscott commented Apr 13, 2020

Can't you just create a wrapper interface and struct?

package wrapper

type Querier interface {
	db.Querier
	WithTx(tx *sqlx.Tx) Querier
}

type Queries struct {
	*db.Queries
}

func (q *Queries) WithTx(tx *sqlx.Tx) Querier {
	return &Queries{
		Queries: q.Queries.WithTx(tx.Tx),
	}
}

func NewQuerier(q *db.Queries) Querier {
	return &Queries{
		Queries: q,
	}
}

I haven't tested this out yet; I'll edit this post if there's any problems with this approach.

@danielmmetz
Copy link
Contributor

Hi there! I ran into related issues recently and put up #1279 to try to address them. I'd appreciate any thoughts and feedback you might have.

@danielbprice
Copy link
Contributor

danielbprice commented Aug 12, 2022

@ScottyFillups your solution has worked well for me. It seems nice to be able to fold the database connection inside the Querier, so that there's only one thing to pass around. I tried to implement what @carlmjohnson was asking for but it's complicated to get right, I think. So in order to be able to avoid passing sql.DB all over my code, I extended Scotty's solution in what is admittedly a clunky way:

 package wrapper

type Querier interface {
	db.Querier
	WithTx(tx *sqlx.Tx) Querier
        DB() *sql.DB
}

I store the DB in the Queries struct during initialization.

type Queries struct {
	*db.Queries
        db *sql.DB
}

The escape hatch of being able to get the DB() out when you need it in order to start a transaction is really helpful.

tx, err := querier.DB().BeginTx(ctx, nil)
...
defer tx.Rollback()
...
qtx := querier.WithTx(tx)

Minor Update: this worked well for me until I needed to mock this interface, and wound up calling into something which in turn called DB(). Something to consider.

@eberkund
Copy link

eberkund commented Jul 1, 2023

Minor Update: this worked well for me until I needed to mock this interface, and wound up calling into something which in turn called DB(). Something to consider.

What was the problem with that? I just implemented something similar but I don't see what the issue would be with mocking.

@PhakornKiong
Copy link

I've just ran into this problem as well, appreciate if the interface for querier could be fixed so we can rely on interface in declaration rather than concrete queries

@Jesse0Michael
Copy link

I'm running into this too.. and the wrapper work around doesn't work for me.. am I doing something wrong?
Given

type Querier interface {
	db.Querier
	WithTx(tx *sql.Tx) Querier
}

I get the error

cannot use queries (variable of type *db.Queries) as Querier value in argument to NewProcessor: *db.Queries does not implement Querier (wrong type for method WithTx)
		have WithTx(*sql.Tx) *db.Queries
		want WithTx(*sql.Tx) Querier

If I instead use WithTx(*sql.Tx) *db.Queries in the interface, I run into the issue when i cant mock the interface. which is the whole reason I sought out emit_interface

@Jesse0Michael Jesse0Michael linked a pull request Jan 11, 2024 that will close this issue
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug Something isn't working 🔧 golang
Projects
None yet
Development

Successfully merging a pull request may close this issue.

9 participants