greenwave

Clone

#495 Handle requests exceptions
Merged 4 years ago by vmaljulin. Opened 4 years ago by vmaljulin.
vmaljulin/greenwave FACTORY-5053  into  master

file modified
+30 -3 
@@ -1,16 +1,43 @@ 
 

  import requests
 

  

+ from json import dumps
 

  from requests.adapters import HTTPAdapter
 

- # pylint: disable=import-error
 

- from requests.packages.urllib3.util.retry import Retry
 

+ from requests.exceptions import ConnectionError, ConnectTimeout, RetryError
 

+ from urllib3.util.retry import Retry
 

+ from urllib3.exceptions import ProxyError, SSLError
 

  

  from greenwave import __version__
 

  

  

+ class ErrorResponse(requests.Response):
 

+     def __init__(self, status_code, error_message, url):
 

+         super().__init__()
 

+         self.status_code = status_code
 

+         self._error_message = error_message
 

+         self.url = url
 

+         self.reason = error_message.encode()
 

+ 

+     @property
 

+     def content(self):
 

+         return dumps({'message' : self._error_message}).encode()
 

+ 

+ 

+ class RequestsSession(requests.Session):
 

+     def request(self, *args, **kwargs):
 

+         req_url = kwargs.get('url', args[1])
 

+         try:
 

+             return super().request(*args, **kwargs)
 

+         except (ConnectionError, ProxyError, SSLError) as e:
 

+             ret_val = ErrorResponse(502, str(e), req_url)
 

+         except (ConnectTimeout, RetryError) as e:
 

+             ret_val = ErrorResponse(504, str(e), req_url)
 

+         return ret_val
 

+ 

+ 

  def get_requests_session():
 

      """ Get http(s) session for request processing.  """
 

  

-     session = requests.Session()
 

+     session = RequestsSession()
 

      retry = Retry(
 

          total=3,
 

          read=3,

file added
+16 
@@ -0,0 +1,16 @@ 
 

+ # SPDX-License-Identifier: GPL-2.0+
 

+ 

+ from mock import patch
 

+ from json import loads
 

+ from greenwave.request_session import get_requests_session
 

+ from requests.exceptions import ConnectionError
 

+ 

+ 

+ @patch('requests.adapters.HTTPAdapter.send')
 

+ def test_retry_handler(mocked_request):
 

+     msg_text = 'It happens...'
 

+     mocked_request.side_effect = ConnectionError(msg_text)
 

+     session = get_requests_session()
 

+     resp = session.get('http://localhost.localdomain')
 

+     assert resp.status_code == 502
 

+     assert loads(resp.content) == {'message': msg_text}

file modified
+22 
@@ -1,6 +1,7 @@ 
 

  # SPDX-License-Identifier: GPL-2.0+
 

  

  import socket
 

+ from requests.exceptions import ConnectionError, HTTPError
 

  

  import pytest
 

  import mock
 
@@ -96,6 +97,27 @@ 
 

              assert session.request.mock_calls == [expected_call]
 

  

  

+ def test_retrieve_yaml_remote_rule_connection_error():
 

+     app = greenwave.app_factory.create_app()
 

+     with app.app_context():
 

+         with mock.patch('requests.Session.request') as mocked_request:
 

+             # Return 404, because we are only interested in the URL in the request
 

+             # and whether it is correct even with empty namespace.
 

+             response = mock.MagicMock()
 

+             response.status_code = 200
 

+             mocked_request.side_effect = [
 

+                 response, ConnectionError('Something went terribly wrong...')
 

+             ]
 

+ 

+             with pytest.raises(HTTPError) as excinfo:
 

+                 retrieve_yaml_remote_rule("deadbeaf", "pkg", "")
 

+ 

+             assert str(excinfo.value) == (
 

+                 '502 Server Error: Something went terribly wrong... for url: '
 

+                 'https://src.fedoraproject.org/pkg/raw/deadbeaf/f/gating.yaml'
 

+             )
 

+ 

+ 

  @mock.patch('greenwave.resources.xmlrpc.client.ServerProxy')
 

  def test_retrieve_scm_from_koji_build_socket_error(mock_xmlrpc_client):
 

      mock_auth_server = mock_xmlrpc_client.return_value

file modified
+1 -4 
@@ -18,10 +18,7 @@ 
 

      (ConnectionError('ERROR'), 502, 'ERROR'),
 

      (ConnectTimeout('TIMEOUT'), 502, 'TIMEOUT'),
 

      (Timeout('TIMEOUT'), 504, 'TIMEOUT'),
 

-     (InternalServerError(), 500, 'The server encountered an internal error'),
 

-     (urllib3.exceptions.MaxRetryError(
 

-         'MAX_RETRY', '.../gating.yaml'), 502, ('There was an error retrieving the '
 

-                                                'gating.yaml file at .../gating.yaml'))

Can you write a test for this? I.e. if there is a retry failure when fetching gating.yaml, a human readable error should be passed to client (there was a bug report for this in past).

 
+     (InternalServerError(), 500, 'The server encountered an internal error')
 

  ])
 

  def test_json_connection_error(error, expected_status_code,
 

                                 expected_error_message_part):

