diff --git a/.github/workflows/ci.yaml b/.github/workflows/ci.yaml index d3e18626..78432bf7 100644 --- a/.github/workflows/ci.yaml +++ b/.github/workflows/ci.yaml @@ -218,7 +218,7 @@ jobs: name: lint runs-on: ubuntu-latest env: - GOLANGCI_LINT_VERSION: v1.64.4 + GOLANGCI_LINT_VERSION: v2.0.0 permissions: contents: read # allow read access to pull request. Use with `only-new-issues` option. @@ -234,7 +234,7 @@ jobs: uses: actions/checkout@v4 - name: Lint - uses: golangci/golangci-lint-action@v4 + uses: golangci/golangci-lint-action@v7 with: # golangci-lint needs to be run separately for every Go module, and # its GitHub Action doesn't provide any way to do that. Have it fetch diff --git a/.golangci.yaml b/.golangci.yaml index 1872697a..a5773f09 100644 --- a/.golangci.yaml +++ b/.golangci.yaml @@ -1,16 +1,7 @@ -issues: - exclude: - - 'Error return value of .(\w+\.Rollback(.*)). is not checked' +version: "2" linters: - presets: - - bugs - - comment - - format - - performance - - style - - test - - unused + default: all disable: # disabled, but which we should enable with discussion @@ -21,82 +12,109 @@ linters: - testpackage # requires tests in test packages like `river_test` # disabled because they're annoying/bad + - cyclop # screams into the void at "cyclomatic complexity" + - funlen # screams when functions are more than 60 lines long; what are we even doing here guys - interfacebloat # we do in fact want >10 methods on the Adapter interface or wherever we see fit. + - gocognit # yells that "cognitive complexity" is too high; why + - gocyclo # ANOTHER "cyclomatic complexity" checker (see also "cyclop" and "gocyclo") - godox # bans TODO statements; total non-starter at the moment - err113 # wants all errors to be defined as variables at the package level; quite obnoxious + - maintidx # ANOTHER ANOTHER "cyclomatic complexity" lint (see also "cyclop" and "gocyclo") - mnd # detects "magic numbers", which it defines as any number; annoying + - nestif # yells when if blocks are nested; what planet do these people come from? - ireturn # bans returning interfaces; questionable as is, but also buggy as hell; very, very annoying - lll # restricts maximum line length; annoying - nlreturn # requires a blank line before returns; annoying - wsl # a bunch of style/whitespace stuff; annoying -linters-settings: - depguard: - rules: - all: - files: ["$all"] - allow: - deny: - - desc: "Use `github.com/google/uuid` package for UUIDs instead." - pkg: "github.com/xtgo/uuid" - not-test: - files: ["!$test"] - deny: - - desc: "Don't use `dbadaptertest` package outside of test environments." - pkg: "github.com/riverqueue/river/internal/dbadaptertest" - - desc: "Don't use `riverinternaltest` package outside of test environments." - pkg: "github.com/riverqueue/river/internal/riverinternaltest" + settings: + depguard: + rules: + all: + files: ["$all"] + deny: + - desc: Use `github.com/google/uuid` package for UUIDs instead. + pkg: github.com/xtgo/uuid + not-test: + files: ["!$test"] + deny: + - desc: Don't use `dbadaptertest` package outside of test environments. + pkg: github.com/riverqueue/river/internal/dbadaptertest + - desc: Don't use `riverinternaltest` package outside of test environments. + pkg: github.com/riverqueue/river/internal/riverinternaltest - forbidigo: - forbid: - - msg: "Use `require` variants instead." - p: '^assert\.' - - msg: "Use `Func` suffix for function variables instead." - p: 'Fn\b' - - msg: "Use built-in `max` function instead." - p: '\bmath\.Max\b' - - msg: "Use built-in `min` function instead." - p: '\bmath\.Min\b' + forbidigo: + forbid: + - msg: Use `require` variants instead. + pattern: ^assert\. + - msg: Use `Func` suffix for function variables instead. + pattern: Fn\b + - msg: Use built-in `max` function instead. + pattern: \bmath\.Max\b + - msg: Use built-in `min` function instead. + pattern: \bmath\.Min\b - gci: - sections: - - Standard - - Default - - Prefix(github.com/riverqueue) + gomoddirectives: + replace-local: true - gomoddirectives: - replace-local: true + gosec: + excludes: + - G404 # use of non-crypto random; overly broad for our use case - gosec: - excludes: - - G404 # use of non-crypto random; overly broad for our use case + revive: + rules: + - name: unused-parameter + disabled: true - revive: - rules: - - name: unused-parameter - disabled: true + tagliatelle: + case: + rules: + json: snake - tagliatelle: - case: - rules: - json: snake + testifylint: + enable-all: true + disable: + - go-require + + varnamelen: + ignore-names: + - db + - eg + - f + - i + - id + - j + - mu + - r + - sb # common convention for string builder + - t + - tt # common convention for table tests + - tx + - w + - wg + + exclusions: + generated: lax + presets: + - comments + - common-false-positives + - legacy + - std-error-handling + rules: + - path: (.+)\.go$ + text: Error return value of .(\w+\.Rollback(.*)). is not checked - testifylint: - enable-all: true - disable: - - go-require +formatters: + enable: + - gci + - gofmt + - gofumpt + - goimports - varnamelen: - ignore-names: - - db - - eg - - f - - i - - id - - j - - mu - - sb # common convention for string builder - - t - - tt # common convention for table tests - - tx - - wg + settings: + gci: + sections: + - Standard + - Default + - Prefix(github.com/riverqueue) + - Prefix(riverqueue.com/riverpro) diff --git a/error_test.go b/error_test.go index a1f3ea3e..8a1533c7 100644 --- a/error_test.go +++ b/error_test.go @@ -31,7 +31,7 @@ func TestUnknownJobKindError_As(t *testing.T) { t.Parallel() var err *river.UnknownJobKindError - require.False(t, errors.As(errors.New("some other error"), &err)) + require.NotErrorAs(t, errors.New("some other error"), &err) }) } diff --git a/internal/dblist/db_list.go b/internal/dblist/db_list.go index e6bd6bab..9498a86d 100644 --- a/internal/dblist/db_list.go +++ b/internal/dblist/db_list.go @@ -99,10 +99,13 @@ func JobList(ctx context.Context, exec riverdriver.Executor, params *JobListPara for i, orderBy := range params.OrderBy { orderByBuilder.WriteString(orderBy.Expr) - if orderBy.Order == SortOrderAsc { + switch orderBy.Order { + case SortOrderAsc: orderByBuilder.WriteString(" ASC") - } else if orderBy.Order == SortOrderDesc { + case SortOrderDesc: orderByBuilder.WriteString(" DESC") + case SortOrderUnspecified: + return nil, errors.New("should not have gotten SortOrderUnspecified by this point before executing list (bug?)") } if i < len(params.OrderBy)-1 { orderByBuilder.WriteString(", ") diff --git a/internal/jobexecutor/job_executor_test.go b/internal/jobexecutor/job_executor_test.go index 694986d5..3c6e58f4 100644 --- a/internal/jobexecutor/job_executor_test.go +++ b/internal/jobexecutor/job_executor_test.go @@ -233,7 +233,7 @@ func TestJobExecutor_Execute(t *testing.T) { executor, bundle := setup(t) - now := executor.Archetype.Time.StubNowUTC(time.Now().UTC()) + now := executor.Time.StubNowUTC(time.Now().UTC()) workerErr := errors.New("job error") executor.WorkUnit = newWorkUnitFactoryWithCustomRetry(func() error { return workerErr }, nil).MakeUnit(bundle.jobRow) @@ -249,7 +249,7 @@ func TestJobExecutor_Execute(t *testing.T) { require.Equal(t, now.Truncate(1*time.Microsecond), job.Errors[0].At.Truncate(1*time.Microsecond)) require.Equal(t, 1, job.Errors[0].Attempt) require.Equal(t, "job error", job.Errors[0].Error) - require.Equal(t, "", job.Errors[0].Trace) + require.Empty(t, job.Errors[0].Trace) }) t.Run("ErrorAgainAfterRetry", func(t *testing.T) { @@ -361,7 +361,7 @@ func TestJobExecutor_Execute(t *testing.T) { require.WithinDuration(t, time.Now(), job.Errors[0].At, 2*time.Second) require.Equal(t, 1, job.Errors[0].Attempt) require.Equal(t, "JobCancelError: throw away this job", job.Errors[0].Error) - require.Equal(t, "", job.Errors[0].Trace) + require.Empty(t, job.Errors[0].Trace) }) t.Run("JobSnoozeErrorReschedulesJobAndDecrementsAttempt", func(t *testing.T) { @@ -721,7 +721,7 @@ func TestJobExecutor_Execute(t *testing.T) { require.Equal(t, 1, job.Errors[0].Attempt) require.Equal(t, "JobCancelError: job cancelled remotely", job.Errors[0].Error) require.Equal(t, rivertype.ErrJobCancelledRemotely.Error(), job.Errors[0].Error) - require.Equal(t, "", job.Errors[0].Trace) + require.Empty(t, job.Errors[0].Trace) }) t.Run("RemoteCancellationJobNotCancelledIfNoErrorReturned", func(t *testing.T) { diff --git a/job_list_params_test.go b/job_list_params_test.go index c5f7ce7f..acc5c758 100644 --- a/job_list_params_test.go +++ b/job_list_params_test.go @@ -25,8 +25,8 @@ func Test_JobListCursor_JobListCursorFromJob(t *testing.T) { cursor := JobListCursorFromJob(jobRow) require.Zero(t, cursor.id) require.Equal(t, jobRow, cursor.job) - require.Zero(t, cursor.kind) - require.Zero(t, cursor.queue) + require.Empty(t, cursor.kind) + require.Empty(t, cursor.queue) require.Zero(t, cursor.sortField) require.Zero(t, cursor.time) } @@ -163,7 +163,7 @@ func Test_JobListCursor_MarshalJSON(t *testing.T) { text, err := json.Marshal(cursor) require.NoError(t, err) - require.NotEqual(t, "", text) + require.NotEmpty(t, text) unmarshaledParams := &JobListCursor{} require.NoError(t, json.Unmarshal(text, unmarshaledParams)) diff --git a/producer.go b/producer.go index bec1657f..18665f97 100644 --- a/producer.go +++ b/producer.go @@ -525,10 +525,7 @@ func (p *producer) innerFetchLoop(workCtx context.Context, fetchResultCh chan pr func (p *producer) executorShutdownLoop() { // No more jobs will be fetched or executed. However, we must wait for all // in-progress jobs to complete. - for { - if len(p.activeJobs) == 0 { - break - } + for len(p.activeJobs) != 0 { result := <-p.jobResultCh p.removeActiveJob(result.ID) } diff --git a/rivershared/testfactory/test_factory.go b/rivershared/testfactory/test_factory.go index 568e24b6..2d6b0b6f 100644 --- a/rivershared/testfactory/test_factory.go +++ b/rivershared/testfactory/test_factory.go @@ -46,7 +46,7 @@ func Job(ctx context.Context, tb testing.TB, exec riverdriver.Executor, opts *Jo return job } -func Job_Build(tb testing.TB, opts *JobOpts) *riverdriver.JobInsertFullParams { //nolint:stylecheck +func Job_Build(tb testing.TB, opts *JobOpts) *riverdriver.JobInsertFullParams { tb.Helper() encodedArgs := opts.EncodedArgs diff --git a/rivertest/worker_test.go b/rivertest/worker_test.go index dded3f16..3a28b6a6 100644 --- a/rivertest/worker_test.go +++ b/rivertest/worker_test.go @@ -100,24 +100,24 @@ func TestWorker_Work(t *testing.T) { worker := river.WorkFunc(func(ctx context.Context, job *river.Job[testArgs]) error { require.Equal(t, testArgs{Value: "test"}, job.Args) - require.Equal(t, 1, job.JobRow.Attempt) - require.NotNil(t, job.JobRow.AttemptedAt) - require.WithinDuration(t, time.Now(), *job.JobRow.AttemptedAt, 5*time.Second) - require.Equal(t, []string{"rivertest-worker"}, job.JobRow.AttemptedBy) - require.WithinDuration(t, time.Now(), job.JobRow.CreatedAt, 5*time.Second) - require.JSONEq(t, `{"value": "test"}`, string(job.JobRow.EncodedArgs)) - require.Empty(t, job.JobRow.Errors) - require.Nil(t, job.JobRow.FinalizedAt) - require.Positive(t, job.JobRow.ID) - require.Equal(t, "rivertest_work_test", job.JobRow.Kind) - require.Equal(t, river.MaxAttemptsDefault, job.JobRow.MaxAttempts) - require.Equal(t, []byte(`{}`), job.JobRow.Metadata) - require.Equal(t, river.PriorityDefault, job.JobRow.Priority) - require.Equal(t, river.QueueDefault, job.JobRow.Queue) - require.WithinDuration(t, time.Now(), job.JobRow.ScheduledAt, 2*time.Second) - require.Equal(t, rivertype.JobStateRunning, job.JobRow.State) - require.Equal(t, []string{}, job.JobRow.Tags) - require.Nil(t, job.JobRow.UniqueKey) + require.Equal(t, 1, job.Attempt) + require.NotNil(t, job.AttemptedAt) + require.WithinDuration(t, time.Now(), *job.AttemptedAt, 5*time.Second) + require.Equal(t, []string{"rivertest-worker"}, job.AttemptedBy) + require.WithinDuration(t, time.Now(), job.CreatedAt, 5*time.Second) + require.JSONEq(t, `{"value": "test"}`, string(job.EncodedArgs)) + require.Empty(t, job.Errors) + require.Nil(t, job.FinalizedAt) + require.Positive(t, job.ID) + require.Equal(t, "rivertest_work_test", job.Kind) + require.Equal(t, river.MaxAttemptsDefault, job.MaxAttempts) + require.Equal(t, []byte(`{}`), job.Metadata) + require.Equal(t, river.PriorityDefault, job.Priority) + require.Equal(t, river.QueueDefault, job.Queue) + require.WithinDuration(t, time.Now(), job.ScheduledAt, 2*time.Second) + require.Equal(t, rivertype.JobStateRunning, job.State) + require.Equal(t, []string{}, job.Tags) + require.Nil(t, job.UniqueKey) _, hasContextKeyInsideTestWorker := ctx.Value(execution.ContextKeyInsideTestWorker{}).(bool) require.True(t, hasContextKeyInsideTestWorker) @@ -156,23 +156,23 @@ func TestWorker_Work(t *testing.T) { worker := river.WorkFunc(func(ctx context.Context, job *river.Job[testArgs]) error { require.Equal(t, testArgs{Value: "test3"}, job.Args) - require.Equal(t, 1, job.JobRow.Attempt) - require.NotNil(t, job.JobRow.AttemptedAt) - require.WithinDuration(t, time.Now().UTC(), *job.JobRow.AttemptedAt, 2*time.Second) - require.Equal(t, []string{"rivertest-worker"}, job.JobRow.AttemptedBy) - require.WithinDuration(t, time.Now().UTC(), job.JobRow.CreatedAt, 2*time.Second) - require.JSONEq(t, `{"value": "test3"}`, string(job.JobRow.EncodedArgs)) - require.Empty(t, job.JobRow.Errors) - require.Nil(t, job.JobRow.FinalizedAt) - require.Positive(t, job.JobRow.ID) - require.Equal(t, "rivertest_work_test", job.JobRow.Kind) - require.Equal(t, 420, job.JobRow.MaxAttempts) - require.JSONEq(t, `{"key": "value"}`, string(job.JobRow.Metadata)) - require.Equal(t, 3, job.JobRow.Priority) - require.Equal(t, "custom_queue", job.JobRow.Queue) - require.WithinDuration(t, hourFromNow, job.JobRow.ScheduledAt, 2*time.Second) - require.Equal(t, rivertype.JobStateRunning, job.JobRow.State) - require.Equal(t, []string{"tag1", "tag2"}, job.JobRow.Tags) + require.Equal(t, 1, job.Attempt) + require.NotNil(t, job.AttemptedAt) + require.WithinDuration(t, time.Now().UTC(), *job.AttemptedAt, 2*time.Second) + require.Equal(t, []string{"rivertest-worker"}, job.AttemptedBy) + require.WithinDuration(t, time.Now().UTC(), job.CreatedAt, 2*time.Second) + require.JSONEq(t, `{"value": "test3"}`, string(job.EncodedArgs)) + require.Empty(t, job.Errors) + require.Nil(t, job.FinalizedAt) + require.Positive(t, job.ID) + require.Equal(t, "rivertest_work_test", job.Kind) + require.Equal(t, 420, job.MaxAttempts) + require.JSONEq(t, `{"key": "value"}`, string(job.Metadata)) + require.Equal(t, 3, job.Priority) + require.Equal(t, "custom_queue", job.Queue) + require.WithinDuration(t, hourFromNow, job.ScheduledAt, 2*time.Second) + require.Equal(t, rivertype.JobStateRunning, job.State) + require.Equal(t, []string{"tag1", "tag2"}, job.Tags) return nil }) @@ -205,8 +205,8 @@ func TestWorker_Work(t *testing.T) { bundle.config.Test.Time = stubTime worker := river.WorkFunc(func(ctx context.Context, job *river.Job[testArgs]) error { - require.Empty(t, job.JobRow.UniqueKey) - require.Empty(t, job.JobRow.UniqueStates) + require.Empty(t, job.UniqueKey) + require.Empty(t, job.UniqueStates) return nil }) tw := NewWorker(t, bundle.driver, bundle.config, worker) @@ -258,9 +258,9 @@ func TestWorker_Work(t *testing.T) { bundle.config.Test.Time = timeStub worker := river.WorkFunc(func(ctx context.Context, job *river.Job[testArgs]) error { - require.WithinDuration(t, hourFromNow, *job.JobRow.AttemptedAt, time.Millisecond) - require.WithinDuration(t, hourFromNow, job.JobRow.CreatedAt, time.Millisecond) - require.WithinDuration(t, hourFromNow, job.JobRow.ScheduledAt, time.Millisecond) + require.WithinDuration(t, hourFromNow, *job.AttemptedAt, time.Millisecond) + require.WithinDuration(t, hourFromNow, job.CreatedAt, time.Millisecond) + require.WithinDuration(t, hourFromNow, job.ScheduledAt, time.Millisecond) return nil }) tw := NewWorker(t, bundle.driver, bundle.config, worker) @@ -373,8 +373,8 @@ func TestWorker_WorkJob(t *testing.T) { testWorker, bundle := setup(t) bundle.workFunc = func(ctx context.Context, job *river.Job[testArgs]) error { - require.WithinDuration(t, time.Now(), *job.JobRow.AttemptedAt, 5*time.Second) - require.Equal(t, []string{"rivertest-workjob"}, job.JobRow.AttemptedBy) + require.WithinDuration(t, time.Now(), *job.AttemptedAt, 5*time.Second) + require.Equal(t, []string{"rivertest-workjob"}, job.AttemptedBy) require.Equal(t, rivertype.JobStateRunning, job.State) return nil } diff --git a/rivertype/river_type.go b/rivertype/river_type.go index ee416c04..d1bbda48 100644 --- a/rivertype/river_type.go +++ b/rivertype/river_type.go @@ -300,6 +300,8 @@ type JobInsertParams struct { // List of hook interfaces that may be implemented: // - HookInsertBegin // - HookWorkBegin +// +// More operation-specific interfaces may be added in future versions. type Hook interface { // 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 @@ -360,6 +362,8 @@ type HookWorkBegin interface { // List of middleware interfaces that may be implemented: // - JobInsertMiddleware // - WorkerMiddleware +// +// More operation-specific interfaces may be added in future versions. type Middleware interface { // IsMiddleware is a sentinel function to check that a type is implementing // Middleware on purpose and not by accident (Middleware would otherwise be