Repository: canonical/operator

Issue:
Tune recently added file API function naming and calling convention
The outcome on this thread in PR #505 doesn't seem ideal:

https://github.com/canonical/operator/pull/505#discussion_r615019063

Let's please fix the API so it conforms with the spec naming (pull/push) and so that it uses a proper file-like interface (think open) that at least can be extended so it doesn't blow up out of memory for large files. It'd make no sense to have push for a file-like and `write_content` for a string. We can have just `push` taking either a string or a file-like under the same content/data/whatever parameter. And pull would consistently return a file-like that can be read trivially with `pebble.pull(path).read(),` which isn't different from `open(path).read()`. We don't have to fix the underlying implementation _today_ to prevent excessive memory situations, but with that there's a clear way to do that and instantly have everyone on the proper path.

For the future implementation and regarding the points on the comment thread, returning a file-like object right away would be trivial, and for the future implementation we can look at how Go does it with temporary files and with an API that conditionally does it in memory or in temporary files depending on the size of the response.

Discussion:

Patch:
diff --git a/ops/model.py b/ops/model.py
index 6be1b93..d4e9b08 100644
--- a/ops/model.py
+++ b/ops/model.py
@@ -1091,31 +1091,35 @@ class Container:
             raise RuntimeError('expected 1 service, got {}'.format(len(services)))
         return services[service_name]
 
-    def read_content(self, path: str, *, encoding: str = 'utf-8') -> typing.Union[bytes, str]:
-        """Read a file's content from the remote system to a string.
+    def pull(self, path: str, *, encoding: str = 'utf-8') -> typing.Union[typing.BinaryIO,
+                                                                          typing.TextIO]:
+        """Read a file's content from the remote system.
 
         Args:
             path: Path of the file to read from the remote system.
-            encoding: Encoding to use for decoding file's bytes to a text string
-                (or None to return raw bytes).
+            encoding: Encoding to use for decoding the file's bytes to str,
+                or None to specify no decoding.
 
         Returns:
-            File's content, decoded and returned as str if encoding is not None,
-            or returned as raw bytes if encoding is None.
+            A readable file-like object, whose read() method will return str
+            objects decoded according to the specified encoding, or bytes if
+            encoding is None.
         """
-        return self._pebble.read_content(path, encoding=encoding)
+        return self._pebble.pull(path, encoding=encoding)
 
-    def write_content(
-            self, path: str, content: typing.Union[bytes, str], *, encoding: str = 'utf-8',
-            make_dirs: bool = False, permissions: int = None, user_id: int = None,
-            user: str = None, group_id: int = None, group: str = None):
+    def push(
+            self, path: str, source: typing.Union[bytes, str, typing.BinaryIO, typing.TextIO], *,
+            encoding: str = 'utf-8', make_dirs: bool = False, permissions: int = None,
+            user_id: int = None, user: str = None, group_id: int = None, group: str = None):
         """Write content to a given file path on the remote system.
 
         Args:
             path: Path of the file to write to on the remote system.
-            content: Content to write (str or bytes).
-            encoding: Encoding to use for encoding content str to bytes.
-                Ignored if content is bytes.
+            source: Source of data to write. This is either a concrete str or
+                bytes instance, or a readable file-like object.
+            encoding: Encoding to use for encoding source str to bytes, or
+                strings read from source if it is a TextIO type. Ignored if
+                source is bytes or BinaryIO.
             make_dirs: If True, create parent directories if they don't exist.
             permissions: Permissions (mode) to create file with (Pebble default
                 is 0o644).
