From c650d4a8e004f8f8729948b6be861eccd111d594 Mon Sep 17 00:00:00 2001 From: Cristhian Garcia Date: Wed, 9 Oct 2024 12:48:24 -0500 Subject: [PATCH 1/9] fix: catch json errors on empty responses from ralph --- event_routing_backends/tasks.py | 4 ++-- event_routing_backends/utils/xapi_lrs_client.py | 2 +- 2 files changed, 3 insertions(+), 3 deletions(-) diff --git a/event_routing_backends/tasks.py b/event_routing_backends/tasks.py index 58c70298..fdb1f436 100644 --- a/event_routing_backends/tasks.py +++ b/event_routing_backends/tasks.py @@ -5,7 +5,7 @@ from celery.utils.log import get_task_logger from celery_utils.persist_on_failure import LoggedPersistOnFailureTask from django.conf import settings - +from json.decoder import JSONDecodeError from event_routing_backends.processors.transformer_utils.exceptions import EventNotDispatched from event_routing_backends.utils.http_client import HttpClient from event_routing_backends.utils.xapi_lrs_client import LrsClient @@ -131,7 +131,7 @@ def bulk_send_events(task, events, router_type, host_config): client_class ) ) - except EventNotDispatched as exc: + except (EventNotDispatched, JSONDecodeError) as exc: logger.exception( 'Exception occurred while trying to bulk dispatch {} events using client: {}'.format( len(events), diff --git a/event_routing_backends/utils/xapi_lrs_client.py b/event_routing_backends/utils/xapi_lrs_client.py index 4e9f1216..1bb61745 100644 --- a/event_routing_backends/utils/xapi_lrs_client.py +++ b/event_routing_backends/utils/xapi_lrs_client.py @@ -75,7 +75,7 @@ def bulk_send(self, statement_data): response = self.lrs_client.save_statements(statement_data) if not response.success: - if response.response.code == 409: + if response.response.code == 409 or response.response.code == 204: logger.warning(f"Duplicate event id found in: {response.request.content}") else: logger.warning(f"Failed request: {response.request.content}") From ea5681c125ef0c33267a86f0b73da2bfdb800af7 Mon Sep 17 00:00:00 2001 From: Cristhian Garcia Date: Tue, 15 Oct 2024 12:59:33 -0500 Subject: [PATCH 2/9] fix: move JSONDecodeError catch statement to xapi lrs --- event_routing_backends/tasks.py | 3 +-- event_routing_backends/utils/xapi_lrs_client.py | 7 ++++++- 2 files changed, 7 insertions(+), 3 deletions(-) diff --git a/event_routing_backends/tasks.py b/event_routing_backends/tasks.py index fdb1f436..ae171736 100644 --- a/event_routing_backends/tasks.py +++ b/event_routing_backends/tasks.py @@ -5,7 +5,6 @@ from celery.utils.log import get_task_logger from celery_utils.persist_on_failure import LoggedPersistOnFailureTask from django.conf import settings -from json.decoder import JSONDecodeError from event_routing_backends.processors.transformer_utils.exceptions import EventNotDispatched from event_routing_backends.utils.http_client import HttpClient from event_routing_backends.utils.xapi_lrs_client import LrsClient @@ -131,7 +130,7 @@ def bulk_send_events(task, events, router_type, host_config): client_class ) ) - except (EventNotDispatched, JSONDecodeError) as exc: + except EventNotDispatched as exc: logger.exception( 'Exception occurred while trying to bulk dispatch {} events using client: {}'.format( len(events), diff --git a/event_routing_backends/utils/xapi_lrs_client.py b/event_routing_backends/utils/xapi_lrs_client.py index 1bb61745..c00c874c 100644 --- a/event_routing_backends/utils/xapi_lrs_client.py +++ b/event_routing_backends/utils/xapi_lrs_client.py @@ -1,6 +1,7 @@ """ An LRS client for xAPI stores. """ +from json.decoder import JSONDecodeError from logging import getLogger from tincan.remote_lrs import RemoteLRS @@ -72,7 +73,11 @@ def bulk_send(self, statement_data): """ logger.debug('Sending {} xAPI statements to {}'.format(len(statement_data), self.URL)) - response = self.lrs_client.save_statements(statement_data) + try: + response = self.lrs_client.save_statements(statement_data) + except JSONDecodeError as e: + logger.warning(f"Events already in LRS: {response.request.content}") + return if not response.success: if response.response.code == 409 or response.response.code == 204: From 1a0e38254c98fb6fd583b9d886e997ceeff981bb Mon Sep 17 00:00:00 2001 From: Cristhian Garcia Date: Tue, 15 Oct 2024 13:02:07 -0500 Subject: [PATCH 3/9] fix: move JSONDecodeError catch statement to xapi lrs --- event_routing_backends/tasks.py | 1 + 1 file changed, 1 insertion(+) diff --git a/event_routing_backends/tasks.py b/event_routing_backends/tasks.py index ae171736..58c70298 100644 --- a/event_routing_backends/tasks.py +++ b/event_routing_backends/tasks.py @@ -5,6 +5,7 @@ from celery.utils.log import get_task_logger from celery_utils.persist_on_failure import LoggedPersistOnFailureTask from django.conf import settings + from event_routing_backends.processors.transformer_utils.exceptions import EventNotDispatched from event_routing_backends.utils.http_client import HttpClient from event_routing_backends.utils.xapi_lrs_client import LrsClient From a119e81fe10d6bed774abb32c5d37029d949652b Mon Sep 17 00:00:00 2001 From: Cristhian Garcia Date: Tue, 15 Oct 2024 13:03:15 -0500 Subject: [PATCH 4/9] chore: remove unnecessary condition --- event_routing_backends/utils/xapi_lrs_client.py | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/event_routing_backends/utils/xapi_lrs_client.py b/event_routing_backends/utils/xapi_lrs_client.py index c00c874c..8761dece 100644 --- a/event_routing_backends/utils/xapi_lrs_client.py +++ b/event_routing_backends/utils/xapi_lrs_client.py @@ -80,7 +80,7 @@ def bulk_send(self, statement_data): return if not response.success: - if response.response.code == 409 or response.response.code == 204: + if response.response.code == 409: logger.warning(f"Duplicate event id found in: {response.request.content}") else: logger.warning(f"Failed request: {response.request.content}") From 6ebab7cf6c3cf819d5fced2b43903def1593ea42 Mon Sep 17 00:00:00 2001 From: Cristhian Garcia Date: Tue, 15 Oct 2024 13:26:53 -0500 Subject: [PATCH 5/9] refactor: define response outside try-except --- event_routing_backends/utils/xapi_lrs_client.py | 5 ++++- 1 file changed, 4 insertions(+), 1 deletion(-) diff --git a/event_routing_backends/utils/xapi_lrs_client.py b/event_routing_backends/utils/xapi_lrs_client.py index 8761dece..9b341f78 100644 --- a/event_routing_backends/utils/xapi_lrs_client.py +++ b/event_routing_backends/utils/xapi_lrs_client.py @@ -72,11 +72,14 @@ def bulk_send(self, statement_data): requests.Response object """ logger.debug('Sending {} xAPI statements to {}'.format(len(statement_data), self.URL)) + response = None try: response = self.lrs_client.save_statements(statement_data) - except JSONDecodeError as e: + except JSONDecodeError: logger.warning(f"Events already in LRS: {response.request.content}") + + if not response: return if not response.success: From fdc867de51f68ab929db17918f567ad57b5d4f77 Mon Sep 17 00:00:00 2001 From: Cristhian Garcia Date: Tue, 15 Oct 2024 13:50:25 -0500 Subject: [PATCH 6/9] test: add tests for jsondecode error --- .../backends/tests/test_events_router.py | 18 +++++++++++++++++- .../utils/xapi_lrs_client.py | 2 +- 2 files changed, 18 insertions(+), 2 deletions(-) diff --git a/event_routing_backends/backends/tests/test_events_router.py b/event_routing_backends/backends/tests/test_events_router.py index fec2bac0..176b4aaf 100644 --- a/event_routing_backends/backends/tests/test_events_router.py +++ b/event_routing_backends/backends/tests/test_events_router.py @@ -5,7 +5,7 @@ import json from copy import copy from unittest.mock import MagicMock, call, patch, sentinel - +from json import JSONDecodeError import ddt from django.conf import settings from django.test import TestCase, override_settings @@ -260,6 +260,22 @@ def test_duplicate_xapi_event_id(self, mocked_logger): mocked_logger.info.mock_calls ) + @patch('event_routing_backends.utils.xapi_lrs_client.logger') + def test_duplicate_xapi_event_id_json(self, mocked_logger): + """ + Test that when we receive a 409 response when inserting an XAPI statement + we do not raise an exception, but do log it. + """ + client = LrsClient({}) + client.lrs_client = MagicMock() + client.lrs_client.save_statements.side_effect = JSONDecodeError(msg="msg", doc="...", pos=0) + + client.bulk_send(statement_data=[]) + self.assertIn( + call('Events already in LRS: []'), + mocked_logger.warning.mock_calls + ) + @override_settings( EVENT_ROUTING_BACKEND_BATCHING_ENABLED=True, EVENT_ROUTING_BACKEND_BATCH_SIZE=2 diff --git a/event_routing_backends/utils/xapi_lrs_client.py b/event_routing_backends/utils/xapi_lrs_client.py index 9b341f78..df9eb2da 100644 --- a/event_routing_backends/utils/xapi_lrs_client.py +++ b/event_routing_backends/utils/xapi_lrs_client.py @@ -77,7 +77,7 @@ def bulk_send(self, statement_data): try: response = self.lrs_client.save_statements(statement_data) except JSONDecodeError: - logger.warning(f"Events already in LRS: {response.request.content}") + logger.warning(f"Events already in LRS: {statement_data}") if not response: return From a760149923092c9a137252156bc7e459137e9d29 Mon Sep 17 00:00:00 2001 From: Cristhian Garcia Date: Tue, 15 Oct 2024 14:17:46 -0500 Subject: [PATCH 7/9] chore: update duplicated message --- event_routing_backends/backends/tests/test_events_router.py | 6 +++--- event_routing_backends/utils/xapi_lrs_client.py | 2 +- 2 files changed, 4 insertions(+), 4 deletions(-) diff --git a/event_routing_backends/backends/tests/test_events_router.py b/event_routing_backends/backends/tests/test_events_router.py index 176b4aaf..7e56f69d 100644 --- a/event_routing_backends/backends/tests/test_events_router.py +++ b/event_routing_backends/backends/tests/test_events_router.py @@ -263,8 +263,8 @@ def test_duplicate_xapi_event_id(self, mocked_logger): @patch('event_routing_backends.utils.xapi_lrs_client.logger') def test_duplicate_xapi_event_id_json(self, mocked_logger): """ - Test that when we receive a 409 response when inserting an XAPI statement - we do not raise an exception, but do log it. + Test that when we receive a 204 response (and the LRSClient fails to parse to JSON + the response) when bulk inserting XAPI statements it may indicates all events are already stored. """ client = LrsClient({}) client.lrs_client = MagicMock() @@ -272,7 +272,7 @@ def test_duplicate_xapi_event_id_json(self, mocked_logger): client.bulk_send(statement_data=[]) self.assertIn( - call('Events already in LRS: []'), + call('JSON Decode Error, this may indicate that all sent events are already stored: []'), mocked_logger.warning.mock_calls ) diff --git a/event_routing_backends/utils/xapi_lrs_client.py b/event_routing_backends/utils/xapi_lrs_client.py index df9eb2da..eb625e9f 100644 --- a/event_routing_backends/utils/xapi_lrs_client.py +++ b/event_routing_backends/utils/xapi_lrs_client.py @@ -77,7 +77,7 @@ def bulk_send(self, statement_data): try: response = self.lrs_client.save_statements(statement_data) except JSONDecodeError: - logger.warning(f"Events already in LRS: {statement_data}") + logger.warning(f"JSON Decode Error, this may indicate that all sent events are already stored: {statement_data}") if not response: return From 08ab2652dfef875fbaa602d6c83f86aa508e91e7 Mon Sep 17 00:00:00 2001 From: Cristhian Garcia Date: Tue, 15 Oct 2024 14:20:55 -0500 Subject: [PATCH 8/9] chore: quality changes --- event_routing_backends/utils/xapi_lrs_client.py | 4 +++- 1 file changed, 3 insertions(+), 1 deletion(-) diff --git a/event_routing_backends/utils/xapi_lrs_client.py b/event_routing_backends/utils/xapi_lrs_client.py index eb625e9f..45f2e2b0 100644 --- a/event_routing_backends/utils/xapi_lrs_client.py +++ b/event_routing_backends/utils/xapi_lrs_client.py @@ -77,7 +77,9 @@ def bulk_send(self, statement_data): try: response = self.lrs_client.save_statements(statement_data) except JSONDecodeError: - logger.warning(f"JSON Decode Error, this may indicate that all sent events are already stored: {statement_data}") + logger.warning( + f"JSON Decode Error, this may indicate that all sent events are already stored: {statement_data}" + ) if not response: return From 793fb94d997c3f67bedd2eb9ec285e7ad47b5663 Mon Sep 17 00:00:00 2001 From: Cristhian Garcia Date: Tue, 15 Oct 2024 14:22:17 -0500 Subject: [PATCH 9/9] chore: quality changes --- event_routing_backends/backends/tests/test_events_router.py | 3 ++- 1 file changed, 2 insertions(+), 1 deletion(-) diff --git a/event_routing_backends/backends/tests/test_events_router.py b/event_routing_backends/backends/tests/test_events_router.py index 7e56f69d..1252de7a 100644 --- a/event_routing_backends/backends/tests/test_events_router.py +++ b/event_routing_backends/backends/tests/test_events_router.py @@ -4,8 +4,9 @@ import datetime import json from copy import copy -from unittest.mock import MagicMock, call, patch, sentinel from json import JSONDecodeError +from unittest.mock import MagicMock, call, patch, sentinel + import ddt from django.conf import settings from django.test import TestCase, override_settings