Solidify transport interface #654

theacodes · 2016-09-20T23:07:05Z

These changes solidify the transport interface.

Add a transport compliance test (see note below). This is use to qualify transport implementations work as expected.
Bring httplib2 transport into compliance.
Make transport._request private.
Define the transport._request return value.
Remove all usage of transport.request in tests in favor of using authed_http directly, just as users would.

Note about compliance tests:
This is the first test in the module to use pytest fixtures as well as use pytest's test & assertion styles. We eventually want to move toward pytest style tests (I plan to do so as I refactor each module for #597), so I'm hoping this won't be controversial. If it is, I'm happy to switch to unittest-style, but I'll be keeping the extremely useful pytest-localserver fixture.

(Also I'm starting to see the light at the end of the tunnel here)

tests/transport/test_transport_compliance.py

+            http, self.server.url + '/basic')
+
+        assert response.status == http_client.OK
+        print(response.headers.keys())


tests/transport/test_transport_compliance.py

+        self.token = '{}{}'.format(self.token, '-new')
+
+
+class TransportComplianceTests(object):


tests/transport/test_transport_compliance.py

+    def test_connection_error(self):
+        http = self.transport.get_http_object()
+
+        with pytest.raises(Exception):


oauth2client/transport/httplib2.py

-def request(http, uri, method='GET', body=None, headers=None,
-            redirections=httplib2.DEFAULT_MAX_REDIRECTS,
-            **kwargs):
+class _ResponseWrapper(object):


tests/transport/test_httplib2_transport.py

-    redirections = 3
-
-    def test_with_request_attr(self):
+    def test_it(self):


oauth2client/client.py

            try:
-                d = json.loads(_helpers._from_bytes(content))
+                d = json.loads(_helpers._from_bytes(response.data))


oauth2client/client.py

            http, _GCE_METADATA_URI, headers=_GCE_HEADERS)
        return (
            response.status == http_client.OK and
-            response.get(_METADATA_FLAVOR_HEADER) == _DESIRED_METADATA_FLAVOR)
+            response.headers.get(
+                _METADATA_FLAVOR_HEADER) == _DESIRED_METADATA_FLAVOR)


oauth2client/client.py

            if 'error' in d:
                # you never know what those providers got to say
                error_msg = (str(d['error']) +
                             str(d.get('error_description', '')))
            else:
-                error_msg = 'Invalid response: {0}.'.format(str(resp.status))
+                error_msg = 'Invalid response: {0}.'.format(
+                    str(response.status))


tests/test_client.py

-        http = self.credentials.authorize(http)
-        transport.request(http, 'http://example.com')
+        authed_http = self.credentials.authorize(http)
+        authed_http = self.credentials.authorize(http)


tests/transport/test_transport_compliance.py

