Add generic HTTP and HTTPS streaming support. #107

robottwo · 2017-02-28T19:42:50Z

Adds support for opening vanilla HTTP and HTTPS addresses.
Supports efficient streaming, gzip and bz2 compression,
as well as Kerberos and username/password (basic) http
authentication.

This patch adds support for custom S3 servers in the connection string. It also adds explicit support for setting the server port, and whether or not to use SSL, both as paramaters to the smart_open function as well as within the connection string. These changes are neccessary to be able to connect to s3proxy and other custom s3 servers which don't run on the default port, or neccessarily use SSL.

into RaRe-Technologies-master Conflicts: smart_open/tests/test_smart_open.py

Adds support for opening vanilla HTTP and HTTPS addresses. Supports efficient streaming, gzip and bz2 compression, as well as Kerberos and username/password (basic) http authentication.

piskvorky · 2017-03-01T05:58:20Z

smart_open/smart_open_lib.py

@@ -573,23 +588,6 @@ def __enter__(self):
    def __exit__(self, type, value, traceback):
        pass

-
-def make_closing(base, **attrs):


Did we drop support for Python 2.6? Why this change?

Oops, sorry, didn't think those comments were still active since python 2.7 has been around so long and there's no reason I know of for people to stick with 2.6. I can revert this part if you need 2.6 support -- let me know.

We still support 2.6

OK, reverted in latest.

piskvorky · 2017-03-01T06:00:08Z

Oh wow, that's really cool! Interesting feature too.

What is your use-case for this, how do you use HTTP streaming yourself, if you don't mind me asking?

robottwo · 2017-03-01T13:21:17Z

I don't have a specific use-case for http streaming specifically (although I can imagine some) -- it's more that I like the idea of being able to use smart_open as a unified go-to tool for any type of uri. I definitely do have use-cases where I want to pull a web page, or a compressed file off of an http server.

tmylk · 2017-03-05T15:08:53Z

smart_open/smart_open_lib.py

+
+        If none of those are set, will connect unauthenticated.
+        """
+        PY2 = sys.version_info[0] == 2


The python 2 check is now used in two places. Better refactor to a global.

Refactored in latest.

tmylk · 2017-03-05T15:16:26Z

smart_open/smart_open_lib.py

+
+    response = HttpReadStream(url, **kwargs)
+
+    if url.endswith('.gz'):


the gz and bz2 handling code is identical to file_smart_open. Please refactor to avoid duplication.

Good Point. This will also make it easier to add support for other compression types later.

Done in latest version.

tmylk · 2017-03-05T15:21:26Z

smart_open/tests/test_smart_open.py

+        with smart_open.HttpOpenRead(smart_open.ParseUri('https://www.google.com'), 'r') as smart_open_object:
+            content = smart_open_object.readline().decode('utf-8')
+            expected_str = '<!doctype html>'
+            self.assertEqual(content[:len(expected_str)], expected_str)


Please add tests for authentication using requests_mock. It would also be great if the tests worked without access to internet and google.com.

OK, fixed. I added a test for Basic http auth, and made the other tests not actually go to google.com. I didn't add a Kerbeos test because I'm not sure that's easily doable without running within a Kerborized environment.

tmylk · 2017-03-05T15:22:20Z

Thanks for the new feature! Just minor refactoring and better tests with mocking are needed before merging.

We still want to maintain Python 2.6 compatibility, so don't rely on contextlib.closing.

Now they don't require internet access, and will test for Basic authentication in the HTTP header.

http => https, and remove old versions of the tests.

robottwo · 2017-03-13T14:44:57Z

Thanks. I've addressed all the comments in the latest version of the PR. Please take a look.

Thanks,
-Dan

tmylk · 2017-03-13T19:13:33Z

smart_open/tests/test_smart_open.py

@@ -168,6 +168,31 @@ def test_webhdfs_read(self):
        smart_open_object = smart_open.WebHdfsOpenRead(smart_open.ParseUri("webhdfs://127.0.0.1:8440/path/file"))
        self.assertEqual(smart_open_object.read().decode("utf-8"), "line1\nline2")

+    @responses.activate
+    def test_http_read(self):
+        """Does webhdfs read method work correctly"""


Just a typo in the docstring ""Does http read method work correctly"""

tmylk · 2017-03-13T19:15:13Z

smart_open/tests/test_smart_open.py

+
+    @responses.activate
+    def test_https_readline(self):
+        """Does webhdfs read method work correctly"""


Typo in docstring as well

tmylk · 2017-03-13T19:20:07Z

