Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

[5.x] Fix JobWatcher when processing deleted instance of SerializesModels #1539

Merged
merged 17 commits into from
Oct 29, 2024
3 changes: 2 additions & 1 deletion src/Watchers/JobWatcher.php
Original file line number Diff line number Diff line change
Expand Up @@ -4,6 +4,7 @@

use Illuminate\Bus\BatchRepository;
use Illuminate\Contracts\Encryption\Encrypter;
use Illuminate\Database\Eloquent\ModelNotFoundException;
use Illuminate\Queue\Events\JobFailed;
use Illuminate\Queue\Events\JobProcessed;
use Illuminate\Queue\Queue;
Expand Down Expand Up @@ -266,7 +267,7 @@ protected function getBatchId(array $data)
$properties = ExtractProperties::from($command);

return $properties['batchId'] ?? null;
} catch (\Exception|\Error) {
Copy link
Contributor Author

@hapidjus hapidjus Oct 26, 2024

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I understrand that catch (\Exception|\Error) is a bit greedy. But there are other ways that the unserialize can go wrong. For instance when in a multi tenancy context and the database connection has not been set yet. See this issue for instance archtechx/tenancy#1263

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

If there a use case to include other exceptions we need to see the actual use case and replication steps so we can add tests to cover that usage. Having the scope too wide add a huge risk to this PR getting rejected.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Alright, I'll see if I can make a minimal reproduction of it. Bur for now I'm happy to have this merged and make another PR later.

} catch (ModelNotFoundException $e) {
if (preg_match('/"batchId";s:\d+:"([^"]+)"/', $data['command'], $matches)) {
return $matches[1];
}
Expand Down
48 changes: 48 additions & 0 deletions tests/Watchers/JobWatcherTest.php
Original file line number Diff line number Diff line change
Expand Up @@ -7,14 +7,17 @@
use Illuminate\Contracts\Debug\ExceptionHandler;
use Illuminate\Contracts\Queue\ShouldQueue;
use Illuminate\Database\Schema\Blueprint;
use Illuminate\Foundation\Auth\User;
use Illuminate\Queue\Jobs\Job;
use Illuminate\Queue\QueueManager;
use Illuminate\Queue\SerializesModels;
use Illuminate\Support\Facades\Schema;
use Illuminate\Support\Str;
use Laravel\Telescope\EntryType;
use Laravel\Telescope\Tests\FeatureTestCase;
use Laravel\Telescope\Watchers\JobWatcher;
use Orchestra\Testbench\Attributes\WithMigration;
use Orchestra\Testbench\Factories\UserFactory;
use Throwable;

#[WithMigration('queue')]
Expand Down Expand Up @@ -124,6 +127,30 @@ public function test_it_handles_pushed_jobs()
$this->assertSame('default', $entry->content['queue']);
$this->assertSame(['framework' => 'Laravel'], $entry->content['data']);
}

public function test_job_can_handle_deleted_serialized_model()
{
$user = UserFactory::new()->create();

$this->app->get(Dispatcher::class)->dispatch(
new MockedDeleteUserJob($user)
);

$this->artisan('queue:work', [
'connection' => 'database',
'--once' => true,
])->run();

$entry = $this->loadTelescopeEntries()->first();

$this->assertSame(EntryType::JOB, $entry->type);
$this->assertSame('processed', $entry->content['status']);
$this->assertSame('database', $entry->content['connection']);
$this->assertSame(MockedDeleteUserJob::class, $entry->content['name']);
$this->assertSame('default', $entry->content['queue']);

$this->assertSame(sprintf('%s:%s', get_class($user), $user->getKey()), $entry->content['data']['user']);
}
}

class MockedBatchableJob implements ShouldQueue
Expand All @@ -145,6 +172,27 @@ public function handle()
}
}

class MockedDeleteUserJob implements ShouldQueue
{
use SerializesModels;

public $connection = 'database';

public $deleteWhenMissingModels = true;

public $user;

public function __construct(User $user)
{
$this->user = $user;
}

public function handle()
{
$this->user->delete();
}
}

class MyDatabaseJob implements ShouldQueue
{
public $connection = 'database';
Expand Down