@@ -1124,9 +1128,9 @@ class Container:
             group_id: GID for file.
             group: Group name for file (group_id takes precedence).
         """
-        self._pebble.write_content(path, content, encoding=encoding, make_dirs=make_dirs,
-                                   permissions=permissions, user_id=user_id, user=user,
-                                   group_id=group_id, group=group)
+        self._pebble.push(path, source, encoding=encoding, make_dirs=make_dirs,
+                          permissions=permissions, user_id=user_id, user=user,
+                          group_id=group_id, group=group)
 
     def list_files(self, path: str, *, pattern: str = None,
                    itself: bool = False) -> typing.List['pebble.FileInfo']:
diff --git a/ops/pebble.py b/ops/pebble.py
index 78e3abe..e498b3c 100644
--- a/ops/pebble.py
+++ b/ops/pebble.py
@@ -23,6 +23,7 @@ import datetime
 import email.parser
 import enum
 import http.client
+import io
 import json
 import re
 import socket
@@ -875,17 +876,19 @@ class Client:
         resp = self._request('GET', '/v1/services', query)
         return [ServiceInfo.from_dict(info) for info in resp['result']]
 
-    def read_content(self, path: str, *, encoding: str = 'utf-8') -> typing.Union[bytes, str]:
-        """Read a file's content from the remote system to a string.
+    def pull(self, path: str, *, encoding: str = 'utf-8') -> typing.Union[typing.BinaryIO,
+                                                                          typing.TextIO]:
+        """Read a file's content from the remote system.
 
         Args:
             path: Path of the file to read from the remote system.
-            encoding: Encoding to use for decoding file's bytes to a text string
-                (or None to return raw bytes).
+            encoding: Encoding to use for decoding the file's bytes to str,
+                or None to specify no decoding.
 
         Returns:
-            File's content, decoded and returned as str if encoding is not None,
-            or returned as raw bytes if encoding is None.
+            A readable file-like object, whose read() method will return str
+            objects decoded according to the specified encoding, or bytes if
+            encoding is None.
         """
         query = {
             'action': 'read',
@@ -934,8 +937,10 @@ class Client:
         if content is None:
             raise ProtocolError('no file content in multipart response')
         if encoding is not None:
-            content = content.decode(encoding)
-        return content
+            reader = io.StringIO(content.decode(encoding))
+        else:
+            reader = io.BytesIO(content)
+        return reader
 
     @staticmethod
     def _raise_on_path_error(resp, path):
@@ -947,17 +952,19 @@ class Client:
         if error:
             raise PathError(error['kind'], error['message'])
 
-    def write_content(
-            self, path: str, content: typing.Union[bytes, str], *, encoding: str = 'utf-8',
-            make_dirs: bool = False, permissions: int = None, user_id: int = None,
-            user: str = None, group_id: int = None, group: str = None):
+    def push(
+            self, path: str, source: typing.Union[bytes, str, typing.BinaryIO, typing.TextIO], *,
+            encoding: str = 'utf-8', make_dirs: bool = False, permissions: int = None,
+            user_id: int = None, user: str = None, group_id: int = None, group: str = None):
         """Write content to a given file path on the remote system.
 
         Args:
             path: Path of the file to write to on the remote system.
-            content: Content to write (str or bytes).
-            encoding: Encoding to use for encoding content str to bytes.
-                Ignored if content is bytes.
+            source: Source of data to write. This is either a concrete str or
+                bytes instance, or a readable file-like object.
+            encoding: Encoding to use for encoding source str to bytes, or
+                strings read from source if it is a TextIO type. Ignored if
+                source is bytes or BinaryIO.
             make_dirs: If True, create parent directories if they don't exist.
             permissions: Permissions (mode) to create file with (Pebble default
                 is 0o644).
@@ -984,6 +991,10 @@ class Client:
 
         part = MIMEBase('application', 'octet-stream')
         part.add_header('Content-Disposition', 'form-data', name='files', filename=path)
+        if hasattr(source, 'read'):
+            content = source.read()
+        else:
+            content = source
         if isinstance(content, str):
             content = content.encode(encoding)
         part.set_payload(content)


Test patch:
diff --git a/test/pebble_cli.py b/test/pebble_cli.py
index bbfd3b1..9cdf37d 100644
--- a/test/pebble_cli.py
+++ b/test/pebble_cli.py
@@ -147,7 +147,7 @@ def main():
         elif args.command == 'plan':
             result = client.get_plan().to_yaml()
         elif args.command == 'pull':
-            content = client.read_content(args.remote_path, encoding=None)
+            content = client.pull(args.remote_path, encoding=None).read()
             if args.local_path != '-':
                 with open(args.local_path, 'wb') as f:
                     f.write(content)
@@ -157,11 +157,10 @@ def main():
                 return
         elif args.command == 'push':
             with open(args.local_path, 'rb') as f:
-                content = f.read()
-            client.write_content(
-                args.remote_path, content, make_dirs=args.dirs,
-                permissions=int(args.mode, 8) if args.mode is not None else None,
-                user=args.user, group=args.group)
+                client.push(
+                    args.remote_path, f, make_dirs=args.dirs,
+                    permissions=int(args.mode, 8) if args.mode is not None else None,
+                    user=args.user, group=args.group)
             result = 'wrote {} to remote file {}'.format(args.local_path, args.remote_path)
         elif args.command == 'rm':
             client.remove_path(args.path, recursive=bool(args.recursive))
diff --git a/test/test_model.py b/test/test_model.py
index 7f7e1c3..b1980dd 100755
--- a/test/test_model.py
+++ b/test/test_model.py
@@ -932,35 +932,34 @@ containers:
         with self.assertRaises(RuntimeError):
             self.container.get_service('s1')
 
-    def test_read_content(self):
-        self.pebble.responses.append('content1')
-        got = self.container.read_content('/path/1')
-        self.assertEqual(got, 'content1')
+    def test_pull(self):
+        self.pebble.responses.append('dummy1')
+        got = self.container.pull('/path/1')
+        self.assertEqual(got, 'dummy1')
         self.assertEqual(self.pebble.requests, [
-            ('read_content', '/path/1', 'utf-8'),
+            ('pull', '/path/1', 'utf-8'),
         ])
         self.pebble.requests = []
 
-        self.pebble.responses.append(b'content2')
-        got = self.container.read_content('/path/2', encoding=None)
-        self.assertEqual(got, b'content2')
+        self.pebble.responses.append(b'dummy2')
+        got = self.container.pull('/path/2', encoding=None)
+        self.assertEqual(got, b'dummy2')
         self.assertEqual(self.pebble.requests, [
-            ('read_content', '/path/2', None),
+            ('pull', '/path/2', None),
         ])
 
-    def test_write_content(self):
-        self.container.write_content('/path/1', 'content1')
+    def test_push(self):
+        self.container.push('/path/1', 'content1')
         self.assertEqual(self.pebble.requests, [
-            ('write_content', '/path/1', 'content1', 'utf-8', False, None,
+            ('push', '/path/1', 'content1', 'utf-8', False, None,
              None, None, None, None),
         ])
         self.pebble.requests = []
 
-        self.container.write_content('/path/2', b'content2', encoding=None, make_dirs=True,
-                                     permissions=0o600, user_id=12, user='bob', group_id=34,
-                                     group='staff')
+        self.container.push('/path/2', b'content2', encoding=None, make_dirs=True,
+                            permissions=0o600, user_id=12, user='bob', group_id=34, group='staff')
         self.assertEqual(self.pebble.requests, [
-            ('write_content', '/path/2', b'content2', None, True, 0o600, 12, 'bob', 34, 'staff'),
+            ('push', '/path/2', b'content2', None, True, 0o600, 12, 'bob', 34, 'staff'),
         ])
 
     def test_list_files(self):
@@ -1040,13 +1039,13 @@ class MockPebbleClient:
         self.requests.append(('get_services', names))
         return self.responses.pop(0)
 
-    def read_content(self, path, *, encoding='utf-8'):
-        self.requests.append(('read_content', path, encoding))
+    def pull(self, path, *, encoding='utf-8'):
+        self.requests.append(('pull', path, encoding))
         return self.responses.pop(0)
 
-    def write_content(self, path, content, *, encoding='utf-8', make_dirs=False, permissions=None,
-                      user_id=None, user=None, group_id=None, group=None):
-        self.requests.append(('write_content', path, content, encoding, make_dirs, permissions,
+    def push(self, path, source, *, encoding='utf-8', make_dirs=False, permissions=None,
+             user_id=None, user=None, group_id=None, group=None):
+        self.requests.append(('push', path, source, encoding, make_dirs, permissions,
                               user_id, user, group_id, group))
 
     def list_files(self, path, *, pattern=None, itself=False):
diff --git a/test/test_pebble.py b/test/test_pebble.py
index 3aca792..48ac44b 100644
--- a/test/test_pebble.py
+++ b/test/test_pebble.py
@@ -1170,7 +1170,7 @@ services:
             ('GET', '/v1/services', {'names': 'svc2'}, None),
         ])
 
-    def test_read_content_str(self):
+    def test_pull_text(self):
         self.client.responses.append((
             {'Content-Type': 'multipart/form-data; boundary=01234567890123456789012345678901'},
             b"""
@@ -1191,7 +1191,7 @@ Content-Disposition: form-data; name="response"
 """,
         ))
 
-        content = self.client.read_content('/etc/hosts')
+        content = self.client.pull('/etc/hosts').read()
         self.assertEqual(content, '127.0.0.1 localhost  # 😀')
 
         self.assertEqual(self.client.requests, [
@@ -1199,7 +1199,7 @@ Content-Disposition: form-data; name="response"
                 {'Accept': 'multipart/form-data'}, None),
         ])
 
-    def test_read_content_bytes(self):
+    def test_pull_binary(self):
         self.client.responses.append((
             {'Content-Type': 'multipart/form-data; boundary=01234567890123456789012345678901'},
             b"""
@@ -1220,7 +1220,7 @@ Content-Disposition: form-data; name="response"
 """,
         ))
 
-        content = self.client.read_content('/etc/hosts', encoding=None)
+        content = self.client.pull('/etc/hosts', encoding=None).read()
         self.assertEqual(content, b'127.0.0.1 localhost  # \xf0\x9f\x98\x80')
 
         self.assertEqual(self.client.requests, [
@@ -1228,7 +1228,7 @@ Content-Disposition: form-data; name="response"
                 {'Accept': 'multipart/form-data'}, None),
         ])
 
-    def test_read_content_path_error(self):
+    def test_pull_path_error(self):
         self.client.responses.append((
             {'Content-Type': 'multipart/form-data; boundary=01234567890123456789012345678901'},
             b"""
@@ -1248,7 +1248,7 @@ Content-Disposition: form-data; name="response"
         ))
 
         with self.assertRaises(pebble.PathError) as cm:
