Only allow one entity to come from a BQ access grant.

Fixes googleapis#1050.

gcloud/bigquery/dataset.py

@@ -23,24 +23,53 @@
 class AccessGrant(object):
     """Represent grant of an access role to an entity.
 
+    Every entry in the access list will have exactly one of
+    ``userByEmail``, ``groupByEmail``, ``domain``, ``specialGroup`` or
+    ``view`` set. And if anything but ``view`` is set, it'll also have a
+    ``role`` specified. ``role`` is omitted for a ``view``, since
+    ``view`` s are always read-only.
+
+    See https://cloud.google.com/bigquery/docs/reference/v2/datasets.
+
     :type role: string
     :param role: Role granted to the entity. One of
 
                  * ``'OWNER'``
                  * ``'WRITER'``
                  * ``'READER'``
 
+                 May also be ``None`` if the ``entity_type`` is ``view``.
+
     :type entity_type: string
     :param entity_type: Type of entity being granted the role. One of
-
-                        * ``'specialGroup'``
-                        * ``'groupByEmail'``
-                        * ``'userByEmail'``
+                        :attr:`ENTITY_TYPES`.
 
     :type entity_id: string
     :param entity_id: ID of entity being granted the role.
+
+    :raises: :class:`ValueError` if the ``entity_type`` is not among
+             :attr:`ENTITY_TYPES`, or if a ``view`` has ``role`` set or
+             a non ``view`` **does not** have a ``role`` set.
     """
+
+    ENTITY_TYPES = frozenset(['userByEmail', 'groupByEmail', 'domain',
+                              'specialGroup', 'view'])
+    """Allowed entity types."""
+
     def __init__(self, role, entity_type, entity_id):
+        if entity_type not in self.ENTITY_TYPES:
+            message = 'Entity type %r not among: %s' % (
+                entity_type, ', '.join(self.ENTITY_TYPES))
+            raise ValueError(message)
+        if entity_type == 'view':
+            if role is not None:
+                raise ValueError('Role must be None for a view. Received '
+                                 'role: %r' % (role,))
+        else:
+            if role is None:
+                raise ValueError('Role must be set for entity '
+                                 'type %r' % (entity_type,))
+
         self.role = role
         self.entity_type = entity_type
         self.entity_id = entity_id
@@ -297,22 +326,26 @@ def _require_client(self, client):
     def _parse_access_grants(access):
         """Parse a resource fragment into a set of access grants.
 
+        ``role`` augments the entity type and present **unless** the entity
+        type is ``view``.
+
         :type access: list of mappings
         :param access: each mapping represents a single access grant
 
         :rtype: list of :class:`AccessGrant`
         :returns: a list of parsed grants
+        :raises: :class:`ValueError` if a grant in ``access`` has more keys
+                 than ``role`` and one additional key.
         """
         result = []
         for grant in access:
             grant = grant.copy()
-            role = grant.pop('role')
-            # Hypothetical case:  we don't know that the back-end will ever
-            # return such structures, but they are logical.  See:
-            # https://github.com/GoogleCloudPlatform/gcloud-python/pull/1046#discussion_r36687769
-            for entity_type, entity_id in sorted(grant.items()):
-                result.append(
-                    AccessGrant(role, entity_type, entity_id))
+            role = grant.pop('role', None)
+            entity_type, entity_id = grant.popitem()
+            if len(grant) != 0:
+                raise ValueError('Grant has unexpected keys remaining.', grant)
+            result.append(
+                AccessGrant(role, entity_type, entity_id))
         return result
 
     def _set_properties(self, api_response):
@@ -335,7 +368,9 @@ def _build_access_resource(self):
         """Generate a resource fragment for dataset's access grants."""
         result = []
         for grant in self.access_grants:
-            info = {'role': grant.role, grant.entity_type: grant.entity_id}
+            info = {grant.entity_type: grant.entity_id}
+            if grant.role is not None:
+                info['role'] = grant.role
             result.append(info)
         return result
 

gcloud/bigquery/test_dataset.py

@@ -30,6 +30,31 @@ def test_ctor_defaults(self):
         self.assertEqual(grant.entity_type, 'userByEmail')
         self.assertEqual(grant.entity_id, 'phred@example.com')
 
+    def test_ctor_bad_entity_type(self):
+        with self.assertRaises(ValueError):
+            self._makeOne(None, 'unknown', None)
+
+    def test_ctor_view_with_role(self):
+        role = 'READER'
+        entity_type = 'view'
+        with self.assertRaises(ValueError):
+            self._makeOne(role, entity_type, None)
+
+    def test_ctor_view_success(self):
+        role = None
+        entity_type = 'view'
+        entity_id = object()
+        grant = self._makeOne(role, entity_type, entity_id)
+        self.assertEqual(grant.role, role)
+        self.assertEqual(grant.entity_type, entity_type)
+        self.assertEqual(grant.entity_id, entity_id)
+
+    def test_ctor_nonview_without_role(self):
+        role = None
+        entity_type = 'userByEmail'
+        with self.assertRaises(ValueError):
+            self._makeOne(role, entity_type, None)
+
 
 class TestDataset(unittest2.TestCase):
     PROJECT = 'project'
