Merge branch '4.x-dev' into 4.x-dev

Author: aaajeetee

src/CloudTasksJob.php

@@ -69,24 +69,9 @@ public function getTaskName(): string
 
     public function delete(): void
     {
-        // Laravel automatically calls delete() after a job is processed successfully. However, this is
-        // not what we want to happen in Cloud Tasks because Cloud Tasks will also delete the task upon
-        // a 200 OK status, which means a task is deleted twice, possibly resulting in errors. So if the
-        // task was processed successfully (no errors or failures) then we will not delete the task
-        // manually and will let Cloud Tasks do it.
-        $successful =
-            // If the task has failed, we should be able to delete it permanently
-            $this->hasFailed() === false
-            // If the task has errored, it should be released, which in process deletes the errored task
-            && $this->hasError() === false;
-
-        if ($successful) {
-            return;
-        }
-
-        parent::delete();
-
-        $this->driver->delete($this);
+        // Laravel automatically calls delete() after a job is processed successfully.
+        // However, this is not what we want to happen in Cloud Tasks because Cloud Tasks
+        // will also delete the task upon a 200 OK status, which means a task is deleted twice.
     }
 
     public function hasError(): bool

src/CloudTasksQueue.php

@@ -16,11 +16,11 @@
 use Google\Protobuf\Timestamp;
 use Illuminate\Contracts\Queue\Queue as QueueContract;
 use Illuminate\Queue\Queue as LaravelQueue;
+use Illuminate\Support\Str;
 use Stackkit\LaravelGoogleCloudTasksQueue\Events\TaskCreated;
 
 use function Safe\json_decode;
 use function Safe\json_encode;
-use function Safe\preg_replace;
 
 class CloudTasksQueue extends LaravelQueue implements QueueContract
 {
@@ -136,7 +136,7 @@ protected function pushToCloudTasks($queue, $payload, $delay, mixed $job)
 
         $payload = (array) json_decode($payload, true);
 
-        $task = tap(new Task())->setName($this->taskName($queue, $payload));
+        $task = tap(new Task())->setName($this->taskName($queue, $payload['displayName']));
 
         $payload = $this->enrichPayloadWithInternalData(
             payload: $payload,
@@ -167,29 +167,19 @@ protected function pushToCloudTasks($queue, $payload, $delay, mixed $job)
         return $payload['uuid'];
     }
 
-    private function taskName(string $queueName, array $payload): string
+    private function taskName(string $queueName, string $displayName): string
     {
-        $displayName = $this->sanitizeTaskName($payload['displayName']);
-
         return CloudTasksClient::taskName(
             $this->config['project'],
             $this->config['location'],
             $queueName,
-            $displayName.'-'.bin2hex(random_bytes(8)),
+            str($displayName)
+                ->afterLast('\\')
+                ->prepend((string) Str::ulid(), '-')
+                ->toString(),
         );
     }
 
-    private function sanitizeTaskName(string $taskName): string
-    {
-        // Remove all characters that are not -, letters, numbers, or whitespace
-        $sanitizedName = preg_replace('![^-\pL\pN\s]+!u', '-', $taskName);
-
-        // Replace all separator characters and whitespace by a -
-        $sanitizedName = preg_replace('![-\s]+!u', '-', $sanitizedName);
-
-        return trim($sanitizedName, '-');
-    }
-
     private function enrichPayloadWithInternalData(
         array $payload,
         string $queueName,
@@ -256,13 +246,11 @@ public function delete(CloudTasksJob $job): void
 
     public function release(CloudTasksJob $job, int $delay = 0): void
     {
-        $job->delete();
-
-        $payload = $job->getRawBody();
-
-        $options = ['delay' => $delay, 'job' => $job];
-
-        $this->pushRaw($payload, $job->getQueue(), $options);
+        $this->pushRaw(
+            payload: $job->getRawBody(),
+            queue: $job->getQueue(),
+            options: ['delay' => $delay, 'job' => $job],
+        );
     }
 
     /** @param string|object $job */

src/IncomingTask.php

@@ -26,7 +26,7 @@ public static function fromJson(string $payload): self
         }
     }
 
-    public function isEmpty(): bool
+    public function isInvalid(): bool
     {
         return $this->task === [];
     }

src/TaskHandler.php

@@ -21,7 +21,7 @@ public function handle(?string $task = null): void
     {
         $task = IncomingTask::fromJson($task ?: request()->getContent());
 
-        if ($task->isEmpty()) {
+        if ($task->isInvalid()) {
             abort(422, 'Invalid task payload');
         }
 
@@ -33,7 +33,12 @@ public function handle(?string $task = null): void
 
         $this->config = is_array($config) ? $config : [];
 
-        $this->run($task);
+        // We want to catch any errors so we have more fine-grained control over
+        // how tasks are retried. Cloud Tasks will retry the task if a 5xx status
+        // is returned. Because we manually manage retries by releaseing jobs,
+        // we never want to return a 5xx status as that will result in duplicate
+        // job attempts.
+        rescue(fn () => $this->run($task), report: false);
     }
 
     private function run(IncomingTask $task): void

tests/QueueTest.php

@@ -14,6 +14,7 @@
 use Illuminate\Support\Facades\DB;
 use Illuminate\Support\Facades\Event;
 use Illuminate\Support\Facades\Queue;
+use Illuminate\Support\Str;
 use Override;
 use PHPUnit\Framework\Attributes\Test;
 use Stackkit\LaravelGoogleCloudTasksQueue\CloudTasksApi;
@@ -466,20 +467,18 @@ public function test_ignoring_jobs_with_deleted_models()
     }
 
     #[Test]
