diff --git a/CHANGELOG.rst b/CHANGELOG.rst index b8eabe16..f9009274 100644 --- a/CHANGELOG.rst +++ b/CHANGELOG.rst @@ -1,3 +1,9 @@ +3.0.0 (2015-XX-XX) +--------------------- +- Support passing in connect_timeout and timeout via _request_options to the Fido and Requests clients +- Timeout in HTTPFuture now defaults to None (wait indefinitely) instead of 5s. You should make sure + any calls to http_future.result(..) without a timeout are updated accordingly. + 2.1.0 (2015-07-20) --------------------- - Add warning for deprecated operations diff --git a/bravado/client.py b/bravado/client.py index fe0ace14..834f8194 100644 --- a/bravado/client.py +++ b/bravado/client.py @@ -174,8 +174,8 @@ def __getattr__(self, name): def construct_request(self, **op_kwargs): """ - :param op_kwargs: parameter name/value pairs to pass to the invocation - of the operation. + :param op_kwargs: parameter name/value pairs to passed to the + invocation of the operation. :return: request in dict form """ request_options = op_kwargs.pop('_request_options', {}) @@ -183,16 +183,22 @@ def construct_request(self, **op_kwargs): request = { 'method': self.operation.http_method.upper(), 'url': url, - 'params': {}, + 'params': {}, # filled in downstream 'headers': request_options.get('headers', {}), } + + # Copy over optional request options + for request_option in ('connect_timeout', 'timeout'): + if request_option in request_options: + request[request_option] = request_options[request_option] + self.construct_params(request, op_kwargs) return request def construct_params(self, request, op_kwargs): """ Given the parameters passed to the operation invocation, validates and - marshals the parmameters into the provided request dict. + marshals the parameters into the provided request dict. :type request: dict :param op_kwargs: the kwargs passed to the operation invocation @@ -225,10 +231,10 @@ def __call__(self, **op_kwargs): """ log.debug(u"%s(%s)" % (self.operation.operation_id, op_kwargs)) warn_for_deprecated_op(self.operation) - request = self.construct_request(**op_kwargs) + request_params = self.construct_request(**op_kwargs) def response_callback(response_adapter): return unmarshal_response(response_adapter, self) return self.operation.swagger_spec.http_client.request( - request, response_callback) + request_params, response_callback) diff --git a/bravado/fido_client.py b/bravado/fido_client.py index 55a4edae..5ddd7385 100644 --- a/bravado/fido_client.py +++ b/bravado/fido_client.py @@ -59,13 +59,19 @@ def request(self, request_params, response_callback=None): url = '%s?%s' % (request_params['url'], urllib_utf8.urlencode( request_params.get('params', []), True)) - request_params = { + fetch_kwargs = { 'method': str(request_params.get('method', 'GET')), 'body': stringify_body(request_params), 'headers': request_params.get('headers', {}), } - return HttpFuture(fido.fetch(url, **request_params), + for fetch_kwarg in ('connect_timeout', 'timeout'): + if fetch_kwarg in request_params: + fetch_kwargs[fetch_kwarg] = request_params[fetch_kwarg] + + concurrent_future = fido.fetch(url, **fetch_kwargs) + + return HttpFuture(concurrent_future, FidoResponseAdapter, response_callback) diff --git a/bravado/http_client.py b/bravado/http_client.py index 07c238be..08fd4844 100644 --- a/bravado/http_client.py +++ b/bravado/http_client.py @@ -9,7 +9,8 @@ class HttpClient(object): def request(self, request_params, response_callback=None): """ - :param request_params: complete request data. + :param request_params: complete request data. e.g. url, method, + headers, body, params, connect_timeout, timeout, etc. :type request_params: dict :param response_callback: Function to be called on response :type response_callback: method diff --git a/bravado/http_future.py b/bravado/http_future.py index 03208b18..90012aff 100644 --- a/bravado/http_future.py +++ b/bravado/http_future.py @@ -1,8 +1,6 @@ # -*- coding: utf-8 -*- from bravado.exception import HTTPError -DEFAULT_TIMEOUT_S = 5.0 - class HttpFuture(object): """A future which inputs HTTP params""" @@ -20,11 +18,12 @@ def __init__(self, future, response_adapter, callback): self.response_adapter = response_adapter self.response_callback = callback - def result(self, timeout=DEFAULT_TIMEOUT_S): + def result(self, timeout=None): """Blocking call to wait for API response - :param timeout: Timeout in seconds for which client will get blocked - to receive the response + :param timeout: Number of seconds to wait for a response. Defaults to + None which means wait indefinitely. + :type timeout: float :return: swagger response return value when given a callback or the http_response otherwise. """ diff --git a/bravado/requests_client.py b/bravado/requests_client.py index bbf3c6d8..fd81074e 100644 --- a/bravado/requests_client.py +++ b/bravado/requests_client.py @@ -93,6 +93,31 @@ def __init__(self): self.session = requests.Session() self.authenticator = None + @staticmethod + def separate_params(request_params): + """Splits the passed in dict of request_params into two buckets. + + - sanitized_params are valid kwargs for constructing a + requests.Request(..) + - misc_options are things like timeouts which can't be communicated + to the Requests library via the requests.Request(...) constructor. + + :param request_params: kitchen sink of request params. Treated as a + read-only dict. + :returns: tuple(sanitized_params, misc_options) + """ + sanitized_params = request_params.copy() + misc_options = {} + + if 'connect_timeout' in sanitized_params: + misc_options['connect_timeout'] = \ + sanitized_params.pop('connect_timeout') + + if 'timeout' in sanitized_params: + misc_options['timeout'] = sanitized_params.pop('timeout') + + return sanitized_params, misc_options + def request(self, request_params, response_callback=None): """ :param request_params: complete request data. @@ -101,8 +126,12 @@ def request(self, request_params, response_callback=None): :returns: HTTP Future object :rtype: :class: `bravado_core.http_future.HttpFuture` """ + sanitized_params, misc_options = self.separate_params(request_params) + requests_future = RequestsFutureAdapter( - self.session, self.authenticated_request(request_params)) + self.session, + self.authenticated_request(sanitized_params), + misc_options) return HttpFuture( requests_future, @@ -168,16 +197,22 @@ def json(self, **kwargs): class RequestsFutureAdapter(object): - """A future which inputs HTTP params""" + """Mimics a :class:`concurrent.futures.Future` for the purposes of making + HTTP calls with the Requests library in a future-y sort of way. + """ - def __init__(self, session, request): + def __init__(self, session, request, misc_options): """Kicks API call for Requests client :param session: session object to use for making the request :param request: dict containing API request parameters + :param misc_options: misc options to apply when sending a HTTP request. + e.g. timeout, connect_timeout, etc + :type misc_options: dict """ self.session = session self.request = request + self.misc_options = misc_options def check_for_exceptions(self, response): try: @@ -185,18 +220,63 @@ def check_for_exceptions(self, response): except Exception as e: add_response_detail_to_errors(e) - def result(self, timeout): + def build_timeout(self, result_timeout): + """ + Build the appropriate timeout object to pass to `session.send(...)` + based on connect_timeout, the timeout passed to the service call, and + the timeout passed to the result call. + + :param result_timeout: timeout that was passed into `future.result(..)` + :return: timeout + :rtype: float or tuple(connect_timeout, timeout) + """ + # The API provides two ways to pass a timeout :( We're stuck + # dealing with it until we're ready to make a non-backwards + # compatible change. + # + # - If both timeouts are the same, no problem + # - If either has a non-None value, use the non-None value + # - If both have a non-None value, use the greater of the two + timeout = None + has_service_timeout = 'timeout' in self.misc_options + service_timeout = self.misc_options.get('timeout') + + if not has_service_timeout: + timeout = result_timeout + elif service_timeout == result_timeout: + timeout = service_timeout + else: + if service_timeout is None: + timeout = result_timeout + elif result_timeout is None: + timeout = service_timeout + else: + timeout = max(service_timeout, result_timeout) + log.warn("Two different timeouts have been passed: " + "_request_options['timeout'] = {0} and " + "future.result(timeout={1}). Using timeout of {2}." + .format(service_timeout, result_timeout, timeout)) + + # Requests is weird in that if you want to specify a connect_timeout + # and idle timeout, then the timeout is passed as a tuple + if 'connect_timeout' in self.misc_options: + timeout = self.misc_options['connect_timeout'], timeout + return timeout + + def result(self, timeout=None): """Blocking call to wait for API response - :param timeout: timeout in seconds to wait for response - :type timeout: integer + :param timeout: timeout in seconds to wait for response. Defaults to + None to wait indefinitely. + :type timeout: float :return: raw response from the server :rtype: dict """ request = self.request + prepared_request = self.session.prepare_request(request) response = self.session.send( - self.session.prepare_request(request), - timeout=timeout) + prepared_request, + timeout=self.build_timeout(timeout)) self.check_for_exceptions(response) diff --git a/tests/fido_client/FidoClient/request_test.py b/tests/fido_client/FidoClient/request_test.py new file mode 100644 index 00000000..9e723c55 --- /dev/null +++ b/tests/fido_client/FidoClient/request_test.py @@ -0,0 +1,52 @@ +from mock import patch, call, Mock +import pytest +import six + + +try: + from bravado.fido_client import FidoClient +except ImportError: + FidoClient = Mock() + + +@pytest.mark.skipif(six.PY3, reason="twisted doesnt support py3 yet") +def test_no_timeouts_passed_to_fido(): + with patch('bravado.fido_client.fido.fetch') as fetch: + fido_client = FidoClient() + request_params = dict(url='http://foo.com/') + fido_client.request(request_params, response_callback=None) + assert fetch.call_args == call( + 'http://foo.com/?', body='', headers={}, method='GET') + + +@pytest.mark.skipif(six.PY3, reason="twisted doesnt support py3 yet") +def test_timeout_passed_to_fido(): + with patch('bravado.fido_client.fido.fetch') as fetch: + fido_client = FidoClient() + request_params = dict(url='http://foo.com/', timeout=1) + fido_client.request(request_params, response_callback=None) + assert fetch.call_args == call( + 'http://foo.com/?', body='', headers={}, method='GET', timeout=1) + + +@pytest.mark.skipif(six.PY3, reason="twisted doesnt support py3 yet") +def test_connect_timeout_passed_to_fido(): + with patch('bravado.fido_client.fido.fetch') as fetch: + fido_client = FidoClient() + request_params = dict(url='http://foo.com/', connect_timeout=1) + fido_client.request(request_params, response_callback=None) + assert fetch.call_args == call( + 'http://foo.com/?', body='', headers={}, method='GET', + connect_timeout=1) + + +@pytest.mark.skipif(six.PY3, reason="twisted doesnt support py3 yet") +def test_connect_timeout_and_timeout_passed_to_fido(): + with patch('bravado.fido_client.fido.fetch') as fetch: + fido_client = FidoClient() + request_params = dict(url='http://foo.com/', connect_timeout=1, + timeout=2) + fido_client.request(request_params, response_callback=None) + assert fetch.call_args == call( + 'http://foo.com/?', body='', headers={}, method='GET', + connect_timeout=1, timeout=2) diff --git a/tests/requests_client/RequestsClient/separate_params_test.py b/tests/requests_client/RequestsClient/separate_params_test.py new file mode 100644 index 00000000..64ce78da --- /dev/null +++ b/tests/requests_client/RequestsClient/separate_params_test.py @@ -0,0 +1,12 @@ +from bravado.requests_client import RequestsClient + + +def test_separate_params(): + request_params = { + 'url': 'http://foo.com', + 'connect_timeout': 1, + 'timeout': 2 + } + sanitized, misc = RequestsClient.separate_params(request_params) + assert sanitized == {'url': 'http://foo.com'} + assert misc == {'connect_timeout': 1, 'timeout': 2} diff --git a/tests/requests_client/RequestsFutureAdapter/build_timeout_test.py b/tests/requests_client/RequestsFutureAdapter/build_timeout_test.py new file mode 100644 index 00000000..83ab8e6c --- /dev/null +++ b/tests/requests_client/RequestsFutureAdapter/build_timeout_test.py @@ -0,0 +1,75 @@ +from mock import Mock +import pytest +from requests.sessions import Session + +from bravado.requests_client import RequestsFutureAdapter + + +@pytest.fixture +def request(): + return dict(url='http://foo.com') + + +@pytest.fixture +def session(): + return Mock(spec=Session) + + +def test_no_timeouts(session, request): + misc_options = {} + future = RequestsFutureAdapter(session, request, misc_options) + assert future.build_timeout(result_timeout=None) is None + + +def test_service_timeout_and_result_timeout_None(session, request): + misc_options = dict(timeout=1) + future = RequestsFutureAdapter(session, request, misc_options) + assert future.build_timeout(result_timeout=None) == 1 + + +def test_no_service_timeout_and_result_timeout_not_None(session, request): + misc_options = {} + future = RequestsFutureAdapter(session, request, misc_options) + assert future.build_timeout(result_timeout=1) == 1 + + +def test_service_timeout_lt_result_timeout(session, request): + misc_options = dict(timeout=10) + future = RequestsFutureAdapter(session, request, misc_options) + assert future.build_timeout(result_timeout=11) == 11 + + +def test_service_timeout_gt_result_timeout(session, request): + misc_options = dict(timeout=11) + future = RequestsFutureAdapter(session, request, misc_options) + assert future.build_timeout(result_timeout=10) == 11 + + +def test_service_timeout_None_result_timeout_not_None(session, request): + misc_options = dict(timeout=None) + future = RequestsFutureAdapter(session, request, misc_options) + assert future.build_timeout(result_timeout=10) == 10 + + +def test_service_timeout_not_None_result_timeout_None(session, request): + misc_options = dict(timeout=10) + future = RequestsFutureAdapter(session, request, misc_options) + assert future.build_timeout(result_timeout=None) == 10 + + +def test_both_timeouts_the_same(session, request): + misc_options = dict(timeout=10) + future = RequestsFutureAdapter(session, request, misc_options) + assert future.build_timeout(result_timeout=10) == 10 + + +def test_connect_timeout_and_idle_timeout(session, request): + misc_options = dict(connect_timeout=1, timeout=11) + future = RequestsFutureAdapter(session, request, misc_options) + assert future.build_timeout(result_timeout=None) == (1, 11) + + +def test_connect_timeout_only(session, request): + misc_options = dict(connect_timeout=1) + future = RequestsFutureAdapter(session, request, misc_options) + assert future.build_timeout(result_timeout=None) == (1, None)