launchpad-reviewers team mailing list archive

[Gavin Panella <gavin.panella@xxxxxxxxxxxxx>]

Gavin Panella has proposed merging lp:~allenap/launchpad/check-mappings-and-sets-for-proxy into lp:launchpad/devel.

Requested reviews:
  Launchpad code reviewers (launchpad-reviewers)


This makes is_security_proxied_or_harmless() check set, frozenset and mapping types for proxied objects. Previously objects of these types would be considered okay.

I have also removed a superfluous makePackageDiff() method in the object factory. There are two methods with than name in a single class, so I removed the earlier one because it is obviously unused.

-- 
https://code.launchpad.net/~allenap/launchpad/check-mappings-and-sets-for-proxy/+merge/38539
Your team Launchpad code reviewers is requested to review the proposed merge of lp:~allenap/launchpad/check-mappings-and-sets-for-proxy into lp:launchpad/devel.

=== modified file 'lib/lp/testing/factory.py'
--- lib/lp/testing/factory.py	2010-10-14 12:57:25 +0000
+++ lib/lp/testing/factory.py	2010-10-15 13:56:49 +0000
@@ -33,7 +33,10 @@
     make_msgid,
     )
 from itertools import count
-from operator import isSequenceType
+from operator import (
+    isMappingType,
+    isSequenceType,
+    )
 import os
 from random import randint
 from StringIO import StringIO
@@ -237,17 +240,13 @@
 from lp.soyuz.interfaces.component import IComponentSet
 from lp.soyuz.interfaces.packageset import IPackagesetSet
 from lp.soyuz.interfaces.processor import IProcessorFamilySet
-from lp.soyuz.interfaces.publishing import (
-    IPublishingSet,
-    )
+from lp.soyuz.interfaces.publishing import IPublishingSet
 from lp.soyuz.interfaces.section import ISectionSet
 from lp.soyuz.model.files import (
     BinaryPackageFile,
     SourcePackageReleaseFile,
     )
-from lp.soyuz.model.packagediff import (
-    PackageDiff,
-    )
+from lp.soyuz.model.packagediff import PackageDiff
 from lp.soyuz.model.processor import ProcessorFamilySet
 from lp.testing import (
     ANONYMOUS,
@@ -1844,22 +1843,6 @@
             expires=expires, restricted=restricted)
         return library_file_alias
 
-    def makePackageDiff(self, from_spr=None, to_spr=None):
-        """Make a completed package diff."""
-        if from_spr is None:
-            from_spr = self.makeSourcePackageRelease()
-        if to_spr is None:
-            to_spr = self.makeSourcePackageRelease()
-
-        diff = from_spr.requestDiffTo(
-            from_spr.creator, to_spr)
-
-        naked_diff = removeSecurityProxy(diff)
-        naked_diff.status = PackageDiffStatus.COMPLETED
-        naked_diff.diff_content = self.makeLibraryFileAlias()
-        naked_diff.date_fulfilled = UTC_NOW
-        return diff
-
     def makeDistribution(self, name=None, displayname=None, owner=None,
                          members=None, title=None, aliases=None):
         """Make a new distribution."""
@@ -3180,9 +3163,17 @@
 # Some factory methods return simple Python types. We don't add
 # security wrappers for them, as well as for objects created by
 # other Python libraries.
-unwrapped_types = (
-    BaseRecipeBranch, DSCFile, InstanceType, MergeDirective2, Message,
-    datetime, int, str, unicode)
+unwrapped_types = frozenset((
+        BaseRecipeBranch,
+        DSCFile,
+        InstanceType,
+        MergeDirective2,
+        Message,
+        datetime,
+        int,
+        str,
+        unicode,
+        ))
 
 
 def is_security_proxied_or_harmless(obj):
@@ -3193,11 +3184,15 @@
         return True
     if type(obj) in unwrapped_types:
         return True
-    if isSequenceType(obj):
-        for element in obj:
-            if not is_security_proxied_or_harmless(element):
-                return False
-        return True
+    if isSequenceType(obj) or isinstance(obj, (set, frozenset)):
+        return all(
+            is_security_proxied_or_harmless(element)
+            for element in obj)
+    if isMappingType(obj):
+        return all(
+            (is_security_proxied_or_harmless(key) and
+             is_security_proxied_or_harmless(obj[key]))
+            for key in obj)
     return False
 
 