@@ -249,39 +274,27 @@ def test_from_api_repr_w_properties(self):
         self._verifyResourceProperties(dataset, RESOURCE)
 
     def test__parse_access_grants_w_unknown_entity_type(self):
-        USER_EMAIL = 'phred@example.com'
-        GROUP_EMAIL = 'group-name@lists.example.com'
-        RESOURCE = {
-            'access': [
-                {'role': 'OWNER', 'userByEmail': USER_EMAIL},
-                {'role': 'WRITER', 'groupByEmail': GROUP_EMAIL},
-                {'role': 'READER', 'specialGroup': 'projectReaders'},
-                {'role': 'READER', 'unknown': 'UNKNOWN'}]
-        }
+        ACCESS = [
+            {'role': 'READER', 'unknown': 'UNKNOWN'},
+        ]
         client = _Client(self.PROJECT)
         dataset = self._makeOne(self.DS_NAME, client=client)
-        grants = dataset._parse_access_grants(RESOURCE['access'])
-        self._verifyAccessGrants(grants, RESOURCE)
+        with self.assertRaises(ValueError):
+            dataset._parse_access_grants(ACCESS)
 
-    def test__parse_access_grants_w_multiple_entity_types(self):
-        # Hypothetical case:  we don't know that the back-end will ever
-        # return such structures, but they are logical.  See:
-        # https://github.com/GoogleCloudPlatform/gcloud-python/pull/1046#discussion_r36687769
+    def test__parse_access_grants_w_extra_keys(self):
         USER_EMAIL = 'phred@example.com'
-        OTHER_EMAIL = 'bharney@example.com'
-        GROUP_EMAIL = 'group-name@lists.example.com'
-        RESOURCE = {
-            'access': [
-                {'role': 'OWNER', 'userByEmail': USER_EMAIL},
-                {'role': 'WRITER', 'groupByEmail': GROUP_EMAIL},
-                {'role': 'READER',
-                 'specialGroup': 'projectReaders',
-                 'userByEmail': OTHER_EMAIL}]
-        }
+        ACCESS = [
+            {
+                'role': 'READER',
+                'specialGroup': 'projectReaders',
+                'userByEmail': USER_EMAIL,
+            },
+        ]
         client = _Client(self.PROJECT)
         dataset = self._makeOne(self.DS_NAME, client=client)
-        grants = dataset._parse_access_grants(RESOURCE['access'])
-        self._verifyAccessGrants(grants, RESOURCE)
+        with self.assertRaises(ValueError):
+            dataset._parse_access_grants(ACCESS)
 
     def test_create_w_bound_client(self):
         PATH = 'projects/%s/datasets' % self.PROJECT
@@ -320,11 +333,19 @@ def test_create_w_alternate_client(self):
         dataset = self._makeOne(self.DS_NAME, client=CLIENT1)
         dataset.friendly_name = TITLE
         dataset.description = DESCRIPTION
+        VIEW = {
+            'projectId': 'my-proj',
+            'datasetId': 'starry-skies',
+            'tableId': 'northern-hemisphere',
+        }
         dataset.access_grants = [
             AccessGrant('OWNER', 'userByEmail', USER_EMAIL),
             AccessGrant('OWNER', 'groupByEmail', GROUP_EMAIL),
+            AccessGrant('READER', 'domain', 'foo.com'),
             AccessGrant('READER', 'specialGroup', 'projectReaders'),
-            AccessGrant('WRITER', 'specialGroup', 'projectWriters')]
+            AccessGrant('WRITER', 'specialGroup', 'projectWriters'),
+            AccessGrant(None, 'view', VIEW),
+        ]
 
         dataset.create(client=CLIENT2)
 
@@ -334,15 +355,20 @@ def test_create_w_alternate_client(self):
         self.assertEqual(req['method'], 'POST')
         self.assertEqual(req['path'], '/%s' % PATH)
         SENT = {
-            'datasetReference':
-                {'projectId': self.PROJECT, 'datasetId': self.DS_NAME},
+            'datasetReference': {
+                'projectId': self.PROJECT,
+                'datasetId': self.DS_NAME,
+            },
             'description': DESCRIPTION,
             'friendlyName': TITLE,
             'access': [
                 {'role': 'OWNER', 'userByEmail': USER_EMAIL},
                 {'role': 'OWNER', 'groupByEmail': GROUP_EMAIL},
+                {'role': 'READER', 'domain': 'foo.com'},
                 {'role': 'READER', 'specialGroup': 'projectReaders'},
-                {'role': 'WRITER', 'specialGroup': 'projectWriters'}],
+                {'role': 'WRITER', 'specialGroup': 'projectWriters'},
+                {'view': VIEW},
+            ],
         }
         self.assertEqual(req['data'], SENT)
         self._verifyResourceProperties(dataset, RESOURCE)