Fix UniqueOpts.ExcludeKind being silently ignored by isEmpty()

[JackDanger]

UniqueOpts.ExcludeKind is silently ignored on master. The public isEmpty() never looks at it — despite its own doc comment warning that every new option must be added there — while the copy of the same opts in internal/dbunique (dbunique.UniqueOpts.IsEmpty()) does count it. So UniqueOpts{ExcludeKind: true} inserts jobs with no unique key at all, and when a worker also carries unique opts, those silently override the caller's instead.

Two commits: first the failing test (TestUniqueOpts_isEmpty asserts the public and internal isEmpty agree for every field combination), then the fix — just the isEmpty() one-liner. Acceptance tests pin the insert-params path around it.

After review, the corner config {ExcludeKind: true} with nothing else set stays valid: it now applies uniqueness across all jobs (a whole-table key, since ByState defaults to UniqueOptsByStateDefault() when unset) instead of being silently ignored.

Reproduce on master with just the test commit:

go test . -run 'TestUniqueOpts_isEmpty' -count=1

(the acceptance tests in the branch stay green on master too — the twin-parity assertion is the one that fails.)

[brandur]

@JackDanger Thanks!!

Could you sign the CLA please?
https://github.com/riverqueue/rivercla

A small bug in the new validation is that this shape of UniqueOpts would become invalid:

UniqueOpts{
    ExcludeKind: true,
    ByState:     rivertype.UniqueOptsByStateDefault(),
}

We could modify your logic to also allow ByState, but that would ban this form of UniqueOpts, which is the same as the UniqueOpts above, and which should probably stay allowed:

UniqueOpts{ExcludeKind: true}

I wonder if maybe what we should do here is keep the isEmpty fix, but back out the new validation logic and just leave as is. Thoughts?

[brandur]

Thanks Jack!

@bgentry Can I get your opinion on this one as well? I suggested to Jack that we drop validation in favor of allowing UniqueOpts{ExcludeKind: true}. He made that change. I think it does make sense logically, but I suppose I could see it as a slight footgun in that it'd allow you to force all your jobs to share a single basically a single uniqueness key.

[JackDanger]

signed! riverqueue/rivercla#36

I'm looking for ways to make this a little more elegant and I appreciate the review and feedback 🙇

[brandur]

@JackDanger Thanks :)

Ugh, I hate to do this, but I was thinking about this more tonight, and I think what you had originally was probably better. Allowing UniqueOpts{ExcludeKind: true} just feels like too much an accident waiting to happen.

What do you think about resurrecting a validation like this?

if o.ExcludeKind && !o.ByArgs && !o.ByQueue && o.ByPeriod == 0 {
    return errors.New("UniqueOpts.ExcludeKind requires ByArgs, ByQueue, or ByPeriod")
}

And in case we ever get a complaint about it, we could reevaluate.

Sorry about the flip flop :/ I should have thought harder about this when I suggested the original change.

[JackDanger]

Thanks Brandur, addressed in 8e3ef79

[brandur]

Thanks!