Skip to content

googleapiclient.http: guard when importing ssl #220

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom
Merged
nathanielmanistaatgoogle merged 1 commit into from
Apr 28, 2016
Merged

googleapiclient.http: guard when importing ssl #220

nathanielmanistaatgoogle merged 1 commit into from
Apr 28, 2016

Conversation

@rctay
Copy link
Contributor

@rctay rctay commented Apr 20, 2016

Fix #191 by adding a guard when importing ssl, and replace all direct
references to SSLError (the sole member of ssl being used) with a
shim, _ssl_SSLError. It is prefixed with a '_' to caution users
against relying on the name when importing googleclient.http.

httplib2 also takes the same approach (see ssl_SSLError in
httplib2/__init__.py), but instead of using None when faking
SSLError, we provide a 'real' class, so that code can continue to
trigger the exception handler for SSLError in googleclient.http (eg.
in tests, SSLError's are raised). If the shim _ssl_SSLError were
None, an exception handler for it would not catch anything, as None
is not a class and we could never throw anything that could be
"compatible" with None [1]

[1] https://docs.python.org/2/reference/compound_stmts.html#except

@googlebot googlebot added the cla: yes This human has signed the Contributor License Agreement. label Apr 20, 2016
@theacodes
Copy link
Contributor

theacodes commented Apr 20, 2016

What situations are you in where ssl isn't available? It's part of the standard library since 2.6.

@rctay rctay mentioned this pull request Apr 20, 2016
Closed
@theacodes
Copy link
Contributor

theacodes commented Apr 20, 2016

Ah okay, I see, I read the issue.

So the issue is that App Engine doesn't have the SSL module available by default.

@theacodes
Copy link
Contributor

theacodes commented Apr 20, 2016

@nathanielmanistaatgoogle I am on the fence about App Engine-specific changes. WDYT?

@rctay
Copy link
Contributor Author

rctay commented Apr 20, 2016

@jonparrott Thanks for looking. Perhaps we can continue the thread at #191?

@theacodes
Copy link
Contributor

theacodes commented Apr 20, 2016

Nah, if @nathanielmanistaatgoogle is cool with this we can merge. :)

@rctay
Copy link
Contributor Author

rctay commented Apr 20, 2016

By the way, I have ran the tests with tox for py27-oauth2client1/2 and they remain working. I also tested on App Engine, by listing a Cloud Storage bucket:

from apiclient.discovery import build
from google.appengine.api import app_identity
from oauth2client.contrib.appengine import AppAssertionCredentials

import httplib2
import webapp2

class MyHandler(webapp2.ResponseHandler):
  def get(self):
    credentials = AppAssertionCredentials('https://www.googleapis.com/auth/devstorage.read_write')
    http_auth = credentials.authorize(httplib2.Http())
    service = build('storage', 'v1', http=http_auth)
    res = service.objects().list(bucket=app_identity.get_default_gcs_bucket_name()).execute()
    self.response.write(repr(res))

nathanielmanistaatgoogle
nathanielmanistaatgoogle reviewed Apr 25, 2016
View reviewed changes
googleapiclient/http.py Outdated
# Engine.
library = ''
reason = ''

This comment was marked as spam.

Sign in to view

@nathanielmanistaatgoogle
Copy link
Contributor

nathanielmanistaatgoogle commented Apr 25, 2016

@jonparrott: My pride is wounded as advertised.

@theacodes
Copy link
Contributor

theacodes commented Apr 25, 2016 •
edited
Loading

@nathanielmanistaatgoogle FWIW ssl library available by default should hopefully happen this quarter, but no promises.

@theacodes
Copy link
Contributor

theacodes commented Apr 25, 2016

With that in mind I agree the absence of the ssl module should not have compatibility. ;)

@rctay rctay force-pushed the rc/191-ssl-gae branch 2 times, most recently from 0b32ed1 to 77f89b0 Compare April 26, 2016 16:49
@rctay
Copy link
Contributor Author

rctay commented Apr 26, 2016 •
edited
Loading

@nathanielmanistaatgoogle Sorry to have wounded it. Dropped the blank line and made accesses to library and reason fail. Hopefully it's loud enough and hope this makes your wound hurt less!

nathanielmanistaatgoogle
nathanielmanistaatgoogle reviewed Apr 26, 2016
View reviewed changes
tests/test_http.py
else:
self.num_errors -= 1
if self.num_errors == 1:
raise ssl.SSLError()

This comment was marked as spam.

Sign in to view

@rctay
Copy link
Contributor Author

rctay commented Apr 27, 2016

You're right, we don't run tests on App Engine, or any environment without ssl for that matter. I have dropped the changes to the test infrastructure, as well as omit implementing SSLError.

@nathanielmanistaatgoogle
Copy link
Contributor

nathanielmanistaatgoogle commented Apr 27, 2016

@jonparrott: please take another look at this? I feel like where I've directed this change is correct, but also pretty radical. Any qualms? Also, any desire for a comment in the clearly weird code? Should we open an issue for removing this as soon as possible?

@theacodes
Copy link
Contributor

theacodes commented Apr 27, 2016

@nathanielmanistaatgoogle This LGTM and I'm okay with it. I agree we should file a bug to remove this once ssl is on by default in GAE.

nathanielmanistaatgoogle
nathanielmanistaatgoogle reviewed Apr 27, 2016
View reviewed changes
googleapiclient/http.py
import uuid


try:

This comment was marked as spam.

Sign in to view

@rctay
googleapiclient.http: guard when importing ssl
3146c92 
Fix googleapis#191 by adding a guard when importing `ssl`, and replace all direct
references to `SSLError` (the sole member of `ssl` being used) with a
shim, `_ssl_SSLError`. It is prefixed with a '_' to caution users
against relying on the name when importing `googleclient.http`.

`httplib2` also takes a similar approach (see `ssl_SSLError` in
`httplib2/__init__.py`).
@rctay
Copy link
Contributor Author

rctay commented Apr 28, 2016 •
edited
Loading

Done! Added the todo wrt #221.

@nathanielmanistaatgoogle nathanielmanistaatgoogle merged commit 6d9ba51 into googleapis:master Apr 28, 2016
akrherz pushed a commit to akrherz/google-api-python-client that referenced this pull request Apr 1, 2019
@dhermes
Merge pull request googleapis#220 from bigblind/ignore-pycharm
5b1b51e 
Ignore PyCharm config files
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

No reviews

Assignees

No one assigned

Labels

cla: yes This human has signed the Contributor License Agreement.

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

ImportError: No module named _ssl on App Engine since 1.5.0

4 participants

@rctay @theacodes @nathanielmanistaatgoogle @googlebot