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

Add DBTX Param config option #1279

Merged
merged 6 commits into from
Nov 20, 2021
Merged

Add DBTX Param config option #1279

merged 6 commits into from
Nov 20, 2021

Conversation

danielmmetz
Copy link
Contributor

Problem

I'd like to be able to define a type that wraps a small interface along the lines of

type WidgetInserter interface {
    InsertWidget(ctx context.Context, db DBTX, name string) error
}

Using sqlc to generate the underlying code today would give a *Queries that has these two relevant methods:

InsertWidget(ctx context.Context, name string) error
WithTx(tx *sql.Tx) *Queries

Because *Queries operates against an internal DBTX, to use InsertWidget against a pre-existing transaction then requires creating a new *Queries by use of its WithTx method. But because WithTx returns *Queries, it's non-trivial to keep this as just an implementation detail—to have an interface that doesn't expose the underlying *Queries requires either:

  • manually adding variant implementations of WithTx that return the desired interface (which also requires now that the interface be accessible to the generated package, instead of being defined potentially only at the consumer site), or
  • requiring that the interfaces being used compose the entire generated query set (i.e. the generated Querier instead of just one of its methods)

Some of the why for both of these is covered in #383 and golang/go#7512.

Proposal

Support a new configuration parameter dbtx_param that modifies the generated method sets to provide methods like shown in WidgetInserter. If set to true, the generated *Queries omits storing a DBTX as a struct field and requires it be passed in to all method calls. In doing so, it allows callers to easily provide the connection for standalone use or for use as part of a broader transaction and makes it easy for the surrounding code to use a narrowly defined interface.

Notes

This PR's current implementation treats emit_prepared_queries and dbtx_param as mutually exclusive config options. The thought being that since prepared queries are prepared against a particular database connection, and because executing the queries would now use a caller-provided connection, these two connections may end up distinct and void the value of preparing the queries. The implementation can be changed to support two the config options together, it'd just be more involved to do so.

Copy link
Collaborator

@kyleconroy kyleconroy left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Instead of adding an test case in the examples, can you create one in the internal/endtoend/testdata directory?

I also think we can improve the configuration option name. Instead of dbtx_param, I'd pick emit_methods_with_db_argument. More wordy, but more clear. Thoughts?

@danielmmetz
Copy link
Contributor Author

Instead of adding an test case in the examples, can you create one in the internal/endtoend/testdata directory?

I also think we can improve the configuration option name. Instead of dbtx_param, I'd pick emit_methods_with_db_argument. More wordy, but more clear. Thoughts?

Appreciate the feedback, thanks. I agree that name is more clear (and more consistent). I'll push up another commit accordingly.

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

Successfully merging this pull request may close these issues.

None yet

2 participants