Skip to content
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.

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

Use requests for HTTP/HTTPS calls in library #240

Merged
merged 8 commits into from
Aug 7, 2020
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension


Conversations
Failed to load comments.
Jump to
Jump to file
Failed to load files.
Diff view
Diff view
1 change: 1 addition & 0 deletions setup.py
Original file line number Diff line number Diff line change
Expand Up @@ -216,6 +216,7 @@ def read(fname):
'PyYAML',
'zeroc-ice>=3.6.4,<3.7',
'pywin32; platform_system=="Windows"',
'requests'
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Are you intentionally keeping pyopenssl as an optional dependency? It's not installed by default:
https://github.com/psf/requests/blob/2d39c0db054e158767ab4a755476844fe40787e7/setup.py#L105

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yes, definitely intentional. Don't want to force it on people if they don't need/want it.

],
tests_require=[
'pytest',
Expand Down
17 changes: 12 additions & 5 deletions src/omero/plugins/import.py
Original file line number Diff line number Diff line change
Expand Up @@ -35,7 +35,7 @@
import csv
import sys
import shlex
from urllib.request import urlopen
import requests
from zipfile import ZipFile


Expand Down Expand Up @@ -312,8 +312,14 @@ def add_python_argument(*args, **kwargs):
for name, help in (
("bulk", "Bulk YAML file for driving multiple imports"),
("logprefix", "Directory or file prefix for --file and --errs"),
("file", "File for storing the standard output from the Java process"),
("errs", "File for storing the standard error from the Java process")
(
"file",
"File for storing the standard output from the Java process"
),
(
"errs",
"File for storing the standard error from the Java process"
)
):
add_python_argument("--%s" % name, nargs="?", help=help)
add_python_argument("---%s" % name, nargs="?", help=SUPPRESS)
Expand Down Expand Up @@ -575,8 +581,8 @@ def download_omero_java(self, version):
jars_dir, omero_java_txt = self._userdir_jars(parentonly=True)
if not jars_dir.exists():
jars_dir.mkdir()
with urlopen(omero_java_zip) as resp:
with ZipFile(BytesIO(resp.read())) as zipfile:
with requests.get(omero_java_zip) as resp:
with ZipFile(BytesIO(resp.content)) as zipfile:
topdirs = set(f.filename.split(
os.path.sep)[0] for f in zipfile.filelist if f.is_dir())
if len(topdirs) != 1:
Expand Down Expand Up @@ -767,6 +773,7 @@ def parse_csv(self, path, delimiter=","):
class TestEngine(ImportControl):
COMMAND = [TEST_CLASS]


try:
register("import", ImportControl, HELP, epilog=EXAMPLES)
register("testengine", TestEngine, TESTHELP)
Expand Down
35 changes: 13 additions & 22 deletions src/omero/util/upgrade_check.py
Original file line number Diff line number Diff line change
Expand Up @@ -6,16 +6,14 @@
"""

from future import standard_library
standard_library.install_aliases()
from builtins import str
from builtins import object
from omero_version import omero_version

import platform
import logging
import urllib.request, urllib.error, urllib.parse
import urllib.request, urllib.parse, urllib.error
import socket
import requests
standard_library.install_aliases()
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@joshmoore Can we get rid of this? Sounds like it's only required for Python 2.7 support https://python-future.org/quickstart.html#to-convert-existing-python-3-code

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@manics: Thought about doing that here but wanted to keep the changes to what were required to do the job. I saw some other weird stuff like this when doing my review of urllib usage:

try:
from urllib.parse import quote, unquote
except ImportError:
# Python2
from urllib.parse import quote, unquote

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Can we get rid of this?

Don't see why not.



class UpgradeCheck(object):
Expand Down Expand Up @@ -47,8 +45,8 @@ class UpgradeCheck(object):
"""

#
# Default timeout is 3 seconds.
# * http://docs.python.org/2/library/socket.html#socket.setdefaulttimeout
# our default timeout to use for requests; package default is no timeout
# * https://requests.readthedocs.io/en/master/user/quickstart/#timeouts
#
DEFAULT_TIMEOUT = 6.0

Expand Down Expand Up @@ -102,7 +100,7 @@ def getOSVersion(self):
platform.mac_ver()[0])
else:
version = platform.platform()
except:
except Exception:
version = platform.platform()
return version

Expand All @@ -129,20 +127,13 @@ def run(self):
params["python.version"] = platform.python_version()
params["python.compiler"] = platform.python_compiler()
params["python.build"] = platform.python_build()
params = urllib.parse.urlencode(params)

old_timeout = socket.getdefaulttimeout()
try:
socket.setdefaulttimeout(self.timeout)
full_url = "%s?%s" % (self.url, params)
request = urllib.request.Request(full_url)
request.add_header('User-Agent', self.agent)
self.log.debug("Attempting to connect to %s" % full_url)
response = urllib.request.urlopen(request)
result = response.read()
finally:
socket.setdefaulttimeout(old_timeout)

headers = {'User-Agent': self.agent}
request = requests.get(
self.url, headers=headers, params=params,
timeout=self.DEFAULT_TIMEOUT
)
result = request.text
except Exception as e:
self.log.error(str(e), exc_info=0)
self._set(None, e)
Expand All @@ -152,5 +143,5 @@ def run(self):
self.log.info("no update needed")
self._set(None, None)
else:
self.log.warn("UPGRADE AVAILABLE:" + result.decode('utf-8'))
self._set(result.decode('utf-8'), None)
self.log.warn("UPGRADE AVAILABLE:" + result)
self._set(result, None)