=== modified file 'lib/lp/testing/tests/test_factory.py'
--- lib/lp/testing/tests/test_factory.py	2010-09-21 18:43:27 +0000
+++ lib/lp/testing/tests/test_factory.py	2010-10-15 13:56:49 +0000
@@ -27,8 +27,6 @@
 from lp.registry.interfaces.sourcepackage import SourcePackageFileType
 from lp.registry.interfaces.suitesourcepackage import ISuiteSourcePackage
 from lp.services.worlddata.interfaces.language import ILanguage
-from lp.soyuz.interfaces.binarypackagebuild import IBinaryPackageBuild
-from lp.soyuz.interfaces.binarypackagename import IBinaryPackageName
 from lp.soyuz.enums import (
     BinaryPackageFileType,
     BinaryPackageFormat,
@@ -36,9 +34,9 @@
     PackagePublishingStatus,
     PackageUploadStatus,
     )
-from lp.soyuz.interfaces.binarypackagerelease import (
-    IBinaryPackageRelease,
-    )
+from lp.soyuz.interfaces.binarypackagebuild import IBinaryPackageBuild
+from lp.soyuz.interfaces.binarypackagename import IBinaryPackageName
+from lp.soyuz.interfaces.binarypackagerelease import IBinaryPackageRelease
 from lp.soyuz.interfaces.files import (
     IBinaryPackageFile,
     ISourcePackageReleaseFile,
@@ -48,9 +46,7 @@
     ISourcePackagePublishingHistory,
     PackagePublishingPocket,
     )
-from lp.soyuz.interfaces.queue import (
-    IPackageUpload,
-    )
+from lp.soyuz.interfaces.queue import IPackageUpload
 from lp.soyuz.interfaces.sourcepackagerelease import ISourcePackageRelease
 from lp.testing import TestCaseWithFactory
 from lp.testing.factory import is_security_proxied_or_harmless
@@ -643,20 +639,50 @@
         self.assertFalse(is_security_proxied_or_harmless(unproxied_person))
 
     def test_is_security_proxied_or_harmless__sequence_harmless_content(self):
-        # is_security_proxied_or_harmless() checks all elements
-        # of a sequence. If all elements are harmless, so is the
-        # sequence.
+        # is_security_proxied_or_harmless() checks all elements of a sequence
+        # (including set and frozenset). If all elements are harmless, so is
+        # the sequence.
         proxied_person = self.factory.makePerson()
         self.assertTrue(
             is_security_proxied_or_harmless([1, '2', proxied_person]))
+        self.assertTrue(
+            is_security_proxied_or_harmless(
+                set([1, '2', proxied_person])))
+        self.assertTrue(
+            is_security_proxied_or_harmless(
+                frozenset([1, '2', proxied_person])))
 
     def test_is_security_proxied_or_harmless__sequence_harmful_content(self):
-        # is_security_proxied_or_harmless() checks all elements
-        # of a sequence. If at least one element is harmful, so is the
-        # sequence.
+        # is_security_proxied_or_harmless() checks all elements of a sequence
+        # (including set and frozenset). If at least one element is harmful,
+        # so is the sequence.
         unproxied_person = removeSecurityProxy(self.factory.makePerson())
         self.assertFalse(
             is_security_proxied_or_harmless([1, '2', unproxied_person]))
+        self.assertFalse(
+            is_security_proxied_or_harmless(
+                set([1, '2', unproxied_person])))
+        self.assertFalse(
+            is_security_proxied_or_harmless(
+                frozenset([1, '2', unproxied_person])))
+
+    def test_is_security_proxied_or_harmless__mapping_harmless_content(self):
+        # is_security_proxied_or_harmless() checks all keys and values in a
+        # mapping. If all elements are harmless, so is the mapping.
+        proxied_person = self.factory.makePerson()
+        self.assertTrue(
+            is_security_proxied_or_harmless({1: proxied_person}))
+        self.assertTrue(
+            is_security_proxied_or_harmless({proxied_person: 1}))
+
+    def test_is_security_proxied_or_harmless__mapping_harmful_content(self):
+        # is_security_proxied_or_harmless() checks all keys and values in a
+        # mapping. If at least one element is harmful, so is the mapping.
+        unproxied_person = removeSecurityProxy(self.factory.makePerson())
+        self.assertFalse(
+            is_security_proxied_or_harmless({1: unproxied_person}))
+        self.assertFalse(
+            is_security_proxied_or_harmless({unproxied_person: 1}))
 
 
 def test_suite():