+
+        return flask.jsonify({
+            'authorization': flask.request.headers.get('authorization'),
+            'user_agent': flask.request.headers.get('user-agent')


tests/transport/test_transport_compliance.py

+class TransportComplianceTests(object):
+    """Test base class to confirm that a transport satisfies the interface
+    expected by oauth2client."""
+    transport = None


tests/transport/test_transport_compliance.py

+    @pytest.fixture(autouse=True)
+    def testserver(self):
+        """Provides a test HTTP server that is automatically created before
+        a test and destoryed at the end. This server is running the test


tests/transport/test_transport_compliance.py

+        response = self.transport._request(
+            authed_http, self.server.url + '/authorized')
+
+        content = json.loads(response.data.decode('utf-8'))


tests/transport/test_transport_compliance.py

+        content = json.loads(response.data.decode('utf-8'))
+
+        assert credentials.token == 'token'
+        assert content['authorization'] == 'Bearer {}'.format(


theacodes

Thanks for the review, @dhermes.

oauth2client/client.py

            try:
-                d = json.loads(_helpers._from_bytes(content))
+                d = json.loads(_helpers._from_bytes(response.data))


oauth2client/client.py

            http, _GCE_METADATA_URI, headers=_GCE_HEADERS)
        return (
            response.status == http_client.OK and
-            response.get(_METADATA_FLAVOR_HEADER) == _DESIRED_METADATA_FLAVOR)
+            response.headers.get(
+                _METADATA_FLAVOR_HEADER) == _DESIRED_METADATA_FLAVOR)


oauth2client/client.py

            if 'error' in d:
                # you never know what those providers got to say
                error_msg = (str(d['error']) +
                             str(d.get('error_description', '')))
            else:
-                error_msg = 'Invalid response: {0}.'.format(str(resp.status))
+                error_msg = 'Invalid response: {0}.'.format(
+                    str(response.status))


oauth2client/transport/__init__.py

+    """Makes an HTTP request.
+
+    Args:
+        http_object: The transport-specific http object to be used to make


oauth2client/transport/__init__.py

+        * headers: dict, the HTTP response headers.
+        * data: bytes, the HTTP response body.
+    """
+    return get_default_transport()._request(


tests/transport/test_transport_compliance.py

+    @pytest.fixture(autouse=True)
+    def testserver(self):
+        """Provides a test HTTP server that is automatically created before
+        a test and destoryed at the end. This server is running the test


tests/transport/test_transport_compliance.py

+            http, self.server.url + '/basic')
+
+        assert response.status == http_client.OK
+        print(response.headers.keys())


tests/transport/test_transport_compliance.py

+    def test_connection_error(self):
+        http = self.transport.get_http_object()
+
+        with pytest.raises(Exception):


tests/transport/test_transport_compliance.py

+        response = self.transport._request(
+            authed_http, self.server.url + '/authorized')
+
+        content = json.loads(response.data.decode('utf-8'))


tests/transport/test_transport_compliance.py

+        content = json.loads(response.data.decode('utf-8'))
+
+        assert credentials.token == 'token'
+        assert content['authorization'] == 'Bearer {}'.format(


tests/test_client.py

-        http = self.credentials.authorize(http)
-        transport.request(http, 'http://example.com')
+        authed_http = self.credentials.authorize(http)
+        # Double-wrap


theacodes · 2016-09-21T05:19:07Z

@dhermes it seems like the remainder of your concerns (save the one about exceptions) can be fixed by making transport.request public again. It's used throughout the package, but I don't not want users to use it directly. Are the concerns you have reason enough to make it public?

dhermes · 2016-09-21T05:24:22Z

Are the concerns you have reason enough to make it public?

Not sure.

theacodes · 2016-09-21T05:24:41Z

I'll let @nathanielmanistaatgoogle decide.

oauth2client/transport/httplib2.py

@@ -29,6 +29,9 @@
 _MAX_REFRESH_ATTEMPTS = 2


+exceptions = (httplib2.HttpLib2Error,)


theacodes · 2016-09-21T23:32:29Z

@nathanielmanistaatgoogle can you weigh in on the contentious topic on making transport.request private?

theacodes · 2016-09-26T16:58:23Z

@dhermes I've made request public again. I'm going to try to make the entire transport module private.

Jon Wayne Parrott added 9 commits September 20, 2016 15:23

Add basic transport compliance test

0a911ad

Bring httplib2 transport into compliance

00d1ec3

Add pytest-localserver as a test dependency

8565be7

Add transport compliant test for httplib2

bbf2b49

Fix library usage of transport.request

538ae72

Fix test usage of transport.request

1d1f7d5

Make transport.request private

d9926cd

Add make_authed_http tests

97a1076

Fix lint issues

ed09074

theacodes assigned dhermes and nathanielmanistaatgoogle Sep 20, 2016

googlebot added the cla: yes label Sep 20, 2016

dhermes reviewed Sep 20, 2016

View reviewed changes

dhermes suggested changes Sep 20, 2016

View reviewed changes

Address review comments

821e48e

theacodes commented Sep 21, 2016

View reviewed changes

dhermes reviewed Sep 21, 2016

View reviewed changes

Jon Wayne Parrott added 2 commits September 20, 2016 22:31

Add transport exceptions

dad21fc

Expand comment

f2aef40

dhermes suggested changes Sep 21, 2016

View reviewed changes

oauth2client/transport/httplib2.py

@@ -29,6 +29,9 @@

_MAX_REFRESH_ATTEMPTS = 2

exceptions = (httplib2.HttpLib2Error,)

This comment was marked as spam.

Sign in to view

This comment was marked as spam.

Sign in to view

Move transport exeptions into the tests

dfb014a

dhermes mentioned this pull request Sep 22, 2016

Drop casting of status codes to integers throughout client.py and elsewhere #656

Closed

Make request public

63c6a8f

dhermes approved these changes Sep 26, 2016

View reviewed changes

Fix lint

670c202

theacodes merged commit d4b3c97 into googleapis:transport-refactor Sep 26, 2016

theacodes deleted the tr-4-transport-test branch September 26, 2016 17:09

google-admin unassigned dhermes and nathanielmanistaatgoogle Sep 11, 2018

		self.token = '{}{}'.format(self.token, '-new')


		class TransportComplianceTests(object):

		@@ -29,6 +29,9 @@
		_MAX_REFRESH_ATTEMPTS = 2


		exceptions = (httplib2.HttpLib2Error,)

Solidify transport interface #654

Solidify transport interface #654

Conversation

theacodes commented Sep 20, 2016

This comment was marked as spam.

This comment was marked as spam.

This comment was marked as spam.

This comment was marked as spam.

This comment was marked as spam.

This comment was marked as spam.

This comment was marked as spam.

This comment was marked as spam.

This comment was marked as spam.

This comment was marked as spam.

This comment was marked as spam.

This comment was marked as spam.

This comment was marked as spam.

This comment was marked as spam.

This comment was marked as spam.

This comment was marked as spam.

This comment was marked as spam.

This comment was marked as spam.

This comment was marked as spam.

This comment was marked as spam.

This comment was marked as spam.

This comment was marked as spam.

This comment was marked as spam.

This comment was marked as spam.

This comment was marked as spam.

This comment was marked as spam.

This comment was marked as spam.

This comment was marked as spam.

This comment was marked as spam.

This comment was marked as spam.

This comment was marked as spam.

This comment was marked as spam.

This comment was marked as spam.

This comment was marked as spam.

This comment was marked as spam.

This comment was marked as spam.

This comment was marked as spam.

This comment was marked as spam.

This comment was marked as spam.

This comment was marked as spam.

This comment was marked as spam.

This comment was marked as spam.

This comment was marked as spam.

This comment was marked as spam.

This comment was marked as spam.

This comment was marked as spam.

This comment was marked as spam.

theacodes left a comment

Choose a reason for hiding this comment

This comment was marked as spam.

This comment was marked as spam.

This comment was marked as spam.

This comment was marked as spam.

This comment was marked as spam.

This comment was marked as spam.

This comment was marked as spam.

This comment was marked as spam.

This comment was marked as spam.

This comment was marked as spam.

This comment was marked as spam.

This comment was marked as spam.

This comment was marked as spam.

This comment was marked as spam.

theacodes commented Sep 21, 2016

dhermes commented Sep 21, 2016

theacodes commented Sep 21, 2016

This comment was marked as spam.

This comment was marked as spam.

theacodes commented Sep 21, 2016

theacodes commented Sep 26, 2016