From 3a06ee3df5bd256a1b746aa675d369913c90407c Mon Sep 17 00:00:00 2001 From: Kieran Brantner-Magee Date: Sun, 27 Sep 2020 21:58:00 -0700 Subject: [PATCH 1/4] make amqp_message properties read-only in order to allow time for cross-sdk unification on this feature. --- sdk/servicebus/azure-servicebus/CHANGELOG.md | 1 + .../azure/servicebus/_common/message.py | 66 +++++++++++-------- .../azure-servicebus/tests/test_queues.py | 49 ++++++++------ 3 files changed, 69 insertions(+), 47 deletions(-) diff --git a/sdk/servicebus/azure-servicebus/CHANGELOG.md b/sdk/servicebus/azure-servicebus/CHANGELOG.md index a3415ad1f9da..885fe711ed97 100644 --- a/sdk/servicebus/azure-servicebus/CHANGELOG.md +++ b/sdk/servicebus/azure-servicebus/CHANGELOG.md @@ -4,6 +4,7 @@ **Breaking Changes** * Passing any type other than `ReceiveMode` as parameter `receive_mode` now throws a `TypeError` instead of `AttributeError`. +* `AMQPMessage` (`Message.amqp_message`) properties are now read-only (will not be reflected in the underlying message) pending cross-language sdk design consensus. ## 7.0.0b6 (2020-09-10) diff --git a/sdk/servicebus/azure-servicebus/azure/servicebus/_common/message.py b/sdk/servicebus/azure-servicebus/azure/servicebus/_common/message.py index 856bd31638fe..55616d715ea8 100644 --- a/sdk/servicebus/azure-servicebus/azure/servicebus/_common/message.py +++ b/sdk/servicebus/azure-servicebus/azure/servicebus/_common/message.py @@ -9,6 +9,7 @@ import uuid import functools import logging +import copy from typing import Optional, List, Union, Iterable, TYPE_CHECKING, Callable, Any import uamqp.message @@ -127,7 +128,7 @@ def __init__(self, body, **kwargs): self.partition_key = kwargs.pop("partition_key", None) self.via_partition_key = kwargs.pop("via_partition_key", None) # If message is the full message, amqp_message is the "public facing interface" for what we expose. - self.amqp_message = AMQPMessage(self.message) + self.amqp_message = AMQPMessage(self.message) # type: AMQPMessage def __str__(self): return str(self.message) @@ -1083,7 +1084,7 @@ def renew_lock(self): class AMQPMessage(object): """ - The internal AMQP message that this ServiceBusMessage represents. + The internal AMQP message that this ServiceBusMessage represents. Is read-only. :param properties: Properties to add to the message. :type properties: ~uamqp.message.MessageProperties @@ -1108,48 +1109,59 @@ def __init__(self, message): @property def properties(self): - return self._message.properties - - @properties.setter - def properties(self, value): - self._message.properties = value + return uamqp.message.MessageProperties(message_id = self._message.properties.message_id, + user_id = self._message.properties.user_id, + to = self._message.properties.to, + subject = self._message.properties.subject, + reply_to = self._message.properties.reply_to, + correlation_id = self._message.properties.correlation_id, + content_type = self._message.properties.content_type, + content_encoding = self._message.properties.content_encoding + ) + + # NOTE: These are disabled pending arch. design and cross-sdk consensus on + # how we will expose sendability of amqp focused messages. To undo, uncomment and remove deepcopies/workarounds. + # + #@properties.setter + #def properties(self, value): + # self._message.properties = value @property def application_properties(self): - return self._message.application_properties + return copy.deepcopy(self._message.application_properties) - @application_properties.setter - def application_properties(self, value): - self._message.application_properties = value + #@application_properties.setter + #def application_properties(self, value): + # self._message.application_properties = value @property def annotations(self): - return self._message.annotations + return copy.deepcopy(self._message.annotations) - @annotations.setter - def annotations(self, value): - self._message.annotations = value + #@annotations.setter + #def annotations(self, value): + # self._message.annotations = value @property def delivery_annotations(self): - return self._message.delivery_annotations + return copy.deepcopy(self._message.delivery_annotations) - @delivery_annotations.setter - def delivery_annotations(self, value): - self._message.delivery_annotations = value + #@delivery_annotations.setter + #def delivery_annotations(self, value): + # self._message.delivery_annotations = value @property def header(self): - return self._message.header + return uamqp.message.MessageHeader(header=self._message.header) - @header.setter - def header(self, value): - self._message.header = value + #@header.setter + #def header(self, value): + # self._message.header = value @property def footer(self): - return self._message.footer + return copy.deepcopy(self._message.footer) - @footer.setter - def footer(self, value): - self._message.footer = value + #@footer.setter + #def footer(self, value): + # self._message.footer = value diff --git a/sdk/servicebus/azure-servicebus/tests/test_queues.py b/sdk/servicebus/azure-servicebus/tests/test_queues.py index c84909654b64..a5c01c337db6 100644 --- a/sdk/servicebus/azure-servicebus/tests/test_queues.py +++ b/sdk/servicebus/azure-servicebus/tests/test_queues.py @@ -1873,20 +1873,21 @@ def test_message_inner_amqp_properties(self, servicebus_namespace_connection_str message = Message("body") - with pytest.raises(TypeError): + with pytest.raises(AttributeError): # Note: If this is made read-writeable, this would be TypeError message.amqp_message.properties = {"properties":1} - message.amqp_message.properties.subject = "subject" - - message.amqp_message.application_properties = {b"application_properties":1} - - message.amqp_message.annotations = {b"annotations":2} - message.amqp_message.delivery_annotations = {b"delivery_annotations":3} - - with pytest.raises(TypeError): - message.amqp_message.header = {"header":4} - message.amqp_message.header.priority = 5 - - message.amqp_message.footer = {b"footer":6} + # NOTE: These are disabled pending cross-language-sdk consensus on sendability/writeability. + # message.amqp_message.properties.subject = "subject" + # + # message.amqp_message.application_properties = {b"application_properties":1} + # + # message.amqp_message.annotations = {b"annotations":2} + # message.amqp_message.delivery_annotations = {b"delivery_annotations":3} + # + # with pytest.raises(TypeError): + # message.amqp_message.header = {"header":4} + # message.amqp_message.header.priority = 5 + # + # message.amqp_message.footer = {b"footer":6} with ServiceBusClient.from_connection_string( servicebus_namespace_connection_string, logging_enable=False) as sb_client: @@ -1895,10 +1896,18 @@ def test_message_inner_amqp_properties(self, servicebus_namespace_connection_str sender.send_messages(message) with sb_client.get_queue_receiver(servicebus_queue.name, max_wait_time=5) as receiver: message = receiver.receive_messages()[0] - assert message.amqp_message.properties.subject == b"subject" - assert message.amqp_message.application_properties[b"application_properties"] == 1 - assert message.amqp_message.annotations[b"annotations"] == 2 - # delivery_annotations and footer disabled pending uamqp bug https://github.com/Azure/azure-uamqp-python/issues/169 - #assert message.amqp_message.delivery_annotations[b"delivery_annotations"] == 3 - assert message.amqp_message.header.priority == 5 - #assert message.amqp_message.footer[b"footer"] == 6 \ No newline at end of file + assert message.amqp_message.application_properties == None \ + and message.amqp_message.annotations != None \ + and message.amqp_message.delivery_annotations != None \ + and message.amqp_message.footer == None \ + and message.amqp_message.properties != None \ + and message.amqp_message.header != None + # NOTE: These are disabled pending cross-language-sdk consensus on sendability/writeability. + # + # assert message.amqp_message.properties.subject == b"subject" + # assert message.amqp_message.application_properties[b"application_properties"] == 1 + # assert message.amqp_message.annotations[b"annotations"] == 2 + # # delivery_annotations and footer disabled pending uamqp bug https://github.com/Azure/azure-uamqp-python/issues/169 + # #assert message.amqp_message.delivery_annotations[b"delivery_annotations"] == 3 + # assert message.amqp_message.header.priority == 5 + # #assert message.amqp_message.footer[b"footer"] == 6 \ No newline at end of file From f22a054a0cbd226d7fe61cd66cae5aa21e97af01 Mon Sep 17 00:00:00 2001 From: Kieran Brantner-Magee Date: Wed, 30 Sep 2020 21:11:07 -0700 Subject: [PATCH 2/4] Fix pylint failures --- .../azure/servicebus/_common/message.py | 16 ++++++++-------- 1 file changed, 8 insertions(+), 8 deletions(-) diff --git a/sdk/servicebus/azure-servicebus/azure/servicebus/_common/message.py b/sdk/servicebus/azure-servicebus/azure/servicebus/_common/message.py index 55616d715ea8..bb8bfc6057c9 100644 --- a/sdk/servicebus/azure-servicebus/azure/servicebus/_common/message.py +++ b/sdk/servicebus/azure-servicebus/azure/servicebus/_common/message.py @@ -1109,14 +1109,14 @@ def __init__(self, message): @property def properties(self): - return uamqp.message.MessageProperties(message_id = self._message.properties.message_id, - user_id = self._message.properties.user_id, - to = self._message.properties.to, - subject = self._message.properties.subject, - reply_to = self._message.properties.reply_to, - correlation_id = self._message.properties.correlation_id, - content_type = self._message.properties.content_type, - content_encoding = self._message.properties.content_encoding + return uamqp.message.MessageProperties(message_id=self._message.properties.message_id, + user_id=self._message.properties.user_id, + to=self._message.properties.to, + subject=self._message.properties.subject, + reply_to=self._message.properties.reply_to, + correlation_id=self._message.properties.correlation_id, + content_type=self._message.properties.content_type, + content_encoding=self._message.properties.content_encoding ) # NOTE: These are disabled pending arch. design and cross-sdk consensus on From cc72e357de921991e4e9c3b5e4f524e52ab1eccd Mon Sep 17 00:00:00 2001 From: KieranBrantnerMagee Date: Thu, 1 Oct 2020 17:43:37 -0700 Subject: [PATCH 3/4] Update sdk/servicebus/azure-servicebus/CHANGELOG.md clarify AMQPMessage changelog entry verbiage --- sdk/servicebus/azure-servicebus/CHANGELOG.md | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/sdk/servicebus/azure-servicebus/CHANGELOG.md b/sdk/servicebus/azure-servicebus/CHANGELOG.md index 885fe711ed97..fd733843ca2c 100644 --- a/sdk/servicebus/azure-servicebus/CHANGELOG.md +++ b/sdk/servicebus/azure-servicebus/CHANGELOG.md @@ -4,7 +4,7 @@ **Breaking Changes** * Passing any type other than `ReceiveMode` as parameter `receive_mode` now throws a `TypeError` instead of `AttributeError`. -* `AMQPMessage` (`Message.amqp_message`) properties are now read-only (will not be reflected in the underlying message) pending cross-language sdk design consensus. +* `AMQPMessage` (`Message.amqp_message`) properties are now read-only, changes of these properties would not be reflected in the underlying message. This may be subject to change before GA. ## 7.0.0b6 (2020-09-10) From 34a116a049f9ebf8a834eb6a30790439915e5728 Mon Sep 17 00:00:00 2001 From: Kieran Brantner-Magee Date: Thu, 1 Oct 2020 18:19:43 -0700 Subject: [PATCH 4/4] Move AMQPMessage docstrings into individual properties instead of a param block (as it does not take params) Also add typehints to the properties. --- .../azure/servicebus/_common/message.py | 56 +++++++++++++------ 1 file changed, 39 insertions(+), 17 deletions(-) diff --git a/sdk/servicebus/azure-servicebus/azure/servicebus/_common/message.py b/sdk/servicebus/azure-servicebus/azure/servicebus/_common/message.py index bb8bfc6057c9..4b1b6eee331f 100644 --- a/sdk/servicebus/azure-servicebus/azure/servicebus/_common/message.py +++ b/sdk/servicebus/azure-servicebus/azure/servicebus/_common/message.py @@ -1085,23 +1085,6 @@ def renew_lock(self): class AMQPMessage(object): """ The internal AMQP message that this ServiceBusMessage represents. Is read-only. - - :param properties: Properties to add to the message. - :type properties: ~uamqp.message.MessageProperties - :param application_properties: Service specific application properties. - :type application_properties: dict - :param annotations: Service specific message annotations. Keys in the dictionary - must be `uamqp.types.AMQPSymbol` or `uamqp.types.AMQPuLong`. - :type annotations: dict - :param delivery_annotations: Delivery-specific non-standard properties at the head of the message. - Delivery annotations convey information from the sending peer to the receiving peer. - Keys in the dictionary must be `uamqp.types.AMQPSymbol` or `uamqp.types.AMQPuLong`. - :type delivery_annotations: dict - :param header: The message header. - :type header: ~uamqp.message.MessageHeader - :param footer: The message footer. - :type footer: dict - """ def __init__(self, message): # type: (uamqp.Message) -> None @@ -1109,6 +1092,12 @@ def __init__(self, message): @property def properties(self): + # type: () -> uamqp.message.MessageProperties + """ + Properties to add to the message. + + :rtype: ~uamqp.message.MessageProperties + """ return uamqp.message.MessageProperties(message_id=self._message.properties.message_id, user_id=self._message.properties.user_id, to=self._message.properties.to, @@ -1128,6 +1117,12 @@ def properties(self): @property def application_properties(self): + # type: () -> dict + """ + Service specific application properties. + + :rtype: dict + """ return copy.deepcopy(self._message.application_properties) #@application_properties.setter @@ -1136,6 +1131,13 @@ def application_properties(self): @property def annotations(self): + # type: () -> dict + """ + Service specific message annotations. Keys in the dictionary + must be `uamqp.types.AMQPSymbol` or `uamqp.types.AMQPuLong`. + + :rtype: dict + """ return copy.deepcopy(self._message.annotations) #@annotations.setter @@ -1144,6 +1146,14 @@ def annotations(self): @property def delivery_annotations(self): + # type: () -> dict + """ + Delivery-specific non-standard properties at the head of the message. + Delivery annotations convey information from the sending peer to the receiving peer. + Keys in the dictionary must be `uamqp.types.AMQPSymbol` or `uamqp.types.AMQPuLong`. + + :rtype: dict + """ return copy.deepcopy(self._message.delivery_annotations) #@delivery_annotations.setter @@ -1152,6 +1162,12 @@ def delivery_annotations(self): @property def header(self): + # type: () -> uamqp.message.MessageHeader + """ + The message header. + + :rtype: ~uamqp.message.MessageHeader + """ return uamqp.message.MessageHeader(header=self._message.header) #@header.setter @@ -1160,6 +1176,12 @@ def header(self): @property def footer(self): + # type: () -> dict + """ + The message footer. + + :rtype: dict + """ return copy.deepcopy(self._message.footer) #@footer.setter