-            self.client.read_content('/etc/hosts')
+            self.client.pull('/etc/hosts')
         self.assertIsInstance(cm.exception, pebble.Error)
         self.assertEqual(cm.exception.kind, 'not-found')
         self.assertEqual(cm.exception.message, 'not found')
@@ -1258,17 +1258,17 @@ Content-Disposition: form-data; name="response"
                 {'Accept': 'multipart/form-data'}, None),
         ])
 
-    def test_read_content_protocol_errors(self):
+    def test_pull_protocol_errors(self):
         self.client.responses.append(({'Content-Type': 'ct'}, b''))
         with self.assertRaises(pebble.ProtocolError) as cm:
-            self.client.read_content('/etc/hosts')
+            self.client.pull('/etc/hosts')
         self.assertIsInstance(cm.exception, pebble.Error)
         self.assertEqual(str(cm.exception),
                          "expected Content-Type 'multipart/form-data', got 'ct'")
 
         self.client.responses.append(({'Content-Type': 'multipart/form-data'}, b''))
         with self.assertRaises(pebble.ProtocolError) as cm:
-            self.client.read_content('/etc/hosts')
+            self.client.pull('/etc/hosts')
         self.assertEqual(str(cm.exception), "invalid boundary ''")
 
         self.client.responses.append((
@@ -1282,7 +1282,7 @@ bad path
 """,
         ))
         with self.assertRaises(pebble.ProtocolError) as cm:
-            self.client.read_content('/etc/hosts')
+            self.client.pull('/etc/hosts')
         self.assertEqual(str(cm.exception), "path not expected: /bad")
 
         self.client.responses.append((
@@ -1296,10 +1296,16 @@ bad path
 """,
         ))
         with self.assertRaises(pebble.ProtocolError) as cm:
