From 3da453c48e41c341035c422a67168623323dacd3 Mon Sep 17 00:00:00 2001 From: Danny Hermes Date: Thu, 16 Feb 2017 16:41:57 -0800 Subject: [PATCH] Adding ability to send version info header on HTTP requests. Added an "extra headers" feature to enable this. I am not a fan of changing `Connection()` so haphazardly, but I hope to completely re-factor / destory `Connection()` in the near-term so I am less worried. This only adds the storage and datastore header info, for the purposes of a simple review. Once we agree on the approach, I can add support in the other subpackages. --- core/google/cloud/_http.py | 16 ++++++- core/unit_tests/test__http.py | 38 +++++++++++++++ datastore/google/cloud/datastore/_http.py | 5 ++ datastore/unit_tests/test__http.py | 57 +++++++++++++++++------ storage/google/cloud/storage/_http.py | 10 ++++ storage/unit_tests/test__http.py | 32 +++++++++++++ 6 files changed, 142 insertions(+), 16 deletions(-) diff --git a/core/google/cloud/_http.py b/core/google/cloud/_http.py index e5d6cd81b239..e1a481e581a7 100644 --- a/core/google/cloud/_http.py +++ b/core/google/cloud/_http.py @@ -15,7 +15,9 @@ """Shared implementation of connections to API servers.""" import json +import platform from pkg_resources import get_distribution + import six from six.moves.urllib.parse import urlencode @@ -29,6 +31,10 @@ get_distribution('google-cloud-core').version) """The user agent for google-cloud-python requests.""" +CLIENT_INFO_HEADER = 'X-Goog-API-Client' +CLIENT_INFO_TEMPLATE = ( + 'gl-python/' + platform.python_version() + ' gccl/{}') + class Connection(object): """A generic connection to Google Cloud Platform. @@ -38,6 +44,11 @@ class Connection(object): """ USER_AGENT = DEFAULT_USER_AGENT + _EXTRA_HEADERS = {} + """Headers to be sent with every request. + + Intended to be over-ridden by subclasses. + """ def __init__(self, client): self._client = client @@ -147,7 +158,9 @@ def _make_request(self, method, url, data=None, content_type=None, :param content_type: The proper MIME type of the data provided. :type headers: dict - :param headers: A dictionary of HTTP headers to send with the request. + :param headers: (Optional) A dictionary of HTTP headers to send with + the request. If passed, will be modified directly + here with added headers. :type target_object: object :param target_object: @@ -161,6 +174,7 @@ def _make_request(self, method, url, data=None, content_type=None, returned by :meth:`_do_request`. """ headers = headers or {} + headers.update(self._EXTRA_HEADERS) headers['Accept-Encoding'] = 'gzip' if data: diff --git a/core/unit_tests/test__http.py b/core/unit_tests/test__http.py index 28e44045f976..1226042b5859 100644 --- a/core/unit_tests/test__http.py +++ b/core/unit_tests/test__http.py @@ -287,6 +287,44 @@ def test_api_request_w_headers(self): } self.assertEqual(http._called_with['headers'], expected_headers) + def test_api_request_w_extra_headers(self): + from six.moves.urllib.parse import urlsplit + + http = _Http( + {'status': '200', 'content-type': 'application/json'}, + b'{}', + ) + client = mock.Mock(_http=http, spec=['_http']) + conn = self._make_mock_one(client) + conn._EXTRA_HEADERS = { + 'X-Baz': 'dax-quux', + 'X-Foo': 'not-bar', # Collision with ``headers``. + } + self.assertEqual( + conn.api_request('GET', '/', headers={'X-Foo': 'bar'}), {}) + self.assertEqual(http._called_with['method'], 'GET') + uri = http._called_with['uri'] + scheme, netloc, path, qs, _ = urlsplit(uri) + self.assertEqual('%s://%s' % (scheme, netloc), conn.API_BASE_URL) + # Intended to emulate self.mock_template + PATH = '/'.join([ + '', + 'mock', + conn.API_VERSION, + '', + ]) + self.assertEqual(path, PATH) + self.assertEqual(qs, '') + self.assertIsNone(http._called_with['body']) + expected_headers = { + 'Accept-Encoding': 'gzip', + 'Content-Length': '0', + 'User-Agent': conn.USER_AGENT, + 'X-Foo': 'not-bar', # The one passed-in is overridden. + 'X-Baz': 'dax-quux', + } + self.assertEqual(http._called_with['headers'], expected_headers) + def test_api_request_w_data(self): import json diff --git a/datastore/google/cloud/datastore/_http.py b/datastore/google/cloud/datastore/_http.py index 4c01f60c84d2..ee9e004b1b0d 100644 --- a/datastore/google/cloud/datastore/_http.py +++ b/datastore/google/cloud/datastore/_http.py @@ -16,6 +16,7 @@ import contextlib import os +from pkg_resources import get_distribution from google.rpc import status_pb2 @@ -61,6 +62,9 @@ _DISABLE_GRPC = os.getenv(DISABLE_GRPC, False) _USE_GRPC = _HAVE_GRPC and not _DISABLE_GRPC +_DATASTORE_DIST = get_distribution('google-cloud-datastore') +_CLIENT_INFO = connection_module.CLIENT_INFO_TEMPLATE.format( + _DATASTORE_DIST.version) class _DatastoreAPIOverHttp(object): @@ -102,6 +106,7 @@ def _request(self, project, method, data): 'Content-Type': 'application/x-protobuf', 'Content-Length': str(len(data)), 'User-Agent': self.connection.USER_AGENT, + connection_module.CLIENT_INFO_HEADER: _CLIENT_INFO, } headers, content = self.connection.http.request( uri=self.connection.build_api_url(project=project, method=method), diff --git a/datastore/unit_tests/test__http.py b/datastore/unit_tests/test__http.py index 7e47e7c2f65a..d50f297317cb 100644 --- a/datastore/unit_tests/test__http.py +++ b/datastore/unit_tests/test__http.py @@ -31,6 +31,9 @@ def _make_one(self, *args, **kw): return self._get_target_class()(*args, **kw) def test__rpc(self): + from google.cloud import _http as connection_module + from google.cloud.datastore._http import _CLIENT_INFO + class ReqPB(object): def SerializeToString(self): @@ -56,17 +59,24 @@ def FromString(cls, pb): self.assertIsInstance(response, RspPB) self.assertEqual(response._pb, 'CONTENT') called_with = http._called_with + self.assertEqual(len(called_with), 4) self.assertEqual(called_with['uri'], URI) self.assertEqual(called_with['method'], 'POST') - self.assertEqual(called_with['headers']['Content-Type'], - 'application/x-protobuf') - self.assertEqual(called_with['headers']['User-Agent'], - conn.USER_AGENT) + expected_headers = { + 'Content-Type': 'application/x-protobuf', + 'User-Agent': conn.USER_AGENT, + 'Content-Length': '5', + connection_module.CLIENT_INFO_HEADER: _CLIENT_INFO, + } + self.assertEqual(called_with['headers'], expected_headers) self.assertEqual(called_with['body'], REQPB) self.assertEqual(conn.build_kwargs, [{'method': METHOD, 'project': PROJECT}]) def test__request_w_200(self): + from google.cloud import _http as connection_module + from google.cloud.datastore._http import _CLIENT_INFO + PROJECT = 'PROJECT' METHOD = 'METHOD' DATA = b'DATA' @@ -77,12 +87,16 @@ def test__request_w_200(self): self.assertEqual(datastore_api._request(PROJECT, METHOD, DATA), 'CONTENT') called_with = http._called_with + self.assertEqual(len(called_with), 4) self.assertEqual(called_with['uri'], URI) self.assertEqual(called_with['method'], 'POST') - self.assertEqual(called_with['headers']['Content-Type'], - 'application/x-protobuf') - self.assertEqual(called_with['headers']['User-Agent'], - conn.USER_AGENT) + expected_headers = { + 'Content-Type': 'application/x-protobuf', + 'User-Agent': conn.USER_AGENT, + 'Content-Length': '4', + connection_module.CLIENT_INFO_HEADER: _CLIENT_INFO, + } + self.assertEqual(called_with['headers'], expected_headers) self.assertEqual(called_with['body'], DATA) self.assertEqual(conn.build_kwargs, [{'method': METHOD, 'project': PROJECT}]) @@ -386,12 +400,18 @@ def _make_one(self, client, use_grpc=False): return self._get_target_class()(client) def _verifyProtobufCall(self, called_with, URI, conn): + from google.cloud import _http as connection_module + from google.cloud.datastore._http import _CLIENT_INFO + self.assertEqual(called_with['uri'], URI) self.assertEqual(called_with['method'], 'POST') - self.assertEqual(called_with['headers']['Content-Type'], - 'application/x-protobuf') - self.assertEqual(called_with['headers']['User-Agent'], - conn.USER_AGENT) + expected_headers = { + 'Content-Type': 'application/x-protobuf', + 'User-Agent': conn.USER_AGENT, + 'Content-Length': str(len(called_with['body'])), + connection_module.CLIENT_INFO_HEADER: _CLIENT_INFO, + } + self.assertEqual(called_with['headers'], expected_headers) def test_default_url(self): klass = self._get_target_class() @@ -681,6 +701,9 @@ def test_lookup_multiple_keys_w_missing(self): def test_lookup_multiple_keys_w_deferred(self): from google.cloud.grpc.datastore.v1 import datastore_pb2 + from google.cloud import _http as connection_module + from google.cloud.datastore._http import _CLIENT_INFO + PROJECT = 'PROJECT' key_pb1 = self._make_key_pb(PROJECT) key_pb2 = self._make_key_pb(PROJECT, id_=2345) @@ -704,9 +727,13 @@ def test_lookup_multiple_keys_w_deferred(self): self._verifyProtobufCall(cw, URI, conn) self.assertEqual(cw['uri'], URI) self.assertEqual(cw['method'], 'POST') - self.assertEqual(cw['headers']['Content-Type'], - 'application/x-protobuf') - self.assertEqual(cw['headers']['User-Agent'], conn.USER_AGENT) + expected_headers = { + 'Content-Type': 'application/x-protobuf', + 'User-Agent': conn.USER_AGENT, + 'Content-Length': '48', + connection_module.CLIENT_INFO_HEADER: _CLIENT_INFO, + } + self.assertEqual(cw['headers'], expected_headers) rq_class = datastore_pb2.LookupRequest request = rq_class() request.ParseFromString(cw['body']) diff --git a/storage/google/cloud/storage/_http.py b/storage/google/cloud/storage/_http.py index 5137082f955d..a1f5f9497dd7 100644 --- a/storage/google/cloud/storage/_http.py +++ b/storage/google/cloud/storage/_http.py @@ -14,9 +14,15 @@ """Create / interact with Google Cloud Storage connections.""" +from pkg_resources import get_distribution + from google.cloud import _http +_STORAGE_DIST = get_distribution('google-cloud-storage') +_CLIENT_INFO = _http.CLIENT_INFO_TEMPLATE.format(_STORAGE_DIST.version) + + class Connection(_http.JSONConnection): """A connection to Google Cloud Storage via the JSON REST API. @@ -32,3 +38,7 @@ class Connection(_http.JSONConnection): API_URL_TEMPLATE = '{api_base_url}/storage/{api_version}{path}' """A template for the URL of a particular API call.""" + + _EXTRA_HEADERS = { + _http.CLIENT_INFO_HEADER: _CLIENT_INFO, + } diff --git a/storage/unit_tests/test__http.py b/storage/unit_tests/test__http.py index ca9bde20cbd3..cb9344a16389 100644 --- a/storage/unit_tests/test__http.py +++ b/storage/unit_tests/test__http.py @@ -14,6 +14,8 @@ import unittest +import mock + class TestConnection(unittest.TestCase): @@ -26,6 +28,36 @@ def _get_target_class(): def _make_one(self, *args, **kw): return self._get_target_class()(*args, **kw) + def test_extra_headers(self): + from google.cloud import _http as base_http + from google.cloud.storage import _http as MUT + + http = mock.Mock(spec=['request']) + response = mock.Mock(status=200, spec=['status']) + data = b'brent-spiner' + http.request.return_value = response, data + client = mock.Mock(_http=http, spec=['_http']) + + conn = self._make_one(client) + req_data = 'hey-yoooouuuuu-guuuuuyyssss' + result = conn.api_request( + 'GET', '/rainbow', data=req_data, expect_json=False) + self.assertEqual(result, data) + + expected_headers = { + 'Content-Length': str(len(req_data)), + 'Accept-Encoding': 'gzip', + base_http.CLIENT_INFO_HEADER: MUT._CLIENT_INFO, + 'User-Agent': conn.USER_AGENT, + } + expected_uri = conn.build_api_url('/rainbow') + http.request.assert_called_once_with( + body=req_data, + headers=expected_headers, + method='GET', + uri=expected_uri, + ) + def test_build_api_url_no_extra_query_params(self): conn = self._make_one(object()) URI = '/'.join([