Стоит ли добавлять декоратор для обработки исключений?

Question

[Sergey Ilichev]

Всем привет.

Есть класс консьюмера (обработчика) принимающий сообщения из очереди. В нем идет блок try catch, внутри вызываются методы сервисов, эти методы сервисов могут кидать исключения, еще эти методы обращаются к репозиториям, типа для создания записей в базе и обновления, соответсвенно репозитории тоже могут кидать исключения типа PDOException. Все эти ошибки ловятся в консьюмере (обработчике), то есть на самом верху. Мне предложили вынести обработку исключений из консьюмера в отдельный декоратор, который соотвественно уже будет дергать метод консьюмера (обработчика), который работает с сервисами. То есть по логике декоратор в данном случае станет консьюмером, так как это будет входная точка сообщения. Нужна ли такая прослойка? Чем это сделает код удобнее? Пока мне кажется, что это лишнее усложнение.

Вот код

public function __invoke(OfdTicketMessage $ofdTicketMessage): void
    {
        $this->logger->info(
            'OfdTicketMessage was received',
            ['data' => $ofdTicketMessage->toArray()]
        );

        try {
            $this->ofdTicketService->create($ofdTicketMessage);
            $this->orangeApiOfdTicketService->createTicket($ofdTicketMessage);

            $this->ofdTicketService->update(
                $ofdTicketMessage,
                [
                    'processed' => true,
                    'statusCode' => OrangeApiResponseStatus::RESPONSE_CREATED
                ]
            );
        } catch (AlreadyCreatedException|AlreadyProcessedException|ConflictException $e) {
            $this->logger->info(
                'Need to ack message because: ' . $e->getMessage(),
                ['ofdTicketId' => $ofdTicketMessage->getId()]
            );

            throw new  UnrecoverableMessageHandlingException($e->getMessage(), $e->getCode());
        } catch (OfdOrangeApiException $e) {
            $this->logger->error(
                'Ofd orange api error: ' . $e->getMessage(),
                ['ofdTicketId' => $ofdTicketMessage->getId()]
            );

            $this->ofdTicketService->update(
                $ofdTicketMessage,
                [
                    'error' => 'Ofd orange api error: ' . $e->getMessage(),
                    'statusCode' => $e->getCode()
                ]
            );
        } catch (TransportExceptionInterface $e) {
            $this->logger->error(
                'Can\'t send request to ofd orange api: ' . $e->getMessage(),
                ['ofdTicketId' => $ofdTicketMessage->getId()]
            );

            $this->ofdTicketService->update(
                $ofdTicketMessage,
                [
                    'error' => 'Can\'t send request to ofd orange api: ' . $e->getMessage(),
                    'statusCode' => $e->getCode()
                ]
            );
        } catch (\Exception|\Throwable $e) {
            $this->logger->error(
                'Something went wrong: ' . $e->getMessage(),
                ['ofdTicketId' => $ofdTicketMessage->getId()]
            );

            $this->ofdTicketService->update(
                $ofdTicketMessage,
                [
                    'error' => 'Something went wrong: ' . $e->getMessage(),
                    'statusCode' => InternalExceptionStatus::INTERNAL_ERROR
                ]
            );

            throw new  UnrecoverableMessageHandlingException($e->getMessage(), $e->getCode());
        }
    }

Comments

[FanatPHP]

Я не очень понял в самом конце, почему обработчик исключений должен работать с сервисами?

[Sergey Ilichev]

FanatPHP, поправил вопрос, чтобы было понятно. Имеется ввиду, что обработку ошибок советуют вынести из консьюмера в декоратор, соответственно этот декоратор как бы станет консьюмером (входной точкой сообщения), и будет дергать в себе метод какого-нибудь класса, который будет выполнять ту логику, которая раньше была в консьюмере, но уже без обработки исключений. Я добавил код для наглядности. Мне предлагают оставить

$this->ofdTicketService->create($ofdTicketMessage);
            $this->orangeApiOfdTicketService->createTicket($ofdTicketMessage);

Обернув в отдельный метод какого-то еще сервиса, а сам обработчик чтобы содержал только try catch. Но я не знаю, стоит ли в отдельную прослойку добавлять просто пару строк, ну или даже если бы их было больше. В чем смысл выносить обработку исключений на уровень выше? Или есть? Вот мне ниже написали, что так тестировать удобнее будет. Я с тестами пока мало занимался, и не могу оценить полезность. Посоветуйте.

[キム ファイブプラス]

Настолько все преисполнились ПАТТЕРНАМИ и ПРИНЦИПАМИ, что уже не в состоянии разобраться с тем что где применяется и зачем. Лишь бы было дорохо бохато и сложно читаемо)

Answer 1

[Михаил]

Зависит от того какая в обработчиках исключений выполняется логика. Если там просто логирование исключений, никак не связанное с логикой консьюмера - то вполне оправданно эту логику отделить. Но можно и не отделять. Тут все зависит от желания переиспользовать консьюмер и декоратор. Также оправданно отделять подобные независимые вещи для облегчения юнит-тестирования.

Но если же в обработке исключений более сложная логика, например, связанная с восстановлением после ошибки, переплетенная с логикой консьюмера - то отделять такую логику будет сложно и не всегда можно сказать нужно ли это делать не видя код.

Comments

[Sergey Ilichev]

В обработчике исключений идет логирование и сохранение в базу данных некоторых ошибок. Код дополню в вопросе, если можно.

[Михаил]

Sergey Ilichev, ну смотрите. У вас все блоки catch имеют дело с OfdTicketMessage. Если у такое же сообщение (или чем-то похожее на него) обрабатывается и в других частях системы и там возникает аналогичный try-catch - то можно сделать из этого декоратор. Иначе говоря, если выделение декоратора упростит вам в чем-то жизнь - в соблюдении DRY, SOLID (особенно SRP), упрощении тестирования и т.д - то выделяйте. Если прямо сейчас выгоды нет - то не выделяйте, выделите потом, когда эта выгода появится.

Answer 2

[Антон Шаманов]

Это не декоратор, а прокси. Я бы скорее сделал логирование ошибок в самих исключениях. Если запрос к api может не пройти из-за серверных проблем api или сети, то лучше не выбрасывать исключение сразу, а попытаться сделать еще пару попыток, а уже потом выбрасывать исключение.