-            self.client.read_content('/etc/hosts')
+            self.client.pull('/etc/hosts')
         self.assertEqual(str(cm.exception), 'no "response" field in multipart body')
 
-    def test_write_content_str(self):
+    def test_push_str(self):
+        self._test_push_str('content 😀')
+
+    def test_push_text(self):
+        self._test_push_str(io.StringIO('content 😀'))
+
+    def _test_push_str(self, source):
         self.client.responses.append((
             {'Content-Type': 'application/json'},
             b"""
@@ -1314,7 +1320,7 @@ bad path
 """,
         ))
 
-        self.client.write_content('/foo/bar', 'content 😀')
+        self.client.push('/foo/bar', source)
 
         self.assertEqual(len(self.client.requests), 1)
         request = self.client.requests[0]
@@ -1330,7 +1336,13 @@ bad path
             'files': [{'path': '/foo/bar'}],
         })
 
-    def test_write_content_bytes(self):
+    def test_push_bytes(self):
+        self._test_push_bytes(b'content \xf0\x9f\x98\x80')
+
+    def test_push_binary(self):
+        self._test_push_bytes(io.BytesIO(b'content \xf0\x9f\x98\x80'))
+
+    def _test_push_bytes(self, source):
         self.client.responses.append((
             {'Content-Type': 'application/json'},
             b"""
@@ -1345,7 +1357,7 @@ bad path
 """,
         ))
 
