More abstracted middleware modeled after hooks

[brandur]

Here, try to reuse some of the concepts developed for client "hooks"
that came in with #789 for middleware. The general premise is to make
hooks and middleware as close to each other as possible and to improve
the DX/syntax of using middleware.

A current DX problem with the middleware design is that for any
middleware that'd like to insert itself for multiple operations an
instance of it needs to go into both JobInsertMiddleware and
WorkerMiddleware, which generally means that it's initialized outside
of a river.NewClient invocation and making the code kind of ugly.
Here's an example of thatfrom an OpenTelemetry middleware I'm working on:

middleware := riveropentelemetry.NewMiddleware(nil)

_, err := river.NewClient(riverpgxv5.New(nil), &river.Config{
    JobInsertMiddleware: []rivertype.JobInsertMiddleware{
        middleware,
    },
    WorkerMiddleware: []rivertype.WorkerMiddleware{
        middleware,
    },
})

Here, we re-integrate the two middleware stacks into one more abstracted
stack that enables a much more succinct injection:

_, err := river.NewClient(riverpgxv5.New(nil), &river.Config{
    Middleware: []rivertype.Middleware{
        riveropentelemetry.NewMiddleware(nil),
    },
})

Like with hooks, the middleware is internalized to a lookup struct so
that we precalculate what middleware belongs where and there's no
performance hit, if any.

The approach is a little less type safe in that it's possible to write a
trivial middleware that fulfills rivertype.Middleware, but accidentally
fails to implement a more useful middleware interface so that in the end
it doesn't actually run anything. We try to hedge against this by
suggesting that users write interface compliance checks like:

// Verify interface compliance. It's recommended that these are included in your
// test suite to make sure that your middlewares are complying to the specific
// interface middlewares that you expected them to be.
var (
    _ rivertype.JobInsertMiddleware = &JobInsertAndWorkMiddleware{}
    _ rivertype.WorkerMiddleware    = &JobInsertAndWorkMiddleware{}
    _ rivertype.JobInsertMiddleware = &JobInsertMiddleware{}
    _ rivertype.WorkerMiddleware    = &WorkerMiddleware{}
)

Also take the opportunity to add some new example tests and tighten up
overly loose or add missing documentation in a few places.

[bgentry]

Looks solid. The typing on these with the otherwise-pointless IsHook + IsMiddleware is a bummer but really unavoidable in Go as you detailed.

We should probably do an overhaul of docs to provide a lot more detail on these middleware implementations (with links to examples) and also add a similar page for hooks.

[bgentry]

feels like there's some missing words here like "is allowed" or "may be provided", or even just an initial prefix of "can"

[brandur]

I changed around the message a bit for clarity.

[bgentry]

At one point (during the extraction of the internal/jobexecutor package) I had an internal package of testworker which might be a useful home for these if they can be leveraged elsewhere. Of course you can't use them from package river tests because of the circular dependency due to the river.Job argument and river.WorkerDefaults, so maybe limited on where that would be useful.

[brandur]

Ah right. Yeah, I struggle a little knowing what the right thing to do for this type of thing is. A key is that the args/worker is dead simple, i.e. no callback functions or anything to complicate the example.

And even here, I'm not sure that common_test.go is the right home for it. It maybe should just be duplicated into each example's file so that a user reading it gets to see the entire code sample for easiest copy/pasta.

[brandur]

Looks solid. The typing on these with the otherwise-pointless IsHook + IsMiddleware is a bummer but really unavoidable in Go as you detailed.

Yeah :/

Actually one thing I wanted to run by you before we release anything is that something we could do is make these functions unexported like isHook. However, that means implementations have to live within the same package, so that would mean moving HookDefaults and MiddlewareDefaults into rivertype, along with special functions like rivertype.HookWorkBeginFunc. Honestly, I think this might be better, but it does make things a little inconsistent because river.WorkerDefaults would still be in river rather than rivertype.

Here's the rough diff:

diff --git a/rivertype/river_type.go b/rivertype/river_type.go
index ee416c0..249c872 100644
--- a/rivertype/river_type.go
+++ b/rivertype/river_type.go
@@ -301,11 +301,11 @@ type JobInsertParams struct {
 // - HookInsertBegin
 // - HookWorkBegin
 type Hook interface {
-	// IsHook is a sentinel function to check that a type is implementing Hook
+	// isHook is a sentinel function to check that a type is implementing Hook
 	// on purpose and not by accident (Hook would otherwise be an empty
 	// interface). Hooks should embed river.HookDefaults to pick up an
 	// implementation for this function automatically.
-	IsHook() bool
+	isHook() bool
 }
 
 // HookInsertBegin is an interface to a hook that runs before job insertion.
@@ -323,6 +323,33 @@ type HookWorkBegin interface {
 	WorkBegin(ctx context.Context, job *JobRow) error
 }
 
+// HookDefaults should be embedded on any hook implementation. It helps
+// guarantee forward compatibility in case additions are necessary to the Hook
+// interface.
+type HookDefaults struct{}
+
+func (d *HookDefaults) isHook() bool { return true }
+
+// HookInsertBeginFunc is a convenience helper for implementing HookInsertBegin
+// using a simple function instead of a struct.
+type HookInsertBeginFunc func(ctx context.Context, params *JobInsertParams) error
+
+func (f HookInsertBeginFunc) InsertBegin(ctx context.Context, params *JobInsertParams) error {
+	return f(ctx, params)
+}
+
+func (f HookInsertBeginFunc) isHook() bool { return true }
+
+// HookWorkBeginFunc is a convenience helper for implementing HookworkBegin
+// using a simple function instead of a struct.
+type HookWorkBeginFunc func(ctx context.Context, job *JobRow) error
+
+func (f HookWorkBeginFunc) WorkBegin(ctx context.Context, job *JobRow) error {
+	return f(ctx, job)
+}
+
+func (f HookWorkBeginFunc) isHook() bool { return true }
+
 // Middleware is an arbitrary interface for a struct which will execute some
 // arbitrary code at a predefined step in the job lifecycle.
 //
diff --git a/rivertype/river_type_test.go b/rivertype/river_type_test.go
index aa677ac..d1f43b7 100644
--- a/rivertype/river_type_test.go
+++ b/rivertype/river_type_test.go
@@ -1,6 +1,7 @@
 package rivertype_test
 
 import (
+	"context"
 	"go/ast"
 	"go/parser"
 	"go/token"
@@ -12,6 +13,15 @@ import (
 	"github.com/riverqueue/river/rivertype"
 )
 
+// Verify interface compliance.
+var (
+	_ rivertype.Hook            = rivertype.HookInsertBeginFunc(func(ctx context.Context, params *rivertype.JobInsertParams) error { return nil })
+	_ rivertype.HookInsertBegin = rivertype.HookInsertBeginFunc(func(ctx context.Context, params *rivertype.JobInsertParams) error { return nil })
+
+	_ rivertype.Hook          = rivertype.HookWorkBeginFunc(func(ctx context.Context, job *rivertype.JobRow) error { return nil })
+	_ rivertype.HookWorkBegin = rivertype.HookWorkBeginFunc(func(ctx context.Context, job *rivertype.JobRow) error { return nil })
+)
+
 func TestJobRow_Output(t *testing.T) {
 	t.Parallel()

We should probably do an overhaul of docs to provide a lot more detail on these middleware implementations (with links to examples) and also add a similar page for hooks.

Yeah I'll take this after the smoke's cleared a little bit here.

[brandur]

@bgentry Thanks man. Going to merge this for now, but lemme know what you think about the idea of potentially unexporting the "sentinel" functions (isHook/isMiddleware) and moving types around instead.