Implement reject conflict strategy #88
Conversation
There was a problem hiding this comment.
Pull Request Overview
This PR implements a new conflict strategy (:reject) for ActiveJob uniqueness, allowing jobs to be silently skipped when a conflict occurs. Key changes include:
- Adding test contexts to verify that using on_conflict: :reject does not raise errors or log skipped jobs.
- Updating the base strategy and configuration files to accept the :reject value.
- Extending test coverage across multiple contexts for different strategies.
Reviewed Changes
Copilot reviewed 6 out of 6 changed files in this pull request and generated 6 comments.
Show a summary per file
| File | Description |
|---|---|
| spec/support/shared_examples/enqueuing.rb | Added tests for on_conflict: :reject behavior |
| spec/active_job/uniqueness/strategies/while_executing_spec.rb | Added tests verifying no exceptions or logging with :reject |
| spec/active_job/uniqueness/strategies/until_and_while_executing_spec.rb | Added tests verifying no exceptions or logging with :reject |
| spec/active_job/uniqueness/active_job_patch/unique_spec.rb | Added tests to assert correct lock options with :reject |
| lib/active_job/uniqueness/strategies/base.rb | Updated conflict handling to support :reject |
| lib/active_job/uniqueness/configuration.rb | Updated on_conflict validation to accept :reject |
| context 'when on_conflict: :reject given' do | ||
| before { job_class.unique strategy, on_conflict: :reject } | ||
|
|
||
| it 'not raises ActiveJob::Uniqueness::JobNotUnique' do |
There was a problem hiding this comment.
[nitpick] Consider rephrasing the test description to 'does not raise ActiveJob::Uniqueness::JobNotUnique' for improved clarity.
| it 'not raises ActiveJob::Uniqueness::JobNotUnique' do | |
| it 'does not raise ActiveJob::Uniqueness::JobNotUnique' do |
| expect { subject }.not_to raise_error | ||
| end | ||
|
|
||
| it 'not logs the skipped job' do |
There was a problem hiding this comment.
[nitpick] Consider rephrasing the test description to 'does not log the skipped job' for improved clarity.
| it 'not logs the skipped job' do | |
| it 'does not log the skipped job' do |
|
|
||
| include_examples 'of a not unique job processing' | ||
|
|
||
| it 'not raises ActiveJob::Uniqueness::JobNotUnique' do |
There was a problem hiding this comment.
[nitpick] Consider rephrasing to 'does not raise ActiveJob::Uniqueness::JobNotUnique' to improve readability.
| it 'not raises ActiveJob::Uniqueness::JobNotUnique' do | |
| it 'does not raise ActiveJob::Uniqueness::JobNotUnique' do |
| expect { process }.not_to raise_error | ||
| end | ||
|
|
||
| it 'not logs the skipped job' do |
There was a problem hiding this comment.
[nitpick] Consider rephrasing to 'does not log the skipped job' to improve readability.
| it 'not logs the skipped job' do | |
| it 'does not log the skipped job' do |
|
|
||
| include_examples 'of a not unique job processing' | ||
|
|
||
| it 'not raises ActiveJob::Uniqueness::JobNotUnique' do |
There was a problem hiding this comment.
[nitpick] Consider rephrasing to 'does not raise ActiveJob::Uniqueness::JobNotUnique' for clarity.
| it 'not raises ActiveJob::Uniqueness::JobNotUnique' do | |
| it 'does not raise ActiveJob::Uniqueness::JobNotUnique' do |
| expect { process }.not_to raise_error | ||
| end | ||
|
|
||
| it 'not logs the skipped job' do |
There was a problem hiding this comment.
[nitpick] Consider rephrasing to 'does not log the skipped job' for clarity.
| it 'not logs the skipped job' do | |
| it 'does not log the skipped job' do |
DmitryTsepelev
force-pushed
the
on-conflict-reject
branch
from
d4a4309 to
4e22630
Compare
DmitryTsepelev
force-pushed
the
on-conflict-reject
branch
from
4e22630 to
5a55e3b
Compare
There was a problem hiding this comment.
Pull Request Overview
Copilot reviewed 7 out of 7 changed files in this pull request and generated no new comments.
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
2 participants
That's a pretty dumb strategy to implement, but I feel like writing
unique :until_executed, on_conflict: :rejectis a bit more readable thanunique :until_executed, on_conflict: ->(_) {}in cases, when I know that there will be a ton of conflicts, I expect that and do not want to be notified