Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

fix: catch json errors on empty responses from ralph #461

Merged
merged 9 commits into from
Oct 15, 2024
18 changes: 17 additions & 1 deletion event_routing_backends/backends/tests/test_events_router.py
Original file line number Diff line number Diff line change
Expand Up @@ -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
Expand Down Expand Up @@ -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
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

These aren't 409s are they?

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
Expand Down
10 changes: 9 additions & 1 deletion event_routing_backends/utils/xapi_lrs_client.py
Original file line number Diff line number Diff line change
@@ -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
Expand Down Expand Up @@ -71,8 +72,15 @@ def bulk_send(self, statement_data):
requests.Response object
"""
logger.debug('Sending {} xAPI statements to {}'.format(len(statement_data), self.URL))
response = None

response = self.lrs_client.save_statements(statement_data)
try:
response = self.lrs_client.save_statements(statement_data)
except JSONDecodeError:
logger.warning(f"Events already in LRS: {statement_data}")
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This error can be triggered for any JSON issues. Since we have no ability to tell the difference between a real error and a spurious 204, maybe the message can be "JSON Decode Error, this may indicate that all sent events are already stored."?


if not response:
return

if not response.success:
if response.response.code == 409:
Expand Down
Loading