laravel / framework

The Laravel Framework.
https://laravel.com
MIT License
31.82k stars 10.77k forks source link

ShouldBeUnique Does Not Universally Prevent Duplicates #51798

Open kellerjmrtn opened 3 weeks ago

kellerjmrtn commented 3 weeks ago

Laravel Version

10.48.12 (although I believe relevant code has not changed in 11.x)

PHP Version

8.1.28

Database Driver & Version

Redis 7.2.5 (although issue should exist with other cache drivers)

Description

When dispatching jobs, the ShouldBeUnique behavior is only respected when dispatching through the PendingDispatch class and the Illuminate\Console\Scheduling\Schedule class (the latter fix was added in #39302). However, it is not respected when using the Illuminate\Bus\Dispatcher class or individual queue drivers (Queue::push( ... ), etc). This behavior can be confusing because depending on how you queue the job, it's easily possible to queue duplicates, despite the job implementing ShouldBeUnique

I found a previous PR (#50381) that attempted to move the ShouldBeUnique check into the Dispatcher class. Although that still wouldn't guarantee uniqueness when using Queue::push(). I was also able to find some other issues which seem to indicate that people are generally expecting ShouldBeUnique to work universally:

There's also a mention in the original PR for the ShouldBeUnique feature mentioning how this could possibly be moved into the enqueueUsing() logic in the Queue class? This seems like it would probably fix all of the above issues

Further, the Dispatcher class and Queue enqueueUsing() method already check other "expressive" interfaces like ShouldQueue, ShouldQueueAfterCommit, etc, so it seems reasonable to also check ShouldBeUnique there

I would be willing to open a PR for this if desired. Was hesitant to do so as a similar PR (#50381) has already been closed

Steps To Reproduce

Create a job which implements ShouldBeUnique

<?php

namespace App\Jobs;

use Illuminate\Contracts\Queue\ShouldBeUnique;
use Illuminate\Contracts\Queue\ShouldQueue;
use Illuminate\Foundation\Bus\Dispatchable;

class UniqueJob implements ShouldQueue, ShouldBeUnique
{
    use Dispatchable;

    ...
}

Confirm that ShouldBeUnique works correctly when using PendingDispatch. In tinker

Queue::size(); // Returns 0
UniqueJob::dispatch();
Queue::size(); // Returns 1. May have to call this multiple times, presumably to trigger the `__destruct()` method in `PendingDispatch`, but it will queue the job eventually
UniqueJob::dispatch();
Queue::size(); // Still returns 1, as `ShouldBeUnique` prevents queuing

Confirm that using Queue::push(new UniqueJob()), app(Dispatcher::class)->dispatch(new UniqueJob()), Bus::dispatch(new UniqueJob()) do not work the same way

Queue::size(); // Returns 0
Queue::push(new UniqueJob());
Queue::size(); // Returns 1
Queue::push(new UniqueJob());
Queue::size(); // Returns 2. One would expect a "unique" job to not be queued again
github-actions[bot] commented 3 weeks ago

Thank you for reporting this issue!

As Laravel is an open source project, we rely on the community to help us diagnose and fix issues as it is not possible to research and fix every issue reported to us via GitHub.

If possible, please make a pull request fixing the issue you have described, along with corresponding tests. All pull requests are promptly reviewed by the Laravel team.

Thank you!