From 1633962d1f2cd0efaadb8138572bfa3b288a9b06 Mon Sep 17 00:00:00 2001 From: Tres Seaver Date: Wed, 19 Apr 2017 11:16:06 -0400 Subject: [PATCH] Store policy bindings as sets, not frozensets. (#3308) The legacy accessors still return frozensets, as they cannot safely be mutated in plcae. --- core/google/cloud/iam.py | 10 ++--- core/tests/unit/test_iam.py | 64 +++++++++++++++++-------------- pubsub/google/cloud/pubsub/iam.py | 4 +- pubsub/tests/unit/test_iam.py | 37 +++++++++--------- 4 files changed, 60 insertions(+), 55 deletions(-) diff --git a/core/google/cloud/iam.py b/core/google/cloud/iam.py index 4747b39bbf07..653cebda1e71 100644 --- a/core/google/cloud/iam.py +++ b/core/google/cloud/iam.py @@ -71,7 +71,7 @@ def __getitem__(self, key): return self._bindings[key] def __setitem__(self, key, value): - self._bindings[key] = frozenset(value) + self._bindings[key] = set(value) def __delitem__(self, key): del self._bindings[key] @@ -91,7 +91,7 @@ def owners(self, value): warnings.warn( _ASSIGNMENT_DEPRECATED_MSG.format('owners', OWNER_ROLE), DeprecationWarning) - self._bindings[OWNER_ROLE] = list(value) + self[OWNER_ROLE] = value @property def editors(self): @@ -108,7 +108,7 @@ def editors(self, value): warnings.warn( _ASSIGNMENT_DEPRECATED_MSG.format('editors', EDITOR_ROLE), DeprecationWarning) - self._bindings[EDITOR_ROLE] = list(value) + self[EDITOR_ROLE] = value @property def viewers(self): @@ -125,7 +125,7 @@ def viewers(self, value): warnings.warn( _ASSIGNMENT_DEPRECATED_MSG.format('viewers', VIEWER_ROLE), DeprecationWarning) - self._bindings[VIEWER_ROLE] = list(value) + self[VIEWER_ROLE] = value @staticmethod def user(email): @@ -209,7 +209,7 @@ def from_api_repr(cls, resource): for binding in resource.get('bindings', ()): role = binding['role'] members = sorted(binding['members']) - policy._bindings[role] = members + policy[role] = members return policy def to_api_repr(self): diff --git a/core/tests/unit/test_iam.py b/core/tests/unit/test_iam.py index 9f1bd9b3904f..42fac7c623c1 100644 --- a/core/tests/unit/test_iam.py +++ b/core/tests/unit/test_iam.py @@ -27,27 +27,26 @@ def _make_one(self, *args, **kw): return self._get_target_class()(*args, **kw) def test_ctor_defaults(self): + empty = frozenset() policy = self._make_one() self.assertIsNone(policy.etag) self.assertIsNone(policy.version) - self.assertIsInstance(policy.owners, frozenset) - self.assertEqual(list(policy.owners), []) - self.assertIsInstance(policy.editors, frozenset) - self.assertEqual(list(policy.editors), []) - self.assertIsInstance(policy.viewers, frozenset) - self.assertEqual(list(policy.viewers), []) + self.assertEqual(policy.owners, empty) + self.assertEqual(policy.editors, empty) + self.assertEqual(policy.viewers, empty) self.assertEqual(len(policy), 0) self.assertEqual(dict(policy), {}) def test_ctor_explicit(self): VERSION = 17 ETAG = 'ETAG' + empty = frozenset() policy = self._make_one(ETAG, VERSION) self.assertEqual(policy.etag, ETAG) self.assertEqual(policy.version, VERSION) - self.assertEqual(list(policy.owners), []) - self.assertEqual(list(policy.editors), []) - self.assertEqual(list(policy.viewers), []) + self.assertEqual(policy.owners, empty) + self.assertEqual(policy.editors, empty) + self.assertEqual(policy.viewers, empty) self.assertEqual(len(policy), 0) self.assertEqual(dict(policy), {}) @@ -58,7 +57,7 @@ def test___getitem___miss(self): def test___setitem__(self): USER = 'user:phred@example.com' - PRINCIPALS = frozenset([USER]) + PRINCIPALS = set([USER]) policy = self._make_one() policy['rolename'] = [USER] self.assertEqual(policy['rolename'], PRINCIPALS) @@ -80,54 +79,59 @@ def test___delitem___miss(self): def test_owners_getter(self): from google.cloud.iam import OWNER_ROLE MEMBER = 'user:phred@example.com' + expected = frozenset([MEMBER]) policy = self._make_one() policy[OWNER_ROLE] = [MEMBER] - self.assertIsInstance(policy.owners, frozenset) - self.assertEqual(list(policy.owners), [MEMBER]) + self.assertEqual(policy.owners, expected) def test_owners_setter(self): import warnings from google.cloud.iam import OWNER_ROLE MEMBER = 'user:phred@example.com' + expected = set([MEMBER]) policy = self._make_one() with warnings.catch_warnings(): + warnings.simplefilter('always') policy.owners = [MEMBER] - self.assertEqual(list(policy[OWNER_ROLE]), [MEMBER]) + self.assertEqual(policy[OWNER_ROLE], expected) def test_editors_getter(self): from google.cloud.iam import EDITOR_ROLE MEMBER = 'user:phred@example.com' + expected = frozenset([MEMBER]) policy = self._make_one() policy[EDITOR_ROLE] = [MEMBER] - self.assertIsInstance(policy.editors, frozenset) - self.assertEqual(list(policy.editors), [MEMBER]) + self.assertEqual(policy.editors, expected) def test_editors_setter(self): import warnings from google.cloud.iam import EDITOR_ROLE MEMBER = 'user:phred@example.com' + expected = set([MEMBER]) policy = self._make_one() with warnings.catch_warnings(): + warnings.simplefilter('always') policy.editors = [MEMBER] - self.assertEqual(list(policy[EDITOR_ROLE]), [MEMBER]) + self.assertEqual(policy[EDITOR_ROLE], expected) def test_viewers_getter(self): from google.cloud.iam import VIEWER_ROLE MEMBER = 'user:phred@example.com' + expected = frozenset([MEMBER]) policy = self._make_one() policy[VIEWER_ROLE] = [MEMBER] - self.assertIsInstance(policy.viewers, frozenset) - self.assertEqual(list(policy.viewers), [MEMBER]) + self.assertEqual(policy.viewers, expected) def test_viewers_setter(self): import warnings from google.cloud.iam import VIEWER_ROLE MEMBER = 'user:phred@example.com' + expected = set([MEMBER]) policy = self._make_one() with warnings.catch_warnings(): warnings.simplefilter('always') policy.viewers = [MEMBER] - self.assertEqual(list(policy[VIEWER_ROLE]), [MEMBER]) + self.assertEqual(policy[VIEWER_ROLE], expected) def test_user(self): EMAIL = 'phred@example.com' @@ -162,6 +166,7 @@ def test_authenticated_users(self): self.assertEqual(policy.authenticated_users(), 'allAuthenticatedUsers') def test_from_api_repr_only_etag(self): + empty = frozenset() RESOURCE = { 'etag': 'ACAB', } @@ -169,9 +174,9 @@ def test_from_api_repr_only_etag(self): policy = klass.from_api_repr(RESOURCE) self.assertEqual(policy.etag, 'ACAB') self.assertIsNone(policy.version) - self.assertEqual(list(policy.owners), []) - self.assertEqual(list(policy.editors), []) - self.assertEqual(list(policy.viewers), []) + self.assertEqual(policy.owners, empty) + self.assertEqual(policy.editors, empty) + self.assertEqual(policy.viewers, empty) self.assertEqual(dict(policy), {}) def test_from_api_repr_complete(self): @@ -196,18 +201,19 @@ def test_from_api_repr_complete(self): {'role': VIEWER_ROLE, 'members': [VIEWER1, VIEWER2]}, ], } + empty = frozenset() klass = self._get_target_class() policy = klass.from_api_repr(RESOURCE) self.assertEqual(policy.etag, 'DEADBEEF') self.assertEqual(policy.version, 17) - self.assertEqual(sorted(policy.owners), [OWNER1, OWNER2]) - self.assertEqual(sorted(policy.editors), [EDITOR1, EDITOR2]) - self.assertEqual(sorted(policy.viewers), [VIEWER1, VIEWER2]) + self.assertEqual(policy.owners, frozenset([OWNER1, OWNER2])) + self.assertEqual(policy.editors, frozenset([EDITOR1, EDITOR2])) + self.assertEqual(policy.viewers, frozenset([VIEWER1, VIEWER2])) self.assertEqual( dict(policy), { - OWNER_ROLE: [OWNER1, OWNER2], - EDITOR_ROLE: [EDITOR1, EDITOR2], - VIEWER_ROLE: [VIEWER1, VIEWER2], + OWNER_ROLE: set([OWNER1, OWNER2]), + EDITOR_ROLE: set([EDITOR1, EDITOR2]), + VIEWER_ROLE: set([VIEWER1, VIEWER2]), }) def test_from_api_repr_unknown_role(self): @@ -224,7 +230,7 @@ def test_from_api_repr_unknown_role(self): policy = klass.from_api_repr(RESOURCE) self.assertEqual(policy.etag, 'DEADBEEF') self.assertEqual(policy.version, 17) - self.assertEqual(dict(policy), {'unknown': [GROUP, USER]}) + self.assertEqual(dict(policy), {'unknown': set([GROUP, USER])}) def test_to_api_repr_defaults(self): policy = self._make_one() diff --git a/pubsub/google/cloud/pubsub/iam.py b/pubsub/google/cloud/pubsub/iam.py index e92f2151dc05..9c7e46af222a 100644 --- a/pubsub/google/cloud/pubsub/iam.py +++ b/pubsub/google/cloud/pubsub/iam.py @@ -121,7 +121,7 @@ def publishers(self, value): _ASSIGNMENT_DEPRECATED_MSG.format( 'publishers', PUBSUB_PUBLISHER_ROLE), DeprecationWarning) - self._bindings[PUBSUB_PUBLISHER_ROLE] = list(value) + self[PUBSUB_PUBLISHER_ROLE] = value @property def subscribers(self): @@ -135,4 +135,4 @@ def subscribers(self, value): _ASSIGNMENT_DEPRECATED_MSG.format( 'subscribers', PUBSUB_SUBSCRIBER_ROLE), DeprecationWarning) - self._bindings[PUBSUB_SUBSCRIBER_ROLE] = list(value) + self[PUBSUB_SUBSCRIBER_ROLE] = value diff --git a/pubsub/tests/unit/test_iam.py b/pubsub/tests/unit/test_iam.py index 3bf4aaa922f0..475d375d0cd8 100644 --- a/pubsub/tests/unit/test_iam.py +++ b/pubsub/tests/unit/test_iam.py @@ -27,31 +27,28 @@ def _make_one(self, *args, **kw): return self._get_target_class()(*args, **kw) def test_ctor_defaults(self): + empty = frozenset() policy = self._make_one() self.assertIsNone(policy.etag) self.assertIsNone(policy.version) - self.assertIsInstance(policy.owners, frozenset) - self.assertEqual(list(policy.owners), []) - self.assertIsInstance(policy.editors, frozenset) - self.assertEqual(list(policy.editors), []) - self.assertIsInstance(policy.viewers, frozenset) - self.assertEqual(list(policy.viewers), []) - self.assertIsInstance(policy.publishers, frozenset) - self.assertEqual(list(policy.publishers), []) - self.assertIsInstance(policy.subscribers, frozenset) - self.assertEqual(list(policy.subscribers), []) + self.assertEqual(policy.owners, empty) + self.assertEqual(policy.editors, empty) + self.assertEqual(policy.viewers, empty) + self.assertEqual(policy.publishers, empty) + self.assertEqual(policy.subscribers, empty) def test_ctor_explicit(self): VERSION = 17 ETAG = 'ETAG' + empty = frozenset() policy = self._make_one(ETAG, VERSION) self.assertEqual(policy.etag, ETAG) self.assertEqual(policy.version, VERSION) - self.assertEqual(list(policy.owners), []) - self.assertEqual(list(policy.editors), []) - self.assertEqual(list(policy.viewers), []) - self.assertEqual(list(policy.publishers), []) - self.assertEqual(list(policy.subscribers), []) + self.assertEqual(policy.owners, empty) + self.assertEqual(policy.editors, empty) + self.assertEqual(policy.viewers, empty) + self.assertEqual(policy.publishers, empty) + self.assertEqual(policy.subscribers, empty) def test_publishers_setter(self): import warnings @@ -59,13 +56,14 @@ def test_publishers_setter(self): PUBSUB_PUBLISHER_ROLE, ) PUBLISHER = 'user:phred@example.com' + expected = set([PUBLISHER]) policy = self._make_one() with warnings.catch_warnings(): policy.publishers = [PUBLISHER] - self.assertEqual(sorted(policy.publishers), [PUBLISHER]) + self.assertEqual(policy.publishers, frozenset(expected)) self.assertEqual( - dict(policy), {PUBSUB_PUBLISHER_ROLE: [PUBLISHER]}) + dict(policy), {PUBSUB_PUBLISHER_ROLE: expected}) def test_subscribers_setter(self): import warnings @@ -73,10 +71,11 @@ def test_subscribers_setter(self): PUBSUB_SUBSCRIBER_ROLE, ) SUBSCRIBER = 'serviceAccount:1234-abcdef@service.example.com' + expected = set([SUBSCRIBER]) policy = self._make_one() with warnings.catch_warnings(): policy.subscribers = [SUBSCRIBER] - self.assertEqual(sorted(policy.subscribers), [SUBSCRIBER]) + self.assertEqual(policy.subscribers, frozenset(expected)) self.assertEqual( - dict(policy), {PUBSUB_SUBSCRIBER_ROLE: [SUBSCRIBER]}) + dict(policy), {PUBSUB_SUBSCRIBER_ROLE: expected})