Skip to content

stronger test coverage around JobDeleteMany#1030

Merged
bgentry merged 1 commit into
masterfrom
bg-fix-job-delete-many-filters
Sep 8, 2025
Merged

stronger test coverage around JobDeleteMany#1030
bgentry merged 1 commit into
masterfrom
bg-fix-job-delete-many-filters

Conversation

@bgentry

@bgentry bgentry commented Sep 7, 2025

Copy link
Copy Markdown
Contributor

While researching a customer issue I wanted to ensure that we had strong coverage so we can be confident that jobs we expect to not be deleted are in fact not deleted.

Ensure that jobs we expect to _not_ be deleted are in fact not deleted.
@bgentry
bgentry requested a review from brandur September 7, 2025 18:23
@bgentry

bgentry commented Sep 7, 2025

Copy link
Copy Markdown
Contributor Author

@defany yeah, I think that's intentional here. There are currently fewer filtering options exposed for JobDeleteMany compared to listing. The standard structured fields like IDs get handled directly in a safer way without having to rely on more free-form where string injection.

The thing I found when researching this is that we had pretty strong coverage already on these methods, with the exception being that we often weren't testing to see if the "survivor" rows were in fact being left around after others were deleted. That coverage was added in this PR and so far I've been unable to expose any situations where it fails.

In your case, I assume you've already confirmed that you're not dealing with some kind of cross transaction visibility issues where some of the queries are in different txns and can't see each others' changes? That's really the only other idea I had.

@defany

defany commented Sep 7, 2025

Copy link
Copy Markdown

Heya! I think it's going to be easier to continue conversation here

So, I've found why tests are going normally and problem as well, so:

// You are making an copy of struct every time when user calls an method, so:
params := river.NewJobDeleteManyParams()
params.IDs(putResult.Job.ID)

_, err = client.JobDeleteMany(ctx, params)
if err != nil {
	panic(err)
}

In this case p.ids is going to be empty

// You are making an copy of struct every time when user calls an method, so:
params := river.NewJobDeleteManyParams()
params = params.IDs(putResult.Job.ID)

_, err = client.JobDeleteMany(ctx, params)
if err != nil {
	panic(err)
}

Everything is going fine

@defany

defany commented Sep 7, 2025

Copy link
Copy Markdown

The solution for me would be not copying struct every time because it is more expected behavior or write it in docs for the params (worse way because user may forget to read it and lose all of his jobs in the table)

Another way is to validate incoming params and if none of them is specified return an error about it with tip such as: didn't you forget to redeclare a variable while calling methods for filtering?

Heya! I think it's going to be easier to continue conversation here

So, I've found why tests are going normally and problem as well, so:

// You are making an copy of struct every time when user calls an method, so:
params := river.NewJobDeleteManyParams()
params.IDs(putResult.Job.ID)

_, err = client.JobDeleteMany(ctx, params)
if err != nil {
	panic(err)
}

In this case p.ids is going to be empty

// You are making an copy of struct every time when user calls an method, so:
params := river.NewJobDeleteManyParams()
params = params.IDs(putResult.Job.ID)

_, err = client.JobDeleteMany(ctx, params)
if err != nil {
	panic(err)
}

Everything is going fine

@bgentry
bgentry merged commit fc65bb4 into master Sep 8, 2025
11 checks passed
@bgentry
bgentry deleted the bg-fix-job-delete-many-filters branch September 8, 2025 00:46
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants