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 Queryable interface #809

Open
wants to merge 1 commit into
base: master
Choose a base branch
from

Conversation

BlakeWilliams
Copy link

@BlakeWilliams BlakeWilliams commented May 9, 2022

I came across #344 and thought it may be useful to open a PR proposing this change since it looks like a few folks could find it useful.


When writing applications it's useful to pass a sqlx.DB and a sqlx.Tx
interchangeably so that you can compose functions that are runnable in
isolation or as part of a transaction.

This introduces the Queryable interface, which includes the common
exportable methods shared between sqlx.DB and sqlx.Tx so users of the
package don't have to implement it themselves.

This also adds tests that validate any new shared methods are
added to the interface.

When writing applications it's useful to pass a sqlx.DB and a sqlx.Tx
interchangeably so that you can compose functions that are runnable in
isolation or as part of a transaction.

This introduces the Queryable interface, which includes the common
exportable methods shared between sqlx.DB and sqlx.Tx so users of the
package don't have to implement it themselves.

This also adds tests that validate any new shared methods are
added to the interface.
@FikriRNurhidayat
Copy link

This is actually very helpful, even tho it hasn't being merged yet, but it does help me to implement Unit of Work

@misha
Copy link

misha commented Oct 31, 2022

Also wanted to add that this abstraction eliminated considerable code from my project. Would love for this to be part of and be maintained by the package. For now I'm using the proposed interface Queryable as is with zero problems.

@dlsniper dlsniper added the needs testing The PR needs more testing before being accepted label Feb 1, 2024
@1mi0
Copy link

1mi0 commented Feb 4, 2024

I think it would make more sense to split the giant interface into a bunch of smaller ones. I do completely agree that having the common methods to sqlx.Tx and sqlx.DB is really useful. But, I also think that a giant interface is hard to mock and deal with.

The Queryable interface carries a lot more functionality than is usually needed(in the use cases that I've seen). Instead, having interfaces like: Getter, NamedExecer, Selector, and their Context alternatives, makes more sense for small CRUD operations, where you also need to mock test them.

I think that composing Queryable out of smaller interfaces would also tie in better with the current style. There already are some interfaces like that.
https://github.com/BlakeWilliams/sqlx/blob/9c5a420018b739f4508c7b07f97f2b121e035f4d/sqlx.go#L244-L248

@dlsniper
Copy link
Collaborator

dlsniper commented Feb 5, 2024

I haven't had a chance to discuss this with @ardan-bkennedy; that's why the label is set to needs testing.

I think we don't need this interface in the package at all. This should be an application concern to define/use. That way, the application establishes the functionality it expects. We should ensure that both sqlx.Tx and sqlx.DB always have identical signatures for the shared methods to make it possible to define such interfaces. In this case, I suggest creating this interface but not exporting it.

@ardan-bkennedy
Copy link
Collaborator

This interface already exists and I use it today to substitute a DB for a Tx when needed.

sqlx.ExtContext

I haven't looked too closely to this issue, but why won't this interface work for you?

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
needs testing The PR needs more testing before being accepted
Projects
None yet
Development

Successfully merging this pull request may close these issues.

6 participants