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

Allow River client to be created with nil database pool #30

Merged
merged 1 commit into from
Nov 17, 2023

Conversation

brandur
Copy link
Contributor

@brandur brandur commented Nov 15, 2023

I realized as I was working on test helpers and thinking about
transactions that having to send a River client a database pool in a
test suite would be very inconvenient. We make a database pool on a
test database relatively easily available in River (I say "relatively"
because the test suite still cannot support -p > 1), but for many
applications that won't be the case. At my work for example, sending
River a database pool in tests would create the risk of tainting the
test database which would be very undesirable.

Here, we add the capacity for a River client/driver to be initialized
with a nil database pool. It works like this:

  • Start is not available and errors if call.
  • The non-transactional variants of Insert/InsertMany are not
    available and error if called.
  • InsertTx/InsertManyTx will continue to work happily and insert
    jobs as they always would.

So when testing something like an API endpoint, users would create and
inject a test transaction, make sure to always use InsertTx and
InsertManyTx, then reuse the same test transaction with the test
helpers to verify the inserts. They assert only that the jobs are
inserted and not that the worker work is performed. That's enough
assuming that River works (and we hope it does) and will help keep their
test suite much faster because not every job needs to actually round
trip through its worker.

There could even be cases where this would be useful in prod. On the API
side you might make sure to initialize the client with a nil database
pool so that an accidental job insertion outside of a transaction (i.e.
accidental call to Insert instead of InsertTx) will fail, and
hopefully be caught in the test suite. If I was using River in a
project, that's how I'd do it.

@brandur brandur force-pushed the brandur-nil-pool-allowed branch from 44cdb68 to f2a7103 Compare November 15, 2023 03:18
@@ -67,6 +67,10 @@ jobs:
env:
TEST_DATABASE_URL: postgres://postgres:postgres@127.0.0.1:5432/river_testdb?sslmode=disable

- name: Test riverpgxv5
working-directory: ./riverdriver/riverpgxv5
run: go test -race ./...
Copy link
Contributor Author

Choose a reason for hiding this comment

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

TIL go test ./... will not run submodule tests and even go test ./riverdriver/riverpgxv5 will refuse to work.

@brandur brandur requested a review from bgentry November 15, 2023 03:21
@@ -433,7 +433,7 @@ func NewClient[TTx any](driver riverdriver.Driver[TTx], config *Config) (*Client
// There are a number of internal components that are only needed/desired if
// we're actually going to be working jobs (as opposed to just enqueueing
// them):
if config.willExecuteJobs() {
Copy link
Contributor

Choose a reason for hiding this comment

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

Should we also make it a config error to specify Queues with no pool? Queues implies there will be workers working on those queues, which doesn't make sense if those workers could never be enabled.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Hah, so that occurred to me as well, but I was 50/50 on the best option. The slight downside of erroring on NewClient is that it makes it slightly harder to share one test client between everything (e.g. in our suite, sending a nil database pool driver combined with a config from our general newTestConfig would be an error because the latter includes Queues.

Let's try it out though. Pushed.

client_test.go Outdated Show resolved Hide resolved
client_test.go Outdated Show resolved Hide resolved
I realized as I was working on test helpers and thinking about
transactions that having to send a River client a database pool in a
test suite would be _very_ inconvenient. We make a database pool on a
test database relatively easily available in River (I say "relatively"
because the test suite still cannot support `-p > 1`), but for many
applications that won't be the case. At my work for example, sending
River a database pool in tests would create the risk of tainting the
test database which would be very undesirable.

Here, we add the capacity for a River client/driver to be initialized
with a `nil` database pool. It works like this:

* `Start` is not available and errors if call.
* The non-transactional variants of `Insert`/`InsertMany` are not
  available and error if called.
* `InsertTx`/`InsertManyTx` will continue to work happily and insert
  jobs as they always would.

So when testing something like an API endpoint, users would create and
inject a test transaction, make sure to always use `InsertTx` and
`InsertManyTx`, then reuse the same test transaction with the test
helpers to verify the inserts. They assert only that the jobs are
inserted and not that the worker work is performed. That's enough
assuming that River works (and we hope it does) and will help keep their
test suite much faster because not every job needs to actually round
trip through its worker.

There could even be cases where this would be useful in prod. On the API
side you might make sure to initialize the client with a `nil` database
pool so that an accidental job insertion outside of a transaction (i.e.
accidental call to `Insert` instead of `InsertTx`) will fail, and
hopefully be caught in the test suite. If I was using River in a
project, that's how I'd do it.
@brandur brandur force-pushed the brandur-nil-pool-allowed branch from cfa52dc to d0bee0e Compare November 17, 2023 01:59
@brandur brandur merged commit 5e8b494 into master Nov 17, 2023
5 checks passed
@brandur brandur deleted the brandur-nil-pool-allowed branch November 17, 2023 02:01
brandur added a commit that referenced this pull request Nov 17, 2023
Add the changes in #29 and #30 to the changelog so that we can cut a
0.0.4 release. For brevity and to keep signal high, I didn't include
changes that came in which aren't user facing.
brandur added a commit that referenced this pull request Nov 18, 2023
Add the changes in #29 and #30 to the changelog so that we can cut a
0.0.4 release. For brevity and to keep signal high, I didn't include
changes that came in which aren't user facing.
brandur added a commit that referenced this pull request Nov 18, 2023
Add the changes in #29 and #30 to the changelog so that we can cut a
0.0.4 release. For brevity and to keep signal high, I didn't include
changes that came in which aren't user facing.
brandur added a commit that referenced this pull request Nov 18, 2023
Add the changes in #29 and #30 to the changelog so that we can cut a
0.0.4 release. For brevity and to keep signal high, I didn't include
changes that came in which aren't user facing.
brandur added a commit that referenced this pull request Nov 18, 2023
Add the changes in #29 and #30 to the changelog so that we can cut a
0.0.4 release. For brevity and to keep signal high, I didn't include
changes that came in which aren't user facing.
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.

2 participants