-    public function it_adds_a_task_name_based_on_the_display_name()
+    public function it_adds_a_pre_defined_task_name()
     {
         // Arrange
         CloudTasksApi::fake();
+        Str::createUlidsUsingSequence(['01HSR4V9QE2F4T0K8RBAYQ88KE']);
 
         // Act
         $this->dispatch((new SimpleJob()));
 
         // Assert
         CloudTasksApi::assertTaskCreated(function (Task $task): bool {
-            return str_starts_with(
-                $task->getName(),
-                'projects/my-test-project/locations/europe-west6/queues/barbequeue/tasks/Tests-Support-SimpleJob'
-            );
+            return $task->getName() === 'projects/my-test-project/locations/europe-west6/queues/barbequeue/tasks/01HSR4V9QE2F4T0K8RBAYQ88KE-SimpleJob';
         });
     }
 

tests/Support/FailingJob.php

@@ -12,6 +12,7 @@ class FailingJob extends BaseJob
 
     public function handle()
     {
+        event(new JobOutput('FailingJob:running'));
         throw new Error('simulating a failing job');
     }
 

tests/TaskHandlerTest.php

@@ -78,33 +78,31 @@ public function after_max_attempts_it_will_log_to_failed_table()
     }
 
     #[Test]
-    public function after_max_attempts_it_will_delete_the_task()
+    public function after_max_attempts_it_will_no_longer_execute_the_task()
     {
         // Arrange
+        Event::fake([JobOutput::class]);
         $job = $this->dispatch(new FailingJob());
 
         // Act & Assert
         $releasedJob = $job->runAndGetReleasedJob();
-        CloudTasksApi::assertDeletedTaskCount(1);
-        CloudTasksApi::assertTaskDeleted($job->task->getName());
+        Event::assertDispatched(JobOutput::class, 1);
         $this->assertDatabaseCount('failed_jobs', 0);
 
         $releasedJob = $releasedJob->runAndGetReleasedJob();
-        CloudTasksApi::assertDeletedTaskCount(2);
-        CloudTasksApi::assertTaskDeleted($job->task->getName());
+        Event::assertDispatched(JobOutput::class, 2);
         $this->assertDatabaseCount('failed_jobs', 0);
 
         $releasedJob->run();
-        CloudTasksApi::assertDeletedTaskCount(3);
-        CloudTasksApi::assertTaskDeleted($job->task->getName());
+        Event::assertDispatched(JobOutput::class, 4);
         $this->assertDatabaseCount('failed_jobs', 1);
     }
 
     #[Test]
     #[TestWith([['now' => '2020-01-01 00:00:00', 'try_at' => '2020-01-01 00:00:00', 'should_fail' => false]])]
     #[TestWith([['now' => '2020-01-01 00:00:00', 'try_at' => '2020-01-01 00:04:59', 'should_fail' => false]])]
     #[TestWith([['now' => '2020-01-01 00:00:00', 'try_at' => '2020-01-01 00:05:00', 'should_fail' => true]])]
-    public function after_max_retry_until_it_will_log_to_failed_table_and_delete_the_task(array $args)
+    public function after_max_retry_until_it_will_log_to_failed_table(array $args)
     {
         // Arrange
         $this->travelTo($args['now']);
@@ -115,8 +113,6 @@ public function after_max_retry_until_it_will_log_to_failed_table_and_delete_the
         $releasedJob = $job->runAndGetReleasedJob();
 
         // Assert
-        CloudTasksApi::assertDeletedTaskCount(1);
-        CloudTasksApi::assertTaskDeleted($job->task->getName());
         $this->assertDatabaseCount('failed_jobs', 0);
 
         // Act
@@ -130,6 +126,9 @@ public function after_max_retry_until_it_will_log_to_failed_table_and_delete_the
     #[Test]
     public function test_unlimited_max_attempts()
     {
+        // Assert
+        Event::fake(JobOutput::class);
+
         // Act
         $job = $this->dispatch(new FailingJobWithUnlimitedTries());
 
@@ -138,8 +137,7 @@ public function test_unlimited_max_attempts()
             $job = $job->runAndGetReleasedJob();
         }
 
-        // -1 because the last job is not run.
-        CloudTasksApi::assertDeletedTaskCount(51);
+        Event::assertDispatched(JobOutput::class, 51);
     }
 
     #[Test]
@@ -204,9 +202,7 @@ public function failing_jobs_are_released()
 
         $job->run();
 
-        CloudTasksApi::assertDeletedTaskCount(1);
         CloudTasksApi::assertCreatedTaskCount(2);
-        CloudTasksApi::assertTaskDeleted($job->task->getName());
         Event::assertDispatched(JobReleasedAfterException::class, function ($event) {
             return $event->job->attempts() === 1;
         });