Thanks a lot for the code changes!
May I ask for some cosmetic changes - an example of the new functionality in the README and a note in the changelog?
Then will merge and release this week.

piskvorky · 2017-03-14T07:48:21Z

smart_open/tests/test_smart_open.py

+        """Does http authentication work correctly"""
+        responses.add(responses.GET, "http://127.0.0.1/index.html", body='line1\nline2')
+        smart_open_object = smart_open.HttpOpenRead(smart_open.ParseUri("http://127.0.0.1/index.html"),
+                                                    user='me', password='pass')


nitpick: we use hanging (not vertical) indent.

robottwo · 2017-03-14T11:02:18Z

All comments addressed in the latest version.

tmylk · 2017-03-14T14:40:19Z

Great! Thanks for the new feature!

Daniel Eisner and others added 14 commits November 29, 2016 11:51

Fix unit tests

a51bedd

updated README.rst with new s3 mode.

d1c9fea

Added a new unit test for the unsecured calling form

d847291

Updated style and unit test.

ad4cac4

Check that the port argument isnt normally passed.

df38217

Merge branch 'master' of github.com:robottwo/smart_open

87d782c

Merge branch 'master' of https://github.com/RaRe-Technologies/smart_open

6043a01

into RaRe-Technologies-master Conflicts: smart_open/tests/test_smart_open.py

Add generic HTTP and HTTPS streaming support.

8a42950

Adds support for opening vanilla HTTP and HTTPS addresses. Supports efficient streaming, gzip and bz2 compression, as well as Kerberos and username/password (basic) http authentication.

removed previous merge artifact;

f8dfc73

Raise exception instead of returning it :/

ef17bcb

Raise http exceptions properly

0097ac2

neccessary import

d55efd9

python 3 compatibility

6702bf7

piskvorky reviewed Mar 1, 2017

View reviewed changes

tmylk reviewed Mar 5, 2017

View reviewed changes

robottwo added 5 commits March 13, 2017 09:29

Reverted make_closing -> closing

18fed99

We still want to maintain Python 2.6 compatibility, so don't rely on contextlib.closing.

Refactor the code to get the Python version

87d1bc6

Refactored the GZfile and BZ2File compression wrappers.

86a1306

Refactored HttpOpenRead unit tests.

c5c30ef

Now they don't require internet access, and will test for Basic authentication in the HTTP header.

Clean up http unit tests.

9db299d

http => https, and remove old versions of the tests.

tmylk reviewed Mar 13, 2017

View reviewed changes

piskvorky reviewed Mar 14, 2017

View reviewed changes

Cosmetic changes and doc updates.

70b17ab

tmylk merged commit a1c2c03 into piskvorky:master Mar 14, 2017

tmylk mentioned this pull request Mar 17, 2017

Test compressed files over http #111

Closed

rctzeng mentioned this pull request Mar 22, 2017

Make #107 compatible and found HttpReadStream resource leak #117

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Add generic HTTP and HTTPS streaming support. #107

Add generic HTTP and HTTPS streaming support. #107

robottwo commented Feb 28, 2017

piskvorky Mar 1, 2017

robottwo Mar 1, 2017

tmylk Mar 5, 2017

robottwo Mar 13, 2017

piskvorky commented Mar 1, 2017

robottwo commented Mar 1, 2017

tmylk Mar 5, 2017

robottwo Mar 13, 2017

tmylk Mar 5, 2017

robottwo Mar 13, 2017

tmylk Mar 5, 2017

robottwo Mar 13, 2017

tmylk commented Mar 5, 2017

robottwo commented Mar 13, 2017

tmylk Mar 13, 2017 •

edited

tmylk Mar 13, 2017

tmylk commented Mar 13, 2017

piskvorky Mar 14, 2017

robottwo Mar 14, 2017

robottwo commented Mar 14, 2017

tmylk commented Mar 14, 2017


		response = HttpReadStream(url, **kwargs)

		if url.endswith('.gz'):

Add generic HTTP and HTTPS streaming support. #107

Add generic HTTP and HTTPS streaming support. #107

Conversation

robottwo commented Feb 28, 2017

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

piskvorky commented Mar 1, 2017

robottwo commented Mar 1, 2017

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

tmylk commented Mar 5, 2017

robottwo commented Mar 13, 2017

tmylk Mar 13, 2017 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

tmylk commented Mar 13, 2017

Choose a reason for hiding this comment

Choose a reason for hiding this comment

robottwo commented Mar 14, 2017

tmylk commented Mar 14, 2017

tmylk Mar 13, 2017 •

edited