From e22e15b3406db2acd9ec07e7a35e394a3ac330c1 Mon Sep 17 00:00:00 2001 From: Marick van Tuil Date: Sat, 27 Aug 2022 14:39:24 +0200 Subject: [PATCH 1/4] Fix job dispatched before commit with afterCommit --- src/CloudTasksQueue.php | 52 +++++++++++++++++++++++++++-------------- tests/QueueTest.php | 23 ++++++++++++++++++ 2 files changed, 58 insertions(+), 17 deletions(-) diff --git a/src/CloudTasksQueue.php b/src/CloudTasksQueue.php index cdcbcf4..d6ebe1e 100644 --- a/src/CloudTasksQueue.php +++ b/src/CloudTasksQueue.php @@ -52,9 +52,15 @@ public function size($queue = null) */ public function push($job, $data = '', $queue = null) { - $this->pushToCloudTasks($queue, $this->createPayload( - $job, $this->getQueue($queue), $data - )); + return $this->enqueueUsing( + $job, + $this->createPayload($job, $this->getQueue($queue), $data), + $queue, + null, + function ($payload, $queue) { + return $this->pushRaw($payload, $queue); + } + ); } /** @@ -63,11 +69,11 @@ public function push($job, $data = '', $queue = null) * @param string $payload * @param string|null $queue * @param array $options - * @return void + * @return string */ public function pushRaw($payload, $queue = null, array $options = []) { - $this->pushToCloudTasks($queue, $payload); + return $this->pushToCloudTasks($queue, $payload); } /** @@ -81,9 +87,15 @@ public function pushRaw($payload, $queue = null, array $options = []) */ public function later($delay, $job, $data = '', $queue = null) { - $this->pushToCloudTasks($queue, $this->createPayload( - $job, $this->getQueue($queue), $data - ), $delay); + return $this->enqueueUsing( + $job, + $this->createPayload($job, $this->getQueue($queue), $data), + $queue, + $delay, + function ($payload, $queue, $delay) { + return $this->pushToCloudTasks($queue, $payload, $delay); + } + ); } /** @@ -92,7 +104,7 @@ public function later($delay, $job, $data = '', $queue = null) * @param string|null $queue * @param string $payload * @param \DateTimeInterface|\DateInterval|int $delay - * @return void + * @return string */ protected function pushToCloudTasks($queue, $payload, $delay = 0) { @@ -103,12 +115,13 @@ protected function pushToCloudTasks($queue, $payload, $delay = 0) $httpRequest = $this->createHttpRequest(); $httpRequest->setUrl($this->getHandler()); $httpRequest->setHttpMethod(HttpMethod::POST); - $httpRequest->setBody( - // Laravel 7+ jobs have a uuid, but Laravel 6 doesn't have it. - // Since we are using and expecting the uuid in some places - // we will add it manually here if it's not present yet. - $this->withUuid($payload) - ); + + // Laravel 7+ jobs have a uuid, but Laravel 6 doesn't have it. + // Since we are using and expecting the uuid in some places + // we will add it manually here if it's not present yet. + [$payload, $uuid] = $this->withUuid($payload); + + $httpRequest->setBody($payload); $task = $this->createTask(); $task->setHttpRequest($httpRequest); @@ -128,9 +141,11 @@ protected function pushToCloudTasks($queue, $payload, $delay = 0) $createdTask = CloudTasksApi::createTask($queueName, $task); event((new TaskCreated)->queue($queue)->task($task)); + + return $uuid; } - private function withUuid(string $payload): string + private function withUuid(string $payload): array { /** @var array $decoded */ $decoded = json_decode($payload, true); @@ -139,7 +154,10 @@ private function withUuid(string $payload): string $decoded['uuid'] = (string) Str::uuid(); } - return json_encode($decoded); + return [ + json_encode($decoded), + $decoded['uuid'], + ]; } /** diff --git a/tests/QueueTest.php b/tests/QueueTest.php index cdd303f..265990d 100644 --- a/tests/QueueTest.php +++ b/tests/QueueTest.php @@ -6,6 +6,9 @@ use Google\Cloud\Tasks\V2\HttpMethod; use Google\Cloud\Tasks\V2\Task; +use Illuminate\Queue\Events\JobQueued; +use Illuminate\Support\Facades\DB; +use Illuminate\Support\Facades\Event; use Stackkit\LaravelGoogleCloudTasksQueue\CloudTasksApi; use Stackkit\LaravelGoogleCloudTasksQueue\TaskHandler; use Tests\Support\FailingJob; @@ -154,4 +157,24 @@ public function it_posts_the_task_the_correct_queue() && $queueName === 'projects/my-test-project/locations/europe-west6/queues/my-special-queue'; }); } + + /** + * @test + */ + public function it_can_dispatch_after_commit() + { + // Arrange + CloudTasksApi::fake(); + Event::fake(); + + // Act & Assert + Event::assertNotDispatched(JobQueued::class); + DB::beginTransaction(); + SimpleJob::dispatch()->afterCommit(); + Event::assertNotDispatched(JobQueued::class); + DB::commit(); + Event::assertDispatched(JobQueued::class, function (JobQueued $event) { + return $event->job instanceof SimpleJob; + }); + } } From 2a85fd44b681947f7f7c35211b63c3af4d418d89 Mon Sep 17 00:00:00 2001 From: Marick van Tuil Date: Fri, 2 Sep 2022 15:02:55 +0200 Subject: [PATCH 2/4] Add request validation to TaskHandler --- src/TaskHandler.php | 67 +++++++++++------ tests/CloudTasksDashboardTest.php | 2 +- tests/TaskHandlerTest.php | 119 +++++++++++++++++++++++++++++- tests/TestCase.php | 25 ++++--- 4 files changed, 178 insertions(+), 35 deletions(-) diff --git a/src/TaskHandler.php b/src/TaskHandler.php index 989679c..90c725b 100644 --- a/src/TaskHandler.php +++ b/src/TaskHandler.php @@ -9,6 +9,8 @@ use Illuminate\Queue\Jobs\Job; use Illuminate\Queue\WorkerOptions; use Illuminate\Support\Str; +use Illuminate\Validation\ValidationException; +use Safe\Exceptions\JsonException; use stdClass; use UnexpectedValueException; use function Safe\json_decode; @@ -40,9 +42,9 @@ public function __construct(CloudTasksClient $client) $this->client = $client; } - public function handle(?array $task = null): void + public function handle(?string $task = null): void { - $task = $task ?: $this->captureTask(); + $task = $this->captureTask($task); $this->loadQueueConnectionConfiguration($task); @@ -53,6 +55,47 @@ public function handle(?array $task = null): void $this->handleTask($task); } + /** + * @param string|array|null $task + * @return array + * @throws JsonException + */ + private function captureTask($task): array + { + $task = $task ?: (string) (request()->getContent()); + + try { + $array = json_decode($task, true); + } catch (JsonException $e) { + $array = []; + } + + $validator = validator([ + 'json' => $task, + 'task' => $array, + 'name_header' => request()->header('X-CloudTasks-Taskname'), + 'retry_count_header' => request()->header('X-CloudTasks-TaskRetryCount'), + ], [ + 'json' => 'required|json', + 'task' => 'required|array', + 'task.data' => 'required|array', + 'name_header' => 'required|string', + 'retry_count_header' => 'required|numeric', + ]); + + try { + $validator->validate(); + } catch (ValidationException $e) { + if (config('app.debug')) { + throw $e; + } else { + abort(404); + } + } + + return json_decode($task, true); + } + private function loadQueueConnectionConfiguration(array $task): void { /** @@ -71,26 +114,6 @@ private function setQueue(): void $this->queue = new CloudTasksQueue($this->config, $this->client); } - /** - * @throws CloudTasksException - */ - private function captureTask(): array - { - $input = (string) (request()->getContent()); - - if (!$input) { - throw new CloudTasksException('Could not read incoming task'); - } - - $task = json_decode($input, true); - - if (!is_array($task)) { - throw new CloudTasksException('Could not decode incoming task'); - } - - return $task; - } - private function handleTask(array $task): void { $job = new CloudTasksJob($task, $this->queue); diff --git a/tests/CloudTasksDashboardTest.php b/tests/CloudTasksDashboardTest.php index 8fbd306..aefcc9e 100644 --- a/tests/CloudTasksDashboardTest.php +++ b/tests/CloudTasksDashboardTest.php @@ -259,7 +259,7 @@ public function when_a_job_is_dispatched_it_will_be_added_to_the_dashboard() 'name' => SimpleJob::class, ]); $payload = \Safe\json_decode($task->getMetadata()['payload'], true); - $this->assertSame($payload, $job->payload); + $this->assertSame($payload, $job->payloadAsArray); } /** diff --git a/tests/TaskHandlerTest.php b/tests/TaskHandlerTest.php index 819fc3f..5743c28 100644 --- a/tests/TaskHandlerTest.php +++ b/tests/TaskHandlerTest.php @@ -9,11 +9,14 @@ use Illuminate\Queue\Events\JobProcessing; use Illuminate\Support\Facades\Event; use Illuminate\Support\Facades\Log; +use Illuminate\Validation\ValidationException; use Stackkit\LaravelGoogleCloudTasksQueue\CloudTasksApi; use Stackkit\LaravelGoogleCloudTasksQueue\CloudTasksException; use Stackkit\LaravelGoogleCloudTasksQueue\LogFake; use Stackkit\LaravelGoogleCloudTasksQueue\OpenIdVerificator; use Stackkit\LaravelGoogleCloudTasksQueue\StackkitCloudTask; +use Stackkit\LaravelGoogleCloudTasksQueue\TaskHandler; +use Symfony\Component\HttpKernel\Exception\NotFoundHttpException; use Tests\Support\EncryptedJob; use Tests\Support\FailingJob; use Tests\Support\SimpleJob; @@ -28,6 +31,118 @@ protected function setUp(): void CloudTasksApi::fake(); } + /** + * @test + * @testWith [true] + * [false] + */ + public function it_returns_responses_for_empty_payloads($debug) + { + // Arrange + config()->set('app.debug', $debug); + + // Act + $response = $this->postJson(action([TaskHandler::class, 'handle'])); + + // Assert + if ($debug) { + $response->assertJsonValidationErrorFor('task'); + } else { + $response->assertNotFound(); + } + } + + /** + * @test + * @testWith [true] + * [false] + */ + public function it_returns_responses_for_invalid_json($debug) + { + // Arrange + config()->set('app.debug', $debug); + + // Act + $response = $this->call( + 'POST', + action([TaskHandler::class, 'handle']), + [], + [], + [], + [ + 'HTTP_ACCEPT' => 'application/json', + ], + 'test', + ); + + // Assert + if ($debug) { + $response->assertJsonValidationErrorFor('task'); + $this->assertEquals('The json must be a valid JSON string.', $response->json('errors.json.0')); + } else { + $response->assertNotFound(); + } + } + + /** + * @test + * @testWith ["{\"invalid\": \"data\"}", "The task.data field is required."] + * ["{\"data\": \"\"}", "The task.data field is required."] + * ["{\"data\": \"test\"}", "The task.data must be an array."] + */ + public function it_returns_responses_for_invalid_payloads(string $payload, string $expectedMessage) + { + // Arrange + + // Act + $response = $this->call( + 'POST', + action([TaskHandler::class, 'handle']), + [], + [], + [], + [ + 'HTTP_ACCEPT' => 'application/json', + ], + $payload, + ); + + // Assert + $response->assertJsonValidationErrorFor('task.data'); + $this->assertEquals($expectedMessage, $response->json(['errors', 'task.data', 0])); + } + + /** + * @test + * @testWith [true] + * [false] + */ + public function it_validates_headers(bool $withHeaders) + { + // Arrange + $this->withExceptionHandling(); + + // Act + $response = $this->postJson( + action([TaskHandler::class, 'handle']), + [], + $withHeaders + ? [ + 'X-CloudTasks-Taskname' => 'MyTask', + 'X-CloudTasks-TaskRetryCount' => 0, + ] : [] + ); + + // Assert + if ($withHeaders) { + $response->assertJsonMissingValidationErrors('name_header'); + $response->assertJsonMissingValidationErrors('retry_count_header'); + } else { + $response->assertJsonValidationErrors('name_header'); + $response->assertJsonValidationErrors('retry_count_header'); + } + } + /** * @test */ @@ -245,7 +360,7 @@ public function test_max_attempts_in_combination_with_retry_until() $job->run(); # After 2 attempts both Laravel versions should report the same: 2 errors and 0 failures. - $task = StackkitCloudTask::whereTaskUuid($job->payload['uuid'])->firstOrFail(); + $task = StackkitCloudTask::whereTaskUuid($job->payloadAsArray['uuid'])->firstOrFail(); $this->assertEquals(2, $task->getNumberOfAttempts()); $this->assertEquals('error', $task->status); @@ -288,7 +403,7 @@ public function it_can_handle_encrypted_jobs() // Assert $this->assertStringContainsString( 'O:26:"Tests\Support\EncryptedJob"', - decrypt($job->payload['data']['command']), + decrypt($job->payloadAsArray['data']['command']), ); Log::assertLogged('EncryptedJob:success'); diff --git a/tests/TestCase.php b/tests/TestCase.php index 17e70f3..f49712a 100644 --- a/tests/TestCase.php +++ b/tests/TestCase.php @@ -116,10 +116,12 @@ protected function setConfigValue($key, $value) public function dispatch($job) { $payload = null; + $payloadAsArray = []; $task = null; - Event::listen(TaskCreated::class, function (TaskCreated $event) use (&$payload, &$task) { - $payload = json_decode($event->task->getHttpRequest()->getBody(), true); + Event::listen(TaskCreated::class, function (TaskCreated $event) use (&$payload, &$payloadAsArray, &$task) { + $payload = $event->task->getHttpRequest()->getBody(); + $payloadAsArray = json_decode($payload, true); $task = $event->task; request()->headers->set('X-Cloudtasks-Taskname', $task->getName()); @@ -127,32 +129,35 @@ public function dispatch($job) dispatch($job); - return new class($payload, $task) { - public array $payload = []; + return new class($payload, $payloadAsArray, $task) { + public string $payload; + public array $payloadAsArray; public Task $task; - public function __construct(array $payload, Task $task) + public function __construct(string $payload, array $payloadAsArray, Task $task) { $this->payload = $payload; + $this->payloadAsArray = $payloadAsArray; $this->task = $task; } public function run(): void { + $taskRetryCount = request()->header('X-CloudTasks-TaskRetryCount', -1); + request()->headers->set('X-CloudTasks-TaskRetryCount', $taskRetryCount + 1); + rescue(function (): void { app(TaskHandler::class)->handle($this->payload); }); - - $taskRetryCount = request()->header('X-CloudTasks-TaskRetryCount', 0); - request()->headers->set('X-CloudTasks-TaskRetryCount', $taskRetryCount + 1); } public function runWithoutExceptionHandler(): void { + $taskRetryCount = request()->header('X-CloudTasks-TaskRetryCount', -1); + request()->headers->set('X-CloudTasks-TaskRetryCount', $taskRetryCount + 1); + app(TaskHandler::class)->handle($this->payload); - $taskRetryCount = request()->header('X-CloudTasks-TaskRetryCount', 0); - request()->headers->set('X-CloudTasks-TaskRetryCount', $taskRetryCount + 1); } }; } From 7353af930d0c5591be6883a720fbbb0e99e31daa Mon Sep 17 00:00:00 2001 From: Marick van Tuil Date: Fri, 2 Sep 2022 15:24:01 +0200 Subject: [PATCH 3/4] Add enqueueUsing fallback for Laravel 6x and 7x --- src/CloudTasksQueue.php | 19 +++++++++++++++++++ tests/QueueTest.php | 4 ++++ 2 files changed, 23 insertions(+) diff --git a/src/CloudTasksQueue.php b/src/CloudTasksQueue.php index d6ebe1e..1e85c7f 100644 --- a/src/CloudTasksQueue.php +++ b/src/CloudTasksQueue.php @@ -42,6 +42,25 @@ public function size($queue = null) return 0; } + /** + * Fallback method for Laravel 6x and 7x + * + * @param \Closure|string|object $job + * @param string $payload + * @param string $queue + * @param \DateTimeInterface|\DateInterval|int|null $delay + * @param callable $callback + * @return mixed + */ + protected function enqueueUsing($job, $payload, $queue, $delay, $callback) + { + if (method_exists(parent::class, 'enqueueUsing')) { + return parent::enqueueUsing($job, $payload, $queue, $delay, $callback); + } + + return $callback($payload, $queue, $delay); + } + /** * Push a new job onto the queue. * diff --git a/tests/QueueTest.php b/tests/QueueTest.php index 265990d..4e45e83 100644 --- a/tests/QueueTest.php +++ b/tests/QueueTest.php @@ -163,6 +163,10 @@ public function it_posts_the_task_the_correct_queue() */ public function it_can_dispatch_after_commit() { + if (version_compare(app()->version(), '8.0.0', '<')) { + $this->markTestSkipped('Not supported by Laravel 7.x and below.'); + } + // Arrange CloudTasksApi::fake(); Event::fake(); From 13d34750fff214eaea552883784b0ff921ad2b0e Mon Sep 17 00:00:00 2001 From: Marick van Tuil Date: Fri, 2 Sep 2022 15:25:28 +0200 Subject: [PATCH 4/4] Fix validation assertions for older Laravel versions --- tests/TaskHandlerTest.php | 6 +++--- 1 file changed, 3 insertions(+), 3 deletions(-) diff --git a/tests/TaskHandlerTest.php b/tests/TaskHandlerTest.php index 5743c28..1869a90 100644 --- a/tests/TaskHandlerTest.php +++ b/tests/TaskHandlerTest.php @@ -46,7 +46,7 @@ public function it_returns_responses_for_empty_payloads($debug) // Assert if ($debug) { - $response->assertJsonValidationErrorFor('task'); + $response->assertJsonValidationErrors('task'); } else { $response->assertNotFound(); } @@ -77,7 +77,7 @@ public function it_returns_responses_for_invalid_json($debug) // Assert if ($debug) { - $response->assertJsonValidationErrorFor('task'); + $response->assertJsonValidationErrors('task'); $this->assertEquals('The json must be a valid JSON string.', $response->json('errors.json.0')); } else { $response->assertNotFound(); @@ -108,7 +108,7 @@ public function it_returns_responses_for_invalid_payloads(string $payload, strin ); // Assert - $response->assertJsonValidationErrorFor('task.data'); + $response->assertJsonValidationErrors('task.data'); $this->assertEquals($expectedMessage, $response->json(['errors', 'task.data', 0])); }