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

pgxpool.Pool / pgxpool.Conn minimal interfaces #644

Closed
ghost opened this issue Dec 2, 2019 · 4 comments
Closed

pgxpool.Pool / pgxpool.Conn minimal interfaces #644

ghost opened this issue Dec 2, 2019 · 4 comments

Comments

@ghost
Copy link

ghost commented Dec 2, 2019

Would it be useful to have minimal interfaces for pgxpool.Pool / pgxpool.Conn for mocking/testing?

@jackc
Copy link
Owner

jackc commented Dec 2, 2019

It is very useful. I typically implement my DB code in terms of the following interface:

type dbconn interface {
	Begin(ctx context.Context) (pgx.Tx, error)
	Exec(ctx context.Context, sql string, arguments ...interface{}) (pgconn.CommandTag, error)
	Query(ctx context.Context, sql string, optionsAndArgs ...interface{}) (pgx.Rows, error)
	QueryRow(ctx context.Context, sql string, optionsAndArgs ...interface{}) pgx.Row
}

It's also useful because the application code will work with a pgxpool.Pool, pgxpool.Conn, pgx.Conn, or a pgx.Tx. The pgx.Tx is particularly useful for testing because that enables transactional tests where any changes can be rolled back at the end of the test.

All that said, as a general rule this type of interface should not be implemented in the library itself. In general a library should only expose interfaces that it consumes -- i.e. return structs and accept interfaces. Unfortunately, implementation concerns compelled violating this rule for pgx.Tx and pgx.Rows but I try to follow the rule when possible.

@ghost
Copy link
Author

ghost commented Dec 4, 2019

wouldn't be easier to have that dbconn as interface on your library which then will be used by anyone who wants to mock pgxpool.Pool? What benefit would I (and everyone else having to mock the library behaviour) get to rewrite the interface each time?

An example of where the interface has been provided and exposed from within the library itself is streadway/amqp#387

I apologies in advance if what I said above is incorrect - I might be wrong

@jackc
Copy link
Owner

jackc commented Dec 6, 2019

There's two downsides to including it in the library.

First, if any new methods are added to the common method set of pgxpool.Pool, pgx.Conn, and pgx.Tx it couldn't be added to this interface without breaking backwards compatibility. i.e. Adding methods to a public interface is a breaking change. It is possible to document particular interfaces as outside the comparability guarantee (like pgx.Rows) but I prefer to avoid this when possible.

The second is that this interface would be broader than most people would use. The true common method set for this interface is bigger than my sample dbconn interface. But the typical use case will only use a few methods in a given spot. If someone is testing with a mock DB they shouldn't have to implement methods that aren't being used. Requiring the user to define their own interface encourages them to define the interface with just the methods they need.

@ghost
Copy link
Author

ghost commented Dec 7, 2019

I see your points - what you are referring to is known as interface pollution and can be avoided by defining smaller interfaces which can be composed together
Mine was just a suggestion, if there is a strong argument against it I'll leave this with you. However thanks for your feedback!

@ghost ghost closed this as completed Dec 7, 2019
This issue was closed.
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

No branches or pull requests

1 participant