From be7b734600f7592fd50480d24312f842f32f5008 Mon Sep 17 00:00:00 2001 From: Alan Wu Date: Wed, 6 Dec 2017 19:31:39 -0800 Subject: [PATCH 1/2] Storage: ProjectID Regex from https://cloud.google.com/resource-manager/reference/rest/v1beta1/projects#Project.FIELDS.project_id --- storage/google/cloud/storage/notification.py | 6 +- storage/tests/unit/test_notification.py | 170 ++++++++++++++++++- 2 files changed, 173 insertions(+), 3 deletions(-) diff --git a/storage/google/cloud/storage/notification.py b/storage/google/cloud/storage/notification.py index a4c5a66a4d5a..354c4970664f 100644 --- a/storage/google/cloud/storage/notification.py +++ b/storage/google/cloud/storage/notification.py @@ -28,10 +28,12 @@ NONE_PAYLOAD_FORMAT = 'NONE' _TOPIC_REF_FMT = '//pubsub.googleapis.com/projects/{}/topics/{}' -_PROJECT_PATTERN = r'(?P[a-z]+-[a-z]+-\d+)' +_PROJECT_PATTERN = r'(?P[a-z][a-z0-9-]{4,28}[a-z0-9])' _TOPIC_NAME_PATTERN = r'(?P[A-Za-z](\w|[-_.~+%])+)' _TOPIC_REF_PATTERN = _TOPIC_REF_FMT.format( - _PROJECT_PATTERN, _TOPIC_NAME_PATTERN) + _PROJECT_PATTERN.replace(r'\A', '').replace(r'\Z', ''), + _TOPIC_NAME_PATTERN +) _TOPIC_REF_RE = re.compile(_TOPIC_REF_PATTERN) diff --git a/storage/tests/unit/test_notification.py b/storage/tests/unit/test_notification.py index ffd33280c4bc..ea191233a6e9 100644 --- a/storage/tests/unit/test_notification.py +++ b/storage/tests/unit/test_notification.py @@ -12,6 +12,7 @@ # See the License for the specific language governing permissions and # limitations under the License. +import re import unittest import mock @@ -130,6 +131,174 @@ def test_from_api_repr_no_topic(self): with self.assertRaises(ValueError): klass.from_api_repr(resource, bucket=bucket) + def test_from_api_repr_project_name_too_long_fail(self): + klass = self._get_target_class() + client = self._make_client() + project = 'a' * 31 + bucket = self._make_bucket(client) + self.TOPIC_REF = self.TOPIC_REF.replace(self.BUCKET_PROJECT, project) + resource = { + 'topic': self.TOPIC_REF, + } + with self.assertRaises(ValueError): + klass.from_api_repr(resource, bucket=bucket) + + def test_from_api_repr_project_name_maximum_length_succeeds(self): + klass = self._get_target_class() + client = self._make_client() + project = 'a' * 30 + bucket = self._make_bucket(client) + self.TOPIC_REF = self.TOPIC_REF.replace(self.BUCKET_PROJECT, project) + resource = { + 'topic': self.TOPIC_REF, + } + notification = klass.from_api_repr(resource, bucket=bucket) + self.assertEqual(notification.topic_project, project) + + def test_from_api_repr_below_min_length_fail(self): + klass = self._get_target_class() + client = self._make_client() + project = 'a' * 5 + bucket = self._make_bucket(client) + self.TOPIC_REF = self.TOPIC_REF.replace(self.BUCKET_PROJECT, project) + resource = { + 'topic': self.TOPIC_REF, + } + with self.assertRaises(ValueError): + klass.from_api_repr(resource, bucket=bucket) + + def test_from_api_repr_above_max_length_fail(self): + klass = self._get_target_class() + client = self._make_client() + project = 'a' * 31 + bucket = self._make_bucket(client) + self.TOPIC_REF = self.TOPIC_REF.replace(self.BUCKET_PROJECT, project) + resource = { + 'topic': self.TOPIC_REF, + } + with self.assertRaises(ValueError): + klass.from_api_repr(resource, bucket=bucket) + + def test_from_api_repr_below_max_length_success(self): + klass = self._get_target_class() + client = self._make_client() + project = 'a' * 30 + bucket = self._make_bucket(client) + self.TOPIC_REF = self.TOPIC_REF.replace(self.BUCKET_PROJECT, project) + resource = { + 'topic': self.TOPIC_REF, + } + notification = klass.from_api_repr(resource, bucket=bucket) + self.assertEqual(notification.topic_project, project) + + def test_from_api_repr_above_max_length_fail(self): + klass = self._get_target_class() + client = self._make_client() + project = 'a' * 31 + bucket = self._make_bucket(client) + self.TOPIC_REF = self.TOPIC_REF.replace(self.BUCKET_PROJECT, project) + resource = { + 'topic': self.TOPIC_REF, + } + with self.assertRaises(ValueError): + klass.from_api_repr(resource, bucket=bucket) + + def test_from_api_repr_capital_letters_fail(self): + klass = self._get_target_class() + client = self._make_client() + project = 'aaaAaa' + bucket = self._make_bucket(client) + self.TOPIC_REF = self.TOPIC_REF.replace(self.BUCKET_PROJECT, project) + resource = { + 'topic': self.TOPIC_REF, + } + with self.assertRaises(ValueError): + klass.from_api_repr(resource, bucket=bucket) + + def test_from_api_repr_leading_digit_fail(self): + klass = self._get_target_class() + client = self._make_client() + project = '1' + 'a' * 5 + bucket = self._make_bucket(client) + self.TOPIC_REF = self.TOPIC_REF.replace(self.BUCKET_PROJECT, project) + resource = { + 'topic': self.TOPIC_REF, + } + with self.assertRaises(ValueError): + klass.from_api_repr(resource, bucket=bucket) + + def test_from_api_repr_leading_hyphen_fail(self): + klass = self._get_target_class() + client = self._make_client() + project = '-' + 'a' * 5 + bucket = self._make_bucket(client) + self.TOPIC_REF = self.TOPIC_REF.replace(self.BUCKET_PROJECT, project) + resource = { + 'topic': self.TOPIC_REF, + } + with self.assertRaises(ValueError): + klass.from_api_repr(resource, bucket=bucket) + + def test_from_api_repr_trailing_hyphen_fail(self): + klass = self._get_target_class() + client = self._make_client() + project = 'a' * 5 + '-' + bucket = self._make_bucket(client) + self.TOPIC_REF = self.TOPIC_REF.replace(self.BUCKET_PROJECT, project) + resource = { + 'topic': self.TOPIC_REF, + } + with self.assertRaises(ValueError): + klass.from_api_repr(resource, bucket=bucket) + + def test_from_api_repr_hyphens_success(self): + klass = self._get_target_class() + client = self._make_client() + project = 'a' + '-' * 28 + 'a' + bucket = self._make_bucket(client) + self.TOPIC_REF = self.TOPIC_REF.replace(self.BUCKET_PROJECT, project) + resource = { + 'topic': self.TOPIC_REF, + } + notification = klass.from_api_repr(resource, bucket=bucket) + self.assertEqual(notification.topic_project, project) + + def test_from_api_repr_letters_success(self): + klass = self._get_target_class() + client = self._make_client() + project = 'abcdefghijklmnopqrstuvwxyz' + bucket = self._make_bucket(client) + self.TOPIC_REF = self.TOPIC_REF.replace(self.BUCKET_PROJECT, project) + resource = { + 'topic': self.TOPIC_REF, + } + notification = klass.from_api_repr(resource, bucket=bucket) + self.assertEqual(notification.topic_project, project) + + def test_from_api_repr_digits_success(self): + klass = self._get_target_class() + client = self._make_client() + project = 'z0123456789' + bucket = self._make_bucket(client) + self.TOPIC_REF = self.TOPIC_REF.replace(self.BUCKET_PROJECT, project) + resource = { + 'topic': self.TOPIC_REF, + } + notification = klass.from_api_repr(resource, bucket=bucket) + self.assertEqual(notification.topic_project, project) + + def test_from_api_repr_assortment_success(self): + klass = self._get_target_class() + client = self._make_client() + project = 'a-bcdefghijklmn12opqrstuv0wxyz' + bucket = self._make_bucket(client) + self.TOPIC_REF = self.TOPIC_REF.replace(self.BUCKET_PROJECT, project) + resource = { + 'topic': self.TOPIC_REF, + } + notification = klass.from_api_repr(resource, bucket=bucket) + self.assertEqual(notification.topic_project, project) + def test_from_api_repr_invalid_topic(self): klass = self._get_target_class() client = self._make_client() @@ -137,7 +306,6 @@ def test_from_api_repr_invalid_topic(self): resource = { 'topic': '@#$%', } - with self.assertRaises(ValueError): klass.from_api_repr(resource, bucket=bucket) From 6fc8ded2bff0856fe5deff40dc5f6b94340b5b08 Mon Sep 17 00:00:00 2001 From: Danny Hermes Date: Tue, 12 Dec 2017 14:46:04 -0800 Subject: [PATCH 2/2] Re-factor as part of review process. Made the regex check in `BucketNotification.from_api_repr` into a standalone helper. This way all the tests can be in "one place". --- storage/google/cloud/storage/notification.py | 60 +++-- storage/tests/unit/test_notification.py | 234 ++++++------------- 2 files changed, 108 insertions(+), 186 deletions(-) diff --git a/storage/google/cloud/storage/notification.py b/storage/google/cloud/storage/notification.py index 354c4970664f..7ac6c9aaf1af 100644 --- a/storage/google/cloud/storage/notification.py +++ b/storage/google/cloud/storage/notification.py @@ -31,10 +31,12 @@ _PROJECT_PATTERN = r'(?P[a-z][a-z0-9-]{4,28}[a-z0-9])' _TOPIC_NAME_PATTERN = r'(?P[A-Za-z](\w|[-_.~+%])+)' _TOPIC_REF_PATTERN = _TOPIC_REF_FMT.format( - _PROJECT_PATTERN.replace(r'\A', '').replace(r'\Z', ''), - _TOPIC_NAME_PATTERN -) + _PROJECT_PATTERN, _TOPIC_NAME_PATTERN) _TOPIC_REF_RE = re.compile(_TOPIC_REF_PATTERN) +_BAD_TOPIC = ( + 'Resource has invalid topic: {}; see ' + 'https://cloud.google.com/storage/docs/json_api/v1/' + 'notifications/insert#topic') class BucketNotification(object): @@ -112,25 +114,12 @@ def from_api_repr(cls, resource, bucket): :rtype: :class:`BucketNotification` :returns: the new notification instance - :raises ValueError: - if resource is missing 'topic' key, or if it is not formatted - per the spec documented in - https://cloud.google.com/storage/docs/json_api/v1/notifications/insert#topic """ topic_path = resource.get('topic') if topic_path is None: raise ValueError('Resource has no topic') - match = _TOPIC_REF_RE.match(topic_path) - if match is None: - raise ValueError( - 'Resource has invalid topic: {}; see {}'.format( - topic_path, - 'https://cloud.google.com/storage/docs/json_api/v1/' - 'notifications/insert#topic')) - - name = match.group('name') - project = match.group('project') + name, project = _parse_topic_path(topic_path) instance = cls(bucket, name, topic_project=project) instance._properties = resource @@ -362,3 +351,40 @@ def delete(self, client=None): path=self.path, query_params=query_params, ) + + +def _parse_topic_path(topic_path): + """Verify that a topic path is in the correct format. + + .. _resource manager docs: https://cloud.google.com/resource-manager/\ + reference/rest/v1beta1/projects#\ + Project.FIELDS.project_id + .. _topic spec: https://cloud.google.com/storage/docs/json_api/v1/\ + notifications/insert#topic + + Expected to be of the form: + + //pubsub.googleapis.com/projects/{project}/topics/{topic} + + where the ``project`` value must be "6 to 30 lowercase letters, digits, + or hyphens. It must start with a letter. Trailing hyphens are prohibited." + (see `resource manager docs`_) and ``topic`` must have length at least two, + must start with a letter and may only contain alphanumeric characters or + ``-``, ``_``, ``.``, ``~``, ``+`` or ``%`` (i.e characters used for URL + encoding, see `topic spec`_). + + Args: + topic_path (str): The topic path to be verified. + + Returns: + Tuple[str, str]: The ``project`` and ``topic`` parsed from the + ``topic_path``. + + Raises: + ValueError: If the topic path is invalid. + """ + match = _TOPIC_REF_RE.match(topic_path) + if match is None: + raise ValueError(_BAD_TOPIC.format(topic_path)) + + return match.group('name'), match.group('project') diff --git a/storage/tests/unit/test_notification.py b/storage/tests/unit/test_notification.py index ea191233a6e9..623bc7013067 100644 --- a/storage/tests/unit/test_notification.py +++ b/storage/tests/unit/test_notification.py @@ -12,7 +12,6 @@ # See the License for the specific language governing permissions and # limitations under the License. -import re import unittest import mock @@ -131,174 +130,6 @@ def test_from_api_repr_no_topic(self): with self.assertRaises(ValueError): klass.from_api_repr(resource, bucket=bucket) - def test_from_api_repr_project_name_too_long_fail(self): - klass = self._get_target_class() - client = self._make_client() - project = 'a' * 31 - bucket = self._make_bucket(client) - self.TOPIC_REF = self.TOPIC_REF.replace(self.BUCKET_PROJECT, project) - resource = { - 'topic': self.TOPIC_REF, - } - with self.assertRaises(ValueError): - klass.from_api_repr(resource, bucket=bucket) - - def test_from_api_repr_project_name_maximum_length_succeeds(self): - klass = self._get_target_class() - client = self._make_client() - project = 'a' * 30 - bucket = self._make_bucket(client) - self.TOPIC_REF = self.TOPIC_REF.replace(self.BUCKET_PROJECT, project) - resource = { - 'topic': self.TOPIC_REF, - } - notification = klass.from_api_repr(resource, bucket=bucket) - self.assertEqual(notification.topic_project, project) - - def test_from_api_repr_below_min_length_fail(self): - klass = self._get_target_class() - client = self._make_client() - project = 'a' * 5 - bucket = self._make_bucket(client) - self.TOPIC_REF = self.TOPIC_REF.replace(self.BUCKET_PROJECT, project) - resource = { - 'topic': self.TOPIC_REF, - } - with self.assertRaises(ValueError): - klass.from_api_repr(resource, bucket=bucket) - - def test_from_api_repr_above_max_length_fail(self): - klass = self._get_target_class() - client = self._make_client() - project = 'a' * 31 - bucket = self._make_bucket(client) - self.TOPIC_REF = self.TOPIC_REF.replace(self.BUCKET_PROJECT, project) - resource = { - 'topic': self.TOPIC_REF, - } - with self.assertRaises(ValueError): - klass.from_api_repr(resource, bucket=bucket) - - def test_from_api_repr_below_max_length_success(self): - klass = self._get_target_class() - client = self._make_client() - project = 'a' * 30 - bucket = self._make_bucket(client) - self.TOPIC_REF = self.TOPIC_REF.replace(self.BUCKET_PROJECT, project) - resource = { - 'topic': self.TOPIC_REF, - } - notification = klass.from_api_repr(resource, bucket=bucket) - self.assertEqual(notification.topic_project, project) - - def test_from_api_repr_above_max_length_fail(self): - klass = self._get_target_class() - client = self._make_client() - project = 'a' * 31 - bucket = self._make_bucket(client) - self.TOPIC_REF = self.TOPIC_REF.replace(self.BUCKET_PROJECT, project) - resource = { - 'topic': self.TOPIC_REF, - } - with self.assertRaises(ValueError): - klass.from_api_repr(resource, bucket=bucket) - - def test_from_api_repr_capital_letters_fail(self): - klass = self._get_target_class() - client = self._make_client() - project = 'aaaAaa' - bucket = self._make_bucket(client) - self.TOPIC_REF = self.TOPIC_REF.replace(self.BUCKET_PROJECT, project) - resource = { - 'topic': self.TOPIC_REF, - } - with self.assertRaises(ValueError): - klass.from_api_repr(resource, bucket=bucket) - - def test_from_api_repr_leading_digit_fail(self): - klass = self._get_target_class() - client = self._make_client() - project = '1' + 'a' * 5 - bucket = self._make_bucket(client) - self.TOPIC_REF = self.TOPIC_REF.replace(self.BUCKET_PROJECT, project) - resource = { - 'topic': self.TOPIC_REF, - } - with self.assertRaises(ValueError): - klass.from_api_repr(resource, bucket=bucket) - - def test_from_api_repr_leading_hyphen_fail(self): - klass = self._get_target_class() - client = self._make_client() - project = '-' + 'a' * 5 - bucket = self._make_bucket(client) - self.TOPIC_REF = self.TOPIC_REF.replace(self.BUCKET_PROJECT, project) - resource = { - 'topic': self.TOPIC_REF, - } - with self.assertRaises(ValueError): - klass.from_api_repr(resource, bucket=bucket) - - def test_from_api_repr_trailing_hyphen_fail(self): - klass = self._get_target_class() - client = self._make_client() - project = 'a' * 5 + '-' - bucket = self._make_bucket(client) - self.TOPIC_REF = self.TOPIC_REF.replace(self.BUCKET_PROJECT, project) - resource = { - 'topic': self.TOPIC_REF, - } - with self.assertRaises(ValueError): - klass.from_api_repr(resource, bucket=bucket) - - def test_from_api_repr_hyphens_success(self): - klass = self._get_target_class() - client = self._make_client() - project = 'a' + '-' * 28 + 'a' - bucket = self._make_bucket(client) - self.TOPIC_REF = self.TOPIC_REF.replace(self.BUCKET_PROJECT, project) - resource = { - 'topic': self.TOPIC_REF, - } - notification = klass.from_api_repr(resource, bucket=bucket) - self.assertEqual(notification.topic_project, project) - - def test_from_api_repr_letters_success(self): - klass = self._get_target_class() - client = self._make_client() - project = 'abcdefghijklmnopqrstuvwxyz' - bucket = self._make_bucket(client) - self.TOPIC_REF = self.TOPIC_REF.replace(self.BUCKET_PROJECT, project) - resource = { - 'topic': self.TOPIC_REF, - } - notification = klass.from_api_repr(resource, bucket=bucket) - self.assertEqual(notification.topic_project, project) - - def test_from_api_repr_digits_success(self): - klass = self._get_target_class() - client = self._make_client() - project = 'z0123456789' - bucket = self._make_bucket(client) - self.TOPIC_REF = self.TOPIC_REF.replace(self.BUCKET_PROJECT, project) - resource = { - 'topic': self.TOPIC_REF, - } - notification = klass.from_api_repr(resource, bucket=bucket) - self.assertEqual(notification.topic_project, project) - - def test_from_api_repr_assortment_success(self): - klass = self._get_target_class() - client = self._make_client() - project = 'a-bcdefghijklmn12opqrstuv0wxyz' - bucket = self._make_bucket(client) - self.TOPIC_REF = self.TOPIC_REF.replace(self.BUCKET_PROJECT, project) - resource = { - 'topic': self.TOPIC_REF, - } - notification = klass.from_api_repr(resource, bucket=bucket) - self.assertEqual(notification.topic_project, project) - def test_from_api_repr_invalid_topic(self): klass = self._get_target_class() client = self._make_client() @@ -306,6 +137,7 @@ def test_from_api_repr_invalid_topic(self): resource = { 'topic': '@#$%', } + with self.assertRaises(ValueError): klass.from_api_repr(resource, bucket=bucket) @@ -655,3 +487,67 @@ def test_delete_hit(self): path=self.NOTIFICATION_PATH, query_params={'userProject': USER_PROJECT}, ) + + +class Test__parse_topic_path(unittest.TestCase): + + @staticmethod + def _call_fut(*args, **kwargs): + from google.cloud.storage import notification + + return notification._parse_topic_path(*args, **kwargs) + + @staticmethod + def _make_topic_path(project, topic_name): + from google.cloud.storage import notification + + return notification._TOPIC_REF_FMT.format(project, topic_name) + + def test_project_name_too_long(self): + project = 'a' * 31 + topic_path = self._make_topic_path(project, 'topic-name') + with self.assertRaises(ValueError): + self._call_fut(topic_path) + + def test_project_name_uppercase(self): + project = 'aaaAaa' + topic_path = self._make_topic_path(project, 'topic-name') + with self.assertRaises(ValueError): + self._call_fut(topic_path) + + def test_leading_digit(self): + project = '1aaaaa' + topic_path = self._make_topic_path(project, 'topic-name') + with self.assertRaises(ValueError): + self._call_fut(topic_path) + + def test_leading_hyphen(self): + project = '-aaaaa' + topic_path = self._make_topic_path(project, 'topic-name') + with self.assertRaises(ValueError): + self._call_fut(topic_path) + + def test_trailing_hyphen(self): + project = 'aaaaa-' + topic_path = self._make_topic_path(project, 'topic-name') + with self.assertRaises(ValueError): + self._call_fut(topic_path) + + def test_invalid_format(self): + topic_path = '@#$%' + with self.assertRaises(ValueError): + self._call_fut(topic_path) + + def test_success(self): + topic_name = 'tah-pic-nehm' + project_choices = ( + 'a' * 30, # Max length. + 'a-b--c---d', # Valid hyphen usage. + 'abcdefghijklmnopqrstuvwxyz', # Valid letters. + 'z0123456789', # Valid digits (non-leading). + 'a-bcdefghijklmn12opqrstuv0wxyz', + ) + for project in project_choices: + topic_path = self._make_topic_path(project, topic_name) + result = self._call_fut(topic_path) + self.assertEqual(result, (topic_name, project))