Add "hooks" interface in addition to middleware

[brandur]

Here, try an experiment that adds "hooks", a middleware-like concept, but one
which differs in subtle ways, and if done right will unlock functionality that
middleware doesn't make possible, like job-specific actions like encryption.

While experimenting anyway, we also try a variation on middleware configuration
that tries to improve the experience of using hooks to address #788.

The middleware interface is very generic, containing just one sentinel function
so that it's distinct from interface{} (and which would make any type
accidentally settable to it):

type Hook interface {
    // IsAnyHook 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.
    IsAnyHook() bool
}

But from there, we get into specific hook interfaces that run at various phases
in a job's lifecycle. These ones should look familiar from middleware:

// HookInsertBegin is an interface to a hook that runs before job insertion.
type HookInsertBegin interface {
    Hook

    InsertBegin(ctx context.Context, params *JobInsertParams) error
}

// HookWorkBegin is an interface to a hook that runs after a job has been locked
// for work and before it's worked.
type HookWorkBegin interface {
    Hook

    WorkBegin(ctx context.Context, job *JobRow) error
}

The primary advantage of hooks is that they don't get layered onto the stack,
and especially when inserting rows, don't all need to share a single set of
hooks, letting each job in question run different hooks if desired.

By extension, this also means that unlike middleware, hooks cannot set anything
to context. This is the main practical distinction between the two concepts

By consolidating different phases into a single interface, we let logically
related hooks be configured much more cleanly. Whereas before we needed to
something like this to install a middleware to different lifecycle phases:

middleware := riverencrypt.NewEncryptMiddleware(riversecretbox.NewSecretboxEncryptor(key))

config := &river.Config{
    JobInsertMiddleware: []rivertype.JobInsertMiddleware{middleware},
    WorkerMiddleware:    []rivertype.WorkerMiddleware{middleware},
}

We now get to do this instead:

config := Config: river.Config{
    Hooks: []rivertype.Hook{
        riverencrypt.NewEncryptHook(riversecretbox.NewSecretboxEncryptor(key)),
    },

This is cleaner code-wise, but it also prevents accidental misuse where say an
encryptor was installed for insertion, but then forgotten for when jobs are
worked.

Similar to JobArgsWithInsertOpts, job args can implement JobArgsWithHooks
to add hooks for specific job kinds, and which will also tak effect for when
jobs are worked:

type JobArgsWithHooks interface {
    // Hooks returns specific hooks to run for this job type. These will run
    // after the global hooks configured on the client.
    //
    // Warning: Hooks returned should be based on the job type only and be
    // invariant of the specific contents of a job. Hooks are extract by
    // instantiating a generic instance of the job even when a specific instance
    // is available, so any conditional logic within will be ignored. This is
    // done because although specific job information may be available in some
    // hook contexts like on InsertBegin, it won't be in others like WorkBegin.
    Hooks() []rivertype.Hook
}

[brandur]

@bgentry Kind of expecting CI to crash and burn on this one, but wanted to get your general thoughts. I don't love having two separate concepts, but this would let us implement encryption (including job kind specific) quite cleanly, and it's lighter weight than middlewares since nothing gets added to the stack and might be suitable as an alternative in many cases anyway.

Also see #788, but found the middleware API fairly inconvenient to use, and I'm thinking that it may be that we could port some of the design here back to middleware as well to see if we could improve that.

[bgentry]

I definitely think this can work well as a pattern and it solves some different use cases than middleware, or at least solves some of them much better than middleware. I think it's worth pursuing for sure ✌️ Thanks for proofing it out.

[bgentry]

I did something similar with unique jobs for extracting river:"unique" struct field tags:

river/internal/dbunique/unique_fields.go

Lines 92 to 117 in 2b277cb

	// getSortedUniqueFieldsCached retrieves unique fields with caching to avoid
	// extracting fields from the same struct type repeatedly.
	func getSortedUniqueFieldsCached(args rivertype.JobArgs) ([]string, error) {
	typ := reflect.TypeOf(args)
	// Check cache first
	cacheMutex.RLock()
	if fields, ok := uniqueFieldsCache[typ]; ok {
	cacheMutex.RUnlock()
	return fields, nil
	}
	cacheMutex.RUnlock()
	// Not in cache; retrieve using reflection
	fields, err := getSortedUniqueFields(args)
	if err != nil {
	return nil, err
	}
	// Store in cache
	cacheMutex.Lock()
	uniqueFieldsCache[typ] = fields
	cacheMutex.Unlock()
	return fields, nil
	}

Obviously since these are things that aren't supposed to change at runtime you can do this once per arg type and cache that indefinitely.

[bgentry]

I'm not really vibing with the naming here. I assume the intention is to keep them sortable by event type (insert vs work) but the names feel unintuitive and not as descriptive as I'd hope. For example BeforeInsert feels obvious as to its timing (right before an insert), where as InsertBegin is not as obvious to me (it's not really the "beginning" of the operation").

IMO this is enough of a setback from the alternative names that it warrants breaking the convention of focusing on sort order & autocompletion as the top priority over legibility & intuitiveness.

[brandur]

Hm, dunno about BeforeInsert — not only does it sort badly, but IMO it breaks naming convention with the middleware's InsertMany. Same if we did BeforeWork alongside the existing Work.

Generally I really like the words "begin" and "end" because they organize so nicely, and this gets important once you start introducing a lot of things. e.g.:

• InsertBegin
• InsertEnd
• WorkBegin
• WorkEnd

Before/after would feel a tad more natural, but are strictly worse for this kind of thing, so IMO the slightly better name isn't worth it enough:

• InsertAfter
• InsertBefore
• WorkAfter
• WorkBefore

Or even worse, using convention like InsertBefore would actually order things randomly compared to how they actually run (order would be 3, 1, 4, 2):

• AfterInsert
• AfterWork
• BeforeInsert
• BeforeWork

I'm open to alternatives, but don't want to throw the baby out with the bathwater — especially when you're implementing a hook with multiple implementations, it's a really great feature if the functions on it sort naturally in the same order that they run. Elegant, organized by default, and avoids ambiguity around what good convention should look like.

So if we're going to change it, it'd be nice to find a pair of words with similar properties. e.g. enter/exit, start/stop. (I think begin/end are better than those ones, but that sort of thing.)

[brandur]

@bgentry Added a bunch of new tests for this one along with a memoization layer. As a follow up, I think I have some ideas on how to do something to help converge middleware a little more to improve the UX there somewhat too. Can I get your thoughts again on this one.

[bgentry]

Needs a changelog entry but otherwise lgtm ✌️

[bgentry]

Maybe this isn't worth being the one thing that pushes us to fully drop 1.22 support, but we're well within our "2 most recent Go versions" policy to support only 1.23+.

[brandur]

Yeah true. Didn't quite feel like I should mess with the Go version for this, but I'm sure something will end up bumping it intentionally or not pretty soon, so I'll take a look then.

[brandur]

ty man! Added changelog and addressed feedback.