launchpad-reviewers team mailing list archive

[Colin Watson <mp+407468@xxxxxxxxxxxxxxxxxx>]

Colin Watson has proposed merging ~cjwatson/launchpad:fix-lfa-encoding into launchpad:master.

Commit message:
Fix position tracking in EncodableLibraryFileAlias

Requested reviews:
  Launchpad code reviewers (launchpad-reviewers)
Related bugs:
  Bug #1940664 in Launchpad itself: "Snap store uploads failing with __len__() should return >= 0"
  https://bugs.launchpad.net/launchpad/+bug/1940664

For more details, see:
https://code.launchpad.net/~cjwatson/launchpad/+git/launchpad/+merge/407468

Python's `len()` requires that `__len__` returns a value greater than or equal to 0.  `EncodableLibraryFileAlias` violated this in the case where a caller passed an explicit length to its `read` method which was greater than the number of bytes remaining.  In this case, increase the position only up to the end of the file, not past it.
-- 
Your team Launchpad code reviewers is requested to review the proposed merge of ~cjwatson/launchpad:fix-lfa-encoding into launchpad:master.

diff --git a/lib/lp/services/librarian/tests/test_utils.py b/lib/lp/services/librarian/tests/test_utils.py
index a7059b9..79b2f74 100644
--- a/lib/lp/services/librarian/tests/test_utils.py
+++ b/lib/lp/services/librarian/tests/test_utils.py
@@ -1,12 +1,20 @@
-# Copyright 2009 Canonical Ltd.  This software is licensed under the
+# Copyright 2009-2021 Canonical Ltd.  This software is licensed under the
 # GNU Affero General Public License version 3 (see the file LICENSE).
 
-import unittest
+import transaction
 
-from lp.services.librarian.utils import guess_librarian_encoding
+from lp.services.librarian.utils import (
+    EncodableLibraryFileAlias,
+    guess_librarian_encoding,
+    )
+from lp.testing import (
+    TestCase,
+    TestCaseWithFactory,
+    )
+from lp.testing.layers import LaunchpadZopelessLayer
 
 
-class LibrarianUtils(unittest.TestCase):
+class LibrarianUtils(TestCase):
     """Librarian utilities functions."""
 
     def test_guess_librarian_encoding(self):
@@ -39,3 +47,52 @@ class LibrarianUtils(unittest.TestCase):
             'foo.diff.gz', 'will_be_overridden')
         self.assertEqual(encoding, 'gzip')
         self.assertEqual(mimetype, 'text/plain')
+
+
+class TestEncodableLibraryFileAlias(TestCaseWithFactory):
+
+    layer = LaunchpadZopelessLayer
+
+    def test_read_all(self):
+        lfa = self.factory.makeLibraryFileAlias(content=b'abcdefgh')
+        transaction.commit()
+        lfa.open()
+        try:
+            encodable_lfa = EncodableLibraryFileAlias(lfa)
+            self.assertEqual(8, len(encodable_lfa))
+            self.assertEqual(b'abcdefgh', encodable_lfa.read())
+            self.assertEqual(0, len(encodable_lfa))
+        finally:
+            lfa.close()
+
+    def test_read_some(self):
+        lfa = self.factory.makeLibraryFileAlias(content=b'abcdefgh')
+        transaction.commit()
+        lfa.open()
+        try:
+            encodable_lfa = EncodableLibraryFileAlias(lfa)
+            self.assertEqual(8, len(encodable_lfa))
+            self.assertEqual(b'a', encodable_lfa.read(1))
+            self.assertEqual(7, len(encodable_lfa))
+            self.assertEqual(b'bc', encodable_lfa.read(2))
+            self.assertEqual(5, len(encodable_lfa))
+            self.assertEqual(b'defgh', encodable_lfa.read())
+            self.assertEqual(0, len(encodable_lfa))
+        finally:
+            lfa.close()
+
+    def test_read_past_end(self):
+        lfa = self.factory.makeLibraryFileAlias(content=b'abcdefgh')
+        transaction.commit()
+        lfa.open()
+        try:
+            encodable_lfa = EncodableLibraryFileAlias(lfa)
+            self.assertEqual(8, len(encodable_lfa))
+            self.assertEqual(b'a', encodable_lfa.read(1))
+            self.assertEqual(7, len(encodable_lfa))
+            self.assertEqual(b'bcdefgh', encodable_lfa.read(8))
+            self.assertEqual(0, len(encodable_lfa))
+            self.assertEqual(b'', encodable_lfa.read(8))
+            self.assertEqual(0, len(encodable_lfa))
+        finally:
+            lfa.close()
diff --git a/lib/lp/services/librarian/utils.py b/lib/lp/services/librarian/utils.py
index bc0895b..a8423e0 100644
--- a/lib/lp/services/librarian/utils.py
+++ b/lib/lp/services/librarian/utils.py
@@ -93,5 +93,5 @@ class EncodableLibraryFileAlias:
         if chunksize is None:
             self.position = self.lfa.content.filesize
         else:
-            self.position += length
+            self.position += len(data)
         return data