Merge lp:~gocept/landscape-client/fetch-strings-and-bytes into lp:~landscape/landscape-client/trunk

[Steffen Allner]

Fix UnicodeDecodeError (bug #1672720) when exchanging message with the server.

Encode unicode strings in landscape.lib.fetch before passing over to libcurl.

Now also allow bytes as data to be fetched. This should enable to use bpickled unicode data. I also added a new test to cover that.

Testing instructions:
* Use the client in communication with a server as reported in the bug.

[🤖 Landscape Builder (landscape-builder)]

review: Abstain (executing tests)

[🤖 Landscape Builder (landscape-builder)]

Command: TRIAL_ARGS=-j4 make check
Result: Fail
Revno: 958
Branch: lp:~gocept/landscape-client/fetch-strings-and-bytes
Jenkins: https://ci.lscape.net/job/latch-test-xenial/3623/

review: Needs Fixing (test results)

[Steffen Allner (sallner)]

Hm, I cannot reproduce that failure on my machine and I actually cannot image, how the code change could have influenced the test so that it fails. Do you see a connection Danilo? Otherwise could you please restart the test?

[🤖 Landscape Builder (landscape-builder)]

review: Abstain (executing tests)

[🤖 Landscape Builder (landscape-builder)]

Command: TRIAL_ARGS=-j4 make check
Result: Success
Revno: 958
Branch: lp:~gocept/landscape-client/fetch-strings-and-bytes
Jenkins: https://ci.lscape.net/job/latch-test-xenial/3625/

review: Approve (test results)

[🤖 Landscape Builder (landscape-builder)]

review: Abstain (executing tests)

[🤖 Landscape Builder (landscape-builder)]

Command: TRIAL_ARGS=-j4 make check
Result: Success
Revno: 958
Branch: lp:~gocept/landscape-client/fetch-strings-and-bytes
Jenkins: https://ci.lscape.net/job/latch-test-xenial/3626/

review: Approve (test results)

[Michael Howitz (mh-gocept)]

I'd suggest to reduce the duplicate code.

review: Needs Fixing

[lp:~gocept/landscape-client/fetch-strings-and-bytes]

959. By Steffen Allner on 2017-03-16

Refactor to use helper method for common code in tests.

[Steffen Allner (sallner)]

I added a helper method.

[🤖 Landscape Builder (landscape-builder)]

review: Abstain (executing tests)

[🤖 Landscape Builder (landscape-builder)]

Command: TRIAL_ARGS=-j4 make check
Result: Success
Revno: 959
Branch: lp:~gocept/landscape-client/fetch-strings-and-bytes
Jenkins: https://ci.lscape.net/job/latch-test-xenial/3629/

review: Approve (test results)

[Michael Howitz (mh-gocept)]

Looks good now.

review: Approve

[Данило Шеган (danilo)]

We are not huge fans of asserts in helpers in the Landscape team: it's too easy to break the code so the asserts are never fired.

I am fine with a helper that sends a request for a payload, but got_result() should live in each individual test, and the unicode test should not check any of the headers but only the content: they are not the point of that test.

With those changes, I think there will be less value in a helper, but if you still think it's worth it, I am happy with that too.

I'll do a quick run to ensure this fixes the problem we've seen before giving my final stamp of approval.

[Данило Шеган (danilo)]

review: Approve

=== modified file 'landscape/broker/tests/test_transport.py'
--- landscape/broker/tests/test_transport.py	2017-03-06 10:38:07 +0000
+++ landscape/broker/tests/test_transport.py	2017-03-16 09:56:25 +0000
@@ -1,3 +1,4 @@
+# -*- coding: utf-8 -*-
 import os
 
 from landscape import VERSION
@@ -49,6 +50,34 @@
         for port in self.ports:
             port.stopListening()
 
+    def request_with_payload(self, payload):
+        resource = DataCollectingResource()
+        port = reactor.listenTCP(
+            0, server.Site(resource), interface="127.0.0.1")
+        self.ports.append(port)
+        transport = HTTPTransport(
+            None, "http://localhost:%d/" % (port.getHost().port,))
+        result = deferToThread(transport.exchange, payload, computer_id="34",
+                               exchange_token="abcd-efgh", message_api="X.Y")
+
+        def got_result(ignored):
+            try:
+                get_header = resource.request.requestHeaders.getRawHeaders
+            except AttributeError:
+                # For backwards compatibility with Twisted versions
+                # without requestHeaders
+                def get_header(header):
+                    return [resource.request.received_headers[header]]
+
+            self.assertEqual(get_header(u"x-computer-id"), ["34"])
+            self.assertEqual(get_header("x-exchange-token"), ["abcd-efgh"])
+            self.assertEqual(
+                get_header("user-agent"), ["landscape-client/%s" % (VERSION,)])
+            self.assertEqual(get_header("x-message-api"), ["X.Y"])
+            self.assertEqual(bpickle.loads(resource.content), payload)
+        result.addCallback(got_result)
+        return result
+
     def test_get_url(self):
         url = "http://example/ooga"
         transport = HTTPTransport(None, url)
@@ -66,32 +95,14 @@
         message API version as HTTP headers, and the payload as a
         bpickled request body.
         """
-        resource = DataCollectingResource()
-        port = reactor.listenTCP(
-            0, server.Site(resource), interface="127.0.0.1")
-        self.ports.append(port)
-        transport = HTTPTransport(
-            None, "http://localhost:%d/" % (port.getHost().port,))
-        result = deferToThread(transport.exchange, "HI", computer_id="34",
-                               exchange_token="abcd-efgh", message_api="X.Y")
-
-        def got_result(ignored):
-            try:
-                get_header = resource.request.requestHeaders.getRawHeaders
-            except AttributeError:
-                # For backwards compatibility with Twisted versions
-                # without requestHeaders
-                def get_header(header):
-                    return [resource.request.received_headers[header]]
-
-            self.assertEqual(get_header(u"x-computer-id"), ["34"])
-            self.assertEqual(get_header("x-exchange-token"), ["abcd-efgh"])
-            self.assertEqual(
-                get_header("user-agent"), ["landscape-client/%s" % (VERSION,)])
-            self.assertEqual(get_header("x-message-api"), ["X.Y"])
-            self.assertEqual(bpickle.loads(resource.content), "HI")
-        result.addCallback(got_result)
-        return result
+        return self.request_with_payload(payload="HI")
+
+    def test_request_data_unicode(self):
+        """
+        When a payload contains unicode characters they are properly handled
+        by bpickle.
+        """
+        return self.request_with_payload(payload=u"проба")
 
     def test_ssl_verification_positive(self):
         """

=== modified file 'landscape/lib/fetch.py'
--- landscape/lib/fetch.py	2017-03-06 15:16:16 +0000
+++ landscape/lib/fetch.py	2017-03-16 09:56:25 +0000
@@ -65,7 +65,9 @@
     @param proxy: The proxy url to use for the request.
     """
     import pycurl
-    output = io.BytesIO(networkString(data))
+    if not isinstance(data, bytes):
+        data = data.encode("utf-8")
+    output = io.BytesIO(data)
     input = io.BytesIO()
 
     if curl is None: