From 4a71e6ef4f7e4284539487cf5b4b2122983f620b Mon Sep 17 00:00:00 2001 From: Jake Barnby Date: Wed, 28 Feb 2024 00:55:11 +1300 Subject: [PATCH 1/6] Use array_push instead of array_merge in loops --- src/Appwrite/Platform/Workers/Messaging.php | 21 +++++++++++++-------- 1 file changed, 13 insertions(+), 8 deletions(-) diff --git a/src/Appwrite/Platform/Workers/Messaging.php b/src/Appwrite/Platform/Workers/Messaging.php index aacd49f94a..ae7fcba69e 100644 --- a/src/Appwrite/Platform/Workers/Messaging.php +++ b/src/Appwrite/Platform/Workers/Messaging.php @@ -118,11 +118,12 @@ class Messaging extends Action $topicIds = $message->getAttribute('topics', []); $targetIds = $message->getAttribute('targets', []); $userIds = $message->getAttribute('users', []); + $providerType = $message->getAttribute('providerType'); /** - * @var array $recipients + * @var array $allTargets */ - $recipients = []; + $allTargets = []; if (\count($topicIds) > 0) { $topics = $dbForProject->find('topics', [ @@ -130,9 +131,11 @@ class Messaging extends Action Query::limit(\count($topicIds)), ]); foreach ($topics as $topic) { - $targets = \array_filter($topic->getAttribute('targets'), fn(Document $target) => - $target->getAttribute('providerType') === $message->getAttribute('providerType')); - $recipients = \array_merge($recipients, $targets); + $targets = \array_filter($topic->getAttribute('targets'), function (Document $target) use ($providerType) { + return $target->getAttribute('providerType') === $providerType; + }); + + \array_push($allTargets, ...$targets); } } @@ -142,9 +145,11 @@ class Messaging extends Action Query::limit(\count($userIds)), ]); foreach ($users as $user) { - $targets = \array_filter($user->getAttribute('targets'), fn(Document $target) => - $target->getAttribute('providerType') === $message->getAttribute('providerType')); - $recipients = \array_merge($recipients, $targets); + $targets = \array_filter($user->getAttribute('targets'), function (Document $target) use ($providerType) { + return $target->getAttribute('providerType') === $providerType; + }); + + \array_push($allTargets, ...$targets); } } From b862c8ee1eb8bb44ff4b730d47f29fa4b8cf459b Mon Sep 17 00:00:00 2001 From: Jake Barnby Date: Wed, 28 Feb 2024 00:56:23 +1300 Subject: [PATCH 2/6] Query provider type directly for targets --- src/Appwrite/Platform/Workers/Messaging.php | 8 ++++---- 1 file changed, 4 insertions(+), 4 deletions(-) diff --git a/src/Appwrite/Platform/Workers/Messaging.php b/src/Appwrite/Platform/Workers/Messaging.php index ae7fcba69e..e4710461ca 100644 --- a/src/Appwrite/Platform/Workers/Messaging.php +++ b/src/Appwrite/Platform/Workers/Messaging.php @@ -156,14 +156,14 @@ class Messaging extends Action if (\count($targetIds) > 0) { $targets = $dbForProject->find('targets', [ Query::equal('$id', $targetIds), + Query::equal('providerType', [$providerType]), Query::limit(\count($targetIds)), ]); - $targets = \array_filter($targets, fn(Document $target) => - $target->getAttribute('providerType') === $message->getAttribute('providerType')); - $recipients = \array_merge($recipients, $targets); + + \array_push($allTargets, ...$targets); } - if (empty($recipients)) { + if (empty($allTargets)) { $dbForProject->updateDocument('messages', $message->getId(), $message->setAttributes([ 'status' => MessageStatus::FAILED, 'deliveryErrors' => ['No valid recipients found.'] From e1e9fc658914ab0abe4aa616d16961e74bb3f0c5 Mon Sep 17 00:00:00 2001 From: Jake Barnby Date: Wed, 28 Feb 2024 00:56:52 +1300 Subject: [PATCH 3/6] Remove redundant checks --- src/Appwrite/Platform/Workers/Messaging.php | 27 +++++++++------------ 1 file changed, 11 insertions(+), 16 deletions(-) diff --git a/src/Appwrite/Platform/Workers/Messaging.php b/src/Appwrite/Platform/Workers/Messaging.php index e4710461ca..84ebf51754 100644 --- a/src/Appwrite/Platform/Workers/Messaging.php +++ b/src/Appwrite/Platform/Workers/Messaging.php @@ -173,18 +173,18 @@ class Messaging extends Action return; } - $fallback = $dbForProject->findOne('providers', [ + $default = $dbForProject->findOne('providers', [ Query::equal('enabled', [true]), - Query::equal('type', [$recipients[0]->getAttribute('providerType')]), + Query::equal('type', [$providerType]), ]); - if ($fallback === false || $fallback->isEmpty()) { + if ($default === false || $default->isEmpty()) { $dbForProject->updateDocument('messages', $message->getId(), $message->setAttributes([ 'status' => MessageStatus::FAILED, - 'deliveryErrors' => ['No fallback provider found.'] + 'deliveryErrors' => ['No enabled provider found.'] ])); - Console::warning('No fallback provider found.'); + Console::warning('No enabled provider found.'); return; } @@ -194,22 +194,17 @@ class Messaging extends Action $identifiers = []; /** - * @var Document[] $providers + * @var array $providers */ $providers = [ - $fallback->getId() => $fallback + $default->getId() => $default ]; - foreach ($recipients as $recipient) { - $providerId = $recipient->getAttribute('providerId'); + foreach ($allTargets as $target) { + $providerId = $target->getAttribute('providerId'); - if ( - !$providerId - && $fallback instanceof Document - && !$fallback->isEmpty() - && $fallback->getAttribute('enabled') - ) { - $providerId = $fallback->getId(); + if (!$providerId) { + $providerId = $default->getId(); } if ($providerId) { From 12e8cc450fa3d3d988f652424fbf34a26b299c68 Mon Sep 17 00:00:00 2001 From: Jake Barnby Date: Wed, 28 Feb 2024 00:59:40 +1300 Subject: [PATCH 4/6] Fix redundant batching --- src/Appwrite/Platform/Workers/Messaging.php | 36 ++++++++++----------- 1 file changed, 18 insertions(+), 18 deletions(-) diff --git a/src/Appwrite/Platform/Workers/Messaging.php b/src/Appwrite/Platform/Workers/Messaging.php index 84ebf51754..4b7b43c66d 100644 --- a/src/Appwrite/Platform/Workers/Messaging.php +++ b/src/Appwrite/Platform/Workers/Messaging.php @@ -218,8 +218,8 @@ class Messaging extends Action /** * @var array $results */ - $results = batch(\array_map(function ($providerId) use ($identifiers, $providers, $fallback, $message, $dbForProject, $deviceForFiles, $deviceForLocalFiles) { - return function () use ($providerId, $identifiers, $providers, $fallback, $message, $dbForProject, $deviceForFiles, $deviceForLocalFiles) { + $results = batch(\array_map(function ($providerId) use ($identifiers, &$providers, $default, $message, $dbForProject, $deviceForFiles, $deviceForLocalFiles) { + return function () use ($providerId, $identifiers, &$providers, $default, $message, $dbForProject, $deviceForFiles, $deviceForLocalFiles) { if (\array_key_exists($providerId, $providers)) { $provider = $providers[$providerId]; } else { @@ -241,12 +241,13 @@ class Messaging extends Action default => throw new Exception(Exception::PROVIDER_INCORRECT_TYPE) }; - $maxBatchSize = $adapter->getMaxMessagesPerRequest(); - $batches = \array_chunk($identifiers, $maxBatchSize); - $batchIndex = 0; + $batches = \array_chunk( + \array_keys($identifiersForProvider), + $adapter->getMaxMessagesPerRequest() + ); - return batch(\array_map(function ($batch) use ($message, $provider, $adapter, &$batchIndex, $dbForProject, $deviceForFiles, $deviceForLocalFiles) { - return function () use ($batch, $message, $provider, $adapter, &$batchIndex, $dbForProject, $deviceForFiles, $deviceForLocalFiles) { + return batch(\array_map(function ($batch) use ($message, $provider, $adapter, $dbForProject, $deviceForFiles, $deviceForLocalFiles) { + return function () use ($batch, $message, $provider, $adapter, $dbForProject, $deviceForFiles, $deviceForLocalFiles) { $deliveredTotal = 0; $deliveryErrors = []; $messageData = clone $message; @@ -283,10 +284,8 @@ class Messaging extends Action } } } catch (\Throwable $e) { - $deliveryErrors[] = 'Failed sending to targets ' . $batchIndex + 1 . ' of ' . \count($batch) . ' with error: ' . $e->getMessage(); + $deliveryErrors[] = 'Failed sending to targets with error: ' . $e->getMessage(); } finally { - $batchIndex++; - return [ 'deliveredTotal' => $deliveredTotal, 'deliveryErrors' => $deliveryErrors, @@ -297,7 +296,7 @@ class Messaging extends Action }; }, \array_keys($identifiers))); - $results = array_merge(...$results); + $results = \array_merge(...$results); $deliveredTotal = 0; $deliveryErrors = []; @@ -330,7 +329,7 @@ class Messaging extends Action $dbForProject->updateDocument('messages', $message->getId(), $message); - // Delete any attachments that were downloaded to the local cache + // Delete any attachments that were downloaded to local storage if ($provider->getAttribute('type') === MESSAGE_TYPE_EMAIL) { if ($deviceForFiles->getType() === Storage::DEVICE_LOCAL) { return; @@ -427,12 +426,13 @@ class Messaging extends Action $adapter = $this->getSmsAdapter($provider); - $maxBatchSize = $adapter->getMaxMessagesPerRequest(); - $batches = \array_chunk($recipients, $maxBatchSize); - $batchIndex = 0; + $batches = \array_chunk( + $recipients, + $adapter->getMaxMessagesPerRequest() + ); - batch(\array_map(function ($batch) use ($message, $provider, $adapter, $batchIndex, $project, $queueForUsage) { - return function () use ($batch, $message, $provider, $adapter, $batchIndex, $project, $queueForUsage) { + batch(\array_map(function ($batch) use ($message, $provider, $adapter, $project, $queueForUsage) { + return function () use ($batch, $message, $provider, $adapter, $project, $queueForUsage) { $message->setAttribute('to', $batch); $data = $this->buildSmsMessage($message, $provider); @@ -445,7 +445,7 @@ class Messaging extends Action ->addMetric(METRIC_MESSAGES, 1) ->trigger(); } catch (\Throwable $e) { - throw new Exception('Failed sending to targets ' . $batchIndex + 1 . '-' . \count($batch) . ' with error: ' . $e->getMessage(), 500); + throw new \Exception('Failed sending to targets with error: ' . $e->getMessage()); } }; }, $batches)); From 7a8ea7278e51d85ed3b860680b6879d60ec737ab Mon Sep 17 00:00:00 2001 From: Jake Barnby Date: Wed, 28 Feb 2024 01:00:36 +1300 Subject: [PATCH 5/6] Fix exceptions --- src/Appwrite/Platform/Workers/Messaging.php | 24 ++++++++++----------- 1 file changed, 12 insertions(+), 12 deletions(-) diff --git a/src/Appwrite/Platform/Workers/Messaging.php b/src/Appwrite/Platform/Workers/Messaging.php index 4b7b43c66d..58d7d31b15 100644 --- a/src/Appwrite/Platform/Workers/Messaging.php +++ b/src/Appwrite/Platform/Workers/Messaging.php @@ -86,7 +86,7 @@ class Messaging extends Action $payload = $message->getPayload() ?? []; if (empty($payload)) { - throw new Exception('Missing payload'); + throw new \Exception('Missing payload'); } $type = $payload['type'] ?? ''; @@ -105,7 +105,7 @@ class Messaging extends Action $this->sendExternalMessage($dbForProject, $message, $deviceForFiles, $deviceForLocalFiles,); break; default: - throw new Exception('Unknown message type: ' . $type); + throw new \Exception('Unknown message type: ' . $type); } } @@ -226,19 +226,19 @@ class Messaging extends Action $provider = $dbForProject->getDocument('providers', $providerId); if ($provider->isEmpty() || !$provider->getAttribute('enabled')) { - $provider = $fallback; + $provider = $default; } else { $providers[$providerId] = $provider; } } - $identifiers = $identifiers[$providerId]; + $identifiersForProvider = $identifiers[$providerId]; $adapter = match ($provider->getAttribute('type')) { MESSAGE_TYPE_SMS => $this->getSmsAdapter($provider), MESSAGE_TYPE_PUSH => $this->getPushAdapter($provider), MESSAGE_TYPE_EMAIL => $this->getEmailAdapter($provider), - default => throw new Exception(Exception::PROVIDER_INCORRECT_TYPE) + default => throw new \Exception('Provider with the requested ID is of the incorrect type') }; $batches = \array_chunk( @@ -257,7 +257,7 @@ class Messaging extends Action MESSAGE_TYPE_SMS => $this->buildSmsMessage($messageData, $provider), MESSAGE_TYPE_PUSH => $this->buildPushMessage($messageData), MESSAGE_TYPE_EMAIL => $this->buildEmailMessage($dbForProject, $messageData, $provider, $deviceForFiles, $deviceForLocalFiles), - default => throw new Exception(Exception::PROVIDER_INCORRECT_TYPE) + default => throw new \Exception('Provider with the requested ID is of the incorrect type') }; try { @@ -344,12 +344,12 @@ class Messaging extends Action $bucket = $dbForProject->getDocument('buckets', $bucketId); if ($bucket->isEmpty()) { - throw new Exception(Exception::STORAGE_BUCKET_NOT_FOUND); + throw new \Exception('Storage bucket with the requested ID could not be found'); } $file = $dbForProject->getDocument('bucket_' . $bucket->getInternalId(), $fileId); if ($file->isEmpty()) { - throw new Exception(Exception::STORAGE_FILE_NOT_FOUND); + throw new \Exception('Storage file with the requested ID could not be found'); } $path = $file->getAttribute('path', ''); @@ -368,7 +368,7 @@ class Messaging extends Action } if ($project->isEmpty()) { - throw new Exception('Project not set in payload'); + throw new \Exception('Project not set in payload'); } Console::log('Project: ' . $project->getId()); @@ -556,19 +556,19 @@ class Messaging extends Action $bucket = $dbForProject->getDocument('buckets', $bucketId); if ($bucket->isEmpty()) { - throw new Exception(Exception::STORAGE_BUCKET_NOT_FOUND); + throw new \Exception('Storage bucket with the requested ID could not be found'); } $file = $dbForProject->getDocument('bucket_' . $bucket->getInternalId(), $fileId); if ($file->isEmpty()) { - throw new Exception(Exception::STORAGE_FILE_NOT_FOUND); + throw new \Exception('Storage file with the requested ID could not be found'); } $mimes = Config::getParam('storage-mimes'); $path = $file->getAttribute('path', ''); if (!$deviceForFiles->exists($path)) { - throw new Exception(Exception::STORAGE_FILE_NOT_FOUND, 'File not found in ' . $path); + throw new \Exception('File not found in ' . $path); } $contentType = 'text/plain'; From f3fa1724826a3eff6ab3ae3957ffd7f799a6c1b8 Mon Sep 17 00:00:00 2001 From: Jake Barnby Date: Wed, 28 Feb 2024 01:01:39 +1300 Subject: [PATCH 6/6] Use a set to avoid message duplication --- src/Appwrite/Platform/Workers/Messaging.php | 5 +++-- 1 file changed, 3 insertions(+), 2 deletions(-) diff --git a/src/Appwrite/Platform/Workers/Messaging.php b/src/Appwrite/Platform/Workers/Messaging.php index 58d7d31b15..ab3a9a41b1 100644 --- a/src/Appwrite/Platform/Workers/Messaging.php +++ b/src/Appwrite/Platform/Workers/Messaging.php @@ -189,7 +189,7 @@ class Messaging extends Action } /** - * @var array> $identifiers + * @var array> $identifiers */ $identifiers = []; @@ -211,7 +211,8 @@ class Messaging extends Action if (!\array_key_exists($providerId, $identifiers)) { $identifiers[$providerId] = []; } - $identifiers[$providerId][] = $recipient->getAttribute('identifier'); + // Use null as value to avoid duplicate keys + $identifiers[$providerId][$target->getAttribute('identifier')] = null; } }