Correct Way to Enforce Uniqueness on Insertion By Kind and Default States

[themaxgoldman]

I think I might be missing something with regard to the best way to specify uniqueness by kind and the default list of states. The default value for ExcludeKind is false. If I want to insert a job where that just enforces all the default uniqueness constraints it would seem if I have to set the Unique Opts to something like

{
  ExcludeKind: false
}

This seems redundant, as this is already the default value. However, if I just provide an empty struct uniqueness won't be enforced at all. Is this the intended behavior? It would seem to be that InsertOpts should instead hold a *UniqueOpts and the presence of the object would determine whether uniqueness is enforced.

[brandur]

Yes, it's intended behavior — checking a struct is zero valued versus not is generally a bit more conventional in Go compared to check a pointer.

ExcludeKind: false won't work because false is the zero value for a boolean so there's no way in Go to check whether you set a false value explicitly versus got a default zero value.

Honestly, we've never gotten this request before. Usually when you're checking for uniqueness you're trying to do it on some other dimension beyond just the job's kind like period. As things stand, you might have to use something like ByArgs: true (if your args are stable), or set the list of default unique states:

ByState: []rivertype.JobState{rivertype.JobStateAvailable, rivertype.JobStateCompleted, rivertype.JobStatePending, rivertype.JobStateRunning, rivertype.JobStateRetryable, rivertype.JobStateScheduled}

I suppose one could argue that unique opts should have been made a pointer originally for this case, but at this point, that API isn't going to be changing, so another route will need to be found.

[bgentry]

We could potentially expose the default unique states list somehow to make this a little cleaner / less verbose / less error prone, but I think @brandur is correct that this is about the best we can do here. At least one unique opts setting needs to be non-empty for us to decide to enforce uniqueness.

Maybe a doc fix is in order for this too?

[themaxgoldman]

Okay, that's interesting. In the end I actually needed to modify the ByState list for my use case so this proved irrelevant. However, the existing documentation did make figuring exactly what to do here a little difficult. I think a simple addition to the documentation to explain this would be very helpful.

[brandur]

+1. Also going to try and reveal a default set of unique states via #707.