Skip to content

Commit

Permalink
Relax checks on response status code
Browse files Browse the repository at this point in the history
Currently we require a 200 status code for events and a 202 for sessions
but there's no need to be so specific as any 2xx status indicates
success

Now we only warn on codes outside of the 2xx range, unless a specific
code has been set in `options['success']`. If a specific code is given
then it must match exactly, i.e. it has the same behaviour as before
  • Loading branch information
imjoehaines committed May 21, 2024
1 parent a3ee85e commit 5781014
Show file tree
Hide file tree
Showing 3 changed files with 92 additions and 8 deletions.
21 changes: 14 additions & 7 deletions bugsnag/delivery.py
Original file line number Diff line number Diff line change
Expand Up @@ -93,7 +93,6 @@ def deliver_sessions(self, config, payload: Any, options=None):
options = {}

options['endpoint'] = config.session_endpoint
options['success'] = 202

self.deliver(config, payload, options)

Expand Down Expand Up @@ -151,10 +150,14 @@ def request():
status = resp.getcode()

if 'success' in options:
success = options['success']
# if an expected status code has been given then it must match
# exactly with the actual status code
success = status == options['success']
else:
success = 200
if status != success:
# warn if we don't get a 2xx status code by default
success = status >= 200 and status < 300

if not success:
config.logger.warning(
'Delivery to %s failed, status %d' % (uri, status)
)
Expand Down Expand Up @@ -184,12 +187,16 @@ def request():

response = requests.post(uri, **req_options)
status = response.status_code

if 'success' in options:
success = options['success']
# if an expected status code has been given then it must match
# exactly with the actual status code
success = status == options['success']
else:
success = requests.codes.ok
# warn if we don't get a 2xx status code by default
success = status >= 200 and status < 300

if status != success:
if not success:
config.logger.warning(
'Delivery to %s failed, status %d' % (uri, status)
)
Expand Down
68 changes: 68 additions & 0 deletions tests/test_delivery.py
Original file line number Diff line number Diff line change
@@ -1,9 +1,12 @@
import pytest
import warnings
import sys
import logging
from unittest.mock import Mock

from bugsnag import Configuration
from bugsnag.delivery import (
Delivery,
UrllibDelivery,
RequestsDelivery,
create_default_delivery,
Expand Down Expand Up @@ -66,6 +69,71 @@ def test_misconfigured_sessions_endpoint_sends_warning(self):
self.assertEqual(1, len(warn))
self.assertEqual(0, len(self.server.events_received))

def test_does_not_warn_on_200_status_code(self):
delivery = Mock(wraps=create_default_delivery())
logger = Mock(spec=logging.Logger)

self.config.configure(delivery=delivery, logger=logger)

self.server.respond_to_next_request_with(200)

delivery.deliver(self.config, '{"apiKey":"aaab"}')

assert logger.warning.call_count == 0
assert len(self.server.events_received) == 1

def test_does_not_warn_on_202_status_code(self):
delivery = Mock(wraps=create_default_delivery())
logger = Mock(spec=logging.Logger)

self.config.configure(delivery=delivery, logger=logger)

self.server.respond_to_next_request_with(202)

delivery.deliver(self.config, '{"apiKey":"aaab"}')

print(logger.warning.assert_not_called())
assert logger.warning.call_count == 0
assert len(self.server.events_received) == 1

def test_warns_on_400_status_code_for_events(self):
delivery = Mock(wraps=create_default_delivery())
logger = Mock(spec=logging.Logger)

self.config.configure(delivery=delivery, logger=logger)

self.server.respond_to_next_request_with(300)

delivery.deliver(self.config, '{"apiKey":"aaab"}')

expected = 'Delivery to %s failed, status %d' % (
self.config.endpoint,
300
)

logger.warning.assert_called_once_with(expected)

assert len(self.server.events_received) == 1

def test_warns_on_300_status_code_for_sessions(self):
delivery = Mock(wraps=create_default_delivery())
logger = Mock(spec=logging.Logger)

self.config.configure(delivery=delivery, logger=logger)

self.server.respond_to_next_request_with(300)

delivery.deliver_sessions(self.config, '{"apiKey":"aaab"}')

expected = 'Delivery to %s failed, status %d' % (
self.config.session_endpoint,
300
)

logger.warning.assert_called_once_with(expected)

assert len(self.server.sessions_received) == 1

def test_it_calls_post_delivery_callback(self):
callback_was_called = False

Expand Down
11 changes: 10 additions & 1 deletion tests/utils.py
Original file line number Diff line number Diff line change
Expand Up @@ -81,6 +81,7 @@ def __init__(self, wait_for_duplicate_requests: bool):
self.sessions_received = []
self.paused = False
self.wait_for_duplicate_requests = wait_for_duplicate_requests
self._response_codes = []

class Handler(SimpleHTTPRequestHandler):
def do_POST(handler):
Expand Down Expand Up @@ -114,7 +115,12 @@ def do_POST(handler):
'unknown endpoint requested: ' + handler.path
)

handler.send_response(200)
try:
code = self._response_codes.pop()
except IndexError:
code = 200

handler.send_response(code)
handler.end_headers()

return ()
Expand Down Expand Up @@ -175,6 +181,9 @@ def sent_report_count(self) -> int:
def sent_session_count(self) -> int:
return len(self.sessions_received)

def respond_to_next_request_with(self, status_code: int) -> None:
self._response_codes.append(status_code)


class ScaryException(Exception):
class NestedScaryException(Exception):
Expand Down

0 comments on commit 5781014

Please sign in to comment.