-        self.client.write_content('/foo/bar', b'content \xf0\x9f\x98\x80')
+        self.client.push('/foo/bar', source)
 
         self.assertEqual(len(self.client.requests), 1)
         request = self.client.requests[0]
@@ -1361,7 +1373,7 @@ bad path
             'files': [{'path': '/foo/bar'}],
         })
 
-    def test_write_content_all_options(self):
+    def test_push_all_options(self):
         self.client.responses.append((
             {'Content-Type': 'application/json'},
             b"""
@@ -1376,8 +1388,8 @@ bad path
 """,
         ))
 
-        self.client.write_content('/foo/bar', 'content', make_dirs=True, permissions=0o600,
-                                  user_id=12, user='bob', group_id=34, group='staff')
+        self.client.push('/foo/bar', 'content', make_dirs=True, permissions=0o600,
+                         user_id=12, user='bob', group_id=34, group='staff')
 
         self.assertEqual(len(self.client.requests), 1)
         request = self.client.requests[0]
@@ -1401,7 +1413,7 @@ bad path
             }],
         })
 
-    def test_write_content_uid_gid(self):
+    def test_push_uid_gid(self):
         self.client.responses.append((
             {'Content-Type': 'application/json'},
             b"""
@@ -1416,7 +1428,7 @@ bad path
 """,
         ))
 
-        self.client.write_content('/foo/bar', 'content', user_id=12, group_id=34)
+        self.client.push('/foo/bar', 'content', user_id=12, group_id=34)
 
         self.assertEqual(len(self.client.requests), 1)
         request = self.client.requests[0]
@@ -1436,7 +1448,7 @@ bad path
             }],
         })
 
-    def test_write_content_path_error(self):
+    def test_push_path_error(self):
         self.client.responses.append((
             {'Content-Type': 'application/json'},
             b"""
@@ -1452,7 +1464,7 @@ bad path
         ))
 
         with self.assertRaises(pebble.PathError) as cm:
-            self.client.write_content('/foo/bar', 'content')
+            self.client.push('/foo/bar', 'content')
         self.assertEqual(cm.exception.kind, 'not-found')
         self.assertEqual(cm.exception.message, 'not found')