file modified
-7 
@@ -35,13 +35,6 @@ 
 

          current_app.logger.exception('Timeout error: {}'.format(error))
 

          msg = 'Timeout connecting to upstream server: {}'.format(error)
 

          status_code = 504
 

-     elif isinstance(error, urllib3.exceptions.MaxRetryError):
 

-         current_app.logger.exception('Connection error: {}'.format(error))
 

-         if error.url.endswith('gating.yaml'):
 

-             msg = 'There was an error retrieving the gating.yaml file at {}'.format(error.url)
 

-         else:
 

-             msg = 'Error connecting to {}'.format(error.url)
 

-         status_code = 502
 

      else:
 

          current_app.logger.exception('Unexpected server error: {}'.format(error))
 

          msg = 'Server encountered unexpected error'

Signed-off-by: Valerij Maljulin vmaljuli@redhat.com

rebased onto ffe76fc3d4e5bb86b32be9e47d189e786ff7b36c
4 years ago

rebased onto 65b3ea397e63f48bb7ed81433f893d3273d519e0
4 years ago

rebased onto 1297b34d6feac1e7738ed8f27f2563831d8d9286
4 years ago

Why not just override request() here? At least I believe get() and post() call it.

Can you write a test for this? I.e. if there is a retry failure when fetching gating.yaml, a human readable error should be passed to client (there was a bug report for this in past).

rebased onto 9a5646787cae7665834286bae6e665e67cfb3b77
4 years ago

Why not just override request() here? At least I believe get() and post() call it.

Good point, thx

Can you write a test for this? I.e. if there is a retry failure when fetching gating.yaml, a human readable error should be passed to client (there was a bug report for this in past).

Done

Can you override directly? The use of a wrapper function seems unnecessary.

Remove redundant line.

Single underscore prefix means that the variable is protected -- should be only set in subclasses of the response.

Single underscore prefix means that the variable is protected -- should be only set in subclasses of the response.

I know that. But I didn't find any other way to set a response content. Because content property doesn't have a setter.

rebased onto 31e5baa6f96306dbac8bb4f84dd979c64c1ea28f
4 years ago

Can you override directly? The use of a wrapper function seems unnecessary.

Done

I know that. But I didn't find any other way to set a response content. Because content property doesn't have a setter.

How about just create a new response class as mentioned here? Or subclass Response and override content property.

Or just move the whole session requests business into simple request function -- on most places it's handled the same way:
- make a request
- raise for status
- get JSON data

rebased onto e8140dd7dbd3c21cc1591adf8763752521e2215f
4 years ago

