From e16dd19a7cd5f2a43ab47094430670615e9405ed Mon Sep 17 00:00:00 2001 From: Simone Robutti Date: Sun, 24 Oct 2021 21:32:28 +0200 Subject: [PATCH] simplified validation (#91) --- mobilizon_reshare/publishers/abstract.py | 32 +++-------- mobilizon_reshare/publishers/coordinator.py | 53 ++++++++++++------- mobilizon_reshare/publishers/exceptions.py | 8 +-- .../publishers/platforms/mastodon.py | 15 +++--- .../publishers/platforms/platform_mapping.py | 5 ++ .../publishers/platforms/telegram.py | 12 ++--- .../publishers/platforms/twitter.py | 3 +- .../publishers/platforms/zulip.py | 5 +- mobilizon_reshare/storage/query.py | 2 +- tests/publishers/conftest.py | 23 +++++--- tests/publishers/test_abstract_validators.py | 14 ----- tests/publishers/test_coordinator.py | 8 +-- tests/publishers/test_mastodon.py | 8 ++- tests/publishers/test_telegram.py | 21 ++++++-- tests/publishers/test_zulip.py | 15 ++++-- tests/storage/test_update.py | 9 +++- 16 files changed, 130 insertions(+), 103 deletions(-) delete mode 100644 tests/publishers/test_abstract_validators.py diff --git a/mobilizon_reshare/publishers/abstract.py b/mobilizon_reshare/publishers/abstract.py index 2e0a2ae..16032d8 100644 --- a/mobilizon_reshare/publishers/abstract.py +++ b/mobilizon_reshare/publishers/abstract.py @@ -76,9 +76,12 @@ class AbstractPlatform(ABC, LoggerMixin, ConfLoaderMixin): # the second the name of its service (ie: 'facebook', 'telegram') def __repr__(self): - return type(self).__name__ + return self.name - __str__ = __repr__ + @property + @abstractmethod + def name(self): + pass @abstractmethod def _send(self, message: str): @@ -99,13 +102,6 @@ class AbstractPlatform(ABC, LoggerMixin, ConfLoaderMixin): def _validate_response(self, response): raise NotImplementedError # pragma: no cover - def are_credentials_valid(self) -> bool: - try: - self.validate_credentials() - except PublisherError: - return False - return True - @abstractmethod def validate_credentials(self) -> None: """ @@ -146,14 +142,6 @@ class AbstractEventFormatter(LoggerMixin, ConfLoaderMixin): template_path = self.conf.msg_template_path or self.default_template_path return JINJA_ENV.get_template(template_path) - def is_message_valid(self, event: MobilizonEvent) -> bool: - # TODO: this thing swallows exception messages. It should be handled differently - try: - self.validate_message(self.get_message_from_event(event)) - except PublisherError: - return False - return True - @abstractmethod def validate_message(self, message: str) -> None: """ @@ -163,13 +151,6 @@ class AbstractEventFormatter(LoggerMixin, ConfLoaderMixin): """ raise NotImplementedError # pragma: no cover - def is_event_valid(self, event) -> bool: - try: - self.validate_event(event) - except PublisherError: - return False - return True - def get_recap_header(self): template_path = ( self.conf.recap_header_template_path @@ -184,6 +165,9 @@ class AbstractEventFormatter(LoggerMixin, ConfLoaderMixin): return JINJA_ENV.get_template(template_path) def get_recap_fragment(self, event: MobilizonEvent) -> str: + """ + Retrieves the fragment that describes a single event inside the event recap. + """ event = self._preprocess_event(event) return event.format(self.get_recap_fragment_template()) diff --git a/mobilizon_reshare/publishers/coordinator.py b/mobilizon_reshare/publishers/coordinator.py index 19e2acd..fe8db0f 100644 --- a/mobilizon_reshare/publishers/coordinator.py +++ b/mobilizon_reshare/publishers/coordinator.py @@ -1,7 +1,6 @@ import logging from dataclasses import dataclass from typing import List, Optional -from uuid import UUID from mobilizon_reshare.models.publication import PublicationStatus from mobilizon_reshare.publishers import get_active_notifiers @@ -30,13 +29,17 @@ class BasePublicationReport: @dataclass class EventPublicationReport(BasePublicationReport): - publication_id: UUID + publication: EventPublication def get_failure_message(self): + if not self.reason: + logger.error("Report of failure without reason.", exc_info=True) + return ( - f"Publication {self.publication_id } failed with status: {self.status}.\n" - f"Reason: {self.reason}" + f"Publication {self.publication.id} failed with status: {self.status}.\n" + f"Reason: {self.reason}\n" + f"Publisher: {self.publication.publisher.name}" ) @@ -72,9 +75,7 @@ class PublisherCoordinator: def _make_successful_report(self, failed_ids): return [ EventPublicationReport( - status=PublicationStatus.COMPLETED, - reason="", - publication_id=publication.id, + status=PublicationStatus.COMPLETED, reason="", publication=publication, ) for publication in self.publications if publication.id not in failed_ids @@ -93,7 +94,7 @@ class PublisherCoordinator: reports.append( EventPublicationReport( status=PublicationStatus.COMPLETED, - publication_id=publication.id, + publication=publication, reason=None, ) ) @@ -102,7 +103,7 @@ class PublisherCoordinator: EventPublicationReport( status=PublicationStatus.FAILED, reason=str(e), - publication_id=publication.id, + publication=publication, ) ) @@ -110,25 +111,37 @@ class PublisherCoordinator: publications=self.publications, reports=reports ) + def _safe_run(self, reasons, f, *args, **kwargs): + try: + f(*args, **kwargs) + return reasons + except Exception as e: + logger.error(str(e)) + return reasons + [str(e)] + def _validate(self): errors = [] for publication in self.publications: + reasons = [] + reasons = self._safe_run( + reasons, publication.publisher.validate_credentials, + ) + reasons = self._safe_run( + reasons, publication.formatter.validate_event, publication.event + ) + reasons = self._safe_run( + reasons, + publication.formatter.validate_message, + publication.formatter.get_message_from_event(publication.event), + ) - reason = [] - if not publication.publisher.are_credentials_valid(): - reason.append("Invalid credentials") - if not publication.formatter.is_event_valid(publication.event): - reason.append("Invalid event") - if not publication.formatter.is_message_valid(publication.event): - reason.append("Invalid message") - - if len(reason) > 0: + if len(reasons) > 0: errors.append( EventPublicationReport( status=PublicationStatus.FAILED, - reason=", ".join(reason), - publication_id=publication.id, + reason=", ".join(reasons), + publication=publication, ) ) diff --git a/mobilizon_reshare/publishers/exceptions.py b/mobilizon_reshare/publishers/exceptions.py index 059c4f4..0420daa 100644 --- a/mobilizon_reshare/publishers/exceptions.py +++ b/mobilizon_reshare/publishers/exceptions.py @@ -32,9 +32,9 @@ class InvalidSettings(PublisherError): """ Publisher settings are either missing or badly configured """ -class HTTPResponseError(PublisherError): - """ Response received with an HTTP error status code""" - - class ZulipError(PublisherError): """ Publisher receives an error response from Zulip""" + + +class HTTPResponseError(PublisherError): + """ Publisher receives a HTTP error""" diff --git a/mobilizon_reshare/publishers/platforms/mastodon.py b/mobilizon_reshare/publishers/platforms/mastodon.py index dbb53eb..b80af9a 100644 --- a/mobilizon_reshare/publishers/platforms/mastodon.py +++ b/mobilizon_reshare/publishers/platforms/mastodon.py @@ -1,6 +1,6 @@ -import pkg_resources from urllib.parse import urljoin +import pkg_resources import requests from requests import Response @@ -13,8 +13,8 @@ from mobilizon_reshare.publishers.exceptions import ( InvalidBot, InvalidEvent, InvalidResponse, - PublisherError, HTTPResponseError, + InvalidMessage, ) @@ -40,7 +40,7 @@ class MastodonFormatter(AbstractEventFormatter): def validate_message(self, message) -> None: if len(message.encode("utf-8")) >= self.conf.toot_length: - raise PublisherError("Message is too long") + raise InvalidMessage("Message is too long") class MastodonPlatform(AbstractPlatform): @@ -50,6 +50,7 @@ class MastodonPlatform(AbstractPlatform): _conf = ("publisher", "mastodon") api_uri = "api/v1/" + name = "mastodon" def _send(self, message: str) -> Response: """ @@ -58,10 +59,7 @@ class MastodonPlatform(AbstractPlatform): return requests.post( url=urljoin(self.conf.instance, self.api_uri) + "statuses", headers={"Authorization": f"Bearer {self.conf.token}"}, - data={ - "status": message, - "visibility": "public", - }, + data={"status": message, "visibility": "public"}, ) def validate_credentials(self): @@ -85,8 +83,7 @@ class MastodonPlatform(AbstractPlatform): except requests.exceptions.HTTPError as e: self._log_debug(str(res)) self._log_error( - str(e), - raise_error=HTTPResponseError, + str(e), raise_error=HTTPResponseError, ) try: diff --git a/mobilizon_reshare/publishers/platforms/platform_mapping.py b/mobilizon_reshare/publishers/platforms/platform_mapping.py index 0e0c333..c2b4fee 100644 --- a/mobilizon_reshare/publishers/platforms/platform_mapping.py +++ b/mobilizon_reshare/publishers/platforms/platform_mapping.py @@ -19,6 +19,11 @@ from mobilizon_reshare.publishers.platforms.zulip import ( ZulipNotifier, ) +""" +This module is required to have an explicit mapping between platform names and the classes implementing those platforms. +It could be refactored in a different pattern but this way makes it more explicit and linear. Eventually this could be +turned into a plugin system with a plugin for each platform.""" + name_to_publisher_class = { "mastodon": MastodonPublisher, "telegram": TelegramPublisher, diff --git a/mobilizon_reshare/publishers/platforms/telegram.py b/mobilizon_reshare/publishers/platforms/telegram.py index f6bb9d9..57d62ef 100644 --- a/mobilizon_reshare/publishers/platforms/telegram.py +++ b/mobilizon_reshare/publishers/platforms/telegram.py @@ -14,8 +14,7 @@ from mobilizon_reshare.publishers.exceptions import ( InvalidBot, InvalidEvent, InvalidResponse, - PublisherError, - HTTPResponseError, + InvalidMessage, ) @@ -72,7 +71,7 @@ class TelegramFormatter(AbstractEventFormatter): def validate_message(self, message: str) -> None: if len(message) >= 4096: - raise PublisherError("Message is too long") + raise InvalidMessage("Message is too long") def _preprocess_event(self, event: MobilizonEvent): event.description = html_to_markdown(event.description) @@ -85,6 +84,8 @@ class TelegramPlatform(AbstractPlatform): Telegram publisher class. """ + name = "telegram" + def _preprocess_message(self, message: str): return TelegramFormatter.escape_message(message) @@ -109,12 +110,11 @@ class TelegramPlatform(AbstractPlatform): def _validate_response(self, res): try: + res.raise_for_status() except requests.exceptions.HTTPError as e: - self._log_debug(str(res)) self._log_error( - str(e), - raise_error=HTTPResponseError, + f"Server returned invalid data: {str(e)}", raise_error=InvalidResponse, ) try: diff --git a/mobilizon_reshare/publishers/platforms/twitter.py b/mobilizon_reshare/publishers/platforms/twitter.py index c1544e2..bf9bca2 100644 --- a/mobilizon_reshare/publishers/platforms/twitter.py +++ b/mobilizon_reshare/publishers/platforms/twitter.py @@ -47,6 +47,7 @@ class TwitterPlatform(AbstractPlatform): """ _conf = ("publisher", "twitter") + name = "twitter" def _get_api(self): @@ -62,7 +63,7 @@ class TwitterPlatform(AbstractPlatform): try: return self._get_api().update_status(message) except TweepyException as e: - raise PublisherError from e + raise PublisherError(e.args[0]) def validate_credentials(self): if not self._get_api().verify_credentials(): diff --git a/mobilizon_reshare/publishers/platforms/zulip.py b/mobilizon_reshare/publishers/platforms/zulip.py index f74cbfc..de3a04e 100644 --- a/mobilizon_reshare/publishers/platforms/zulip.py +++ b/mobilizon_reshare/publishers/platforms/zulip.py @@ -14,7 +14,7 @@ from mobilizon_reshare.publishers.exceptions import ( InvalidEvent, InvalidResponse, ZulipError, - PublisherError, + InvalidMessage, ) @@ -40,7 +40,7 @@ class ZulipFormatter(AbstractEventFormatter): def validate_message(self, message) -> None: if len(message.encode("utf-8")) >= 10000: - raise PublisherError("Message is too long") + raise InvalidMessage("Message is too long") def _preprocess_event(self, event: MobilizonEvent): event.description = html_to_markdown(event.description) @@ -56,6 +56,7 @@ class ZulipPlatform(AbstractPlatform): _conf = ("publisher", "zulip") api_uri = "https://zulip.twc-italia.org/api/v1/" + name = "zulip" def _send_private(self, message: str) -> Response: """ diff --git a/mobilizon_reshare/storage/query.py b/mobilizon_reshare/storage/query.py index 615f8e7..4383023 100644 --- a/mobilizon_reshare/storage/query.py +++ b/mobilizon_reshare/storage/query.py @@ -201,7 +201,7 @@ async def save_publication_report( publications: Dict[UUID, Publication], ) -> None: for publication_report in coordinator_report.reports: - publication_id = publication_report.publication_id + publication_id = publication_report.publication.id publications[publication_id].status = publication_report.status publications[publication_id].reason = publication_report.reason publications[publication_id].timestamp = arrow.now().datetime diff --git a/tests/publishers/conftest.py b/tests/publishers/conftest.py index 27afbf8..70ce70a 100644 --- a/tests/publishers/conftest.py +++ b/tests/publishers/conftest.py @@ -65,13 +65,13 @@ def message_collector(): def mock_formatter_invalid(): class MockFormatter(AbstractEventFormatter): def validate_event(self, event) -> None: - raise PublisherError("Invalid event") + raise PublisherError("Invalid event error") def get_message_from_event(self, event) -> str: return "" def validate_message(self, event) -> None: - raise PublisherError("Invalid message") + raise PublisherError("Invalid message error") return MockFormatter() @@ -79,8 +79,10 @@ def mock_formatter_invalid(): @pytest.fixture def mock_publisher_valid(message_collector): class MockPublisher(AbstractPlatform): + name = "mock" + def _send(self, message): - message_collector.collect_message(message) + message_collector.append(message) def _validate_response(self, response): pass @@ -94,15 +96,17 @@ def mock_publisher_valid(message_collector): @pytest.fixture def mock_publisher_invalid(message_collector): class MockPublisher(AbstractPlatform): - def _send(self, message): - message_collector.collect_message(message) + name = "mock" + + def _send(self, message): + message_collector.append(message) def _validate_response(self, response): - return InvalidResponse("error") + return InvalidResponse("response error") def validate_credentials(self) -> None: - raise PublisherError("error") + raise PublisherError("credentials error") return MockPublisher() @@ -110,8 +114,11 @@ def mock_publisher_invalid(message_collector): @pytest.fixture def mock_publisher_invalid_response(message_collector): class MockPublisher(AbstractPlatform): + + name = "mock" + def _send(self, message): - message_collector.collect_message(message) + message_collector.append(message) def _validate_response(self, response): raise InvalidResponse("Invalid response") diff --git a/tests/publishers/test_abstract_validators.py b/tests/publishers/test_abstract_validators.py deleted file mode 100644 index 30e51cd..0000000 --- a/tests/publishers/test_abstract_validators.py +++ /dev/null @@ -1,14 +0,0 @@ -def test_is_event_valid(mock_formatter_valid, event): - assert mock_formatter_valid.is_event_valid(event) - - -def test_is_event_valid_false(mock_formatter_invalid, event): - assert not mock_formatter_invalid.is_event_valid(event) - - -def test_is_message_valid(mock_formatter_valid, event): - assert mock_formatter_valid.is_message_valid(event) - - -def test_is_message_valid_false(mock_formatter_invalid, event): - assert not mock_formatter_invalid.is_message_valid(event) diff --git a/tests/publishers/test_coordinator.py b/tests/publishers/test_coordinator.py index bbfb71e..b9af503 100644 --- a/tests/publishers/test_coordinator.py +++ b/tests/publishers/test_coordinator.py @@ -33,7 +33,7 @@ def test_publication_report_successful(statuses, successful): reports = [] for i, status in enumerate(statuses): reports.append( - EventPublicationReport(reason=None, publication_id=None, status=status) + EventPublicationReport(reason=None, publication=None, status=status) ) assert ( PublisherCoordinatorReport(publications=[], reports=reports).successful @@ -116,7 +116,7 @@ async def test_publication_coordinator_run_failure( assert not report.successful assert ( list(report.reports)[0].reason - == "Invalid credentials, Invalid event, Invalid message" + == "credentials error, Invalid event error, Invalid message error" ) @@ -142,7 +142,9 @@ async def test_notifier_coordinator_publication_failed(mock_publisher_valid): report = EventPublicationReport( status=PublicationStatus.FAILED, reason="some failure", - publication_id=UUID(int=1), + publication=EventPublication( + id=UUID(int=4), publisher=mock_publisher_valid, formatter=None, event=None + ), ) coordinator = PublicationFailureNotifiersCoordinator( report, [mock_publisher_valid, mock_publisher_valid] diff --git a/tests/publishers/test_mastodon.py b/tests/publishers/test_mastodon.py index b963aed..054ed11 100644 --- a/tests/publishers/test_mastodon.py +++ b/tests/publishers/test_mastodon.py @@ -5,6 +5,7 @@ from mobilizon_reshare.publishers.exceptions import ( InvalidEvent, InvalidResponse, HTTPResponseError, + InvalidMessage, ) from mobilizon_reshare.publishers.platforms.mastodon import ( MastodonFormatter, @@ -15,13 +16,16 @@ from mobilizon_reshare.publishers.platforms.mastodon import ( def test_message_length_success(event): message = "a" * 200 event.name = message - assert MastodonFormatter().is_message_valid(event) + message = MastodonFormatter().get_message_from_event(event) + MastodonFormatter().validate_message(message) def test_message_length_failure(event): message = "a" * 500 event.name = message - assert not MastodonFormatter().is_message_valid(event) + message = MastodonFormatter().get_message_from_event(event) + with pytest.raises(InvalidMessage): + MastodonFormatter().validate_message(message) def test_event_validation(event): diff --git a/tests/publishers/test_telegram.py b/tests/publishers/test_telegram.py index d2c7b5a..0e77615 100644 --- a/tests/publishers/test_telegram.py +++ b/tests/publishers/test_telegram.py @@ -1,7 +1,11 @@ import pytest import requests -from mobilizon_reshare.publishers.exceptions import InvalidEvent, InvalidResponse, HTTPResponseError +from mobilizon_reshare.publishers.exceptions import ( + InvalidEvent, + InvalidResponse, + InvalidMessage, +) from mobilizon_reshare.publishers.platforms.telegram import ( TelegramFormatter, TelegramPublisher, @@ -11,13 +15,22 @@ from mobilizon_reshare.publishers.platforms.telegram import ( def test_message_length_success(event): message = "a" * 500 event.description = message - assert TelegramFormatter().is_message_valid(event) + assert ( + TelegramFormatter().validate_message( + TelegramFormatter().get_message_from_event(event) + ) + is None + ) def test_message_length_failure(event): message = "a" * 10000 event.description = message - assert not TelegramFormatter().is_message_valid(event) + + with pytest.raises(InvalidMessage): + TelegramFormatter().validate_message( + TelegramFormatter().get_message_from_event(event) + ) @pytest.mark.parametrize( @@ -68,7 +81,7 @@ def test_validate_response_invalid_request(): response = requests.Response() response.status_code = 400 response._content = b"""{"error":true}""" - with pytest.raises(HTTPResponseError) as e: + with pytest.raises(InvalidResponse) as e: TelegramPublisher()._validate_response(response) diff --git a/tests/publishers/test_zulip.py b/tests/publishers/test_zulip.py index 06817a3..a92a920 100644 --- a/tests/publishers/test_zulip.py +++ b/tests/publishers/test_zulip.py @@ -13,6 +13,7 @@ from mobilizon_reshare.publishers.exceptions import ( InvalidEvent, InvalidResponse, ZulipError, + InvalidMessage, ) from mobilizon_reshare.publishers.platforms.zulip import ZulipFormatter, ZulipPublisher from mobilizon_reshare.storage.query import ( @@ -139,7 +140,7 @@ async def test_zulip_publishr_failure_invalid_credentials( ) ).run() assert report.reports[0].status == PublicationStatus.FAILED - assert report.reports[0].reason == "Invalid credentials" + assert report.reports[0].reason == "403 Error - Your credentials are not valid!" @pytest.mark.asyncio @@ -170,13 +171,21 @@ def test_event_validation(event): def test_message_length_success(event): message = "a" * 500 event.description = message - assert ZulipFormatter().is_message_valid(event) + assert ( + ZulipFormatter().validate_message( + ZulipFormatter().get_message_from_event(event) + ) + is None + ) def test_message_length_failure(event): message = "a" * 10000 event.description = message - assert not ZulipFormatter().is_message_valid(event) + with pytest.raises(InvalidMessage): + ZulipFormatter().validate_message( + ZulipFormatter().get_message_from_event(event) + ) def test_validate_response(): diff --git a/tests/storage/test_update.py b/tests/storage/test_update.py index 8e6c449..be6c856 100644 --- a/tests/storage/test_update.py +++ b/tests/storage/test_update.py @@ -7,6 +7,7 @@ import pytest from mobilizon_reshare.event.event import MobilizonEvent, EventPublicationStatus from mobilizon_reshare.models.publication import PublicationStatus, Publication from mobilizon_reshare.models.publisher import Publisher +from mobilizon_reshare.publishers.abstract import EventPublication from mobilizon_reshare.publishers.coordinator import ( PublisherCoordinatorReport, EventPublicationReport, @@ -73,12 +74,16 @@ async def test_update_publishers( EventPublicationReport( status=PublicationStatus.FAILED, reason="Invalid credentials", - publication_id=UUID(int=3), + publication=EventPublication( + id=UUID(int=3), formatter=None, event=None, publisher=None + ), ), EventPublicationReport( status=PublicationStatus.COMPLETED, reason="", - publication_id=UUID(int=4), + publication=EventPublication( + id=UUID(int=4), formatter=None, event=None, publisher=None + ), ), ], ),