From 1539d31d7204f90e6855c10783b4de3726fc624c Mon Sep 17 00:00:00 2001 From: Anthonios Partheniou Date: Wed, 16 Feb 2022 01:01:44 +0000 Subject: [PATCH 1/5] fix: additional logic to mitigate collisions with reserved terms --- proto/message.py | 62 +++++++++++++++++++++++-- tests/test_fields_mitigate_collision.py | 34 +++++++++++++- 2 files changed, 90 insertions(+), 6 deletions(-) diff --git a/proto/message.py b/proto/message.py index 7293fdb5..4e806eda 100644 --- a/proto/message.py +++ b/proto/message.py @@ -530,7 +530,7 @@ def __init__( # Underscores may be appended to field names # that collide with python or proto-plus keywords. # In case a key only exists with a `_` suffix, coerce the key - # to include the `_` suffix. Is not possible to + # to include the `_` suffix. It's not possible to # natively define the same field with a trailing underscore in protobuf. # See related issue # https://github.com/googleapis/python-api-core/issues/227 @@ -545,7 +545,27 @@ def __init__( "Unknown field for {}: {}".format(self.__class__.__name__, key) ) - pb_value = marshal.to_proto(pb_type, value) + try: + pb_value = marshal.to_proto(pb_type, value) + except ValueError: + # Underscores may be appended to field names + # that collide with python or proto-plus keywords. + # In case a key only exists with a `_` suffix, coerce the key + # to include the `_` suffix. It's not possible to + # natively define the same field with a trailing underscore in protobuf. + # See related issue + # https://github.com/googleapis/python-api-core/issues/227 + if isinstance(value, dict): + keys_to_update = [] + for item in value: + if not hasattr(pb_type, item) and hasattr(pb_type, f"{item}_"): + keys_to_update.append(item) + for item in keys_to_update: + value[f"{item}_"] = value[item] + del value[item] + + pb_value = marshal.to_proto(pb_type, value) + if pb_value is not None: params[key] = pb_value @@ -662,7 +682,24 @@ def __getattr__(self, key): more details. """ try: - pb_type = self._meta.fields[key].pb_type + try: + pb_type = self._meta.fields[key].pb_type + except KeyError: + # Underscores may be appended to field names + # that collide with python or proto-plus keywords. + # In case a key only exists with a `_` suffix, coerce the key + # to include the `_` suffix. It's not possible to + # natively define the same field with a trailing underscore in protobuf. + # See related issue + # https://github.com/googleapis/python-api-core/issues/227 + if f"{key}_" in self._meta.fields: + key = f"{key}_" + pb_type = self._meta.fields[key].pb_type + else: + raise KeyError( + "Unknown field for {}: {}".format(self.__class__.__name__, key) + ) + pb_value = getattr(self._pb, key) marshal = self._meta.marshal return marshal.to_python(pb_type, pb_value, absent=key not in self) @@ -685,7 +722,24 @@ def __setattr__(self, key, value): if key[0] == "_": return super().__setattr__(key, value) marshal = self._meta.marshal - pb_type = self._meta.fields[key].pb_type + try: + pb_type = self._meta.fields[key].pb_type + except KeyError: + # Underscores may be appended to field names + # that collide with python or proto-plus keywords. + # In case a key only exists with a `_` suffix, coerce the key + # to include the `_` suffix. It's not possible to + # natively define the same field with a trailing underscore in protobuf. + # See related issue + # https://github.com/googleapis/python-api-core/issues/227 + if f"{key}_" in self._meta.fields: + key = f"{key}_" + pb_type = self._meta.fields[key].pb_type + else: + raise KeyError( + "Unknown field for {}: {}".format(self.__class__.__name__, key) + ) + pb_value = marshal.to_proto(pb_type, value) # Clear the existing field. diff --git a/tests/test_fields_mitigate_collision.py b/tests/test_fields_mitigate_collision.py index 0dca71df..9b4ea0de 100644 --- a/tests/test_fields_mitigate_collision.py +++ b/tests/test_fields_mitigate_collision.py @@ -13,12 +13,12 @@ # limitations under the License. import proto - +import pytest # Underscores may be appended to field names # that collide with python or proto-plus keywords. # In case a key only exists with a `_` suffix, coerce the key -# to include the `_` suffix. Is not possible to +# to include the `_` suffix. It's not possible to # natively define the same field with a trailing underscore in protobuf. # See related issue # https://github.com/googleapis/python-api-core/issues/227 @@ -27,6 +27,9 @@ class TestMessage(proto.Message): spam_ = proto.Field(proto.STRING, number=1) eggs = proto.Field(proto.STRING, number=2) + class TextStream(proto.Message): + text_stream = proto.Field(TestMessage, number=1) + obj = TestMessage(spam_="has_spam") obj.eggs = "has_eggs" assert obj.spam_ == "has_spam" @@ -34,3 +37,30 @@ class TestMessage(proto.Message): # Test that `spam` is coerced to `spam_` modified_obj = TestMessage({"spam": "has_spam", "eggs": "has_eggs"}) assert modified_obj.spam_ == "has_spam" + + # Test __setattr__ and __getattr___ + modified_obj.__setattr__("spam", "no_spam") + modified_obj.__getattr__("spam") == "no_spam" + + modified_obj.__setattr__("spam_", "yes_spam") + modified_obj.__getattr__("spam_") == "yes_spam" + + # Try nested values + modified_obj = TextStream( + text_stream=TestMessage({"spam": "has_spam", "eggs": "has_eggs"}) + ) + assert modified_obj.text_stream.spam_ == "has_spam" + + # Test __setattr__ and __getattr___ + modified_obj.text_stream.__setattr__("spam", "no_spam") + modified_obj.text_stream.__getattr__("spam") == "no_spam" + + modified_obj.text_stream.__setattr__("spam_", "yes_spam") + modified_obj.text_stream.__getattr__("spam_") == "yes_spam" + + with pytest.raises(KeyError): + modified_obj.text_stream.__setattr__("key_does_not_exist", "n/a") + + # Try using dict + modified_obj = TextStream(text_stream={"spam": "has_spam", "eggs": "has_eggs"}) + assert modified_obj.text_stream.spam_ == "has_spam" From 28fd6ccccb58204ea642caec5c77b042e6b338ed Mon Sep 17 00:00:00 2001 From: Anthonios Partheniou Date: Thu, 17 Feb 2022 00:30:13 +0000 Subject: [PATCH 2/5] address review feedback --- proto/message.py | 113 +++++++++++------------- tests/test_fields_mitigate_collision.py | 39 +++++--- 2 files changed, 81 insertions(+), 71 deletions(-) diff --git a/proto/message.py b/proto/message.py index 4e806eda..8231fc7b 100644 --- a/proto/message.py +++ b/proto/message.py @@ -524,26 +524,14 @@ def __init__( # coerced. marshal = self._meta.marshal for key, value in mapping.items(): - try: - pb_type = self._meta.fields[key].pb_type - except KeyError: - # Underscores may be appended to field names - # that collide with python or proto-plus keywords. - # In case a key only exists with a `_` suffix, coerce the key - # to include the `_` suffix. It's not possible to - # natively define the same field with a trailing underscore in protobuf. - # See related issue - # https://github.com/googleapis/python-api-core/issues/227 - if f"{key}_" in self._meta.fields: - key = f"{key}_" - pb_type = self._meta.fields[key].pb_type - else: - if ignore_unknown_fields: - continue + (key, pb_type) = self._get_pb_type_from_key(key) + if pb_type is None: + if ignore_unknown_fields: + continue - raise ValueError( - "Unknown field for {}: {}".format(self.__class__.__name__, key) - ) + raise ValueError( + "Unknown field for {}: {}".format(self.__class__.__name__, key) + ) try: pb_value = marshal.to_proto(pb_type, value) @@ -556,13 +544,13 @@ def __init__( # See related issue # https://github.com/googleapis/python-api-core/issues/227 if isinstance(value, dict): - keys_to_update = [] - for item in value: - if not hasattr(pb_type, item) and hasattr(pb_type, f"{item}_"): - keys_to_update.append(item) + keys_to_update = [ + item + for item in value + if not hasattr(pb_type, item) and hasattr(pb_type, f"{item}_") + ] for item in keys_to_update: - value[f"{item}_"] = value[item] - del value[item] + value[f"{item}_"] = value.pop(item) pb_value = marshal.to_proto(pb_type, value) @@ -572,6 +560,37 @@ def __init__( # Create the internal protocol buffer. super().__setattr__("_pb", self._meta.pb(**params)) + def _get_pb_type_from_key(self, key): + """Given a key, return the corresponding pb_type. + + Args: + key(str): The name of the field. + + Returns: + A tuple containing a key and pb_type. The pb_type will be + the composite type of the field, or the primitive type if a primitive. + If no corresponding field exists, return None. + """ + + pb_type = None + + try: + pb_type = self._meta.fields[key].pb_type + except KeyError: + # Underscores may be appended to field names + # that collide with python or proto-plus keywords. + # In case a key only exists with a `_` suffix, coerce the key + # to include the `_` suffix. It's not possible to + # natively define the same field with a trailing underscore in protobuf. + # See related issue + # https://github.com/googleapis/python-api-core/issues/227 + if f"{key}_" in self._meta.fields: + key = f"{key}_" + pb_type = self._meta.fields[key].pb_type + + return (key, pb_type) + + def __dir__(self): desc = type(self).pb().DESCRIPTOR names = {f_name for f_name in self._meta.fields.keys()} @@ -682,23 +701,11 @@ def __getattr__(self, key): more details. """ try: - try: - pb_type = self._meta.fields[key].pb_type - except KeyError: - # Underscores may be appended to field names - # that collide with python or proto-plus keywords. - # In case a key only exists with a `_` suffix, coerce the key - # to include the `_` suffix. It's not possible to - # natively define the same field with a trailing underscore in protobuf. - # See related issue - # https://github.com/googleapis/python-api-core/issues/227 - if f"{key}_" in self._meta.fields: - key = f"{key}_" - pb_type = self._meta.fields[key].pb_type - else: - raise KeyError( - "Unknown field for {}: {}".format(self.__class__.__name__, key) - ) + (key, pb_type) = self._get_pb_type_from_key(key) + if pb_type is None: + raise AttributeError( + "Unknown field for {}: {}".format(self.__class__.__name__, key) + ) pb_value = getattr(self._pb, key) marshal = self._meta.marshal @@ -722,23 +729,11 @@ def __setattr__(self, key, value): if key[0] == "_": return super().__setattr__(key, value) marshal = self._meta.marshal - try: - pb_type = self._meta.fields[key].pb_type - except KeyError: - # Underscores may be appended to field names - # that collide with python or proto-plus keywords. - # In case a key only exists with a `_` suffix, coerce the key - # to include the `_` suffix. It's not possible to - # natively define the same field with a trailing underscore in protobuf. - # See related issue - # https://github.com/googleapis/python-api-core/issues/227 - if f"{key}_" in self._meta.fields: - key = f"{key}_" - pb_type = self._meta.fields[key].pb_type - else: - raise KeyError( - "Unknown field for {}: {}".format(self.__class__.__name__, key) - ) + (key, pb_type) = self._get_pb_type_from_key(key) + if pb_type is None: + raise AttributeError( + "Unknown field for {}: {}".format(self.__class__.__name__, key) + ) pb_value = marshal.to_proto(pb_type, value) diff --git a/tests/test_fields_mitigate_collision.py b/tests/test_fields_mitigate_collision.py index 9b4ea0de..f07f3358 100644 --- a/tests/test_fields_mitigate_collision.py +++ b/tests/test_fields_mitigate_collision.py @@ -38,12 +38,18 @@ class TextStream(proto.Message): modified_obj = TestMessage({"spam": "has_spam", "eggs": "has_eggs"}) assert modified_obj.spam_ == "has_spam" - # Test __setattr__ and __getattr___ - modified_obj.__setattr__("spam", "no_spam") - modified_obj.__getattr__("spam") == "no_spam" + # Test get and set + modified_obj.spam = "no_spam" + assert modified_obj.spam == "no_spam" - modified_obj.__setattr__("spam_", "yes_spam") - modified_obj.__getattr__("spam_") == "yes_spam" + modified_obj.spam_ = "yes_spam" + assert modified_obj.spam_ == "yes_spam" + + modified_obj.spam = "maybe_spam" + assert modified_obj.spam_ == "maybe_spam" + + modified_obj.spam_ = "maybe_not_spam" + assert modified_obj.spam == "maybe_not_spam" # Try nested values modified_obj = TextStream( @@ -51,15 +57,24 @@ class TextStream(proto.Message): ) assert modified_obj.text_stream.spam_ == "has_spam" - # Test __setattr__ and __getattr___ - modified_obj.text_stream.__setattr__("spam", "no_spam") - modified_obj.text_stream.__getattr__("spam") == "no_spam" + # Test get and set for nested values + modified_obj.text_stream.spam = "no_spam" + assert modified_obj.text_stream.spam == "no_spam" + + modified_obj.text_stream.spam_ = "yes_spam" + assert modified_obj.text_stream.spam_ == "yes_spam" + + modified_obj.text_stream.spam = "maybe_spam" + assert modified_obj.text_stream.spam_ == "maybe_spam" + + modified_obj.text_stream.spam_ = "maybe_not_spam" + assert modified_obj.text_stream.spam == "maybe_not_spam" - modified_obj.text_stream.__setattr__("spam_", "yes_spam") - modified_obj.text_stream.__getattr__("spam_") == "yes_spam" + with pytest.raises(AttributeError): + modified_obj.text_stream.attribute_does_not_exist == "n/a" - with pytest.raises(KeyError): - modified_obj.text_stream.__setattr__("key_does_not_exist", "n/a") + with pytest.raises(AttributeError): + modified_obj.text_stream.attribute_does_not_exist = "n/a" # Try using dict modified_obj = TextStream(text_stream={"spam": "has_spam", "eggs": "has_eggs"}) From 65cfdd3c80fa8cdb2ebdc1180d88cca49e2a4f31 Mon Sep 17 00:00:00 2001 From: Anthonios Partheniou Date: Thu, 17 Feb 2022 00:32:53 +0000 Subject: [PATCH 3/5] remove line break --- proto/message.py | 1 - 1 file changed, 1 deletion(-) diff --git a/proto/message.py b/proto/message.py index 8231fc7b..42c0970e 100644 --- a/proto/message.py +++ b/proto/message.py @@ -590,7 +590,6 @@ def _get_pb_type_from_key(self, key): return (key, pb_type) - def __dir__(self): desc = type(self).pb().DESCRIPTOR names = {f_name for f_name in self._meta.fields.keys()} From 78e3a91d9339e3474519829df0de87ef8c621ba5 Mon Sep 17 00:00:00 2001 From: Anthonios Partheniou Date: Thu, 17 Feb 2022 00:38:35 +0000 Subject: [PATCH 4/5] remove redundant code --- proto/message.py | 20 ++++++++------------ 1 file changed, 8 insertions(+), 12 deletions(-) diff --git a/proto/message.py b/proto/message.py index 42c0970e..881300a0 100644 --- a/proto/message.py +++ b/proto/message.py @@ -699,18 +699,14 @@ def __getattr__(self, key): their Python equivalents. See the ``marshal`` module for more details. """ - try: - (key, pb_type) = self._get_pb_type_from_key(key) - if pb_type is None: - raise AttributeError( - "Unknown field for {}: {}".format(self.__class__.__name__, key) - ) - - pb_value = getattr(self._pb, key) - marshal = self._meta.marshal - return marshal.to_python(pb_type, pb_value, absent=key not in self) - except KeyError as ex: - raise AttributeError(str(ex)) + (key, pb_type) = self._get_pb_type_from_key(key) + if pb_type is None: + raise AttributeError( + "Unknown field for {}: {}".format(self.__class__.__name__, key) + ) + pb_value = getattr(self._pb, key) + marshal = self._meta.marshal + return marshal.to_python(pb_type, pb_value, absent=key not in self) def __ne__(self, other): """Return True if the messages are unequal, False otherwise.""" From 331c80916d793d5fd757319f72417bf3b5df5fbf Mon Sep 17 00:00:00 2001 From: Anthonios Partheniou Date: Fri, 18 Feb 2022 00:27:49 +0000 Subject: [PATCH 5/5] add assert --- tests/test_fields_mitigate_collision.py | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/tests/test_fields_mitigate_collision.py b/tests/test_fields_mitigate_collision.py index f07f3358..117af48a 100644 --- a/tests/test_fields_mitigate_collision.py +++ b/tests/test_fields_mitigate_collision.py @@ -71,7 +71,7 @@ class TextStream(proto.Message): assert modified_obj.text_stream.spam == "maybe_not_spam" with pytest.raises(AttributeError): - modified_obj.text_stream.attribute_does_not_exist == "n/a" + assert modified_obj.text_stream.attribute_does_not_exist == "n/a" with pytest.raises(AttributeError): modified_obj.text_stream.attribute_does_not_exist = "n/a"