rebased onto 77cb0f15343f8a9649317b854d9dad5372eea96a
4 years ago

I know that. But I didn't find any other way to set a response content. Because content property doesn't have a setter.

How about just create a new response class as mentioned here? Or subclass Response and override content property.
Or just move the whole session requests business into simple request function -- on most places it's handled the same way:
- make a request
- raise for status
- get JSON data

Created a subclass

rebased onto 01a63be89d44a3fe2169cc00b8a1524ae87748af
4 years ago

You can write this also as:
url_arg = kwargs.get('url', args[1])

But are we sure args[1] is always there? I'm worried that IndexError could be raised here.

rebased onto 401a778bd7229cb69157d097b7238da4cf2bd96d
4 years ago

You can write this also as:
url_arg = kwargs.get('url', args[1])
But are we sure args[1] is always there? I'm worried that IndexError could be raised here.

Good point. Thank you. Added.
There should be as long as 'url' is a mandatory argument in request function. So if this argument is missed TypeError will be thrown normally.

Awesome, thank you!
The PR looks good to me +1
Let's wait for @lholecek final review, then it's good to go.

I liked the generic response from your previous patch better. This submodule shouldn't know about any details from other submodules.

If there needs to be a custom error message passed to client, it should be handled by the code which requests the gating.yaml.

Shouldn't this also include urllib3.exceptions.MaxRetryError?

I noticed some of these exceptions you're handling are handled in json_error: https://pagure.io/greenwave/blob/master/f/greenwave/utils.py#_30

Any reason why those are kept there or why you are moving away from this?

1 new commit added

  • fixup! Handle requests exceptions This fixes #479
4 years ago

Do you think it would work better to override the property in this subclass? I'm bit skeptical that _content won't break in some future requests releases (I don't think it's a documented attribute).

@property
def content(self):
    return self.__error_message

Shouldn't this also include urllib3.exceptions.MaxRetryError?

MaxRetryError is always being reraised by one of those five errors. See requests/adapters.py line 500. The logic here is that MaxRetryError is not an error itself. It is always caused by something else (like we've reached MaxRetryError cause we were unable to connect).

Probably better not to check these calls. The details/arguments can change often, forcing us to update tests.

I noticed some of these exceptions you're handling are handled in json_error: https://pagure.io/greenwave/blob/master/f/greenwave/utils.py#_30
Any reason why those are kept there or why you are moving away from this?

Only one exception is the same there. It's a ConnectionError. Maybe I could remove it safely but I'm not sure if it wouldn't also cover something else. It always better to have two exception handlers instead of none.

2 new commits added

  • fixup! Handle requests exceptions This fixes #479
  • Handle requests exceptions
4 years ago

2 new commits added

  • fixup! Handle requests exceptions This fixes #479
  • Handle requests exceptions
4 years ago

Probably better not to check these calls. The details/arguments can change often, forcing us to update tests.

Removed

Do you think it would work better to override the property in this subclass? I'm bit skeptical that _content won't break in some future requests releases (I don't think it's a documented attribute).
@property
def content(self):
return self.__error_message

Done

This is no longer covered by tests and I expect it's now handled in greenwave/request_session.py. Is that correct? If so, remove this conditional branch.

2 new commits added

  • fixup! Handle requests exceptions This fixes #479
  • Handle requests exceptions
4 years ago

+1

rebased onto 647680f
4 years ago

Pull-Request has been merged by vmaljulin
4 years ago
Metadata
None
No Tags
Changes Summary 5
+30 -3
file changed
greenwave/request_session.py
+16
file added
greenwave/tests/test_request_session.py
+22 -0
file changed
greenwave/tests/test_retrieve_gating_yaml.py
+1 -4
file changed
greenwave/tests/test_utils.py
+0 -7
file changed
greenwave/utils.py
Powered by Pagure 5.13.3
Documentation • File an Issue • About • SSH Hostkey/Fingerprint
© Red Hat, Inc. and others.